Merge pull request #15635 from edx/ssemenova/ed-699

Educator-699 Modify deleted group warning on the unit/container page

cms/static/js/factories/xblock_validation.js

 define(['js/views/xblock_validation', 'js/models/xblock_validation'],
 function(XBlockValidationView, XBlockValidationModel) {
     'use strict';
-    return function(validationMessages, hasEditingUrl, isRoot, validationEle) {
+    return function(validationMessages, hasEditingUrl, isRoot, isUnit, validationEle) {
+        var model, response;
+
         if (hasEditingUrl && !isRoot) {
             validationMessages.showSummaryOnly = true;
         }
+        response = validationMessages;
+        response.isUnit = isUnit;
 
-        var model = new XBlockValidationModel(validationMessages, {parse: true});
+        model = new XBlockValidationModel(response, {parse: true});
 
         if (!model.get('empty')) {
             new XBlockValidationView({el: validationEle, model: model, root: isRoot}).render();

cms/static/js/models/xblock_validation.js

@@ -19,7 +19,11 @@ define(['backbone', 'gettext', 'underscore'], function(Backbone, gettext, _) {
                 var summary = 'summary' in response ? response.summary : {};
                 var messages = 'messages' in response ? response.messages : [];
                 if (!summary.text) {
-                    summary.text = gettext('This component has validation issues.');
+                    if (response.isUnit) {
+                        summary.text = gettext('This unit has validation issues.');
+                    } else {
+                        summary.text = gettext('This component has validation issues.');
+                    }
                 }
                 if (!summary.type) {
                     summary.type = this.WARNING;

cms/static/js/spec/factories/xblock_validation_spec.js

 define(['jquery', 'js/factories/xblock_validation', 'common/js/spec_helpers/template_helpers'],
     function($, XBlockValidationFactory, TemplateHelpers) {
         describe('XBlockValidationFactory', function() {
-            var messageDiv;
+            var $messageDiv;
 
             beforeEach(function() {
                 TemplateHelpers.installTemplate('xblock-validation-messages');
                 appendSetFixtures($('<div class="messages"></div>'));
-                messageDiv = $('.messages');
+                $messageDiv = $('.messages');
             });
 
             it('Does not attach a view if messages is empty', function() {
-                XBlockValidationFactory({'empty': true}, false, false, messageDiv);
-                expect(messageDiv.children().length).toEqual(0);
+                XBlockValidationFactory({empty: true}, false, false, false, $messageDiv);
+                expect($messageDiv.children().length).toEqual(0);
             });
 
             it('Does attach a view if messages are not empty', function() {
-                XBlockValidationFactory({'empty': false}, false, false, messageDiv);
-                expect(messageDiv.children().length).toEqual(1);
+                XBlockValidationFactory({empty: false}, false, false, false, $messageDiv);
+                expect($messageDiv.children().length).toEqual(1);
             });
 
             it('Passes through the root property to the view.', function() {
@@ -29,12 +29,12 @@ define(['jquery', 'js/factories/xblock_validation', 'common/js/spec_helpers/temp
                     'xblock_id': 'id'
                 };
                 // Root is false, will not add noContainerContent.
-                XBlockValidationFactory(notConfiguredMessages, true, false, messageDiv);
-                expect(messageDiv.find('.validation')).not.toHaveClass(noContainerContent);
+                XBlockValidationFactory(notConfiguredMessages, true, false, false, $messageDiv);
+                expect($messageDiv.find('.validation')).not.toHaveClass(noContainerContent);
 
                 // Root is true, will add noContainerContent.
-                XBlockValidationFactory(notConfiguredMessages, true, true, messageDiv);
-                expect(messageDiv.find('.validation')).toHaveClass(noContainerContent);
+                XBlockValidationFactory(notConfiguredMessages, true, true, false, $messageDiv);
+                expect($messageDiv.find('.validation')).toHaveClass(noContainerContent);
             });
 
             describe('Controls display of detailed messages based on url and root property', function() {
@@ -50,25 +50,25 @@ define(['jquery', 'js/factories/xblock_validation', 'common/js/spec_helpers/temp
                 });
 
                 checkDetailedMessages = function(expectedDetailedMessages) {
-                    expect(messageDiv.children().length).toEqual(1);
-                    expect(messageDiv.find('.xblock-message-item').length).toBe(expectedDetailedMessages);
+                    expect($messageDiv.children().length).toEqual(1);
+                    expect($messageDiv.find('.xblock-message-item').length).toBe(expectedDetailedMessages);
                 };
 
                 it('Does not show details if xblock has an editing URL and it is not rendered as root', function() {
-                    XBlockValidationFactory(messagesWithSummary, true, false, messageDiv);
+                    XBlockValidationFactory(messagesWithSummary, true, false, false, $messageDiv);
                     checkDetailedMessages(0);
                 });
 
                 it('Shows details if xblock does not have its own editing URL, regardless of root value', function() {
-                    XBlockValidationFactory(messagesWithSummary, false, false, messageDiv);
+                    XBlockValidationFactory(messagesWithSummary, false, false, false, $messageDiv);
                     checkDetailedMessages(2);
 
-                    XBlockValidationFactory(messagesWithSummary, false, true, messageDiv);
+                    XBlockValidationFactory(messagesWithSummary, false, true, false, $messageDiv);
                     checkDetailedMessages(2);
                 });
 
                 it('Shows details if xblock has its own editing URL and is rendered as root', function() {
-                    XBlockValidationFactory(messagesWithSummary, true, true, messageDiv);
+                    XBlockValidationFactory(messagesWithSummary, true, true, false, $messageDiv);
                     checkDetailedMessages(2);
                 });
             });

cms/templates/studio_xblock_wrapper.html

@@ -3,6 +3,7 @@
 from django.utils.translation import ugettext as _
 from contentstore.views.helpers import xblock_studio_url
 from contentstore.utils import is_visible_to_specific_partition_groups
+from lms.lib.utils import is_unit
 from openedx.core.djangolib.js_utils import (
     dump_js_escaped_json, js_escaped_string
 )
@@ -14,6 +15,7 @@ section_class = "level-nesting" if show_inline else "level-element"
 collapsible_class = "is-collapsible" if xblock.has_children else ""
 label = xblock.display_name_with_default or xblock.scope_ids.block_type
 messages = xblock.validate().to_json()
+block_is_unit = is_unit(xblock)
 %>
 
 <%namespace name='static' file='static_content.html'/>
@@ -30,6 +32,7 @@ messages = xblock.validate().to_json()
             ${messages | n, dump_js_escaped_json},
             ${bool(xblock_url) | n, dump_js_escaped_json},  // xblock_url will be None or a string
             ${bool(is_root) | n, dump_js_escaped_json},  // is_root will be None or a boolean
+            ${bool(block_is_unit) | n, dump_js_escaped_json},  // block_is_unit will be None or a boolean
             $('div.xblock-validation-messages[data-locator="${xblock.location | n, js_escaped_string}"]')
         );
     });

cms/templates/visibility_editor.html

@@ -4,6 +4,7 @@ from django.conf import settings
 from django.utils.translation import ugettext as _
 from contentstore.utils import ancestor_has_staff_lock, get_visibility_partition_info
 from openedx.core.djangolib.markup import HTML, Text
+from lms.lib.utils import is_unit
 
 partition_info = get_visibility_partition_info(xblock)
 selectable_partitions = partition_info["selectable_partitions"]
@@ -11,18 +12,30 @@ selected_partition_index = partition_info["selected_partition_index"]
 selected_groups_label = partition_info["selected_groups_label"]
 
 is_staff_locked = ancestor_has_staff_lock(xblock)
+block_is_unit = is_unit(xblock)
 %>
-
 <div class="modal-section visibility-summary">
     % if len(selectable_partitions) == 0:
         <div class="is-not-configured has-actions">
             <h3 class="title">${_('Access is not restricted')}</h3>
             <div class="copy">
-                <p>${_('Access to this component is not restricted, but visibility might be affected by inherited settings.')}</p>
+                % if block_is_unit:
+                    <p>${_('Access to this unit is not restricted, but visibility might be affected by inherited settings.')}</p>
+                % else:
+                    <p>${_('Access to this component is not restricted, but visibility might be affected by inherited settings.')}</p>
+                %endif
                 % if settings.FEATURES.get('ENABLE_ENROLLMENT_TRACK_USER_PARTITION'):
-                <p>${_('You can restrict access to this component to learners in specific enrollment tracks or content groups.')}</p>
+                    % if block_is_unit:
+                        <p>${_('You can restrict access to this unit to learners in specific enrollment tracks or content groups.')}</p>
+                    % else:
+                        <p>${_('You can restrict access to this component to learners in specific enrollment tracks or content groups.')}</p>
+                    % endif
                 % else:
-                <p>${_('You can restrict access to this component to learners in specific content groups.')}</p>
+                    % if block_is_unit:
+                        <p>${_('You can restrict access to this unit to learners in specific content groups.')}</p>
+                    % else:
+                        <p>${_('You can restrict access to this component to learners in specific content groups.')}</p>
+                    % endif
                 % endif
             </div>
 
@@ -98,7 +111,9 @@ is_staff_locked = ancestor_has_staff_lock(xblock)
                             % if group["deleted"]:
                             <label for="visibility-group-${partition["id"]}-${group["id"]}" class="label">
                                 ${_("Deleted Group")}
-                                <span class="note">${_('This group no longer exists. Choose another group or do not restrict access to this component.')}</span>
+                                <span class="note">
+                                    ${_('This group no longer exists.  Choose another group or remove the access restriction.')}
+                                </span>
                             </label>
                             % else:
                             <label for="visibility-group-${partition["id"]}-${group["id"]}" class="label">${group["name"]}</label>

common/test/acceptance/tests/studio/test_studio_container.py

@@ -319,7 +319,7 @@ class BaseGroupConfigurationsTest(ContainerBase):
     CHOOSE_ONE = "Select a group type"
     CONTENT_GROUP_PARTITION = XBlockVisibilityEditorView.CONTENT_GROUP_PARTITION
     ENROLLMENT_TRACK_PARTITION = XBlockVisibilityEditorView.ENROLLMENT_TRACK_PARTITION
-    MISSING_GROUP_LABEL = 'Deleted Group\nThis group no longer exists. Choose another group or do not restrict access to this component.'
+    MISSING_GROUP_LABEL = 'Deleted Group\nThis group no longer exists. Choose another group or remove the access restriction.'
     VALIDATION_ERROR_LABEL = 'This component has validation issues.'
     VALIDATION_ERROR_MESSAGE = "Error:\nThis component's access settings refer to deleted or invalid groups."
     GROUP_VISIBILITY_MESSAGE = 'Access to some content in this unit is restricted to specific groups of learners.'

lms/djangoapps/lms_xblock/mixin.py

@@ -8,6 +8,7 @@ from xblock.core import XBlock
 from xblock.fields import Boolean, Dict, Scope, String, XBlockMixin
 from xblock.validation import ValidationMessage
 
+from lms.lib.utils import is_unit
 from xmodule.modulestore.inheritance import UserPartitionList
 from xmodule.partitions.partitions import NoSuchUserPartitionError, NoSuchUserPartitionGroupError
 
@@ -15,8 +16,16 @@ from xmodule.partitions.partitions import NoSuchUserPartitionError, NoSuchUserPa
 # more information can be found here: https://openedx.atlassian.net/browse/PLAT-902
 _ = lambda text: text
 
-INVALID_USER_PARTITION_VALIDATION = _(u"This component's access settings refer to deleted or invalid group configurations.")
-INVALID_USER_PARTITION_GROUP_VALIDATION = _(u"This component's access settings refer to deleted or invalid groups.")
+INVALID_USER_PARTITION_VALIDATION_COMPONENT = _(
+    u"This component's access settings refer to deleted or invalid group configurations."
+)
+INVALID_USER_PARTITION_VALIDATION_UNIT = _(
+    u"This unit's access settings refer to deleted or invalid group configurations."
+)
+INVALID_USER_PARTITION_GROUP_VALIDATION_COMPONENT = _(
+    u"This component's access settings refer to deleted or invalid groups."
+)
+INVALID_USER_PARTITION_GROUP_VALIDATION_UNIT = _(u"This unit's access settings refer to deleted or invalid groups.")
 NONSENSICAL_ACCESS_RESTRICTION = _(u"This component's access settings contradict its parent's access settings.")
 
 
@@ -32,6 +41,7 @@ class GroupAccessDict(Dict):
 
 
 @XBlock.needs('partitions')
+@XBlock.needs('i18n')
 class LmsBlockMixin(XBlockMixin):
     """
     Mixin that defines fields common to all blocks used in the LMS
@@ -185,6 +195,7 @@ class LmsBlockMixin(XBlockMixin):
         validation = super(LmsBlockMixin, self).validate()
         has_invalid_user_partitions = False
         has_invalid_groups = False
+        block_is_unit = is_unit(self)
 
         for user_partition_id, group_ids in self.group_access.iteritems():
             try:
@@ -204,7 +215,9 @@ class LmsBlockMixin(XBlockMixin):
             validation.add(
                 ValidationMessage(
                     ValidationMessage.ERROR,
-                    INVALID_USER_PARTITION_VALIDATION
+                    (INVALID_USER_PARTITION_VALIDATION_UNIT
+                     if block_is_unit
+                     else INVALID_USER_PARTITION_VALIDATION_COMPONENT)
                 )
             )
 
@@ -212,7 +225,9 @@ class LmsBlockMixin(XBlockMixin):
             validation.add(
                 ValidationMessage(
                     ValidationMessage.ERROR,
-                    INVALID_USER_PARTITION_GROUP_VALIDATION
+                    (INVALID_USER_PARTITION_GROUP_VALIDATION_UNIT
+                     if block_is_unit
+                     else INVALID_USER_PARTITION_GROUP_VALIDATION_COMPONENT)
                 )
             )
 

lms/lib/tests/test_utils.py

@@ -57,3 +57,11 @@ class LmsUtilsTest(ModuleStoreTestCase):
         self.assertIsNone(utils.get_parent_unit(self.course))
         self.assertIsNone(utils.get_parent_unit(self.chapter))
         self.assertIsNone(utils.get_parent_unit(self.sequential))
+
+    def test_is_unit(self):
+        """
+        Tests `is_unit` method for the successful result.
+        """
+        self.assertFalse(utils.is_unit(self.html_module_1))
+        self.assertFalse(utils.is_unit(self.child_vertical))
+        self.assertTrue(utils.is_unit(self.vertical))

lms/lib/utils.py

@@ -28,3 +28,19 @@ def get_parent_unit(xblock):
         if parent.category == "vertical" and grandparent.category == "sequential":
             return parent
         xblock = parent
+
+
+def is_unit(xblock):
+    """
+    Checks whether the xblock is a unit.
+
+    Get_parent_unit() returns None if the current xblock either does
+    not have a parent unit or is itself a unit.
+    To make sure that get_parent_unit() isn't returning None because
+    the xblock is an orphan, we check that the xblock has a parent.
+
+    Returns:
+        True if the xblock is itself a unit, False otherwise.
+    """
+
+    return get_parent_unit(xblock) is None and xblock.get_parent()

lms/lib/xblock/test/test_mixin.py

@@ -5,7 +5,11 @@ import ddt
 from nose.plugins.attrib import attr
 
 from lms_xblock.mixin import (
-    INVALID_USER_PARTITION_VALIDATION, INVALID_USER_PARTITION_GROUP_VALIDATION, NONSENSICAL_ACCESS_RESTRICTION
+    INVALID_USER_PARTITION_GROUP_VALIDATION_COMPONENT,
+    INVALID_USER_PARTITION_GROUP_VALIDATION_UNIT,
+    INVALID_USER_PARTITION_VALIDATION_COMPONENT,
+    INVALID_USER_PARTITION_VALIDATION_UNIT,
+    NONSENSICAL_ACCESS_RESTRICTION
 )
 from xblock.validation import ValidationMessage
 from xmodule.modulestore import ModuleStoreEnum
@@ -92,14 +96,14 @@ class XBlockValidationTest(LmsXBlockMixinTestCase):
 
     def test_validate_invalid_user_partitions(self):
         """
-        Test the validation messages produced for an xblock referring to non-existent user partitions.
+        Test the validation messages produced for a component referring to non-existent user partitions.
         """
         self.set_group_access(self.video_location, {999: [self.group1.id]})
         validation = self.store.get_item(self.video_location).validate()
         self.assertEqual(len(validation.messages), 1)
         self.verify_validation_message(
             validation.messages[0],
-            INVALID_USER_PARTITION_VALIDATION,
+            INVALID_USER_PARTITION_VALIDATION_COMPONENT,
             ValidationMessage.ERROR,
         )
 
@@ -111,7 +115,32 @@ class XBlockValidationTest(LmsXBlockMixinTestCase):
         self.assertEqual(len(validation.messages), 1)
         self.verify_validation_message(
             validation.messages[0],
-            INVALID_USER_PARTITION_VALIDATION,
+            INVALID_USER_PARTITION_VALIDATION_COMPONENT,
+            ValidationMessage.ERROR,
+        )
+
+    def test_validate_invalid_user_partitions_unit(self):
+        """
+        Test the validation messages produced for a unit referring to non-existent user partitions.
+        """
+        self.set_group_access(self.vertical_location, {999: [self.group1.id]})
+        validation = self.store.get_item(self.vertical_location).validate()
+        self.assertEqual(len(validation.messages), 1)
+        self.verify_validation_message(
+            validation.messages[0],
+            INVALID_USER_PARTITION_VALIDATION_UNIT,
+            ValidationMessage.ERROR,
+        )
+
+        # Now add a second invalid user partition and validate again.
+        # Note that even though there are two invalid configurations,
+        # only a single error message will be returned.
+        self.set_group_access(self.vertical_location, {998: [self.group2.id]})
+        validation = self.store.get_item(self.vertical_location).validate()
+        self.assertEqual(len(validation.messages), 1)
+        self.verify_validation_message(
+            validation.messages[0],
+            INVALID_USER_PARTITION_VALIDATION_UNIT,
             ValidationMessage.ERROR,
         )
 
@@ -124,7 +153,7 @@ class XBlockValidationTest(LmsXBlockMixinTestCase):
         self.assertEqual(len(validation.messages), 1)
         self.verify_validation_message(
             validation.messages[0],
-            INVALID_USER_PARTITION_GROUP_VALIDATION,
+            INVALID_USER_PARTITION_GROUP_VALIDATION_COMPONENT,
             ValidationMessage.ERROR,
         )
 
@@ -134,7 +163,7 @@ class XBlockValidationTest(LmsXBlockMixinTestCase):
         self.assertEqual(len(validation.messages), 1)
         self.verify_validation_message(
             validation.messages[0],
-            INVALID_USER_PARTITION_GROUP_VALIDATION,
+            INVALID_USER_PARTITION_GROUP_VALIDATION_COMPONENT,
             ValidationMessage.ERROR,
         )
 
@@ -165,6 +194,19 @@ class XBlockValidationTest(LmsXBlockMixinTestCase):
             ValidationMessage.ERROR,
         )
 
+    def test_validate_invalid_groups_for_unit(self):
+        """
+        Test the validation messages produced for a unit-level xblock referring to non-existent groups.
+        """
+        self.set_group_access(self.vertical_location, {self.user_partition.id: [self.group1.id, 999]})
+        validation = self.store.get_item(self.vertical_location).validate()
+        self.assertEqual(len(validation.messages), 1)
+        self.verify_validation_message(
+            validation.messages[0],
+            INVALID_USER_PARTITION_GROUP_VALIDATION_UNIT,
+            ValidationMessage.ERROR,
+        )
+
     def test_validate_nonsensical_access_restriction(self):
         """
         Test the validation messages produced for a component whose
@@ -221,7 +263,7 @@ class XBlockValidationTest(LmsXBlockMixinTestCase):
         self.assertEqual(len(validation.messages), 2)
         self.verify_validation_message(
             validation.messages[0],
-            INVALID_USER_PARTITION_GROUP_VALIDATION,
+            INVALID_USER_PARTITION_GROUP_VALIDATION_COMPONENT,
             ValidationMessage.ERROR,
         )
         self.verify_validation_message(