Merge pull request #6650 from edx/dan-f/validate-group-configuration-group-names

Require unique group names in group configurations

cms/static/js/factories/group_configurations.js

@@ -7,7 +7,9 @@ define([
         var experimentGroupConfigurations = new GroupConfigurationCollection(
                 experimentGroupConfigurationsJson, {parse: true}
             ),
-            contentGroupConfiguration = new GroupConfigurationModel(contentGroupConfigurationJson, {parse: true});
+            contentGroupConfiguration = new GroupConfigurationModel(contentGroupConfigurationJson, {
+                parse: true, canBeEmpty: true
+            });
 
         experimentGroupConfigurations.url = groupConfigurationUrl;
         experimentGroupConfigurations.outlineUrl = courseOutlineUrl;

cms/static/js/models/group_configuration.js

@@ -35,7 +35,8 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
             collectionType: GroupCollection
         }],
 
-        initialize: function() {
+        initialize: function(attributes, options) {
+            this.canBeEmpty = options && options.canBeEmpty;
             this.setOriginalAttributes();
             return this;
         },
@@ -45,7 +46,7 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
         },
 
         reset: function() {
-            this.set(this._originalAttributes, { parse: true });
+            this.set(this._originalAttributes, { parse: true, validate: true });
         },
 
         isDirty: function() {
@@ -87,23 +88,35 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
                 };
             }
 
-            if (attrs.groups.length < 1) {
+            if (!this.canBeEmpty && attrs.groups.length < 1) {
                 return {
                     message: gettext('There must be at least one group.'),
                     attributes: { groups: true }
                 };
             } else {
                 // validate all groups
-                var invalidGroups = [];
+                var validGroups = new Backbone.Collection(),
+                    invalidGroups = new Backbone.Collection();
                 attrs.groups.each(function(group) {
-                    if(!group.isValid()) {
-                        invalidGroups.push(group);
+                    if (!group.isValid()) {
+                        invalidGroups.add(group);
+                    } else {
+                        validGroups.add(group);
                     }
                 });
-                if (!_.isEmpty(invalidGroups)) {
+
+                if (!invalidGroups.isEmpty()) {
                     return {
                         message: gettext('All groups must have a name.'),
-                        attributes: { groups: invalidGroups }
+                        attributes: { groups: invalidGroups.toJSON() }
+                    };
+                }
+
+                var groupNames = validGroups.map(function(group) { return group.get('name'); });
+                if (groupNames.length !== _.uniq(groupNames).length) {
+                    return {
+                        message: gettext('All groups must have a unique name.'),
+                        attributes: { groups: validGroups.toJSON() }
                     };
                 }
             }

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

@@ -48,10 +48,12 @@ define([
             });
 
             it('should be able to reset itself', function() {
-                this.model.set('name', 'foobar');
-                this.model.reset();
+                var originalName = 'Original Name',
+                    model = new GroupConfigurationModel({name: originalName});
+                model.set({name: 'New Name'});
+                model.reset();
 
-                expect(this.model.get('name')).toEqual('');
+                expect(model.get('name')).toEqual(originalName);
             });
 
             it('should be dirty after it\'s been changed', function() {
@@ -149,10 +151,42 @@ define([
             });
 
             it('can pass validation', function() {
+                // Note that two groups - Group A and Group B - are
+                // created by default.
                 var model = new GroupConfigurationModel({ name: 'foo' });
 
                 expect(model.isValid()).toBeTruthy();
             });
+
+            it('requires at least one group', function() {
+                var group1 = new GroupModel({ name: 'Group A' }),
+                    model = new GroupConfigurationModel({ name: 'foo', groups: [] });
+
+                expect(model.isValid()).toBeFalsy();
+
+                model.get('groups').add(group1);
+                expect(model.isValid()).toBeTruthy();
+            });
+
+            it('requires a valid group', function() {
+                var model = new GroupConfigurationModel({ name: 'foo', groups: [{ name: '' }] });
+
+                expect(model.isValid()).toBeFalsy();
+            });
+
+            it('requires all groups to be valid', function() {
+                var model = new GroupConfigurationModel({ name: 'foo', groups: [{ name: 'Group A' }, { name: '' }] });
+
+                expect(model.isValid()).toBeFalsy();
+            });
+
+            it('requires all groups to have unique names', function() {
+                var model = new GroupConfigurationModel({
+                    name: 'foo', groups: [{ name: 'Group A' }, { name: 'Group A' }]
+                });
+
+                expect(model.isValid()).toBeFalsy();
+            });
         });
     });
 
@@ -184,36 +218,6 @@ define([
 
                 expect(model.isValid()).toBeTruthy();
             });
-
-            it('requires at least one group', function() {
-                var group1 = new GroupModel({ name: 'Group A' }),
-                    model = new GroupConfigurationModel({ name: 'foo' });
-
-                model.get('groups').reset([]);
-                expect(model.isValid()).toBeFalsy();
-
-                model.get('groups').add(group1);
-                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();
-            });
         });
     });
 

cms/static/js/spec/views/group_configuration_spec.js

@@ -659,7 +659,7 @@ define([
                     id: 0,
                     name: 'Content Group Configuration',
                     groups: groups
-                });
+                }, {canBeEmpty: true});
             groupConfiguration.urlRoot = '/mock_url';
             return groups;
         };

cms/static/js/views/list_item.js

@@ -18,7 +18,7 @@ define([
 ) {
     'use strict';
 
-    var ListItemView     = BaseView.extend({
+    var ListItemView = BaseView.extend({
         canDelete: false,
 
         initialize: function() {

cms/static/js/views/list_item_editor.js

@@ -17,11 +17,12 @@ define([
     var ListItemEditorView = BaseView.extend({
         initialize: function() {
             this.listenTo(this.model, 'invalid', this.render);
+            this.listenTo(this.getSaveableModel(), 'invalid', this.render);
         },
 
         render: function() {
             this.$el.html(this.template(_.extend({
-                error: this.model.validationError
+                error: this.model.validationError || this.getSaveableModel().validationError
             }, this.getTemplateOptions())));
         },
 
@@ -29,7 +30,7 @@ define([
             if (event && event.preventDefault) { event.preventDefault(); }
 
             this.setValues();
-            if (!this.model.isValid()) {
+            if (!this.model.isValid() || !this.getSaveableModel().isValid()) {
                 return false;
             }