-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Front-end work for duplicating components on the unit page. #2205
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| define ["backbone", "jquery", "underscore", "gettext", "xblock/runtime.v1", | ||
| "js/views/feedback_notification", "js/views/metadata", "js/collections/metadata" | ||
| "js/utils/modal", "jquery.inputnumber", "xmodule"], | ||
| "js/utils/modal", "jquery.inputnumber", "xmodule", "coffee/src/main"], | ||
| (Backbone, $, _, gettext, XBlock, NotificationView, MetadataView, MetadataCollection, ModalUtils) -> | ||
| class ModuleEdit extends Backbone.View | ||
| tagName: 'li' | ||
|
|
@@ -62,7 +62,7 @@ define ["backbone", "jquery", "underscore", "gettext", "xblock/runtime.v1", | |
| changedMetadata: -> | ||
| return _.extend(@metadataEditor.getModifiedMetadataValues(), @customMetadata()) | ||
|
|
||
| createItem: (parent, payload) -> | ||
| createItem: (parent, payload, callback=->) -> | ||
| payload.parent_locator = parent | ||
| $.postJSON( | ||
| @model.urlRoot | ||
|
|
@@ -71,7 +71,7 @@ define ["backbone", "jquery", "underscore", "gettext", "xblock/runtime.v1", | |
| @model.set(id: data.locator) | ||
| @$el.data('locator', data.locator) | ||
| @render() | ||
| ) | ||
| ).success(callback) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What happens if this call is unsuccessful?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The "Adding" or "Duplicating" notification is replaced by our standard "Studio is having trouble saving your work" message at the bottom. I don't add the component into the unit, under the assumption that the error meant the creation or duplication did not work. In reality, it is possible that the creation or duplication did work, and the error occurred after that. In that case, refreshing the page (which we recommend in the error message) will show the component. The code as previously written always assumed that the operation was successful-- the component was added to the page immediately, and the analytics tracking message was created, both before we even heard back from the server. |
||
|
|
||
| render: -> | ||
| if @model.id | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ define ["jquery", "jquery.ui", "gettext", "backbone", | |
| 'click .create-draft': 'createDraft' | ||
| 'click .publish-draft': 'publishDraft' | ||
| 'change .visibility-select': 'setVisibility' | ||
| "click .component-actions .duplicate-button": 'duplicateComponent' | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we've placed other .component-actions for editing under module_edit.coffee, should event handler move into that file?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I don't think so. module_edit is for a particular module within a unit. All we would be doing is passing a function to module_edit, and all the logic of how the operation is done would still be within unit.coffee (since it needs the parent location and needs to insert the new component into the new place). I originally coded it that way (following the same sort of pattern as the onDelete callback which is passed to module_edit), but it just didn't feel like it made sense. It also forced me to add code into the tabs.coffee file, even though tabs will not support duplicate.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok that makes sense to me. thanks for clarifying. |
||
|
|
||
| initialize: => | ||
| @visibilityView = new UnitEditView.Visibility( | ||
|
|
@@ -86,28 +87,62 @@ define ["jquery", "jquery.ui", "gettext", "backbone", | |
| @$newComponentItem.removeClass('adding') | ||
| @$newComponentItem.find('.rendered-component').remove() | ||
|
|
||
| saveNewComponent: (event) => | ||
| createComponent: (event, data, notification_message, analytics_message, success_callback) => | ||
| event.preventDefault() | ||
|
|
||
| editor = new ModuleEditView( | ||
| onDelete: @deleteComponent | ||
| model: new ModuleModel() | ||
| ) | ||
|
|
||
| @$newComponentItem.before(editor.$el) | ||
| notification = new NotificationView.Mini | ||
| title: notification_message | ||
|
|
||
| notification.show() | ||
|
|
||
| callback = -> | ||
| notification.hide() | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Who knows if our analytics tracking would actually still work... But I can tell you that .data('location') wouldn't work as written because
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need to add investigation into making sure analytics still works as a separate story? I don't know that we are actively reviewing this data, but I would hate to lose the ability to do so. Is there an easy way to change .data('location') to be in the proper syntax or should this just be removed for now ?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I changed to the locator (and moved the analytics call into the success callback), so it should theoretically work. Lou said he doesn't care right now about analytics. He doesn't think it is something that needs to be on the backlog now, and he doesn't care whether or not we keep the code in. :) |
||
| success_callback() | ||
| analytics.track analytics_message, | ||
| course: course_location_analytics | ||
| unit_id: unit_location_analytics | ||
| type: editor.$el.data('locator') | ||
|
|
||
| editor.createItem( | ||
| @$el.data('locator'), | ||
| $(event.currentTarget).data() | ||
| data, | ||
| callback | ||
| ) | ||
|
|
||
| analytics.track "Added a Component", | ||
| course: course_location_analytics | ||
| unit_id: unit_location_analytics | ||
| type: $(event.currentTarget).data('location') | ||
| return editor | ||
|
|
||
| saveNewComponent: (event) => | ||
| success_callback = => | ||
| @$newComponentItem.before(editor.$el) | ||
| editor = @createComponent( | ||
| event, $(event.currentTarget).data(), | ||
| gettext('Adding…'), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not do: gettext('Adding') + '…'
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ugh, nevermind. I don't know which is correct. |
||
| "Creating new component", | ||
| success_callback | ||
| ) | ||
| @closeNewComponent(event) | ||
|
|
||
| duplicateComponent: (event) => | ||
| $component = $(event.currentTarget).parents('.component') | ||
| source_locator = $component.data('locator') | ||
| success_callback = -> | ||
| $component.after(editor.$el) | ||
| $('html, body').animate({ | ||
| scrollTop: editor.$el.offset().top | ||
| }, 500) | ||
| editor = @createComponent( | ||
| event, | ||
| {duplicate_source_locator: source_locator}, | ||
| gettext('Duplicating…') | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not do: gettext('Duplicating') + '…' |
||
| "Duplicating " + source_locator, | ||
| success_callback | ||
| ) | ||
|
|
||
| components: => @$('.component').map((idx, el) -> $(el).data('locator')).get() | ||
|
|
||
| wait: (value) => | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,164 @@ | ||
| define(["coffee/src/views/unit", "js/models/module_info", "js/spec/create_sinon", "js/views/feedback_notification", | ||
| "jasmine-stealth"], | ||
| function (UnitEditView, ModuleModel, create_sinon, NotificationView) { | ||
| var verifyJSON = function (requests, json) { | ||
| var request = requests[requests.length - 1]; | ||
| expect(request.url).toEqual("/xblock"); | ||
| expect(request.method).toEqual("POST"); | ||
| expect(request.requestBody).toEqual(json); | ||
| }; | ||
|
|
||
| var verifyComponents = function (unit, locators) { | ||
| var components = unit.$(".component"); | ||
| expect(components.length).toBe(locators.length); | ||
| for (var i=0; i < locators.length; i++) { | ||
| expect($(components[i]).data('locator')).toBe(locators[i]); | ||
| } | ||
| }; | ||
|
|
||
| var verifyNotification = function (notificationSpy, text, requests) { | ||
| expect(notificationSpy.constructor).toHaveBeenCalled(); | ||
| expect(notificationSpy.show).toHaveBeenCalled(); | ||
| expect(notificationSpy.hide).not.toHaveBeenCalled(); | ||
| var options = notificationSpy.constructor.mostRecentCall.args[0]; | ||
| expect(options.title).toMatch(text); | ||
| create_sinon.respondWithJson(requests, {"locator": "new_item"}); | ||
| expect(notificationSpy.hide).toHaveBeenCalled(); | ||
| }; | ||
|
|
||
| describe('duplicateComponent ', function () { | ||
| var duplicateFixture = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this live in a separate file?
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have a place to store testing-only fixtures (as far as I can tell). In other tests, we have put the fixture code directly in the test. |
||
| '<div class="main-wrapper edit-state-draft" data-locator="unit_locator"> \ | ||
| <ol class="components"> \ | ||
| <li class="component" data-locator="loc_1"> \ | ||
| <div class="wrapper wrapper-component-editor"/> \ | ||
| <div class="component-actions"> \ | ||
| <a href="#" class="duplicate-button standard"><span class="duplicate-icon icon-copy"></span>Duplicate</a> \ | ||
| </div> \ | ||
| </li> \ | ||
| <li class="component" data-locator="loc_2"> \ | ||
| <div class="wrapper wrapper-component-editor"/> \ | ||
| <div class="component-actions"> \ | ||
| <a href="#" class="duplicate-button standard"><span class="duplicate-icon icon-copy"></span>Duplicate</a> \ | ||
| </div> \ | ||
| </li> \ | ||
| </ol> \ | ||
| </div>'; | ||
|
|
||
| var unit; | ||
| var clickDuplicate = function (index) { | ||
| unit.$(".duplicate-button")[index].click(); | ||
| }; | ||
| beforeEach(function () { | ||
| setFixtures(duplicateFixture); | ||
| unit = new UnitEditView({ | ||
| el: $('.main-wrapper'), | ||
| model: new ModuleModel({ | ||
| id: 'unit_locator', | ||
| state: 'draft' | ||
| }) | ||
| }); | ||
| }); | ||
|
|
||
| it('sends the correct JSON to the server', function () { | ||
| var requests = create_sinon.requests(this); | ||
| clickDuplicate(0); | ||
| verifyJSON(requests, '{"duplicate_source_locator":"loc_1","parent_locator":"unit_locator"}'); | ||
| }); | ||
|
|
||
| it('inserts duplicated component immediately after source upon success', function () { | ||
| var requests = create_sinon.requests(this); | ||
| clickDuplicate(0); | ||
| create_sinon.respondWithJson(requests, {"locator": "duplicated_item"}); | ||
| verifyComponents(unit, ['loc_1', 'duplicated_item', 'loc_2']); | ||
| }); | ||
|
|
||
| it('inserts duplicated component at end if source at end', function () { | ||
| var requests = create_sinon.requests(this); | ||
| clickDuplicate(1); | ||
| create_sinon.respondWithJson(requests, {"locator": "duplicated_item"}); | ||
| verifyComponents(unit, ['loc_1', 'loc_2', 'duplicated_item']); | ||
| }); | ||
|
|
||
| it('shows a notification while duplicating', function () { | ||
| var notificationSpy = spyOnConstructor(NotificationView, "Mini", ["show", "hide"]); | ||
| notificationSpy.show.andReturn(notificationSpy); | ||
|
|
||
| var requests = create_sinon.requests(this); | ||
| clickDuplicate(0); | ||
| verifyNotification(notificationSpy, /Duplicating/, requests); | ||
| }); | ||
|
|
||
| it('does not insert duplicated component upon failure', function () { | ||
| var server = create_sinon.server(500, this); | ||
| clickDuplicate(0); | ||
| server.respond(); | ||
| verifyComponents(unit, ['loc_1', 'loc_2']); | ||
| }); | ||
| }); | ||
| describe('saveNewComponent ', function () { | ||
| var newComponentFixture = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here -- separate file? |
||
| '<div class="main-wrapper edit-state-draft" data-locator="unit_locator"> \ | ||
| <ol class="components"> \ | ||
| <li class="component" data-locator="loc_1"> \ | ||
| <div class="wrapper wrapper-component-editor"/> \ | ||
| </li> \ | ||
| <li class="component" data-locator="loc_2"> \ | ||
| <div class="wrapper wrapper-component-editor"/> \ | ||
| </li> \ | ||
| <li class="new-component-item adding"> \ | ||
| <div class="new-component"> \ | ||
| <ul class="new-component-type"> \ | ||
| <li> \ | ||
| <a href="#" class="single-template" data-type="discussion" data-category="discussion"/> \ | ||
| </li> \ | ||
| </ul> \ | ||
| </div> \ | ||
| </li> \ | ||
| </ol> \ | ||
| </div>'; | ||
|
|
||
| var unit; | ||
| var clickNewComponent = function () { | ||
| unit.$(".new-component .new-component-type a.single-template").click(); | ||
| }; | ||
| beforeEach(function () { | ||
| setFixtures(newComponentFixture); | ||
| unit = new UnitEditView({ | ||
| el: $('.main-wrapper'), | ||
| model: new ModuleModel({ | ||
| id: 'unit_locator', | ||
| state: 'draft' | ||
| }) | ||
| }); | ||
| }); | ||
| it('sends the correct JSON to the server', function () { | ||
| var requests = create_sinon.requests(this); | ||
| clickNewComponent(); | ||
| verifyJSON(requests, '{"category":"discussion","type":"discussion","parent_locator":"unit_locator"}'); | ||
| }); | ||
|
|
||
| it('inserts new component at end', function () { | ||
| var requests = create_sinon.requests(this); | ||
| clickNewComponent(); | ||
| create_sinon.respondWithJson(requests, {"locator": "new_item"}); | ||
| verifyComponents(unit, ['loc_1', 'loc_2', 'new_item']); | ||
| }); | ||
|
|
||
| it('shows a notification while creating', function () { | ||
| var notificationSpy = spyOnConstructor(NotificationView, "Mini", ["show", "hide"]); | ||
| notificationSpy.show.andReturn(notificationSpy); | ||
| var requests = create_sinon.requests(this); | ||
| clickNewComponent(); | ||
| verifyNotification(notificationSpy, /Adding/, requests); | ||
| }); | ||
|
|
||
| it('does not insert duplicated component upon failure', function () { | ||
| var server = create_sinon.server(500, this); | ||
| clickNewComponent(); | ||
| server.respond(); | ||
| verifyComponents(unit, ['loc_1', 'loc_2']); | ||
| }); | ||
| }); | ||
| } | ||
| ); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -163,6 +163,10 @@ | |
| margin-right: 12px; | ||
| } | ||
| } | ||
|
|
||
| .duplicate-button.standard { | ||
| display: none; | ||
| } | ||
| } | ||
|
|
||
| .edit-static-page { | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
postJSON is defined in main.coffee. We always load that when running CMS, but we don't always load it when running unit tests (unless an earlier test loads it). I'm making the dependency more clearly stated.