From d4e82424775e2e7d1ab190acda29bed703f8dbee Mon Sep 17 00:00:00 2001
From: Braden MacDonald <braden@opencraft.com>
Date: Wed, 10 Dec 2014 13:02:38 -0800
Subject: [PATCH] Friendly error message when library key is invalid

---
 cms/djangoapps/contentstore/views/item.py     |  5 ++--
 .../contentstore/views/tests/test_item.py     | 23 +++++++++++++++++++
 .../xmodule/xmodule/library_content_module.py | 17 +++++++++++---
 3 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py
index 847d9c91af..3ed4305a2c 100644
--- a/cms/djangoapps/contentstore/views/item.py
+++ b/cms/djangoapps/contentstore/views/item.py
@@ -426,8 +426,9 @@ def _save_xblock(user, xblock, data=None, children_strings=None, metadata=None,
                     else:
                         try:
                             value = field.from_json(value)
-                        except ValueError:
-                            return JsonResponse({"error": "Invalid data"}, 400)
+                        except ValueError as verr:
+                            reason = _("Invalid data ({details})").format(details=verr.message) if verr.message else _("Invalid data")
+                            return JsonResponse({"error": reason}, 400)
                         field.write_to(xblock, value)
 
         # update the xblock and call any xblock callbacks
diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py
index d6d913a594..6b595ae007 100644
--- a/cms/djangoapps/contentstore/views/tests/test_item.py
+++ b/cms/djangoapps/contentstore/views/tests/test_item.py
@@ -894,6 +894,29 @@ class TestEditItem(ItemTest):
         self._verify_published_with_draft(unit_usage_key)
         self._verify_published_with_draft(html_usage_key)
 
+    def test_field_value_errors(self):
+        """
+        Test that if the user's input causes a ValueError on an XBlock field,
+        we provide a friendly error message back to the user.
+        """
+        response = self.create_xblock(parent_usage_key=self.seq_usage_key, category='video')
+        video_usage_key = self.response_usage_key(response)
+        update_url = reverse_usage_url('xblock_handler', video_usage_key)
+
+        response = self.client.ajax_post(
+            update_url,
+            data={
+                'id': unicode(video_usage_key),
+                'metadata': {
+                    'saved_video_position': "Not a valid relative time",
+                },
+            }
+        )
+        self.assertEqual(response.status_code, 400)
+        parsed = json.loads(response.content)
+        self.assertIn("error", parsed)
+        self.assertIn("Incorrect RelativeTime value", parsed["error"])  # See xmodule/fields.py
+
 
 class TestEditSplitModule(ItemTest):
     """
diff --git a/common/lib/xmodule/xmodule/library_content_module.py b/common/lib/xmodule/xmodule/library_content_module.py
index 2d1e386847..4233429e0b 100644
--- a/common/lib/xmodule/xmodule/library_content_module.py
+++ b/common/lib/xmodule/xmodule/library_content_module.py
@@ -1,11 +1,12 @@
 """
 LibraryContent: The XBlock used to include blocks from a library in a course.
 """
-from bson.objectid import ObjectId
+from bson.objectid import ObjectId, InvalidId
 from collections import namedtuple
 from copy import copy
 import hashlib
 from .mako_module import MakoModuleDescriptor
+from opaque_keys import InvalidKeyError
 from opaque_keys.edx.locator import LibraryLocator
 import random
 from webob import Response
@@ -46,7 +47,10 @@ class LibraryVersionReference(namedtuple("LibraryVersionReference", "library_id
                 version = library_id.version_guid
             library_id = library_id.for_version(None)
         if version and not isinstance(version, ObjectId):
-            version = ObjectId(version)
+            try:
+                version = ObjectId(version)
+            except InvalidId:
+                raise ValueError(version)
         return super(LibraryVersionReference, cls).__new__(cls, library_id, version)
 
     @staticmethod
@@ -86,7 +90,14 @@ class LibraryList(List):
                 val = val.strip(' []')
                 parts = val.rsplit(',', 1)
                 val = [parts[0], parts[1] if len(parts) > 1 else None]
-            return LibraryVersionReference.from_json(val)
+            try:
+                return LibraryVersionReference.from_json(val)
+            except InvalidKeyError:
+                try:
+                    friendly_val = val[0]  # Just get the library key part, not the version
+                except IndexError:
+                    friendly_val = unicode(val)
+                raise ValueError(_('"{value}" is not a valid library ID.').format(value=friendly_val))
         return [parse(v) for v in values]
 
     def to_json(self, values):