fixup! course_api multiple value fields.

lms/djangoapps/course_api/blocks/forms.py

@@ -3,33 +3,27 @@
 """
 from django.contrib.auth.models import User
 from django.core.exceptions import ValidationError
-from django.forms import Form, CharField, ChoiceField, Field, IntegerField, MultipleHiddenInput
+from django.forms import Form, CharField, ChoiceField, IntegerField
 from django.http import Http404
 from rest_framework.exceptions import PermissionDenied
 
-from xmodule.modulestore.django import modulestore
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.keys import UsageKey
+from openedx.core.djangoapps.util.forms import MultiValueField
+from xmodule.modulestore.django import modulestore
 
 from .permissions import can_access_other_users_blocks, can_access_users_blocks
 
 
-class ListField(Field):
-    """
-    Field for a list of strings
-    """
-    widget = MultipleHiddenInput
-
-
 class BlockListGetForm(Form):
     """
     A form to validate query parameters in the block list retrieval endpoint
     """
     user = CharField(required=True)  # TODO return all blocks if user is not specified by requesting staff user
     usage_key = CharField(required=True)
-    requested_fields = ListField(required=False)
-    student_view_data = ListField(required=False)
-    block_counts = ListField(required=False)
+    requested_fields = MultiValueField(required=False)
+    student_view_data = MultiValueField(required=False)
+    block_counts = MultiValueField(required=False)
     depth = CharField(required=False)
     nav_depth = IntegerField(required=False, min_value=0)
     return_type = ChoiceField(
@@ -38,8 +32,10 @@ class BlockListGetForm(Form):
     )
 
     def clean_requested_fields(self):
+        requested_fields = self.cleaned_data['requested_fields']
+
         # add default requested_fields
-        return set(self.cleaned_data['requested_fields'] or set()) | {'type', 'display_name'}
+        return (requested_fields or set()) | {'type', 'display_name'}
 
     def clean_depth(self):
         value = self.cleaned_data['depth']
@@ -112,7 +108,8 @@ class BlockListGetForm(Form):
             'nav_depth',
         ]
         for additional_field in additional_requested_fields:
-            if not cleaned_data.get(additional_field) in (None, [], {}):  # allow 0 as a requested value
+            field_value = cleaned_data.get(additional_field)
+            if field_value or field_value == 0:  # allow 0 as a requested value
                 cleaned_data['requested_fields'].add(additional_field)
 
         usage_key = cleaned_data.get('usage_key')

lms/djangoapps/course_api/blocks/tests/test_forms.py

@@ -49,12 +49,12 @@ class TestBlockListGetForm(FormTestMixin, SharedModuleStoreTestCase):
             mutable=True,
         )
         self.cleaned_data = {
-            'block_counts': [],
+            'block_counts': set(),
             'depth': 0,
             'nav_depth': None,
             'return_type': 'dict',
             'requested_fields': {'display_name', 'type'},
-            'student_view_data': [],
+            'student_view_data': set(),
             'usage_key': usage_key,
             'user': self.student,
         }
@@ -181,7 +181,7 @@ class TestBlockListGetForm(FormTestMixin, SharedModuleStoreTestCase):
 
     @ddt.data('block_counts', 'student_view_data')
     def test_higher_order_field(self, field_name):
-        field_value = ['block_type1', 'block_type2']
+        field_value = {'block_type1', 'block_type2'}
         self.form_data.setlist(field_name, field_value)
         self.cleaned_data[field_name] = field_value
         self.cleaned_data['requested_fields'].add(field_name)
@@ -192,7 +192,7 @@ class TestBlockListGetForm(FormTestMixin, SharedModuleStoreTestCase):
         self.form_data.setlist('requested_fields', ['field1', 'field2'])
 
         # add higher order fields
-        block_types_list = ['block_type1', 'block_type2']
+        block_types_list = {'block_type1', 'block_type2'}
         for field_name in ['block_counts', 'student_view_data']:
             self.form_data.setlist(field_name, block_types_list)
             self.cleaned_data[field_name] = block_types_list

lms/djangoapps/course_api/blocks/tests/test_views.py

@@ -3,6 +3,8 @@ Tests for Blocks Views
 """
 
 from django.core.urlresolvers import reverse
+from string import join
+
 from opaque_keys.edx.locator import CourseLocator
 from student.models import CourseEnrollment
 from student.tests.factories import CourseEnrollmentFactory, UserFactory
@@ -53,6 +55,26 @@ class TestBlocksViewMixin(object):
             self.non_orphaned_block_usage_keys,
         )
 
+    requested_fields = ['graded', 'format', 'student_view_multi_device', 'children', 'not_a_field']
+
+    def verify_response_with_requested_fields(self, response):
+        self.verify_response_block_dict(response)
+        for block_key_string, block_data in response.data['blocks'].iteritems():
+            block_key = deserialize_usage_key(block_key_string, self.course_key)
+            xblock = self.store.get_item(block_key)
+
+            self.assert_in_iff('children', block_data, xblock.has_children)
+            self.assert_in_iff('graded', block_data, xblock.graded is not None)
+            self.assert_in_iff('format', block_data, xblock.format is not None)
+            self.assert_true_iff(block_data['student_view_multi_device'], block_data['type'] == 'html')
+            self.assertNotIn('not_a_field', block_data)
+
+            if xblock.has_children:
+                self.assertSetEqual(
+                    set(unicode(child.location) for child in xblock.get_children()),
+                    set(block_data['children']),
+                )
+
     def assert_in_iff(self, member, container, predicate):
         if predicate:
             self.assertIn(member, container)
