From 83db659f948e60884b87926bbbb07bfe8cec8484 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Mon, 19 May 2014 12:47:43 +0500 Subject: [PATCH 1/3] create draft request gets already created version if it exists STUD-1616 --- cms/djangoapps/contentstore/views/item.py | 8 +++-- .../contentstore/views/tests/test_item.py | 33 +++++++++++++++++++ 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 6916120caf..45000ac9e9 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -21,7 +21,7 @@ from xblock.fragment import Fragment import xmodule from xmodule.modulestore.django import modulestore, loc_mapper -from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError +from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, DuplicateItemError from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.locator import BlockUsageLocator from xmodule.modulestore import Location @@ -314,7 +314,11 @@ def _save_item(request, usage_loc, item_location, data=None, children=None, meta elif publish == 'create_draft': # This recursively clones the existing item location to a draft location (the draft is # implicit, because modulestore is a Draft modulestore) - _xmodule_recurse(existing_item, lambda i: modulestore().convert_to_draft(i.location)) + try: + _xmodule_recurse(existing_item, lambda i: modulestore().convert_to_draft(i.location)) + except DuplicateItemError: + # Since there is already a draft version of this module so no need to create another + pass if data: # TODO Allow any scope.content fields not just "data" (exactly like the get below this) diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 5f4f3a46df..9b105b983b 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -632,6 +632,39 @@ class TestEditItem(ItemTest): draft = self.get_item_from_modulestore(self.problem_locator, True) self.assertEqual(draft.due, datetime(2077, 10, 10, 4, 0, tzinfo=UTC)) + def test_create_draft_with_multiple_requests(self): + """ + Create a draft request returns already created version if it exists. + """ + # Make problem public. + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'make_public'} + ) + self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False)) + # Now make it draft, which means both versions will exist. + self.client.ajax_post( + self.problem_update_url, + data={ + 'publish': 'create_draft' + } + ) + self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False)) + draft_1 = self.get_item_from_modulestore(self.problem_locator, True) + self.assertIsNotNone(draft_1) + + # Now check that when a user sends request to create a draft when there is already a draft version then + # user gets that already created draft instead of getting DuplicateItemError exception. + self.client.ajax_post( + self.problem_update_url, + data={ + 'publish': 'create_draft' + } + ) + draft_2 = self.get_item_from_modulestore(self.problem_locator, True) + self.assertIsNotNone(draft_2) + self.assertEqual(draft_1, draft_2) + def test_published_and_draft_contents_with_update(self): """ Create a draft and publish it then modify the draft and check that published content is not modified """ From 6bdf984e8d6585353629658701ad7a3825fc3f06 Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Wed, 21 May 2014 16:34:19 +0500 Subject: [PATCH 2/3] update _xmodule_recurse to ignore some exception --- cms/djangoapps/contentstore/views/helpers.py | 18 +++++++- cms/djangoapps/contentstore/views/item.py | 18 +++++--- .../contentstore/views/tests/test_item.py | 43 ++++++++++++++++++- 3 files changed, 69 insertions(+), 10 deletions(-) diff --git a/cms/djangoapps/contentstore/views/helpers.py b/cms/djangoapps/contentstore/views/helpers.py index 2141ecc3c5..466ed178f4 100644 --- a/cms/djangoapps/contentstore/views/helpers.py +++ b/cms/djangoapps/contentstore/views/helpers.py @@ -4,6 +4,7 @@ from django.http import HttpResponse from django.shortcuts import redirect from edxmako.shortcuts import render_to_string, render_to_response from xmodule.modulestore.django import loc_mapper, modulestore +from xmodule.modulestore.exceptions import DuplicateItemError, ItemNotFoundError __all__ = ['edge', 'event', 'landing'] @@ -36,9 +37,22 @@ def render_from_lms(template_name, dictionary, context=None, namespace='main'): return render_to_string(template_name, dictionary, context, namespace="lms." + namespace) -def _xmodule_recurse(item, action): +def _xmodule_recurse(item, action, ignore_exception=None): + """ + Recursively apply provided action on item and its children + + ignore_duplicate_exception (str): A optional argument; when set to a certain value then ignores the corresponding + exception raised while xmodule recursion + """ for child in item.get_children(): - _xmodule_recurse(child, action) + _xmodule_recurse(child, action, ignore_exception) + + if ignore_exception in ['ItemNotFoundError', 'DuplicateItemError']: + # In case of valid provided exception; ignore it and continue recursion + try: + return action(item) + except (ItemNotFoundError, DuplicateItemError): + return action(item) diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 45000ac9e9..4ed5e29a75 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -21,7 +21,7 @@ from xblock.fragment import Fragment import xmodule from xmodule.modulestore.django import modulestore, loc_mapper -from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, DuplicateItemError +from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.locator import BlockUsageLocator from xmodule.modulestore import Location @@ -310,15 +310,19 @@ def _save_item(request, usage_loc, item_location, data=None, children=None, meta if publish: if publish == 'make_private': - _xmodule_recurse(existing_item, lambda i: modulestore().unpublish(i.location)) + _xmodule_recurse( + existing_item, + lambda i: modulestore().unpublish(i.location), + ignore_exception='ItemNotFoundError' + ) elif publish == 'create_draft': # This recursively clones the existing item location to a draft location (the draft is # implicit, because modulestore is a Draft modulestore) - try: - _xmodule_recurse(existing_item, lambda i: modulestore().convert_to_draft(i.location)) - except DuplicateItemError: - # Since there is already a draft version of this module so no need to create another - pass + _xmodule_recurse( + existing_item, + lambda i: modulestore().convert_to_draft(i.location), + ignore_exception='DuplicateItemError' + ) if data: # TODO Allow any scope.content fields not just "data" (exactly like the get below this) diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index 9b105b983b..2939743a4b 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -654,7 +654,7 @@ class TestEditItem(ItemTest): self.assertIsNotNone(draft_1) # Now check that when a user sends request to create a draft when there is already a draft version then - # user gets that already created draft instead of getting DuplicateItemError exception. + # user gets that already created draft instead of getting 'DuplicateItemError' exception. self.client.ajax_post( self.problem_update_url, data={ @@ -665,6 +665,47 @@ class TestEditItem(ItemTest): self.assertIsNotNone(draft_2) self.assertEqual(draft_1, draft_2) + + def test_make_private_with_multiple_requests(self): + """ + Make private requests gets proper response even if xmodule is already made private. + """ + # Make problem public. + self.client.ajax_post( + self.problem_update_url, + data={'publish': 'make_public'} + ) + self.assertIsNotNone(self.get_item_from_modulestore(self.problem_locator, False)) + + # Now make it private, and check that its published version not exists + resp = self.client.ajax_post( + self.problem_update_url, + data={ + 'publish': 'make_private' + } + ) + self.assertEqual(resp.status_code, 200) + with self.assertRaises(ItemNotFoundError): + self.get_item_from_modulestore(self.problem_locator, False) + draft_1 = self.get_item_from_modulestore(self.problem_locator, True) + self.assertIsNotNone(draft_1) + + # Now check that when a user sends request to make it private when it already is private then + # user gets that private version instead of getting 'ItemNotFoundError' exception. + self.client.ajax_post( + self.problem_update_url, + data={ + 'publish': 'make_private' + } + ) + self.assertEqual(resp.status_code, 200) + with self.assertRaises(ItemNotFoundError): + self.get_item_from_modulestore(self.problem_locator, False) + draft_2 = self.get_item_from_modulestore(self.problem_locator, True) + self.assertIsNotNone(draft_2) + self.assertEqual(draft_1, draft_2) + + def test_published_and_draft_contents_with_update(self): """ Create a draft and publish it then modify the draft and check that published content is not modified """ From 6d0e2b215c2a69cb32b3c2108bc67926394c745c Mon Sep 17 00:00:00 2001 From: zubair-arbi Date: Wed, 21 May 2014 18:31:19 +0500 Subject: [PATCH 3/3] update _xmodule_recurse method --- cms/djangoapps/contentstore/views/helpers.py | 19 +++++++------------ cms/djangoapps/contentstore/views/item.py | 6 +++--- 2 files changed, 10 insertions(+), 15 deletions(-) diff --git a/cms/djangoapps/contentstore/views/helpers.py b/cms/djangoapps/contentstore/views/helpers.py index 466ed178f4..06b9258017 100644 --- a/cms/djangoapps/contentstore/views/helpers.py +++ b/cms/djangoapps/contentstore/views/helpers.py @@ -4,7 +4,6 @@ from django.http import HttpResponse from django.shortcuts import redirect from edxmako.shortcuts import render_to_string, render_to_response from xmodule.modulestore.django import loc_mapper, modulestore -from xmodule.modulestore.exceptions import DuplicateItemError, ItemNotFoundError __all__ = ['edge', 'event', 'landing'] @@ -37,24 +36,20 @@ def render_from_lms(template_name, dictionary, context=None, namespace='main'): return render_to_string(template_name, dictionary, context, namespace="lms." + namespace) -def _xmodule_recurse(item, action, ignore_exception=None): +def _xmodule_recurse(item, action, ignore_exception=()): """ Recursively apply provided action on item and its children - ignore_duplicate_exception (str): A optional argument; when set to a certain value then ignores the corresponding - exception raised while xmodule recursion + ignore_exception (Exception Object): A optional argument; when passed ignores the corresponding + exception raised during xmodule recursion, """ for child in item.get_children(): _xmodule_recurse(child, action, ignore_exception) - if ignore_exception in ['ItemNotFoundError', 'DuplicateItemError']: - # In case of valid provided exception; ignore it and continue recursion - try: - return action(item) - except (ItemNotFoundError, DuplicateItemError): - return - - action(item) + try: + return action(item) + except ignore_exception: + return def get_parent_xblock(xblock): diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index 4ed5e29a75..60753f33e3 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -21,7 +21,7 @@ from xblock.fragment import Fragment import xmodule from xmodule.modulestore.django import modulestore, loc_mapper -from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError +from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError, DuplicateItemError from xmodule.modulestore.inheritance import own_metadata from xmodule.modulestore.locator import BlockUsageLocator from xmodule.modulestore import Location @@ -313,7 +313,7 @@ def _save_item(request, usage_loc, item_location, data=None, children=None, meta _xmodule_recurse( existing_item, lambda i: modulestore().unpublish(i.location), - ignore_exception='ItemNotFoundError' + ignore_exception=ItemNotFoundError ) elif publish == 'create_draft': # This recursively clones the existing item location to a draft location (the draft is @@ -321,7 +321,7 @@ def _save_item(request, usage_loc, item_location, data=None, children=None, meta _xmodule_recurse( existing_item, lambda i: modulestore().convert_to_draft(i.location), - ignore_exception='DuplicateItemError' + ignore_exception=DuplicateItemError ) if data: