Skip to content

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.

Sign up for GitHub

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

Merged
dianekaplan merged 8 commits into from
Nov 4, 2020
Merged

REV-1564: add user-metadata API #25450

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e11c820
ported logic from user_metadata.html to utils so it can be reused by …
Oct 27, 2020
466f57a
added url and endpoint view to get metadata
Oct 27, 2020
837bc4a
updated get_experiment_user_metadata_context to set a user_metadata p…
Oct 28, 2020
5b05a9b
added basic tests for get_experiment_user_metadata_context and the view
Oct 28, 2020
ed8405b
added handling/tests for requested course or user not being found
Oct 29, 2020
13721ba
Let dashboard call get_experiment_user_metadata_context with only a user
Oct 30, 2020
0bbb3a9
made new permission for user-level results, updated view to use it, u…
Oct 30, 2020
0e0c223
Updates for 11/2 feedback
Nov 2, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion common/djangoapps/student/views/dashboard.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
from entitlements.models import CourseEntitlement
from lms.djangoapps.commerce.utils import EcommerceService
from lms.djangoapps.courseware.access import has_access
from lms.djangoapps.experiments.utils import get_dashboard_course_info
from lms.djangoapps.experiments.utils import get_dashboard_course_info, get_experiment_user_metadata_context
from lms.djangoapps.verify_student.services import IDVerificationService
from openedx.core.djangoapps.catalog.utils import (
get_programs,
Expand Down Expand Up @@ -805,6 +805,13 @@ def student_dashboard(request):
)
context.update(context_from_plugins)

course = None
context.update(
get_experiment_user_metadata_context(
course,
user,
)
)
if ecommerce_service.is_enabled(request.user):
context.update({
'use_ecommerce_payment_flow': True,
Expand Down
10 changes: 10 additions & 0 deletions lms/djangoapps/experiments/permissions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,13 @@ def has_permission(self, request, view):
class IsStaffOrReadOnly(BasePermission):
def has_permission(self, request, view):
return request.user.is_staff or request.method in SAFE_METHODS


class IsStaffOrReadOnlyForSelf(BasePermission):
"""
Grants access to staff or to user reading info about their own user
Copy link
Contributor

@jhan217 jhan217 Nov 2, 2020

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?

Copy link
Contributor Author

@dianekaplan dianekaplan Nov 2, 2020 •
edited
Loading

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)

"""
def has_permission(self, request, view):
username = request.user.username
return request.user.is_staff or (request.method in SAFE_METHODS and (
username == request.parser_context['kwargs']['username']))
46 changes: 43 additions & 3 deletions lms/djangoapps/experiments/tests/test_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,26 @@
Tests of experiment functionality
"""


from datetime import timedelta
from decimal import Decimal
from django.utils.timezone import now
from unittest import TestCase

from opaque_keys.edx.keys import CourseKey

from lms.djangoapps.course_blocks.transformers.tests.helpers import ModuleStoreTestCase
from lms.djangoapps.courseware import courses
from lms.djangoapps.experiments.utils import (
get_course_entitlement_price_and_sku,
get_experiment_user_metadata_context,
get_program_price_and_skus,
get_unenrolled_courses,
is_enrolled_in_course_run
)
from student.tests.factories import CourseEnrollmentFactory, UserFactory
from xmodule.modulestore.tests.factories import CourseFactory


class ExperimentUtilsTests(TestCase):
class ExperimentUtilsTests(ModuleStoreTestCase, TestCase):
"""
Tests of experiment functionality
"""
Expand Down Expand Up @@ -116,3 +121,38 @@ def test_price_and_sku_from_multiple_courses(self):
self.assertEqual(2, len(skus))
self.assertIn(self.run_a_sku, skus)
self.assertIn(self.entitlement_a_sku, skus)

def test_get_experiment_user_metadata_context(self):
course = CourseFactory.create(start=now() - timedelta(days=30), pacing_type="instructor_paced", course_duration=None, upgrade_price='Free',
upgrade_link=None, enrollment_mode=None, audit_access_deadline=None, program_key_fields=None, schedule_start=None,
enrollment_time=None, dynamic_upgrade_deadline=None, course_upgrade_deadline=None, course_key_fields={'org': 'org.0', 'course': 'course_0', 'run': 'Run_0'})
user = UserFactory()
context = get_experiment_user_metadata_context(course, user)
CourseEnrollmentFactory(course_id=course.id, user=user)

user_metadata_expected_result = {'username': user.username,
'user_id': user.id,
'course_id': course.id,
'enrollment_mode': course.enrollment_mode,
'upgrade_link': course.upgrade_link,
'upgrade_price': course.upgrade_price,
'audit_access_deadline': course.audit_access_deadline,
'course_duration': course.course_duration,
'pacing_type': course.pacing_type,
'has_staff_access': False,
'forum_roles': [],
'partition_groups': {},
'has_non_audit_enrollments': False,
'program_key_fields': course.program_key_fields,
'email': user.email,
'schedule_start': course.schedule_start,
'enrollment_time': course.enrollment_time,
'course_start': course.start,
'course_end': course.end,
'dynamic_upgrade_deadline': course.dynamic_upgrade_deadline,
'course_upgrade_deadline': course.course_upgrade_deadline,
'course_key_fields': course.course_key_fields}

user_metadata = context.get('user_metadata')

self.assertTrue(user_metadata, user_metadata_expected_result)
59 changes: 58 additions & 1 deletion lms/djangoapps/experiments/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand All @@ -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')
Expand Down Expand Up @@ -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)
Copy link
Contributor

@jhan217 jhan217 Nov 2, 2020

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@dianekaplan dianekaplan Nov 2, 2020

Choose a reason for hiding this comment

The 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')
5 changes: 4 additions & 1 deletion lms/djangoapps/experiments/urls.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
Experimentation URLs
"""


from django.conf import settings
from django.conf.urls import include, url

from . import routers, views, views_custom
Expand All @@ -14,4 +14,7 @@
urlpatterns = [
url(r'^v0/custom/REV-934/', views_custom.Rev934.as_view(), name='rev_934'),
url(r'^v0/', include((router.urls, "api"), namespace='v0')),
url(r'^v0/custom/userMetadata/{username},{course_key}$'.format(
username=settings.USERNAME_PATTERN,
course_key=settings.COURSE_ID_PATTERN), views.UserMetaDataView.as_view(), name='user_metadata'),
]
137 changes: 105 additions & 32 deletions lms/djangoapps/experiments/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

user_metadata = {
Copy link
Contributor

@jhan217 jhan217 Nov 2, 2020

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

@dianekaplan dianekaplan Nov 2, 2020

Choose a reason for hiding this comment

The 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:

  1. get_experiment_user_metadata_context (which we'd originally thought would be the whole story)
  2. get_base_experiment_metadata_context, called by the function above, which then adds authentication/FBE info
  3. then this other logic living in user_metadata.html which gathers together the pieces we want to dump into the DOM

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)
)
}

# 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
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


Expand Down
Loading