Merge pull request #4480 from edx/anton/create-edit-delete-groups

Group Configuration: create, edit, delete groups

cms/djangoapps/contentstore/views/course.py

@@ -901,12 +901,11 @@ class GroupConfiguration(object):
         if len(self.configuration.get('groups', [])) < 2:
             raise GroupConfigurationsValidationError(_("must have at least two groups"))
 
-    def generate_id(self):
+    def generate_id(self, used_ids):
         """
         Generate unique id for the group configuration.
         If this id is already used, we generate new one.
         """
-        used_ids = self.get_used_ids()
         cid = random.randint(100, 10 ** 12)
 
         while cid in used_ids:
@@ -918,21 +917,18 @@ class GroupConfiguration(object):
         """
         Assign id for the json representation of group configuration.
         """
-        self.configuration['id'] = int(configuration_id) if configuration_id else self.generate_id()
+        self.configuration['id'] = int(configuration_id) if configuration_id else self.generate_id(self.get_used_ids())
 
     def assign_group_ids(self):
         """
         Assign ids for the group_configuration's groups.
         """
-        # this is temporary logic, we are going to build default groups on front-end
-        if not self.configuration.get('groups'):
-            self.configuration['groups'] = [
-                {'name': 'Group A'}, {'name': 'Group B'},
-            ]
-
+        used_ids = [g.id for p in self.course.user_partitions for g in p.groups]
         # Assign ids to every group in configuration.
-        for index, group in enumerate(self.configuration.get('groups', [])):
-            group['id'] = index
+        for group in self.configuration.get('groups', []):
+            if group.get('id') is None:
+                group["id"] = self.generate_id(used_ids)
+                used_ids.append(group["id"])
 
     def get_used_ids(self):
         """

cms/djangoapps/contentstore/views/tests/test_group_configurations.py

@@ -5,6 +5,7 @@ import json
 from unittest import skipUnless
 from django.conf import settings
 from contentstore.utils import reverse_course_url
+from contentstore.views.component import SPLIT_TEST_COMPONENT_TYPE
 from contentstore.tests.utils import CourseTestCase
 from xmodule.partitions.partitions import Group, UserPartition
 
@@ -12,6 +13,10 @@ from xmodule.partitions.partitions import Group, UserPartition
 GROUP_CONFIGURATION_JSON = {
     u'name': u'Test name',
     u'description': u'Test description',
+    u'groups': [
+        {u'name': u'Group A'},
+        {u'name': u'Group B'},
+    ],
 }
 
 
@@ -96,7 +101,6 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations
     """
     Test cases for group_configurations_list_handler.
     """
-
     def setUp(self):
         """
         Set up GroupConfigurationsListHandlerTestCase.
@@ -109,13 +113,35 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations
         """
         return reverse_course_url('group_configurations_list_handler', self.course.id)
 
-    def test_can_retrieve_html(self):
+    def test_view_index_ok(self):
         """
-        Check that the group configuration index page responds correctly.
+        Basic check that the groups configuration page responds correctly.
         """
+
+        self.course.user_partitions = [
+            UserPartition(0, 'First name', 'First description', [Group(0, 'Group A'), Group(1, 'Group B'), Group(2, 'Group C')]),
+        ]
+        self.save_course()
+
+        if SPLIT_TEST_COMPONENT_TYPE not in self.course.advanced_modules:
+            self.course.advanced_modules.append(SPLIT_TEST_COMPONENT_TYPE)
+            self.store.update_item(self.course, self.user.id)
+
         response = self.client.get(self._url())
         self.assertEqual(response.status_code, 200)
-        self.assertIn('New Group Configuration', response.content)
+        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")
 
     def test_unsupported_http_accept_header(self):
         """
@@ -157,8 +183,12 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations
         self.assertEqual(len(group_ids), 2)
         self.reload_course()
         # Verify that user_partitions in the course contains the new group configuration.
-        self.assertEqual(len(self.course.user_partitions), 1)
-        self.assertEqual(self.course.user_partitions[0].name, u'Test name')
+        user_partititons = self.course.user_partitions
+        self.assertEqual(len(user_partititons), 1)
+        self.assertEqual(user_partititons[0].name, u'Test name')
+        self.assertEqual(len(user_partititons[0].groups), 2)
+        self.assertEqual(user_partititons[0].groups[0].name, u'Group A')
+        self.assertEqual(user_partititons[0].groups[1].name, u'Group B')
 
 
 # pylint: disable=no-member
