Merge pull request #4424 from edx/anton/edit-group-configurations

Edit group configurations.

cms/djangoapps/contentstore/views/course.py

@@ -869,43 +869,90 @@ class GroupConfiguration(object):
     """
     Prepare Group Configuration for the course.
     """
+    def __init__(self, json_string, course, configuration_id=None):
+        """
+        Receive group configuration as a json (`json_string`), deserialize it
+        and validate.
+        """
+        self.configuration = GroupConfiguration.parse(json_string)
+        self.course = course
+        self.assign_id(configuration_id)
+        self.assign_group_ids()
+        self.validate()
+
     @staticmethod
-    def parse(configuration_json):
+    def parse(json_string):
         """
-        Parse given string that represents group configuration.
+        Deserialize given json that represents group configuration.
         """
         try:
-            group_configuration = json.loads(configuration_json)
+            configuration = json.loads(json_string)
         except ValueError:
             raise GroupConfigurationsValidationError(_("invalid JSON"))
 
-        if not group_configuration.get('version'):
-            group_configuration['version'] = UserPartition.VERSION
+        return configuration
+
+    def validate(self):
+        """
+        Validate group configuration representation.
+        """
+        if not self.configuration.get("name"):
+            raise GroupConfigurationsValidationError(_("must have name of the configuration"))
+        if len(self.configuration.get('groups', [])) < 2:
+            raise GroupConfigurationsValidationError(_("must have at least two groups"))
+
+    def generate_id(self):
+        """
+        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:
+            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()
 
+    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 group_configuration.get('groups'):
-            group_configuration['groups'] = [
+        if not self.configuration.get('groups'):
+            self.configuration['groups'] = [
                 {'name': 'Group A'}, {'name': 'Group B'},
             ]
 
-        for group in group_configuration['groups']:
-            group['version'] = Group.VERSION
-        return group_configuration
+        # Assign ids to every group in configuration.
+        for index, group in enumerate(self.configuration.get('groups', [])):
+            group['id'] = index
 
-    @staticmethod
-    def validate(group_configuration):
+    def get_used_ids(self):
         """
