diff --git a/CHANGELOG.rst b/CHANGELOG.rst index b12fc47216..f9d007c62e 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -5,6 +5,8 @@ These are notable changes in edx-platform. This is a rolling list of changes, in roughly chronological order, most recent first. Add your entries at or near the top. Include a label indicating the component affected. +Studio/LMS: Implement cohorted courseware. TNL-648 + LMS: Student Notes: Eventing for Student Notes. TNL-931 LMS: Student Notes: Add course structure view. TNL-762 diff --git a/cms/djangoapps/contentstore/tests/test_utils.py b/cms/djangoapps/contentstore/tests/test_utils.py index 8698feb6d9..76b809a96d 100644 --- a/cms/djangoapps/contentstore/tests/test_utils.py +++ b/cms/djangoapps/contentstore/tests/test_utils.py @@ -423,3 +423,63 @@ class InheritedStaffLockTest(StaffLockTest): def test_no_inheritance_for_orphan(self): """Tests that an orphaned xblock does not inherit staff lock""" self.assertFalse(utils.ancestor_has_staff_lock(self.orphan)) + + +class GroupVisibilityTest(CourseTestCase): + """ + Test content group access rules. + """ + def setUp(self): + super(GroupVisibilityTest, self).setUp() + + chapter = ItemFactory.create(category='chapter', parent_location=self.course.location) + sequential = ItemFactory.create(category='sequential', parent_location=chapter.location) + vertical = ItemFactory.create(category='vertical', parent_location=sequential.location) + html = ItemFactory.create(category='html', parent_location=vertical.location) + problem = ItemFactory.create( + category='problem', parent_location=vertical.location, data="" + ) + self.sequential = self.store.get_item(sequential.location) + self.vertical = self.store.get_item(vertical.location) + self.html = self.store.get_item(html.location) + self.problem = self.store.get_item(problem.location) + + def set_group_access(self, xblock, value): + """ Sets group_access to specified value and calls update_item to persist the change. """ + xblock.group_access = value + self.store.update_item(xblock, self.user.id) + + def test_no_visibility_set(self): + """ Tests when group_access has not been set on anything. """ + + def verify_all_components_visible_to_all(): # pylint: disable=invalid-name + """ Verifies when group_access has not been set on anything. """ + for item in (self.sequential, self.vertical, self.html, self.problem): + self.assertFalse(utils.has_children_visible_to_specific_content_groups(item)) + self.assertFalse(utils.is_visible_to_specific_content_groups(item)) + + verify_all_components_visible_to_all() + + # Test with group_access set to Falsey values. + self.set_group_access(self.vertical, {1: []}) + self.set_group_access(self.html, {2: None}) + + verify_all_components_visible_to_all() + + def test_sequential_and_problem_have_group_access(self): + """ Tests when group_access is set on a few different components. """ + self.set_group_access(self.sequential, {1: [0]}) + # This is a no-op. + self.set_group_access(self.vertical, {1: []}) + self.set_group_access(self.problem, {2: [3, 4]}) + + # Note that "has_children_visible_to_specific_content_groups" only checks immediate children. + self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.sequential)) + self.assertTrue(utils.has_children_visible_to_specific_content_groups(self.vertical)) + self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.html)) + self.assertFalse(utils.has_children_visible_to_specific_content_groups(self.problem)) + + self.assertTrue(utils.is_visible_to_specific_content_groups(self.sequential)) + self.assertFalse(utils.is_visible_to_specific_content_groups(self.vertical)) + self.assertFalse(utils.is_visible_to_specific_content_groups(self.html)) + self.assertTrue(utils.is_visible_to_specific_content_groups(self.problem)) diff --git a/cms/djangoapps/contentstore/utils.py b/cms/djangoapps/contentstore/utils.py index 07a8a8cac1..41fca796f4 100644 --- a/cms/djangoapps/contentstore/utils.py +++ b/cms/djangoapps/contentstore/utils.py @@ -179,6 +179,36 @@ def is_currently_visible_to_students(xblock): return True +def has_children_visible_to_specific_content_groups(xblock): + """ + Returns True if this xblock has children that are limited to specific content groups. + Note that this method is not recursive (it does not check grandchildren). + """ + if not xblock.has_children: + return False + + for child in xblock.get_children(): + if is_visible_to_specific_content_groups(child): + return True + + return False + + +def is_visible_to_specific_content_groups(xblock): + """ + Returns True if this xblock has visibility limited to specific content groups. + """ + if not xblock.group_access: + return False + for __, value in xblock.group_access.iteritems(): + # value should be a list of group IDs. If it is an empty list or None, the xblock is visible + # to all groups in that particular partition. So if value is a truthy value, the xblock is + # restricted in some way. + if value: + return True + return False + + def find_release_date_source(xblock): """ Finds the ancestor of xblock that set its release date. diff --git a/cms/djangoapps/contentstore/views/component.py b/cms/djangoapps/contentstore/views/component.py index 90f1dde267..ea4412e7b7 100644 --- a/cms/djangoapps/contentstore/views/component.py +++ b/cms/djangoapps/contentstore/views/component.py @@ -21,7 +21,7 @@ from xblock.runtime import Mixologist from contentstore.utils import get_lms_link_for_item from contentstore.views.helpers import get_parent_xblock, is_unit, xblock_type_display_name -from contentstore.views.item import create_xblock_info +from contentstore.views.item import create_xblock_info, add_container_page_publishing_info from opaque_keys.edx.keys import UsageKey @@ -186,8 +186,9 @@ def container_handler(request, usage_key_string): # about the block's ancestors and siblings for use by the Unit Outline. xblock_info = create_xblock_info(xblock, include_ancestor_info=is_unit_page) - # Create the link for preview. - preview_lms_base = settings.FEATURES.get('PREVIEW_LMS_BASE') + if is_unit_page: + add_container_page_publishing_info(xblock, xblock_info) + # need to figure out where this item is in the list of children as the # preview will need this index = 1 diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index f8b4de4781..b25cf6895f 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -16,6 +16,7 @@ from django.core.urlresolvers import reverse from django.http import HttpResponseBadRequest, HttpResponseNotFound, HttpResponse, Http404 from util.json_request import JsonResponse, JsonResponseBadRequest from util.date_utils import get_default_time_display +from util.db import generate_int_id, MYSQL_MAX_INT from edxmako.shortcuts import render_to_response from xmodule.course_module import DEFAULT_START_DATE @@ -29,6 +30,7 @@ from xmodule.modulestore.exceptions import ItemNotFoundError, DuplicateCourseErr from opaque_keys import InvalidKeyError from opaque_keys.edx.locations import Location from opaque_keys.edx.keys import CourseKey +from openedx.core.djangoapps.course_groups.partition_scheme import get_cohorted_user_partition from django_future.csrf import ensure_csrf_cookie from contentstore.course_info_model import get_course_updates, update_course_updates, delete_course_update @@ -70,7 +72,15 @@ from course_action_state.models import CourseRerunState, CourseRerunUIStateManag from course_action_state.managers import CourseActionStateItemNotFoundError from microsite_configuration import microsite from xmodule.course_module import CourseFields +from xmodule.split_test_module import get_split_user_partitions +MINIMUM_GROUP_ID = 100 + +# Note: the following content group configuration strings are not +# translated since they are not visible to users. +CONTENT_GROUP_CONFIGURATION_DESCRIPTION = 'The groups in this configuration can be mapped to cohort groups in the LMS.' + +CONTENT_GROUP_CONFIGURATION_NAME = 'Content Group Configuration' __all__ = ['course_info_handler', 'course_handler', 'course_listing', 'course_info_update_handler', @@ -1252,23 +1262,16 @@ class GroupConfiguration(object): if len(self.configuration.get('groups', [])) < 1: raise GroupConfigurationsValidationError(_("must have at least one group")) - def generate_id(self, used_ids): - """ - Generate unique id for the group configuration. - If this id is already used, we generate new one. - """ - cid = random.randint(100, 10 ** 12) - - while cid in used_ids: - cid = random.randint(100, 10 ** 12) - - return cid - def assign_id(self, configuration_id=None): """ Assign id for the json representation of group configuration. """ - self.configuration['id'] = int(configuration_id) if configuration_id else self.generate_id(self.get_used_ids()) + if configuration_id: + self.configuration['id'] = int(configuration_id) + else: + self.configuration['id'] = generate_int_id( + MINIMUM_GROUP_ID, MYSQL_MAX_INT, GroupConfiguration.get_used_ids(self.course) + ) def assign_group_ids(self): """ @@ -1278,14 +1281,15 @@ class GroupConfiguration(object): # Assign ids to every group in configuration. for group in self.configuration.get('groups', []): if group.get('id') is None: - group["id"] = self.generate_id(used_ids) + group["id"] = generate_int_id(MINIMUM_GROUP_ID, MYSQL_MAX_INT, used_ids) used_ids.append(group["id"]) - def get_used_ids(self): + @staticmethod + def get_used_ids(course): """ Return a list of IDs that already in use. """ - return set([p.id for p in self.course.user_partitions]) + return set([p.id for p in course.user_partitions]) def get_user_partition(self): """ @@ -1296,21 +1300,19 @@ class GroupConfiguration(object): @staticmethod def get_usage_info(course, store): """ - Get usage information for all Group Configurations. + Get usage information for all Group Configurations currently referenced by a split_test instance. """ split_tests = store.get_items(course.id, qualifiers={'category': 'split_test'}) return GroupConfiguration._get_usage_info(store, course, split_tests) @staticmethod - def add_usage_info(course, store): + def get_split_test_partitions_with_usage(course, store): """ - Add usage information to group configurations jsons in course. - - Returns json of group configurations updated with usage information. + Returns json split_test group configurations updated with usage information. """ usage_info = GroupConfiguration.get_usage_info(course, store) configurations = [] - for partition in course.user_partitions: + for partition in get_split_user_partitions(course.user_partitions): configuration = partition.to_json() configuration['usage'] = usage_info.get(partition.id, []) configurations.append(configuration) @@ -1384,6 +1386,26 @@ class GroupConfiguration(object): configuration_json['usage'] = usage_information.get(configuration.id, []) return configuration_json + @staticmethod + def get_or_create_content_group_configuration(course): + """ + Returns the first user partition from the course which uses the + CohortPartitionScheme, or generates one if no such partition is + found. The created partition is not saved to the course until + the client explicitly creates a group within the partition and + POSTs back. + """ + content_group_configuration = get_cohorted_user_partition(course.id) + if content_group_configuration is None: + content_group_configuration = UserPartition( + id=generate_int_id(MINIMUM_GROUP_ID, MYSQL_MAX_INT, GroupConfiguration.get_used_ids(course)), + name=CONTENT_GROUP_CONFIGURATION_NAME, + description=CONTENT_GROUP_CONFIGURATION_DESCRIPTION, + groups=[], + scheme_id='cohort' + ) + return content_group_configuration + @require_http_methods(("GET", "POST")) @login_required @@ -1405,12 +1427,21 @@ def group_configurations_list_handler(request, course_key_string): if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): group_configuration_url = reverse_course_url('group_configurations_list_handler', course_key) course_outline_url = reverse_course_url('course_handler', course_key) - configurations = GroupConfiguration.add_usage_info(course, store) + should_show_experiment_groups = are_content_experiments_enabled(course) + if should_show_experiment_groups: + experiment_group_configurations = GroupConfiguration.get_split_test_partitions_with_usage(course, store) + else: + experiment_group_configurations = None + content_group_configuration = GroupConfiguration.get_or_create_content_group_configuration( + course + ).to_json() return render_to_response('group_configurations.html', { 'context_course': course, 'group_configuration_url': group_configuration_url, 'course_outline_url': course_outline_url, - 'configurations': configurations if should_show_group_configurations_page(course) else None, + 'experiment_group_configurations': experiment_group_configurations, + 'should_show_experiment_groups': should_show_experiment_groups, + 'content_group_configuration': content_group_configuration }) elif "application/json" in request.META.get('HTTP_ACCEPT'): if request.method == 'POST': @@ -1489,9 +1520,9 @@ def group_configurations_detail_handler(request, course_key_string, group_config return JsonResponse(status=204) -def should_show_group_configurations_page(course): +def are_content_experiments_enabled(course): """ - Returns true if Studio should show the "Group Configurations" page for the specified course. + Returns True if content experiments have been enabled for the course. """ return ( SPLIT_TEST_COMPONENT_TYPE in ADVANCED_COMPONENT_TYPES and diff --git a/cms/djangoapps/contentstore/views/item.py b/cms/djangoapps/contentstore/views/item.py index e390811096..d93ac2c42e 100644 --- a/cms/djangoapps/contentstore/views/item.py +++ b/cms/djangoapps/contentstore/views/item.py @@ -39,7 +39,7 @@ from util.json_request import expect_json, JsonResponse from student.auth import has_studio_write_access, has_studio_read_access from contentstore.utils import find_release_date_source, find_staff_lock_source, is_currently_visible_to_students, \ - ancestor_has_staff_lock + ancestor_has_staff_lock, has_children_visible_to_specific_content_groups from contentstore.views.helpers import is_unit, xblock_studio_url, xblock_primary_child_category, \ xblock_type_display_name, get_parent_xblock from contentstore.views.preview import get_preview_fragment @@ -48,8 +48,11 @@ from models.settings.course_grading import CourseGradingModel from cms.lib.xblock.runtime import handler_url, local_resource_url from opaque_keys.edx.keys import UsageKey, CourseKey from opaque_keys.edx.locator import LibraryUsageLocator +from cms.lib.xblock.authoring_mixin import VISIBILITY_VIEW -__all__ = ['orphan_handler', 'xblock_handler', 'xblock_view_handler', 'xblock_outline_handler'] +__all__ = [ + 'orphan_handler', 'xblock_handler', 'xblock_view_handler', 'xblock_outline_handler', 'xblock_container_handler' +] log = logging.getLogger(__name__) @@ -59,7 +62,6 @@ CREATE_IF_NOT_FOUND = ['course_info'] NEVER = lambda x: False ALWAYS = lambda x: True - # In order to allow descriptors to use a handler url, we need to # monkey-patch the x_module library. # TODO: Remove this code when Runtimes are no longer created by modulestores @@ -143,8 +145,9 @@ def xblock_handler(request, usage_key_string): # right now can't combine output of this w/ output of _get_module_info, but worthy goal return JsonResponse(CourseGradingModel.get_section_grader_type(usage_key)) # TODO: pass fields to _get_module_info and only return those - rsp = _get_module_info(_get_xblock(usage_key, request.user)) - return JsonResponse(rsp) + with modulestore().bulk_operations(usage_key.course_key): + response = _get_module_info(_get_xblock(usage_key, request.user)) + return JsonResponse(response) else: return HttpResponse(status=406) @@ -225,14 +228,14 @@ def xblock_view_handler(request, usage_key_string, view_name): request_token=request_token(request), )) - if view_name == STUDIO_VIEW: + if view_name in (STUDIO_VIEW, VISIBILITY_VIEW): try: - fragment = xblock.render(STUDIO_VIEW) + fragment = xblock.render(view_name) # catch exceptions indiscriminately, since after this point they escape the # dungeon and surface as uneditable, unsaveable, and undeletable # component-goblins. except Exception as exc: # pylint: disable=broad-except - log.debug("unable to render studio_view for %r", xblock, exc_info=True) + log.debug("Unable to render %s for %r", view_name, xblock, exc_info=True) fragment = Fragment(render_to_string('html_error.html', {'message': str(exc)})) elif view_name in (PREVIEW_VIEWS + container_views): @@ -333,6 +336,31 @@ def xblock_outline_handler(request, usage_key_string): return Http404 +@require_http_methods(("GET")) +@login_required +@expect_json +def xblock_container_handler(request, usage_key_string): + """ + The restful handler for requests for XBlock information about the block and its children. + This is used by the container page in particular to get additional information about publish state + and ancestor state. + """ + usage_key = usage_key_with_run(usage_key_string) + + if not has_studio_read_access(request.user, usage_key.course_key): + raise PermissionDenied() + + response_format = request.REQUEST.get('format', 'html') + if response_format == 'json' or 'application/json' in request.META.get('HTTP_ACCEPT', 'application/json'): + with modulestore().bulk_operations(usage_key.course_key): + response = _get_module_info( + _get_xblock(usage_key, request.user), include_ancestor_info=True, include_publishing_info=True + ) + return JsonResponse(response) + else: + return Http404 + + def _update_with_callback(xblock, user, old_metadata=None, old_content=None): """ Updates the xblock in the modulestore. @@ -695,7 +723,7 @@ def _get_xblock(usage_key, user): return JsonResponse({"error": "Can't find item by location: " + unicode(usage_key)}, 404) -def _get_module_info(xblock, rewrite_static_links=True): +def _get_module_info(xblock, rewrite_static_links=True, include_ancestor_info=False, include_publishing_info=False): """ metadata, data, id representation of a leaf module fetcher. :param usage_key: A UsageKey @@ -715,7 +743,12 @@ def _get_module_info(xblock, rewrite_static_links=True): modulestore().has_changes(modulestore().get_course(xblock.location.course_key, depth=None)) # Note that children aren't being returned until we have a use case. - return create_xblock_info(xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=True) + xblock_info = create_xblock_info( + xblock, data=data, metadata=own_metadata(xblock), include_ancestor_info=include_ancestor_info + ) + if include_publishing_info: + add_container_page_publishing_info(xblock, xblock_info) + return xblock_info def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=False, include_child_info=False, @@ -735,24 +768,6 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F In addition, an optional include_children_predicate argument can be provided to define whether or not a particular xblock should have its children included. """ - - def safe_get_username(user_id): - """ - Guard against bad user_ids, like the infamous "**replace_user**". - Note that this will ignore our special known IDs (ModuleStoreEnum.UserID). - We should consider adding special handling for those values. - - :param user_id: the user id to get the username of - :return: username, or None if the user does not exist or user_id is None - """ - if user_id: - try: - return User.objects.get(id=user_id).username - except: # pylint: disable=bare-except - pass - - return None - is_library_block = isinstance(xblock.location, LibraryUsageLocator) is_xblock_unit = is_unit(xblock, parent_xblock) # this should not be calculated for Sections and Subsections on Unit page or for library blocks @@ -778,8 +793,6 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F else: child_info = None - # Treat DEFAULT_START_DATE as a magic number that means the release date has not been set - release_date = get_default_time_display(xblock.start) if xblock.start != DEFAULT_START_DATE else None if xblock.category != 'course': visibility_state = _compute_visibility_state(xblock, child_info, is_xblock_unit and has_changes) else: @@ -795,7 +808,7 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F "published_on": get_default_time_display(xblock.published_on) if published and xblock.published_on else None, "studio_url": xblock_studio_url(xblock, parent_xblock), "released_to_students": datetime.now(UTC) > xblock.start, - "release_date": release_date, + "release_date": _get_release_date(xblock), "visibility_state": visibility_state, "has_explicit_staff_lock": xblock.fields['visible_to_staff_only'].is_set_on(xblock), "start": xblock.fields['start'].to_json(xblock.start), @@ -819,19 +832,6 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F else: xblock_info["ancestor_has_staff_lock"] = False - # Currently, 'edited_by', 'published_by', and 'release_date_from' are only used by the - # container page when rendering a unit. Since they are expensive to compute, only include them for units - # that are not being rendered on the course outline. - if is_xblock_unit and not course_outline: - xblock_info["edited_by"] = safe_get_username(xblock.subtree_edited_by) - xblock_info["published_by"] = safe_get_username(xblock.published_by) - xblock_info["currently_visible_to_students"] = is_currently_visible_to_students(xblock) - if release_date: - xblock_info["release_date_from"] = _get_release_date_from(xblock) - if visibility_state == VisibilityState.staff_only: - xblock_info["staff_lock_from"] = _get_staff_lock_from(xblock) - else: - xblock_info["staff_lock_from"] = None if course_outline: if xblock_info["has_explicit_staff_lock"]: xblock_info["staff_only_message"] = True @@ -843,6 +843,40 @@ def create_xblock_info(xblock, data=None, metadata=None, include_ancestor_info=F return xblock_info +def add_container_page_publishing_info(xblock, xblock_info): # pylint: disable=invalid-name + """ + Adds information about the xblock's publish state to the supplied + xblock_info for the container page. + """ + def safe_get_username(user_id): + """ + Guard against bad user_ids, like the infamous "**replace_user**". + Note that this will ignore our special known IDs (ModuleStoreEnum.UserID). + We should consider adding special handling for those values. + + :param user_id: the user id to get the username of + :return: username, or None if the user does not exist or user_id is None + """ + if user_id: + try: + return User.objects.get(id=user_id).username + except: # pylint: disable=bare-except + pass + + return None + + xblock_info["edited_by"] = safe_get_username(xblock.subtree_edited_by) + xblock_info["published_by"] = safe_get_username(xblock.published_by) + xblock_info["currently_visible_to_students"] = is_currently_visible_to_students(xblock) + xblock_info["has_content_group_components"] = has_children_visible_to_specific_content_groups(xblock) + if xblock_info["release_date"]: + xblock_info["release_date_from"] = _get_release_date_from(xblock) + if xblock_info["visibility_state"] == VisibilityState.staff_only: + xblock_info["staff_lock_from"] = _get_staff_lock_from(xblock) + else: + xblock_info["staff_lock_from"] = None + + class VisibilityState(object): """ Represents the possible visibility states for an xblock: @@ -962,6 +996,14 @@ def _create_xblock_child_info(xblock, course_outline, graders, include_children_ return child_info +def _get_release_date(xblock): + """ + Returns the release date for the xblock, or None if the release date has never been set. + """ + # Treat DEFAULT_START_DATE as a magic number that means the release date has not been set + return get_default_time_display(xblock.start) if xblock.start != DEFAULT_START_DATE else None + + def _get_release_date_from(xblock): """ Returns a string representation of the section or subsection that sets the xblock's release date diff --git a/cms/djangoapps/contentstore/views/preview.py b/cms/djangoapps/contentstore/views/preview.py index 63e1a6b792..528b2714c9 100644 --- a/cms/djangoapps/contentstore/views/preview.py +++ b/cms/djangoapps/contentstore/views/preview.py @@ -17,6 +17,7 @@ from xmodule.exceptions import NotFoundError, ProcessingError from xmodule.library_tools import LibraryToolsService from xmodule.modulestore.django import modulestore, ModuleI18nService from opaque_keys.edx.keys import UsageKey +from opaque_keys.edx.locator import LibraryUsageLocator from xmodule.x_module import ModuleSystem from xblock.runtime import KvsFieldData from xblock.django.request import webob_to_django_response, django_to_webob_request @@ -242,6 +243,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): # Only add the Studio wrapper when on the container page. The "Pages" page will remain as is for now. if not context.get('is_pages_view', None) and view in PREVIEW_VIEWS: root_xblock = context.get('root_xblock') + can_edit_visibility = not isinstance(xblock.location, LibraryUsageLocator) is_root = root_xblock and xblock.location == root_xblock.location is_reorderable = _is_xblock_reorderable(xblock, context) template_context = { @@ -251,6 +253,7 @@ def _studio_wrap_xblock(xblock, view, frag, context, display_name_only=False): 'is_root': is_root, 'is_reorderable': is_reorderable, 'can_edit': context.get('can_edit', True), + 'can_edit_visibility': can_edit_visibility, } html = render_to_string('studio_xblock_wrapper.html', template_context) frag = wrap_fragment(frag, html) diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py index 5c0e5130d1..9845d78d47 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -207,17 +207,7 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations self.assertEqual(response.status_code, 200) self.assertContains(response, 'First name') self.assertContains(response, 'Group C') - - def test_view_index_disabled(self): - """ - Check that group configuration page is not displayed when turned off. - """ - if SPLIT_TEST_COMPONENT_TYPE in self.course.advanced_modules: - self.course.advanced_modules.remove(SPLIT_TEST_COMPONENT_TYPE) - self.store.update_item(self.course, self.user.id) - - resp = self.client.get(self._url()) - self.assertContains(resp, "module is disabled") + self.assertContains(response, 'Content Group Configuration') def test_unsupported_http_accept_header(self): """ @@ -243,12 +233,9 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations {u'name': u'Group B', u'version': 1}, ], } - response = self.client.post( + response = self.client.ajax_post( self._url(), - data=json.dumps(GROUP_CONFIGURATION_JSON), - content_type="application/json", - HTTP_ACCEPT="application/json", - HTTP_X_REQUESTED_WITH="XMLHttpRequest", + data=GROUP_CONFIGURATION_JSON ) self.assertEqual(response.status_code, 201) self.assertIn("Location", response) @@ -267,6 +254,16 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations self.assertEqual(user_partititons[0].groups[0].name, u'Group A') self.assertEqual(user_partititons[0].groups[1].name, u'Group B') + def test_lazily_creates_cohort_configuration(self): + """ + Test that a cohort schemed user partition is NOT created by + default for the user. + """ + self.assertEqual(len(self.course.user_partitions), 0) + self.client.get(self._url()) + self.reload_course() + self.assertEqual(len(self.course.user_partitions), 0) + # pylint: disable=no-member class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): @@ -436,7 +433,7 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): Test that right data structure will be created if group configuration is not used. """ self._add_user_partitions() - actual = GroupConfiguration.add_usage_info(self.course, self.store) + actual = GroupConfiguration.get_split_test_partitions_with_usage(self.course, self.store) expected = [{ 'id': 0, 'name': 'Name 0', @@ -460,7 +457,7 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): vertical, __ = self._create_content_experiment(cid=0, name_suffix='0') self._create_content_experiment(name_suffix='1') - actual = GroupConfiguration.add_usage_info(self.course, self.store) + actual = GroupConfiguration.get_split_test_partitions_with_usage(self.course, self.store) expected = [{ 'id': 0, @@ -503,7 +500,7 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): vertical, __ = self._create_content_experiment(cid=0, name_suffix='0') vertical1, __ = self._create_content_experiment(cid=0, name_suffix='1') - actual = GroupConfiguration.add_usage_info(self.course, self.store) + actual = GroupConfiguration.get_split_test_partitions_with_usage(self.course, self.store) expected = [{ 'id': 0, @@ -567,7 +564,7 @@ class GroupConfigurationsValidationTestCase(CourseTestCase, HelperMethods): validation.add(mocked_message) mocked_validation_messages.return_value = validation - group_configuration = GroupConfiguration.add_usage_info(self.course, self.store)[0] + group_configuration = GroupConfiguration.get_split_test_partitions_with_usage(self.course, self.store)[0] self.assertEqual(expected_result.to_json(), group_configuration['usage'][0]['validation']) def test_error_message_present(self): diff --git a/cms/djangoapps/contentstore/views/tests/test_item.py b/cms/djangoapps/contentstore/views/tests/test_item.py index c322233319..01a84dea5c 100644 --- a/cms/djangoapps/contentstore/views/tests/test_item.py +++ b/cms/djangoapps/contentstore/views/tests/test_item.py @@ -18,8 +18,9 @@ from contentstore.views.component import ( component_handler, get_component_templates ) - -from contentstore.views.item import create_xblock_info, ALWAYS, VisibilityState, _xblock_type_and_display_name +from contentstore.views.item import ( + create_xblock_info, ALWAYS, VisibilityState, _xblock_type_and_display_name, add_container_page_publishing_info +) from contentstore.tests.utils import CourseTestCase from student.tests.factories import UserFactory from xmodule.capa_module import CapaDescriptor @@ -116,9 +117,9 @@ class GetItemTest(ItemTest): return resp @ddt.data( - (1, 21, 23, 35, 37), - (2, 22, 24, 38, 39), - (3, 23, 25, 41, 41), + (1, 16, 14, 15, 11), + (2, 16, 14, 15, 11), + (3, 16, 14, 15, 11), ) @ddt.unpack def test_get_query_count(self, branching_factor, chapter_queries, section_queries, unit_queries, problem_queries): @@ -133,6 +134,17 @@ class GetItemTest(ItemTest): with check_mongo_calls(problem_queries): self.client.get(reverse_usage_url('xblock_handler', self.populated_usage_keys['problem'][-1])) + @ddt.data( + (1, 26), + (2, 28), + (3, 30), + ) + @ddt.unpack + def test_container_get_query_count(self, branching_factor, unit_queries,): + self.populate_course(branching_factor) + with check_mongo_calls(unit_queries): + self.client.get(reverse_usage_url('xblock_container_handler', self.populated_usage_keys['vertical'][-1])) + def test_get_vertical(self): # Add a vertical resp = self.create_xblock(category='vertical') @@ -411,21 +423,46 @@ class TestDuplicateItem(ItemTest): except for location and display name. """ def duplicate_and_verify(source_usage_key, parent_usage_key): + """ Duplicates the source, parenting to supplied parent. Then does equality check. """ usage_key = self._duplicate_item(parent_usage_key, source_usage_key) - self.assertTrue(check_equality(source_usage_key, usage_key), "Duplicated item differs from original") + self.assertTrue( + check_equality(source_usage_key, usage_key, parent_usage_key), + "Duplicated item differs from original" + ) - def check_equality(source_usage_key, duplicate_usage_key): + def check_equality(source_usage_key, duplicate_usage_key, parent_usage_key=None): + """ + Gets source and duplicated items from the modulestore using supplied usage keys. + Then verifies that they represent equivalent items (modulo parents and other + known things that may differ). + """ original_item = self.get_item_from_modulestore(source_usage_key) duplicated_item = self.get_item_from_modulestore(duplicate_usage_key) self.assertNotEqual( - original_item.location, - duplicated_item.location, + unicode(original_item.location), + unicode(duplicated_item.location), "Location of duplicate should be different from original" ) - # Set the location and display name to be the same so we can make sure the rest of the duplicate is equal. + + # Parent will only be equal for root of duplicated structure, in the case + # where an item is duplicated in-place. + if parent_usage_key and unicode(original_item.parent) == unicode(parent_usage_key): + self.assertEqual( + unicode(parent_usage_key), unicode(duplicated_item.parent), + "Parent of duplicate should equal parent of source for root xblock when duplicated in-place" + ) + else: + self.assertNotEqual( + unicode(original_item.parent), unicode(duplicated_item.parent), + "Parent duplicate should be different from source" + ) + + # Set the location, display name, and parent to be the same so we can make sure the rest of the + # duplicate is equal. duplicated_item.location = original_item.location duplicated_item.display_name = original_item.display_name + duplicated_item.parent = original_item.parent # Children will also be duplicated, so for the purposes of testing equality, we will set # the children to the original after recursively checking the children. @@ -1367,6 +1404,7 @@ class TestXBlockInfo(ItemTest): include_children_predicate=ALWAYS, include_ancestor_info=True ) + add_container_page_publishing_info(vertical, xblock_info) self.validate_vertical_xblock_info(xblock_info) def test_component_xblock_info(self): @@ -1487,10 +1525,6 @@ class TestXBlockInfo(ItemTest): ) else: self.assertIsNone(xblock_info.get('child_info', None)) - if xblock_info['category'] == 'vertical' and not course_outline: - self.assertEqual(xblock_info['edited_by'], 'testuser') - else: - self.assertIsNone(xblock_info.get('edited_by', None)) class TestLibraryXBlockInfo(ModuleStoreTestCase): @@ -1595,7 +1629,8 @@ class TestXBlockPublishingInfo(ItemTest): ) if staff_only: self._enable_staff_only(child.location) - return child + # In case the staff_only state was set, return the updated xblock. + return modulestore().get_item(child.location) def _get_child_xblock_info(self, xblock_info, index): """ @@ -1684,12 +1719,6 @@ class TestXBlockPublishingInfo(ItemTest): """ self._verify_xblock_info_state(xblock_info, 'has_explicit_staff_lock', expected_state, path, should_equal) - def _verify_staff_lock_from_state(self, xblock_info, expected_state, path=None, should_equal=True): - """ - Verify the staff_lock_from state of an item in the xblock_info. - """ - self._verify_xblock_info_state(xblock_info, 'staff_lock_from', expected_state, path, should_equal) - def test_empty_chapter(self): empty_chapter = self._create_child(self.course, 'chapter', "Empty Chapter") xblock_info = self._get_xblock_info(empty_chapter.location) @@ -1779,7 +1808,7 @@ class TestXBlockPublishingInfo(ItemTest): """ chapter = self._create_child(self.course, 'chapter', "Test Chapter", staff_only=True) sequential = self._create_child(chapter, 'sequential', "Test Sequential") - self._create_child(sequential, 'vertical', "Unit") + vertical = self._create_child(sequential, 'vertical', "Unit") xblock_info = self._get_xblock_info(chapter.location) self._verify_visibility_state(xblock_info, VisibilityState.staff_only) self._verify_visibility_state(xblock_info, VisibilityState.staff_only, path=self.FIRST_SUBSECTION_PATH) @@ -1789,7 +1818,9 @@ class TestXBlockPublishingInfo(ItemTest): self._verify_explicit_staff_lock_state(xblock_info, False, path=self.FIRST_SUBSECTION_PATH) self._verify_explicit_staff_lock_state(xblock_info, False, path=self.FIRST_UNIT_PATH) - self._verify_staff_lock_from_state(xblock_info, _xblock_type_and_display_name(chapter), path=self.FIRST_UNIT_PATH) + vertical_info = self._get_xblock_info(vertical.location) + add_container_page_publishing_info(vertical, vertical_info) + self.assertEqual(_xblock_type_and_display_name(chapter), vertical_info["staff_lock_from"]) def test_no_staff_only_section(self): """ @@ -1810,7 +1841,7 @@ class TestXBlockPublishingInfo(ItemTest): """ chapter = self._create_child(self.course, 'chapter', "Test Chapter") sequential = self._create_child(chapter, 'sequential', "Test Sequential", staff_only=True) - self._create_child(sequential, 'vertical', "Unit") + vertical = self._create_child(sequential, 'vertical', "Unit") xblock_info = self._get_xblock_info(chapter.location) self._verify_visibility_state(xblock_info, VisibilityState.staff_only) self._verify_visibility_state(xblock_info, VisibilityState.staff_only, path=self.FIRST_SUBSECTION_PATH) @@ -1820,7 +1851,9 @@ class TestXBlockPublishingInfo(ItemTest): self._verify_explicit_staff_lock_state(xblock_info, True, path=self.FIRST_SUBSECTION_PATH) self._verify_explicit_staff_lock_state(xblock_info, False, path=self.FIRST_UNIT_PATH) - self._verify_staff_lock_from_state(xblock_info, _xblock_type_and_display_name(sequential), path=self.FIRST_UNIT_PATH) + vertical_info = self._get_xblock_info(vertical.location) + add_container_page_publishing_info(vertical, vertical_info) + self.assertEqual(_xblock_type_and_display_name(sequential), vertical_info["staff_lock_from"]) def test_no_staff_only_subsection(self): """ @@ -1838,7 +1871,7 @@ class TestXBlockPublishingInfo(ItemTest): def test_staff_only_unit(self): chapter = self._create_child(self.course, 'chapter', "Test Chapter") sequential = self._create_child(chapter, 'sequential', "Test Sequential") - unit = self._create_child(sequential, 'vertical', "Unit", staff_only=True) + vertical = self._create_child(sequential, 'vertical', "Unit", staff_only=True) xblock_info = self._get_xblock_info(chapter.location) self._verify_visibility_state(xblock_info, VisibilityState.staff_only) self._verify_visibility_state(xblock_info, VisibilityState.staff_only, path=self.FIRST_SUBSECTION_PATH) @@ -1848,7 +1881,9 @@ class TestXBlockPublishingInfo(ItemTest): self._verify_explicit_staff_lock_state(xblock_info, False, path=self.FIRST_SUBSECTION_PATH) self._verify_explicit_staff_lock_state(xblock_info, True, path=self.FIRST_UNIT_PATH) - self._verify_staff_lock_from_state(xblock_info, _xblock_type_and_display_name(unit), path=self.FIRST_UNIT_PATH) + vertical_info = self._get_xblock_info(vertical.location) + add_container_page_publishing_info(vertical, vertical_info) + self.assertEqual(_xblock_type_and_display_name(vertical), vertical_info["staff_lock_from"]) def test_unscheduled_section_with_live_subsection(self): chapter = self._create_child(self.course, 'chapter', "Test Chapter") diff --git a/cms/envs/common.py b/cms/envs/common.py index 04dc2b699a..3aea5b983b 100644 --- a/cms/envs/common.py +++ b/cms/envs/common.py @@ -37,6 +37,7 @@ from path import path from warnings import simplefilter from lms.djangoapps.lms_xblock.mixin import LmsBlockMixin +from cms.lib.xblock.authoring_mixin import AuthoringMixin import dealer.git from xmodule.modulestore.edit_info import EditInfoMixin @@ -121,6 +122,12 @@ FEATURES = { # for consistency in user-experience, keep the value of this feature flag # in sync with the one in lms/envs/common.py 'ENABLE_EDXNOTES': False, + + # Enable support for content libraries. Note that content libraries are + # only supported in courses using split mongo. Change the setting + # DEFAULT_STORE_FOR_NEW_COURSE to be 'split' to have future courses + # and libraries created with split. + 'ENABLE_CONTENT_LIBRARIES': False, } ENABLE_JASMINE = False @@ -269,7 +276,13 @@ from xmodule.x_module import XModuleMixin # This should be moved into an XBlock Runtime/Application object # once the responsibility of XBlock creation is moved out of modulestore - cpennington -XBLOCK_MIXINS = (LmsBlockMixin, InheritanceMixin, XModuleMixin, EditInfoMixin) +XBLOCK_MIXINS = ( + LmsBlockMixin, + InheritanceMixin, + XModuleMixin, + EditInfoMixin, + AuthoringMixin, +) # Allow any XBlock in Studio # You should also enable the ALLOW_ALL_ADVANCED_COMPONENTS feature flag, so that diff --git a/cms/lib/xblock/authoring_mixin.py b/cms/lib/xblock/authoring_mixin.py new file mode 100644 index 0000000000..a4bbf472e8 --- /dev/null +++ b/cms/lib/xblock/authoring_mixin.py @@ -0,0 +1,49 @@ +""" +Mixin class that provides authoring capabilities for XBlocks. +""" + +import logging + +from xblock.core import XBlock +from xblock.fields import XBlockMixin +from xblock.fragment import Fragment + +log = logging.getLogger(__name__) + +VISIBILITY_VIEW = 'visibility_view' + + +@XBlock.needs("i18n") +class AuthoringMixin(XBlockMixin): + """ + Mixin class that provides authoring capabilities for XBlocks. + """ + _services_requested = { + 'i18n': 'need', + } + + def _get_studio_resource_url(self, relative_url): + """ + Returns the Studio URL to a static resource. + """ + # TODO: is there a cleaner way to do this? + from cms.envs.common import STATIC_URL + return STATIC_URL + relative_url + + def visibility_view(self, _context=None): + """ + Render the view to manage an xblock's visibility settings in Studio. + Args: + _context: Not actively used for this view. + Returns: + (Fragment): An HTML fragment for editing the visibility of this XBlock. + """ + fragment = Fragment() + from contentstore.utils import reverse_course_url + fragment.add_content(self.system.render_template('visibility_editor.html', { + 'xblock': self, + 'manage_groups_url': reverse_course_url('group_configurations_list_handler', self.location.course_key), + })) + fragment.add_javascript_url(self._get_studio_resource_url('/js/xblock/authoring.js')) + fragment.initialize_js('VisibilityEditorInit') + return fragment diff --git a/cms/lib/xblock/test/test_authoring_mixin.py b/cms/lib/xblock/test/test_authoring_mixin.py new file mode 100644 index 0000000000..326b8cc1b8 --- /dev/null +++ b/cms/lib/xblock/test/test_authoring_mixin.py @@ -0,0 +1,125 @@ +""" +Tests for the Studio authoring XBlock mixin. +""" + +from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase +from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory +from xmodule.partitions.partitions import Group, UserPartition + + +class AuthoringMixinTestCase(ModuleStoreTestCase): + """ + Tests the studio authoring XBlock mixin. + """ + def setUp(self): + """ + Create a simple course with a video component. + """ + super(AuthoringMixinTestCase, self).setUp() + self.course = CourseFactory.create() + chapter = ItemFactory.create( + category='chapter', + parent_location=self.course.location, + display_name='Test Chapter' + ) + sequential = ItemFactory.create( + category='sequential', + parent_location=chapter.location, + display_name='Test Sequential' + ) + self.vertical = ItemFactory.create( + category='vertical', + parent_location=sequential.location, + display_name='Test Vertical' + ) + self.video = ItemFactory.create( + category='video', + parent_location=self.vertical.location, + display_name='Test Vertical' + ) + self.pet_groups = [Group(1, 'Cat Lovers'), Group(2, 'Dog Lovers')] + + def create_content_groups(self, content_groups): + """ + Create a cohorted user partition with the specified content groups. + """ + # pylint: disable=attribute-defined-outside-init + self.content_partition = UserPartition( + 1, + 'Content Groups', + 'Contains Groups for Cohorted Courseware', + content_groups, + scheme_id='cohort' + ) + self.course.user_partitions = [self.content_partition] + self.store.update_item(self.course, self.user.id) + + def set_staff_only(self, item): + """Make an item visible to staff only.""" + item.visible_to_staff_only = True + self.store.update_item(item, self.user.id) + + def set_group_access(self, item, group_ids): + """ + Set group_access for the specified item to the specified group + ids within the content partition. + """ + item.group_access[self.content_partition.id] = group_ids # pylint: disable=no-member + self.store.update_item(item, self.user.id) + + def verify_visibility_view_contains(self, item, substrings): + """ + Verify that an item's visibility view returns an html string + containing all the expected substrings. + """ + html = item.visibility_view().body_html() + for string in substrings: + self.assertIn(string, html) + + def test_html_no_partition(self): + self.verify_visibility_view_contains(self.video, 'No content groups exist') + + def test_html_empty_partition(self): + self.create_content_groups([]) + self.verify_visibility_view_contains(self.video, 'No content groups exist') + + def test_html_populated_partition(self): + self.create_content_groups(self.pet_groups) + self.verify_visibility_view_contains(self.video, ['Cat Lovers', 'Dog Lovers']) + + def test_html_no_partition_staff_locked(self): + self.set_staff_only(self.vertical) + self.verify_visibility_view_contains(self.video, ['No content groups exist']) + + def test_html_empty_partition_staff_locked(self): + self.create_content_groups([]) + self.set_staff_only(self.vertical) + self.verify_visibility_view_contains(self.video, 'No content groups exist') + + def test_html_populated_partition_staff_locked(self): + self.create_content_groups(self.pet_groups) + self.set_staff_only(self.vertical) + self.verify_visibility_view_contains( + self.video, ['The Unit this component is contained in is hidden from students.', 'Cat Lovers', 'Dog Lovers'] + ) + + def test_html_false_content_group(self): + self.create_content_groups(self.pet_groups) + self.set_group_access(self.video, ['false_group_id']) + self.verify_visibility_view_contains( + self.video, ['Cat Lovers', 'Dog Lovers', 'Content group no longer exists.'] + ) + + def test_html_false_content_group_staff_locked(self): + self.create_content_groups(self.pet_groups) + self.set_staff_only(self.vertical) + self.set_group_access(self.video, ['false_group_id']) + self.verify_visibility_view_contains( + self.video, + [ + 'Cat Lovers', + 'Dog Lovers', + 'The Unit this component is contained in is hidden from students.', + 'Content group no longer exists.' + ] + ) diff --git a/cms/static/coffee/src/views/module_edit.coffee b/cms/static/coffee/src/views/module_edit.coffee index b839ade446..b04495f8c5 100644 --- a/cms/static/coffee/src/views/module_edit.coffee +++ b/cms/static/coffee/src/views/module_edit.coffee @@ -52,7 +52,5 @@ define ["jquery", "underscore", "gettext", "xblock/runtime.v1", clickEditButton: (event) -> event.preventDefault() - modal = new EditXBlockModal({ - view: 'student_view' - }); + modal = new EditXBlockModal(); modal.edit(this.$el, self.model, { refresh: _.bind(@render, this) }) diff --git a/cms/static/js/factories/container.js b/cms/static/js/factories/container.js index 429ae58f51..6593c25fb3 100644 --- a/cms/static/js/factories/container.js +++ b/cms/static/js/factories/container.js @@ -1,14 +1,14 @@ define([ - 'jquery', 'underscore', 'js/models/xblock_info', 'js/views/pages/container', + 'jquery', 'underscore', 'js/models/xblock_container_info', 'js/views/pages/container', 'js/collections/component_template', 'xmodule', 'coffee/src/main', 'xblock/cms.runtime.v1' ], -function($, _, XBlockInfo, ContainerPage, ComponentTemplates, xmoduleLoader) { +function($, _, XBlockContainerInfo, ContainerPage, ComponentTemplates, xmoduleLoader) { 'use strict'; return function (componentTemplates, XBlockInfoJson, action, options) { var main_options = { el: $('#content'), - model: new XBlockInfo(XBlockInfoJson, {parse: true}), + model: new XBlockContainerInfo(XBlockInfoJson, {parse: true}), action: action, templates: new ComponentTemplates(componentTemplates, {parse: true}) }; diff --git a/cms/static/js/factories/group_configurations.js b/cms/static/js/factories/group_configurations.js index 559c4344ee..df892363e5 100644 --- a/cms/static/js/factories/group_configurations.js +++ b/cms/static/js/factories/group_configurations.js @@ -1,16 +1,22 @@ define([ - 'js/collections/group_configuration', 'js/views/pages/group_configurations' -], function(GroupConfigurationCollection, GroupConfigurationsPage) { + 'js/collections/group_configuration', 'js/models/group_configuration', 'js/views/pages/group_configurations' +], function(GroupConfigurationCollection, GroupConfigurationModel, GroupConfigurationsPage) { 'use strict'; - return function (configurations, groupConfigurationUrl, courseOutlineUrl) { - var collection = new GroupConfigurationCollection(configurations, { parse: true }), - configurationsPage; + return function (experimentsEnabled, experimentGroupConfigurationsJson, contentGroupConfigurationJson, + groupConfigurationUrl, courseOutlineUrl) { + var experimentGroupConfigurations = new GroupConfigurationCollection( + experimentGroupConfigurationsJson, {parse: true} + ), + contentGroupConfiguration = new GroupConfigurationModel(contentGroupConfigurationJson, {parse: true}); - collection.url = groupConfigurationUrl; - collection.outlineUrl = courseOutlineUrl; - configurationsPage = new GroupConfigurationsPage({ + experimentGroupConfigurations.url = groupConfigurationUrl; + experimentGroupConfigurations.outlineUrl = courseOutlineUrl; + contentGroupConfiguration.urlRoot = groupConfigurationUrl; + new GroupConfigurationsPage({ el: $('#content'), - collection: collection + experimentsEnabled: experimentsEnabled, + experimentGroupConfigurations: experimentGroupConfigurations, + contentGroupConfiguration: contentGroupConfiguration }).render(); }; }); diff --git a/cms/static/js/models/custom_sync_xblock_info.js b/cms/static/js/models/custom_sync_xblock_info.js new file mode 100644 index 0000000000..da3b6b0c7a --- /dev/null +++ b/cms/static/js/models/custom_sync_xblock_info.js @@ -0,0 +1,10 @@ +define(["js/models/xblock_info"], + function(XBlockInfo) { + var CustomSyncXBlockInfo = XBlockInfo.extend({ + sync: function(method, model, options) { + options.url = (this.urlRoots[method] || this.urlRoot) + '/' + this.get('id'); + return XBlockInfo.prototype.sync.call(this, method, model, options); + } + }); + return CustomSyncXBlockInfo; + }); diff --git a/cms/static/js/models/xblock_container_info.js b/cms/static/js/models/xblock_container_info.js new file mode 100644 index 0000000000..f60f491d38 --- /dev/null +++ b/cms/static/js/models/xblock_container_info.js @@ -0,0 +1,9 @@ +define(["js/models/custom_sync_xblock_info"], + function(CustomSyncXBlockInfo) { + var XBlockContainerInfo = CustomSyncXBlockInfo.extend({ + urlRoots: { + 'read': '/xblock/container' + } + }); + return XBlockContainerInfo; + }); diff --git a/cms/static/js/models/xblock_info.js b/cms/static/js/models/xblock_info.js index d2324313b5..4e643f7d5f 100644 --- a/cms/static/js/models/xblock_info.js +++ b/cms/static/js/models/xblock_info.js @@ -32,7 +32,8 @@ function(Backbone, _, str, ModuleUtils) { */ 'edited_on':null, /** - * User who last edited the xblock or any of its descendants. + * User who last edited the xblock or any of its descendants. Will only be present if + * publishing info was explicitly requested. */ 'edited_by':null, /** @@ -44,7 +45,8 @@ function(Backbone, _, str, ModuleUtils) { */ 'published_on': null, /** - * User who last published the xblock, or null if never published. + * User who last published the xblock, or null if never published. Will only be present if + * publishing info was explicitly requested. */ 'published_by': null, /** @@ -70,12 +72,14 @@ function(Backbone, _, str, ModuleUtils) { /** * The xblock which is determining the release date. For instance, for a unit, * this will either be the parent subsection or the grandparent section. - * This can be null if the release date is unscheduled. + * This can be null if the release date is unscheduled. Will only be present if + * publishing info was explicitly requested. */ 'release_date_from':null, /** * True if this xblock is currently visible to students. This is computed server-side - * so that the logic isn't duplicated on the client. + * so that the logic isn't duplicated on the client. Will only be present if + * publishing info was explicitly requested. */ 'currently_visible_to_students': null, /** @@ -114,13 +118,20 @@ function(Backbone, _, str, ModuleUtils) { /** * The xblock which is determining the staff lock value. For instance, for a unit, * this will either be the parent subsection or the grandparent section. - * This can be null if the xblock has no inherited staff lock. + * This can be null if the xblock has no inherited staff lock. Will only be present if + * publishing info was explicitly requested. */ 'staff_lock_from': null, /** * True iff this xblock should display a "Contains staff only content" message. */ - 'staff_only_message': null + 'staff_only_message': null, + /** + * True iff this xblock is a unit, and it has children that are only visible to certain + * content groups. Note that this is not a recursive property. Will only be present if + * publishing info was explicitly requested. + */ + 'has_content_group_components': null }, initialize: function () { diff --git a/cms/static/js/models/xblock_outline_info.js b/cms/static/js/models/xblock_outline_info.js index da34adb219..b90001c98e 100644 --- a/cms/static/js/models/xblock_outline_info.js +++ b/cms/static/js/models/xblock_outline_info.js @@ -1,6 +1,6 @@ -define(["js/models/xblock_info"], - function(XBlockInfo) { - var XBlockOutlineInfo = XBlockInfo.extend({ +define(["js/models/custom_sync_xblock_info"], + function(CustomSyncXBlockInfo) { + var XBlockOutlineInfo = CustomSyncXBlockInfo.extend({ urlRoots: { 'read': '/xblock/outline' @@ -8,15 +8,6 @@ define(["js/models/xblock_info"], createChild: function(response) { return new XBlockOutlineInfo(response, { parse: true }); - }, - - sync: function(method, model, options) { - var urlRoot = this.urlRoots[method]; - if (!urlRoot) { - urlRoot = this.urlRoot; - } - options.url = urlRoot + '/' + this.get('id'); - return XBlockInfo.prototype.sync.call(this, method, model, options); } }); return XBlockOutlineInfo; diff --git a/cms/static/js/spec/views/group_configuration_spec.js b/cms/static/js/spec/views/group_configuration_spec.js index 5a23bd1c7b..bc37013475 100644 --- a/cms/static/js/spec/views/group_configuration_spec.js +++ b/cms/static/js/spec/views/group_configuration_spec.js @@ -1,17 +1,15 @@ define([ - 'underscore', 'js/models/course', 'js/models/group_configuration', - 'js/collections/group_configuration', - 'js/views/group_configuration_details', - 'js/views/group_configurations_list', 'js/views/group_configuration_edit', - 'js/views/group_configuration_item', 'js/models/group', - 'js/collections/group', 'js/views/group_edit', + 'underscore', 'js/models/course', 'js/models/group_configuration', 'js/models/group', + 'js/collections/group_configuration', 'js/collections/group', + 'js/views/group_configuration_details', 'js/views/group_configurations_list', 'js/views/group_configuration_editor', + 'js/views/group_configuration_item', 'js/views/experiment_group_edit', 'js/views/content_group_list', 'js/views/feedback_notification', 'js/common_helpers/ajax_helpers', 'js/common_helpers/template_helpers', 'js/spec_helpers/view_helpers', 'jasmine-stealth' ], function( - _, Course, GroupConfigurationModel, GroupConfigurationCollection, - GroupConfigurationDetails, GroupConfigurationsList, GroupConfigurationEdit, - GroupConfigurationItem, GroupModel, GroupCollection, GroupEdit, - Notification, AjaxHelpers, TemplateHelpers, ViewHelpers + _, Course, GroupConfigurationModel, GroupModel, GroupConfigurationCollection, GroupCollection, + GroupConfigurationDetailsView, GroupConfigurationsListView, GroupConfigurationEditorView, + GroupConfigurationItemView, ExperimentGroupEditView, GroupList, Notification, AjaxHelpers, TemplateHelpers, + ViewHelpers ) { 'use strict'; var SELECTORS = { @@ -26,7 +24,7 @@ define([ groupsAllocation: '.group-allocation', errorMessage: '.group-configuration-edit-error', inputGroupName: '.group-name', - inputName: '.group-configuration-name-input', + inputName: '.collection-name-input', inputDescription: '.group-configuration-description-input', usageCount: '.group-configuration-usage-count', usage: '.group-configuration-usage', @@ -90,7 +88,7 @@ define([ delete window.course; }); - describe('GroupConfigurationDetails', function() { + describe('Experiment group configurations details view', function() { beforeEach(function() { TemplateHelpers.installTemplate('group-configuration-details', true); @@ -102,7 +100,7 @@ define([ this.collection = new GroupConfigurationCollection([ this.model ]); this.collection.outlineUrl = '/outline'; - this.view = new GroupConfigurationDetails({ + this.view = new GroupConfigurationDetailsView({ model: this.model }); appendSetFixtures(this.view.render().el); @@ -259,7 +257,7 @@ define([ }); }); - describe('GroupConfigurationEdit', function() { + describe('Experiment group configurations editor view', function() { var setValuesToInputs = function (view, values) { _.each(values, function (value, selector) { @@ -272,7 +270,7 @@ define([ beforeEach(function() { ViewHelpers.installViewTemplates(); TemplateHelpers.installTemplates([ - 'group-configuration-edit', 'group-edit' + 'group-configuration-editor', 'group-edit' ]); this.model = new GroupConfigurationModel({ @@ -283,7 +281,7 @@ define([ }); this.collection = new GroupConfigurationCollection([this.model]); this.collection.url = '/group_configurations'; - this.view = new GroupConfigurationEdit({ + this.view = new GroupConfigurationEditorView({ model: this.model }); appendSetFixtures(this.view.render().el); @@ -490,15 +488,17 @@ define([ }); }); - describe('GroupConfigurationsList', function() { - var emptyMessage = 'You haven\'t created any group configurations yet.'; + describe('Experiment group configurations list view', function() { + var emptyMessage = 'You have not created any group configurations yet.'; beforeEach(function() { - TemplateHelpers.installTemplate('no-group-configurations', true); + TemplateHelpers.installTemplates( + ['group-configuration-editor', 'group-edit', 'list'] + ); this.model = new GroupConfigurationModel({ id: 0 }); this.collection = new GroupConfigurationCollection(); - this.view = new GroupConfigurationsList({ + this.view = new GroupConfigurationsListView({ collection: this.collection }); appendSetFixtures(this.view.render().el); @@ -526,20 +526,25 @@ define([ expect(this.view.$el).toContainText(emptyMessage); expect(this.view.$(SELECTORS.itemView)).not.toExist(); }); + + it('can create a new group configuration', function () { + this.view.$('.new-button').click(); + expect($('.group-configuration-edit').length).toBeGreaterThan(0); + }); }); }); - describe('GroupConfigurationItem', function() { + describe('Experiment group configurations controller view', function() { var clickDeleteItem; beforeEach(function() { TemplateHelpers.installTemplates([ - 'group-configuration-edit', 'group-configuration-details' + 'group-configuration-editor', 'group-configuration-details' ], true); this.model = new GroupConfigurationModel({ id: 0 }); this.collection = new GroupConfigurationCollection([ this.model ]); this.collection.url = '/group_configurations'; - this.view = new GroupConfigurationItem({ + this.view = new GroupConfigurationItemView({ model: this.model }); appendSetFixtures(this.view.render().el); @@ -547,7 +552,7 @@ define([ clickDeleteItem = function (view, promptSpy) { view.$('.delete').click(); - ViewHelpers.verifyPromptShowing(promptSpy, /Delete this Group Configuration/); + ViewHelpers.verifyPromptShowing(promptSpy, /Delete this group configuration/); ViewHelpers.confirmPrompt(promptSpy); ViewHelpers.verifyPromptHidden(promptSpy); }; @@ -598,7 +603,7 @@ define([ }); }); - describe('GroupEdit', function() { + describe('Experiment group configurations group editor view', function() { beforeEach(function() { TemplateHelpers.installTemplate('group-edit', true); @@ -608,7 +613,7 @@ define([ this.collection = new GroupCollection([this.model]); - this.view = new GroupEdit({ + this.view = new ExperimentGroupEditView({ model: this.model }); }); @@ -626,4 +631,211 @@ define([ }); }); }); + + describe('Content groups list view', function() { + var newGroupCss = '.new-button', + addGroupCss = '.action-add', + inputCss = '.collection-name-input', + saveButtonCss = '.action-primary', + cancelButtonCss = '.action-cancel', + validationErrorCss = '.content-group-edit-error', + scopedGroupSelector, createGroups, renderView, saveOrCancel, editNewGroup, editExistingGroup, + verifyEditingGroup, respondToSave, expectGroupsVisible, correctValidationError; + + scopedGroupSelector = function(groupIndex, additionalSelectors) { + var groupSelector = '.content-groups-list-item-' + groupIndex; + if (additionalSelectors) { + return groupSelector + ' ' + additionalSelectors; + } else { + return groupSelector; + } + }; + + createGroups = function (groupNames) { + var groups = new GroupCollection(_.map(groupNames, function (groupName) { + return {name: groupName}; + })), + groupConfiguration = new GroupConfigurationModel({ + id: 0, + name: 'Content Group Configuration', + groups: groups + }); + groupConfiguration.urlRoot = '/mock_url'; + return groups; + }; + + renderView = function(groupNames) { + var view = new GroupList({collection: createGroups(groupNames || [])}).render(); + appendSetFixtures(view.el); + return view; + }; + + saveOrCancel = function(view, options, groupIndex) { + if (options.save) { + view.$(scopedGroupSelector(groupIndex, saveButtonCss)).click(); + } else if (options.cancel) { + view.$(scopedGroupSelector(groupIndex, cancelButtonCss)).click(); + } + }; + + editNewGroup = function(view, options) { + var newGroupIndex; + if (view.collection.length === 0) { + view.$(newGroupCss).click(); + } else { + view.$(addGroupCss).click(); + } + newGroupIndex = view.collection.length - 1; + view.$(inputCss).val(options.newName); + verifyEditingGroup(view, true, newGroupIndex); + saveOrCancel(view, options, newGroupIndex); + }; + + editExistingGroup = function(view, options) { + var groupIndex = options.groupIndex || 0; + view.$(scopedGroupSelector(groupIndex, '.edit')).click(); + view.$(scopedGroupSelector(groupIndex, inputCss)).val(options.newName); + saveOrCancel(view, options, groupIndex); + }; + + verifyEditingGroup = function(view, expectEditing, index) { + // Should prevent the user from opening more than one edit + // form at a time by removing the add button(s) when + // editing a group. + index = index || 0; + if (expectEditing) { + expect(view.$(scopedGroupSelector(index, '.content-group-edit'))).toExist(); + expect(view.$(newGroupCss)).not.toExist(); + expect(view.$(addGroupCss)).toHaveClass('is-hidden'); + } else { + expect(view.$('.content-group-edit')).not.toExist(); + if (view.collection.length === 0) { + expect(view.$(newGroupCss)).toExist(); + expect(view.$(addGroupCss)).not.toExist(); + } else { + expect(view.$(newGroupCss)).not.toExist(); + expect(view.$(addGroupCss)).not.toHaveClass('is-hidden'); + } + } + }; + + respondToSave = function(requests, view) { + expect(requests.length).toBe(1); + expect(requests[0].method).toBe('POST'); + expect(requests[0].url).toBe('/mock_url/0'); + AjaxHelpers.respondWithJson(requests, { + name: 'Content Group Configuration', + groups: view.collection.map(function(groupModel, index) { + return _.extend(groupModel.toJSON(), {id: index}); + }) + }); + }; + + correctValidationError = function(view, requests, newGroupName) { + expect(view.$(validationErrorCss)).toExist(); + verifyEditingGroup(view, true); + view.$(inputCss).val(newGroupName); + view.$(saveButtonCss).click(); + respondToSave(requests, view); + expect(view.$(validationErrorCss)).not.toExist(); + }; + + expectGroupsVisible = function(view, groupNames) { + _.each(groupNames, function(groupName) { + expect(view.$('.content-groups-list-item')).toContainText(groupName); + }); + }; + + beforeEach(function() { + TemplateHelpers.installTemplates( + ['content-group-editor', 'content-group-details', 'list'] + ); + }); + + it('shows a message when no groups are present', function() { + expect(renderView().$('.no-content')) + .toContainText('You have not created any content groups yet.'); + }); + + it('can render groups', function() { + var groupNames = ['Group 1', 'Group 2', 'Group 3']; + renderView(groupNames).$('.content-group-details').each(function(index) { + expect($(this)).toContainText(groupNames[index]); + }); + }); + + it('can create an initial group and save', function() { + var requests = AjaxHelpers.requests(this), + newGroupName = 'New Group Name', + view = renderView(); + editNewGroup(view, {newName: newGroupName, save: true}); + respondToSave(requests, view); + verifyEditingGroup(view, false); + expectGroupsVisible(view, [newGroupName]); + }); + + it('can add another group and save', function() { + var requests = AjaxHelpers.requests(this), + oldGroupName = 'Old Group Name', + newGroupName = 'New Group Name', + view = renderView([oldGroupName]); + editNewGroup(view, {newName: newGroupName, save: true}); + respondToSave(requests, view); + verifyEditingGroup(view, false, 1); + expectGroupsVisible(view, [oldGroupName, newGroupName]); + }); + + it('can cancel adding a group', function() { + var requests = AjaxHelpers.requests(this), + newGroupName = 'New Group Name', + view = renderView(); + editNewGroup(view, {newName: newGroupName, cancel: true}); + expect(requests.length).toBe(0); + verifyEditingGroup(view, false); + expect(view.$()).not.toContainText(newGroupName); + }); + + it('can cancel editing a group', function() { + var requests = AjaxHelpers.requests(this), + originalGroupName = 'Original Group Name', + view = renderView([originalGroupName]); + editExistingGroup(view, {newName: 'New Group Name', cancel: true}); + verifyEditingGroup(view, false); + expect(requests.length).toBe(0); + expect(view.collection.at(0).get('name')).toBe(originalGroupName); + }); + + it('can show and correct a validation error', function() { + var requests = AjaxHelpers.requests(this), + newGroupName = 'New Group Name', + view = renderView(); + editNewGroup(view, {newName: '', save: true}); + expect(requests.length).toBe(0); + correctValidationError(view, requests, newGroupName); + }); + + it('can not invalidate an existing content group', function() { + var requests = AjaxHelpers.requests(this), + oldGroupName = 'Old Group Name', + view = renderView([oldGroupName]); + editExistingGroup(view, {newName: '', save: true}); + expect(requests.length).toBe(0); + correctValidationError(view, requests, oldGroupName); + }); + + it('trims whitespace', function() { + var requests = AjaxHelpers.requests(this), + newGroupName = 'New Group Name', + view = renderView(); + editNewGroup(view, {newName: ' ' + newGroupName + ' ', save: true}); + respondToSave(requests, view); + expect(view.collection.at(0).get('name')).toBe(newGroupName); + }); + + it('only edits one form at a time', function() { + var view = renderView(); + view.collection.add({name: 'Editing Group', editing: true}); + verifyEditingGroup(view, true); + }); + }); }); diff --git a/cms/static/js/spec/views/pages/container_spec.js b/cms/static/js/spec/views/pages/container_spec.js index 7ef32bb7b7..37025ee838 100644 --- a/cms/static/js/spec/views/pages/container_spec.js +++ b/cms/static/js/spec/views/pages/container_spec.js @@ -1,6 +1,6 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_helpers", "js/common_helpers/template_helpers", "js/spec_helpers/edit_helpers", - "js/views/pages/container", "js/views/pages/paged_container", "js/models/xblock_info"], + "js/views/pages/container", "js/views/pages/paged_container", "js/models/xblock_info", "jquery.simulate"], function ($, _, str, AjaxHelpers, TemplateHelpers, EditHelpers, ContainerPage, PagedContainerPage, XBlockInfo) { function parameterized_suite(label, global_page_options, fixtures) { @@ -14,7 +14,9 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel mockBadXBlockContainerXBlockHtml = readFixtures('mock/mock-bad-xblock-container-xblock.underscore'), mockUpdatedContainerXBlockHtml = readFixtures('mock/mock-updated-container-xblock.underscore'), mockXBlockEditorHtml = readFixtures('mock/mock-xblock-editor.underscore'), - PageClass = fixtures.page; + mockXBlockVisibilityEditorHtml = readFixtures('mock/mock-xblock-visibility-editor.underscore'), + PageClass = fixtures.page, + hasVisibilityEditor = fixtures.has_visibility_editor; beforeEach(function () { var newDisplayName = 'New Display Name'; @@ -219,6 +221,26 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel }); expect(EditHelpers.isShowingModal()).toBeTruthy(); }); + + it('can show a visibility modal for a child xblock if supported for the page', function() { + var visibilityButtons; + renderContainerPage(this, mockContainerXBlockHtml); + visibilityButtons = containerPage.$('.wrapper-xblock .visibility-button'); + if (hasVisibilityEditor) { + expect(visibilityButtons.length).toBe(6); + visibilityButtons[0].click(); + expect(str.startsWith(lastRequest().url, '/xblock/locator-component-A1/visibility_view')) + .toBeTruthy(); + AjaxHelpers.respondWithJson(requests, { + html: mockXBlockVisibilityEditorHtml, + resources: [] + }); + expect(EditHelpers.isShowingModal()).toBeTruthy(); + } + else { + expect(visibilityButtons.length).toBe(0); + } + }); }); describe("Editing an xmodule", function () { @@ -572,19 +594,25 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel }); } + // Create a suite for a non-paged container that includes 'edit visibility' buttons parameterized_suite("Non paged", { }, { page: ContainerPage, initial: 'mock/mock-container-xblock.underscore', - add_response: 'mock/mock-xblock.underscore' + add_response: 'mock/mock-xblock.underscore', + has_visibility_editor: true } ); + + // Create a suite for a paged container that does not include 'edit visibility' buttons parameterized_suite("Paged", { page_size: 42 }, { page: PagedContainerPage, initial: 'mock/mock-container-paged-xblock.underscore', - add_response: 'mock/mock-xblock-paged.underscore' - }); + add_response: 'mock/mock-xblock-paged.underscore', + has_visibility_editor: false + } + ); }); diff --git a/cms/static/js/spec/views/pages/container_subviews_spec.js b/cms/static/js/spec/views/pages/container_subviews_spec.js index 2809c779a4..8ac10470ed 100644 --- a/cms/static/js/spec/views/pages/container_subviews_spec.js +++ b/cms/static/js/spec/views/pages/container_subviews_spec.js @@ -80,7 +80,8 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel describe("PreviewActionController", function () { var viewPublishedCss = '.button-view', - previewCss = '.button-preview'; + previewCss = '.button-preview', + visibilityNoteCss = '.note-visibility'; it('renders correctly for unscheduled unit', function () { renderContainerPage(this, mockContainerXBlockHtml); @@ -109,6 +110,18 @@ define(["jquery", "underscore", "underscore.string", "js/common_helpers/ajax_hel fetch({published: false, has_changes: false}); expect(containerPage.$(previewCss)).not.toHaveClass(disabledCss); }); + + it('updates when has_content_group_components attribute changes', function () { + renderContainerPage(this, mockContainerXBlockHtml); + fetch({has_content_group_components: false}); + expect(containerPage.$(visibilityNoteCss).length).toBe(0); + + fetch({has_content_group_components: true}); + expect(containerPage.$(visibilityNoteCss).length).toBe(1); + + fetch({has_content_group_components: false}); + expect(containerPage.$(visibilityNoteCss).length).toBe(0); + }); }); describe("Publisher", function () { diff --git a/cms/static/js/spec/views/pages/group_configurations_spec.js b/cms/static/js/spec/views/pages/group_configurations_spec.js index f5eec76c6e..3ebd6f576f 100644 --- a/cms/static/js/spec/views/pages/group_configurations_spec.js +++ b/cms/static/js/spec/views/pages/group_configurations_spec.js @@ -1,21 +1,24 @@ define([ 'jquery', 'underscore', 'js/views/pages/group_configurations', - 'js/collections/group_configuration', 'js/common_helpers/template_helpers' -], function ($, _, GroupConfigurationsPage, GroupConfigurationCollection, TemplateHelpers) { + 'js/models/group_configuration', 'js/collections/group_configuration', + 'js/common_helpers/template_helpers' +], function ($, _, GroupConfigurationsPage, GroupConfigurationModel, GroupConfigurationCollection, TemplateHelpers) { 'use strict'; describe('GroupConfigurationsPage', function() { var mockGroupConfigurationsPage = readFixtures( 'mock/mock-group-configuration-page.underscore' ), - itemClassName = '.group-configurations-list-item'; + groupConfigItemClassName = '.group-configurations-list-item'; var initializePage = function (disableSpy) { var view = new GroupConfigurationsPage({ el: $('#content'), - collection: new GroupConfigurationCollection({ + experimentsEnabled: true, + experimentGroupConfigurations: new GroupConfigurationCollection({ id: 0, name: 'Configuration 1' - }) + }), + contentGroupConfiguration: new GroupConfigurationModel({groups: []}) }); if (!disableSpy) { @@ -29,15 +32,11 @@ define([ return initializePage().render(); }; - var clickNewConfiguration = function (view) { - view.$('.nav-actions .new-button').click(); - }; - beforeEach(function () { setFixtures(mockGroupConfigurationsPage); TemplateHelpers.installTemplates([ - 'no-group-configurations', 'group-configuration-edit', - 'group-configuration-details' + 'group-configuration-editor', 'group-configuration-details', 'content-group-details', + 'content-group-editor', 'group-edit', 'list' ]); this.addMatchers({ @@ -52,69 +51,67 @@ define([ var view = initializePage(); expect(view.$('.ui-loading')).toBeVisible(); view.render(); - expect(view.$(itemClassName)).toExist(); + expect(view.$(groupConfigItemClassName)).toExist(); + expect(view.$('.content-groups .no-content')).toExist(); expect(view.$('.ui-loading')).toHaveClass('is-hidden'); }); }); - describe('on page close/change', function() { - it('I see notification message if the model is changed', - function() { - var view = initializePage(true), - message; - - view.render(); - message = view.onBeforeUnload(); - expect(message).toBeUndefined(); - }); - - it('I do not see notification message if the model is not changed', - function() { - var expectedMessage ='You have unsaved changes. Do you really want to leave this page?', - view = renderPage(), - message; - - view.collection.at(0).set('name', 'Configuration 2'); - message = view.onBeforeUnload(); - expect(message).toBe(expectedMessage); - }); - }); - - describe('Check that Group Configuration will focus and expand depending on content of url hash', function() { + describe('Experiment group configurations', function() { beforeEach(function () { spyOn($.fn, 'focus'); TemplateHelpers.installTemplate('group-configuration-details'); this.view = initializePage(true); }); - it('should focus and expand group configuration if its id is part of url hash', function() { + it('should focus and expand if its id is part of url hash', function() { spyOn(this.view, 'getLocationHash').andReturn('#0'); this.view.render(); // We cannot use .toBeFocused due to flakiness. expect($.fn.focus).toHaveBeenCalled(); - expect(this.view.$(itemClassName)).toBeExpanded(); + expect(this.view.$(groupConfigItemClassName)).toBeExpanded(); }); - it('should not focus on any group configuration if url hash is empty', function() { + it('should not focus on any experiment configuration if url hash is empty', function() { spyOn(this.view, 'getLocationHash').andReturn(''); this.view.render(); expect($.fn.focus).not.toHaveBeenCalled(); - expect(this.view.$(itemClassName)).not.toBeExpanded(); + expect(this.view.$(groupConfigItemClassName)).not.toBeExpanded(); }); - it('should not focus on any group configuration if url hash contains wrong id', function() { + it('should not focus on any experiment configuration if url hash contains wrong id', function() { spyOn(this.view, 'getLocationHash').andReturn('#1'); this.view.render(); expect($.fn.focus).not.toHaveBeenCalled(); - expect(this.view.$(itemClassName)).not.toBeExpanded(); + expect(this.view.$(groupConfigItemClassName)).not.toBeExpanded(); + }); + + it('should not show a notification message if an experiment configuration is not changed', function () { + this.view.render(); + expect(this.view.onBeforeUnload()).toBeUndefined(); + }); + + it('should show a notification message if an experiment configuration is changed', function () { + this.view.experimentGroupConfigurations.at(0).set('name', 'Configuration 2'); + expect(this.view.onBeforeUnload()) + .toBe('You have unsaved changes. Do you really want to leave this page?'); }); }); - it('create new group configuration', function () { - var view = renderPage(); + describe('Content groups', function() { + beforeEach(function() { + this.view = renderPage(); + }); - clickNewConfiguration(view); - expect($('.group-configuration-edit').length).toBeGreaterThan(0); + it('should not show a notification message if a content group is not changed', function () { + expect(this.view.onBeforeUnload()).toBeUndefined(); + }); + + it('should show a notification message if a content group is changed', function () { + this.view.contentGroupConfiguration.get('groups').add({name: 'Content Group'}); + expect(this.view.onBeforeUnload()) + .toBe('You have unsaved changes. Do you really want to leave this page?'); + }); }); }); }); diff --git a/cms/static/js/spec/views/xblock_editor_spec.js b/cms/static/js/spec/views/xblock_editor_spec.js index 59116c603b..d681db4b5e 100644 --- a/cms/static/js/spec/views/xblock_editor_spec.js +++ b/cms/static/js/spec/views/xblock_editor_spec.js @@ -85,7 +85,7 @@ define([ "jquery", "underscore", "js/common_helpers/ajax_helpers", "js/spec_help }); // Give the mock xblock a save method... editor.xblock.save = window.MockDescriptor.save; - editor.model.save(editor.getXModuleData()); + editor.model.save(editor.getXBlockFieldData()); request = requests[requests.length - 1]; response = JSON.parse(request.requestBody); expect(response.metadata.display_name).toBe(testDisplayName); diff --git a/cms/static/js/views/content_group_details.js b/cms/static/js/views/content_group_details.js new file mode 100644 index 0000000000..9388db60ed --- /dev/null +++ b/cms/static/js/views/content_group_details.js @@ -0,0 +1,32 @@ +/** + * This class defines a simple display view for a content group. + * It is expected to be backed by a Group model. + */ +define([ + 'js/views/baseview' +], function(BaseView) { + 'use strict'; + + var ContentGroupDetailsView = BaseView.extend({ + tagName: 'div', + className: 'content-group-details collection', + + events: { + 'click .edit': 'editGroup' + }, + + editGroup: function() { + this.model.set({'editing': true}); + }, + + initialize: function() { + this.template = this.loadTemplate('content-group-details'); + }, + + render: function() { + this.$el.html(this.template(this.model.toJSON())); + } + }); + + return ContentGroupDetailsView; +}); diff --git a/cms/static/js/views/content_group_editor.js b/cms/static/js/views/content_group_editor.js new file mode 100644 index 0000000000..7d87048f02 --- /dev/null +++ b/cms/static/js/views/content_group_editor.js @@ -0,0 +1,44 @@ +/** + * This class defines an editing view for content groups. + * It is expected to be backed by a Group model. + */ +define([ + 'js/views/list_item_editor', 'underscore' +], +function(ListItemEditorView, _) { + 'use strict'; + + var ContentGroupEditorView = ListItemEditorView.extend({ + tagName: 'div', + className: 'content-group-edit collection-edit', + events: { + 'submit': 'setAndClose', + 'click .action-cancel': 'cancel' + }, + + initialize: function() { + ListItemEditorView.prototype.initialize.call(this); + this.template = this.loadTemplate('content-group-editor'); + }, + + getTemplateOptions: function() { + return { + name: this.model.escape('name'), + index: this.model.collection.indexOf(this.model), + isNew: this.model.isNew(), + uniqueId: _.uniqueId() + }; + }, + + setValues: function() { + this.model.set({name: this.$('input').val().trim()}); + return this; + }, + + getSaveableModel: function() { + return this.model.collection.parents[0]; + } + }); + + return ContentGroupEditorView; +}); diff --git a/cms/static/js/views/content_group_item.js b/cms/static/js/views/content_group_item.js new file mode 100644 index 0000000000..199fe31a0f --- /dev/null +++ b/cms/static/js/views/content_group_item.js @@ -0,0 +1,27 @@ +/** + * This class defines an controller view for content groups. + * It renders an editor view or a details view depending on the state + * of the underlying model. + * It is expected to be backed by a Group model. + */ +define([ + 'js/views/list_item', 'js/views/content_group_editor', 'js/views/content_group_details' +], function(ListItemView, ContentGroupEditorView, ContentGroupDetailsView) { + 'use strict'; + + var ContentGroupItemView = ListItemView.extend({ + tagName: 'section', + + baseClassName: 'content-group', + + createEditView: function() { + return new ContentGroupEditorView({model: this.model}); + }, + + createDetailsView: function() { + return new ContentGroupDetailsView({model: this.model}); + } + }); + + return ContentGroupItemView; +}); diff --git a/cms/static/js/views/content_group_list.js b/cms/static/js/views/content_group_list.js new file mode 100644 index 0000000000..d4b6242e18 --- /dev/null +++ b/cms/static/js/views/content_group_list.js @@ -0,0 +1,26 @@ +/** + * This class defines a list view for content groups. + * It is expected to be backed by a Group collection. + */ +define([ + 'js/views/list', 'js/views/content_group_item', 'gettext' +], function(ListView, ContentGroupItemView, gettext) { + 'use strict'; + + var ContentGroupListView = ListView.extend({ + tagName: 'div', + + className: 'content-group-list', + + // Translators: This refers to a content group that can be linked to a student cohort. + itemCategoryDisplayName: gettext('content group'), + + emptyMessage: gettext('You have not created any content groups yet.'), + + createItemView: function(options) { + return new ContentGroupItemView(options); + } + }); + + return ContentGroupListView; +}); diff --git a/cms/static/js/views/group_edit.js b/cms/static/js/views/experiment_group_edit.js similarity index 77% rename from cms/static/js/views/group_edit.js rename to cms/static/js/views/experiment_group_edit.js index 9b990d4195..8e3dccea0f 100644 --- a/cms/static/js/views/group_edit.js +++ b/cms/static/js/views/experiment_group_edit.js @@ -1,10 +1,14 @@ +/** + * This class defines an edit view for groups within content experiment group configurations. + * It is expected to be backed by a Group model. + */ define([ - 'js/views/baseview', 'underscore', 'underscore.string', 'jquery', 'gettext' + 'js/views/baseview', 'underscore', 'underscore.string', 'gettext' ], -function(BaseView, _, str, $, gettext) { +function(BaseView, _, str, gettext) { 'use strict'; _.str = str; // used in template - var GroupEdit = BaseView.extend({ + var ExperimentGroupEditView = BaseView.extend({ tagName: 'li', events: { 'click .action-close': 'removeGroup', @@ -38,7 +42,7 @@ function(BaseView, _, str, $, gettext) { }, changeName: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } + if (event && event.preventDefault) { event.preventDefault(); } this.model.set({ name: this.$('.group-name').val() }, { silent: true }); @@ -47,7 +51,7 @@ function(BaseView, _, str, $, gettext) { }, removeGroup: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } + if (event && event.preventDefault) { event.preventDefault(); } this.model.collection.remove(this.model); return this.remove(); }, @@ -65,5 +69,5 @@ function(BaseView, _, str, $, gettext) { } }); - return GroupEdit; + return ExperimentGroupEditView; }); diff --git a/cms/static/js/views/group_configuration_details.js b/cms/static/js/views/group_configuration_details.js index ac73a52610..24fc35dee2 100644 --- a/cms/static/js/views/group_configuration_details.js +++ b/cms/static/js/views/group_configuration_details.js @@ -1,9 +1,13 @@ +/** + * This class defines a details view for content experiment group configurations. + * It is expected to be instantiated with a GroupConfiguration model. + */ define([ 'js/views/baseview', 'underscore', 'gettext', 'underscore.string' ], function(BaseView, _, gettext, str) { 'use strict'; - var GroupConfigurationDetails = BaseView.extend({ + var GroupConfigurationDetailsView = BaseView.extend({ tagName: 'div', events: { 'click .edit': 'editConfiguration', @@ -15,6 +19,7 @@ function(BaseView, _, gettext, str) { var index = this.model.collection.indexOf(this.model); return [ + 'collection', 'group-configuration-details', 'group-configuration-details-' + index ].join(' '); @@ -40,17 +45,17 @@ function(BaseView, _, gettext, str) { }, editConfiguration: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } + if (event && event.preventDefault) { event.preventDefault(); } this.model.set('editing', true); }, showGroups: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } + if (event && event.preventDefault) { event.preventDefault(); } this.model.set('showGroups', true); }, hideGroups: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } + if (event && event.preventDefault) { event.preventDefault(); } this.model.set('showGroups', false); }, @@ -107,5 +112,5 @@ function(BaseView, _, gettext, str) { } }); - return GroupConfigurationDetails; + return GroupConfigurationDetailsView; }); diff --git a/cms/static/js/views/group_configuration_edit.js b/cms/static/js/views/group_configuration_edit.js deleted file mode 100644 index 3c5c59591f..0000000000 --- a/cms/static/js/views/group_configuration_edit.js +++ /dev/null @@ -1,157 +0,0 @@ -define([ - 'js/views/baseview', 'underscore', 'jquery', 'gettext', - 'js/views/group_edit', 'js/views/utils/view_utils' -], -function(BaseView, _, $, gettext, GroupEdit, ViewUtils) { - 'use strict'; - var GroupConfigurationEdit = BaseView.extend({ - tagName: 'div', - events: { - 'change .group-configuration-name-input': 'setName', - 'change .group-configuration-description-input': 'setDescription', - "click .action-add-group": "createGroup", - 'focus .input-text': 'onFocus', - 'blur .input-text': 'onBlur', - 'submit': 'setAndClose', - 'click .action-cancel': 'cancel' - }, - - className: function () { - var index = this.model.collection.indexOf(this.model); - - return [ - 'group-configuration-edit', - 'group-configuration-edit-' + index - ].join(' '); - }, - - initialize: function() { - var groups; - - this.template = this.loadTemplate('group-configuration-edit'); - this.listenTo(this.model, 'invalid', this.render); - groups = this.model.get('groups'); - this.listenTo(groups, 'add', this.addOne); - this.listenTo(groups, 'reset', this.addAll); - this.listenTo(groups, 'all', this.render); - }, - - render: function() { - this.$el.html(this.template({ - id: this.model.get('id'), - uniqueId: _.uniqueId(), - name: this.model.escape('name'), - description: this.model.escape('description'), - usage: this.model.get('usage'), - isNew: this.model.isNew(), - error: this.model.validationError - })); - this.addAll(); - return this; - }, - - addOne: function(group) { - var view = new GroupEdit({ model: group }); - this.$('ol.groups').append(view.render().el); - - return this; - }, - - addAll: function() { - this.model.get('groups').each(this.addOne, this); - }, - - createGroup: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } - var collection = this.model.get('groups'); - collection.add([{ - name: collection.getNextDefaultGroupName(), - order: collection.nextOrder() - }]); - }, - - setName: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } - this.model.set( - 'name', this.$('.group-configuration-name-input').val(), - { silent: true } - ); - }, - - setDescription: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } - this.model.set( - 'description', - this.$('.group-configuration-description-input').val(), - { silent: true } - ); - }, - - setValues: function() { - this.setName(); - this.setDescription(); - - _.each(this.$('.groups li'), function(li, i) { - var group = this.model.get('groups').at(i); - - if(group) { - group.set({ - 'name': $('.group-name', li).val() - }); - } - }, this); - - return this; - }, - - setAndClose: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } - - this.setValues(); - if(!this.model.isValid()) { - return false; - } - - ViewUtils.runOperationShowingMessage( - gettext('Saving'), - function () { - var dfd = $.Deferred(); - - this.model.save({}, { - success: function() { - this.model.setOriginalAttributes(); - this.close(); - dfd.resolve(); - }.bind(this) - }); - - return dfd; - }.bind(this) - ); - }, - - cancel: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } - - this.model.reset(); - return this.close(); - }, - - close: function() { - var groupConfigurations = this.model.collection; - - this.remove(); - if(this.model.isNew()) { - // if the group configuration has never been saved, remove it - groupConfigurations.remove(this.model); - } else { - // tell the model that it's no longer being edited - this.model.set('editing', false); - } - - return this; - } - }); - - return GroupConfigurationEdit; -}); diff --git a/cms/static/js/views/group_configuration_editor.js b/cms/static/js/views/group_configuration_editor.js new file mode 100644 index 0000000000..77df7112bd --- /dev/null +++ b/cms/static/js/views/group_configuration_editor.js @@ -0,0 +1,121 @@ +/** + * This class defines an editing view for content experiment group configurations. + * It is expected to be backed by a GroupConfiguration model. + */ +define([ + 'js/views/list_item_editor', 'underscore', 'jquery', 'gettext', + 'js/views/experiment_group_edit' +], +function(ListItemEditorView, _, $, gettext, ExperimentGroupEditView) { + 'use strict'; + var GroupConfigurationEditorView = ListItemEditorView.extend({ + tagName: 'div', + events: { + 'change .collection-name-input': 'setName', + 'change .group-configuration-description-input': 'setDescription', + 'click .action-add-group': 'createGroup', + 'focus .input-text': 'onFocus', + 'blur .input-text': 'onBlur', + 'submit': 'setAndClose', + 'click .action-cancel': 'cancel' + }, + + className: function () { + var index = this.model.collection.indexOf(this.model); + + return [ + 'collection-edit', + 'group-configuration-edit', + 'group-configuration-edit-' + index + ].join(' '); + }, + + initialize: function() { + var groups = this.model.get('groups'); + + ListItemEditorView.prototype.initialize.call(this); + + this.template = this.loadTemplate('group-configuration-editor'); + this.listenTo(groups, 'add', this.onAddItem); + this.listenTo(groups, 'reset', this.addAll); + this.listenTo(groups, 'all', this.render); + }, + + render: function() { + ListItemEditorView.prototype.render.call(this); + this.addAll(); + return this; + }, + + getTemplateOptions: function() { + return { + id: this.model.get('id'), + uniqueId: _.uniqueId(), + name: this.model.escape('name'), + description: this.model.escape('description'), + usage: this.model.get('usage'), + isNew: this.model.isNew() + }; + }, + + getSaveableModel: function() { + return this.model; + }, + + onAddItem: function(group) { + var view = new ExperimentGroupEditView({ model: group }); + this.$('ol.groups').append(view.render().el); + + return this; + }, + + addAll: function() { + this.model.get('groups').each(this.onAddItem, this); + }, + + createGroup: function(event) { + if (event && event.preventDefault) { event.preventDefault(); } + var collection = this.model.get('groups'); + collection.add([{ + name: collection.getNextDefaultGroupName(), + order: collection.nextOrder() + }]); + }, + + setName: function(event) { + if (event && event.preventDefault) { event.preventDefault(); } + this.model.set( + 'name', this.$('.collection-name-input').val(), + { silent: true } + ); + }, + + setDescription: function(event) { + if (event && event.preventDefault) { event.preventDefault(); } + this.model.set( + 'description', + this.$('.group-configuration-description-input').val(), + { silent: true } + ); + }, + + setValues: function() { + this.setName(); + this.setDescription(); + + _.each(this.$('.groups li'), function(li, i) { + var group = this.model.get('groups').at(i); + + if (group) { + group.set({ + 'name': $('.group-name', li).val() + }); + } + }, this); + + return this; + } + }); + + return GroupConfigurationEditorView; +}); diff --git a/cms/static/js/views/group_configuration_item.js b/cms/static/js/views/group_configuration_item.js index 2b2e192d1d..a0ba843b8b 100644 --- a/cms/static/js/views/group_configuration_item.js +++ b/cms/static/js/views/group_configuration_item.js @@ -1,77 +1,45 @@ +/** + * This class defines an controller view for content experiment group configurations. + * It renders an editor view or a details view depending on the state + * of the underlying model. + * It is expected to be backed by a Group model. + */ define([ - 'js/views/baseview', 'jquery', "gettext", 'js/views/group_configuration_details', - 'js/views/group_configuration_edit', "js/views/utils/view_utils" + 'js/views/list_item', 'js/views/group_configuration_details', 'js/views/group_configuration_editor', 'gettext' ], function( - BaseView, $, gettext, GroupConfigurationDetails, GroupConfigurationEdit, ViewUtils + ListItemView, GroupConfigurationDetailsView, GroupConfigurationEditorView, gettext ) { 'use strict'; - var GroupConfigurationsItem = BaseView.extend({ + + var GroupConfigurationItemView = ListItemView.extend({ + events: { + 'click .delete': 'deleteItem' + }, + tagName: 'section', + + baseClassName: 'group-configuration', + + canDelete: true, + + // Translators: this refers to a collection of groups. + itemDisplayName: gettext('group configuration'), + attributes: function () { return { 'id': this.model.get('id'), 'tabindex': -1 }; }, - events: { - 'click .delete': 'deleteConfiguration' + + createEditView: function() { + return new GroupConfigurationEditorView({model: this.model}); }, - className: function () { - var index = this.model.collection.indexOf(this.model); - - return [ - 'group-configuration', - 'group-configurations-list-item', - 'group-configurations-list-item-' + index - ].join(' '); - }, - - initialize: function() { - this.listenTo(this.model, 'change:editing', this.render); - this.listenTo(this.model, 'remove', this.remove); - }, - - deleteConfiguration: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } - var self = this; - ViewUtils.confirmThenRunOperation( - gettext('Delete this Group Configuration?'), - gettext('Deleting this Group Configuration is permanent and cannot be undone.'), - gettext('Delete'), - function() { - return ViewUtils.runOperationShowingMessage( - gettext('Deleting'), - function () { - return self.model.destroy({ wait: true }); - } - ); - } - ); - }, - - render: function() { - // Removes a view from the DOM, and calls stopListening to remove - // any bound events that the view has listened to. - if (this.view) { - this.view.remove(); - } - - if (this.model.get('editing')) { - this.view = new GroupConfigurationEdit({ - model: this.model - }); - } else { - this.view = new GroupConfigurationDetails({ - model: this.model - }); - } - - this.$el.html(this.view.render().el); - - return this; + createDetailsView: function() { + return new GroupConfigurationDetailsView({model: this.model}); } }); - return GroupConfigurationsItem; + return GroupConfigurationItemView; }); diff --git a/cms/static/js/views/group_configurations_list.js b/cms/static/js/views/group_configurations_list.js index 9cce9a1435..e40af573e9 100644 --- a/cms/static/js/views/group_configurations_list.js +++ b/cms/static/js/views/group_configurations_list.js @@ -1,71 +1,28 @@ +/** + * This class defines a list view for content experiment group configurations. + * It is expected to be backed by a GroupConfiguration collection. + */ define([ - 'js/views/baseview', 'jquery', 'js/views/group_configuration_item' -], function( - BaseView, $, GroupConfigurationItemView -) { + 'js/views/list', 'js/views/group_configuration_item', 'gettext' +], function(ListView, GroupConfigurationItemView, gettext) { 'use strict'; - var GroupConfigurationsList = BaseView.extend({ + + var GroupConfigurationsListView = ListView.extend({ tagName: 'div', + className: 'group-configurations-list', - events: { - 'click .new-button': 'addOne' - }, - initialize: function() { - this.emptyTemplate = this.loadTemplate('no-group-configurations'); - this.listenTo(this.collection, 'add', this.addNewItemView); - this.listenTo(this.collection, 'remove', this.handleDestory); - }, + newModelOptions: {addDefaultGroups: true}, - render: function() { - var configurations = this.collection; + // Translators: this refers to a collection of groups. + itemCategoryDisplayName: gettext('group configuration'), - if(configurations.length === 0) { - this.$el.html(this.emptyTemplate()); - } else { - var frag = document.createDocumentFragment(); + emptyMessage: gettext('You have not created any group configurations yet.'), - configurations.each(function(configuration) { - var view = new GroupConfigurationItemView({ - model: configuration - }); - - frag.appendChild(view.render().el); - }); - - this.$el.html([frag]); - } - - return this; - }, - - addNewItemView: function (model) { - var view = new GroupConfigurationItemView({ - model: model - }); - - // If items already exist, just append one new. Otherwise, overwrite - // no-content message. - if (this.collection.length > 1) { - this.$el.append(view.render().el); - } else { - this.$el.html(view.render().el); - } - - view.$el.focus(); - }, - - addOne: function(event) { - if(event && event.preventDefault) { event.preventDefault(); } - this.collection.add([{ editing: true }]); - }, - - handleDestory: function () { - if(this.collection.length === 0) { - this.$el.html(this.emptyTemplate()); - } + createItemView: function(options) { + return new GroupConfigurationItemView(options); } }); - return GroupConfigurationsList; + return GroupConfigurationsListView; }); diff --git a/cms/static/js/views/list.js b/cms/static/js/views/list.js new file mode 100644 index 0000000000..5930329ef9 --- /dev/null +++ b/cms/static/js/views/list.js @@ -0,0 +1,99 @@ +/** + * A generic list view class. + * + * Expects the following properties to be overriden: + * render when the collection is empty. + * - createItemView (function): Create and return an item view for a + * model in the collection. + * - newModelOptions (object): Options to pass to models which are + * added to the collection. + * - itemCategoryDisplayName (string): Display name for the category + * of items this list contains. For example, 'Group Configuration'. + * Note that it must be translated. + * - emptyMessage (string): Text to render when the list is empty. + */ +define([ + 'js/views/baseview' +], function(BaseView) { + 'use strict'; + var ListView = BaseView.extend({ + events: { + 'click .action-add': 'onAddItem', + 'click .new-button': 'onAddItem' + }, + + listContainerCss: '.list-items', + + initialize: function() { + this.listenTo(this.collection, 'add', this.addNewItemView); + this.listenTo(this.collection, 'remove', this.onRemoveItem); + this.template = this.loadTemplate('list'); + + // Don't render the add button when editing a form + this.listenTo(this.collection, 'change:editing', this.toggleAddButton); + this.listenTo(this.collection, 'add', this.toggleAddButton); + this.listenTo(this.collection, 'remove', this.toggleAddButton); + }, + + render: function(model) { + this.$el.html(this.template({ + itemCategoryDisplayName: this.itemCategoryDisplayName, + emptyMessage: this.emptyMessage, + length: this.collection.length, + isEditing: model && model.get('editing') + })); + + this.collection.each(function(model) { + this.$(this.listContainerCss).append(this.createItemView({model: model}).render().el); + }, this); + + return this; + }, + + hideOrShowAddButton: function(shouldShow) { + var addButtonCss = '.action-add'; + if (this.collection.length) { + if (shouldShow) { + this.$(addButtonCss).removeClass('is-hidden'); + } else { + this.$(addButtonCss).addClass('is-hidden'); + } + } + }, + + toggleAddButton: function(model) { + if (model.get('editing') && this.collection.contains(model)) { + this.hideOrShowAddButton(false); + } else { + this.hideOrShowAddButton(true); + } + }, + + addNewItemView: function (model) { + var view = this.createItemView({model: model}); + + // If items already exist, just append one new. + // Otherwise re-render the empty list HTML. + if (this.collection.length > 1) { + this.$(this.listContainerCss).append(view.render().el); + } else { + this.render(); + } + + view.$el.focus(); + }, + + onAddItem: function(event) { + if (event && event.preventDefault) { event.preventDefault(); } + this.collection.add({editing: true}, this.newModelOptions); + }, + + onRemoveItem: function () { + if (this.collection.length === 0) { + this.render(); + } + } + }); + + return ListView; +}); diff --git a/cms/static/js/views/list_item.js b/cms/static/js/views/list_item.js new file mode 100644 index 0000000000..a4bf11eeff --- /dev/null +++ b/cms/static/js/views/list_item.js @@ -0,0 +1,90 @@ +/** + * A generic view to represent an editable item in a list. The item + * has a edit view and a details view. + * + * Subclasses must implement: + * - itemDisplayName (string): Display name for the list item. + * Must be translated. + * - baseClassName (string): CSS class name representing the item. + * - createEditView (function): Render and append the edit view to the + * DOM. + * - createDetailsView (function): Render and append the details view + * to the DOM. + */ +define([ + 'js/views/baseview', 'jquery', "gettext", "js/views/utils/view_utils" +], function( + BaseView, $, gettext, ViewUtils +) { + 'use strict'; + + var ListItemView = BaseView.extend({ + canDelete: false, + + initialize: function() { + this.listenTo(this.model, 'change:editing', this.render); + this.listenTo(this.model, 'remove', this.remove); + }, + + className: function () { + var index = this.model.collection.indexOf(this.model); + + return [ + 'wrapper-collection', + 'wrapper-collection-' + index, + this.baseClassName, + this.baseClassName + 's-list-item', + this.baseClassName + 's-list-item-' + index + ].join(' '); + }, + + deleteItem: function(event) { + if (event && event.preventDefault) { event.preventDefault(); } + if (!this.canDelete) { return; } + var model = this.model, + itemDisplayName = this.itemDisplayName; + ViewUtils.confirmThenRunOperation( + interpolate( + // Translators: "item_display_name" is the name of the item to be deleted. + gettext('Delete this %(item_display_name)s?'), + {item_display_name: itemDisplayName}, true + ), + interpolate( + // Translators: "item_display_name" is the name of the item to be deleted. + gettext('Deleting this %(item_display_name)s is permanent and cannot be undone.'), + {item_display_name: itemDisplayName}, + true + ), + gettext('Delete'), + function() { + return ViewUtils.runOperationShowingMessage( + gettext('Deleting'), + function () { + return model.destroy({wait: true}); + } + ); + } + ); + }, + + render: function() { + // Removes a view from the DOM, and calls stopListening to remove + // any bound events that the view has listened to. + if (this.view) { + this.view.remove(); + } + + if (this.model.get('editing')) { + this.view = this.createEditView(); + } else { + this.view = this.createDetailsView(); + } + + this.$el.html(this.view.render().el); + + return this; + } + }); + + return ListItemView; +}); diff --git a/cms/static/js/views/list_item_editor.js b/cms/static/js/views/list_item_editor.js new file mode 100644 index 0000000000..29638082bf --- /dev/null +++ b/cms/static/js/views/list_item_editor.js @@ -0,0 +1,76 @@ +/** + * A generic view to represent a list item in its editing state. + * + * Subclasses must implement: + * - getTemplateOptions (function): Return an object to pass to the + * template. + * - setValues (function): Set values on the model according to the + * DOM. + * - getSaveableModel (function): Return the model which should be + * saved by this view. Note this may be a parent model. + */ +define([ + 'js/views/baseview', 'js/views/utils/view_utils', 'underscore', 'gettext' +], function(BaseView, ViewUtils, _, gettext) { + 'use strict'; + + var ListItemEditorView = BaseView.extend({ + initialize: function() { + this.listenTo(this.model, 'invalid', this.render); + }, + + render: function() { + this.$el.html(this.template(_.extend({ + error: this.model.validationError + }, this.getTemplateOptions()))); + }, + + setAndClose: function(event) { + if (event && event.preventDefault) { event.preventDefault(); } + + this.setValues(); + if (!this.model.isValid()) { + return false; + } + + ViewUtils.runOperationShowingMessage( + gettext('Saving'), + function () { + var dfd = $.Deferred(); + var actionableModel = this.getSaveableModel(); + + actionableModel.save({}, { + success: function() { + actionableModel.setOriginalAttributes(); + this.close(); + dfd.resolve(); + }.bind(this) + }); + + return dfd; + }.bind(this)); + }, + + cancel: function(event) { + if (event && event.preventDefault) { event.preventDefault(); } + + this.getSaveableModel().reset(); + return this.close(); + }, + + close: function() { + this.remove(); + if (this.model.isNew() && !_.isUndefined(this.model.collection)) { + // if the item has never been saved, remove it + this.model.collection.remove(this.model); + } else { + // tell the model that it's no longer being edited + this.model.set('editing', false); + } + + return this; + } + }); + + return ListItemEditorView; +}); diff --git a/cms/static/js/views/metadata.js b/cms/static/js/views/metadata.js index f70f77b745..3159ada436 100644 --- a/cms/static/js/views/metadata.js +++ b/cms/static/js/views/metadata.js @@ -49,7 +49,7 @@ function(BaseView, _, MetadataModel, AbstractEditor, FileUpload, UploadDialog, V }, /** - * Returns the just the modified metadata values, in the format used to persist to the server. + * Returns just the modified metadata values, in the format used to persist to the server. */ getModifiedMetadataValues: function () { var modified_values = {}; diff --git a/cms/static/js/views/modals/base_modal.js b/cms/static/js/views/modals/base_modal.js index eb543295ed..fb02299484 100644 --- a/cms/static/js/views/modals/base_modal.js +++ b/cms/static/js/views/modals/base_modal.js @@ -1,5 +1,23 @@ /** * This is a base modal implementation that provides common utilities. + * + * A modal implementation should override the following methods: + * + * getTitle(): + * returns the title for the modal. + * getHTMLContent(): + * returns the HTML content to be shown inside the modal. + * + * A modal implementation should also provide the following options: + * + * modalName: A string identifying the modal. + * modalType: A string identifying the type of the modal. + * modalSize: A string, either 'sm', 'med', or 'lg' indicating the + * size of the modal. + * viewSpecificClasses: A string of CSS classes to be attached to + * the modal window. + * addSaveButton: A boolean indicating whether to include a save + * button on the modal. */ define(["jquery", "underscore", "gettext", "js/views/baseview"], function($, _, gettext, BaseView) { @@ -41,7 +59,7 @@ define(["jquery", "underscore", "gettext", "js/views/baseview"], name: this.options.modalName, type: this.options.modalType, size: this.options.modalSize, - title: this.options.title, + title: this.getTitle(), viewSpecificClasses: this.options.viewSpecificClasses })); this.addActionButtons(); @@ -49,6 +67,10 @@ define(["jquery", "underscore", "gettext", "js/views/baseview"], this.parentElement.append(this.$el); }, + getTitle: function() { + return this.options.title; + }, + renderContents: function() { var contentHtml = this.getContentHtml(); this.$('.modal-content').html(contentHtml); diff --git a/cms/static/js/views/modals/edit_xblock.js b/cms/static/js/views/modals/edit_xblock.js index 65d97ad636..104ef571cb 100644 --- a/cms/static/js/views/modals/edit_xblock.js +++ b/cms/static/js/views/modals/edit_xblock.js @@ -6,6 +6,8 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/views/utils/view_utils", "js/models/xblock_info", "js/views/xblock_editor"], function($, _, gettext, BaseModal, ViewUtils, XBlockInfo, XBlockEditorView) { + "strict mode"; + var EditXBlockModal = BaseModal.extend({ events : { "click .action-save": "save", @@ -15,7 +17,10 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie options: $.extend({}, BaseModal.prototype.options, { modalName: 'edit-xblock', addSaveButton: true, - viewSpecificClasses: 'modal-editor confirm' + view: 'studio_view', + viewSpecificClasses: 'modal-editor confirm', + // Translators: "title" is the name of the current component being edited. + titleFormat: gettext("Editing: %(title)s") }), initialize: function() { @@ -56,7 +61,8 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie displayXBlock: function() { this.editorView = new XBlockEditorView({ el: this.$('.xblock-editor'), - model: this.xblockInfo + model: this.xblockInfo, + view: this.options.view }); this.editorView.render({ success: _.bind(this.onDisplayXBlock, this) @@ -66,7 +72,7 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie onDisplayXBlock: function() { var editorView = this.editorView, title = this.getTitle(), - readOnlyView = (this.editOptions && this.editOptions.readOnlyView) || !editorView.xblock.save; + readOnlyView = (this.editOptions && this.editOptions.readOnlyView) || !this.canSave(); // Notify the runtime that the modal has been shown editorView.notifyRuntime('modal-shown', this); @@ -99,6 +105,10 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie this.resize(); }, + canSave: function() { + return this.editorView.xblock.save || this.editorView.xblock.collectFieldData; + }, + disableSave: function() { var saveButton = this.getActionButton('save'), cancelButton = this.getActionButton('cancel'); @@ -112,7 +122,7 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie if (!displayName) { displayName = gettext('Component'); } - return interpolate(gettext("Editing: %(title)s"), { title: displayName }, true); + return interpolate(this.options.titleFormat, { title: displayName }, true); }, addDefaultModes: function() { @@ -147,7 +157,7 @@ define(["jquery", "underscore", "gettext", "js/views/modals/base_modal", "js/vie var self = this, editorView = this.editorView, xblockInfo = this.xblockInfo, - data = editorView.getXModuleData(); + data = editorView.getXBlockFieldData(); event.preventDefault(); if (data) { ViewUtils.runOperationShowingMessage(gettext('Saving'), diff --git a/cms/static/js/views/pages/container.js b/cms/static/js/views/pages/container.js index 71619551cd..45ef51a75d 100644 --- a/cms/static/js/views/pages/container.js +++ b/cms/static/js/views/pages/container.js @@ -15,6 +15,7 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views events: { "click .edit-button": "editXBlock", + "click .visibility-button": "editVisibilitySettings", "click .duplicate-button": "duplicateXBlock", "click .delete-button": "deleteXBlock", "click .new-component-button": "scrollToNewComponentButtons" @@ -161,10 +162,10 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views } }, - editXBlock: function(event) { + editXBlock: function(event, options) { var xblockElement = this.findXBlockElement(event.target), self = this, - modal = new EditXBlockModal({ }); + modal = new EditXBlockModal(options); event.preventDefault(); modal.edit(xblockElement, this.model, { @@ -175,6 +176,16 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views }); }, + editVisibilitySettings: function(event) { + this.editXBlock(event, { + view: 'visibility_view', + // Translators: "title" is the name of the current component being edited. + titleFormat: gettext("Editing visibility for: %(title)s"), + viewSpecificClasses: '', + modalSize: 'med' + }); + }, + duplicateXBlock: function(event) { event.preventDefault(); this.duplicateComponent(this.findXBlockElement(event.target)); diff --git a/cms/static/js/views/pages/container_subviews.js b/cms/static/js/views/pages/container_subviews.js index df77759876..441a4d6860 100644 --- a/cms/static/js/views/pages/container_subviews.js +++ b/cms/static/js/views/pages/container_subviews.js @@ -100,7 +100,8 @@ define(["jquery", "underscore", "gettext", "js/views/baseview", "js/views/utils/ onSync: function(model) { if (ViewUtils.hasChangedAttributes(model, [ - 'has_changes', 'published', 'edited_on', 'edited_by', 'visibility_state', 'has_explicit_staff_lock' + 'has_changes', 'published', 'edited_on', 'edited_by', 'visibility_state', + 'has_explicit_staff_lock', 'has_content_group_components' ])) { this.render(); } @@ -120,7 +121,8 @@ define(["jquery", "underscore", "gettext", "js/views/baseview", "js/views/utils/ releaseDate: this.model.get('release_date'), releaseDateFrom: this.model.get('release_date_from'), hasExplicitStaffLock: this.model.get('has_explicit_staff_lock'), - staffLockFrom: this.model.get('staff_lock_from') + staffLockFrom: this.model.get('staff_lock_from'), + hasContentGroupComponents: this.model.get('has_content_group_components') })); return this; diff --git a/cms/static/js/views/pages/course_outline.js b/cms/static/js/views/pages/course_outline.js index a812eb7b02..4e815fd08b 100644 --- a/cms/static/js/views/pages/course_outline.js +++ b/cms/static/js/views/pages/course_outline.js @@ -26,7 +26,7 @@ define(["jquery", "underscore", "gettext", "js/views/pages/base_page", "js/views }); this.model.on('change', this.setCollapseExpandVisibility, this); $('.dismiss-button').bind('click', ViewUtils.deleteNotificationHandler(function () { - $('.wrapper-alert-announcement').removeClass('is-shown').addClass('is-hidden') + $('.wrapper-alert-announcement').removeClass('is-shown').addClass('is-hidden'); })); }, diff --git a/cms/static/js/views/pages/group_configurations.js b/cms/static/js/views/pages/group_configurations.js index 2bce4afe6d..543eb3616e 100644 --- a/cms/static/js/views/pages/group_configurations.js +++ b/cms/static/js/views/pages/group_configurations.js @@ -1,21 +1,31 @@ define([ 'jquery', 'underscore', 'gettext', 'js/views/pages/base_page', - 'js/views/group_configurations_list' + 'js/views/group_configurations_list', 'js/views/content_group_list' ], -function ($, _, gettext, BasePage, GroupConfigurationsList) { +function ($, _, gettext, BasePage, GroupConfigurationsListView, ContentGroupListView) { 'use strict'; var GroupConfigurationsPage = BasePage.extend({ - initialize: function() { + initialize: function(options) { BasePage.prototype.initialize.call(this); - this.listView = new GroupConfigurationsList({ - collection: this.collection + this.experimentsEnabled = options.experimentsEnabled; + if (this.experimentsEnabled) { + this.experimentGroupConfigurations = options.experimentGroupConfigurations; + this.experimentGroupsListView = new GroupConfigurationsListView({ + collection: this.experimentGroupConfigurations + }); + } + this.contentGroupConfiguration = options.contentGroupConfiguration; + this.cohortGroupsListView = new ContentGroupListView({ + collection: this.contentGroupConfiguration.get('groups') }); }, renderPage: function() { var hash = this.getLocationHash(); - this.$('.content-primary').append(this.listView.render().el); - this.addButtonActions(); + if (this.experimentsEnabled) { + this.$('.wrapper-groups.experiment-groups').append(this.experimentGroupsListView.render().el); + } + this.$('.wrapper-groups.content-groups').append(this.cohortGroupsListView.render().el); this.addWindowActions(); if (hash) { // Strip leading '#' to get id string to match @@ -24,22 +34,17 @@ function ($, _, gettext, BasePage, GroupConfigurationsList) { return $.Deferred().resolve().promise(); }, - addButtonActions: function () { - this.$('.nav-actions .new-button').click(function (event) { - this.listView.addOne(event); - }.bind(this)); - }, - addWindowActions: function () { $(window).on('beforeunload', this.onBeforeUnload.bind(this)); }, onBeforeUnload: function () { - var dirty = this.collection.find(function(configuration) { - return configuration.isDirty(); - }); + var dirty = this.contentGroupConfiguration.isDirty() || + (this.experimentsEnabled && this.experimentGroupConfigurations.find(function(configuration) { + return configuration.isDirty(); + })); - if(dirty) { + if (dirty) { return gettext('You have unsaved changes. Do you really want to leave this page?'); } }, @@ -57,7 +62,7 @@ function ($, _, gettext, BasePage, GroupConfigurationsList) { * @param {String|Number} Id of the group configuration. */ expandConfiguration: function (id) { - var groupConfig = this.collection.findWhere({ + var groupConfig = this.experimentsEnabled && this.experimentGroupConfigurations.findWhere({ id: parseInt(id) }); diff --git a/cms/static/js/views/xblock_editor.js b/cms/static/js/views/xblock_editor.js index 7cdf2de34a..e549fa6b54 100644 --- a/cms/static/js/views/xblock_editor.js +++ b/cms/static/js/views/xblock_editor.js @@ -89,17 +89,23 @@ define(["jquery", "underscore", "gettext", "js/views/xblock", "js/views/metadata }, /** - * Returns the data saved for the xmodule. Note that this *does not* work for XBlocks. + * Returns the updated field data for the xblock. Note that this works for all + * XModules as well as for XBlocks that provide a 'collectFieldData' API. */ - getXModuleData: function() { + getXBlockFieldData: function() { var xblock = this.xblock, metadataEditor = this.getMetadataEditor(), data = null; - if (xblock.save) { + // If the xblock supports returning its field data then collect it + if (xblock.collectFieldData) { + data = xblock.collectFieldData(); + // ... else if this is an XModule then call its save method + } else if (xblock.save) { data = xblock.save(); if (metadataEditor) { data.metadata = _.extend(data.metadata || {}, this.getChangedMetadata()); } + // ... else log an error } else { console.error('Cannot save xblock as it has no save method'); } diff --git a/cms/static/js/xblock/authoring.js b/cms/static/js/xblock/authoring.js new file mode 100644 index 0000000000..dfc8e7cc0d --- /dev/null +++ b/cms/static/js/xblock/authoring.js @@ -0,0 +1,48 @@ +/** + * Client-side logic to support XBlock authoring. + */ +(function($) { + 'use strict'; + + function VisibilityEditorView(runtime, element) { + this.getGroupAccess = function() { + var groupAccess, userPartitionId, selectedGroupIds; + if (element.find('.visibility-level-all').prop('checked')) { + return {}; + } + userPartitionId = element.find('.wrapper-visibility-specific').data('user-partition-id').toString(); + selectedGroupIds = []; + element.find('.field-visibility-content-group input:checked').each(function(index, input) { + selectedGroupIds.push(parseInt($(input).val())); + }); + groupAccess = {}; + groupAccess[userPartitionId] = selectedGroupIds; + return groupAccess; + }; + + element.find('.field-visibility-level input').change(function(event) { + if ($(event.target).hasClass('visibility-level-all')) { + element.find('.field-visibility-content-group input').prop('checked', false); + } + }); + element.find('.field-visibility-content-group input').change(function(event) { + element.find('.visibility-level-all').prop('checked', false); + element.find('.visibility-level-specific').prop('checked', true); + }); + } + + VisibilityEditorView.prototype.collectFieldData = function collectFieldData() { + return { + metadata: { + "group_access": this.getGroupAccess() + } + }; + }; + + function initializeVisibilityEditor(runtime, element) { + return new VisibilityEditorView(runtime, element); + } + + // XBlock initialization functions must be global + window.VisibilityEditorInit = initializeVisibilityEditor; +})($); diff --git a/cms/static/sass/_variables.scss b/cms/static/sass/_variables.scss index d9739ab081..b68e0399f7 100644 --- a/cms/static/sass/_variables.scss +++ b/cms/static/sass/_variables.scss @@ -183,6 +183,7 @@ $color-ready: $green; $color-warning: $orange-l2; $color-error: $red-l2; $color-staff-only: $black; +$color-visibility-set: $black; $color-heading-base: $gray-d2; $color-copy-base: $gray-l1; diff --git a/cms/static/sass/elements/_forms.scss b/cms/static/sass/elements/_forms.scss index c6425b1144..cb924078e0 100644 --- a/cms/static/sass/elements/_forms.scss +++ b/cms/static/sass/elements/_forms.scss @@ -1,7 +1,7 @@ // studio - elements - forms // ==================== -// Table of Contents +// Table of Contents // * +Forms - General // * +Field - Is Editable // * +Field - With Error @@ -12,7 +12,23 @@ // * +Form - Grandfathered // +Forms - General -// ==================== +// ==================== +// element-specific utilities +// -------------------- +// UI: checkbox/radio inputs +%input-tickable { + + ~ label { + color: $color-copy-base; + } + + // STATE: checked/selected + &:checked ~ label { + @extend %t-strong; + color: $ui-action-primary-color-focus; + } +} + input[type="text"], input[type="email"], input[type="password"], @@ -77,7 +93,7 @@ form { } .input-checkbox-checked, .input-checkbox-unchecked { - width: $baseline; + width: ($baseline*0.75); } .input-checkbox { @@ -107,8 +123,18 @@ form { } } + // CASE: checkbox input + .field-checkbox .input-checkbox { + @extend %input-tickable; + } + + // CASE: radio input + .field-radio .input-radio { + @extend %input-tickable; + } + // CASE: file input - input[type=file] { + input[type="file"] { @extend %t-copy-sub1; } diff --git a/cms/static/sass/elements/_modal-window.scss b/cms/static/sass/elements/_modal-window.scss index 978b5e1437..4c8683c1c4 100644 --- a/cms/static/sass/elements/_modal-window.scss +++ b/cms/static/sass/elements/_modal-window.scss @@ -52,6 +52,45 @@ } } + // UI: summary messages + .summary-message { + margin-bottom: $baseline; + padding: ($baseline*0.75); + background: $gray-d3; + + .icon, .copy { + display: inline-block; + vertical-align: top; + } + + .icon { + @extend %t-icon4; + @include margin-right($baseline/2); + color: $white; + } + + .copy { + @extend %t-copy-sub1; + max-width: 85%; + color: $white; + } + } + + // CASE: Warning summary message + .summary-message-warning { + border-top: ($baseline/5) solid $color-warning; + + .icon { + color: $color-warning; + } + } + + // visual dividers + .divider-visual { + margin: ($baseline*0.75) 0; + border: ($baseline/20) solid $gray-l4; + } + // sections within a modal .modal-section { margin-bottom: ($baseline*0.75); @@ -64,11 +103,20 @@ .modal-section-title { @extend %t-title6; margin: 0 0 ($baseline/2) 0; - border-bottom: 1px solid $gray-l4; + border-bottom: ($baseline/10) solid $gray-l4; padding-bottom: ($baseline/4); color: $gray-d2; } + .modal-subsection-title { + @extend %t-title8; + @extend %t-strong; + margin-bottom: ($baseline/4); + text-transform: uppercase; + letter-spacing: 0.1; + color: $gray-l2; + } + .modal-section-content { .list-fields, .list-actions { @@ -238,143 +286,6 @@ } } - // outline: edit item settings - .wrapper-modal-window-bulkpublish-section, - .wrapper-modal-window-bulkpublish-subsection, - .wrapper-modal-window-bulkpublish-unit, - .course-outline-modal { - - .list-fields { - - .field { - display: inline-block; - vertical-align: top; - margin-right: ($baseline/2); - margin-bottom: ($baseline/4); - - label { - @extend %t-copy-sub1; - @extend %t-strong; - @include transition(color $tmg-f3 ease-in-out 0s); - margin: 0 0 ($baseline/4) 0; - - &.is-focused { - color: $blue; - } - } - - - input, textarea { - @extend %t-copy-base; - @include transition(all $tmg-f2 ease-in-out 0s); - height: 100%; - width: 100%; - padding: ($baseline/2); - - // CASE: long length - &.long { - width: 100%; - } - - // CASE: short length - &.short { - width: 25%; - } - } - - // CASE: specific release + due times/dates - .start-date, - .start-time, - .due-date, - .due-time { - width: ($baseline*7); - } - - .tip { - @extend %t-copy-sub1; - @include transition(color, 0.15s, ease-in-out); - display: block; - margin-top: ($baseline/4); - color: $gray-l2; - } - - .tip-warning { - color: $gray-d2; - } - } - - // CASE: type-based input - .field-text { - - // TODO: refactor the _forms.scss partial to allow for this area to inherit from it - label, input, textarea { - display: block; - } - } - - // CASE: select input - .field-select { - - .label, .input { - display: inline-block; - vertical-align: middle; - } - - .label { - margin-right: ($baseline/2); - } - - .input { - width: 100%; - } - - // CASE: checkbox input - .field-checkbox { - - .label, label { - margin-bottom: 0; - } - } - } - } - - - - // UI: grading section - .edit-settings-grading { - - .grading-type { - margin-bottom: $baseline; - } - } - - // UI: staff lock section - .edit-staff-lock { - - .checkbox-cosmetic .input-checkbox { - @extend %cont-text-sr; - - // CASE: unchecked - ~ .tip-warning { - display: block; - } - - // CASE: checked - &:checked { - - ~ .tip-warning { - display: none; - } - } - } - - // needed to override poorly scoped margin-bottom on any label element in a view (from _forms.scss) - .checkbox-cosmetic .label { - margin-bottom: 0; - } - } - } - // xblock custom actions .modal-window .editor-with-buttons { margin-bottom: ($baseline*3); @@ -394,7 +305,7 @@ } - // special overrides for video module editor/hidden tab editors + // MODAL TYPE: component - video modal (includes special overrides for xblock-related editing view) .modal-lg.modal-type-video { .modal-content { @@ -517,4 +428,225 @@ opacity: 0.5; filter: alpha(opacity=50); } + + // MODAL TYPE: component - visibility modal + .xblock-visibility_view { + + .visibility-controls-secondary { + max-height: 100%; + overflow-y: auto; + @include margin(($baseline*0.75), 0, 0, $baseline); + } + + .visibility-controls-group { + @extend %wipe-last-child; + margin-bottom: $baseline; + } + + // UI: form fields + .list-fields { + + .field { + @extend %wipe-last-child; + margin-bottom: ($baseline/4); + + label { + @extend %t-copy-sub1; + } + } + + // UI: radio and checkbox inputs + .field-radio, .field-checkbox { + + label { + @include margin-left($baseline/4); + } + } + } + + // CASE: content group has been removed + .field-visibility-content-group.was-removed { + + .input-checkbox:checked ~ label { + color: $color-error; + } + + .note { + @extend %t-copy-sub2; + @extend %t-regular; + display: block; + color: $color-error; + } + } + + // CASE: no groups configured for visibility + .is-not-configured { + @extend %no-content; + padding: ($baseline); + @include text-align(left); // reset for %no-content's default styling + + .title { + @extend %t-title6; + font-weight: 600; // needed for poorly scoped .title rule in modals + margin: 0 0 ($baseline/2) 0; // needed for poorly scoped .title rule in modals + } + + .copy { + @extend %t-copy-sub1; + + p { + @extend %wipe-last-child; + margin-bottom: $baseline; + } + } + + &.has-actions { + + .actions { + margin-top: $baseline; + } + + .action { + @include margin-left(0); // reset for %no-content's default styling + } + } + } + } + + // MODAL TYPE: outline - edit item settings + .wrapper-modal-window-bulkpublish-section, + .wrapper-modal-window-bulkpublish-subsection, + .wrapper-modal-window-bulkpublish-unit, + .course-outline-modal { + + .list-fields { + + .field { + display: inline-block; + vertical-align: top; + @include margin-right($baseline/2); + margin-bottom: ($baseline/4); + + label { + @extend %t-copy-sub1; + @extend %t-strong; + @include transition(color $tmg-f3 ease-in-out 0s); + margin: 0 0 ($baseline/4) 0; + + &.is-focused { + color: $blue; + } + } + + + input, textarea { + @extend %t-copy-base; + @include transition(all $tmg-f2 ease-in-out 0s); + height: 100%; + width: 100%; + padding: ($baseline/2); + + // CASE: long length + &.long { + width: 100%; + } + + // CASE: short length + &.short { + width: 25%; + } + } + + // CASE: specific release + due times/dates + .start-date, + .start-time, + .due-date, + .due-time { + width: ($baseline*7); + } + + .tip { + @extend %t-copy-sub1; + @include transition(color, 0.15s, ease-in-out); + display: block; + margin-top: ($baseline/4); + color: $gray-l2; + } + + .tip-warning { + color: $gray-d2; + } + } + + // CASE: type-based input + .field-text { + + // TODO: refactor the _forms.scss partial to allow for this area to inherit from it + label, input, textarea { + display: block; + } + } + + // CASE: select input + .field-select { + + .label, .input { + display: inline-block; + vertical-align: middle; + } + + .label { + @include margin-right($baseline/2); + } + + .input { + width: 100%; + } + + // CASE: checkbox input + .field-checkbox { + + .label, label { + margin-bottom: 0; + } + } + } + } + + + + // UI: grading section + .edit-settings-grading { + + .grading-type { + margin-bottom: $baseline; + } + } + + // UI: staff lock section + .edit-staff-lock { + + .checkbox-cosmetic .input-checkbox { + @extend %cont-text-sr; + + // CASE: unchecked + ~ .tip-warning { + display: block; + } + + // CASE: checked + &:checked { + + ~ .tip-warning { + display: none; + } + } + } + + // needed to override poorly scoped margin-bottom on any label element in a view (from _forms.scss) + .checkbox-cosmetic .label { + margin-bottom: 0; + } + } + } } diff --git a/cms/static/sass/elements/_xblocks.scss b/cms/static/sass/elements/_xblocks.scss index 33d4646f0d..186b78a9e6 100644 --- a/cms/static/sass/elements/_xblocks.scss +++ b/cms/static/sass/elements/_xblocks.scss @@ -150,42 +150,50 @@ // ==================== - // UI: xblocks - calls-to-action - .wrapper-xblock .header-actions { + .wrapper-xblock { - .actions-list { + // UI: xblocks - calls-to-action + .header-actions .actions-list { @extend %actions-list; } - } - // UI: xblock is collapsible - .wrapper-xblock.is-collapsible, - .wrapper-xblock.xblock-type-container { + // CASE: xblock is collapsible + &.is-collapsible, + &.xblock-type-container { - .icon { - font-style: normal; - } + .icon { + font-style: normal; + } - .expand-collapse { - @extend %expand-collapse; - margin: 0 ($baseline/4); - height: ($baseline*1.25); - width: $baseline; + .expand-collapse { + @extend %expand-collapse; + margin: 0 ($baseline/4); + height: ($baseline*1.25); + width: $baseline; - &:focus { - outline: 0; + &:focus { + outline: 0; + } + } + + .action-view { + + .action-button { + transition: none; + } + + .action-button-text { + padding-right: ($baseline/5); + padding-left: 0; + } } } - .action-view { + // CASE: xblock has specific visibility based on content groups set + &.has-group-visibility-set { - .action-button { - transition: none; - } - - .action-button-text { - padding-right: ($baseline/5); - padding-left: 0; + .action-visibility .visibility-button.visibility-button { // needed to cascade in front of overscoped header-actions CSS rule + color: $color-visibility-set; } } } diff --git a/cms/static/sass/views/_container.scss b/cms/static/sass/views/_container.scss index af9539c7b7..74a275e424 100644 --- a/cms/static/sass/views/_container.scss +++ b/cms/static/sass/views/_container.scss @@ -6,7 +6,20 @@ // ==================== +// view-specific utilities +// -------------------- +%status-value-base { + @extend %t-title7; + @extend %t-strong; +} + +%status-value-sub1 { + @extend %t-title8; + display: block; +} + // UI: container page view +// -------------------- .view-container { @extend %two-col-1; @@ -102,6 +115,7 @@ @extend %t-title8; } + // UI: publishing details/summary .bit-publishing { @extend %bar-module; @@ -159,37 +173,43 @@ .wrapper-release { .release-date { - @extend %t-strong; + @extend %status-value-base; } .release-with { - @extend %t-title8; - display: block; + @extend %status-value-sub1; } } .wrapper-visibility { .copy { - @extend %t-strong; + @extend %status-value-base; margin-bottom: ($baseline/10); } .icon { - margin-left: ($baseline/4); color: $gray-d1; } .inherited-from { - @extend %t-title8; - display: block; + @extend %status-value-sub1; } + // UI: note about specific access + .note-visibility { + @extend %status-value-sub1; + .icon { + @include margin-right($baseline/4); + } + } } .wrapper-pub-actions { - padding: ($baseline*0.75); + border-top: 1px solid $gray-l4; + margin-top: ($baseline/2); + padding: $baseline ($baseline*0.75) ($baseline*0.75) ($baseline*0.75); .action-publish { @extend %btn-primary-blue; @@ -209,7 +229,6 @@ } } } - } // versioning widget @@ -244,8 +263,7 @@ .wrapper-unit-id, .wrapper-library-id { .unit-id-value, .library-id-value { - @extend %cont-text-wrap; - @extend %t-copy-sub1; + @extend %status-value-base; display: inline-block; width: 100%; } @@ -308,5 +326,3 @@ } } } - - diff --git a/cms/static/sass/views/_group-configuration.scss b/cms/static/sass/views/_group-configuration.scss index 92b973c8b0..ac6168423f 100644 --- a/cms/static/sass/views/_group-configuration.scss +++ b/cms/static/sass/views/_group-configuration.scss @@ -19,206 +19,199 @@ text-align: center; } - .no-group-configurations-content { - @extend %ui-well; - padding: ($baseline*2); - background-color: $gray-l4; - text-align: center; - color: $gray; + .no-content { + @extend %no-content; } - .new-button { - @extend %t-action3; - margin-left: $baseline; + .wrapper-groups { + margin-bottom: ($baseline*1.5); - .icon { - margin-right: ($baseline/2); + .title { + @extend %t-title4; + @extend %t-strong; + margin-bottom: ($baseline/2); + } + + .copy { + @extend %t-copy-sub1; } } - .group-configuration { + .wrapper-collection { @extend %ui-window; position: relative; outline: none; - .group-configuration-details { - .wrapper-group-configuration { - padding: $baseline ($baseline*1.5); + &:hover .collection .actions { + opacity: 1.0; + } - .group-configuration-header { - margin-bottom: 0; - border-bottom: 0; - } + .collection-details { + padding: $baseline ($baseline*1.5); + } - .group-configuration-title { - @extend %t-title4; - @extend %t-strong; - overflow: hidden; - text-overflow: ellipsis; - margin-right: ($baseline*14); + .collection-header { + margin-bottom: 0; + border-bottom: 0; + padding-bottom: 0; - .group-toggle { - display: inline-block; - padding-left: $baseline; - color: $black; + .title { + @extend %cont-truncated; + @extend %t-title5; + @extend %t-strong; + margin-right: ($baseline*14); + color: $black; - &:hover, &:focus { - color: $blue; - } - } - } - - .group-configuration-info { - @extend %t-copy-sub1; - color: $gray-l1; - margin-left: $baseline; - - &.group-configuration-info-inline { - display: table; - width: 70%; - margin: ($baseline/4) 0 ($baseline/2) $baseline; - - li { - @include box-sizing(border-box); - display: table-cell; - margin-right: 1%; - - &.group-configuration-usage-count { - font-style: italic; - } - } - } - - &.group-configuration-info-block { - li { - padding: ($baseline/4) 0; - } - } - - .group-configuration-label { - text-transform: uppercase; - } - - .group-configuration-description { - overflow: hidden; - text-overflow: ellipsis; - } - } - - .ui-toggle-expansion { - @include transition(rotate .15s ease-in-out .25s); - @extend %t-action1; + .toggle { display: inline-block; - width: ($baseline*0.75); - vertical-align: baseline; - margin-left: -$baseline; - } + padding-left: $baseline; + color: $black; - &.is-selectable { - @extend %ui-fake-link; - - &:hover { + &:hover, &:focus { color: $blue; - - .ui-toggle-expansion { - color: $blue; - } } - } - .groups { - margin-left: $baseline; - margin-bottom: ($baseline*0.75); - - .group { - @extend %t-copy-lead1; - padding: ($baseline/7) 0 ($baseline/4); - border-top: 1px solid $gray-l4; - white-space: nowrap; - - &:first-child { - border-top: none; - } - - .group-name { - overflow: hidden; - text-overflow: ellipsis; - display: inline-block; - vertical-align: middle; - width: 75%; - margin-right: 5%; - } - - .group-allocation { - display: inline-block; - vertical-align: middle; - width: 20%; - color: $gray-l1; - text-align: right; - } - } - } - - .actions { - @include transition(opacity .15s .25s ease-in-out); - opacity: 0.0; - position: absolute; - top: $baseline; - right: $baseline; - - .action { + .ui-toggle-expansion { + @include transition(rotate .15s ease-in-out .25s); + @extend %t-action1; display: inline-block; - vertical-align: middle; - margin-right: ($baseline/4); - - .edit { - @include blue-button; - @extend %t-action4; - } - - .delete { - @extend %ui-btn-non; - - &.is-disabled { - background-color: $gray-l3; - color: $gray-l6; - } - } + width: ($baseline*0.75); + vertical-align: baseline; + margin-left: -$baseline; } - } - } - .wrapper-group-configuration-usages { - @extend %t-copy-sub1; - box-shadow: 0 2px 2px 0 $shadow inset; - padding: $baseline ($baseline*1.5) $baseline ($baseline*2.5); - color: $gray-l1; + &.is-selectable { + @extend %ui-fake-link; - .group-configuration-usage { - margin-left: $baseline; + &:hover { + color: $blue; - .group-configuration-usage-unit { - padding: ($baseline/4) 0; - - a { - font-weight: 600; - } - - .fa-warning { - margin: ($baseline/4) ($baseline/2) 0 ($baseline*1.5); - color: $orange; - } - - .fa-times-circle { - margin: ($baseline/4) ($baseline/2) 0 ($baseline*1.5); - color: $red-l2; + .ui-toggle-expansion { + color: $blue; + } } } } } } - .wrapper-group-configuration-validation { + .collection-info { + @extend %t-copy-sub1; + color: $gray-l1; + margin-left: $baseline; + + &.collection-info-inline { + display: table; + width: 70%; + margin: ($baseline/4) 0 ($baseline/2) $baseline; + + li { + @include box-sizing(border-box); + display: table-cell; + margin-right: 1%; + padding: ($baseline/4) 0; + + &.collection-usage-count { + font-style: italic; + } + } + } + + .collection-label { + text-transform: uppercase; + } + + .collection-description { + overflow: hidden; + text-overflow: ellipsis; + } + } + + .collection-items { + margin-left: $baseline; + margin-bottom: ($baseline*0.75); + + .item { + @extend %t-copy-lead1; + padding: ($baseline/7) 0 ($baseline/4); + border-top: 1px solid $gray-l4; + white-space: nowrap; + + &:first-child { + border-top: none; + } + + .name { + overflow: hidden; + text-overflow: ellipsis; + display: inline-block; + vertical-align: middle; + width: 75%; + margin-right: 5%; + } + } + } + + .collection-details { + + .actions { + @include transition(opacity .15s .25s ease-in-out); + position: absolute; + top: $baseline; + right: $baseline; + opacity: 0.0; + + .action { + display: inline-block; + vertical-align: middle; + margin-right: ($baseline/4); + + .edit { + @extend %ui-btn-non-blue; + } + + .delete { + @extend %ui-btn-non; + + &.is-disabled { + background-color: $gray-l3; + color: $gray-l6; + } + } + } + } + } + + .collection-references { + @extend %t-copy-sub1; + box-shadow: 0 2px 2px 0 $shadow inset; + padding: $baseline ($baseline*1.5) $baseline ($baseline*2.5); + color: $gray-l1; + + .usage { + margin-left: $baseline; + + .usage-unit { + padding: ($baseline/4) 0; + + a { + @extend %t-strong; + } + + .fa-warning { + margin: ($baseline/4) ($baseline/2) 0 ($baseline*1.5); + color: $orange; + } + + .fa-times-circle { + margin: ($baseline/4) ($baseline/2) 0 ($baseline*1.5); + color: $red-l2; + } + } + } + } + + .usage-validation { @extend %t-copy-sub1; background-color: $gray-l6; margin-top: $baseline; @@ -230,17 +223,21 @@ float: left; } - .group-configuration-validation-text { + .collection-validation-text { overflow: auto; } } - .group-configuration-edit { + .collection-edit { @include box-sizing(border-box); border-radius: 2px; width: 100%; background: $white; + .message { + margin-bottom: 0; + } + .wrapper-form { padding: $baseline ($baseline*1.5); } @@ -253,10 +250,117 @@ color: $gray-l3; } - .is-focused .tip{ + .is-focused .tip { color: $gray; } + + .collection-fields { + @extend %cont-no-list; + margin-bottom: $baseline; + } + + .field { + margin: 0 0 ($baseline*0.75) 0; + + &:last-child { + @extend %wipe-last-child; + } + + &.required { + + label { + @extend %t-strong; + } + + label:after { + margin-left: ($baseline/4); + content: "*"; + } + } + + label, input, textarea { + display: block; + } + + textarea { + resize: vertical; + } + + label { + @extend %t-copy-sub1; + @include transition(color, 0.15s, ease-in-out); + margin: 0 0 ($baseline/4) 0; + + &.is-focused { + color: $blue; + } + } + + //this section is borrowed from _account.scss - we should clean up and unify later + input, textarea { + @extend %t-copy-base; + height: 100%; + width: 100%; + padding: ($baseline/2); + + &.long { + width: 100%; + } + + &.short { + width: 25%; + } + + ::-webkit-input-placeholder { + color: $gray-l4; + } + + :-moz-placeholder { + color: $gray-l3; + } + + ::-moz-placeholder { + color: $gray-l3; + } + + :-ms-input-placeholder { + color: $gray-l3; + } + + &:focus { + + .tip { + color: $gray; + } + } + } + + &.error { + label { + color: $red; + } + + input { + border-color: $red; + } + } + } + + label.required { + @extend %t-strong; + + &:after { + margin-left: ($baseline/4); + content: "*"; + } + } + + .field.add-collection-name label { + @extend %t-title5; + display: inline-block; + vertical-align: bottom; + } + .actions { box-shadow: inset 0 1px 2px $shadow; border-top: 1px solid $gray-l1; @@ -273,23 +377,13 @@ // add a group is below with groups styling .action-primary { - @include blue-button; - @include transition(all .15s); - @extend %t-action2; - @extend %t-strong; - display: inline-block; - padding: ($baseline/5) $baseline; - text-transform: uppercase; + @extend %btn-primary-blue; + padding: ($baseline/4) $baseline; } .action-secondary { - @include grey-button; - @include transition(all .15s); - @extend %t-action2; - @extend %t-strong; - display: inline-block; - padding: ($baseline/5) $baseline; - text-transform: uppercase; + @extend %btn-secondary-gray; + padding: ($baseline/4) $baseline; } .wrapper-delete-button { @@ -311,191 +405,9 @@ @extend %t-strong; } } - - .groups-fields, - .group-configuration-fields { - @extend %cont-no-list; - - .field { - margin: 0 0 ($baseline*0.75) 0; - - &:last-child { - margin-bottom: 0; - } - - &.required { - - label { - @extend %t-strong; - } - - label:after { - margin-left: ($baseline/4); - content: "*"; - } - } - - label, input, textarea { - display: block; - } - - textarea { - resize: vertical; - } - - label { - @extend %t-copy-sub1; - @include transition(color, 0.15s, ease-in-out); - margin: 0 0 ($baseline/4) 0; - - &.is-focused { - color: $blue; - } - } - - //this section is borrowed from _account.scss - we should clean up and unify later - input, textarea { - @extend %t-copy-base; - height: 100%; - width: 100%; - padding: ($baseline/2); - - &.long { - width: 100%; - } - - &.short { - width: 25%; - } - - ::-webkit-input-placeholder { - color: $gray-l4; - } - - :-moz-placeholder { - color: $gray-l3; - } - - ::-moz-placeholder { - color: $gray-l3; - } - - :-ms-input-placeholder { - color: $gray-l3; - } - - &:focus { - + .tip { - color: $gray; - } - } - } - - &.error { - label { - color: $red; - } - - input { - border-color: $red; - } - } - - &.add-group-configuration-name label { - @extend %t-title5; - display: inline-block; - width: 50%; - padding-right: 5%; - overflow: hidden; - text-overflow: ellipsis; - vertical-align: bottom; - } - - .group-configuration-id { - display: inline-block; - width: 45%; - text-align: right; - vertical-align: top; - color: $gray-l1; - - .group-configuration-value { - @extend %t-strong; - white-space: nowrap; - margin-left: ($baseline*0.5); - } - } - - &.group-allocation { - color: $gray-l1; - } - } - - label.required { - @extend %t-strong; - - &:after { - margin-left: ($baseline/4); - content: "*"; - } - } - - .field-group { - @include clearfix(); - margin: 0 0 ($baseline/2) 0; - padding: ($baseline/4) 0 0 0; - - .group-allocation, - .field { - display: inline-block; - vertical-align: middle; - margin: 0 3% 0 0; - } - - .group-allocation { - max-width: 10%; - min-width: 5%; - color: $gray-l1; - } - - .field { - position: relative; - - &.long { - width: 80%; - } - - &.short { - width: 10%; - } - } - - .action-close { - @include transition(color $tmg-f2 ease-in-out); - @extend %t-action1; - display: inline-block; - border: 0; - padding: 0; - background: transparent; - color: $blue-l3; - vertical-align: middle; - - &:hover { - color: $blue; - } - } - } - } - - .group-configuration-fields { - margin-bottom: $baseline; - } } - &:hover .wrapper-group-configuration .actions { - opacity: 1.0; - } - - .action-add-group { + .action-add-item { @extend %ui-btn-flat-outline; @extend %t-action2; @extend %t-strong; @@ -505,6 +417,183 @@ padding: ($baseline/2); } } + + // add/new collection + .action-add { + @extend %ui-btn-flat-outline; + display: block; + width: 100%; + margin-top: ($baseline*0.75); + padding: ($baseline/2) $baseline; + + &.is-hidden { + display: none; + } + + .icon { + display: inline-block; + vertical-align: middle; + @include margin-right($baseline/2); + } + } + + // specific group-type styles + .content-groups { + + .collection-header{ + + .title { + margin-bottom: 0; + } + } + } + + .experiment-groups { + + .group-configuration-details { + + .group-configuration-info { + @extend %t-copy-sub1; + color: $gray-l1; + margin-left: $baseline; + + &.group-configuration-info-inline { + display: table; + width: 70%; + margin: ($baseline/4) 0 ($baseline/2) $baseline; + + li { + @include box-sizing(border-box); + display: table-cell; + margin-right: 1%; + + &.group-configuration-usage-count { + font-style: italic; + } + } + } + + &.group-configuration-info-block { + li { + padding: ($baseline/4) 0; + } + } + + .group-configuration-label { + text-transform: uppercase; + } + + .group-configuration-description { + overflow: hidden; + text-overflow: ellipsis; + } + } + + .groups { + margin-left: $baseline; + margin-bottom: ($baseline*0.75); + + .group { + @extend %t-copy-lead1; + padding: ($baseline/7) 0 ($baseline/4); + border-top: 1px solid $gray-l4; + white-space: nowrap; + + &:first-child { + border-top: none; + } + + .group-name { + overflow: hidden; + text-overflow: ellipsis; + display: inline-block; + vertical-align: middle; + width: 75%; + margin-right: 5%; + } + + .group-allocation { + display: inline-block; + vertical-align: middle; + width: 20%; + color: $gray-l1; + text-align: right; + } + } + } + } + + .group-configuration-edit { + + .add-collection-name label { + width: 50%; + padding-right: 5%; + overflow: hidden; + text-overflow: ellipsis; + vertical-align: bottom; + } + + .group-configuration-id { + display: inline-block; + width: 45%; + text-align: right; + vertical-align: top; + color: $gray-l1; + + .group-configuration-value { + @extend %t-strong; + white-space: nowrap; + margin-left: ($baseline*0.5); + } + } + + .field-group { + @include clearfix(); + margin: 0 0 ($baseline/2) 0; + padding: ($baseline/4) 0 0 0; + + .group-allocation, + .field { + display: inline-block; + vertical-align: middle; + margin: 0 3% 0 0; + } + + .group-allocation { + max-width: 10%; + min-width: 5%; + color: $gray-l1; + } + + .field { + position: relative; + + &.long { + width: 80%; + } + + &.short { + width: 10%; + } + } + + .action-close { + @include transition(color $tmg-f2 ease-in-out); + @extend %t-action1; + display: inline-block; + border: 0; + padding: 0; + background: transparent; + color: $blue-l3; + vertical-align: middle; + + &:hover { + color: $blue; + } + } + } + } + } } .content-supplementary { diff --git a/cms/templates/base.html b/cms/templates/base.html index 4397cfedde..176f9599a1 100644 --- a/cms/templates/base.html +++ b/cms/templates/base.html @@ -86,6 +86,9 @@ import json
+ + <%block name="modal_placeholder"> + <%block name="jsextra"> @@ -19,11 +20,9 @@ <%block name="requirejs"> -% if configurations is not None: require(["js/factories/group_configurations"], function(GroupConfigurationsFactory) { - GroupConfigurationsFactory(${json.dumps(configurations)}, "${group_configuration_url}", "${course_outline_url}"); + GroupConfigurationsFactory(${json.dumps(should_show_experiment_groups)}, ${json.dumps(experiment_group_configurations)}, ${json.dumps(content_group_configuration)}, "${group_configuration_url}", "${course_outline_url}"); }); -% endif <%block name="content"> @@ -33,45 +32,56 @@ ${_("Settings")} > ${_("Group Configurations")} -
- % if configurations is None: -
-

