Added baseview(extended from backbone view), which can further extended ...

[zubair-arbi]

...by other views

STUD-823
@adampalay
@cahrens

Extend views from BaseView instead of Backbone.View.

Sample for extending other views from this baseview:

define(["backbone", "underscore", "js/views/baseview"],
    function(Backbone, _, BaseView) {

    var AbcView = BaseView.extend({
    )};
    return AbcView;
)};

[cahrens]

Is this PR just intended to get feedback on the general approach? @singingwolfboy, can you give your opinion on the approach? I was hoping for something simpler.

Next steps after the approach is validated--

1. To fix the bug, you need to change existing views to extend this class. And you also need to put the fix in base.js for code that is not using BackBone.

2. I would put this class in js/views directory.

3. You also need to unit test the class to make sure it is working as expected.

[singingwolfboy]

You don't need to call the ready function every time -- by the time this executes, the DOM will be loaded.

[singingwolfboy]

In terms of the general approach, this seems reasonable, but I'd suggest using constructor rather than initialize, like this blog post does: http://fahad19.tumblr.com/post/28158699664/afterrender-callback-in-backbone-js-views

[cahrens]

Do not duplicate code. You can put this function a utility class and use it from both places. For an example, see /edx-platform/cms/static/js/utils/get_date.js.

[cahrens]

Note that since the Backbone selector should be specific to the element associated with the Backbone view (see below), your utility function will need to be able to handle that.

[cahrens]

@zubair-arbi Are you able to reproduce the bug in your environment? For the case of the Course Settings page, your fix (in base.js) does not address the problem. handle_iframe_binding ends up getting run too early, and the video has not yet been added to the page. OH, but that's being the Backbone view is not yet extending your view. Let me try doing that, and I will report back.

BTW, here are the reproduction steps for me:

1. Go to Course Settings and make sure there is an introductory video for the course. If not, specify the YouTube ID of a video.
2. Click on "Upload Course Image". Note that the dialog goes behind the video as you scroll up and down.

I have attached an image.

[cahrens]

This should be scoped to the element associated with this view (this.el). Otherwise, this code will try to change all iframes on the page, regardless of whether or not they are part of the view. That will cause each iframe to be touched multiple times (assuming multiples views per page).

[singingwolfboy]

You should be able to just call this.$("iframe") instead of $("iframe") -- the local jQuery reference will be automatically scoped to the view's element.

[cahrens]

@zubair-arbi The basic approach is working. I changed ValidatingView (/edx-platform/cms/static/js/views/validation.js) to extend baseview, and that fixed the problem with the CourseSettings page (repro steps above).

I also changed course_info_update.js to extend baseview, and that fixed the case of a video being in a CourseUpdate. Here are the reproduction steps for that casel

1. Create a CourseUpdate with a video in it (for example, put this text in a CourseUpdate):
   <
   iframe width="700" height="394" src="https://www.youtube.com/embed/QashL9jhwsk?rel=0" frameborder="0" allowfullscreen></iframe>

2. Try to delete the CourseUpdate. The delete confirmation dialog will can go behind the video.

So these are the next steps:

1. Modify the code to address my feedback.
2. Write a Jasmine unit test for baseview.js (and the utility function that will do the iframe manipulation).
3. Change all our BackBone views that extend Backbone.View to instead extend your view.

[singingwolfboy]

If this JS module only contains this one function, then perhaps it should just return that function, rather than an object containing the function. require.js has no problem with modules that return functions rather than objects.

[cahrens]

I agree, unless it is necessary for unit test purposes.

[zubair-arbi]

I tried as @singingwolfboy told, all modules work fine but not unit tests. I am having difficulty trying to spy of iframeBinding function.

[cahrens]

Yeah, I was afraid that would be a problem. @singingwolfboy, is there a reasonable workaround for the test?

[singingwolfboy]

Well, the proper tool for that would be Squire.js, which is designed to mock require.js dependencies. We're already using Squire in some of our Jasmine tests, but for some reason those tests don't play nicely with the tests that don't use Squire -- I think that something in the JS execution environment is holding on to the mocks even after Squire tries to remove them. As a result, we're currently running those tests using a separate test runner, which makes for some duplicated code.

I suggest that you give Squire a shot to see if you can easily mock it that way. If you can, great! If it ends up being annoying or requires even more duplicated code, then just go with the solution you've currently got.

[cahrens]

@zubair-arbi This is getting close! After you have responded to the comments, please let me know so I can re-review. Github unfortunately does not send out a notification when a PR is updated.

[cahrens]

I'd love to give this a thumbs-up, but while testing on the branch, I found a couple cases that still don't work.

Reproduction steps (again this is for me-- I don't think you've ever answered whether or not you are able to reproduce this z-ordering issue in your environment):

1. Create a CourseUpdate with a video in it (for example, put this text in a CourseUpdate):
   <
   iframe width="700" height="394" src="https://www.youtube.com/embed/QashL9jhwsk?rel=0" frameborder="0"></iframe>

2. Refresh the page and try to delete the course update. All is well, the dialog cannot go behind the video.

3. Now click "Edit" on the update and "Cancel".

4. Again try to delete the update. Now the dialog CAN go behind the video. Inspecting the element, the URL no longer has the wmode=transparent parameter.

I also find that the dialog always goes behind the video that was put in via an "embed" tag. I'm not sure if there is anything we can do about that. Click the "edit pencil" in the upper right corner to see the embed XML below.

Please let me know if you can reproduce these issues.

[zubair-arbi]

@cahrens I was able to reproduce this issue. The reason is that render function of course_info_update view is not called when onSave/onCancel event is fired that’s why src url of iframe in not modified. Possible solutions could be:

1. Call iframeBinding function manually after onSave/onCancel event:
   IframeBinding.iframeBinding(this.modelDom(event)[0]);
2. add 'afterOnSave/afterOnCancel' event in baseview(but onSave/onCancel events are likely to change according to different views)
3. If this issue is specific to views which use course_info_helper to update model then we can modify 'changeContentToPreview' to call iframeBinding function to update content.

For embed code I will just have to add search for embed tag too:
$("iframe, embed") instead of $("iframe")

[cahrens]

@zubair-arbi

I wouldn't expect onSave/onCancel to exist as methods in baseview. So I think #2 is out.

#3 seems like a reasonable short-term workaround. However, a better solution would be to change how course_info_update renders views. The render method for CourseInfoUpdateView should create a separate view for each model in the collection, and then when just one entry changes (onSave or onCancel), the render method for that subview should be called. That would allow your general fix in baseview to work.

We probably have other similar problems in other Backbone code, but this is the only case I'm aware of where a HTML module exists within a collection.

Did you check if searching for the embed tag works? If it does, that sounds like the right solution. But when I tried adding wmode=transparent (editing directly in the Chrome debugger), it didn't seem to work. Perhaps I didn't do something correctly.

[cahrens]

The variable name "e" is not descriptive. Make it something like "text" or "html".

[cahrens]

@zubair-arbi Please follow up on the comments above from Andy and me. Then I will take a final look, and finally, you will need to interactively rebase in order to squash the commits into a single commit before merging with master.

Here is a wiki page that explains interactive rebases. The goal is to make the commit history as clean as possible so a given merge does not contain a lot of small, iterative commits.

https://edx-wiki.atlassian.net/wiki/pages/viewpage.action?pageId=25855607

See " interactive rebase" about halfway down the page.

[cahrens]

👍

[cahrens]

@zubair-arbi Unfortunately there is now a merge conflict with this PR. You will need to rebase on top of master. Let me know if you have any difficulty resolving the conflicts.