Skip to content

[TNL-5947] Fix: Don't show visibility settings on library pages. #14073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
itsjeyd wants to merge 6 commits into from
Closed

[TNL-5947] Fix: Don't show visibility settings on library pages. #14073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7b0c8f6
Fix: Don't show visibility settings on library pages.
itsjeyd Nov 30, 2016
c777d83
Improve variable names.
itsjeyd Nov 30, 2016
a5f157f
Rename "container_child_preview" to "library_container_child_preview"
itsjeyd Nov 30, 2016
f0ceab2
Alternative approach: Pass "can_edit_visibility" info explicitly when
itsjeyd Nov 30, 2016
a2ae07b
XBlockView.render: Make sure "options" is defined before using it in …
itsjeyd Dec 1, 2016
a239786
Fix quality violations.
itsjeyd Dec 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion cms/djangoapps/contentstore/views/item.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from __future__ import absolute_import

import hashlib
import json
import logging
from collections import OrderedDict
from datetime import datetime
Expand Down Expand Up @@ -268,7 +269,9 @@ def xblock_view_handler(request, usage_key_string, view_name):
if 'application/json' in accept_header:
store = modulestore()
xblock = store.get_item(usage_key)
container_views = ['container_preview', 'reorderable_container_child_preview', 'container_child_preview']
container_views = [
'container_preview', 'reorderable_container_child_preview', 'library_container_child_preview'
]

# wrap the generated fragment in the xmodule_editor div so that the javascript
# can bind to it correctly
Expand Down Expand Up @@ -324,6 +327,7 @@ def xblock_view_handler(request, usage_key_string, view_name):
)

force_render = request.GET.get('force_render', None)
can_edit_visibility = request.GET.get('can_edit_visibility', None)

# Set up the context to be passed to each XBlock's render method.
context = {
Expand All @@ -336,6 +340,9 @@ def xblock_view_handler(request, usage_key_string, view_name):
'force_render': force_render,
}

if can_edit_visibility is not None:
context['can_edit_visibility'] = json.loads(can_edit_visibility)

fragment = get_preview_fragment(request, xblock, context)

# Note that the container view recursively adds headers into the preview fragment,
Expand Down
2 changes: 1 addition & 1 deletion cms/static/js/views/paged_container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ define(['jquery', 'underscore', 'common/js/components/utils/view_utils', 'js/vie
};
},

new_child_view: 'container_child_preview',
new_child_view: 'library_container_child_preview',

render: function(options) {
options = options || {};
Expand Down
7 changes: 4 additions & 3 deletions cms/static/js/views/pages/container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,11 +298,11 @@ define(['jquery', 'underscore', 'gettext', 'js/views/pages/base_page', 'common/j
* Refresh an xblock element inline on the page, using the specified xblockInfo.
* Note that the element is removed and replaced with the newly rendered xblock.
* @param xblockElement The xblock element to be refreshed.
* @param block_added Specifies if a block has been added, rather than just needs
* @param blockAdded Specifies if a block has been added, rather than just needs
* refreshing.
* @returns {jQuery promise} A promise representing the complete operation.
*/
refreshChildXBlock: function(xblockElement, block_added, is_duplicate) {
refreshChildXBlock: function(xblockElement, blockAdded, isDuplicate, renderParameters) {
var self = this,
xblockInfo,
TemporaryXBlockView,
Expand All @@ -327,8 +327,9 @@ define(['jquery', 'underscore', 'gettext', 'js/views/pages/base_page', 'common/j
el: xblockElement
});
return temporaryView.render({
data: renderParameters || {},
success: function() {
self.onXBlockRefresh(temporaryView, block_added, is_duplicate);
self.onXBlockRefresh(temporaryView, blockAdded, isDuplicate);
temporaryView.unbind(); // Remove the temporary view
},
initRuntimeData: this
Expand Down
6 changes: 4 additions & 2 deletions cms/static/js/views/pages/paged_container.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@ define(['jquery', 'underscore', 'gettext', 'js/views/pages/container', 'js/views

refreshXBlock: function(element, block_added, is_duplicate) {
var xblockElement = this.findXBlockElement(element),
rootLocator = this.xblockView.model.id;
rootLocator = this.xblockView.model.id,
renderParameters;
if (xblockElement.length === 0 || xblockElement.data('locator') === rootLocator) {
this.render({refresh: true, block_added: block_added});
} else {
this.refreshChildXBlock(xblockElement, block_added, is_duplicate);
renderParameters = {can_edit_visibility: false};

@cahrens cahrens Nov 30, 2016 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

paged_container is a general class for containers that can be paginated. This is the wrong place to be setting can_edit_visibility (although the general approach seems like a good idea).

The disabling of visibility editing needs to be contained in library-specific code.

@itsjeyd itsjeyd Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cahrens Agreed that it would be best to contain disabling of visibility editing in library-specific code. But unless I'm missing something, I'm already doing that here:

There are two files called paged_container.js, cms/static/js/views/pages/paged_container.js and cms/static/js/views/paged_container.js. The code in this file (cms/static/js/views/pages/paged_container.js) seems to be library-specific: On library pages, it is pulled in by cms/static/js/factories/library.js. It is not referenced anywhere else. library.js, in turn, is pulled in by the cms/templates/library.html template for library pages, and is also not referenced by any other JS modules that implement client-side functionality for Studio (as you would expect). That should mean that the code in pages/paged_container.js is only used on library pages.

(It was introduced as part of the main content libraries implementation, #6459.)

@cahrens cahrens Dec 1, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itsjeyd paged_container is currently only used by libraries, but the intention of the code is to be a general paging container. Fortunately there is an existing extension of paged_container called library_container for example the purpose of putting library-specific code. Can you move the disabling of visibility settings to that class?

@itsjeyd itsjeyd Dec 1, 2016

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cahrens That might be possible, but I think it would require a different (possibly more involved) approach: The view defined in library_container (LibraryContainerView) extends the view defined in views/paged_container (PagedContainerView), not the one defined in this file. So it's not a matter of just overriding the refreshXBlock function in LibraryContainerView.

Additionally, the code in PagedContainerView is not (directly) involved in rendering a newly created XBlock; this part of the process is handled by pages/container (XBlockContainerPage) and this file (PagedXBlockContainerPage), as well as the generic XBlockView. Control only moves to PagedContainerView after the new XBlock has been rendered, to update the current page if necessary.

So if we wanted to handle disabling of visibility settings in LibraryContainerView we wouldn't be able to stick with the current approach; that step would have to be handled entirely on the client somehow. I'm not sure if it would it make sense for LibraryContainerView to be responsible for that.

this.refreshChildXBlock(xblockElement, block_added, is_duplicate, renderParameters);
}
},

Expand Down
4 changes: 3 additions & 1 deletion cms/static/js/views/xblock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,13 @@ define(['jquery', 'underscore', 'common/js/components/utils/view_utils', 'js/vie
var self = this,
view = this.view,
xblockInfo = this.model,
xblockUrl = xblockInfo.url();
xblockUrl = xblockInfo.url(),
data = options === 'undefined' ? {} : options.data;
return $.ajax({
url: decodeURIComponent(xblockUrl) + '/' + view,
type: 'GET',
cache: false,
data: data,
headers: {Accept: 'application/json'},
success: function(fragment) {
self.handleXBlockFragment(fragment, options);
Expand Down