Merge pull request #14910 from edx/ai/TNL-4412-beta-tester-addition

TNL-4412 user is not enrolled as a beta tester when account is unactivated.

common/test/acceptance/pages/lms/auto_auth.py

@@ -18,7 +18,7 @@ class AutoAuthPage(PageObject):
     CONTENT_REGEX = r'.+? user (?P<username>\S+) \((?P<email>.+?)\) with password \S+ and user_id (?P<user_id>\d+)$'
 
     def __init__(self, browser, username=None, email=None, password=None, full_name=None, staff=None, course_id=None,
-                 enrollment_mode=None, roles=None):
+                 enrollment_mode=None, roles=None, is_active=None):
         """
         Auto-auth is an end-point for HTTP GET requests.
         By default, it will create accounts with random user credentials,
@@ -62,6 +62,9 @@ class AutoAuthPage(PageObject):
         if roles is not None:
             self._params['roles'] = roles
 
+        if is_active is not None:
+            self._params['is_active'] = "true" if is_active else "false"
+
     @property
     def url(self):
         """

common/test/acceptance/pages/lms/instructor_dashboard.py

@@ -194,6 +194,12 @@ class MembershipPage(PageObject):
         """
         return MembershipPageAutoEnrollSection(self.browser)
 
+    def batch_beta_tester_addition(self):
+        """
+        Returns the MembershipPageBetaTesterSection page object.
+        """
+        return MembershipPageBetaTesterSection(self.browser)
+
 
 class SpecialExamsPage(PageObject):
     """
@@ -880,6 +886,44 @@ class MembershipPageAutoEnrollSection(PageObject):
         return self.q(css="{} h3".format(notification_selector)).text
 
 
+class MembershipPageBetaTesterSection(PageObject):
+    """
+    Beta tester section of the Membership tab of the Instructor dashboard.
+    """
+    url = None
+
+    batch_beta_tester_heading_selector = '#heading-batch-beta-testers'
+    batch_beta_tester_selector = '.batch-beta-testers'
+
+    def is_browser_on_page(self):
+        return self.q(css=self.batch_beta_tester_heading_selector).present
+
+    def fill_batch_beta_tester_addition_text_box(self, username):
+        """
+        Fill in the form with the provided username and submit it.
+        """
+        username_selector = "{} textarea".format(self.batch_beta_tester_selector)
+        enrollment_button = "{} .enrollment-button[data-action='add']".format(self.batch_beta_tester_selector)
+
+        # Fill the username  after the username selector is visible.
+        self.wait_for_element_visibility(username_selector, 'username field is visible')
+        self.q(css=username_selector).fill(username)
+
+        # Verify enrollment button is present before clicking
+        self.wait_for_element_visibility(enrollment_button, 'Add beta testers button')
+        self.q(css=enrollment_button).click()
+
+    def get_notification_text(self):
+        """
+        Check notification div is visible and have message.
+        """
+        notification_selector = '{} .request-response'.format(self.batch_beta_tester_selector)
+        self.wait_for_element_visibility(notification_selector, 'Notification div is visible')
+        notification_header_text = self.q(css="{} h3".format(notification_selector)).text
+        notification_username = self.q(css="{} li".format(notification_selector)).text
+        return notification_header_text, notification_username
+
+
 class SpecialExamsPageAllowanceSection(PageObject):
     """
     Allowance section of the Instructor dashboard's Special Exams tab.

common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py

@@ -213,6 +213,55 @@ class AutoEnrollmentWithCSVTest(BaseInstructorDashboardTest):
         self.auto_enroll_section.a11y_audit.check_for_accessibility_errors()
 
 
+class BatchBetaTestersTest(BaseInstructorDashboardTest):
+    """
+    End-to-end tests for Batch beta testers functionality.
+    """
+
+    def setUp(self):
+        super(BatchBetaTestersTest, self).setUp()
+        self.username = "test_{uuid}".format(uuid=self.unique_id[0:6])
+        self.email = "{user}@example.com".format(user=self.username)
+        AutoAuthPage(self.browser, username=self.username, email=self.email, is_active=False).visit()
+        self.course_fixture = CourseFixture(**self.course_info).install()
+        self.instructor_username = self.log_in_as_instructor()
+        instructor_dashboard_page = self.visit_instructor_dashboard()
+        self.batch_beta_tester_section = instructor_dashboard_page.select_membership().batch_beta_tester_addition()
+        self.inactive_user_message = 'These users could not be added as beta testers ' \
+                                     'because their accounts are not yet activated:'
+
+    def test_enroll_inactive_beta_tester(self):
+        """
+        Scenario: On the Membership tab of the Instructor Dashboard, Batch Beta tester div is visible.
+            Given that I am on the Membership tab on the Instructor Dashboard
+            Then I enter the username and add it into beta testers.
+            Then I see the inactive user is not added in beta testers.
+        """
+        self.batch_beta_tester_section.fill_batch_beta_tester_addition_text_box(self.username)
+        header_text, username = self.batch_beta_tester_section.get_notification_text()
+        self.assertIn(self.inactive_user_message, header_text[0])
+        self.assertEqual(self.username, username[0])
+
+    def test_enroll_active_and_inactive_beta_tester(self):
+        """
+        Scenario: On the Membership tab of the Instructor Dashboard, Batch Beta tester div is visible.
+            Given that I am on the Membership tab on the Instructor Dashboard
+            Then I enter the active and inactive usernames and add it into beta testers.
+            Then I see the different messages related to active and inactive users.
+        """
+        active_and_inactive_username = self.username + ',' + self.instructor_username[0]
+        self.batch_beta_tester_section.fill_batch_beta_tester_addition_text_box(active_and_inactive_username)
+        header_text, username = self.batch_beta_tester_section.get_notification_text()
+
+        # Verify that Inactive username and message.
+        self.assertIn(self.inactive_user_message, header_text[1])
+        self.assertEqual(self.username, username[1])
+
+        # Verify that active username and message.
+        self.assertIn('These users were successfully added as beta testers:', header_text[0])
+        self.assertEqual(self.instructor_username[0], username[0])
+
+
 @attr(shard=10)
 class ProctoredExamsTest(BaseInstructorDashboardTest):
     """

