diff --git a/common/test/acceptance/pages/lms/auto_auth.py b/common/test/acceptance/pages/lms/auto_auth.py index 42ea5ca5c3..601a6bf83c 100644 --- a/common/test/acceptance/pages/lms/auto_auth.py +++ b/common/test/acceptance/pages/lms/auto_auth.py @@ -18,7 +18,7 @@ class AutoAuthPage(PageObject): CONTENT_REGEX = r'.+? user (?P\S+) \((?P.+?)\) with password \S+ and user_id (?P\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): """ diff --git a/common/test/acceptance/pages/lms/instructor_dashboard.py b/common/test/acceptance/pages/lms/instructor_dashboard.py index b1c96e8aa3..3d6dab0ebd 100644 --- a/common/test/acceptance/pages/lms/instructor_dashboard.py +++ b/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. diff --git a/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py b/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py index 7efe185ed4..d2633d5fad 100644 --- a/common/test/acceptance/tests/lms/test_lms_instructor_dashboard.py +++ b/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): """ diff --git a/lms/djangoapps/instructor/enrollment.py b/lms/djangoapps/instructor/enrollment.py index bf1dff0294..07b75efa20 100644 --- a/lms/djangoapps/instructor/enrollment.py +++ b/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): diff --git a/lms/djangoapps/instructor/tests/test_api.py b/lms/djangoapps/instructor/tests/test_api.py index 1f67070d89..b06a99bd7d 100644 --- a/lms/djangoapps/instructor/tests/test_api.py +++ b/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 } ] } diff --git a/lms/djangoapps/instructor/views/api.py b/lms/djangoapps/instructor/views/api.py index 930635ef59..fbe6fd97ce 100644 --- a/lms/djangoapps/instructor/views/api.py +++ b/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 = { diff --git a/lms/static/js/instructor_dashboard/membership.js b/lms/static/js/instructor_dashboard/membership.js index 01d8f728cd..9cf969a5b0 100644 --- a/lms/static/js/instructor_dashboard/membership.js +++ b/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; }());