[TNL-5947] Fix: Don't show visibility settings on library pages. by itsjeyd · Pull Request #14073 · openedx/openedx-platform

itsjeyd · 2016-11-30T13:35:55Z

This PR fixes a bug where components added to a content library would incorrectly display a button for editing visibility settings (that button should be hidden on library pages).

JIRA tickets:

Discussions:

See TNL-5947.

Dependencies:

None

Screenshots:

Content library after adding HTML block (before):

Content library after adding HTMl block (after):

Sandbox URLs:

Deployment targets:

edx.org
edge.edx.org

Merge deadline:

ASAP

Testing instructions:

Log in to Studio and navigate to the "Libraries" tab.
Create a new library by clicking "New Library" or click on an existing one.
Add an HTML component to the library. Observe that the header does not show a visibility button.
Refresh the page and make sure the visibility button is still hidden.

Reviewers

OpenCraft: @smarnach
edX: @awaisdar001, CC @cahrens

openedx-webhooks · 2016-11-30T13:36:05Z

Thanks for the pull request, @itsjeyd! It looks like you're a member of a company that does contract work for edX. If you're doing this work as part of a paid contract with edX, you should talk to edX about who will review this pull request. If this work is not part of a paid contract with edX, then you should ensure that there is an OSPR issue to track this work in JIRA, so that we don't lose track of your pull request.

Create an OSPR issue for this pull request.

cahrens · 2016-11-30T14:59:18Z


        elif view_name in PREVIEW_VIEWS + container_views:
            is_pages_view = view_name == STUDENT_VIEW   # Only the "Pages" view uses student view in Studio
+            is_library_view = view_name == 'container_child_preview'


How do we know that only libraries will have the view name "container_child_preview"? Won't that be the case for any child of a container view (for instance, components inside of content experiments/split_test_module)?

FYI @andy-armstrong

Also, this method shouldn't know anything about libraries. It is unfortunate that this file already has some library-specific logic, but I would very much hope that things that are specific to libraries can remain within library code.

@cahrens Ah, perhaps the naming of the variable is a little confusing: The library root block itself uses container_preview as a view name. Child blocks that are added to a library use container_child_preview. That view name was added as part of the main content library implementation (#6459), and doesn't seem to be used anywhere else.

I just checked, components inside of content experiments don't use container_child_preview (like child blocks on unit pages, they use reorderable_container_child_preview).

If the view name is specific to libraries, then yes, I think it should be renamed.

But I also think that this CMS code should not have to know about libraries. Is there no way to keep the logic contained within libraries?

@cahrens I updated the variable name in c777d83, please have another look.

cahrens · 2016-11-30T15:45:22Z

        elif view_name in PREVIEW_VIEWS + container_views:
            is_pages_view = view_name == STUDENT_VIEW   # Only the "Pages" view uses student view in Studio
            can_edit = has_studio_write_access(request.user, usage_key.course_key)
+            can_edit_visibility = not view_name == 'container_child_preview'


I don't think that this rename really helps. It just leads one to wonder why children of containers can't have visibility settings.

What I meant is that the library view name should be renamed so that it is clear that it is specific to libraries. And the library code should be contained with libraries themselves, not put into views/item.py. Adding logic that is specific to libraries in this method is technical debt.

@cahrens I renamed the view in a5f157f.

f0ceab2 implements an alternative approach that explicitly passes can_edit_visibility info from the library JS code instead of relying on knowledge about view names inside xblock_view_handler.

to reflect the fact that this view name is only used for components displayed on library pages.

rendering a newly added component on a library page (instead of setting this info based on view name inside "xblock_view_handler").

cahrens · 2016-11-30T19:04:41Z

                    this.render({refresh: true, block_added: block_added});
                } else {
-                    this.refreshChildXBlock(xblockElement, block_added, is_duplicate);
+                    renderParameters = {can_edit_visibility: false};


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.

@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.)

@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?

@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.

…AJAX call.

smarnach · 2017-03-14T13:45:27Z

@itsjeyd Can this PR be closed? I'm shutting down the sandobx because it's broken.

itsjeyd · 2017-03-14T14:43:24Z

@smarnach Yes, it should be fine to close this PR.

Fix: Don't show visibility settings on library pages.

7b0c8f6

cahrens reviewed Nov 30, 2016

View reviewed changes

Improve variable names.

c777d83

cahrens reviewed Nov 30, 2016

View reviewed changes

itsjeyd added 2 commits November 30, 2016 17:43

Rename "container_child_preview" to "library_container_child_preview"

a5f157f

to reflect the fact that this view name is only used for components displayed on library pages.

Alternative approach: Pass "can_edit_visibility" info explicitly when

f0ceab2

rendering a newly added component on a library page (instead of setting this info based on view name inside "xblock_view_handler").

cahrens reviewed Nov 30, 2016

View reviewed changes

itsjeyd added 2 commits December 1, 2016 11:12

XBlockView.render: Make sure "options" is defined before using it in …

a2ae07b

…AJAX call.

Fix quality violations.

a239786

itsjeyd force-pushed the itsjeyd/fix-library-visibility branch from 91d221b to a239786 Compare December 1, 2016 11:29

itsjeyd closed this Mar 14, 2017

bradenmacdonald deleted the itsjeyd/fix-library-visibility branch May 18, 2019 21:58

Uh oh!

Conversation

itsjeyd commented Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openedx-webhooks commented Nov 30, 2016

Uh oh!

cahrens Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cahrens Nov 30, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarnach commented Mar 14, 2017

Uh oh!

itsjeyd commented Mar 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

itsjeyd commented Nov 30, 2016 •

edited

Loading

cahrens Nov 30, 2016 •

edited

Loading

cahrens Nov 30, 2016 •

edited

Loading