Discard changes rewrite.

STUD-1860

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.

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,

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 () {
+                    var notificationSpy = edit_helpers.createNotificationSpy(),
+                        renderPageSpy = spyOn(containerPage.xblockPublisher, 'renderPage').andCallThrough(),
+                        numRequests;
+
                     sendDiscardChangesToServer(this);
+                    numRequests = requests.length;
 
-                    var numRequests = requests.length;
-                    create_sinon.respondToDelete(requests);
+                    // 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 () {
+                    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 () {

cms/static/js/views/pages/container_subviews.js

 /**
  * 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.
          */

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):
         """

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):
         """