- ${_("This module is disabled at the moment.")} -

+
+

${_("Content Groups")}

+
+

${_("Loading")}

+
- % else: -
-

${_("Loading")}

+ + % if should_show_experiment_groups: +
+

${_("Experiment Group Configurations")}

+ % if experiment_group_configurations is None: +
+

+ ${_("This module is disabled at the moment.")} +

+
+ % else: +
+

${_("Loading")}

+
+ % endif
% endif
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore new file mode 100644 index 0000000000..e787c19cc7 --- /dev/null +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-form.underscore @@ -0,0 +1,130 @@ +<% var isNewCohort = cohort.id == null; %> +
+
+ <% if (isNewCohort) { %> +

<%- gettext('Add a New Cohort') %>

+
+ <% } %> +
+ <% + // Don't allow renaming of existing cohorts yet as it doesn't interact well with + // the course's advanced setting for auto cohorting. + if (isNewCohort) { + %> +
+
+ + " required="required" /> +
+
+ <% } %> + + <% + var foundSelected = false; + var selectedContentGroupId = cohort.get('group_id'); + var selectedUserPartitionId = cohort.get('user_partition_id'); + var hasSelectedContentGroup = selectedContentGroupId != null; + var hasContentGroups = contentGroups.length > 0; + %> +
+
+