-        Validate group configuration representation.
+        Return a list of IDs that already in use.
         """
-        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"))
+        return set([p.id for p in self.course.user_partitions])
+
+    def get_user_partition(self):
+        """
+        Get user partition for saving in course.
+        """
+        groups = [Group(g["id"], g["name"]) for g in self.configuration["groups"]]
+
+        return UserPartition(
+            self.configuration["id"],
+            self.configuration["name"],
+            self.configuration["description"],
+            groups
+        )
+
 
 @require_http_methods(("GET", "POST"))
 @login_required
@@ -932,40 +979,63 @@ def group_configurations_list_handler(request, course_key_string):
             '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':
+    elif "application/json" in request.META.get('HTTP_ACCEPT'):
+        if 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
+            try:
+                new_configuration = GroupConfiguration(request.body, course).get_user_partition()
+            except GroupConfigurationsValidationError as err:
+                return JsonResponse({"error": err.message}, status=400)
 
-        course.user_partitions.append(UserPartition.from_json(configuration))
-        store.update_item(course, request.user.id)
-        response = JsonResponse(configuration, status=201)
+            course.user_partitions.append(new_configuration)
+            response = JsonResponse(new_configuration.to_json(), status=201)
 
-        response["Location"] = reverse_course_url(
-            'group_configurations_detail_handler',
-            course.id,
-            kwargs={'group_configuration_id': configuration["id"]}
-        )
-        return response
+            response["Location"] = reverse_course_url(
+                'group_configurations_detail_handler',
+                course.id,
+                kwargs={'group_configuration_id': new_configuration.id}  # pylint: disable=no-member
+            )
+            store.update_item(course, request.user.id)
+            return response
     else:
         return HttpResponse(status=406)
 
 
-@require_http_methods(("GET", "POST"))
 @login_required
 @ensure_csrf_cookie
+@require_http_methods(("POST", "PUT"))
 def group_configurations_detail_handler(request, course_key_string, group_configuration_id):
-    return JsonResponse(status=404)
+    """
+    JSON API endpoint for manipulating a group configuration via its internal ID.
+    Used by the Backbone application.
+
+    POST or PUT
+        json: update group configuration based on provided information
+    """
+    course_key = CourseKey.from_string(course_key_string)
+    course = _get_course_module(course_key, request.user)
+    store = modulestore()
+    matching_id = [p for p in course.user_partitions
+                   if unicode(p.id) == unicode(group_configuration_id)]
+    if matching_id:
+        configuration = matching_id[0]
+    else:
+        configuration = None
+
+    if request.method in ('POST', 'PUT'):  # can be either and sometimes
+                                        # django is rewriting one to the other
+        try:
+            new_configuration = GroupConfiguration(request.body, course, group_configuration_id).get_user_partition()
+        except GroupConfigurationsValidationError as err:
+            return JsonResponse({"error": err.message}, status=400)
+
+        if configuration:
+            index = course.user_partitions.index(configuration)
+            course.user_partitions[index] = new_configuration
+        else:
+            course.user_partitions.append(new_configuration)
+        store.update_item(course, request.user.id)
+        return JsonResponse(new_configuration.to_json(), status=201)
 
 
 def _get_course_creator_status(user):

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

@@ -65,6 +65,7 @@ class TextbookIndexTestCase(CourseTestCase):
         )
         self.assertEqual(resp.status_code, 200)
         obj = json.loads(resp.content)
+
         self.assertEqual(content, obj)
 
     def test_view_index_xhr_put(self):

cms/static/js/models/group.js

@@ -4,7 +4,10 @@ define([
     'use strict';
     var Group = Backbone.AssociatedModel.extend({
         defaults: function() {
-            return { name: '' };
+            return {
+                name: '',
+                version: null
+            };
         },
 
         isEmpty: function() {
@@ -12,7 +15,10 @@ define([
         },
 
         toJSON: function() {
-            return { name: this.get('name') };
+            return {
+                name: this.get('name'),
+                version: this.get('version')
+             };
         },
 
         validate: function(attrs) {

cms/static/js/models/group_configuration.js

@@ -10,6 +10,7 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
             return {
                 name: '',
                 description: '',
+                version: null,
                 groups: new GroupCollection([]),
                 showGroups: false,
                 editing: false
@@ -51,6 +52,7 @@ function(Backbone, _, str, gettext, GroupModel, GroupCollection) {
                 id: this.get('id'),
                 name: this.get('name'),
                 description: this.get('description'),
+                version: this.get('version'),
                 groups: this.get('groups').toJSON()
             };
         },

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

@@ -90,30 +90,36 @@ define([
 
             it('should match server model to client model', function() {
                 var serverModelSpec = {
-                      'id': 10,
-                      'name': 'My Group Configuration',
-                      'description': 'Some description',
-                      'groups': [
-                        {
-                          'name': 'Group 1'
-                        }, {
-                          'name': 'Group 2'
-                        }
-                      ]
+                        'id': 10,
+                        'name': 'My Group Configuration',
+                        'description': 'Some description',
+                        'version': 1,
+                        'groups': [
+                            {
+                                'version': 1,
+                                'name': 'Group 1'
+                            }, {
+                                'version': 1,
+                                'name': 'Group 2'
+                            }
+                        ]
                     },
                     clientModelSpec = {
-                      'id': 10,
-                      'name': 'My Group Configuration',
-                      'description': 'Some description',
-                      'showGroups': false,
-                      'editing': false,
-                      'groups': [
-                        {
-                          'name': 'Group 1'
-                        }, {
-                          'name': 'Group 2'
-                        }
-                      ]
+                        'id': 10,
+                        'name': 'My Group Configuration',
+                        'description': 'Some description',
+                        'showGroups': false,
+                        'editing': false,
+                        'version': 1,
+                        'groups': [
+                            {
+                                'version': 1,
+                                'name': 'Group 1'
+                            }, {
+                                'version': 1,
+                                'name': 'Group 2'
+                            }
+                        ]
                     },
                     model = new GroupConfigurationModel(serverModelSpec);
 

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

@@ -273,7 +273,9 @@ define([
 
     describe('GroupConfigurationItem', function() {
         beforeEach(function() {
-            this.model = new GroupConfigurationModel({ });
+            view_helpers.installTemplate('group-configuration-edit', true);
+            view_helpers.installTemplate('group-configuration-details');
+            this.model = new GroupConfigurationModel({ id: 0 });
             this.collection = new GroupConfigurationCollection([ this.model ]);
             this.view = new GroupConfigurationItem({
                 model: this.model
@@ -284,10 +286,10 @@ define([
         it('should render properly', function() {
             // Details view by default
             expect(this.view.$(SELECTORS.detailsView)).toExist();
-            this.model.set('editing', true);
+            this.view.$('.action-edit .edit').click();
             expect(this.view.$(SELECTORS.editView)).toExist();
             expect(this.view.$(SELECTORS.detailsView)).not.toExist();
-            this.model.set('editing', false);
+            this.view.$('.action-cancel').click();
             expect(this.view.$(SELECTORS.detailsView)).toExist();
             expect(this.view.$(SELECTORS.editView)).not.toExist();
         });

cms/static/js/views/group_configuration_details.js

@@ -6,6 +6,7 @@ function(BaseView, _, gettext) {
     var GroupConfigurationDetails = BaseView.extend({
         tagName: 'div',
         events: {
+            'click .edit': 'editConfiguration',
             'click .show-groups': 'showGroups',
             'click .hide-groups': 'hideGroups'
         },
@@ -36,6 +37,11 @@ function(BaseView, _, gettext) {
             return this;
         },
 
+        editConfiguration: function(event) {
+            if(event && event.preventDefault) { event.preventDefault(); }
+            this.model.set('editing', true);
+        },
+
         showGroups: function(e) {
             if(e && e.preventDefault) { e.preventDefault(); }
             this.model.set('showGroups', true);

cms/static/js/views/group_configuration_item.js

@@ -44,6 +44,7 @@ define([
             }
 
             this.$el.html(this.view.render().el);
+            this.$el.focus();
 
             return this;
         }

cms/static/js/views/group_configurations_list.js

@@ -34,6 +34,7 @@ define([
 
                 this.$el.html([frag]);
             }
+
             return this;
         },
 

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

@@ -157,6 +157,24 @@
             }
           }
         }
+
+        .actions {
+          @include transition(opacity .15s .25s ease-in-out);
+          opacity: 0.0;
+          position: absolute;
+          top: $baseline;
+          right: $baseline;
+
+          .action {
+            display: inline-block;
+            margin-right: ($baseline/4);
+
+            .edit {
+              @include blue-button;
+              @extend %t-action4;
+            }
+          }
+        }
       }
 
       &:hover .actions {
@@ -322,6 +340,26 @@
 
           &.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 {
+              font-weight: 600;
+              white-space: nowrap;
+              margin-left: ($baseline*0.5);
+            }
           }
         }
 

cms/templates/group_configurations.html

@@ -69,7 +69,17 @@ function(doc, GroupConfigurationCollection, GroupConfigurationsPage) {
       % endif
       </article>
       <aside class="content-supplementary" role="complimentary">
-        <div class="bit">
+      <div class="bit">
+        <h3 class="title-3">${_("What can I do on this page?")}</h3>
+        <p>${_("You can create and edit group configurations.")}</p>
+
+        <p>${_("A group configuration defines how many groups of students are in an experiment. When you create a content experiment, you select the group configuration to use.")}</p>
+
+        <p>${_("Click {em_start}New Group Configuration{em_end} to add a new configuration. To edit an existing configuration, hover over its box and click {em_start}Edit{em_end}.").format(em_start='<strong>', em_end="</strong>")}</p>
+
+        <p><a href="${get_online_help_info(online_help_token())['doc_url']}" target="_blank">${_("Learn More")}</a></p>
+      </div>
+      <div class="bit">
       % if context_course:
         <%
           details_url = utils.reverse_course_url('settings_handler', context_course.id)

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

@@ -37,4 +37,9 @@
           <% }) %>
         </ol>
     <% } %>
+    <ul class="actions group-configuration-actions">
+        <li class="action action-edit">
+            <button class="edit"><%= gettext("Edit") %></button>
+        </li>
+    </ul>
 </div>

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

@@ -8,7 +8,14 @@
         <fieldset class="group-configuration-fields">
             <legend class="sr"><%= gettext("Group Configuration information") %></legend>
             <div class="input-wrap field text required add-group-configuration-name <% if(error && error.attributes && error.attributes.name) { print('error'); } %>">
-                <label for="group-configuration-name-<%= uniqueId %>"><%= gettext("Group Configuration Name") %></label>
+                <label for="group-configuration-name-<%= uniqueId %>"><%= gettext("Group Configuration Name") %></label><%
+                if (!_.isUndefined(id)) {
+                    %><span class="group-configuration-id">
+                        <span class="group-configuration-label"><%= gettext('Group Configuration ID') %></span>
+                        <span class="group-configuration-value"><%= id %></span>
+                    </span><%
+                }
+                %>
                 <input id="group-configuration-name-<%= uniqueId %>" class="group-configuration-name-input input-text" name="group-configuration-name" type="text" placeholder="<%= gettext("This is the Name of the Group Configuration") %>" value="<%= name %>">
                 <span class="tip tip-stacked"><%= gettext("Name or short description of the configuration") %></span>
             </div>

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

@@ -63,7 +63,7 @@ class ComponentEditorView(PageObject):
             action = action.send_keys(Keys.BACKSPACE)
         # Send the new text, then Tab to move to the next field (so change event is triggered).
         action.send_keys(value).send_keys(Keys.TAB).perform()
-        click_css(self, 'a.action-save')
+        self.save()
 
     def set_select_value_and_save(self, label, value):
         """
