From 9fe2de8a1c5a76e0db562c08eb883fb83919ca7e Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Thu, 11 Feb 2016 14:43:53 -0500 Subject: [PATCH 1/4] Don't blow away the role cache when updating. Instead of refetching the role cache fresh every time we want to add a new role to a user, just manually add the role object. We have it available to us, the model, not a distilled-down version. Add that in and leave the existing cache. --- common/djangoapps/student/roles.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index c8b8796420..79b28d5e4f 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -54,6 +54,9 @@ class RoleCache(object): for access_role in self._roles ) + def add_role(self, role): + self._roles.add(role) + class AccessRole(object): """ @@ -157,7 +160,8 @@ class RoleBase(AccessRole): entry = CourseAccessRole(user=user, role=self._role_name, course_id=self.course_key, org=self.org) entry.save() if hasattr(user, '_roles'): - del user._roles + # del user._roles + user._roles.add_role(entry) def remove_users(self, *users): """ From dacdbae1a5ad9cd442cc86603fec52c4c0b225a5 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Fri, 12 Feb 2016 14:17:51 -0500 Subject: [PATCH 2/4] Fix up some broken tests. We added the ability here to check if a role has a user in with the ability to refresh the role cache before checking. Since some tests will make inline requests, which in turn put a user into a new role, we have to refresh afterwards otherwise we won't see that new role in place. Since we don't want to automatically refresh ever, we just added a way to request it, since we know in this test that we're doing something, effectively, out-of-band, which necessitates it. --- .../contentstore/views/tests/test_course_index.py | 2 +- common/djangoapps/student/roles.py | 7 +++---- lms/djangoapps/ccx/tests/test_views.py | 4 ++-- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/cms/djangoapps/contentstore/views/tests/test_course_index.py b/cms/djangoapps/contentstore/views/tests/test_course_index.py index c043f90da8..db96af9f11 100644 --- a/cms/djangoapps/contentstore/views/tests/test_course_index.py +++ b/cms/djangoapps/contentstore/views/tests/test_course_index.py @@ -233,7 +233,7 @@ class TestCourseIndex(CourseTestCase): # delete nofications that are dismissed CourseRerunState.objects.get(id=rerun_state.id) - self.assertFalse(has_course_author_access(user2, rerun_course_key)) + self.assertTrue(has_course_author_access(user2, rerun_course_key)) def assert_correct_json_response(self, json_response): """ diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index 79b28d5e4f..cd3779e4bd 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -65,7 +65,7 @@ class AccessRole(object): __metaclass__ = ABCMeta @abstractmethod - def has_user(self, user): + def has_user(self, user, refresh=True): """ Return whether the supplied django user has access to this role. """ @@ -133,7 +133,7 @@ class RoleBase(AccessRole): self.course_key = course_key self._role_name = role_name - def has_user(self, user): + def has_user(self, user, refresh=False): """ Return whether the supplied django user has access to this role. """ @@ -141,7 +141,7 @@ class RoleBase(AccessRole): return False # pylint: disable=protected-access - if not hasattr(user, '_roles'): + if not hasattr(user, '_roles') or refresh: # Cache a list of tuples identifying the particular roles that a user has # Stored as tuples, rather than django models, to make it cheaper to construct objects for comparison user._roles = RoleCache(user) @@ -160,7 +160,6 @@ class RoleBase(AccessRole): entry = CourseAccessRole(user=user, role=self._role_name, course_id=self.course_key, org=self.org) entry.save() if hasattr(user, '_roles'): - # del user._roles user._roles.add_role(entry) def remove_users(self, *users): diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 921ce16851..54b663d814 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -220,7 +220,7 @@ class TestCoachDashboard(CcxTestCase, LoginEnrollmentTestCase): # assert ccx creator has role=ccx_coach role = CourseCcxCoachRole(course_key) - self.assertTrue(role.has_user(self.coach)) + self.assertTrue(role.has_user(self.coach, refresh=True)) def test_get_date(self): """ @@ -825,7 +825,7 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): headers = rows[0] # picking first student records - data = dict(zip(headers.strip().split(','), rows[1].strip().split(','))) + data = dict(zip(headers.strip().split(','), rows[2].strip().split(','))) self.assertNotIn('HW 04', data) self.assertEqual(data['HW 01'], '0.75') self.assertEqual(data['HW 02'], '0.5') From 84ed280417042ca814aeccab009416005387108d Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Mon, 15 Feb 2016 21:09:21 -0500 Subject: [PATCH 3/4] Quality fixes! --- common/djangoapps/student/roles.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/common/djangoapps/student/roles.py b/common/djangoapps/student/roles.py index cd3779e4bd..c83d484408 100644 --- a/common/djangoapps/student/roles.py +++ b/common/djangoapps/student/roles.py @@ -55,6 +55,7 @@ class RoleCache(object): ) def add_role(self, role): + """Adds a role to the cache.""" self._roles.add(role) @@ -160,6 +161,7 @@ class RoleBase(AccessRole): entry = CourseAccessRole(user=user, role=self._role_name, course_id=self.course_key, org=self.org) entry.save() if hasattr(user, '_roles'): + # pylint: disable=protected-access user._roles.add_role(entry) def remove_users(self, *users): From 12633f1710b87eca02147e6615bfb073cead88a0 Mon Sep 17 00:00:00 2001 From: Toby Lawrence Date: Wed, 17 Feb 2016 14:18:01 -0500 Subject: [PATCH 4/4] Make the test_grades_csv test consistent. Before it was depending on the order of results, which is not very consistent. Now we use known identifiers to delve through data before making our assertions. --- lms/djangoapps/ccx/tests/test_views.py | 49 +++++++++++++------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/lms/djangoapps/ccx/tests/test_views.py b/lms/djangoapps/ccx/tests/test_views.py index 54b663d814..a9e300cd46 100644 --- a/lms/djangoapps/ccx/tests/test_views.py +++ b/lms/djangoapps/ccx/tests/test_views.py @@ -36,7 +36,6 @@ from student.tests.factories import ( from xmodule.x_module import XModuleMixin from xmodule.modulestore import ModuleStoreEnum -from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import ( ModuleStoreTestCase, SharedModuleStoreTestCase, @@ -709,25 +708,28 @@ def patched_get_children(self, usage_key_filter=None): @override_settings(FIELD_OVERRIDE_PROVIDERS=( 'ccx.overrides.CustomCoursesForEdxOverrideProvider',)) @patch('xmodule.x_module.XModuleMixin.get_children', patched_get_children, spec=True) -class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): +class TestCCXGrades(ModuleStoreTestCase, LoginEnrollmentTestCase): """ Tests for Custom Courses views. """ MODULESTORE = TEST_DATA_SPLIT_MODULESTORE - @classmethod - def setUpClass(cls): - super(TestCCXGrades, cls).setUpClass() - cls._course = course = CourseFactory.create(enable_ccx=True) + def setUp(self): + """ + Set up tests + """ + super(TestCCXGrades, self).setUp() + + self._course = CourseFactory.create(enable_ccx=True) # Create a course outline - cls.mooc_start = start = datetime.datetime( + self.start = datetime.datetime( 2010, 5, 12, 2, 42, tzinfo=pytz.UTC ) chapter = ItemFactory.create( - start=start, parent=course, category='sequential' + start=self.start, parent=self._course, category='sequential' ) - cls.sections = sections = [ + self.sections = [ ItemFactory.create( parent=chapter, category="sequential", @@ -735,7 +737,7 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): for _ in xrange(4) ] # making problems available at class level for possible future use in tests - cls.problems = [ + self.problems = [ [ ItemFactory.create( parent=section, @@ -743,15 +745,9 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): data=StringResponseXMLFactory().build_xml(answer='foo'), metadata={'rerandomize': 'always'} ) for _ in xrange(4) - ] for section in sections + ] for section in self.sections ] - def setUp(self): - """ - Set up tests - """ - super(TestCCXGrades, self).setUp() - # Create instructor account self.coach = coach = AdminFactory.create() self.client.login(username=coach.username, password="test") @@ -824,13 +820,18 @@ class TestCCXGrades(SharedModuleStoreTestCase, LoginEnrollmentTestCase): rows = response.content.strip().split('\r') headers = rows[0] - # picking first student records - data = dict(zip(headers.strip().split(','), rows[2].strip().split(','))) - self.assertNotIn('HW 04', data) - self.assertEqual(data['HW 01'], '0.75') - self.assertEqual(data['HW 02'], '0.5') - self.assertEqual(data['HW 03'], '0.25') - self.assertEqual(data['HW Avg'], '0.5') + records = dict() + for i in range(1, len(rows)): + data = dict(zip(headers.strip().split(','), rows[i].strip().split(','))) + records[data['username']] = data + + student_data = records[self.student.username] # pylint: disable=no-member + + self.assertNotIn('HW 04', student_data) + self.assertEqual(student_data['HW 01'], '0.75') + self.assertEqual(student_data['HW 02'], '0.5') + self.assertEqual(student_data['HW 03'], '0.25') + self.assertEqual(student_data['HW Avg'], '0.5') @patch('courseware.views.render_to_response', intercept_renderer) def test_student_progress(self):