From 5177e994c84da508eb7c4b60d4729a1d2a2432db Mon Sep 17 00:00:00 2001 From: Gabe Mulley Date: Mon, 19 Nov 2018 08:37:48 -0500 Subject: [PATCH] REVE-37: Prevent modifications to feature based enrollment user partitions --- .../contentstore/course_group_config.py | 7 ++- cms/djangoapps/contentstore/views/course.py | 3 ++ .../views/tests/test_group_configurations.py | 54 ++++++++++++++++++- cms/static/js/models/group_configuration.js | 6 ++- .../spec/models/group_configuration_spec.js | 6 ++- .../js/views/pages/group_configurations.js | 5 +- .../xmodule/xmodule/partitions/partitions.py | 10 ++++ .../partition_scheme.py | 12 +---- .../tests/test_partition_scheme.py | 14 ++--- .../content_type_gating/partitions.py | 2 + 10 files changed, 93 insertions(+), 26 deletions(-) diff --git a/cms/djangoapps/contentstore/course_group_config.py b/cms/djangoapps/contentstore/course_group_config.py index f3473d0af9..e06a23be1b 100644 --- a/cms/djangoapps/contentstore/course_group_config.py +++ b/cms/djangoapps/contentstore/course_group_config.py @@ -10,7 +10,7 @@ from contentstore.utils import reverse_usage_url from lms.lib.utils import get_parent_unit from openedx.core.djangoapps.course_groups.partition_scheme import get_cohorted_user_partition from util.db import MYSQL_MAX_INT, generate_int_id -from xmodule.partitions.partitions import MINIMUM_STATIC_PARTITION_ID, UserPartition +from xmodule.partitions.partitions import MINIMUM_STATIC_PARTITION_ID, ReadOnlyUserPartitionError, UserPartition from xmodule.partitions.partitions_service import get_all_partitions_for_course from xmodule.split_test_module import get_split_user_partitions @@ -105,7 +105,10 @@ class GroupConfiguration(object): """ Get user partition for saving in course. """ - return UserPartition.from_json(self.configuration) + try: + return UserPartition.from_json(self.configuration) + except ReadOnlyUserPartitionError: + raise GroupConfigurationsValidationError(_("unable to load this type of group configuration")) @staticmethod def _get_usage_info(course, unit, item, usage_info, group_id, scheme_name=None): diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index cca0c67c01..6e4c8771da 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -88,6 +88,7 @@ from xmodule.error_module import ErrorDescriptor from xmodule.modulestore import EdxJSONEncoder from xmodule.modulestore.django import modulestore from xmodule.modulestore.exceptions import DuplicateCourseError, ItemNotFoundError +from xmodule.partitions.partitions import UserPartition from xmodule.tabs import CourseTab, CourseTabList, InvalidTabsException from .component import ADVANCED_COMPONENT_TYPES @@ -1602,6 +1603,8 @@ def group_configurations_list_handler(request, course_key_string): has_content_groups = False displayable_partitions = [] for partition in all_partitions: + partition['read_only'] = getattr(UserPartition.get_scheme(partition['scheme']), 'read_only', False) + if partition['scheme'] == COHORT_SCHEME: has_content_groups = True displayable_partitions.append(partition) diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index 38aadf2886..ec4fa763ca 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -11,7 +11,8 @@ from operator import itemgetter from contentstore.utils import reverse_course_url, reverse_usage_url from contentstore.course_group_config import GroupConfiguration, CONTENT_GROUP_CONFIGURATION_NAME from contentstore.tests.utils import CourseTestCase -from xmodule.partitions.partitions import Group, UserPartition +from openedx.features.content_type_gating.partitions import CONTENT_GATING_PARTITION_ID +from xmodule.partitions.partitions import Group, UserPartition, ENROLLMENT_TRACK_PARTITION_ID from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory from xmodule.validation import StudioValidation, StudioValidationMessage from xmodule.modulestore.django import modulestore @@ -238,6 +239,7 @@ class GroupConfigurationsBaseTestCase(object): self.assertIn("error", content) +@ddt.ddt class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): """ Test cases for group_configurations_list_handler. @@ -329,7 +331,22 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations self.reload_course() self.assertEqual(len(self.course.user_partitions), 0) + @ddt.data('content_type_gate', 'enrollment_track') + def test_cannot_create_restricted_group_configuration(self, scheme_id): + """ + Test that you cannot create a restricted group configuration. + """ + group_config = dict(GROUP_CONFIGURATION_JSON) + group_config['scheme'] = scheme_id + group_config.setdefault('parameters', {})['course_id'] = unicode(self.course.id) + response = self.client.ajax_post( + self._url(), + data=group_config + ) + self.assertEqual(response.status_code, 400) + +@ddt.ddt class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): """ Test cases for group_configurations_detail_handler. @@ -633,6 +650,41 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio self.assertEqual(len(user_partititons), 2) self.assertEqual(user_partititons[0].name, 'Name 0') + @ddt.data('content_type_gate', 'enrollment_track') + def test_cannot_create_restricted_group_configuration(self, scheme_id): + """ + Test that you cannot create a restricted group configuration. + """ + group_config = dict(GROUP_CONFIGURATION_JSON) + group_config['scheme'] = scheme_id + group_config.setdefault('parameters', {})['course_id'] = unicode(self.course.id) + response = self.client.ajax_post( + self._url(), + data=group_config + ) + self.assertEqual(response.status_code, 400) + + @ddt.data( + ('content_type_gate', CONTENT_GATING_PARTITION_ID), + ('enrollment_track', ENROLLMENT_TRACK_PARTITION_ID), + ) + @ddt.unpack + def test_cannot_edit_restricted_group_configuration(self, scheme_id, partition_id): + """ + Test that you cannot edit a restricted group configuration. + """ + group_config = dict(GROUP_CONFIGURATION_JSON) + group_config['scheme'] = scheme_id + group_config.setdefault('parameters', {})['course_id'] = unicode(self.course.id) + response = self.client.put( + self._url(cid=partition_id), + data=json.dumps(group_config), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest", + ) + self.assertEqual(response.status_code, 400) + @ddt.ddt class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): diff --git a/cms/static/js/models/group_configuration.js b/cms/static/js/models/group_configuration.js index 7211ddd56d..c4d8f9e61d 100644 --- a/cms/static/js/models/group_configuration.js +++ b/cms/static/js/models/group_configuration.js @@ -23,7 +23,8 @@ function(Backbone, _, gettext, GroupModel, GroupCollection) { ]), showGroups: false, editing: false, - usage: [] + usage: [], + read_only: false }; }, @@ -78,7 +79,8 @@ function(Backbone, _, gettext, GroupModel, GroupCollection) { scheme: this.get('scheme'), description: this.get('description'), version: this.get('version'), - groups: this.get('groups').toJSON() + groups: this.get('groups').toJSON(), + read_only: this.get('read_only') }; }, diff --git a/cms/static/js/spec/models/group_configuration_spec.js b/cms/static/js/spec/models/group_configuration_spec.js index 8426d6b376..be1de362d2 100644 --- a/cms/static/js/spec/models/group_configuration_spec.js +++ b/cms/static/js/spec/models/group_configuration_spec.js @@ -101,7 +101,8 @@ define([ name: 'Group 2', usage: [] } - ] + ], + read_only: true }, clientModelSpec = { id: 10, @@ -124,7 +125,8 @@ define([ usage: [] } ], - usage: [] + usage: [], + read_only: true }, model = new GroupConfigurationModel( serverModelSpec, {parse: true} diff --git a/cms/static/js/views/pages/group_configurations.js b/cms/static/js/views/pages/group_configurations.js index 9da77189bc..943628d97c 100644 --- a/cms/static/js/views/pages/group_configurations.js +++ b/cms/static/js/views/pages/group_configurations.js @@ -7,8 +7,7 @@ function($, _, gettext, BasePage, GroupConfigurationsListView, PartitionGroupLis var GroupConfigurationsPage = BasePage.extend({ initialize: function(options) { var currentScheme, - i, - enrollmentScheme = 'enrollment_track'; + i; BasePage.prototype.initialize.call(this); this.experimentsEnabled = options.experimentsEnabled; @@ -26,7 +25,7 @@ function($, _, gettext, BasePage, GroupConfigurationsListView, PartitionGroupLis this.allGroupViewList.push( new PartitionGroupListView({ collection: this.allGroupConfigurations[i].get('groups'), - restrictEditing: currentScheme === enrollmentScheme, + restrictEditing: this.allGroupConfigurations[i].get('read_only'), scheme: currentScheme }) ); diff --git a/common/lib/xmodule/xmodule/partitions/partitions.py b/common/lib/xmodule/xmodule/partitions/partitions.py index f17437ccae..49aa36e2ef 100644 --- a/common/lib/xmodule/xmodule/partitions/partitions.py +++ b/common/lib/xmodule/xmodule/partitions/partitions.py @@ -40,6 +40,13 @@ class NoSuchUserPartitionGroupError(UserPartitionError): pass +class ReadOnlyUserPartitionError(UserPartitionError): + """ + Exception to be raised when attempting to modify a read only partition. + """ + pass + + class Group(namedtuple("Group", "id name")): """ An id and name for a group of students. The id should be unique @@ -199,6 +206,9 @@ class UserPartition(namedtuple("UserPartition", "id name description groups sche if not scheme: raise TypeError("UserPartition dict {0} has unrecognized scheme {1}".format(value, scheme_id)) + if getattr(scheme, 'read_only', False): + raise ReadOnlyUserPartitionError("UserPartition dict {0} uses scheme {1} which is read only".format(value, scheme_id)) + if hasattr(scheme, "create_user_partition"): return scheme.create_user_partition( value["id"], diff --git a/openedx/core/djangoapps/verified_track_content/partition_scheme.py b/openedx/core/djangoapps/verified_track_content/partition_scheme.py index cc87622e4a..6c80898485 100644 --- a/openedx/core/djangoapps/verified_track_content/partition_scheme.py +++ b/openedx/core/djangoapps/verified_track_content/partition_scheme.py @@ -49,22 +49,14 @@ class EnrollmentTrackUserPartition(UserPartition): for mode in CourseMode.modes_for_course(course_key, include_expired=True) ] - def from_json(self): - """ - Because this partition is dynamic, `from_json` is not supported. - `to_json` is supported, but shouldn't be used to persist this partition - within the course itself (used by Studio for sending data to front-end code) - - Calling this method will raise a TypeError. - """ - raise TypeError("Because EnrollmentTrackUserPartition is a dynamic partition, 'from_json' is not supported.") - class EnrollmentTrackPartitionScheme(object): """ This scheme uses learner enrollment tracks to map learners into partition groups. """ + read_only = True + @classmethod def get_group_for_user(cls, course_key, user, user_partition, **kwargs): # pylint: disable=unused-argument """ diff --git a/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py b/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py index a1062b6c7c..cae993d869 100644 --- a/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py +++ b/openedx/core/djangoapps/verified_track_content/tests/test_partition_scheme.py @@ -2,17 +2,18 @@ Tests for verified_track_content/partition_scheme.py. """ from datetime import datetime, timedelta + import pytz -from ..partition_scheme import EnrollmentTrackPartitionScheme, EnrollmentTrackUserPartition, ENROLLMENT_GROUP_IDS -from ..models import VerifiedTrackCohortedCourse from course_modes.models import CourseMode - from student.models import CourseEnrollment from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -from xmodule.partitions.partitions import UserPartition, MINIMUM_STATIC_PARTITION_ID +from xmodule.partitions.partitions import MINIMUM_STATIC_PARTITION_ID, UserPartition, ReadOnlyUserPartitionError + +from ..models import VerifiedTrackCohortedCourse +from ..partition_scheme import ENROLLMENT_GROUP_IDS, EnrollmentTrackPartitionScheme, EnrollmentTrackUserPartition class EnrollmentTrackUserPartitionTest(SharedModuleStoreTestCase): @@ -60,8 +61,9 @@ class EnrollmentTrackUserPartitionTest(SharedModuleStoreTestCase): self.assertEqual('Test partition for segmenting users by enrollment track', user_partition_json['description']) def test_from_json_not_supported(self): - with self.assertRaises(TypeError): - EnrollmentTrackUserPartition.from_json() + user_partition_json = create_enrollment_track_partition(self.course).to_json() + with self.assertRaises(ReadOnlyUserPartitionError): + UserPartition.from_json(user_partition_json) def test_group_ids(self): """ diff --git a/openedx/features/content_type_gating/partitions.py b/openedx/features/content_type_gating/partitions.py index 7be8236ad5..66dc01ab67 100644 --- a/openedx/features/content_type_gating/partitions.py +++ b/openedx/features/content_type_gating/partitions.py @@ -127,6 +127,8 @@ class ContentTypeGatingPartitionScheme(object): LIMITED_ACCESS = Group(CONTENT_TYPE_GATE_GROUP_IDS['limited_access'], 'Limited-access Users') FULL_ACCESS = Group(CONTENT_TYPE_GATE_GROUP_IDS['full_access'], 'Full-access Users') + read_only = True + @classmethod def get_group_for_user(cls, course_key, user, user_partition, **kwargs): # pylint: disable=unused-argument """