${_("Settings")} > ${_("Group Configurations")}
+<%= gettext("You haven't created any group configurations yet.") %>
+<%= gettext("You haven't created any group configurations yet.") %><%= gettext("Add your first Group Configuration") %>
diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 4ff410fecb..b9ccbd53e9 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -13,7 +13,7 @@ from django.conf import settings from django.views.decorators.http import require_http_methods from django.core.exceptions import PermissionDenied from django.core.urlresolvers import reverse -from django.http import HttpResponseBadRequest, HttpResponseNotFound +from django.http import HttpResponseBadRequest, HttpResponseNotFound, HttpResponse from util.json_request import JsonResponse from edxmako.shortcuts import render_to_response @@ -21,6 +21,7 @@ from xmodule.error_module import ErrorDescriptor from xmodule.modulestore.django import modulestore from xmodule.contentstore.content import StaticContent from xmodule.tabs import PDFTextbookTabs +from xmodule.partitions.partitions import UserPartition, Group from xmodule.modulestore.exceptions import ItemNotFoundError, InvalidLocationError from opaque_keys import InvalidKeyError @@ -67,7 +68,7 @@ __all__ = ['course_info_handler', 'course_handler', 'course_info_update_handler' 'grading_handler', 'advanced_settings_handler', 'textbooks_list_handler', 'textbooks_detail_handler', - 'group_configurations_list_handler'] + 'group_configurations_list_handler', 'group_configurations_detail_handler'] class AccessListFallback(Exception): @@ -855,7 +856,56 @@ def textbooks_detail_handler(request, course_key_string, textbook_id): return JsonResponse() -@require_http_methods(("GET")) +class GroupConfigurationsValidationError(Exception): + """ + An error thrown when a group configurations input is invalid. + """ + pass + + +class GroupConfiguration(object): + """ + Prepare Group Configuration for the course. + """ + @staticmethod + def parse(configuration_json): + """ + Parse given string that represents group configuration. + """ + try: + group_configuration = json.loads(configuration_json) + except ValueError: + raise GroupConfigurationsValidationError(_("invalid JSON")) + + if not group_configuration.get('version'): + group_configuration['version'] = UserPartition.VERSION + + # this is temporary logic, we are going to build default groups on front-end + if not group_configuration.get('groups'): + group_configuration['groups'] = [ + {'name': 'Group A'}, {'name': 'Group B'}, + ] + + for group in group_configuration['groups']: + group['version'] = Group.VERSION + return group_configuration + + @staticmethod + def validate(group_configuration): + """ + Validate group configuration representation. + """ + if not group_configuration.get("name"): + raise GroupConfigurationsValidationError(_("must have name of the configuration")) + if not isinstance(group_configuration.get("description"), basestring): + raise GroupConfigurationsValidationError(_("must have description of the configuration")) + if len(group_configuration.get('groups')) < 2: + raise GroupConfigurationsValidationError(_("must have at least two groups")) + group_id = unicode(group_configuration.get("id", "")) + if group_id and not group_id.isdigit(): + raise GroupConfigurationsValidationError(_("group configuration ID must be numeric")) + +@require_http_methods(("GET", "POST")) @login_required @ensure_csrf_cookie def group_configurations_list_handler(request, course_key_string): @@ -864,17 +914,56 @@ def group_configurations_list_handler(request, course_key_string): GET html: return Group Configurations list page (Backbone application) + POST + json: create new group configuration """ course_key = CourseKey.from_string(course_key_string) course = _get_course_module(course_key, request.user) - group_configuration_url = reverse_course_url('group_configurations_list_handler', course_key) - splite_test_enabled = SPLIT_TEST_COMPONENT_TYPE in course.advanced_modules + store = modulestore() - return render_to_response('group_configurations.html', { - 'context_course': course, - 'group_configuration_url': group_configuration_url, - 'configurations': [u.to_json() for u in course.user_partitions] if splite_test_enabled else None, - }) + if 'text/html' in request.META.get('HTTP_ACCEPT', 'text/html'): + group_configuration_url = reverse_course_url('group_configurations_list_handler', course_key) + split_test_enabled = SPLIT_TEST_COMPONENT_TYPE in course.advanced_modules + + return render_to_response('group_configurations.html', { + 'context_course': course, + 'group_configuration_url': group_configuration_url, + 'configurations': [u.to_json() for u in course.user_partitions] if split_test_enabled else None, + }) + elif "application/json" in request.META.get('HTTP_ACCEPT') and request.method == 'POST': + # create a new group configuration for the course + try: + configuration = GroupConfiguration.parse(request.body) + GroupConfiguration.validate(configuration) + except GroupConfigurationsValidationError as err: + return JsonResponse({"error": err.message}, status=400) + + if not configuration.get("id"): + configuration["id"] = random.randint(100, 10**12) + + # Assign ids to every group in configuration. + for index, group in enumerate(configuration.get('groups', [])): + group["id"] = index + + course.user_partitions.append(UserPartition.from_json(configuration)) + store.update_item(course, request.user.id) + response = JsonResponse(configuration, status=201) + + response["Location"] = reverse_course_url( + 'group_configurations_detail_handler', + course.id, + kwargs={'group_configuration_id': configuration["id"]} + ) + return response + else: + return HttpResponse(status=406) + + +@require_http_methods(("GET", "POST")) +@login_required +@ensure_csrf_cookie +def group_configurations_detail_handler(request, course_key_string, group_configuration_id): + return JsonResponse(status=404) def _get_course_creator_status(user): diff --git a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py new file mode 100644 index 0000000000..9a48006461 --- /dev/null +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -0,0 +1,151 @@ +import json +from unittest import skipUnless +from django.conf import settings +from contentstore.utils import reverse_course_url +from contentstore.tests.utils import CourseTestCase + + +@skipUnless(settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS'), 'Tests Group Configurations feature') +class GroupConfigurationsCreateTestCase(CourseTestCase): + """ + Test cases for creating a new group configurations. + """ + + def setUp(self): + """ + Set up a url and group configuration content for tests. + """ + super(GroupConfigurationsCreateTestCase, self).setUp() + self.url = reverse_course_url('group_configurations_list_handler', self.course.id) + self.group_configuration_json = { + u'description': u'Test description', + u'name': u'Test name' + } + + def test_index_page(self): + """ + Check that the group configuration index page responds correctly. + """ + response = self.client.get(self.url) + self.assertEqual(response.status_code, 200) + self.assertIn('New Group Configuration', response.content) + + def test_group_success(self): + """ + Test that you can create a group configuration. + """ + expected_group_configuration = { + u'description': u'Test description', + u'name': u'Test name', + u'version': 1, + u'groups': [ + {u'id': 0, u'name': u'Group A', u'version': 1}, + {u'id': 1, u'name': u'Group B', u'version': 1} + ] + } + response = self.client.post( + self.url, + data=json.dumps(self.group_configuration_json), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + self.assertEqual(response.status_code, 201) + self.assertIn("Location", response) + group_configuration = json.loads(response.content) + del group_configuration['id'] # do not check for id, it is unique + self.assertEqual(expected_group_configuration, group_configuration) + + def test_bad_group(self): + """ + Test if only one group in configuration exist. + """ + # Only one group in group configuration here. + bad_group_configuration = { + u'description': u'Test description', + u'id': 1, + u'name': u'Test name', + u'version': 1, + u'groups': [ + {u'id': 0, u'name': u'Group A', u'version': 1}, + ] + } + response = self.client.post( + self.url, + data=json.dumps(bad_group_configuration), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + self.assertEqual(response.status_code, 400) + self.assertNotIn("Location", response) + content = json.loads(response.content) + self.assertIn("error", content) + + def test_bad_configuration_id(self): + """ + Test if configuration id is not numeric. + """ + # Configuration id is string here. + bad_group_configuration = { + u'description': u'Test description', + u'id': 'bad_id', + u'name': u'Test name', + u'version': 1, + u'groups': [ + {u'id': 0, u'name': u'Group A', u'version': 1}, + {u'id': 1, u'name': u'Group B', u'version': 1} + ] + } + response = self.client.post( + self.url, + data=json.dumps(bad_group_configuration), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest" + ) + self.assertEqual(response.status_code, 400) + self.assertNotIn("Location", response) + content = json.loads(response.content) + self.assertIn("error", content) + + def test_bad_json(self): + """ + Test bad json handling. + """ + bad_jsons = [ + {u'name': 'Test Name'}, + {u'description': 'Test description'}, + {} + ] + for bad_json in bad_jsons: + response = self.client.post( + self.url, + data=json.dumps(bad_json), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest", + ) + self.assertEqual(response.status_code, 400) + self.assertNotIn("Location", response) + content = json.loads(response.content) + self.assertIn("error", content) + + def test_invalid_json(self): + """ + Test invalid json handling. + """ + # No property name. + invalid_json = "{u'name': 'Test Name', []}" + + response = self.client.post( + self.url, + data=invalid_json, + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest", + ) + self.assertEqual(response.status_code, 400) + self.assertNotIn("Location", response) + content = json.loads(response.content) + self.assertIn("error", content) diff --git a/cms/static/js/models/group_configuration.js b/cms/static/js/models/group_configuration.js index 178c0a432f..d4fcaa0129 100644 --- a/cms/static/js/models/group_configuration.js +++ b/cms/static/js/models/group_configuration.js @@ -1,17 +1,18 @@ define([ - 'backbone', 'underscore', 'gettext', 'js/models/group', + 'backbone', 'underscore', 'underscore.string', 'gettext', 'js/models/group', 'js/collections/group', 'backbone.associations', 'coffee/src/main' ], -function(Backbone, _, gettext, GroupModel, GroupCollection) { +function(Backbone, _, str, gettext, GroupModel, GroupCollection) { 'use strict'; + _.str = str; var GroupConfiguration = Backbone.AssociatedModel.extend({ defaults: function() { return { - id: null, name: '', description: '', - groups: new GroupCollection([{}, {}]), - showGroups: false + groups: new GroupCollection([]), + showGroups: false, + editing: false }; }, @@ -55,32 +56,12 @@ function(Backbone, _, gettext, GroupModel, GroupCollection) { }, validate: function(attrs) { - if (!attrs.name) { + if (!_.str.trim(attrs.name)) { return { message: gettext('Group Configuration name is required'), attributes: {name: true} }; } - if (attrs.groups.length === 0) { - return { - message: gettext('Please add at least one group'), - attributes: {groups: true} - }; - } else { - // validate all groups - var invalidGroups = []; - attrs.groups.each(function(group) { - if(!group.isValid()) { - invalidGroups.push(group); - } - }); - if (!_.isEmpty(invalidGroups)) { - return { - message: gettext('All groups must have a name'), - attributes: {groups: invalidGroups} - }; - } - } } }); return GroupConfiguration; diff --git a/cms/static/js/spec/models/group_configuration_spec.js b/cms/static/js/spec/models/group_configuration_spec.js index 6ca7e8f6d1..2d812b617b 100644 --- a/cms/static/js/spec/models/group_configuration_spec.js +++ b/cms/static/js/spec/models/group_configuration_spec.js @@ -1,9 +1,8 @@ define([ - 'backbone', 'js/models/group_configuration', - 'js/collections/group_configuration', 'js/models/group', - 'js/collections/group', 'coffee/src/main' + 'backbone', 'coffee/src/main', 'js/models/group_configuration', + 'js/models/group', 'js/collections/group' ], function( - Backbone, GroupConfiguration, GroupConfigurationSet, Group, GroupSet, main + Backbone, main, GroupConfigurationModel, GroupModel, GroupCollection ) { 'use strict'; beforeEach(function() { @@ -14,10 +13,10 @@ define([ }); }); - describe('GroupConfiguration model', function() { + describe('GroupConfigurationModel', function() { beforeEach(function() { main(); - this.model = new GroupConfiguration(); + this.model = new GroupConfigurationModel(); }); describe('Basic', function() { @@ -33,16 +32,10 @@ define([ expect(this.model.get('showGroups')).toBeFalsy(); }); - it('should have a GroupSet with two groups by default', function() { + it('should be empty by default', function() { var groups = this.model.get('groups'); - expect(groups).toBeInstanceOf(GroupSet); - expect(groups.length).toEqual(2); - expect(groups.at(0).isEmpty()).toBeTruthy(); - expect(groups.at(1).isEmpty()).toBeTruthy(); - }); - - it('should be empty by default', function() { + expect(groups).toBeInstanceOf(GroupCollection); expect(this.model.isEmpty()).toBeTruthy(); }); @@ -53,21 +46,23 @@ define([ expect(this.model.get('name')).toEqual(''); }); - it('should not be dirty by default', function() { - expect(this.model.isDirty()).toBeFalsy(); - }); - it('should be dirty after it\'s been changed', function() { this.model.set('name', 'foobar'); expect(this.model.isDirty()).toBeTruthy(); }); - it('should not be dirty after calling setOriginalAttributes', function() { - this.model.set('name', 'foobar'); - this.model.setOriginalAttributes(); + describe('should not be dirty', function () { + it('by default', function() { + expect(this.model.isDirty()).toBeFalsy(); + }); - expect(this.model.isDirty()).toBeFalsy(); + it('after calling setOriginalAttributes', function() { + this.model.set('name', 'foobar'); + this.model.setOriginalAttributes(); + + expect(this.model.isDirty()).toBeFalsy(); + }); }); }); @@ -96,7 +91,7 @@ define([ it('should match server model to client model', function() { var serverModelSpec = { 'id': 10, - 'name': 'My GroupConfiguration', + 'name': 'My Group Configuration', 'description': 'Some description', 'groups': [ { @@ -108,9 +103,10 @@ define([ }, clientModelSpec = { 'id': 10, - 'name': 'My GroupConfiguration', + 'name': 'My Group Configuration', 'description': 'Some description', 'showGroups': false, + 'editing': false, 'groups': [ { 'name': 'Group 1' @@ -119,7 +115,7 @@ define([ } ] }, - model = new GroupConfiguration(serverModelSpec); + model = new GroupConfigurationModel(serverModelSpec); expect(deepAttributes(model)).toEqual(clientModelSpec); expect(model.toJSON()).toEqual(serverModelSpec); @@ -128,55 +124,22 @@ define([ describe('Validation', function() { it('requires a name', function() { - var model = new GroupConfiguration({ name: '' }); - - expect(model.isValid()).toBeFalsy(); - }); - - it('requires at least one group', function() { - var model = new GroupConfiguration({ name: 'foo' }); - model.get('groups').reset(); - - expect(model.isValid()).toBeFalsy(); - }); - - it('requires a valid group', function() { - var group = new Group(), - model = new GroupConfiguration({ name: 'foo' }); - - group.isValid = function() { return false; }; - model.get('groups').reset([group]); - - expect(model.isValid()).toBeFalsy(); - }); - - it('requires all groups to be valid', function() { - var group1 = new Group(), - group2 = new Group(), - model = new GroupConfiguration({ name: 'foo' }); - - group1.isValid = function() { return true; }; - group2.isValid = function() { return false; }; - model.get('groups').reset([group1, group2]); + var model = new GroupConfigurationModel({ name: '' }); expect(model.isValid()).toBeFalsy(); }); it('can pass validation', function() { - var group = new Group(), - model = new GroupConfiguration({ name: 'foo' }); - - group.isValid = function() { return true; }; - model.get('groups').reset([group]); + var model = new GroupConfigurationModel({ name: 'foo' }); expect(model.isValid()).toBeTruthy(); }); }); }); - describe('Group model', function() { + describe('GroupModel', function() { beforeEach(function() { - this.model = new Group(); + this.model = new GroupModel(); }); describe('Basic', function() { @@ -191,22 +154,22 @@ define([ describe('Validation', function() { it('requires a name', function() { - var model = new Group({ name: '' }); + var model = new GroupModel({ name: '' }); expect(model.isValid()).toBeFalsy(); }); it('can pass validation', function() { - var model = new Group({ name: 'a' }); + var model = new GroupModel({ name: 'a' }); expect(model.isValid()).toBeTruthy(); }); }); }); - describe('Group collection', function() { + describe('GroupCollection', function() { beforeEach(function() { - this.collection = new GroupSet(); + this.collection = new GroupCollection(); }); it('is empty by default', function() { diff --git a/cms/static/js/spec/views/group_configuration_spec.js b/cms/static/js/spec/views/group_configuration_spec.js index ade63adaf6..c1aac48557 100644 --- a/cms/static/js/spec/views/group_configuration_spec.js +++ b/cms/static/js/spec/views/group_configuration_spec.js @@ -1,12 +1,31 @@ define([ - 'js/models/group_configuration', 'js/models/course', - 'js/collections/group_configuration', 'js/views/group_configuration_details', - 'js/views/group_configurations_list', 'jasmine-stealth' + '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/views/feedback_notification', + 'js/spec_helpers/create_sinon', 'js/spec_helpers/edit_helpers', + 'jasmine-stealth' ], function( - GroupConfigurationModel, Course, GroupConfigurationSet, - GroupConfigurationDetails, GroupConfigurationsList + Course, GroupConfigurationModel, GroupConfigurationCollection, + GroupConfigurationDetails, GroupConfigurationsList, GroupConfigurationEdit, + GroupConfigurationItem, Notification, create_sinon, view_helpers ) { 'use strict'; + var SELECTORS = { + detailsView: '.group-configuration-details', + editView: '.group-configuration-edit', + itemView: '.group-configurations-list-item', + group: '.group', + name: '.group-configuration-name', + description: '.group-configuration-description', + groupsCount: '.group-configuration-groups-count', + groupsAllocation: '.group-allocation', + errorMessage: '.group-configuration-edit-error', + inputName: '.group-configuration-name-input', + inputDescription: '.group-configuration-description-input' + }; + beforeEach(function() { window.course = new Course({ id: '5', @@ -26,6 +45,20 @@ define([ } else { return trimmedText.indexOf(text) !== -1; } + }, + toBeCorrectValuesInInputs: function (values) { + var expected = { + name: this.actual.$(SELECTORS.inputName).val(), + description: this.actual + .$(SELECTORS.inputDescription).val() + }; + + return _.isEqual(values, expected); + }, + toBeCorrectValuesInModel: function (values) { + return _.every(values, function (value, key) { + return this.actual.get(key) === value; + }.bind(this)); } }); }); @@ -35,13 +68,8 @@ define([ }); describe('GroupConfigurationDetails', function() { - var tpl = readFixtures('group-configuration-details.underscore'); - beforeEach(function() { - setFixtures($(' @@ -20,14 +20,16 @@ <%block name="jsextra"> @@ -40,6 +42,14 @@ function(doc, GroupConfigurationCollection, GroupConfigurationsPage, xmoduleLoad ${_("Settings")} > ${_("Group Configurations")} + diff --git a/cms/templates/js/group-configuration-details.underscore b/cms/templates/js/group-configuration-details.underscore index 79a746c5b0..bf9b09b76e 100644 --- a/cms/templates/js/group-configuration-details.underscore +++ b/cms/templates/js/group-configuration-details.underscore @@ -1,42 +1,40 @@ -
<%= gettext("You haven't created any group configurations yet.") %>
+<%= gettext("You haven't created any group configurations yet.") %><%= gettext("Add your first Group Configuration") %>