-
Notifications
You must be signed in to change notification settings - Fork 4.2k
REV-1564: add user-metadata API #25450
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
e11c820
466f57a
837bc4a
5b05a9b
ed8405b
13721ba
0bbb3a9
0e0c223
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,10 +8,13 @@ | |
| import six.moves.urllib.error | ||
| import six.moves.urllib.parse | ||
| import six.moves.urllib.request | ||
| from datetime import timedelta | ||
| from django.conf import settings | ||
| from django.core.handlers.wsgi import WSGIRequest | ||
| from django.utils.timezone import now | ||
| from django.test.utils import override_settings | ||
| from django.urls import reverse | ||
| from lms.djangoapps.course_blocks.transformers.tests.helpers import ModuleStoreTestCase | ||
| from mock import patch | ||
| from rest_framework.test import APITestCase | ||
|
|
||
|
|
@@ -20,10 +23,12 @@ | |
| from lms.djangoapps.experiments.serializers import ExperimentDataSerializer | ||
| from student.tests.factories import UserFactory | ||
|
|
||
| from xmodule.modulestore.tests.factories import CourseFactory | ||
|
|
||
| CROSS_DOMAIN_REFERER = 'https://ecommerce.edx.org' | ||
|
|
||
|
|
||
| class ExperimentDataViewSetTests(APITestCase): | ||
| class ExperimentDataViewSetTests(APITestCase, ModuleStoreTestCase): | ||
|
|
||
| def assert_data_created_for_user(self, user, method='post', status=201): | ||
| url = reverse('api_experiments:v0:data-list') | ||
|
|
@@ -288,3 +293,55 @@ def test_permissions(self): | |
|
|
||
| response = self.client.delete(url) | ||
| self.assertEqual(response.status_code, 403) | ||
|
|
||
|
|
||
| class ExperimentUserMetaDataViewTests(APITestCase, ModuleStoreTestCase): | ||
| """ Internal user_metadata view/API for use in Optimizely experiments """ | ||
|
|
||
| def test_UserMetaDataView_get_success_same_user(self): | ||
| """ Request succeeds when logged-in user makes request for self """ | ||
| lookup_user = UserFactory() | ||
| lookup_course = CourseFactory.create(start=now() - timedelta(days=30)) | ||
| call_args = [lookup_user.username, lookup_course.id] | ||
| self.client.login(username=lookup_user.username, password=UserFactory._DEFAULT_PASSWORD) | ||
|
|
||
| response = self.client.get(reverse('api_experiments:user_metadata', args=call_args)) | ||
| self.assertEqual(response.status_code, 200) | ||
|
|
||
| def test_UserMetaDataView_get_success_staff_user(self): | ||
| """ Request succeeds when logged-in staff user makes request for different user """ | ||
| lookup_user = UserFactory() | ||
| lookup_course = CourseFactory.create(start=now() - timedelta(days=30)) | ||
| call_args = [lookup_user.username, lookup_course.id] | ||
| staff_user = UserFactory(is_staff=True) | ||
|
|
||
| self.client.login(username=staff_user.username, password=UserFactory._DEFAULT_PASSWORD) | ||
|
|
||
| response = self.client.get(reverse('api_experiments:user_metadata', args=call_args)) | ||
| self.assertEqual(response.status_code, 200) | ||
dianekaplan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Worth asserting that all the top level keys are returned as well, to make sure it doesn't return a 200 + blank page?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure- added some assertions for the presence/values of the top items we expect |
||
| self.assertTrue(response.json()['course_id']) | ||
| self.assertTrue(response.json()['user_id']) | ||
| self.assertEqual(response.json()['username'], lookup_user.username) | ||
| self.assertEqual(response.json()['email'], lookup_user.email) | ||
|
|
||
| def test_UserMetaDataView_get_different_user(self): | ||
| """ Request fails when not logged in for requested user or staff """ | ||
| lookup_user = UserFactory() | ||
| lookup_course = CourseFactory.create(start=now() - timedelta(days=30)) | ||
| call_args = [lookup_user.username, lookup_course.id] | ||
|
|
||
| response = self.client.get(reverse('api_experiments:user_metadata', args=call_args)) | ||
| self.assertEqual(response.status_code, 401) | ||
|
|
||
| def test_UserMetaDataView_get_missing_course(self): | ||
| """ Request fails when not course not found """ | ||
| lookup_user = UserFactory() | ||
| lookup_course = CourseFactory.create(start=now() - timedelta(days=30)) | ||
| call_args = [lookup_user.username, lookup_course.id] | ||
| self.client.login(username=lookup_user.username, password=UserFactory._DEFAULT_PASSWORD) | ||
| bogus_course_name = str(lookup_course.id) + '_FOOBAR' | ||
|
|
||
| call_args_with_bogus_course = [lookup_user.username, bogus_course_name] | ||
| response = self.client.get(reverse('api_experiments:user_metadata', args=call_args_with_bogus_course)) | ||
| self.assertEqual(response.status_code, 404) | ||
| self.assertEqual(response.json()['message'], 'Provided course is not found') | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -264,46 +264,119 @@ def get_dashboard_course_info(user, dashboard_enrollments): | |
|
|
||
| def get_experiment_user_metadata_context(course, user): | ||
| """ | ||
| Return a context dictionary with the keys used by the user_metadata.html. | ||
| Return a context dictionary with the keys used for Optimizely experiments, exposed via user_metadata.html: | ||
| view from the DOM in those calling views using: JSON.parse($("#user-metadata").text()); | ||
| Most views call this function with both parameters, but student dashboard has only a user | ||
| """ | ||
| enrollment = None | ||
| # TODO: clean up as part of REVO-28 (START) | ||
| user_enrollments = None | ||
| audit_enrollments = None | ||
| has_non_audit_enrollments = False | ||
| try: | ||
| user_enrollments = CourseEnrollment.objects.select_related('course', 'schedule').filter(user_id=user.id) | ||
| has_non_audit_enrollments = user_enrollments.exclude(mode__in=CourseMode.UPSELL_TO_VERIFIED_MODES).exists() | ||
| context = {} | ||
| if course is not None: | ||
| try: | ||
| user_enrollments = CourseEnrollment.objects.select_related('course', 'schedule').filter(user_id=user.id) | ||
| has_non_audit_enrollments = user_enrollments.exclude(mode__in=CourseMode.UPSELL_TO_VERIFIED_MODES).exists() | ||
| # TODO: clean up as part of REVO-28 (END) | ||
| enrollment = CourseEnrollment.objects.select_related( | ||
| 'course', 'schedule' | ||
| ).get(user_id=user.id, course_id=course.id) | ||
| except CourseEnrollment.DoesNotExist: | ||
| pass # Not enrolled, use the default values | ||
|
|
||
| has_entitlements = False | ||
| if user.is_authenticated: | ||
| has_entitlements = CourseEntitlement.objects.filter(user=user).exists() | ||
|
|
||
| context = get_base_experiment_metadata_context(course, user, enrollment, user_enrollments) | ||
| has_staff_access = has_staff_access_to_preview_mode(user, course.id) | ||
| forum_roles = [] | ||
| if user.is_authenticated: | ||
| forum_roles = list(Role.objects.filter(users=user, course_id=course.id).values_list('name').distinct()) | ||
|
|
||
| # get user partition data | ||
| if user.is_authenticated: | ||
| partition_groups = get_all_partitions_for_course(course) | ||
| user_partitions = get_user_partition_groups(course.id, partition_groups, user, 'name') | ||
| else: | ||
| user_partitions = {} | ||
|
|
||
| # TODO: clean up as part of REVO-28 (START) | ||
| context['has_non_audit_enrollments'] = has_non_audit_enrollments or has_entitlements | ||
| # TODO: clean up as part of REVO-28 (END) | ||
| enrollment = CourseEnrollment.objects.select_related( | ||
| 'course', 'schedule' | ||
| ).get(user_id=user.id, course_id=course.id) | ||
| except CourseEnrollment.DoesNotExist: | ||
| pass # Not enrolled, use the default values | ||
|
|
||
| has_entitlements = False | ||
| if user.is_authenticated: | ||
| has_entitlements = CourseEntitlement.objects.filter(user=user).exists() | ||
|
|
||
| context = get_base_experiment_metadata_context(course, user, enrollment, user_enrollments) | ||
| has_staff_access = has_staff_access_to_preview_mode(user, course.id) | ||
| forum_roles = [] | ||
| if user.is_authenticated: | ||
| forum_roles = list(Role.objects.filter(users=user, course_id=course.id).values_list('name').distinct()) | ||
|
|
||
| # get user partition data | ||
| if user.is_authenticated: | ||
| partition_groups = get_all_partitions_for_course(course) | ||
| user_partitions = get_user_partition_groups(course.id, partition_groups, user, 'name') | ||
| else: | ||
| user_partitions = {} | ||
| context['has_staff_access'] = has_staff_access | ||
| context['forum_roles'] = forum_roles | ||
| context['partition_groups'] = user_partitions | ||
|
|
||
dianekaplan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| user_metadata = { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it possible to break this function into smaller functions to make it easier to follow?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes- there is a downside though. Background context: the old code had three different code blocks involved in gathering the user-metadata:
This separation also resulted in a bit of an inconsistency: most views updated their context from #1, but the dashboard view was only using #3. When I first ported over this code, I did add it as a separate function that the original one could call, to update the context with the user_metadata item. One clunky side-effect was that we had to add several more arguments: instead of being able to use course and user, we also needed to explicitly pass course_id and course_key. (There were apparently some mixins helping to access those fields on user_metadata.html, and they're available in get_experiment_user_metadata_context, but when you break out into a new function they were no longer defined). But worse than that, it felt differently clunky/confusing to have yet another function helping with the same purpose of just gathering/setting the user-metadata. Especially given the inconsistency and confusion noted above, I think it's better to try and have this logic together in one place. The existing code is visually long, so I could see one day looking to consolidate/separate parts of it, but the purpose of this ticket is to use the existing code to recreate the user-metadata as an endpoint, so I think that sort of cleanup would be beyond the scope of this ticket, and would add change where this is currently working the way we want. |
||
| key: context.get(key) | ||
| for key in ( | ||
| 'username', | ||
| 'user_id', | ||
| 'course_id', | ||
| 'enrollment_mode', | ||
| 'upgrade_link', | ||
| 'upgrade_price', | ||
| 'audit_access_deadline', | ||
| 'course_duration', | ||
| 'pacing_type', | ||
| 'has_staff_access', | ||
| 'forum_roles', | ||
| 'partition_groups', | ||
| # TODO: clean up as part of REVO-28 (START) | ||
| 'has_non_audit_enrollments', | ||
| # TODO: clean up as part of REVO-28 (END) | ||
| # TODO: clean up as part of REVEM-199 (START) | ||
| 'program_key_fields', | ||
| # TODO: clean up as part of REVEM-199 (END) | ||
| ) | ||
dianekaplan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| # TODO: clean up as part of REVO-28 (START) | ||
| context['has_non_audit_enrollments'] = has_non_audit_enrollments or has_entitlements | ||
| # TODO: clean up as part of REVO-28 (END) | ||
| context['has_staff_access'] = has_staff_access | ||
| context['forum_roles'] = forum_roles | ||
| context['partition_groups'] = user_partitions | ||
| if user: | ||
| user_metadata['username'] = user.username | ||
dianekaplan marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
| user_metadata['user_id'] = user.id | ||
| if hasattr(user, 'email'): | ||
| user_metadata['email'] = user.email | ||
|
|
||
| for datekey in ( | ||
| 'schedule_start', | ||
| 'enrollment_time', | ||
| 'course_start', | ||
| 'course_end', | ||
| 'dynamic_upgrade_deadline', | ||
| 'course_upgrade_deadline', | ||
| 'audit_access_deadline', | ||
| ): | ||
| user_metadata[datekey] = ( | ||
| context.get(datekey).isoformat() if context.get(datekey) else None | ||
| ) | ||
|
|
||
| for timedeltakey in ( | ||
| 'course_duration', | ||
| ): | ||
| user_metadata[timedeltakey] = ( | ||
| context.get(timedeltakey).total_seconds() if context.get(timedeltakey) else None | ||
| ) | ||
|
|
||
| course_key = context.get('course_key') | ||
| if course and not course_key: | ||
| course_key = course.id | ||
|
|
||
| if course_key: | ||
| if isinstance(course_key, CourseKey): | ||
| user_metadata['course_key_fields'] = { | ||
| 'org': course_key.org, | ||
| 'course': course_key.course, | ||
| 'run': course_key.run, | ||
| } | ||
|
|
||
| if not context.get('course_id'): | ||
| user_metadata['course_id'] = six.text_type(course_key) | ||
| elif isinstance(course_key, six.string_types): | ||
| user_metadata['course_id'] = course_key | ||
|
|
||
| context['user_metadata'] = user_metadata | ||
| return context | ||
|
|
||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need staff access if Optimizely makes the request on behalf of the user and any user can access their own data via the new endpoint, including staff users?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimizely itself won't need this, but my thinking is:
(a) this is helpful for testing and troubleshooting if we want to call directly to look at results for different users
(b) the neighboring permission class also is used by either a regular user (with one level of access) or admins (with increased access)