@@ -72,4 +72,27 @@ class ComponentEditorView(PageObject):
         elem = self.get_setting_element(label)
         select = Select(elem)
         select.select_by_value(value)
+        self.save()
+
+    def get_selected_option_text(self, label):
+        """
+        Returns the text of the first selected option for the select with given label (display name).
+        """
+        elem = self.get_setting_element(label)
+        if elem:
+            select = Select(elem)
+            return select.first_selected_option.text
+        else:
+            return None
+
+    def save(self):
+        """
+        Clicks save button.
+        """
         click_css(self, 'a.action-save')
+
+    def cancel(self):
+        """
+        Clicks cancel button.
+        """
+        click_css(self, 'a.action-cancel', require_notification=False)

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

@@ -134,6 +134,12 @@ class ContainerPage(PageObject):
         """
         return self.q(css='.add-missing-groups-button').present
 
+    def get_xblock_information_message(self):
+        """
+        Returns an information message for the container page.
+        """
+        return self.q(css=".xblock-message.information").first.text[0]
+
 
 class XBlockWrapper(PageObject):
     """

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

@@ -17,7 +17,7 @@ class GroupConfigurationsPage(CoursePage):
 
     def group_configurations(self):
         """
-        Returns list of the group configurations for the course.
+        Return list of the group configurations for the course.
         """
         css = '.group-configurations-list-item'
         return [GroupConfiguration(self, index) for index in xrange(len(self.q(css=css)))]