@@ -204,7 +234,7 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio
 
         response = self.client.put(
             self._url(cid=999),
-            data=json.dumps(GROUP_CONFIGURATION_JSON),
+            data=json.dumps(expected),
             content_type="application/json",
             HTTP_ACCEPT="application/json",
             HTTP_X_REQUESTED_WITH="XMLHttpRequest",
@@ -213,8 +243,12 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio
         self.assertEqual(content, expected)
         self.reload_course()
         # Verify that user_partitions in the course contains the new group configuration.
-        self.assertEqual(len(self.course.user_partitions), 1)
-        self.assertEqual(self.course.user_partitions[0].name, u'Test name')
+        user_partititons = self.course.user_partitions
+        self.assertEqual(len(user_partititons), 1)
+        self.assertEqual(user_partititons[0].name, u'Test name')
+        self.assertEqual(len(user_partititons[0].groups), 2)
+        self.assertEqual(user_partititons[0].groups[0].name, u'Group A')
+        self.assertEqual(user_partititons[0].groups[1].name, u'Group B')
 
     def test_can_edit_group_configuration(self):
         """
@@ -231,8 +265,8 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio
             u'description': u'New Test description',
             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},
+                {u'id': 0, u'name': u'New Group Name', u'version': 1},
+                {u'id': 2, u'name': u'Group C', u'version': 1},
             ],
         }
         response = self.client.put(
@@ -246,5 +280,9 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio
         self.assertEqual(content, expected)
         self.reload_course()
         # Verify that user_partitions is properly updated in the course.
-        self.assertEqual(len(self.course.user_partitions), 1)
-        self.assertEqual(self.course.user_partitions[0].name, u'New Test name')
+        user_partititons = self.course.user_partitions
+        self.assertEqual(len(user_partititons), 1)
+        self.assertEqual(user_partititons[0].name, u'New Test name')
+        self.assertEqual(len(user_partititons[0].groups), 2)
+        self.assertEqual(user_partititons[0].groups[0].name, u'New Group Name')
+        self.assertEqual(user_partititons[0].groups[1].name, u'Group C')

cms/djangoapps/contentstore/views/tests/test_item.py

@@ -6,19 +6,17 @@ import ddt
 
 from mock import patch
 from pytz import UTC
-from unittest import skipUnless
 from webob import Response
 
-from django.conf import settings
 from django.http import Http404
 from django.test import TestCase
 from django.test.client import RequestFactory
 from django.core.urlresolvers import reverse
-from contentstore.utils import reverse_usage_url, reverse_course_url
+from contentstore.utils import reverse_usage_url
+from contentstore.views.preview import StudioUserService
 
 from contentstore.views.component import (
-    component_handler, get_component_templates,
-    SPLIT_TEST_COMPONENT_TYPE
+    component_handler, get_component_templates
 )
 
 from contentstore.tests.utils import CourseTestCase
@@ -776,7 +774,6 @@ class TestEditItem(ItemTest):
         self.verify_publish_state(html_usage_key, PublishState.draft)
 
 
-@skipUnless(settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS'), 'Tests Group Configurations feature')
 class TestEditSplitModule(ItemTest):
     """
     Tests around editing instances of the split_test module.
@@ -977,6 +974,12 @@ class TestEditSplitModule(ItemTest):
         group_id_to_child = split_test.group_id_to_child
         self.assertEqual(2, len(group_id_to_child))
 
+        # Test environment and Studio use different module systems
+        # (CachingDescriptorSystem is used in tests, PreviewModuleSystem in Studio).
+        # CachingDescriptorSystem doesn't have user service, that's needed for
+        # SplitTestModule. So, in this line of code we add this service manually.
+        split_test.runtime._services['user'] = StudioUserService(self.request)  # pylint: disable=protected-access
+
         # Call add_missing_groups method to add the missing group.
         split_test.add_missing_groups(self.request)
         split_test = self._assert_children(3)
@@ -989,34 +992,6 @@ class TestEditSplitModule(ItemTest):
         split_test = self._assert_children(3)
         self.assertEqual(group_id_to_child, split_test.group_id_to_child)
 
-    def test_view_index_ok(self):
-        """
-        Basic check that the groups configuration page responds correctly.
-        """
-        if SPLIT_TEST_COMPONENT_TYPE not in self.course.advanced_modules:
-            self.course.advanced_modules.append(SPLIT_TEST_COMPONENT_TYPE)
-            self.store.update_item(self.course, self.user.id)
-
-        url = reverse_course_url('group_configurations_list_handler', self.course.id)
-        resp = self.client.get(url)
-        self.assertContains(resp, self.course.display_name)
-        self.assertContains(resp, 'First Partition')
-        self.assertContains(resp, 'alpha')
-        self.assertContains(resp, 'Second Partition')
-        self.assertContains(resp, 'Group 1')
-
-    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)
-
-        url = reverse_course_url('group_configurations_list_handler', self.course.id)
-        resp = self.client.get(url)
-        self.assertContains(resp, "module is disabled")
-
 
 @ddt.ddt
 class TestComponentHandler(TestCase):