@@ -145,24 +167,9 @@ class TestBlocksView(TestBlocksViewMixin, SharedModuleStoreTestCase):
 
     def test_requested_fields_param(self):
         response = self.verify_response(
-            params={'requested_fields': ['graded', 'format', 'student_view_multi_device', 'children', 'not_a_field']}
+            params={'requested_fields': self.requested_fields}
         )
-        self.verify_response_block_dict(response)
-        for block_key_string, block_data in response.data['blocks'].iteritems():
-            block_key = deserialize_usage_key(block_key_string, self.course_key)
-            xblock = self.store.get_item(block_key)
-
-            self.assert_in_iff('children', block_data, xblock.has_children)
-            self.assert_in_iff('graded', block_data, xblock.graded is not None)
-            self.assert_in_iff('format', block_data, xblock.format is not None)
-            self.assert_true_iff(block_data['student_view_multi_device'], block_data['type'] == 'html')
-            self.assertNotIn('not_a_field', block_data)
-
-            if xblock.has_children:
-                self.assertSetEqual(
-                    set(unicode(child.location) for child in xblock.get_children()),
-                    set(block_data['children']),
-                )
+        self.verify_response_with_requested_fields(response)
 
 
 class TestBlocksInCourseView(TestBlocksViewMixin, SharedModuleStoreTestCase):
@@ -188,3 +195,18 @@ class TestBlocksInCourseView(TestBlocksViewMixin, SharedModuleStoreTestCase):
 
     def test_invalid_course_id(self):
         self.verify_response(400, params={'course_id': 'invalid_course_id'})
+
+    def test_with_list_field_url(self):
+        url = '{base_url}?course_id={course_id}&user={username}&depth=all'.format(
+            course_id=unicode(self.course_key),
+            base_url=self.url.format(),
+            username=self.user.username,
+        )
+        url += '&requested_fields={0}&requested_fields={1}&requested_fields={2}'.format(
+            self.requested_fields[0],
+            self.requested_fields[1],
+            join(self.requested_fields[1:], ','),
+        )
+        response = self.client.get(url)
+        self.assertEquals(response.status_code, 200)
+        self.verify_response_with_requested_fields(response)

lms/djangoapps/discussion_api/forms.py

@@ -6,26 +6,14 @@ from django.forms import (
     BooleanField,
     CharField,
     ChoiceField,
-    Field,
     Form,
     IntegerField,
-    MultipleHiddenInput,
     NullBooleanField,
 )
 
 from opaque_keys import InvalidKeyError
 from opaque_keys.edx.locator import CourseLocator
-
-
-class TopicIdField(Field):
-    """
-    Field for a list of topic_ids
-    """
-    widget = MultipleHiddenInput
-
-    def validate(self, value):
-        if value and "" in value:
-            raise ValidationError("This field cannot be empty.")
+from openedx.core.djangoapps.util.forms import MultiValueField
 
 
 class _PaginationForm(Form):
@@ -49,7 +37,7 @@ class ThreadListGetForm(_PaginationForm):
     EXCLUSIVE_PARAMS = ["topic_id", "text_search", "following"]
 
     course_id = CharField()
-    topic_id = TopicIdField(required=False)
+    topic_id = MultiValueField(required=False)
     text_search = CharField(required=False)
     following = NullBooleanField(required=False)
     view = ChoiceField(

lms/djangoapps/discussion_api/tests/test_forms.py

@@ -63,7 +63,7 @@ class ThreadListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase):
                 "course_id": CourseLocator.from_string("Foo/Bar/Baz"),
                 "page": 2,
                 "page_size": 13,
-                "topic_id": [],
+                "topic_id": set(),
                 "text_search": "",
                 "following": None,
                 "view": "",
@@ -77,7 +77,7 @@ class ThreadListGetFormTest(FormTestMixin, PaginationTestMixin, TestCase):
         form = self.get_form(expected_valid=True)
         self.assertEqual(
             form.cleaned_data["topic_id"],
-            ["example topic_id", "example 2nd topic_id"],
+            {"example topic_id", "example 2nd topic_id"},
         )
 
     def test_text_search(self):

openedx/core/djangoapps/util/forms.py

+from django.core.exceptions import ValidationError
+from django.forms import Field, MultipleHiddenInput
+
+
+class MultiValueField(Field):
+    """
+    Field class that supports a list of values for a single form field.
+
+    The field values can be specified as:
+        1. a comma-separated-list (foo:bar1,bar2,bar3), or
+        2. a repeated field in a MultiValueDict (foo:bar1, foo:bar2, foo:bar3)
+    """
+    widget = MultipleHiddenInput
+
+    def to_python(self, list_of_string_values):
+        values = super(MultiValueField, self).to_python(list_of_string_values) or set()
+
+        if values:
+            # combine all values if there were multiple specified individually
+            values = ','.join(values)
+
+            # parse them into a set
+            values = set(values.split(',')) if values else set()
+
+        return values
+
+    def validate(self, values):
+        if values and "" in values:
+            raise ValidationError("This field cannot be empty.")