From 27013e74f41c4339cdeb77234a083bfc12d160b9 Mon Sep 17 00:00:00 2001 From: polesye Date: Thu, 24 Jul 2014 11:30:45 +0300 Subject: [PATCH] BLD-1049: Remove Group Configurations. --- cms/djangoapps/contentstore/views/course.py | 30 +- .../views/tests/test_group_configurations.py | 73 ++- .../js/spec/views/group_configuration_spec.js | 102 +++- cms/static/js/spec_helpers/create_sinon.js | 25 +- cms/static/js/spec_helpers/view_helpers.js | 75 ++- .../js/views/group_configuration_edit.js | 2 +- .../js/views/group_configuration_item.js | 21 + .../js/views/group_configurations_list.js | 7 + .../sass/views/_group-configuration.scss | 441 +++++++++--------- cms/templates/base.html | 3 +- cms/templates/group_configurations.html | 8 +- .../js/group-configuration-details.underscore | 9 + .../js/group-configuration-edit.underscore | 11 + .../src/discussion/tooltip_manager.coffee | 8 +- common/test/acceptance/fixtures/course.py | 1 - .../studio/settings_group_configurations.py | 54 ++- common/test/acceptance/pages/studio/utils.py | 11 + .../tests/test_studio_split_test.py | 70 +++ 18 files changed, 661 insertions(+), 290 deletions(-) diff --git a/cms/djangoapps/contentstore/views/course.py b/cms/djangoapps/contentstore/views/course.py index 0f74fd41b2..dce7966c0c 100644 --- a/cms/djangoapps/contentstore/views/course.py +++ b/cms/djangoapps/contentstore/views/course.py @@ -982,7 +982,7 @@ class GroupConfiguration(object): ) @staticmethod - def _get_usage_info(course, modulestore): + def get_usage_info(course, store): """ Get all units names and their urls that have experiments and associated with configurations. @@ -996,18 +996,18 @@ class GroupConfiguration(object): } """ usage_info = {} - descriptors = modulestore.get_items(course.id, category='split_test') + descriptors = store.get_items(course.id, category='split_test') for split_test in descriptors: if split_test.user_partition_id not in usage_info: usage_info[split_test.user_partition_id] = [] - unit_location = modulestore.get_parent_location(split_test.location) + unit_location = store.get_parent_location(split_test.location) if not unit_location: log.warning("Parent location of split_test module not found: %s", split_test.location) continue try: - unit = modulestore.get_item(unit_location) + unit = store.get_item(unit_location) except ItemNotFoundError: log.warning("Unit not found: %s", unit_location) continue @@ -1023,13 +1023,13 @@ class GroupConfiguration(object): return usage_info @staticmethod - def add_usage_info(course, modulestore): + def add_usage_info(course, store): """ Add usage information to group configurations json. Returns json of group configurations updated with usage information. """ - usage_info = GroupConfiguration._get_usage_info(course, modulestore) + usage_info = GroupConfiguration.get_usage_info(course, store) configurations = [] for partition in course.user_partitions: configuration = partition.to_json() @@ -1091,7 +1091,7 @@ def group_configurations_list_handler(request, course_key_string): @login_required @ensure_csrf_cookie -@require_http_methods(("POST", "PUT")) +@require_http_methods(("POST", "PUT", "DELETE")) def group_configurations_detail_handler(request, course_key_string, group_configuration_id): """ JSON API endpoint for manipulating a group configuration via its internal ID. @@ -1124,6 +1124,22 @@ def group_configurations_detail_handler(request, course_key_string, group_config course.user_partitions.append(new_configuration) store.update_item(course, request.user.id) return JsonResponse(new_configuration.to_json(), status=201) + elif request.method == "DELETE": + if not configuration: + return JsonResponse(status=404) + + # Verify that group configuration is not already in use. + usages = GroupConfiguration.get_usage_info(course, store) + if usages.get(int(group_configuration_id)): + return JsonResponse( + {"error": _("This Group Configuration is already in use and cannot be removed.")}, + status=400 + ) + + index = course.user_partitions.index(configuration) + course.user_partitions.pop(index) + store.update_item(course, request.user.id) + return JsonResponse(status=204) 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 index 34e9466762..8552325db5 100644 --- a/cms/djangoapps/contentstore/views/tests/test_group_configurations.py +++ b/cms/djangoapps/contentstore/views/tests/test_group_configurations.py @@ -22,6 +22,7 @@ GROUP_CONFIGURATION_JSON = { } +# pylint: disable=no-member class HelperMethods(object): """ Mixin that provides useful methods for Group Configuration tests. @@ -137,7 +138,7 @@ class GroupConfigurationsBaseTestCase(object): # pylint: disable=no-member @skipUnless(settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS'), 'Tests Group Configurations feature') -class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase): +class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): """ Test cases for group_configurations_list_handler. """ @@ -233,7 +234,7 @@ class GroupConfigurationsListHandlerTestCase(CourseTestCase, GroupConfigurations # pylint: disable=no-member @skipUnless(settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS'), 'Tests Group Configurations feature') -class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase): +class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfigurationsBaseTestCase, HelperMethods): """ Test cases for group_configurations_detail_handler. """ @@ -294,9 +295,7 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio """ Edit group configuration and check its id and modified fields. """ - self.course.user_partitions = [ - UserPartition(self.ID, 'First name', 'First description', [Group(0, 'Group A'), Group(1, 'Group B'), Group(2, 'Group C')]), - ] + self._add_user_partitions() self.save_course() expected = { @@ -327,6 +326,65 @@ class GroupConfigurationsDetailHandlerTestCase(CourseTestCase, GroupConfiguratio self.assertEqual(user_partititons[0].groups[0].name, u'New Group Name') self.assertEqual(user_partititons[0].groups[1].name, u'Group C') + def test_can_delete_group_configuration(self): + """ + Delete group configuration and check user partitions. + """ + self._add_user_partitions(count=2) + self.save_course() + + response = self.client.delete( + self._url(cid=0), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest", + ) + self.assertEqual(response.status_code, 204) + self.reload_course() + # Verify that user_partitions is properly updated in the course. + user_partititons = self.course.user_partitions + self.assertEqual(len(user_partititons), 1) + self.assertEqual(user_partititons[0].name, u'Name 1') + + def test_cannot_delete_used_group_configuration(self): + """ + Cannot delete group configuration if it is in use. + """ + self._add_user_partitions(count=2) + self._create_content_experiment(cid=0) + + response = self.client.delete( + self._url(cid=0), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest", + ) + self.assertEqual(response.status_code, 400) + content = json.loads(response.content) + self.assertTrue(content['error']) + self.reload_course() + # Verify that user_partitions is still the same. + user_partititons = self.course.user_partitions + self.assertEqual(len(user_partititons), 2) + self.assertEqual(user_partititons[0].name, u'Name 0') + + def test_cannot_delete_non_existent_group_configuration(self): + """ + Cannot delete group configuration if it is doesn't exist. + """ + self._add_user_partitions(count=2) + response = self.client.delete( + self._url(cid=999), + content_type="application/json", + HTTP_ACCEPT="application/json", + HTTP_X_REQUESTED_WITH="XMLHttpRequest", + ) + self.assertEqual(response.status_code, 404) + # Verify that user_partitions is still the same. + user_partititons = self.course.user_partitions + self.assertEqual(len(user_partititons), 2) + self.assertEqual(user_partititons[0].name, u'Name 0') + # pylint: disable=no-member @skipUnless(settings.FEATURES.get('ENABLE_GROUP_CONFIGURATIONS'), 'Tests Group Configurations feature') @@ -335,6 +393,9 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): Tests for usage information of configurations. """ def setUp(self): + """ + Set up group configurations and split test module. + """ super(GroupConfigurationsUsageInfoTestCase, self).setUp() def test_group_configuration_not_used(self): @@ -439,5 +500,5 @@ class GroupConfigurationsUsageInfoTestCase(CourseTestCase, HelperMethods): display_name='Test Content Experiment' ) self.save_course() - actual = GroupConfiguration._get_usage_info(self.course, self.store) + actual = GroupConfiguration.get_usage_info(self.course, self.store) self.assertEqual(actual, {0: []}) diff --git a/cms/static/js/spec/views/group_configuration_spec.js b/cms/static/js/spec/views/group_configuration_spec.js index 100ae9890c..758e9cce66 100644 --- a/cms/static/js/spec/views/group_configuration_spec.js +++ b/cms/static/js/spec/views/group_configuration_spec.js @@ -1,5 +1,5 @@ define([ - 'js/models/course', 'js/models/group_configuration', + 'underscore', 'js/models/course', 'js/models/group_configuration', 'js/collections/group_configuration', 'js/views/group_configuration_details', 'js/views/group_configurations_list', 'js/views/group_configuration_edit', @@ -8,7 +8,7 @@ define([ 'js/views/feedback_notification', 'js/spec_helpers/create_sinon', 'js/spec_helpers/edit_helpers', 'jasmine-stealth' ], function( - Course, GroupConfigurationModel, GroupConfigurationCollection, + _, Course, GroupConfigurationModel, GroupConfigurationCollection, GroupConfigurationDetails, GroupConfigurationsList, GroupConfigurationEdit, GroupConfigurationItem, GroupModel, GroupCollection, GroupEdit, Notification, create_sinon, view_helpers @@ -33,7 +33,8 @@ define([ usageText: '.group-configuration-usage-text', usageTextAnchor: '.group-configuration-usage-text > a', usageUnit: '.group-configuration-usage-unit', - usageUnitAnchor: '.group-configuration-usage-unit > a' + usageUnitAnchor: '.group-configuration-usage-unit > a', + note: '.wrapper-delete-button' }; beforeEach(function() { @@ -105,6 +106,7 @@ define([ it('should render properly', function() { expect(this.view.$el).toContainText('Configuration'); expect(this.view.$el).toContainText('ID: 0'); + expect(this.view.$('.delete')).toExist(); }); it('should show groups appropriately', function() { @@ -171,6 +173,10 @@ define([ usageUnitAnchors = this.view.$(SELECTORS.usageUnitAnchor); + expect(this.view.$(SELECTORS.note)).toHaveAttr( + 'data-tooltip', 'Cannot delete when in use by an experiment' + ); + expect(this.view.$('.delete')).toHaveClass('is-disabled'); expect(this.view.$(SELECTORS.usageCount)).not.toExist(); expect(this.view.$(SELECTORS.usageText)) .toContainText('This Group Configuration is used in:'); @@ -183,15 +189,17 @@ define([ }); it('should hide non-empty usage appropriately', function() { - this.model.set('usage', - [ - {'label': 'label1', 'url': 'url1'}, - {'label': 'label2', 'url': 'url2'} - ] - ); + this.model.set('usage', [ + {'label': 'label1', 'url': 'url1'}, + {'label': 'label2', 'url': 'url2'} + ]); this.model.set('showGroups', true); this.view.$('.hide-groups').click(); + expect(this.view.$(SELECTORS.note)).toHaveAttr( + 'data-tooltip', 'Cannot delete when in use by an experiment' + ); + expect(this.view.$('.delete')).toHaveClass('is-disabled'); expect(this.view.$(SELECTORS.usageText)).not.toExist(); expect(this.view.$(SELECTORS.usageUnit)).not.toExist(); expect(this.view.$(SELECTORS.usageCount)) @@ -234,6 +242,7 @@ define([ name: 'Configuration', description: 'Configuration Description' }); + expect(this.view.$('.delete')).toExist(); }); it ('should allow you to create new groups', function() { @@ -372,7 +381,7 @@ define([ }); }); - it('groups have correct default names and placeholders', function () { + it('groups have correct default names', function () { var group1 = new GroupModel({ name: 'Group A' }), group2 = new GroupModel({ name: 'Group B' }), collection = this.model.get('groups'); @@ -400,12 +409,24 @@ define([ 'Group A', 'Group C', 'Group D', 'Group E', 'Group F', 'Group G' ]); }); + + it('cannot be deleted if it is in use', function () { + this.model.set('usage', [ {'label': 'label1', 'url': 'url1'} ]); + this.view.render(); + expect(this.view.$(SELECTORS.note)).toHaveAttr( + 'data-tooltip', 'Cannot delete when in use by an experiment' + ); + expect(this.view.$('.delete')).toHaveClass('is-disabled'); + }); }); describe('GroupConfigurationsList', function() { + var emptyMessage = 'You haven\'t created any group configurations yet.'; + beforeEach(function() { view_helpers.installTemplate('no-group-configurations', true); + this.model = new GroupConfigurationModel({ id: 0 }); this.collection = new GroupConfigurationCollection(); this.view = new GroupConfigurationsList({ collection: this.collection @@ -415,39 +436,52 @@ define([ describe('empty template', function () { it('should be rendered if no group configurations', function() { - expect(this.view.$el).toContainText( - 'You haven\'t created any group configurations yet.' - ); + expect(this.view.$el).toContainText(emptyMessage); expect(this.view.$('.new-button')).toExist(); expect(this.view.$(SELECTORS.itemView)).not.toExist(); }); it('should disappear if group configuration is added', function() { - var emptyMessage = 'You haven\'t created any group ' + - 'configurations yet.'; - expect(this.view.$el).toContainText(emptyMessage); expect(this.view.$(SELECTORS.itemView)).not.toExist(); - this.collection.add([{}]); + this.collection.add(this.model); expect(this.view.$el).not.toContainText(emptyMessage); expect(this.view.$(SELECTORS.itemView)).toExist(); }); + + it('should appear if configurations were removed', function() { + this.collection.add(this.model); + expect(this.view.$(SELECTORS.itemView)).toExist(); + this.collection.remove(this.model); + expect(this.view.$el).toContainText(emptyMessage); + expect(this.view.$(SELECTORS.itemView)).not.toExist(); + }); }); }); describe('GroupConfigurationItem', function() { + var clickDeleteItem; + beforeEach(function() { view_helpers.installTemplates([ 'group-configuration-edit', 'group-configuration-details' ], true); this.model = new GroupConfigurationModel({ id: 0 }); this.collection = new GroupConfigurationCollection([ this.model ]); + this.collection.url = '/group_configurations'; this.view = new GroupConfigurationItem({ model: this.model }); appendSetFixtures(this.view.render().el); }); + clickDeleteItem = function (view, promptSpy) { + view.$('.delete').click(); + view_helpers.verifyPromptShowing(promptSpy, /Delete this Group Configuration/); + view_helpers.confirmPrompt(promptSpy); + view_helpers.verifyPromptHidden(promptSpy); + }; + it('should render properly', function() { // Details view by default expect(this.view.$(SELECTORS.detailsView)).toExist(); @@ -458,6 +492,40 @@ define([ expect(this.view.$(SELECTORS.detailsView)).toExist(); expect(this.view.$(SELECTORS.editView)).not.toExist(); }); + + it('should destroy itself on confirmation of deleting', function () { + var requests = create_sinon.requests(this), + promptSpy = view_helpers.createPromptSpy(), + notificationSpy = view_helpers.createNotificationSpy(); + + clickDeleteItem(this.view, promptSpy); + // Backbone.emulateHTTP is enabled in our system, so setting this + // option will fake PUT, PATCH and DELETE requests with a HTTP POST, + // setting the X-HTTP-Method-Override header with the true method. + create_sinon.expectJsonRequest(requests, 'POST', '/group_configurations/0'); + expect(_.last(requests).requestHeaders['X-HTTP-Method-Override']).toBe('DELETE'); + view_helpers.verifyNotificationShowing(notificationSpy, /Deleting/); + create_sinon.respondToDelete(requests); + view_helpers.verifyNotificationHidden(notificationSpy); + expect($(SELECTORS.itemView)).not.toExist(); + }); + + it('does not hide deleting message if failure', function() { + var requests = create_sinon.requests(this), + promptSpy = view_helpers.createPromptSpy(), + notificationSpy = view_helpers.createNotificationSpy(); + + clickDeleteItem(this.view, promptSpy); + // Backbone.emulateHTTP is enabled in our system, so setting this + // option will fake PUT, PATCH and DELETE requests with a HTTP POST, + // setting the X-HTTP-Method-Override header with the true method. + create_sinon.expectJsonRequest(requests, 'POST', '/group_configurations/0'); + expect(_.last(requests).requestHeaders['X-HTTP-Method-Override']).toBe('DELETE'); + view_helpers.verifyNotificationShowing(notificationSpy, /Deleting/); + create_sinon.respondWithError(requests); + view_helpers.verifyNotificationShowing(notificationSpy, /Deleting/); + expect($(SELECTORS.itemView)).toExist(); + }); }); describe('GroupEdit', function() { diff --git a/cms/static/js/spec_helpers/create_sinon.js b/cms/static/js/spec_helpers/create_sinon.js index 8ecf77c603..9121561e7c 100644 --- a/cms/static/js/spec_helpers/create_sinon.js +++ b/cms/static/js/spec_helpers/create_sinon.js @@ -1,5 +1,5 @@ define(["sinon", "underscore"], function(sinon, _) { - var fakeServer, fakeRequests, respondWithJson, respondWithError; + var fakeServer, fakeRequests, expectJsonRequest, respondWithJson, respondWithError, respondToDelete; /* These utility methods are used by Jasmine tests to create a mock server or * get reference to mock requests. In either case, the cleanup (restore) is done with @@ -45,6 +45,17 @@ define(["sinon", "underscore"], function(sinon, _) { return requests; }; + expectJsonRequest = function(requests, method, url, jsonRequest, requestIndex) { + var request; + if (_.isUndefined(requestIndex)) { + requestIndex = requests.length - 1; + } + request = requests[requestIndex]; + expect(request.url).toEqual(url); + expect(request.method).toEqual(method); + expect(JSON.parse(request.requestBody)).toEqual(jsonRequest); + }; + respondWithJson = function(requests, jsonResponse, requestIndex) { if (_.isUndefined(requestIndex)) { requestIndex = requests.length - 1; @@ -63,10 +74,20 @@ define(["sinon", "underscore"], function(sinon, _) { JSON.stringify({ })); }; + respondToDelete = function(requests, requestIndex) { + if (_.isUndefined(requestIndex)) { + requestIndex = requests.length - 1; + } + requests[requestIndex].respond(204, + { "Content-Type": "application/json" }); + }; + return { "server": fakeServer, "requests": fakeRequests, + "expectJsonRequest": expectJsonRequest, "respondWithJson": respondWithJson, - "respondWithError": respondWithError + "respondWithError": respondWithError, + "respondToDelete": respondToDelete }; }); diff --git a/cms/static/js/spec_helpers/view_helpers.js b/cms/static/js/spec_helpers/view_helpers.js index 9baa72fe27..58ad26627d 100644 --- a/cms/static/js/spec_helpers/view_helpers.js +++ b/cms/static/js/spec_helpers/view_helpers.js @@ -1,19 +1,22 @@ /** * Provides helper methods for invoking Studio modal windows in Jasmine tests. */ -define(["jquery", "js/views/feedback_notification", "js/spec_helpers/create_sinon"], - function($, NotificationView, create_sinon) { - var installTemplate, installTemplates, installViewTemplates, createNotificationSpy, - verifyNotificationShowing, verifyNotificationHidden; +define(['jquery', 'js/views/feedback_notification', 'js/views/feedback_prompt'], + function($, NotificationView, Prompt) { + 'use strict'; + var installTemplate, installTemplates, installViewTemplates, createFeedbackSpy, verifyFeedbackShowing, + verifyFeedbackHidden, createNotificationSpy, verifyNotificationShowing, + verifyNotificationHidden, createPromptSpy, confirmPrompt, verifyPromptShowing, + verifyPromptHidden; installTemplate = function(templateName, isFirst) { var template = readFixtures(templateName + '.underscore'), templateId = templateName + '-tpl'; if (isFirst) { - setFixtures($("