lms/djangoapps/instructor/enrollment.py

@@ -196,20 +196,15 @@ def send_beta_role_email(action, user, email_params):
     `user` is the User affected
     `email_params` parameters used while parsing email templates (a `dict`).
     """
-    if action == 'add':
-        email_params['message'] = 'add_beta_tester'
+    if action in ('add', 'remove'):
+        email_params['message'] = '%s_beta_tester' % action
         email_params['email_address'] = user.email
         email_params['full_name'] = user.profile.name
-
-    elif action == 'remove':
-        email_params['message'] = 'remove_beta_tester'
-        email_params['email_address'] = user.email
-        email_params['full_name'] = user.profile.name
-
     else:
         raise ValueError("Unexpected action received '{}' - expected 'add' or 'remove'".format(action))
-
-    send_mail_to_student(user.email, email_params, language=get_user_email_language(user))
+    trying_to_add_inactive_user = not user.is_active and action == 'add'
+    if not trying_to_add_inactive_user:
+        send_mail_to_student(user.email, email_params, language=get_user_email_language(user))
 
 
 def reset_student_attempts(course_id, student, module_state_key, requesting_user, delete_module=False):

lms/djangoapps/instructor/tests/test_api.py

@@ -1774,7 +1774,8 @@ class TestInstructorAPIBulkBetaEnrollment(SharedModuleStoreTestCase, LoginEnroll
                 {
                     "identifier": identifier,
                     "error": False,
-                    "userDoesNotExist": False
+                    "userDoesNotExist": False,
+                    "is_active": True
                 }
             ]
         }
@@ -1825,7 +1826,8 @@ class TestInstructorAPIBulkBetaEnrollment(SharedModuleStoreTestCase, LoginEnroll
                 {
                     "identifier": self.notenrolled_student.email,
                     "error": False,
-                    "userDoesNotExist": False
+                    "userDoesNotExist": False,
+                    "is_active": True
                 }
             ]
         }
@@ -1872,7 +1874,8 @@ class TestInstructorAPIBulkBetaEnrollment(SharedModuleStoreTestCase, LoginEnroll
                 {
                     "identifier": self.notenrolled_student.email,
                     "error": False,
-                    "userDoesNotExist": False
+                    "userDoesNotExist": False,
+                    "is_active": True
                 }
             ]
         }
@@ -1935,7 +1938,8 @@ class TestInstructorAPIBulkBetaEnrollment(SharedModuleStoreTestCase, LoginEnroll
                 {
                     "identifier": self.notregistered_email,
                     "error": True,
-                    "userDoesNotExist": True
+                    "userDoesNotExist": True,
+                    "is_active": None
                 }
             ]
         }
@@ -1965,7 +1969,8 @@ class TestInstructorAPIBulkBetaEnrollment(SharedModuleStoreTestCase, LoginEnroll
                 {
                     "identifier": self.beta_tester.email,
                     "error": False,
-                    "userDoesNotExist": False
+                    "userDoesNotExist": False,
+                    "is_active": True
                 }
             ]
         }
@@ -1995,7 +2000,8 @@ class TestInstructorAPIBulkBetaEnrollment(SharedModuleStoreTestCase, LoginEnroll
                 {
                     "identifier": self.beta_tester.email,
                     "error": False,
-                    "userDoesNotExist": False
+                    "userDoesNotExist": False,
+                    "is_active": True
                 }
             ]
         }

lms/djangoapps/instructor/views/api.py

@@ -760,6 +760,7 @@ def bulk_beta_modify_access(request, course_id):
             error = False
             user_does_not_exist = False
             user = get_student_from_identifier(identifier)
+            user_active = user.is_active
 
             if action == 'add':
                 allow_access(course, user, rolename)
@@ -772,6 +773,7 @@ def bulk_beta_modify_access(request, course_id):
         except User.DoesNotExist:
             error = True
             user_does_not_exist = True
+            user_active = None
         # catch and log any unexpected exceptions
         # so that one error doesn't cause a 500.
         except Exception as exc:  # pylint: disable=broad-except
@@ -793,7 +795,8 @@ def bulk_beta_modify_access(request, course_id):
             results.append({
                 'identifier': identifier,
                 'error': error,
-                'userDoesNotExist': user_does_not_exist
+                'userDoesNotExist': user_does_not_exist,
+                'is_active': user_active
             })
 
     response_payload = {

lms/static/js/instructor_dashboard/membership.js

@@ -461,16 +461,27 @@ such that the value can be defined later than this assignment (file load order).
                 return displayResponse.$task_response.append($taskResSection);
             };
             if (successes.length && dataFromServer.action === 'add') {
-                // Translators: A list of users appears after this sentence;
-                renderList(gettext('These users were successfully added as beta testers:'), (function() {
-                    var j, len1, results;
-                    results = [];
-                    for (j = 0, len1 = successes.length; j < len1; j++) {
-                        sr = successes[j];
-                        results.push(sr.identifier);
+                var j, len1, inActiveUsers, activeUsers; // eslint-disable-line vars-on-top
+                activeUsers = [];
+                inActiveUsers = [];
+                for (j = 0, len1 = successes.length; j < len1; j++) {
+                    sr = successes[j];
+                    if (sr.is_active) {
+                        activeUsers.push(sr.identifier);
+                    } else {
+                        inActiveUsers.push(sr.identifier);
                     }
-                    return results;
-                }()));
+                }
+                if (activeUsers.length) {
+                    // Translators: A list of users appears after this sentence;
+                    renderList(gettext('These users were successfully added as beta testers:'), activeUsers);
+                }
+                if (inActiveUsers.length) {
+                    // Translators: A list of users appears after this sentence;
+                    renderList(gettext(
+                        'These users could not be added as beta testers because their accounts are not yet activated:'),
+                        inActiveUsers);
+                }
             }
             if (successes.length && dataFromServer.action === 'remove') {
                 // Translators: A list of users appears after this sentence;
@@ -524,7 +535,6 @@ such that the value can be defined later than this assignment (file load order).
             }
             return renderList();
         };
-
         return betaTesterBulkAddition;
     }());