@@ -55,6 +55,19 @@ class GroupConfiguration(object):
         css = 'a.group-toggle'
         self.find_css(css).first.click()
 
+    def get_text(self, css):
+        """
+        Return text for the defined by css locator.
+        """
+        return self.find_css(css).first.text[0]
+
+    def edit(self):
+        """
+        Open editing view for the group configuration.
+        """
+        css = '.action-edit .edit'
+        self.find_css(css).first.click()
+
     def save(self):
         """
         Save group configuration.
@@ -73,7 +86,7 @@ class GroupConfiguration(object):
     @property
     def mode(self):
         """
-        Returns group configuration mode.
+        Return group configuration mode.
         """
         if self.find_css('.group-configuration-edit').present:
             return 'edit'
@@ -83,31 +96,28 @@ class GroupConfiguration(object):
     @property
     def id(self):
         """
-        Returns group configuration id.
+        Return group configuration id.
         """
-        css = '.group-configuration-id .group-configuration-value'
-        return self.find_css(css).first.text[0]
+        return self.get_text('.group-configuration-id .group-configuration-value')
 
     @property
     def validation_message(self):
         """
-        Returns validation message.
+        Return validation message.
         """
-        css = '.message-status.error'
-        return self.find_css(css).first.text[0]
+        return self.get_text('.message-status.error')
 
     @property
     def name(self):
         """
-        Returns group configuration name.
+        Return group configuration name.
         """
-        css = '.group-configuration-title'
-        return self.find_css(css).first.text[0]
+        return self.get_text('.group-configuration-title')
 
     @name.setter
     def name(self, value):
         """
-        Sets group configuration name.
+        Set group configuration name.
         """
         css = '.group-configuration-name-input'
         self.find_css(css).first.fill(value)
@@ -115,15 +125,14 @@ class GroupConfiguration(object):
     @property
     def description(self):
         """
-        Returns group configuration description.
+        Return group configuration description.
         """
-        css = '.group-configuration-description'
-        return self.find_css(css).first.text[0]
+        return self.get_text('.group-configuration-description')
 
     @description.setter
     def description(self, value):
         """
-        Sets group configuration description.
+        Set group configuration description.
         """
         css = '.group-configuration-description-input'
         self.find_css(css).first.fill(value)
@@ -131,7 +140,7 @@ class GroupConfiguration(object):
     @property
     def groups(self):
         """
-        Returns list of groups.
+        Return list of groups.
         """
         css = '.group'
 
@@ -161,7 +170,7 @@ class Group(object):
     @property
     def name(self):
         """
-        Returns group name.
+        Return group name.
         """
         css = '.group-name'
         return self.find_css(css).first.text[0]
@@ -169,7 +178,7 @@ class Group(object):
     @property
     def allocation(self):
         """
-        Returns allocation for the group.
+        Return allocation for the group.
         """
         css = '.group-allocation'
         return self.find_css(css).first.text[0]

docs/config.ini

@@ -33,9 +33,10 @@ export = building_course/export_import_course.html#export-a-course
 welcome = getting_started/get_started.html
 login = getting_started/get_started.html
 register = getting_started/get_started.html
+group_configurations = content_experiments/content_experiments_configure.html#set-up-group-configurations-in-edx-studio
 
 # below are the language directory names for the different locales
 [locales]
 default = en
 en = en
-en_us = en
+en_us = en
\ No newline at end of file