From b05af6b344934455ce318ace0bff74ef601c2db5 Mon Sep 17 00:00:00 2001 From: cdeery Date: Tue, 3 Aug 2021 13:15:00 -0400 Subject: [PATCH 1/5] fix: [AA-893] Don't show View As Learner if partitions Remove the option to masquerade as a generic learner if there are enrollment tracks or partition groups because the behavior is unpredictable and unhelpful. Fixes: AA-893 --- lms/djangoapps/courseware/masquerade.py | 20 +++++---- .../courseware/tests/test_masquerade.py | 44 +++++++++++++++++++ 2 files changed, 55 insertions(+), 9 deletions(-) diff --git a/lms/djangoapps/courseware/masquerade.py b/lms/djangoapps/courseware/masquerade.py index 6ca3da6e7a..33485c5386 100644 --- a/lms/djangoapps/courseware/masquerade.py +++ b/lms/djangoapps/courseware/masquerade.py @@ -128,17 +128,19 @@ class MasqueradeView(View): 'name': 'Staff', 'role': 'staff', }, - { - 'name': 'Learner', - 'role': 'student', - }, - { - 'name': 'Specific Student...', - 'role': 'student', - 'user_name': course.user_name or '', - }, ], } + if len(partitions) == 0: + data['available'].append( { + 'name': 'Learner', + 'role': 'student', + }) + + data['available'].append({ + 'name': 'Specific Student...', + 'role': 'student', + 'user_name': course.user_name or '', + }) for partition in partitions: if partition.active: data['available'].extend([ diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index 06ae1151fb..9b2c7f0a65 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -128,6 +128,18 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas ) return self.client.get(url) + def get_available_masquerade_identities(self): + """ + Returns: the server response for masquerade options + """ + url = reverse( + 'masquerade_update', + kwargs={ + 'course_key_string': str(self.course.id), + } + ) + return self.client.get(url) + def verify_staff_debug_present(self, staff_debug_expected): """ Verifies that the staff debug control visibility is as expected (for staff only). @@ -159,6 +171,18 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas assert self.problem_display_name in problem_html assert show_answer_expected == ('Show answer' in problem_html) + def verify_learner_masquerade_available(self, learner_option_expected): + """ + Verifies if learner masquerade option is available + Args: + exists: True to test if Learner is in options, False to test it is NOT + """ + response = self.get_available_masquerade_identities() + items = response.content.json['available'].items() + is_it_there = (('name', 'Learner') in items) + assert learner_option_expected == is_it_there + assert learner_option_expected == (('name', 'Learner') in content.items()) + def ensure_masquerade_as_group_member(self, partition_id, group_id): """ Installs a masquerade for the test_user and test course, to enable the @@ -209,6 +233,26 @@ class StaffMasqueradeTestCase(MasqueradeTestCase): return StaffFactory(course_key=self.course.id) +class TestMasqueradeOptionsForNoPartitions(MasqueradeTestCase): + """ + Check that 'Learner' option is available if there are no groups or partitions + """ + @patch.dict('django.conf.settings.FEATURES', {'ENABLE_MASQUERADE': True}) + @patch.dict('django.conf.settings.FEATURES', {'ENABLE_ENROLLMENT_TRACK_USER_PARTITION': False}) + def test_masquerade_options_enrollment_track(self): + self.verify_learner_masquerade_available(True) + + +class TestMasqueradeOptionsForUserPartition(MasqueradeTestCase): + """ + Check that 'Learner' option is NOT available if there are no groups or partitions + """ + @ patch.dict('django.conf.settings.FEATURES', {'ENABLE_ENROLLMENT_TRACK_USER_PARTITION': True}) + @ patch.dict('django.conf.settings.FEATURES', {'ENABLE_MASQUERADE': True}) + def test_masquerade_options_no_cohort(self): + self.verify_learner_masquerade_available(False) + + class TestStaffMasqueradeAsStudent(StaffMasqueradeTestCase): """ Check for staff being able to masquerade as student. From fc8b0a4d6021b2943387db9b59e338761368255f Mon Sep 17 00:00:00 2001 From: cdeery Date: Tue, 3 Aug 2021 15:12:03 -0400 Subject: [PATCH 2/5] fix the logic of the validation in the unt tests so it was correct in all cases --- lms/djangoapps/courseware/tests/test_masquerade.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index 9b2c7f0a65..95578b3974 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -10,6 +10,7 @@ from importlib import import_module from unittest.mock import patch import pytest import ddt +from operator import itemgetter from django.conf import settings from django.test import TestCase, RequestFactory from django.urls import reverse @@ -171,6 +172,7 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas assert self.problem_display_name in problem_html assert show_answer_expected == ('Show answer' in problem_html) + def verify_learner_masquerade_available(self, learner_option_expected): """ Verifies if learner masquerade option is available @@ -178,10 +180,9 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas exists: True to test if Learner is in options, False to test it is NOT """ response = self.get_available_masquerade_identities() - items = response.content.json['available'].items() - is_it_there = (('name', 'Learner') in items) + items = response.json()['available'] + is_it_there = 'Learner' in map(itemgetter('name'), items) assert learner_option_expected == is_it_there - assert learner_option_expected == (('name', 'Learner') in content.items()) def ensure_masquerade_as_group_member(self, partition_id, group_id): """ @@ -233,7 +234,7 @@ class StaffMasqueradeTestCase(MasqueradeTestCase): return StaffFactory(course_key=self.course.id) -class TestMasqueradeOptionsForNoPartitions(MasqueradeTestCase): +class TestMasqueradeOptionsForNoPartitions(StaffMasqueradeTestCase): """ Check that 'Learner' option is available if there are no groups or partitions """ @@ -243,7 +244,7 @@ class TestMasqueradeOptionsForNoPartitions(MasqueradeTestCase): self.verify_learner_masquerade_available(True) -class TestMasqueradeOptionsForUserPartition(MasqueradeTestCase): +class TestMasqueradeOptionsForUserPartition(StaffMasqueradeTestCase): """ Check that 'Learner' option is NOT available if there are no groups or partitions """ From 35caeb40364cad4fdfa1e77c02243ae68db42ada Mon Sep 17 00:00:00 2001 From: cdeery Date: Tue, 3 Aug 2021 17:16:02 -0400 Subject: [PATCH 3/5] Clean up to pass pep-8 --- lms/djangoapps/courseware/masquerade.py | 11 ++++++----- lms/djangoapps/courseware/tests/test_masquerade.py | 1 - 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lms/djangoapps/courseware/masquerade.py b/lms/djangoapps/courseware/masquerade.py index 33485c5386..1be4d75c0a 100644 --- a/lms/djangoapps/courseware/masquerade.py +++ b/lms/djangoapps/courseware/masquerade.py @@ -4,7 +4,6 @@ Allow course staff to see a student or staff view of courseware. Which kind of view has been selected is stored in the session state. ''' - import logging from datetime import datetime @@ -49,6 +48,7 @@ class CourseMasquerade: """ Masquerade settings for a particular course. """ + def __init__(self, course_key, role='student', user_partition_id=None, group_id=None, user_name=None): # All parameters to this function must be named identically to the corresponding attribute. # If you remove or rename an attribute, also update the __setstate__() method to migrate @@ -131,7 +131,7 @@ class MasqueradeView(View): ], } if len(partitions) == 0: - data['available'].append( { + data['available'].append({ 'name': 'Learner', 'role': 'student', }) @@ -211,9 +211,9 @@ def setup_masquerade(request, course_key, staff_access=False, reset_masquerade_d If the reset_masquerade_data flag is set, the field data stored in the session will be cleared. """ if ( - request.user is None or - not settings.FEATURES.get('ENABLE_MASQUERADE', False) or - not staff_access + request.user is None or + not settings.FEATURES.get('ENABLE_MASQUERADE', False) or + not staff_access ): return None, request.user if reset_masquerade_data: @@ -414,6 +414,7 @@ class MasqueradingKeyValueStore(KeyValueStore): This `KeyValueStore` wraps an underlying `KeyValueStore`. Reads are forwarded to the underlying store, but writes go to a Django session (or other dictionary-like object). """ + def __init__(self, kvs, session): # lint-amnesty, pylint: disable=super-init-not-called """ Arguments: diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index 95578b3974..c0b730f034 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -172,7 +172,6 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas assert self.problem_display_name in problem_html assert show_answer_expected == ('Show answer' in problem_html) - def verify_learner_masquerade_available(self, learner_option_expected): """ Verifies if learner masquerade option is available From eafea504aca2218da0ad82e9077ca1ba006b1ed3 Mon Sep 17 00:00:00 2001 From: cdeery Date: Thu, 5 Aug 2021 12:35:54 -0400 Subject: [PATCH 4/5] fix: changes suggested in code review Cleaned up the nits. --- lms/djangoapps/courseware/tests/test_masquerade.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index c0b730f034..42568e6af9 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -176,7 +176,7 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas """ Verifies if learner masquerade option is available Args: - exists: True to test if Learner is in options, False to test it is NOT + learner_option_expected: True to test if Learner is in options, False to test it is NOT """ response = self.get_available_masquerade_identities() items = response.json()['available'] @@ -247,8 +247,8 @@ class TestMasqueradeOptionsForUserPartition(StaffMasqueradeTestCase): """ Check that 'Learner' option is NOT available if there are no groups or partitions """ - @ patch.dict('django.conf.settings.FEATURES', {'ENABLE_ENROLLMENT_TRACK_USER_PARTITION': True}) @ patch.dict('django.conf.settings.FEATURES', {'ENABLE_MASQUERADE': True}) + @ patch.dict('django.conf.settings.FEATURES', {'ENABLE_ENROLLMENT_TRACK_USER_PARTITION': True}) def test_masquerade_options_no_cohort(self): self.verify_learner_masquerade_available(False) From b4b461b698ca01ba98c08bed0d5be4ee9d69601b Mon Sep 17 00:00:00 2001 From: cdeery Date: Thu, 5 Aug 2021 13:40:03 -0400 Subject: [PATCH 5/5] fix: collapse unit tests by using ddt --- .../courseware/tests/test_masquerade.py | 53 +++++++++---------- 1 file changed, 25 insertions(+), 28 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_masquerade.py b/lms/djangoapps/courseware/tests/test_masquerade.py index 42568e6af9..a625e6e173 100644 --- a/lms/djangoapps/courseware/tests/test_masquerade.py +++ b/lms/djangoapps/courseware/tests/test_masquerade.py @@ -2,7 +2,6 @@ Unit tests for masquerade. """ - import json import pickle from datetime import datetime @@ -45,6 +44,7 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas """ Base class for masquerade tests that sets up a test course and enrolls a user in the course. """ + @classmethod def setUpClass(cls): super().setUpClass() @@ -172,17 +172,6 @@ class MasqueradeTestCase(SharedModuleStoreTestCase, LoginEnrollmentTestCase, Mas assert self.problem_display_name in problem_html assert show_answer_expected == ('Show answer' in problem_html) - def verify_learner_masquerade_available(self, learner_option_expected): - """ - Verifies if learner masquerade option is available - Args: - learner_option_expected: True to test if Learner is in options, False to test it is NOT - """ - response = self.get_available_masquerade_identities() - items = response.json()['available'] - is_it_there = 'Learner' in map(itemgetter('name'), items) - assert learner_option_expected == is_it_there - def ensure_masquerade_as_group_member(self, partition_id, group_id): """ Installs a masquerade for the test_user and test course, to enable the @@ -201,6 +190,7 @@ class NormalStudentVisibilityTest(MasqueradeTestCase): """ Verify the course displays as expected for a "normal" student (to ensure test setup is correct). """ + def create_user(self): """ Creates a normal student user. @@ -226,6 +216,7 @@ class StaffMasqueradeTestCase(MasqueradeTestCase): """ Base class for tests of the masquerade behavior for a staff member. """ + def create_user(self): """ Creates a staff user. @@ -233,30 +224,30 @@ class StaffMasqueradeTestCase(MasqueradeTestCase): return StaffFactory(course_key=self.course.id) -class TestMasqueradeOptionsForNoPartitions(StaffMasqueradeTestCase): +@ddt.ddt +class TestMasqueradeLearnerOptions(StaffMasqueradeTestCase): """ - Check that 'Learner' option is available if there are no groups or partitions + Check that 'View as Learner' option is available only if there are NO groups or partitions """ + + @ddt.data(True, False) @patch.dict('django.conf.settings.FEATURES', {'ENABLE_MASQUERADE': True}) - @patch.dict('django.conf.settings.FEATURES', {'ENABLE_ENROLLMENT_TRACK_USER_PARTITION': False}) - def test_masquerade_options_enrollment_track(self): - self.verify_learner_masquerade_available(True) - - -class TestMasqueradeOptionsForUserPartition(StaffMasqueradeTestCase): - """ - Check that 'Learner' option is NOT available if there are no groups or partitions - """ - @ patch.dict('django.conf.settings.FEATURES', {'ENABLE_MASQUERADE': True}) - @ patch.dict('django.conf.settings.FEATURES', {'ENABLE_ENROLLMENT_TRACK_USER_PARTITION': True}) - def test_masquerade_options_no_cohort(self): - self.verify_learner_masquerade_available(False) + def test_masquerade_options_for_learner(self, partitions_enabled): + """ + If there are partitions, then the View as Learner should NOT be available + """ + with patch.dict('django.conf.settings.FEATURES', + {'ENABLE_ENROLLMENT_TRACK_USER_PARTITION': partitions_enabled}): + response = self.get_available_masquerade_identities() + is_learner_available = 'Learner' in map(itemgetter('name'), response.json()['available']) + assert partitions_enabled != is_learner_available class TestStaffMasqueradeAsStudent(StaffMasqueradeTestCase): """ Check for staff being able to masquerade as student. """ + @patch.dict('django.conf.settings.FEATURES', {'DISABLE_START_DATES': False}) def test_staff_debug_with_masquerade(self): """ @@ -295,6 +286,7 @@ class TestStaffMasqueradeAsSpecificStudent(StaffMasqueradeTestCase, ProblemSubmi """ Check for staff being able to masquerade as a specific student. """ + def setUp(self): super().setUp() self.student_user = self.create_user() @@ -481,6 +473,7 @@ class TestGetMasqueradingGroupId(StaffMasqueradeTestCase): """ Check for staff being able to masquerade as belonging to a group. """ + def setUp(self): super().setUp() self.user_partition = UserPartition( @@ -514,6 +507,7 @@ class ReadOnlyKeyValueStore(DictKeyValueStore): Used to make sure MasqueradingKeyValueStore does not try to modify the underlying KeyValueStore. """ + def set(self, key, value): assert False, "ReadOnlyKeyValueStore may not be modified." @@ -533,6 +527,7 @@ class MasqueradingKeyValueStoreTest(TestCase): """ Unit tests for the MasqueradingKeyValueStore class. """ + def setUp(self): super().setUp() self.ro_kvs = ReadOnlyKeyValueStore({'a': 42, 'b': None, 'c': 'OpenCraft'}) @@ -572,6 +567,7 @@ class CourseMasqueradeTest(TestCase): """ Unit tests for the CourseMasquerade class. """ + def test_unpickling_sets_all_attributes(self): """ Make sure that old CourseMasquerade objects receive missing attributes when unpickled from @@ -584,10 +580,11 @@ class CourseMasqueradeTest(TestCase): assert unpickled_cmasq.user_name is None -class SetupMasqueradeTests(SharedModuleStoreTestCase,): +class SetupMasqueradeTests(SharedModuleStoreTestCase, ): """ Tests for the setup_masquerade function. """ + def setUp(self): super().setUp() self.course = CourseFactory.create(number='setup-masquerade-test', metadata={'start': datetime.now(UTC)})