adds TabsEditingDescriptor, pluggen in VideoAlphaDescriptor #386

auraz · 2013-07-12T15:27:51Z

Introduces tabs for Videoalpha in studio.
@cahrens and @dmitchell please review.

For now, subtitles tabs is disabled, as it empty. It will be enabled in transcripts PR.
.

cahrens · 2013-07-12T18:16:59Z

cms/static/coffee/spec/tabs/edit.coffee

+      expect($.fn.trigger.calls.length).toEqual(1)
+
+  describe "save", ->
+    it "if CodeMirror exist, data should be retreived", ->


This test isn't really about codemirror, just getValue.

cahrens · 2013-07-12T19:33:44Z

I am on vacation all next week. When my comments are addressed, @dmitchell is another Studio person who can review the code (he's back in the office on Tuesday, I believe).

frrrances · 2013-07-13T20:50:10Z

@auraz i wasn't able to get the style sheet you added for the video alpha tabs working, so I have some ugly rules in the unit sass file I would like to clean up before merge if possible, but the tabs are looking like they should for now. let's talk in standup on monday.

rocha · 2013-07-15T19:30:41Z

@auraz, @dmitchell may be a better person to review the code than me. I think he is back in the office Tuesday June 16.

auraz · 2013-07-16T09:42:22Z

@rocha ok, thank you

dmitchell · 2013-07-16T14:43:44Z

cms/static/coffee/fixtures/tabs-edit.html

+    </section>
+
+    <div class="component-edit-header" style="display: block"/>
+</div>


Probably should have newline at end of file.

frrrances · 2013-07-19T16:00:02Z

@cahrens: on the previous PR for this (https://github.com/edx/edx-platform/pull/370#issuecomment-20838804), you noted that the placement of the help text in settings was too high, but this is unrelated to this PR, so I would like to address that in a separate PR.

auraz · 2013-07-25T08:49:23Z

These tests are failing here and as well as in master, this is unrelated to current PR

Course Overview, runshould save model when save is clicked and should show a confirmation on save.

cahrens · 2013-07-25T17:23:50Z

common/lib/xmodule/xmodule/js/src/tabs/tabs-aggregator.coffee

@@ -0,0 +1,125 @@
+window.TabsEditingDescriptorModel =


Why is there a global TabsEditingDescriptorModel, vs. TabsEditingDescriptor maintaining an instance?

Because html templates are loaded before xmodule descriptor is instantiated.
Template need to register save functions and callback.
That's why TabsEditingDescriptorModel is global, so tab templates can register their functions.
I would like to have TabsEditingDescriptorModel part of TabsEditingDescriptor, but due to described behavior, I do not know how to do that. If you have ideas please share.

@cpennington Do you have ideas about this? Will there be a solution in the xblock world?

I don't follow the explanation of why this needs to be global. Can you point to particular lines of code that would work for a local instance loaded after the html is loaded onto the page?

@cpennington For example, in codemirror-edit.html (one of tabs) there are two functions:

window.TabsEditingDescriptorModel.addModelUpdate, which registers modelUpdate callback for current tab.

window.TabsEditingDescriptorModel.addOnSwitch, which registers tab onswitch function.
At the very moment of loading templates for tabs, tabs-aggregator.coffee is loaded, so window.TabsEditingDescriptorModel is defined,
but descriptor instance is not created (TabsEditingDescriptor.constructor is not invoked).

We have next order of loading in studio:

At page load, all js files are loaded, this is where window.TabsEditingDescriptorModel is created.

At adding component, all templates are loaded. This is where TabsEditingDescriptorModel.addModelUpdate and TabsEditingDescriptorModel.addOnSwitch are invoked,

On pressing edit button in studio, for editing component, descriptor instance is created (TabsEditingDescriptor.constructor is invoked).

Couldn't the two functions from 2. be invoked by the TabsEditingDescriptor? All of the javascript in codemirror-edit.html is initialization code that I'd expect to only get called when the editor is actually needed. And then if those two functions are called when the edit button is clicked, we don't need to put TabsEditingDescriptorModel in the global namespace either.

I think the issue is that TabsEditingDescriptor is a generic class. It should not be tied to the video module. How about the function calls occurring in a JavaScript file registered by VideoAlphaDescriptor? Right now, VideoAlphaDescritpor only registers HTML and CSS files. But I would expect this sort of initialization to happen in a JavaScript file instead of an HTML template.

@cpennington Sorry, I do not understand, what do you proposing. If I add javascript code in tab's js/html, it goes somewhere, now it belongs to window.TabsEditingDescriptorModel. Don proposed move TabsEditingDescriptorModel to CMS.models. I agree that " All of the javascript in codemirror-edit.html is initialization code that I'd expect to only get called when the editor is actually needed". But those functions from tab need to be put somewhere before being invoked.

@cahrens If i move them to tab's js, they anyway will be put somewhere on js load. How it will solve the problem?

auraz · 2013-07-31T14:39:30Z

@cahrens @dmitchell It is ok to move TabsEditingDescriptorModel definition to CMS.models?

cahrens · 2013-07-31T15:03:51Z

Nothing in common should refer to CMS.models.

auraz · 2013-08-02T07:27:50Z

@cahrens @cpennington @dmitchell During talking with Cale, we figured out with solution to make TabEditingDescriptorModel class property of TabEditingDescriptor. All works fine. Look again over PR please.

@cahrens Regarding moving out js code from tab templates to tab js files. I prefer not to do it in this PR. As now it will not solve any current issue. But will introduce more complexity to the code. If we will need it in future, we can always do it in another PR. Also I do not want for this PR to grow more. I try to make PR granular, as we have lot dependencies with videoalpha.

cpennington · 2013-08-02T10:09:15Z

I think there may be refinements available for the future (for instance, jquery already has a notion of binding methods to events, which might require less code on our part for the onSwitch binding). However, I'm 👍 on this as is.

cahrens · 2013-08-02T17:41:50Z

common/lib/xmodule/xmodule/videoalpha_module.py

    """Descriptor for `VideoAlphaModule`."""
    module_class = VideoAlphaModule
+    template_dir_name = "videoalpha"


I don't think this template_dir_name is actually used for videoalpha. I think this should refer to a directory under /edx-platform/common/lib/xmodule/xmodule/templates, and there is no videoalpha directory there since there are not multiple types of videoalpha instances that can be created.

I believe it is safe to just delete this line. Or I guess in some places we actually put template_dir_name = None.

Note that cleanup should be done at some point to other xmodules, as a cleanup related to Don's PR to remove the majority of our template files.

@cahrens As far i understood, we do not need template_dir_name for video alpha right now, as it is advanced component. Removed.

cahrens · 2013-08-02T17:58:42Z

Just one question and a minor cleanup item. Otherwise, 👍

In the future, it would be helpful to not rebase after every iteration of the code. That makes it impossible to just review the diffs from the last commit (when iterating during the PR process). Instead, make changes as new commits and squash everything only once the review is complete.

Updates comment Make Model descriptor class property. Fix tests

auraz · 2013-08-05T10:46:28Z

@cahrens Comments addressed. I will no do rebase after every iteration. Thank you.
@dmitchell Cale and Christina said ok, is it ok, or you will look over?

dmitchell · 2013-08-05T13:08:23Z

cms/static/coffee/spec/tabs/edit.coffee

+    @isInactiveClass = "is-inactive"
+    @isCurrent = "current"
+    loadFixtures 'tabs-edit.html'
+    @descriptor = new window.TabsEditingDescriptor($('.base_wrapper'))


This is the only place I can see where you reference this from window. Am I missing something?

dmitchell · 2013-08-05T13:10:22Z

I was on vacation. I don't think i have anything to add. I didn't keep up w/ the namespace discussion.

adds TabsEditingDescriptor, pluggen in VideoAlphaDescriptor

Fix metadata inheritance

mattdrayer/MCKIN-2988: Added stop to backwards data migration.

…ird-party-auth-password Change the logic of password generation of third-party-auth openedx#369

…ird-party-auth-password Change the logic of password generation of third-party-auth openedx#369 (cherry picked from commit 6478819)

…ssion_test_for_direct_access Fix of discussion test broken by direct access

* update PUT response to include serializer.data * delete sites and return microsite ids for PUT and POST responses * check for exisiting Sites for PUT * remove unnecessary code

* Proversity/add microsites api ficus (openedx#385) * microsite api for ficus, markdown not displaying correctly in browsable api * work on tests for ficus, reverse url not working for TestCase * catch organization exception * reverse is now working * update test working, need to assert 404s * remove typo * update working again * post test failing * revert changes * swtiching branches, saving work * tests all passing * removes prints, deal with pylint * more pylint * remove unused code * more pylint * add some comments * clean up code and add course_id * update PUT response to include serializer.data (openedx#386) * update PUT response to include serializer.data * delete sites and return microsite ids for PUT and POST responses * check for exisiting Sites for PUT * remove unnecessary code * UPD: recap xblock update to taged version * remove editable flag from freetextresponse req * fix bad last accessed conditional

* update PUT response to include serializer.data * delete sites and return microsite ids for PUT and POST responses * check for exisiting Sites for PUT * remove unnecessary code

* Proversity/add microsites api ficus (#385) * microsite api for ficus, markdown not displaying correctly in browsable api * work on tests for ficus, reverse url not working for TestCase * catch organization exception * reverse is now working * update test working, need to assert 404s * remove typo * update working again * post test failing * revert changes * swtiching branches, saving work * tests all passing * removes prints, deal with pylint * more pylint * remove unused code * more pylint * add some comments * clean up code and add course_id * update PUT response to include serializer.data (#386) * update PUT response to include serializer.data * delete sites and return microsite ids for PUT and POST responses * check for exisiting Sites for PUT * remove unnecessary code * UPD: recap xblock update to taged version * remove editable flag from freetextresponse req * fix bad last accessed conditional

modify

ghost assigned cahrens Jul 12, 2013

cahrens reviewed Jul 12, 2013
View reviewed changes

dmitchell reviewed Jul 16, 2013
View reviewed changes

cahrens reviewed Jul 25, 2013
View reviewed changes

cahrens reviewed Aug 2, 2013
View reviewed changes

auraz added 2 commits August 5, 2013 13:38

Add TabsEditingDescriptor for VideoAlpha

da4f295

Updates comment Make Model descriptor class property. Fix tests

Fixes

6225c2a

dmitchell reviewed Aug 5, 2013
View reviewed changes

auraz added a commit that referenced this pull request Aug 5, 2013

Merge pull request #386 from edx/alex/editor-tabs-for-videoalpha

16cee19

adds TabsEditingDescriptor, pluggen in VideoAlphaDescriptor

auraz merged commit 16cee19 into master Aug 5, 2013

auraz deleted the alex/editor-tabs-for-videoalpha branch August 5, 2013 15:58

chrisrossi pushed a commit to jazkarta/edx-platform that referenced this pull request Mar 31, 2014

Merge pull request openedx#386 from MITx/feature/victor/missing-metadata

516c6a5

Fix metadata inheritance

e-kolpakov referenced this pull request in open-craft/edx-platform Mar 3, 2015

Merge pull request #386 from edx-solutions/mattdrayer/MCKIN-2988

3484a3d

mattdrayer/MCKIN-2988: Added stop to backwards data migration.

hachiyanagi-ks added a commit to nttks/edx-platform that referenced this pull request Oct 2, 2015

Merge pull request openedx#386 from hachiyanagi-ks/develop/cypress/th…

6478819

…ird-party-auth-password Change the logic of password generation of third-party-auth openedx#369

diegomillan pushed a commit to eduNEXT/edx-platform that referenced this pull request Sep 14, 2016

Merge pull request openedx#386 from Stanford-Online/dcadams/fix_discu…

67a970e

…ssion_test_for_direct_access Fix of discussion test broken by direct access

pdpinch mentioned this pull request Aug 4, 2017

DO NOT MERGE: testing change to enrollment email mitodl/edx-platform#29

Closed

xavierchan pushed a commit to xavierchan/edx-platform-1 that referenced this pull request May 20, 2019

Merge pull request openedx#386 from ufresh2013/en

95e7b74

modify

Sujeet1379 pushed a commit to chandrudev/edx-platform that referenced this pull request Nov 17, 2022

Update language for last date to upgrade (openedx#386)

ff93d7f

baby636 mentioned this pull request Nov 30, 2023

[Snyk] Fix for 1 vulnerabilities baby636/edx-platform#55

Open

baby636 mentioned this pull request Dec 20, 2023

[Snyk] Fix for 46 vulnerabilities baby636/edx-platform#56

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

adds TabsEditingDescriptor, pluggen in VideoAlphaDescriptor #386

adds TabsEditingDescriptor, pluggen in VideoAlphaDescriptor #386

auraz commented Jul 12, 2013

cahrens Jul 12, 2013

cahrens commented Jul 12, 2013

frrrances commented Jul 13, 2013

rocha commented Jul 15, 2013

auraz commented Jul 16, 2013

dmitchell Jul 16, 2013

auraz Jul 19, 2013

frrrances commented Jul 19, 2013

auraz commented Jul 25, 2013

cahrens Jul 25, 2013

auraz Jul 26, 2013

cahrens Jul 29, 2013

cpennington Jul 29, 2013

auraz Jul 30, 2013

cpennington Jul 30, 2013

cahrens Jul 30, 2013

auraz Jul 31, 2013

auraz Jul 31, 2013

auraz commented Jul 31, 2013

cahrens commented Jul 31, 2013

auraz commented Aug 2, 2013

cpennington commented Aug 2, 2013

cahrens Aug 2, 2013

auraz Aug 5, 2013

cahrens commented Aug 2, 2013

auraz commented Aug 5, 2013

dmitchell Aug 5, 2013

dmitchell commented Aug 5, 2013

adds TabsEditingDescriptor, pluggen in VideoAlphaDescriptor #386

adds TabsEditingDescriptor, pluggen in VideoAlphaDescriptor #386

Conversation

auraz commented Jul 12, 2013

Choose a reason for hiding this comment

cahrens commented Jul 12, 2013

frrrances commented Jul 13, 2013

rocha commented Jul 15, 2013

auraz commented Jul 16, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frrrances commented Jul 19, 2013

auraz commented Jul 25, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

auraz commented Jul 31, 2013

cahrens commented Jul 31, 2013

auraz commented Aug 2, 2013

cpennington commented Aug 2, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cahrens commented Aug 2, 2013

auraz commented Aug 5, 2013

Choose a reason for hiding this comment

dmitchell commented Aug 5, 2013