diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 2a08be6b4f..07a7309df2 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -74,7 +74,7 @@ def usage_key_with_run(usage_key_string): # pylint: disable=unused-argument -@require_http_methods(("DELETE", "GET", "PUT", "POST")) +@require_http_methods(("DELETE", "GET", "PUT", "POST", "PATCH")) @login_required @expect_json def xblock_handler(request, usage_key_string): @@ -96,7 +96,10 @@ def xblock_handler(request, usage_key_string): to None! Absent ones will be left alone. :nullout: which metadata fields to set to None :graderType: change how this unit is graded - :publish: can be only one value-- 'make_public' + :publish: can be either -- 'make_public' (which publishes the content) or 'discard_changes' + (which reverts to the last published version). If 'discard_changes', the other fields + will not be used; that is, it is not possible to update and discard changes + in a single operation. The JSON representation on the updated xblock (minus children) is returned. if usage_key_string is not specified, create a new xblock instance, either by duplicating @@ -273,6 +276,13 @@ def _save_item(user, usage_key, data=None, children=None, metadata=None, nullout log.error("Can't find item by location.") return JsonResponse({"error": "Can't find item by location: " + unicode(usage_key)}, 404) + # Don't allow updating an xblock and discarding changes in a single operation (unsupported by UI). + if publish == "discard_changes": + store.revert_to_published(usage_key, user.id) + # Returning the same sort of result that we do for other save operations. In the future, + # we may want to return the full XBlockInfo. + return JsonResponse({'id': unicode(usage_key)}) + old_metadata = own_metadata(existing_item) old_content = existing_item.get_explicitly_set_fields_by_scope(Scope.content) @@ -339,9 +349,9 @@ def _save_item(user, usage_key, data=None, children=None, metadata=None, nullout if grader_type is not None: result.update(CourseGradingModel.update_section_grader_type(existing_item, grader_type, user)) - # Make public after updating the xblock, in case the caller asked - # for both an update and a publish. - if publish and publish == 'make_public': + # Make public after updating the xblock, in case the caller asked for both an update and a publish. + # Although not supported in the UI, Bok Choy tests use this. + if publish == 'make_public': modulestore().publish(existing_item.location, user.id) # Note that children aren't being returned until we have a use case. diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index aed94865cc..fb4c37075c 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -553,6 +553,22 @@ class TestEditItem(ItemTest): def test_make_draft(self): """ Test creating a draft version of a public problem. """ + self._make_draft_content_different_from_published() + + def test_revert_to_published(self): + """ Test reverting draft content to published """ + self._make_draft_content_different_from_published() + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'discard_changes'} + ) + published = self.verify_publish_state(self.problem_usage_key, PublishState.public) + self.assertIsNone(published.due) + + def _make_draft_content_different_from_published(self): + """ + Helper method to create different draft and published versions of a problem. + """ # Make problem public. self.client.ajax_post( self.problem_update_url, diff --git a/cms/static/js/spec/views/pages/container_subviews_spec.js b/cms/static/js/spec/views/pages/container_subviews_spec.js index 37e8130f8a..3a22b543dd 100644 --- a/cms/static/js/spec/views/pages/container_subviews_spec.js +++ b/cms/static/js/spec/views/pages/container_subviews_spec.js @@ -149,10 +149,9 @@ define(["jquery", "underscore", "underscore.string", "js/spec_helpers/create_sin expect(promptSpies.constructor).toHaveBeenCalled(); promptSpies.constructor.mostRecentCall.args[0].actions.primary.click(promptSpies); - request = lastRequest(); - expect(request.url).toEqual("/xblock/locator-container"); - expect(request.method).toEqual("DELETE"); - expect(request.requestBody).toEqual(null); + create_sinon.expectJsonRequest(requests, "POST", "/xblock/locator-container", + {"publish": "discard_changes"} + ); }; beforeEach(function() { @@ -201,19 +200,15 @@ define(["jquery", "underscore", "underscore.string", "js/spec_helpers/create_sin containerPage.$(publishButtonCss).click(); edit_helpers.verifyNotificationShowing(notificationSpy, /Publishing/); - request = lastRequest(); - expect(request.url).toEqual("/xblock/locator-container"); - expect(request.method).toEqual("POST"); - expect(JSON.parse(request.requestBody).publish).toEqual("make_public"); + create_sinon.expectJsonRequest(requests, "POST", "/xblock/locator-container", + {"publish": "make_public"} + ); // Response to publish call - respondWithJson({"id": "locator-container", "published": true, "has_changes": false}); + respondWithJson({"id": "locator-container", "data": null, "metadata":{}}); edit_helpers.verifyNotificationHidden(notificationSpy); - request = lastRequest(); - expect(request.url).toEqual("/xblock/locator-container"); - expect(request.method).toEqual("GET"); - expect(request.requestBody).toEqual(null); + create_sinon.expectJsonRequest(requests, "GET", "/xblock/locator-container"); // Response to fetch respondWithJson({"id": "locator-container", "published": true, "has_changes": false}); @@ -243,28 +238,39 @@ define(["jquery", "underscore", "underscore.string", "js/spec_helpers/create_sin expect(containerPage.model.get("publish")).toBeNull(); }); - /* STUD-1860 it('can discard changes', function () { - sendDiscardChangesToServer(this); + var notificationSpy = edit_helpers.createNotificationSpy(), + renderPageSpy = spyOn(containerPage.xblockPublisher, 'renderPage').andCallThrough(), + numRequests; - var numRequests = requests.length; - create_sinon.respondToDelete(requests); + sendDiscardChangesToServer(this); + numRequests = requests.length; + + // Respond with success. + respondWithJson({"id": "locator-container"}); + edit_helpers.verifyNotificationHidden(notificationSpy); + + // Verify other requests are sent to the server to update page state. // Response to fetch, specifying the very next request (as multiple requests will be sent to server) - respondWithJson({"id": "locator-container", "published": true, "has_changes": false}, numRequests); - expect(containerPage.$(discardChangesButtonCss)).toHaveClass('is-disabled'); - expect(containerPage.$(bitPublishingCss)).toHaveClass(publishedBit); + expect(requests.length > numRequests).toBeTruthy(); + expect(containerPage.model.get("publish")).toBeNull(); + expect(renderPageSpy).toHaveBeenCalled(); }); - */ it('does not fetch if discard changes fails', function () { - sendDiscardChangesToServer(this); + var renderPageSpy = spyOn(containerPage.xblockPublisher, 'renderPage').andCallThrough(), + numRequests; + + sendDiscardChangesToServer(this); + numRequests = requests.length; - var numRequests = requests.length; // Respond with failure create_sinon.respondWithError(requests); expect(requests.length).toEqual(numRequests); expect(containerPage.$(discardChangesButtonCss)).not.toHaveClass('is-disabled'); + expect(containerPage.model.get("publish")).toBeNull(); + expect(renderPageSpy).not.toHaveBeenCalled(); }); it('does not discard changes on cancel', function () { diff --git a/cms/static/js/views/pages/container_subviews.js b/cms/static/js/views/pages/container_subviews.js index 1d27d2ba2b..a4b535867a 100644 --- a/cms/static/js/views/pages/container_subviews.js +++ b/cms/static/js/views/pages/container_subviews.js @@ -1,8 +1,8 @@ /** * Subviews (usually small side panels) for XBlockContainerPage. */ -define(["jquery", "underscore", "gettext", "js/views/baseview", "js/views/feedback_prompt"], - function ($, _, gettext, BaseView, PromptView) { +define(["jquery", "underscore", "gettext", "js/views/baseview"], + function ($, _, gettext, BaseView) { var disabledCss = "is-disabled"; @@ -123,7 +123,7 @@ define(["jquery", "underscore", "gettext", "js/views/baseview", "js/views/feedba } this.runOperationShowingMessage(gettext('Publishing…'), function () { - return xblockInfo.save({publish: 'make_public'}); + return xblockInfo.save({publish: 'make_public'}, {patch: true}); }).always(function() { xblockInfo.set("publish", null); }).done(function () { @@ -132,50 +132,28 @@ define(["jquery", "underscore", "gettext", "js/views/baseview", "js/views/feedba }, discardChanges: function (e) { + var xblockInfo = this.model, that=this, renderPage = this.renderPage; if (e && e.preventDefault) { e.preventDefault(); } - var xblockInfo = this.model, view, renderPage = this.renderPage; - - view = new PromptView.Warning({ - title: gettext("Discard Changes"), - message: gettext("Are you sure you want to discard changes and revert to the last published version?"), - actions: { - primary: { - text: gettext("Discard Changes"), - click: function (view) { - view.hide(); - $.ajax({ - type: 'DELETE', - url: xblockInfo.url() - }).success(function () { - window.alert("Refresh the page to see that changes were discarded. " + - "Auto-refresh will be implemented in a later story."); - /* Fetch is never returning on sandbox-- try - doing a PUT instead of a DELETE with publish option - to work around, or contact dev ops. - STUD-1860 - window.crazyAjaxHandler = xblockInfo.fetch({ - complete: function(a, b, c) { - debugger; - } - }); - renderPage(); - */ - }); - } - }, - secondary: { - text: gettext("Cancel"), - click: function (view) { - view.hide(); - } - } + this.confirmThenRunOperation(gettext("Discard Changes"), + gettext("Are you sure you want to discard changes and revert to the last published version?"), + gettext("Discard Changes"), + function () { + that.runOperationShowingMessage(gettext('Discarding Changes…'), + function () { + return xblockInfo.save({publish: 'discard_changes'}, {patch: true}); + }).always(function() { + xblockInfo.set("publish", null); + }).done(function () { + renderPage(); + }); } - }).show(); + ); } }); + /** * PublishHistory displays when and by whom the xblock was last published, if it ever was. */ diff --git a/common/test/acceptance/pages/studio/container.py b/common/test/acceptance/pages/studio/container.py index df0d25a859..4a89a870c2 100644 --- a/common/test/acceptance/pages/studio/container.py +++ b/common/test/acceptance/pages/studio/container.py @@ -88,22 +88,13 @@ class ContainerPage(PageObject): """ return self.q(css='.action-publish').first - # def discard_changes(self): - # """ - # Discards draft changes and reloads the page. - # NOT YET IMPLEMENTED-- part of future story - # """ - # - # self.q(css='.action-discard').first.click() - # - # # TODO: work with Jay/Christine on this. I can't find something to wait on - # # that guarantees the button will be clickable. - # time.sleep(2) - # - # self.q(css='a.button.action-primary').first.click() - # self.wait_for_ajax() - # - # return ContainerPage(self.browser, self.locator).visit() + def discard_changes(self): + """ + Discards draft changes (which will then re-render the page). + """ + click_css(self, 'a.action-discard', 0, require_notification=False) + self.q(css='a.button.action-primary').first.click() + self.wait_for_ajax() def view_published_version(self): """ diff --git a/common/test/acceptance/tests/test_studio_container.py b/common/test/acceptance/tests/test_studio_container.py index 18276cbe67..55a047d3f3 100644 --- a/common/test/acceptance/tests/test_studio_container.py +++ b/common/test/acceptance/tests/test_studio_container.py @@ -375,6 +375,8 @@ class UnitPublishingTest(ContainerBase): """ Tests of the publishing control and related widgets on the Unit page. """ + PUBLISHED_STATUS = "Publishing Status\nPublished" + DRAFT_STATUS = "Publishing Status\nDraft (Unpublished changes)" def setup_fixtures(self): """ @@ -407,26 +409,26 @@ class UnitPublishingTest(ContainerBase): Test the state changes when a published unit has draft changes. """ unit = self.go_to_unit_page() - self.assertEqual("Publishing Status\nPublished", unit.publish_title) + self.assertEqual(self.PUBLISHED_STATUS, unit.publish_title) # Should not be able to click on Publish action -- but I don't know how to test that it is not clickable. # TODO: continue discussion with Muhammad and Jay about this. # Add a component to the page so it will have unpublished changes. add_discussion(unit) - self.assertEqual("Publishing Status\nDraft (Unpublished changes)", unit.publish_title) + self.assertEqual(self.DRAFT_STATUS, unit.publish_title) unit.publish_action.click() unit.wait_for_ajax() - self.assertEqual("Publishing Status\nPublished", unit.publish_title) + self.assertEqual(self.PUBLISHED_STATUS, unit.publish_title) - # TODO: part of future story. - # def test_discard_changes(self): - # """ - # Test the state after discard changes. - # """ - # unit = self.go_to_unit_page() - # add_discussion(unit) - # unit = unit.discard_changes() - # self.assertEqual("Publishing Status\nPublished", unit.publish_title) + def test_discard_changes(self): + """ + Test the state after discard changes. + """ + unit = self.go_to_unit_page() + add_discussion(unit) + self.assertEqual(self.DRAFT_STATUS, unit.publish_title) + unit.discard_changes() + self.assertEqual(self.PUBLISHED_STATUS, unit.publish_title) def test_view_live_no_changes(self): """