Merge pull request #23718 from edx/aakbar/course_update_items_fix

add improved re-write for course updates

cms/djangoapps/contentstore/course_info_model.py

@@ -57,20 +57,26 @@ def update_course_updates(location, update, passed_id=None, user=None):
         course_updates = modulestore().create_item(user.id, location.course_key, location.block_type, location.block_id)
 
     course_update_items = list(reversed(get_course_update_items(course_updates)))
+    course_update_dict = None
 
     if passed_id is not None:
         passed_index = _get_index(passed_id)
-        # oldest update at start of list
-        if 0 < passed_index <= len(course_update_items):
-            course_update_dict = course_update_items[passed_index - 1]
-            course_update_dict["date"] = update["date"]
-            course_update_dict["content"] = update["content"]
-            course_update_items[passed_index - 1] = course_update_dict
-        else:
+
+        # if passed_index in course_update_items_ids:
+        for course_update_item in course_update_items:
+            if course_update_item["id"] == passed_index:
+                course_update_dict = course_update_item
+                course_update_item["date"] = update["date"]
+                course_update_item["content"] = update["content"]
+                break
+        if course_update_dict is None:
             return HttpResponseBadRequest(_("Invalid course update id."))
     else:
+        course_update_items_ids = [course_update_item['id'] for course_update_item in course_update_items]
+
         course_update_dict = {
-            "id": len(course_update_items) + 1,
+            # if no course updates then the id will be 1 otherwise maxid + 1
+            "id": max(course_update_items_ids) + 1 if course_update_items_ids else 1,
             "date": update["date"],
             "content": update["content"],
             "status": CourseInfoBlock.STATUS_VISIBLE
@@ -131,17 +137,15 @@ def delete_course_update(location, update, passed_id, user):
     passed_index = _get_index(passed_id)
 
     # delete update item from given index
-    if 0 < passed_index <= len(course_update_items):
-        course_update_item = course_update_items[passed_index - 1]
-        # soft delete course update item
-        course_update_item["status"] = CourseInfoBlock.STATUS_DELETED
-        course_update_items[passed_index - 1] = course_update_item
+    for course_update_item in course_update_items:
+        if course_update_item["id"] == passed_index:
+            # soft delete course update item
+            course_update_item["status"] = CourseInfoBlock.STATUS_DELETED
+            # update db record
+            save_course_update_items(location, course_updates, course_update_items, user)
+            return _get_visible_update(course_update_items)
 
-        # update db record
-        save_course_update_items(location, course_updates, course_update_items, user)
-        return _get_visible_update(course_update_items)
-    else:
-        return HttpResponseBadRequest(_("Invalid course update id."))
+    return HttpResponseBadRequest(_("Invalid course update id."))
 
 
 def _get_index(passed_id=None):

cms/djangoapps/contentstore/views/tests/test_course_updates.py

@@ -11,6 +11,7 @@ from opaque_keys.edx.keys import UsageKey
 
 from contentstore.tests.test_course_settings import CourseTestCase
 from contentstore.utils import reverse_course_url, reverse_usage_url
+from openedx.core.lib.xblock_utils import get_course_update_items
 from xmodule.modulestore.django import modulestore
 
 
@@ -280,3 +281,42 @@ class CourseUpdateTest(CourseTestCase):
 
         payload = json.loads(resp.content.decode('utf-8'))
         self.assertHTMLEqual(payload['data'], content)
+
+    def test_course_update_id(self):
+        """
+        Test that a user can successfully update a course update without a sequential ids
+        """
+        # create two course updates
+        self.post_course_update()
+        self.post_course_update()
+
+        updates_location = self.course.id.make_usage_key('course_info', 'updates')
+        self.assertTrue(isinstance(updates_location, UsageKey))
+        self.assertEqual(updates_location.block_id, u'updates')
+
+        course_updates = modulestore().get_item(updates_location)
+        course_update_items = list(reversed(get_course_update_items(course_updates)))
+
+        # Delete the course update with id 1
+        course_update_items = [
+            course_update_item for course_update_item in course_update_items if course_update_item.get('id') != 1
+        ]
+
+        course_updates.items = course_update_items
+        course_updates.data = ""
+
+        # update db record
+        modulestore().update_item(course_updates, self.user.id)
+
+        update_content = 'Testing'
+        update_date = u"January 23, 2014"
+        course_update_url = self.create_update_url()
+        payload = {'content': update_content, 'date': update_date}
+        resp = self.client.ajax_post(
+            course_update_url + '2', payload, HTTP_X_HTTP_METHOD_OVERRIDE="PUT", REQUEST_METHOD="POST"
+        )
+
+        self.assertHTMLEqual(update_content, json.loads(resp.content.decode('utf-8'))['content'])
+        course_updates = modulestore().get_item(updates_location)
+        del course_updates.items[0]["status"]
+        self.assertEqual(course_updates.items, [{u'date': update_date, u'content': update_content, u'id': 2}])