cms/static/coffee/spec/main.coffee

@@ -214,7 +214,6 @@ define([
 
     "js/spec/models/component_template_spec",
     "js/spec/models/explicit_url_spec",
-    "js/spec/models/group_configuration_spec",
 
     "js/spec/utils/drag_and_drop_spec",
     "js/spec/utils/handle_iframe_binding_spec",
@@ -223,6 +222,7 @@ define([
     "js/spec/views/baseview_spec",
     "js/spec/views/paging_spec",
     "js/spec/views/assets_spec",
+    "js/spec/views/group_configuration_spec",
 
     "js/spec/views/container_spec",
     "js/spec/views/unit_spec",
@@ -235,8 +235,6 @@ define([
     "js/spec/views/modals/base_modal_spec",
     "js/spec/views/modals/edit_xblock_spec",
 
-    "js/spec/views/group_configuration_spec",
-
     "js/spec/xblock/cms.runtime.v1_spec",
 
     # these tests are run separately in the cms-squire suite, due to process

cms/static/coffee/spec/main_squire.coffee

@@ -176,5 +176,6 @@ jasmine.getFixtures().fixturesPath += 'coffee/fixtures'
 define([
     "coffee/spec/views/assets_spec",
     "js/spec/video/translations_editor_spec",
-    "js/spec/video/file_uploader_editor_spec"
+    "js/spec/video/file_uploader_editor_spec",
+    "js/spec/models/group_configuration_spec"
     ])

cms/static/js/collections/group.js

 define([
-    'backbone', 'js/models/group'
+    'underscore', 'underscore.string', 'backbone', 'gettext', 'js/models/group'
 ],
-function (Backbone, GroupModel) {
+function (_, str, Backbone, gettext, GroupModel) {
     'use strict';
     var GroupCollection = Backbone.Collection.extend({
         model: GroupModel,
+        comparator: 'order',
+        /*
+         * Return next index for the model.
+         * @return {Number}
+         */
+        nextOrder: function() {
+            if(!this.length) {
+                return 0;
+            }
+
+            return this.last().get('order') + 1;
+        },
         /**
          * Indicates if the collection is empty when all the models are empty
          * or the collection does not include any models.
@@ -13,7 +25,86 @@ function (Backbone, GroupModel) {
             return this.length === 0 || this.every(function(m) {
                 return m.isEmpty();
             });
-        }
+        },
+
+        /*
+         * Return default name for the group.
+         * @return {String}
+         * @examples
+         * Group A, Group B, Group AA, Group ZZZ etc.
+         */
+        getNextDefaultGroupName: function () {
+            var index = this.nextOrder(),
+                usedNames = _.pluck(this.toJSON(), 'name'),
+                name = '';
+
+            do {
+                name = str.sprintf(gettext('Group %s'), this.getGroupId(index));
+                index ++;
+            } while (_.contains(usedNames, name));
+
+            return name;
+        },
+
+        /*
+         * Return group id for the default name of the group.
+         * @param {Number} number Current index of the model in the collection.
+         * @return {String}
+         * @examples
+         * A, B, AA in Group A, Group B, ..., Group AA, etc.
+         */
+        getGroupId: (function () {
+            /*
+                Translators: Dictionary used for creation ids that are used in
+                default group names. For example: A, B, AA in Group A,
+                Group B, ..., Group AA, etc.
+            */
+            var dict = gettext('ABCDEFGHIJKLMNOPQRSTUVWXYZ').split(''),
+                len = dict.length,
+                divide;
+
+            divide = function(numerator, denominator) {
+                if (!_.isNumber(numerator) || !denominator) {
+                    return null;
+                }
+
+                return {
+                    quotient: numerator / denominator,
+                    remainder: numerator % denominator
+                };
+            };
+
+            return function getId(number) {
+                var accumulatedValues = '',
+                    result = divide(number, len),
+                    index;
+
+                if (result) {
+                    // subtract 1 to start the count with 0.
+                    index = Math.floor(result.quotient) - 1;
+
+                    // Proceed by dividing the non-remainder part of the
+                    // dividend by the desired base until the result is less
+                    // than one.
+                    if (index < len) {
+                        // if index < 0, we do not need an additional power.
+                        if (index > -1) {
+                            // Get value for the next power.
+                            accumulatedValues += dict[index];
+                        }
+                    } else {
+                        // If we need more than 1 additional power.
+                        // Get value for the next powers.
+                        accumulatedValues += getId(index);
+                    }
+
+                    // Accumulated values + the current reminder
+                    return accumulatedValues + dict[result.remainder];
+                }
+
+                return String(number);
+            };
+        }())
     });
 
     return GroupCollection;

cms/static/js/models/group.js

 define([
-    'backbone', 'gettext', 'backbone.associations'
-], function(Backbone, gettext) {
+    'backbone', 'underscore', 'underscore.string', 'gettext',
+    'backbone.associations'
+], function(Backbone, _, str, gettext) {
     'use strict';
     var Group = Backbone.AssociatedModel.extend({
         defaults: function() {
             return {
                 name: '',
-                version: null
-            };
+                version: null,
+                order: null
+             };
         },
 
         isEmpty: function() {
@@ -16,13 +18,14 @@ define([
 
         toJSON: function() {
             return {
+                id: this.get('id'),
                 name: this.get('name'),
                 version: this.get('version')
              };
         },
 
         validate: function(attrs) {
-            if (!attrs.name) {
+            if (!str.trim(attrs.name)) {
                 return {
                     message: gettext('Group name is required'),
                     attributes: { name: true }

cms/static/js/models/group_configuration.js

@@ -11,7 +11,16 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
                 name: '',
                 description: '',
                 version: null,
-                groups: new GroupCollection([]),
+                groups: new GroupCollection([
+                    {
+                        name: gettext('Group A'),
+                        order: 0
+                    },
+                    {
+                        name: gettext('Group B'),
+                        order: 1
+                    }
+                ]),
                 showGroups: false,
                 editing: false
             };
@@ -30,16 +39,16 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
         },
 
         setOriginalAttributes: function() {
-            this._originalAttributes = this.toJSON();
+            this._originalAttributes = this.parse(this.toJSON());
         },
 
         reset: function() {
-            this.set(this._originalAttributes);
+            this.set(this._originalAttributes, { parse: true });
         },
 
         isDirty: function() {
             return !_.isEqual(
-                this._originalAttributes, this.toJSON()
+                this._originalAttributes, this.parse(this.toJSON())
             );
         },
 
@@ -47,6 +56,16 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
             return !this.get('name') && this.get('groups').isEmpty();
         },
 
+        parse: function(response) {
+            var attrs = $.extend(true, {}, response);
+
+            _.each(attrs.groups, function(group, index) {
+                group.order = group.order || index;
+            });
+
+            return attrs;
+        },
+
         toJSON: function() {
             return {
                 id: this.get('id'),
@@ -64,7 +83,29 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
                     attributes: {name: true}
                 };
             }
+
+            if (attrs.groups.length < 2) {
+                return {
+                    message: gettext('There must be at least two groups'),
+                    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;
 });

cms/static/js/spec/models/group_configuration_spec.js

 define([
     'backbone', 'coffee/src/main', 'js/models/group_configuration',
-    'js/models/group', 'js/collections/group'
+    'js/models/group', 'js/collections/group', 'squire'
 ], function(
-    Backbone, main, GroupConfigurationModel, GroupModel, GroupCollection
+    Backbone, main, GroupConfigurationModel, GroupModel, GroupCollection, Squire
 ) {
     'use strict';
     beforeEach(function() {
@@ -32,11 +32,12 @@ define([
                 expect(this.model.get('showGroups')).toBeFalsy();
             });
 
-            it('should be empty by default', function() {
+            it('should have a collection with 2 groups by default', function() {
                 var groups = this.model.get('groups');
 
                 expect(groups).toBeInstanceOf(GroupCollection);
-                expect(this.model.isEmpty()).toBeTruthy();
+                expect(groups.at(0).get('name')).toBe('Group A');
+                expect(groups.at(1).get('name')).toBe('Group B');
             });
 
             it('should be able to reset itself', function() {
@@ -72,8 +73,6 @@ define([
                     return deepAttributes(obj.attributes);
                 } else if (obj instanceof Backbone.Collection) {
                     return obj.map(deepAttributes);
-                } else if (_.isArray(obj)) {
-                    return _.map(obj, deepAttributes);
                 } else if (_.isObject(obj)) {
                     var attributes = {};
 
@@ -114,14 +113,18 @@ define([
                         'groups': [
                             {
                                 'version': 1,
+                                'order': 0,
                                 'name': 'Group 1'
                             }, {
                                 'version': 1,
+                                'order': 1,
                                 'name': 'Group 2'
                             }
                         ]
                     },
-                    model = new GroupConfigurationModel(serverModelSpec);
+                    model = new GroupConfigurationModel(
+                        serverModelSpec, { parse: true }
+                    );
 
                 expect(deepAttributes(model)).toEqual(clientModelSpec);
                 expect(model.toJSON()).toEqual(serverModelSpec);
@@ -145,11 +148,12 @@ define([
 
     describe('GroupModel', function() {
         beforeEach(function() {
-            this.model = new GroupModel();
+            this.collection = new GroupCollection([{}]);
+            this.model = this.collection.at(0);
         });
 
         describe('Basic', function() {
-            it('should have a name by default', function() {
+            it('should have an empty name by default', function() {
                 expect(this.model.get('name')).toEqual('');
             });
 
@@ -166,10 +170,41 @@ define([
             });
 
             it('can pass validation', function() {
-                var model = new GroupModel({ name: 'a' });
+                var model = new GroupConfigurationModel({ name: 'foo' });
 
                 expect(model.isValid()).toBeTruthy();
             });
+
+            it('requires at least two groups', function() {
+                var group1 = new GroupModel({ name: 'Group A' }),
+                    group2 = new GroupModel({ name: 'Group B' }),
+                    model = new GroupConfigurationModel({ name: 'foo' });
+
+                model.get('groups').reset([group1]);
+                expect(model.isValid()).toBeFalsy();
+
+                model.get('groups').add(group2);
+                expect(model.isValid()).toBeTruthy();
+            });
+
+            it('requires a valid group', function() {
+                var group = new GroupModel(),
+                    model = new GroupConfigurationModel({ name: 'foo' });
+
+                model.get('groups').reset([group]);
+
+                expect(model.isValid()).toBeFalsy();
+            });
+
+            it('requires all groups to be valid', function() {
+                var group1 = new GroupModel({ name: 'Group A' }),
+                    group2 = new GroupModel(),
+                    model = new GroupConfigurationModel({ name: 'foo' });
+
+                model.get('groups').reset([group1, group2]);
+
+                expect(model.isValid()).toBeFalsy();
+            });
         });
     });
 
@@ -183,15 +218,94 @@ define([
         });
 
         it('is empty if all groups are empty', function() {
-            this.collection.add([{}, {}, {}]);
+            this.collection.add([{ name: '' }, { name: '' }, { name: '' }]);
 
             expect(this.collection.isEmpty()).toBeTruthy();
         });
 
         it('is not empty if a group is not empty', function() {
-            this.collection.add([{}, { name: 'full' }, {} ]);
+            this.collection.add([
+                { name: '' }, { name: 'full' }, { name: '' }
+            ]);
 
             expect(this.collection.isEmpty()).toBeFalsy();
         });
+
+        describe('getGroupId', function () {
+            var collection, injector, mockGettext, initializeGroupModel;
+
+            mockGettext = function (returnedValue) {
+                var injector = new Squire();
+
+                injector.mock('gettext', function () {
+                    return function () { return returnedValue; };
+                });
+
+                return injector;
+            };
+
+            initializeGroupModel = function (dict, that) {
+                runs(function() {
+                    injector = mockGettext(dict);
+                    injector.require(['js/collections/group'],
+                    function(GroupCollection) {
+                        collection = new GroupCollection();
+                    });
+                });
+
+                waitsFor(function() {
+                    return collection;
+                }, 'GroupModel was not instantiated', 500);
+
+                that.after(function () {
+                    collection = null;
+                    injector.clean();
+                    injector.remove();
+                });
+            };
+
+            it('returns correct ids', function () {
+                var collection = new GroupCollection();
+
+                expect(collection.getGroupId(0)).toBe('A');
+                expect(collection.getGroupId(1)).toBe('B');
+                expect(collection.getGroupId(25)).toBe('Z');
+                expect(collection.getGroupId(702)).toBe('AAA');
+                expect(collection.getGroupId(704)).toBe('AAC');
+                expect(collection.getGroupId(475253)).toBe('ZZZZ');
+                expect(collection.getGroupId(475254)).toBe('AAAAA');
+                expect(collection.getGroupId(475279)).toBe('AAAAZ');
+            });
+
+            it('just 1 character in the dictionary', function () {
+                initializeGroupModel('1', this);
+                runs(function() {
+                    expect(collection.getGroupId(0)).toBe('1');
+                    expect(collection.getGroupId(1)).toBe('11');
+                    expect(collection.getGroupId(5)).toBe('111111');
+                });
+            });
+
+            it('allow to use unicode characters in the dict', function () {
+                initializeGroupModel('ö诶úeœ', this);
+                runs(function() {
+                    expect(collection.getGroupId(0)).toBe('ö');
+                    expect(collection.getGroupId(1)).toBe('诶');
+                    expect(collection.getGroupId(5)).toBe('öö');
+                    expect(collection.getGroupId(29)).toBe('œœ');
+                    expect(collection.getGroupId(30)).toBe('ööö');
+                    expect(collection.getGroupId(43)).toBe('öúe');
+                });
+            });
+
+            it('return initial value if dictionary is empty', function () {
+                initializeGroupModel('', this);
+                runs(function() {
+                    expect(collection.getGroupId(0)).toBe('0');
+                    expect(collection.getGroupId(5)).toBe('5');
+                    expect(collection.getGroupId(30)).toBe('30');
+                });
+            });
+        });
     });
 });

cms/static/js/spec_helpers/view_helpers.js

@@ -3,12 +3,13 @@
  */
 define(["jquery", "js/views/feedback_notification", "js/spec_helpers/create_sinon"],
     function($, NotificationView, create_sinon) {
-        var installTemplate, installViewTemplates, createNotificationSpy, verifyNotificationShowing,
-            verifyNotificationHidden;
+        var installTemplate, installTemplates, installViewTemplates, createNotificationSpy,
+            verifyNotificationShowing, verifyNotificationHidden;
 
         installTemplate = function(templateName, isFirst) {
             var template = readFixtures(templateName + '.underscore'),
                 templateId = templateName + '-tpl';
+
             if (isFirst) {
                 setFixtures($("<script>", { id: templateId, type: "text/template" }).text(template));
             } else {
@@ -16,6 +17,19 @@ define(["jquery", "js/views/feedback_notification", "js/spec_helpers/create_sino
             }
         };
 
+        installTemplates = function(templateNames, isFirst) {
+            if (!$.isArray(templateNames)) {
+                templateNames = [templateNames];
+            }
+
+            $.each(templateNames, function(index, templateName) {
+                installTemplate(templateName, isFirst);
+                if (isFirst) {
+                    isFirst = false;
+                }
+            });
+        };
+
         installViewTemplates = function(append) {
             installTemplate('system-feedback', !append);
             appendSetFixtures('<div id="page-notification"></div>');
@@ -41,6 +55,7 @@ define(["jquery", "js/views/feedback_notification", "js/spec_helpers/create_sino
 
         return {
             'installTemplate': installTemplate,
+            'installTemplates': installTemplates,
             'installViewTemplates': installViewTemplates,
             'createNotificationSpy': createNotificationSpy,
             'verifyNotificationShowing': verifyNotificationShowing,

cms/static/js/views/group_configuration_details.js

@@ -42,13 +42,13 @@ function(BaseView, _, gettext) {
             this.model.set('editing', true);
         },
 
-        showGroups: function(e) {
-            if(e && e.preventDefault) { e.preventDefault(); }
+        showGroups: function(event) {
+            if(event && event.preventDefault) { event.preventDefault(); }
             this.model.set('showGroups', true);
         },
 
-        hideGroups: function(e) {
-            if(e && e.preventDefault) { e.preventDefault(); }
+        hideGroups: function(event) {
+            if(event && event.preventDefault) { event.preventDefault(); }
             this.model.set('showGroups', false);
         },
 

cms/static/js/views/group_configuration_edit.js

 define([
-    'js/views/baseview', 'underscore', 'jquery', 'gettext'
+    'js/views/baseview', 'underscore', 'jquery', 'gettext',
+    'js/views/group_edit'
 ],
-function(BaseView, _, $, gettext) {
+function(BaseView, _, $, gettext, GroupEdit) {
     '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',
@@ -24,8 +26,14 @@ function(BaseView, _, $, gettext) {
         },
 
         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() {
@@ -37,10 +45,31 @@ function(BaseView, _, $, gettext) {
                 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(
@@ -62,6 +91,16 @@ function(BaseView, _, $, gettext) {
             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;
         },
 

cms/static/js/views/group_configurations_list.js

@@ -56,7 +56,7 @@ define([
 
         addOne: function(event) {
             if(event && event.preventDefault) { event.preventDefault(); }
-            this.collection.add([{editing: true}]);
+            this.collection.add([{ editing: true }]);
         }
     });
 

cms/static/js/views/group_edit.js

+define([
+    'js/views/baseview', 'underscore', 'underscore.string', 'jquery', 'gettext'
+],
+function(BaseView, _, str, $, gettext) {
+    'use strict';
+    _.str = str; // used in template
+    var GroupEdit = BaseView.extend({
+        tagName: 'li',
+        events: {
+            'click .action-close': 'removeGroup',
+            'change .group-name': 'changeName',
+            'focus .groups-fields input': 'onFocus',
+            'blur .groups-fields input': 'onBlur'
+        },
+
+        className: function() {
+            var index = this.model.collection.indexOf(this.model);
+            return 'field-group group group-' + index;
+        },
+
+        initialize: function() {
+            this.template = this.loadTemplate('group-edit');
+            this.listenTo(this.model, 'change', this.render);
+        },
+
+        render: function() {
+            var collection = this.model.collection,
+                index = collection.indexOf(this.model);
+
+            this.$el.html(this.template({
+                name: this.model.escape('name'),
+                allocation: this.getAllocation(),
+                index: index,
+                error: this.model.validationError
+            }));
+
+            return this;
+        },
+
+        changeName: function(event) {
+            if(event && event.preventDefault) { event.preventDefault(); }
+            this.model.set({
+                name: this.$('.group-name').val()
+            }, { silent: true });
+
+            return this;
+        },
+
+        removeGroup: function(event) {
+            if(event && event.preventDefault) { event.preventDefault(); }
+            this.model.collection.remove(this.model);
+            return this.remove();
+        },
+
+        getAllocation: function() {
+            return Math.floor(100 / this.model.collection.length);
+        },
+
+        onFocus: function () {
+            this.$el.closest('.groups-fields').addClass('is-focused');
+        },
+
+        onBlur: function () {
+            this.$el.closest('.groups-fields').removeClass('is-focused');
+        }
+    });
+
+    return GroupEdit;
+});

cms/static/js_test_squire.yml

@@ -50,6 +50,7 @@ lib_paths:
     - xmodule_js/common_static/js/vendor/jasmine-imagediff.js
     - xmodule_js/common_static/js/vendor/jasmine.async.js
     - xmodule_js/common_static/js/vendor/CodeMirror/codemirror.js
+    - xmodule_js/common_static/js/vendor/domReady.js
     - xmodule_js/common_static/js/vendor/URI.min.js
     - xmodule_js/src/xmodule.js
     - xmodule_js/common_static/coffee/src/jquery.immediateDescendents.js

cms/static/sass/views/_group-configuration.scss

@@ -250,6 +250,7 @@
         }
       }
 
+      .groups-fields,
       .group-configuration-fields {
         @extend %cont-no-list;
 
@@ -361,6 +362,10 @@
               margin-left: ($baseline*0.5);
             }
           }
+
+          &.group-allocation {
+            color: $gray-l1;
+          }
         }
 
         label.required {
@@ -371,11 +376,67 @@
             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 0.25s ease-in-out);
+            @include font-size(22);
+            display: inline-block;
+            border: 0;
+            padding: 0;
+            background: transparent;
+            color: $blue-l3;
+            vertical-align: middle;
+
+            &:hover {
+              color: $blue;
+            }
+          }
+        }
       }
 
       .group-configuration-fields {
         margin-bottom: $baseline;
       }
+
+      .action-add-group {
+        @extend %ui-btn-flat-outline;
+        @include font-size(16);
+        display: block;
+        width: 100%;
+        margin: ($baseline*1.5) 0 0 0;
+        padding: ($baseline/2);
+        font-weight: 600;
+      }
     }
   }
 

cms/templates/group_configurations.html

@@ -11,7 +11,7 @@
 <%block name="bodyclass">is-signedin course view-group-configurations</%block>
 
 <%block name="header_extras">
-% for template_name in ["group-configuration-details", "group-configuration-edit", "no-group-configurations", "basic-modal", "modal-button"]:
+% for template_name in ["group-configuration-details", "group-configuration-edit", "no-group-configurations", "group-edit", "basic-modal", "modal-button"]:
   <script type="text/template" id="${template_name}-tpl">
     <%static:include path="js/${template_name}.underscore" />
   </script>
@@ -23,7 +23,7 @@
 require(["domReady!", "js/collections/group_configuration", "js/views/pages/group_configurations"],
 function(doc, GroupConfigurationCollection, GroupConfigurationsPage) {
   % if configurations is not None:
-    var collection = new GroupConfigurationCollection(${json.dumps(configurations)});
+    var collection = new GroupConfigurationCollection(${json.dumps(configurations)}, { parse: true });
 
     collection.url = "${group_configuration_url}";
     new GroupConfigurationsPage({

cms/templates/js/group-configuration-edit.underscore

@@ -25,6 +25,13 @@
                 <span class="tip tip-stacked"><%= gettext("Optional long description") %></span>
             </div>
         </fieldset>
+        <fieldset class="groups-fields">
+            <legend class="sr"><%= gettext("Group information") %></legend>
+            <label class="groups-fields-label required"><%= gettext("Groups") %></label>
+            <span class="tip tip-stacked"><%= gettext("Name of the groups that students will be assigned to, for example, Control, Video, Problems. You must have two or more groups.") %></span>
+            <ol class="groups list-input enum"></ol>
+            <button class="action action-add-group"><i class="icon-plus"></i> <%= gettext("Add another group") %></button>
+        </fieldset>
     </div>
     <div class="actions">
         <button class="action action-primary" type="submit"><% if (isNew) { print(gettext("Create")) } else { print(gettext("Save")) } %></button>

cms/templates/js/group-edit.underscore

+<div class="input-wrap field long text required field-add-group-name group-<%= index %>-name
+    <% if (error && error.attributes && error.attributes.name) { print('error'); } %>"><input name="group-<%= index %>-name" class="group-name long" value="<%= name %>" type="text">
+</div><div class="group-allocation"><%= allocation %>%</div>
+<a href="" class="action action-close"><i class="icon-remove-sign"></i> <span class="sr"><%= gettext("delete group") %></span></a>

common/test/acceptance/pages/studio/container.py

@@ -126,7 +126,7 @@ class ContainerPage(PageObject):
         """
         Click the "add missing groups" link.
         """
-        click_css(self, '.add-missing-groups-button')
+        self.q(css='.add-missing-groups-button').first.click()
 
     def missing_groups_button_present(self):
         """

common/test/acceptance/pages/studio/settings_group_configurations.py

@@ -55,6 +55,13 @@ class GroupConfiguration(object):
         css = 'a.group-toggle'
         self.find_css(css).first.click()
 
+    def add_group(self):
+        """
+        Add new group.
+        """
+        css = 'button.action-add-group'
+        self.find_css(css).first.click()
+
     def get_text(self, css):
         """
         Return text for the defined by css locator.
@@ -144,10 +151,10 @@ class GroupConfiguration(object):
         """
         css = '.group'
 
-        def group_selector(config_index, group_index):
-            return self.get_selector('.groups-{} .group-{} '.format(config_index, group_index))
+        def group_selector(group_index):
+            return self.get_selector('.group-{} '.format(group_index))
 
-        return [Group(self.page, group_selector(self.index, index)) for index, element in enumerate(self.find_css(css))]
+        return [Group(self.page, group_selector(index)) for index, element in enumerate(self.find_css(css))]
 
     def __repr__(self):
         return "<{}:{}>".format(self.__class__.__name__, self.name)
@@ -170,11 +177,19 @@ class Group(object):
     @property
     def name(self):
         """
-        Return group name.
+        Return the name of the group .
         """
         css = '.group-name'
         return self.find_css(css).first.text[0]
 
+    @name.setter
+    def name(self, value):
+        """
+        Set the name for the group.
+        """
+        css = '.group-name'
+        self.find_css(css).first.fill(value)
+
     @property
     def allocation(self):
         """
@@ -183,5 +198,12 @@ class Group(object):
         css = '.group-allocation'
         return self.find_css(css).first.text[0]
 
+    def remove(self):
+        """
+        Remove the group.
+        """
+        css = '.action-close'
+        return self.find_css(css).first.click()
+
     def __repr__(self):
         return "<{}:{}>".format(self.__class__.__name__, self.name)

common/test/acceptance/tests/test_studio_container.py

@@ -11,7 +11,7 @@ from ..pages.studio.component_editor import ComponentEditorView
 from ..pages.studio.utils import add_discussion
 
 from unittest import skip
-from bok_choy.promise import Promise
+
 
 class ContainerBase(UniqueCourseTest):
     """
@@ -93,30 +93,6 @@ class ContainerBase(UniqueCourseTest):
                     break
         self.assertEqual(len(blocks_checked), len(xblocks))
 
-    def verify_groups(self, container, active_groups, inactive_groups):
-        """
-        Check that the groups appear and are correctly categorized as to active and inactive.
-
-        Also checks that the "add missing groups" button/link is not present unless a value of False is passed
-        for verify_missing_groups_not_present.
-        """
-        def wait_for_xblocks_to_render():
-            # First xblock is the container for the page, subtract 1.
-            return (len(active_groups) + len(inactive_groups) == len(container.xblocks) - 1, len(active_groups))
-
-        Promise(wait_for_xblocks_to_render, "Number of xblocks on the page are incorrect").fulfill()
-
-        def check_xblock_names(expected_groups, actual_blocks):
-            self.assertEqual(len(expected_groups), len(actual_blocks))
-            for idx, expected in enumerate(expected_groups):
-                self.assertEqual('Expand or Collapse\n{}'.format(expected), actual_blocks[idx].name)
-
-        check_xblock_names(active_groups, container.active_xblocks)
-        check_xblock_names(inactive_groups, container.inactive_xblocks)
-
-        # Verify inactive xblocks appear after active xblocks
-        check_xblock_names(active_groups + inactive_groups, container.xblocks[1:])
-
     def do_action_and_verify(self, action, expected_ordering):
         """
         Perform the supplied action and then verify the resulting ordering.