+ <%- gettext('Associated Content Group') %> +

+ +
+ + <% if (hasContentGroups) { %> +
+ + + + <% if (hasSelectedContentGroup && !foundSelected) { %> +
+

+ + <%= + interpolate( + // Translators: Any text between %(screen_reader_start)s and %(screen_reader_end)s is only read by screen readers and never shown in the browser. + '%(screen_reader_start)sWarning:%(screen_reader_end)s The previously selected content group was deleted. Select another content group.', + { + screen_reader_start: '', + screen_reader_end: '' + }, + true + ) + %> +

+
+ <% } %> +
+ <% } else { // no content groups available %> +
+
+

+ + <%= + interpolate( + // Translators: Any text between %(screen_reader_start)s and %(screen_reader_end)s is only read by screen readers and never shown in the browser. + '%(screen_reader_start)sWarning:%(screen_reader_end)s No content groups exist.', + { + screen_reader_start: '', + screen_reader_end: '' + }, + true + ) + %> + <%- gettext("Create a content group") %> +

+
+
+ <% } %> +
+
+
+
+ <% if (isNewCohort) { %> +
+ <% } %> + +
+ + <% if (isNewCohort) { %> + <%- gettext('Cancel') %> + <% } %> +
+
+
diff --git a/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore b/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore index 36ee294685..e91c2d2c33 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohort-selector.underscore @@ -1,5 +1,5 @@ <% if (!selectedCohort) { %> - + <% } %> <% _.each(cohorts, function(cohort) { %> <% diff --git a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore index 8113089642..c2e3a9ae1f 100644 --- a/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore +++ b/lms/templates/instructor/instructor_dashboard_2/cohorts.underscore @@ -4,26 +4,28 @@
-

<%- gettext('Assign students to cohort groups manually') %>

+

<%- gettext('Assign students to cohorts manually') %>

- +
- +
- - <%- gettext('Add Cohort Group') %> + + <%- gettext('Add Cohort') %>
+ +
+
@@ -32,14 +34,14 @@
- <%- gettext('Assign students to cohort groups by uploading a CSV file') %> + <%- gettext('Assign students to cohorts by uploading a CSV file') %>

- + <%= interpolate( - gettext('To review student cohort group assignments or see the results of uploading a CSV file, download course profile information or cohort results on %(link_start)s the Data Download page. %(link_end)s'), + gettext('To review student cohort assignments or see the results of uploading a CSV file, download course profile information or cohort results on %(link_start)s the Data Download page. %(link_end)s'), {link_start: '', link_end: ''}, true ) %> diff --git a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html index a3e389f271..04d618216a 100644 --- a/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html +++ b/lms/templates/instructor/instructor_dashboard_2/instructor_dashboard_2.html @@ -57,15 +57,17 @@ - - - - + + + + + + ## Include Underscore templates <%block name="header_extras"> -% for template_name in ["cohorts", "cohort-editor", "cohort-selector", "add-cohort-form", "notification"]: +% for template_name in ["cohorts", "cohort-editor", "cohort-selector", "cohort-form", "notification"]: diff --git a/lms/templates/instructor/instructor_dashboard_2/membership.html b/lms/templates/instructor/instructor_dashboard_2/membership.html index 2d18373c37..b53b47e987 100644 --- a/lms/templates/instructor/instructor_dashboard_2/membership.html +++ b/lms/templates/instructor/instructor_dashboard_2/membership.html @@ -1,6 +1,8 @@ <%! from django.utils.translation import ugettext as _ %> <%page args="section_data"/> +<%! from courseware.courses import get_studio_url %> <%! from microsite_configuration import microsite %> +<%! from openedx.core.djangoapps.course_groups.partition_scheme import get_cohorted_user_partition %> - - - - - - - - - - - - - - - -<%static:js group='module-descriptor-js'/> -<%static:js group='instructor_dash'/> - - -

-
-
-

Instructor Dashboard

- -
- -
-

Batch Enrollment

-

- - -

- -
- - -
- -

- If this option is checked, users who have not yet registered for {platform_name} will be automatically enrolled.").format(platform_name=settings.PLATFORM_NAME)} - If this option is left unchecked, users who have not yet registered for {platform_name} will not be enrolled, but will be allowed to enroll once they make an account.").format(platform_name=settings.PLATFORM_NAME)} -

- Checking this box has no effect if 'Unenroll' is selected. -

-
-
- -
- - - -
- -
- - -
-
-
-
- -
- -
-

Batch Beta Tester Addition

-

- - - -

- -
- - -
- -

- If this option is checked, users who have not enrolled in your course will be automatically enrolled. -

- Checking this box has no effect if 'Remove beta testers' is selected. -

-
-
- -
- - - -
- -
- - -
- -
-
-
- -
- -
- ## Translators: an "Administration List" is a list, such as Course Staff, that users can be added to. -

Administration List Management

- -
- -
- ## Translators: an "Administrator Group" is a group, such as Course Staff, that users can be added to. - - - -
- - -

- Staff cannot modify staff or beta tester lists. To modify these lists, " - "contact your instructor and ask them to add you as an instructor for staff " - "and beta lists, or a discussion admin for discussion management. -

- -
- -
- -
- -
- -
- -
-
- -
- -
-

- Cohort Management - Cohorts are such and such and are used for this and that to do all the things -

- - -
-
- -
-
- - - -
-
- -
- -
-
- - - - Add Cohort Group - -
- - -
-

You currently have no cohort groups configured

- -
-

Please complete your cohort group configuration by creating groups within Studio

-
- -
- -
-
- - -
-

There's currently an error with your cohorts configuration within this course.

- -
-

Error output (if any and near-human legible/comprehendable can be displayed here)

-
- -
- Review Configuration in Studio -
-
- - -
-
- -

Add a New Cohort Group

- -
-
- - -
-
- -
- - Cancel -
-
-
- - -
-
- -

Editing "Cohort Group's Name"

- -
-
- - -
-
- -
- - Cancel -
-
-
- - -
-

New Cohort Name has been created. You can manually add students to this group below.

-
- - -
-
-

- Cohort Name Can be Placed Here and Should Accommodate Almost Everything - (contains 12,546 Students) - - Edit -

- -
-

This cohort group's current management set up:

-
- Students are added to this group automatically. - What does this mean? -
- -
- Edit settings in Studio -
-
- -
-

This cohort group's current management set up:

-
- Students are added to this group only when you provide their email addresses or usernames on this page. - What does this mean? -
- -
- Edit settings in Studio -
-
-
- - -
-
- -

Add students to this cohort group

- -
-

Note: Students can only be in one cohort group. Adding students to this group overrides any previous group assignment.

-
- - -
-

2,546 students have been added to this cohort group

- -
-
    -
  • 1,245 were removed from Cohort Name is Placed Here and Should Accommodate Almost Everything
    • -
  • 1,245 were removed from Cohort Name is Placed Here and Should Accommodate Almost Everything
    • -
  • 1,245 were removed from Cohort Name is Placed Here and Should Accommodate Almost Everything
    • -
-
-
- - -
-

There were 25 errors when trying to add students:

- -
-
    -
  • Unknown user: ahgaeubgoq
    • -
  • Unknown user: hagaihga
    • -
  • Unknown user: ahgaeubgoq
    • -
  • Unknown user: ahgaeubgoq
    • -
  • Unknown user: hagaihga
    • -
-
- -
- View all errors -
-
- -
-
- - - - - You will not get notification for emails that bounce, so please double-check spelling. -
-
- -
- -
-
-
-
- - -
-

You may view individual student information for each cohort via your entire course profile data download on the data download view

-
-
-
-
-
-
diff --git a/lms/urls.py b/lms/urls.py index 4af2de7571..3bf18ff8d3 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -345,11 +345,8 @@ if settings.COURSEWARE_ENABLED: 'open_ended_grading.views.take_action_on_flags', name='open_ended_flagged_problems_take_action'), # Cohorts management - url(r'^courses/{}/cohorts$'.format(settings.COURSE_KEY_PATTERN), - 'openedx.core.djangoapps.course_groups.views.list_cohorts', name="cohorts"), - url(r'^courses/{}/cohorts/add$'.format(settings.COURSE_KEY_PATTERN), - 'openedx.core.djangoapps.course_groups.views.add_cohort', - name="add_cohort"), + url(r'^courses/{}/cohorts/(?P[0-9]+)?$'.format(settings.COURSE_KEY_PATTERN), + 'openedx.core.djangoapps.course_groups.views.cohort_handler', name="cohorts"), url(r'^courses/{}/cohorts/(?P[0-9]+)$'.format(settings.COURSE_KEY_PATTERN), 'openedx.core.djangoapps.course_groups.views.users_in_cohort', name="list_cohort"), @@ -389,7 +386,8 @@ if settings.COURSEWARE_ENABLED: # allow course staff to change to student view of courseware if settings.FEATURES.get('ENABLE_MASQUERADE'): urlpatterns += ( - url(r'^masquerade/(?P.*)$', 'courseware.masquerade.handle_ajax', name="masquerade-switch"), + url(r'^courses/{}/masquerade$'.format(settings.COURSE_KEY_PATTERN), + 'courseware.masquerade.handle_ajax', name="masquerade_update"), ) # discussion forums live within courseware, so courseware must be enabled first diff --git a/openedx/core/djangoapps/course_groups/cohorts.py b/openedx/core/djangoapps/course_groups/cohorts.py index 7fba71cafa..ed98ebf80e 100644 --- a/openedx/core/djangoapps/course_groups/cohorts.py +++ b/openedx/core/djangoapps/course_groups/cohorts.py @@ -1,5 +1,5 @@ """ -This file contains the logic for cohort groups, as exposed internally to the +This file contains the logic for cohorts, as exposed internally to the forums, and to the cohort admin views. """ @@ -16,6 +16,7 @@ from eventtracking import tracker from student.models import get_user_by_username_or_email from .models import CourseUserGroup, CourseUserGroupPartitionGroup + log = logging.getLogger(__name__) @@ -87,8 +88,8 @@ class CohortAssignmentType(object): # No automatic rules are applied to this cohort; users must be manually added. NONE = "none" - # One of (possibly) multiple cohort groups to which users are randomly assigned. - # Note: The 'default cohort' group is included in this category iff it exists and + # One of (possibly) multiple cohorts to which users are randomly assigned. + # Note: The 'default' cohort is included in this category iff it exists and # there are no other random groups. (Also see Note 2 above.) RANDOM = "random" @@ -381,15 +382,15 @@ def add_user_to_cohort(cohort, username_or_email): return (user, previous_cohort_name) -def get_partition_group_id_for_cohort(cohort): +def get_group_info_for_cohort(cohort): """ - Get the ids of the partition and group to which this cohort has been linked + Get the ids of the group and partition to which this cohort has been linked as a tuple of (int, int). - If the cohort has not been linked to any partition/group, both values in the + If the cohort has not been linked to any group/partition, both values in the tuple will be None. """ res = CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort) if len(res): - return res[0].partition_id, res[0].group_id + return res[0].group_id, res[0].partition_id return None, None diff --git a/openedx/core/djangoapps/course_groups/partition_scheme.py b/openedx/core/djangoapps/course_groups/partition_scheme.py index cc36c14373..bb1530ac38 100644 --- a/openedx/core/djangoapps/course_groups/partition_scheme.py +++ b/openedx/core/djangoapps/course_groups/partition_scheme.py @@ -3,7 +3,12 @@ Provides a UserPartition driver for cohorts. """ import logging -from .cohorts import get_cohort, get_partition_group_id_for_cohort +from courseware import courses +from courseware.masquerade import get_masquerading_group_info +from xmodule.partitions.partitions import NoSuchUserPartitionGroupError + +from .cohorts import get_cohort, get_group_info_for_cohort + log = logging.getLogger(__name__) @@ -15,8 +20,9 @@ class CohortPartitionScheme(object): Groups. """ + # pylint: disable=unused-argument @classmethod - def get_group_for_user(cls, course_id, user, user_partition, track_function=None): + def get_group_for_user(cls, course_key, user, user_partition, track_function=None): """ Returns the Group from the specified user partition to which the user is assigned, via their cohort membership and any mappings from cohorts @@ -30,12 +36,22 @@ class CohortPartitionScheme(object): If the user has no cohort mapping, or there is no (valid) cohort -> partition group mapping found, the function returns None. """ - cohort = get_cohort(user, course_id) + # If the current user is masquerading as being in a group belonging to the + # specified user partition then return the masquerading group. + group_id, user_partition_id = get_masquerading_group_info(user, course_key) + if group_id is not None and user_partition_id == user_partition.id: + try: + return user_partition.get_group(group_id) + except NoSuchUserPartitionGroupError: + # If the group no longer exists then the masquerade is not in effect + pass + + cohort = get_cohort(user, course_key) if cohort is None: # student doesn't have a cohort return None - partition_id, group_id = get_partition_group_id_for_cohort(cohort) + group_id, partition_id = get_group_info_for_cohort(cohort) if partition_id is None: # cohort isn't mapped to any partition group. return None @@ -56,8 +72,9 @@ class CohortPartitionScheme(object): # fail silently return None - group = user_partition.get_group(group_id) - if group is None: + try: + return user_partition.get_group(group_id) + except NoSuchUserPartitionGroupError: # if we have a match but the group doesn't exist in the partition, # it means the mapping is invalid. the previous state of the # partition configuration may have been modified. @@ -67,9 +84,22 @@ class CohortPartitionScheme(object): "requested_partition_id": user_partition.id, "requested_group_id": group_id, "cohort_id": cohort.id, - } + }, + exc_info=True ) # fail silently return None - return group + +def get_cohorted_user_partition(course_key): + """ + Returns the first user partition from the specified course which uses the CohortPartitionScheme, + or None if one is not found. Note that it is currently recommended that each course have only + one cohorted user partition. + """ + course = courses.get_course_by_id(course_key) + for user_partition in course.user_partitions: + if user_partition.scheme == CohortPartitionScheme: + return user_partition + + return None diff --git a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py index 1707dd0530..aed51f7a23 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_cohorts.py +++ b/openedx/core/djangoapps/course_groups/tests/test_cohorts.py @@ -623,13 +623,13 @@ class TestCohortsAndPartitionGroups(TestCase): link.save() return link - def test_get_partition_group_id_for_cohort(self): + def test_get_group_info_for_cohort(self): """ - Basic test of the partition_group_id accessor function + Basic test of the partition_group_info accessor function """ # api should return nothing for an unmapped cohort self.assertEqual( - cohorts.get_partition_group_id_for_cohort(self.first_cohort), + cohorts.get_group_info_for_cohort(self.first_cohort), (None, None), ) # create a link for the cohort in the db @@ -640,14 +640,14 @@ class TestCohortsAndPartitionGroups(TestCase): ) # api should return the specified partition and group self.assertEqual( - cohorts.get_partition_group_id_for_cohort(self.first_cohort), - (self.partition_id, self.group1_id) + cohorts.get_group_info_for_cohort(self.first_cohort), + (self.group1_id, self.partition_id) ) # delete the link in the db link.delete() # api should return nothing again self.assertEqual( - cohorts.get_partition_group_id_for_cohort(self.first_cohort), + cohorts.get_group_info_for_cohort(self.first_cohort), (None, None), ) @@ -666,12 +666,12 @@ class TestCohortsAndPartitionGroups(TestCase): self.group1_id, ) self.assertEqual( - cohorts.get_partition_group_id_for_cohort(self.first_cohort), - (self.partition_id, self.group1_id), + cohorts.get_group_info_for_cohort(self.first_cohort), + (self.group1_id, self.partition_id), ) self.assertEqual( - cohorts.get_partition_group_id_for_cohort(self.second_cohort), - cohorts.get_partition_group_id_for_cohort(self.first_cohort), + cohorts.get_group_info_for_cohort(self.second_cohort), + cohorts.get_group_info_for_cohort(self.first_cohort), ) def test_multiple_partition_groups(self): @@ -701,14 +701,14 @@ class TestCohortsAndPartitionGroups(TestCase): self.group1_id ) self.assertEqual( - cohorts.get_partition_group_id_for_cohort(self.first_cohort), - (self.partition_id, self.group1_id) + cohorts.get_group_info_for_cohort(self.first_cohort), + (self.group1_id, self.partition_id) ) # delete the link self.first_cohort.delete() # api should return nothing at that point self.assertEqual( - cohorts.get_partition_group_id_for_cohort(self.first_cohort), + cohorts.get_group_info_for_cohort(self.first_cohort), (None, None), ) # link should no longer exist because of delete cascade diff --git a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py index 6e0f2e1a4b..965dc9a07b 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py +++ b/openedx/core/djangoapps/course_groups/tests/test_partition_scheme.py @@ -3,26 +3,32 @@ Test the partitions and partitions service """ +import json from django.conf import settings import django.test from django.test.utils import override_settings from mock import patch +from unittest import skipUnless +from courseware.masquerade import handle_ajax, setup_masquerade +from courseware.tests.test_masquerade import StaffMasqueradeTestCase from student.tests.factories import UserFactory from xmodule.partitions.partitions import Group, UserPartition, UserPartitionError from xmodule.modulestore.django import modulestore, clear_existing_modulestores from xmodule.modulestore.tests.django_utils import mixed_store_config from opaque_keys.edx.locations import SlashSeparatedCourseKey -from ..partition_scheme import CohortPartitionScheme +from openedx.core.djangoapps.user_api.partition_schemes import RandomUserPartitionScheme +from ..partition_scheme import CohortPartitionScheme, get_cohorted_user_partition from ..models import CourseUserGroupPartitionGroup -from ..cohorts import add_user_to_cohort +from ..views import link_cohort_to_partition_group, unlink_cohort_partition_group +from ..cohorts import add_user_to_cohort, get_course_cohorts from .helpers import CohortFactory, config_course_cohorts TEST_DATA_DIR = settings.COMMON_TEST_DATA_ROOT TEST_MAPPING = {'edX/toy/2012_Fall': 'xml'} -TEST_DATA_MIXED_MODULESTORE = mixed_store_config(TEST_DATA_DIR, TEST_MAPPING) +TEST_DATA_MIXED_MODULESTORE = mixed_store_config(TEST_DATA_DIR, TEST_MAPPING, include_xml=True) @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) @@ -37,7 +43,8 @@ class TestCohortPartitionScheme(django.test.TestCase): and a student for each test. """ self.course_key = SlashSeparatedCourseKey("edX", "toy", "2012_Fall") - config_course_cohorts(modulestore().get_course(self.course_key), [], cohorted=True) + self.course = modulestore().get_course(self.course_key) + config_course_cohorts(self.course, [], cohorted=True) self.groups = [Group(10, 'Group 10'), Group(20, 'Group 20')] self.user_partition = UserPartition( @@ -49,22 +56,6 @@ class TestCohortPartitionScheme(django.test.TestCase): ) self.student = UserFactory.create() - def link_cohort_partition_group(self, cohort, partition, group): - """ - Utility for creating cohort -> partition group links - """ - CourseUserGroupPartitionGroup( - course_user_group=cohort, - partition_id=partition.id, - group_id=group.id, - ).save() - - def unlink_cohort_partition_group(self, cohort): - """ - Utility for removing cohort -> partition group links - """ - CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete() - def assert_student_in_group(self, group, partition=None): """ Utility for checking that our test student comes up as assigned to the @@ -93,16 +84,16 @@ class TestCohortPartitionScheme(django.test.TestCase): self.assert_student_in_group(None) # link first cohort to group 0 in the partition - self.link_cohort_partition_group( + link_cohort_to_partition_group( first_cohort, - self.user_partition, - self.groups[0], + self.user_partition.id, + self.groups[0].id, ) # link second cohort to to group 1 in the partition - self.link_cohort_partition_group( + link_cohort_to_partition_group( second_cohort, - self.user_partition, - self.groups[1], + self.user_partition.id, + self.groups[1].id, ) self.assert_student_in_group(self.groups[0]) @@ -127,31 +118,55 @@ class TestCohortPartitionScheme(django.test.TestCase): self.assert_student_in_group(None) # link cohort to group 0 - self.link_cohort_partition_group( + link_cohort_to_partition_group( test_cohort, - self.user_partition, - self.groups[0], + self.user_partition.id, + self.groups[0].id, ) # now the scheme should find a link self.assert_student_in_group(self.groups[0]) # link cohort to group 1 (first unlink it from group 0) - self.unlink_cohort_partition_group(test_cohort) - self.link_cohort_partition_group( + unlink_cohort_partition_group(test_cohort) + link_cohort_to_partition_group( test_cohort, - self.user_partition, - self.groups[1], + self.user_partition.id, + self.groups[1].id, ) # scheme should pick up the link self.assert_student_in_group(self.groups[1]) # unlink cohort from anywhere - self.unlink_cohort_partition_group( + unlink_cohort_partition_group( test_cohort, ) # scheme should now return nothing self.assert_student_in_group(None) + def test_student_lazily_assigned(self): + """ + Test that the lazy assignment of students to cohorts works + properly when accessed via the CohortPartitionScheme. + """ + # don't assign the student to any cohort initially + self.assert_student_in_group(None) + + # get the default cohort, which is automatically created + # during the `get_course_cohorts` API call if it doesn't yet exist + cohort = get_course_cohorts(self.course)[0] + + # map that cohort to a group in our partition + link_cohort_to_partition_group( + cohort, + self.user_partition.id, + self.groups[0].id, + ) + + # The student will be lazily assigned to the default cohort + # when CohortPartitionScheme.get_group_for_user makes its internal + # call to cohorts.get_cohort. + self.assert_student_in_group(self.groups[0]) + def setup_student_in_group_0(self): """ Utility to set up a cohort, add our student to the cohort, and link @@ -160,10 +175,10 @@ class TestCohortPartitionScheme(django.test.TestCase): test_cohort = CohortFactory(course_id=self.course_key) # link cohort to group 0 - self.link_cohort_partition_group( + link_cohort_to_partition_group( test_cohort, - self.user_partition, - self.groups[0], + self.user_partition.id, + self.groups[0].id, ) # place student into cohort add_user_to_cohort(test_cohort, self.student.username) @@ -255,3 +270,110 @@ class TestExtension(django.test.TestCase): self.assertEqual(UserPartition.get_scheme('cohort'), CohortPartitionScheme) with self.assertRaisesRegexp(UserPartitionError, 'Unrecognized scheme'): UserPartition.get_scheme('other') + + +@override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE) +class TestGetCohortedUserPartition(django.test.TestCase): + """ + Test that `get_cohorted_user_partition` returns the first user_partition with scheme `CohortPartitionScheme`. + """ + def setUp(self): + """ + Regenerate a course with cohort configuration, partition and groups, + and a student for each test. + """ + self.course_key = SlashSeparatedCourseKey("edX", "toy", "2012_Fall") + self.course = modulestore().get_course(self.course_key) + self.student = UserFactory.create() + + self.random_user_partition = UserPartition( + 1, + 'Random Partition', + 'Should not be returned', + [Group(0, 'Group 0'), Group(1, 'Group 1')], + scheme=RandomUserPartitionScheme + ) + + self.cohort_user_partition = UserPartition( + 0, + 'Cohort Partition 1', + 'Should be returned', + [Group(10, 'Group 10'), Group(20, 'Group 20')], + scheme=CohortPartitionScheme + ) + + self.second_cohort_user_partition = UserPartition( + 2, + 'Cohort Partition 2', + 'Should not be returned', + [Group(10, 'Group 10'), Group(1, 'Group 1')], + scheme=CohortPartitionScheme + ) + + def test_returns_first_cohort_user_partition(self): + """ + Test get_cohorted_user_partition returns first user_partition with scheme `CohortPartitionScheme`. + """ + self.course.user_partitions.append(self.random_user_partition) + self.course.user_partitions.append(self.cohort_user_partition) + self.course.user_partitions.append(self.second_cohort_user_partition) + self.assertEqual(self.cohort_user_partition, get_cohorted_user_partition(self.course_key)) + + def test_no_cohort_user_partitions(self): + """ + Test get_cohorted_user_partition returns None when there are no cohorted user partitions. + """ + self.course.user_partitions.append(self.random_user_partition) + self.assertIsNone(get_cohorted_user_partition(self.course_key)) + + +class TestMasqueradedGroup(StaffMasqueradeTestCase): + """ + Check for staff being able to masquerade as belonging to a group. + """ + def setUp(self): + super(TestMasqueradedGroup, self).setUp() + self.user_partition = UserPartition( + 0, 'Test User Partition', '', + [Group(0, 'Group 1'), Group(1, 'Group 2')], + scheme_id='cohort' + ) + self.course.user_partitions.append(self.user_partition) + self.session = {} + modulestore().update_item(self.course, self.test_user.id) + + def _verify_masquerade_for_group(self, group): + """ + Verify that the masquerade works for the specified group id. + """ + # Send the request to set the masquerade + request_json = { + "role": "student", + } + if group and self.user_partition: + request_json['user_partition_id'] = self.user_partition.id + request_json['group_id'] = group.id + request = self._create_mock_json_request( + self.test_user, + body=json.dumps(request_json), + session=self.session + ) + handle_ajax(request, unicode(self.course.id)) + + # Now setup the masquerade for the test user + setup_masquerade(request, self.test_user, True) + scheme = self.user_partition.scheme # pylint: disable=no-member + self.assertEqual( + scheme.get_group_for_user(self.course.id, self.test_user, self.user_partition), + group + ) + + @skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in LMS') + @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) + def test_group_masquerade(self): + """ + Tests that a staff member can masquerade as being in a particular group. + """ + self._verify_masquerade_for_group(self.user_partition.groups[0]) + self._verify_masquerade_for_group(self.user_partition.groups[1]) + self._verify_masquerade_for_group(None) diff --git a/openedx/core/djangoapps/course_groups/tests/test_views.py b/openedx/core/djangoapps/course_groups/tests/test_views.py index 712c966ac8..ca7ad34f27 100644 --- a/openedx/core/djangoapps/course_groups/tests/test_views.py +++ b/openedx/core/djangoapps/course_groups/tests/test_views.py @@ -15,11 +15,17 @@ from student.models import CourseEnrollment from student.tests.factories import UserFactory from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory +from xmodule.modulestore.django import modulestore from opaque_keys.edx.locations import SlashSeparatedCourseKey from ..models import CourseUserGroup -from ..views import list_cohorts, add_cohort, users_in_cohort, add_users_to_cohort, remove_user_from_cohort -from ..cohorts import get_cohort, CohortAssignmentType, get_cohort_by_name, DEFAULT_COHORT_NAME +from ..views import ( + cohort_handler, users_in_cohort, add_users_to_cohort, remove_user_from_cohort, link_cohort_to_partition_group +) +from ..cohorts import ( + get_cohort, CohortAssignmentType, get_cohort_by_name, get_cohort_by_id, + DEFAULT_COHORT_NAME, get_group_info_for_cohort +) from .helpers import config_course_cohorts, CohortFactory @@ -78,58 +84,82 @@ class CohortViewsTestCase(ModuleStoreTestCase): self.assertRaises(Http404, view, *view_args) -class ListCohortsTestCase(CohortViewsTestCase): +class CohortHandlerTestCase(CohortViewsTestCase): """ - Tests the `list_cohorts` view. + Tests the `cohort_handler` view. """ - def request_list_cohorts(self, course): + def get_cohort_handler(self, course, cohort=None): """ - Call `list_cohorts` for a given `course` and return its response as a - dict. + Call a GET on `cohort_handler` for a given `course` and return its response as a + dict. If `cohort` is specified, only information for that specific cohort is returned. """ request = RequestFactory().get("dummy_url") request.user = self.staff_user - response = list_cohorts(request, course.id.to_deprecated_string()) + if cohort: + response = cohort_handler(request, unicode(course.id), cohort.id) + else: + response = cohort_handler(request, unicode(course.id)) self.assertEqual(response.status_code, 200) return json.loads(response.content) + def put_cohort_handler(self, course, cohort=None, data=None, expected_response_code=200): + """ + Call a PUT on `cohort_handler` for a given `course` and return its response as a + dict. If `cohort` is not specified, a new cohort is created. If `cohort` is specified, + the existing cohort is updated. + """ + if not isinstance(data, basestring): + data = json.dumps(data or {}) + request = RequestFactory().put(path="dummy path", data=data, content_type="application/json") + request.user = self.staff_user + if cohort: + response = cohort_handler(request, unicode(course.id), cohort.id) + else: + response = cohort_handler(request, unicode(course.id)) + self.assertEqual(response.status_code, expected_response_code) + return json.loads(response.content) + def verify_lists_expected_cohorts(self, expected_cohorts, response_dict=None): """ Verify that the server response contains the expected_cohorts. If response_dict is None, the list of cohorts is requested from the server. """ if response_dict is None: - response_dict = self.request_list_cohorts(self.course) + response_dict = self.get_cohort_handler(self.course) - self.assertTrue(response_dict.get("success")) - self.assertItemsEqual( + self.assertEqual( response_dict.get("cohorts"), [ { "name": cohort.name, "id": cohort.id, "user_count": cohort.user_count, - "assignment_type": cohort.assignment_type + "assignment_type": cohort.assignment_type, + "user_partition_id": None, + "group_id": None } for cohort in expected_cohorts ] ) @staticmethod - def create_expected_cohort(cohort, user_count, assignment_type): + def create_expected_cohort(cohort, user_count, assignment_type, user_partition_id=None, group_id=None): """ Create a tuple storing the expected cohort information. """ - cohort_tuple = namedtuple("Cohort", "name id user_count assignment_type") + cohort_tuple = namedtuple("Cohort", "name id user_count assignment_type user_partition_id group_id") return cohort_tuple( - name=cohort.name, id=cohort.id, user_count=user_count, assignment_type=assignment_type + name=cohort.name, id=cohort.id, user_count=user_count, assignment_type=assignment_type, + user_partition_id=user_partition_id, group_id=group_id ) def test_non_staff(self): """ - Verify that we cannot access list_cohorts if we're a non-staff user. + Verify that we cannot access cohort_handler if we're a non-staff user. """ - self._verify_non_staff_cannot_access(list_cohorts, "GET", [self.course.id.to_deprecated_string()]) + self._verify_non_staff_cannot_access(cohort_handler, "GET", [unicode(self.course.id)]) + self._verify_non_staff_cannot_access(cohort_handler, "POST", [unicode(self.course.id)]) + self._verify_non_staff_cannot_access(cohort_handler, "PUT", [unicode(self.course.id)]) def test_no_cohorts(self): """ @@ -143,9 +173,9 @@ class ListCohortsTestCase(CohortViewsTestCase): """ self._create_cohorts() expected_cohorts = [ - ListCohortsTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE), - ListCohortsTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE), - ListCohortsTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE), + CohortHandlerTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE), + CohortHandlerTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE), + CohortHandlerTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE), ] self.verify_lists_expected_cohorts(expected_cohorts) @@ -159,16 +189,16 @@ class ListCohortsTestCase(CohortViewsTestCase): # Will create cohort1, cohort2, and cohort3. Auto cohorts remain uncreated. self._create_cohorts() # Get the cohorts from the course, which will cause auto cohorts to be created. - actual_cohorts = self.request_list_cohorts(self.course) + actual_cohorts = self.get_cohort_handler(self.course) # Get references to the created auto cohorts. auto_cohort_1 = get_cohort_by_name(self.course.id, "AutoGroup1") auto_cohort_2 = get_cohort_by_name(self.course.id, "AutoGroup2") expected_cohorts = [ - ListCohortsTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE), - ListCohortsTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE), - ListCohortsTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE), - ListCohortsTestCase.create_expected_cohort(auto_cohort_1, 0, CohortAssignmentType.RANDOM), - ListCohortsTestCase.create_expected_cohort(auto_cohort_2, 0, CohortAssignmentType.RANDOM), + CohortHandlerTestCase.create_expected_cohort(self.cohort1, 3, CohortAssignmentType.NONE), + CohortHandlerTestCase.create_expected_cohort(self.cohort2, 2, CohortAssignmentType.NONE), + CohortHandlerTestCase.create_expected_cohort(self.cohort3, 2, CohortAssignmentType.NONE), + CohortHandlerTestCase.create_expected_cohort(auto_cohort_1, 0, CohortAssignmentType.RANDOM), + CohortHandlerTestCase.create_expected_cohort(auto_cohort_2, 0, CohortAssignmentType.RANDOM), ] self.verify_lists_expected_cohorts(expected_cohorts, actual_cohorts) @@ -195,94 +225,197 @@ class ListCohortsTestCase(CohortViewsTestCase): # verify the default cohort is automatically created default_cohort = get_cohort_by_name(self.course.id, DEFAULT_COHORT_NAME) - actual_cohorts = self.request_list_cohorts(self.course) + actual_cohorts = self.get_cohort_handler(self.course) self.verify_lists_expected_cohorts( - [ListCohortsTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.RANDOM)], + [CohortHandlerTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.RANDOM)], actual_cohorts, ) # set auto_cohort_groups and verify the default cohort is no longer listed as RANDOM config_course_cohorts(self.course, [], cohorted=True, auto_cohort_groups=["AutoGroup"]) - actual_cohorts = self.request_list_cohorts(self.course) + actual_cohorts = self.get_cohort_handler(self.course) auto_cohort = get_cohort_by_name(self.course.id, "AutoGroup") self.verify_lists_expected_cohorts( [ - ListCohortsTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.NONE), - ListCohortsTestCase.create_expected_cohort(auto_cohort, 0, CohortAssignmentType.RANDOM), + CohortHandlerTestCase.create_expected_cohort(default_cohort, len(users), CohortAssignmentType.NONE), + CohortHandlerTestCase.create_expected_cohort(auto_cohort, 0, CohortAssignmentType.RANDOM), ], actual_cohorts, ) - -class AddCohortTestCase(CohortViewsTestCase): - """ - Tests the `add_cohort` view. - """ - def request_add_cohort(self, cohort_name, course): + def test_get_single_cohort(self): """ - Call `add_cohort` and return its response as a dict. - """ - request = RequestFactory().post("dummy_url", {"name": cohort_name}) - request.user = self.staff_user - response = add_cohort(request, course.id.to_deprecated_string()) - self.assertEqual(response.status_code, 200) - return json.loads(response.content) - - def verify_contains_added_cohort(self, response_dict, cohort_name, expected_error_msg=None): - """ - Check that `add_cohort`'s response correctly returns the newly added - cohort (or error) in the response. Also verify that the cohort was - actually created/exists. - """ - if expected_error_msg is not None: - self.assertFalse(response_dict.get("success")) - self.assertEqual( - response_dict.get("msg"), - expected_error_msg - ) - else: - self.assertTrue(response_dict.get("success")) - self.assertEqual( - response_dict.get("cohort").get("name"), - cohort_name - ) - self.assertIsNotNone(get_cohort_by_name(self.course.id, cohort_name)) - - def test_non_staff(self): - """ - Verify that non-staff users cannot access add_cohort. - """ - self._verify_non_staff_cannot_access(add_cohort, "POST", [self.course.id.to_deprecated_string()]) - - def test_new_cohort(self): - """ - Verify that we can add a new cohort. - """ - cohort_name = "New Cohort" - self.verify_contains_added_cohort( - self.request_add_cohort(cohort_name, self.course), - cohort_name, - ) - - def test_no_cohort(self): - """ - Verify that we cannot explicitly add no cohort. - """ - response_dict = self.request_add_cohort("", self.course) - self.assertFalse(response_dict.get("success")) - self.assertEqual(response_dict.get("msg"), "No name specified") - - def test_existing_cohort(self): - """ - Verify that we cannot add a cohort with the same name as an existing - cohort. + Tests that information for just a single cohort can be requested. """ self._create_cohorts() - cohort_name = self.cohort1.name - self.verify_contains_added_cohort( - self.request_add_cohort(cohort_name, self.course), - cohort_name, - expected_error_msg="You cannot create two cohorts with the same name" + response_dict = self.get_cohort_handler(self.course, self.cohort2) + self.assertEqual( + response_dict, + { + "name": self.cohort2.name, + "id": self.cohort2.id, + "user_count": 2, + "assignment_type": "none", + "user_partition_id": None, + "group_id": None + } + ) + + ############### Tests of adding a new cohort ############### + + def verify_contains_added_cohort( + self, response_dict, cohort_name, expected_user_partition_id=None, expected_group_id=None + ): + """ + Verifies that the cohort was created properly and the correct response was returned. + """ + created_cohort = get_cohort_by_name(self.course.id, cohort_name) + self.assertIsNotNone(created_cohort) + self.assertEqual( + response_dict, + { + "name": cohort_name, + "id": created_cohort.id, + "user_count": 0, + "assignment_type": CohortAssignmentType.NONE, + "user_partition_id": expected_user_partition_id, + "group_id": expected_group_id + } + ) + self.assertEqual((expected_group_id, expected_user_partition_id), get_group_info_for_cohort(created_cohort)) + + def test_create_new_cohort(self): + """ + Verify that a new cohort can be created, with and without user_partition_id/group_id information. + """ + new_cohort_name = "New cohort unassociated to content groups" + response_dict = self.put_cohort_handler(self.course, data={'name': new_cohort_name}) + self.verify_contains_added_cohort(response_dict, new_cohort_name) + + new_cohort_name = "New cohort linked to group" + response_dict = self.put_cohort_handler( + self.course, data={'name': new_cohort_name, 'user_partition_id': 1, 'group_id': 2} + ) + self.verify_contains_added_cohort(response_dict, new_cohort_name, 1, 2) + + def test_create_new_cohort_missing_name(self): + """ + Verify that we cannot create a cohort without specifying a name. + """ + response_dict = self.put_cohort_handler(self.course, expected_response_code=400) + self.assertEqual("In order to create a cohort, a name must be specified.", response_dict.get("error")) + + def test_create_new_cohort_existing_name(self): + """ + Verify that we cannot add a cohort with the same name as an existing cohort. + """ + self._create_cohorts() + response_dict = self.put_cohort_handler( + self.course, data={'name': self.cohort1.name}, expected_response_code=400 + ) + self.assertEqual("You cannot create two cohorts with the same name", response_dict.get("error")) + + def test_create_new_cohort_missing_user_partition_id(self): + """ + Verify that we cannot create a cohort with a group_id if the user_partition_id is not also specified. + """ + response_dict = self.put_cohort_handler( + self.course, data={'name': "Cohort missing user_partition_id", 'group_id': 2}, expected_response_code=400 + ) + self.assertEqual( + "If group_id is specified, user_partition_id must also be specified.", response_dict.get("error") + ) + + ############### Tests of updating an existing cohort ############### + + def test_update_manual_cohort_name(self): + """ + Test that it is possible to update the name of an existing manual cohort. + """ + self._create_cohorts() + updated_name = self.cohort1.name + "_updated" + response_dict = self.put_cohort_handler(self.course, self.cohort1, {'name': updated_name}) + self.assertEqual(updated_name, get_cohort_by_id(self.course.id, self.cohort1.id).name) + self.assertEqual(updated_name, response_dict.get("name")) + self.assertEqual(CohortAssignmentType.NONE, response_dict.get("assignment_type")) + self.assertEqual(CohortAssignmentType.NONE, CohortAssignmentType.get(self.cohort1, self.course)) + + def test_update_random_cohort_name_not_supported(self): + """ + Test that it is not possible to update the name of an existing random cohort. + """ + random_cohort = CohortFactory(course_id=self.course.id) + random_cohort_name = random_cohort.name + + # Update course cohort_config so random_cohort is in the list of auto cohorts. + self.course.cohort_config["auto_cohort_groups"] = [random_cohort_name] + modulestore().update_item(self.course, self.staff_user.id) + + updated_name = random_cohort.name + "_updated" + response_dict = self.put_cohort_handler( + self.course, random_cohort, {'name': updated_name}, expected_response_code=400 + ) + self.assertEqual( + "Renaming of random cohorts is not supported at this time.", response_dict.get("error") + ) + self.assertEqual(random_cohort_name, get_cohort_by_id(self.course.id, random_cohort.id).name) + self.assertEqual(CohortAssignmentType.RANDOM, CohortAssignmentType.get(random_cohort, self.course)) + + def test_update_cohort_group_id(self): + """ + Test that it is possible to update the user_partition_id/group_id of an existing cohort. + """ + self._create_cohorts() + self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1)) + response_dict = self.put_cohort_handler( + self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2, 'user_partition_id': 3} + ) + self.assertEqual((2, 3), get_group_info_for_cohort(self.cohort1)) + self.assertEqual(2, response_dict.get("group_id")) + self.assertEqual(3, response_dict.get("user_partition_id")) + # Check that the name didn't change. + self.assertEqual(self.cohort1.name, response_dict.get("name")) + + def test_update_cohort_remove_group_id(self): + """ + Test that it is possible to remove the user_partition_id/group_id linking of an existing cohort. + """ + self._create_cohorts() + link_cohort_to_partition_group(self.cohort1, 5, 0) + self.assertEqual((0, 5), get_group_info_for_cohort(self.cohort1)) + response_dict = self.put_cohort_handler( + self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': None} + ) + self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1)) + self.assertIsNone(response_dict.get("group_id")) + self.assertIsNone(response_dict.get("user_partition_id")) + + def test_change_cohort_group_id(self): + """ + Test that it is possible to change the user_partition_id/group_id of an existing cohort to a + different group_id. + """ + self._create_cohorts() + self.assertEqual((None, None), get_group_info_for_cohort(self.cohort1)) + self.put_cohort_handler( + self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2, 'user_partition_id': 3} + ) + self.assertEqual((2, 3), get_group_info_for_cohort(self.cohort1)) + self.put_cohort_handler( + self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 1, 'user_partition_id': 3} + ) + self.assertEqual((1, 3), get_group_info_for_cohort(self.cohort1)) + + def test_update_cohort_missing_user_partition_id(self): + """ + Verify that we cannot update a cohort with a group_id if the user_partition_id is not also specified. + """ + self._create_cohorts() + response_dict = self.put_cohort_handler( + self.course, self.cohort1, data={'name': self.cohort1.name, 'group_id': 2}, expected_response_code=400 + ) + self.assertEqual( + "If group_id is specified, user_partition_id must also be specified.", response_dict.get("error") ) @@ -298,7 +431,7 @@ class UsersInCohortTestCase(CohortViewsTestCase): """ request = RequestFactory().get("dummy_url", {"page": requested_page}) request.user = self.staff_user - response = users_in_cohort(request, course.id.to_deprecated_string(), cohort.id) + response = users_in_cohort(request, unicode(course.id), cohort.id) if should_return_bad_request: self.assertEqual(response.status_code, 400) @@ -328,7 +461,7 @@ class UsersInCohortTestCase(CohortViewsTestCase): Verify that non-staff users cannot access `check_users_in_cohort`. """ cohort = CohortFactory(course_id=self.course.id, users=[]) - self._verify_non_staff_cannot_access(users_in_cohort, "GET", [self.course.id.to_deprecated_string(), cohort.id]) + self._verify_non_staff_cannot_access(users_in_cohort, "GET", [unicode(self.course.id), cohort.id]) def test_no_users(self): """ @@ -437,10 +570,10 @@ class AddUsersToCohortTestCase(CohortViewsTestCase): if should_raise_404: self.assertRaises( Http404, - lambda: add_users_to_cohort(request, course.id.to_deprecated_string(), cohort.id) + lambda: add_users_to_cohort(request, unicode(course.id), cohort.id) ) else: - response = add_users_to_cohort(request, course.id.to_deprecated_string(), cohort.id) + response = add_users_to_cohort(request, unicode(course.id), cohort.id) self.assertEqual(response.status_code, 200) return json.loads(response.content) @@ -457,14 +590,14 @@ class AddUsersToCohortTestCase(CohortViewsTestCase): `expected_unknown` is a list of strings corresponding to the input """ self.assertTrue(response_dict.get("success")) - self.assertItemsEqual( + self.assertEqual( response_dict.get("added"), [ {"username": user.username, "name": user.profile.name, "email": user.email} for user in expected_added ] ) - self.assertItemsEqual( + self.assertEqual( response_dict.get("changed"), [ { @@ -476,11 +609,11 @@ class AddUsersToCohortTestCase(CohortViewsTestCase): for (user, previous_cohort) in expected_changed ] ) - self.assertItemsEqual( + self.assertEqual( response_dict.get("present"), [username_or_email for (_, username_or_email) in expected_present] ) - self.assertItemsEqual(response_dict.get("unknown"), expected_unknown) + self.assertEqual(response_dict.get("unknown"), expected_unknown) for user in expected_added + [user for (user, _) in expected_changed + expected_present]: self.assertEqual( CourseUserGroup.objects.get( @@ -499,7 +632,7 @@ class AddUsersToCohortTestCase(CohortViewsTestCase): self._verify_non_staff_cannot_access( add_users_to_cohort, "POST", - [self.course.id.to_deprecated_string(), cohort.id] + [unicode(self.course.id), cohort.id] ) def test_empty(self): @@ -730,7 +863,7 @@ class RemoveUserFromCohortTestCase(CohortViewsTestCase): else: request = RequestFactory().post("dummy_url") request.user = self.staff_user - response = remove_user_from_cohort(request, self.course.id.to_deprecated_string(), cohort.id) + response = remove_user_from_cohort(request, unicode(self.course.id), cohort.id) self.assertEqual(response.status_code, 200) return json.loads(response.content) @@ -756,7 +889,7 @@ class RemoveUserFromCohortTestCase(CohortViewsTestCase): self._verify_non_staff_cannot_access( remove_user_from_cohort, "POST", - [self.course.id.to_deprecated_string(), cohort.id] + [unicode(self.course.id), cohort.id] ) def test_no_username_given(self): diff --git a/openedx/core/djangoapps/course_groups/views.py b/openedx/core/djangoapps/course_groups/views.py index 37872e60bf..8248576642 100644 --- a/openedx/core/djangoapps/course_groups/views.py +++ b/openedx/core/djangoapps/course_groups/views.py @@ -4,6 +4,9 @@ from django.contrib.auth.models import User from django.core.paginator import Paginator, EmptyPage from django.core.urlresolvers import reverse from django.http import Http404, HttpResponse, HttpResponseBadRequest +from django.views.decorators.http import require_http_methods +from util.json_request import expect_json, JsonResponse +from django.contrib.auth.decorators import login_required import json import logging import re @@ -12,9 +15,8 @@ from opaque_keys.edx.locations import SlashSeparatedCourseKey from courseware.courses import get_course_with_access from edxmako.shortcuts import render_to_response -from util.json_request import JsonResponse from . import cohorts -from .models import CourseUserGroup +from .models import CourseUserGroup, CourseUserGroupPartitionGroup log = logging.getLogger(__name__) @@ -29,76 +31,118 @@ def json_http_response(data): def split_by_comma_and_whitespace(cstr): """ - Split a string both by commas and whitespice. Returns a list. + Split a string both by commas and whitespace. Returns a list. """ return re.split(r'[\s,]+', cstr) +def link_cohort_to_partition_group(cohort, partition_id, group_id): + """ + Create cohort to partition_id/group_id link. + """ + CourseUserGroupPartitionGroup( + course_user_group=cohort, + partition_id=partition_id, + group_id=group_id, + ).save() + + +def unlink_cohort_partition_group(cohort): + """ + Remove any existing cohort to partition_id/group_id link. + """ + CourseUserGroupPartitionGroup.objects.filter(course_user_group=cohort).delete() + + +def _get_cohort_representation(cohort, course): + """ + Returns a JSON representation of a cohort. + """ + group_id, partition_id = cohorts.get_group_info_for_cohort(cohort) + return { + 'name': cohort.name, + 'id': cohort.id, + 'user_count': cohort.users.count(), + 'assignment_type': cohorts.CohortAssignmentType.get(cohort, course), + 'user_partition_id': partition_id, + 'group_id': group_id + } + + +@require_http_methods(("GET", "PUT", "POST", "PATCH")) @ensure_csrf_cookie -def list_cohorts(request, course_key_string): +@expect_json +@login_required +def cohort_handler(request, course_key_string, cohort_id=None): """ - Return json dump of dict: - - {'success': True, - 'cohorts': [{'name': name, 'id': id}, ...]} + The restful handler for cohort requests. Requires JSON. + GET + If a cohort ID is specified, returns a JSON representation of the cohort + (name, id, user_count, assignment_type, user_partition_id, group_id). + If no cohort ID is specified, returns the JSON representation of all cohorts. + This is returned as a dict with the list of cohort information stored under the + key `cohorts`. + PUT or POST or PATCH + If a cohort ID is specified, updates the cohort with the specified ID. Currently the only + properties that can be updated are `name`, `user_partition_id` and `group_id`. + Returns the JSON representation of the updated cohort. + If no cohort ID is specified, creates a new cohort and returns the JSON representation of the updated + cohort. """ - - # this is a string when we get it here course_key = SlashSeparatedCourseKey.from_deprecated_string(course_key_string) - course = get_course_with_access(request.user, 'staff', course_key) + if request.method == 'GET': + if not cohort_id: + all_cohorts = [ + _get_cohort_representation(c, course) + for c in cohorts.get_course_cohorts(course) + ] + return JsonResponse({'cohorts': all_cohorts}) + else: + cohort = cohorts.get_cohort_by_id(course_key, cohort_id) + return JsonResponse(_get_cohort_representation(cohort, course)) + else: + # If cohort_id is specified, update the existing cohort. Otherwise, create a new cohort. + if cohort_id: + cohort = cohorts.get_cohort_by_id(course_key, cohort_id) + name = request.json.get('name') + if name != cohort.name: + if cohorts.CohortAssignmentType.get(cohort, course) == cohorts.CohortAssignmentType.RANDOM: + return JsonResponse( + # Note: error message not translated because it is not exposed to the user (UI prevents). + {"error": "Renaming of random cohorts is not supported at this time."}, 400 + ) + cohort.name = name + cohort.save() + else: + name = request.json.get('name') + if not name: + # Note: error message not translated because it is not exposed to the user (UI prevents this state). + return JsonResponse({"error": "In order to create a cohort, a name must be specified."}, 400) + try: + cohort = cohorts.add_cohort(course_key, name) + except ValueError as err: + return JsonResponse({"error": unicode(err)}, 400) - all_cohorts = [ - { - 'name': c.name, - 'id': c.id, - 'user_count': c.users.count(), - 'assignment_type': cohorts.CohortAssignmentType.get(c, course) - } - for c in cohorts.get_course_cohorts(course) - ] + group_id = request.json.get('group_id') + if group_id is not None: + user_partition_id = request.json.get('user_partition_id') + if user_partition_id is None: + # Note: error message not translated because it is not exposed to the user (UI prevents this state). + return JsonResponse( + {"error": "If group_id is specified, user_partition_id must also be specified."}, 400 + ) + existing_group_id, existing_partition_id = cohorts.get_group_info_for_cohort(cohort) + if group_id != existing_group_id or user_partition_id != existing_partition_id: + unlink_cohort_partition_group(cohort) + link_cohort_to_partition_group(cohort, user_partition_id, group_id) + else: + # If group_id was specified as None, unlink the cohort if it previously was associated with a group. + existing_group_id, _ = cohorts.get_group_info_for_cohort(cohort) + if existing_group_id is not None: + unlink_cohort_partition_group(cohort) - return json_http_response({'success': True, - 'cohorts': all_cohorts}) - - -@ensure_csrf_cookie -@require_POST -def add_cohort(request, course_key_string): - """ - Return json of dict: - {'success': True, - 'cohort': {'id': id, - 'name': name}} - - or - - {'success': False, - 'msg': error_msg} if there's an error - """ - # this is a string when we get it here - course_key = SlashSeparatedCourseKey.from_deprecated_string(course_key_string) - - get_course_with_access(request.user, 'staff', course_key) - - name = request.POST.get("name") - if not name: - return json_http_response({'success': False, - 'msg': "No name specified"}) - - try: - cohort = cohorts.add_cohort(course_key, name) - except ValueError as err: - return json_http_response({'success': False, - 'msg': str(err)}) - - return json_http_response({ - 'success': 'True', - 'cohort': { - 'id': cohort.id, - 'name': cohort.name - } - }) + return JsonResponse(_get_cohort_representation(cohort, course)) @ensure_csrf_cookie @@ -121,7 +165,7 @@ def users_in_cohort(request, course_key_string, cohort_id): get_course_with_access(request.user, 'staff', course_key) # this will error if called with a non-int cohort_id. That's ok--it - # shoudn't happen for valid clients. + # shouldn't happen for valid clients. cohort = cohorts.get_cohort_by_id(course_key, int(cohort_id)) paginator = Paginator(cohort.users.all(), 100) diff --git a/openedx/core/djangoapps/user_api/partition_schemes.py b/openedx/core/djangoapps/user_api/partition_schemes.py index a664500b98..756adc4049 100644 --- a/openedx/core/djangoapps/user_api/partition_schemes.py +++ b/openedx/core/djangoapps/user_api/partition_schemes.py @@ -1,11 +1,13 @@ """ Provides partition support to the user service. """ - +import logging import random import api.course_tag as course_tag_api -from xmodule.partitions.partitions import UserPartitionError +from xmodule.partitions.partitions import UserPartitionError, NoSuchUserPartitionGroupError + +log = logging.getLogger(__name__) class RandomUserPartitionScheme(object): @@ -15,14 +17,29 @@ class RandomUserPartitionScheme(object): RANDOM = random.Random() @classmethod - def get_group_for_user(cls, course_id, user, user_partition, assign=True, track_function=None): + def get_group_for_user(cls, course_key, user, user_partition, assign=True, track_function=None): """ Returns the group from the specified user position to which the user is assigned. If the user has not yet been assigned, a group will be randomly chosen for them if assign flag is True. """ partition_key = cls._key_for_partition(user_partition) - group_id = course_tag_api.get_course_tag(user, course_id, partition_key) - group = user_partition.get_group(int(group_id)) if not group_id is None else None + group_id = course_tag_api.get_course_tag(user, course_key, partition_key) + + group = None + if group_id is not None: + # attempt to look up the presently assigned group + try: + group = user_partition.get_group(int(group_id)) + except NoSuchUserPartitionGroupError: + # jsa: we can turn off warnings here if this is an expected case. + log.warn( + "group not found in RandomUserPartitionScheme: %r", + { + "requested_partition_id": user_partition.id, + "requested_group_id": group_id, + }, + exc_info=True + ) if group is None and assign: if not user_partition.groups: @@ -35,7 +52,7 @@ class RandomUserPartitionScheme(object): group = cls.RANDOM.choice(user_partition.groups) # persist the value as a course tag - course_tag_api.set_course_tag(user, course_id, partition_key, group.id) + course_tag_api.set_course_tag(user, course_key, partition_key, group.id) if track_function: # emit event for analytics