From 450072582e295dcdd31a358e76c898492344a392 Mon Sep 17 00:00:00 2001 From: Michael Terry Date: Mon, 8 Jun 2020 13:59:56 -0400 Subject: [PATCH] AA-137: Support courseware celebrations - Add a new CourseEnrollmentCelebration model, which ties a course enrollment to some booleans about progress celebrations - Add serialization of the new model to the existing courseware_api app's existing course info view - Add new API in courseware_api to update a celebration model --- common/djangoapps/student/admin.py | 95 ++++++++++++------- common/djangoapps/student/apps.py | 8 +- common/djangoapps/student/helpers.py | 1 + .../0034_courseenrollmentcelebration.py | 29 ++++++ common/djangoapps/student/models.py | 37 +++++++- .../djangoapps/student/signals/receivers.py | 39 +++++++- common/djangoapps/student/tests/factories.py | 8 ++ .../student/tests/test_receivers.py | 34 +++++++ lms/djangoapps/experiments/flags.py | 2 +- .../djangoapps/courseware_api/serializers.py | 1 + .../courseware_api/tests/test_views.py | 55 ++++++++++- .../core/djangoapps/courseware_api/urls.py | 3 + .../core/djangoapps/courseware_api/views.py | 72 +++++++++++++- 13 files changed, 332 insertions(+), 52 deletions(-) create mode 100644 common/djangoapps/student/migrations/0034_courseenrollmentcelebration.py create mode 100644 common/djangoapps/student/tests/test_receivers.py diff --git a/common/djangoapps/student/admin.py b/common/djangoapps/student/admin.py index 0801426cb0..5977062a1d 100644 --- a/common/djangoapps/student/admin.py +++ b/common/djangoapps/student/admin.py @@ -31,6 +31,7 @@ from student.models import ( CourseAccessRole, CourseEnrollment, CourseEnrollmentAllowed, + CourseEnrollmentCelebration, DashboardConfiguration, LinkedInAddToProfileConfiguration, LoginFailures, @@ -80,6 +81,44 @@ class _Check(object): return inner +class DisableEnrollmentAdminMixin: + """ Disables admin access to an admin page that scales with enrollments, as performance is poor at that size. """ + @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) + def has_view_permission(self, request, obj=None): + """ + Returns True if CourseEnrollment objects can be viewed via the admin view. + """ + return super().has_view_permission(request, obj) + + @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) + def has_add_permission(self, request): + """ + Returns True if CourseEnrollment objects can be added via the admin view. + """ + return super().has_add_permission(request) + + @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) + def has_change_permission(self, request, obj=None): + """ + Returns True if CourseEnrollment objects can be modified via the admin view. + """ + return super().has_change_permission(request, obj) + + @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) + def has_delete_permission(self, request, obj=None): + """ + Returns True if CourseEnrollment objects can be deleted via the admin view. + """ + return super().has_delete_permission(request, obj) + + @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) + def has_module_permission(self, request): + """ + Returns True if links to the CourseEnrollment admin view can be displayed. + """ + return super().has_module_permission(request) + + class CourseAccessRoleForm(forms.ModelForm): """Form for adding new Course Access Roles view the Django Admin Panel.""" @@ -238,7 +277,7 @@ class CourseEnrollmentForm(forms.ModelForm): @admin.register(CourseEnrollment) -class CourseEnrollmentAdmin(admin.ModelAdmin): +class CourseEnrollmentAdmin(DisableEnrollmentAdminMixin, admin.ModelAdmin): """ Admin interface for the CourseEnrollment model. """ list_display = ('id', 'course_id', 'mode', 'user', 'is_active',) list_filter = ('mode', 'is_active',) @@ -264,41 +303,6 @@ class CourseEnrollmentAdmin(admin.ModelAdmin): def queryset(self, request): return super(CourseEnrollmentAdmin, self).queryset(request).select_related('user') - @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) - def has_view_permission(self, request, obj=None): - """ - Returns True if CourseEnrollment objects can be viewed via the admin view. - """ - return super(CourseEnrollmentAdmin, self).has_view_permission(request, obj) - - @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) - def has_add_permission(self, request): - """ - Returns True if CourseEnrollment objects can be added via the admin view. - """ - return super(CourseEnrollmentAdmin, self).has_add_permission(request) - - @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) - def has_change_permission(self, request, obj=None): - """ - Returns True if CourseEnrollment objects can be modified via the admin view. - """ - return super(CourseEnrollmentAdmin, self).has_change_permission(request, obj) - - @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) - def has_delete_permission(self, request, obj=None): - """ - Returns True if CourseEnrollment objects can be deleted via the admin view. - """ - return super(CourseEnrollmentAdmin, self).has_delete_permission(request, obj) - - @_Check.is_enabled(COURSE_ENROLLMENT_ADMIN_SWITCH.is_enabled) - def has_module_permission(self, request): - """ - Returns True if links to the CourseEnrollment admin view can be displayed. - """ - return super(CourseEnrollmentAdmin, self).has_module_permission(request) - class UserProfileInline(admin.StackedInline): """ Inline admin interface for UserProfile model. """ @@ -511,6 +515,25 @@ class AllowedAuthUserAdmin(admin.ModelAdmin): model = AllowedAuthUser +@admin.register(CourseEnrollmentCelebration) +class CourseEnrollmentCelebrationAdmin(DisableEnrollmentAdminMixin, admin.ModelAdmin): + """Admin interface for the CourseEnrollmentCelebration model. """ + raw_id_fields = ('enrollment',) + list_display = ('id', 'course', 'user', 'celebrate_first_section') + search_fields = ('enrollment__course__id', 'enrollment__user__username') + + class Meta(object): + model = CourseEnrollmentCelebration + + def course(self, obj): + return obj.enrollment.course.id + course.short_description = 'Course' + + def user(self, obj): + return obj.enrollment.user.username + user.short_description = 'User' + + admin.site.register(UserTestGroup) admin.site.register(Registration) admin.site.register(PendingNameChange) diff --git a/common/djangoapps/student/apps.py b/common/djangoapps/student/apps.py index ff728cf38a..83287fc4dd 100644 --- a/common/djangoapps/student/apps.py +++ b/common/djangoapps/student/apps.py @@ -6,8 +6,6 @@ Configuration for the ``student`` Django application. import os from django.apps import AppConfig -from django.contrib.auth.signals import user_logged_in -from django.db.models.signals import pre_save class StudentConfig(AppConfig): @@ -17,10 +15,8 @@ class StudentConfig(AppConfig): name = 'student' def ready(self): - - from django.contrib.auth.models import User - from .signals.receivers import on_user_updated - pre_save.connect(on_user_updated, sender=User) + # Connect signal handlers. + from .signals import receivers # pylint: disable=unused-import # The django-simple-history model on CourseEnrollment creates performance # problems in testing, we mock it here so that the mock impacts all tests. diff --git a/common/djangoapps/student/helpers.py b/common/djangoapps/student/helpers.py index 934096c129..a6abb093b9 100644 --- a/common/djangoapps/student/helpers.py +++ b/common/djangoapps/student/helpers.py @@ -61,6 +61,7 @@ DISABLE_UNENROLL_CERT_STATES = [ 'generating', 'downloadable', ] +EMAIL_EXISTS_MSG_FMT = _("An account with the Email '{email}' already exists.") USERNAME_EXISTS_MSG_FMT = _("An account with the Public Username '{username}' already exists.") diff --git a/common/djangoapps/student/migrations/0034_courseenrollmentcelebration.py b/common/djangoapps/student/migrations/0034_courseenrollmentcelebration.py new file mode 100644 index 0000000000..7bf6f3c857 --- /dev/null +++ b/common/djangoapps/student/migrations/0034_courseenrollmentcelebration.py @@ -0,0 +1,29 @@ +# Generated by Django 2.2.12 on 2020-06-08 15:12 + +from django.db import migrations, models +import django.db.models.deletion +import django.utils.timezone +import model_utils.fields + + +class Migration(migrations.Migration): + + dependencies = [ + ('student', '0033_userprofile_state'), + ] + + operations = [ + migrations.CreateModel( + name='CourseEnrollmentCelebration', + fields=[ + ('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')), + ('created', model_utils.fields.AutoCreatedField(default=django.utils.timezone.now, editable=False, verbose_name='created')), + ('modified', model_utils.fields.AutoLastModifiedField(default=django.utils.timezone.now, editable=False, verbose_name='modified')), + ('celebrate_first_section', models.BooleanField(default=False)), + ('enrollment', models.OneToOneField(on_delete=django.db.models.deletion.CASCADE, related_name='celebration', to='student.CourseEnrollment')), + ], + options={ + 'abstract': False, + }, + ), + ] diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index 231e224df5..9f6c01996e 100644 --- a/common/djangoapps/student/models.py +++ b/common/djangoapps/student/models.py @@ -13,7 +13,6 @@ file and check it in at the same time as your model changes. To do that, import hashlib -import inspect import json import logging import uuid @@ -3048,3 +3047,39 @@ class AccountRecoveryConfiguration(ConfigurationModel): first row being the header and columns will be as follows: \ username, email, new_email") ) + + +class CourseEnrollmentCelebration(TimeStampedModel): + """ + Keeps track of how we've celebrated a user's course progress. + + An example of a celebration is a dialog that pops up after you complete your first section + in a course saying "good job!". Just some positive feedback like that. (This specific example is + controlled by the celebrated_first_section field below.) + + In general, if a row does not exist for an enrollment, we don't want to show any celebrations. + We don't want to suddenly inject celebrations in the middle of a course, because they + might not make contextual sense and it's an inconsistent experience. The helper methods below + (starting with "should_") can help by looking up values with appropriate fallbacks. + + See the create_course_enrollment_celebration signal handler for how these get created. + + .. no_pii: + """ + enrollment = models.OneToOneField(CourseEnrollment, models.CASCADE, related_name='celebration') + celebrate_first_section = models.BooleanField(default=False) + + def __str__(self): + return ( + "[CourseEnrollmentCelebration] course: {}; user: {}; first_section: {}" + ).format(self.enrollment.course.id, self.enrollment.user.username, self.celebrate_first_section) + + @staticmethod + def should_celebrate_first_section(enrollment): + """ Returns the celebration value for first_section with appropriate fallback if it doesn't exist """ + if not enrollment: + return False + try: + return enrollment.celebration.celebrate_first_section + except CourseEnrollmentCelebration.DoesNotExist: + return False diff --git a/common/djangoapps/student/signals/receivers.py b/common/djangoapps/student/signals/receivers.py index 57e673ce4e..b6f0d60415 100644 --- a/common/djangoapps/student/signals/receivers.py +++ b/common/djangoapps/student/signals/receivers.py @@ -2,13 +2,20 @@ Signal receivers for the "student" application. """ +# pylint: disable=unused-argument from django.conf import settings +from django.contrib.auth import get_user_model +from django.db import IntegrityError +from django.db.models.signals import post_save, pre_save +from django.dispatch import receiver -from student.helpers import USERNAME_EXISTS_MSG_FMT, AccountValidationError -from student.models import is_email_retired, is_username_retired +from lms.djangoapps.courseware.toggles import REDIRECT_TO_COURSEWARE_MICROFRONTEND +from student.helpers import EMAIL_EXISTS_MSG_FMT, USERNAME_EXISTS_MSG_FMT, AccountValidationError +from student.models import CourseEnrollment, CourseEnrollmentCelebration, is_email_retired, is_username_retired +@receiver(pre_save, sender=get_user_model()) def on_user_updated(sender, instance, **kwargs): """ Check for retired usernames. @@ -34,6 +41,32 @@ def on_user_updated(sender, instance, **kwargs): # Check for a retired email. if is_email_retired(instance.email): raise AccountValidationError( - EMAIL_EXISTS_MSG_FMT.format(username=instance.email), + EMAIL_EXISTS_MSG_FMT.format(email=instance.email), field="email" ) + + +@receiver(post_save, sender=CourseEnrollment) +def create_course_enrollment_celebration(sender, instance, created, **kwargs): + """ + Creates celebration rows when enrollments are created + + This is how we distinguish between new enrollments that we want to celebrate and old ones + that existed before we introduced a given celebration. + """ + if not created: + return + + # The UI for celebrations is only supported on the MFE right now, so don't turn on + # celebrations unless this enrollment's course is MFE-enabled. + if not REDIRECT_TO_COURSEWARE_MICROFRONTEND.is_enabled(instance.course_id): + return + + try: + CourseEnrollmentCelebration.objects.create( + enrollment=instance, + celebrate_first_section=True, + ) + except IntegrityError: + # A celebration object was already created. Shouldn't happen, but ignore it if it does. + pass diff --git a/common/djangoapps/student/tests/factories.py b/common/djangoapps/student/tests/factories.py index 0274788602..51ab00825c 100644 --- a/common/djangoapps/student/tests/factories.py +++ b/common/djangoapps/student/tests/factories.py @@ -19,6 +19,7 @@ from student.models import ( CourseAccessRole, CourseEnrollment, CourseEnrollmentAllowed, + CourseEnrollmentCelebration, PendingEmailChange, Registration, User, @@ -165,6 +166,13 @@ class CourseEnrollmentFactory(DjangoModelFactory): return manager.create(*args, **kwargs) +class CourseEnrollmentCelebrationFactory(DjangoModelFactory): + class Meta: + model = CourseEnrollmentCelebration + + enrollment = factory.SubFactory(CourseEnrollmentFactory) + + class CourseAccessRoleFactory(DjangoModelFactory): class Meta(object): model = CourseAccessRole diff --git a/common/djangoapps/student/tests/test_receivers.py b/common/djangoapps/student/tests/test_receivers.py new file mode 100644 index 0000000000..07f65a8a03 --- /dev/null +++ b/common/djangoapps/student/tests/test_receivers.py @@ -0,0 +1,34 @@ +""" Tests for student signal receivers. """ + +from lms.djangoapps.courseware.toggles import REDIRECT_TO_COURSEWARE_MICROFRONTEND +from student.models import CourseEnrollmentCelebration +from student.tests.factories import CourseEnrollmentFactory +from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase + + +class ReceiversTest(SharedModuleStoreTestCase): + """ + Tests for dashboard utility functions + """ + @REDIRECT_TO_COURSEWARE_MICROFRONTEND.override(active=True) + def test_celebration_created(self): + """ Test that we make celebration objects when enrollments are created """ + self.assertEqual(CourseEnrollmentCelebration.objects.count(), 0) + + # Test initial creation upon an enrollment being made + enrollment = CourseEnrollmentFactory() + self.assertEqual(CourseEnrollmentCelebration.objects.count(), 1) + celebration = CourseEnrollmentCelebration.objects.get(enrollment=enrollment, celebrate_first_section=True) + + # Test nothing changes if we update that enrollment + celebration.celebrate_first_section = False + celebration.save() + enrollment.mode = 'test-mode' + enrollment.save() + self.assertEqual(CourseEnrollmentCelebration.objects.count(), 1) + CourseEnrollmentCelebration.objects.get(enrollment=enrollment, celebrate_first_section=False) + + def test_celebration_gated_by_waffle(self): + """ Test we don't make a celebration if the MFE redirect waffle flag is off """ + CourseEnrollmentFactory() + self.assertEqual(CourseEnrollmentCelebration.objects.count(), 0) diff --git a/lms/djangoapps/experiments/flags.py b/lms/djangoapps/experiments/flags.py index 254cb81bef..7b5a5ee923 100644 --- a/lms/djangoapps/experiments/flags.py +++ b/lms/djangoapps/experiments/flags.py @@ -124,7 +124,7 @@ class ExperimentWaffleFlag(CourseWaffleFlag): if not request: return 0 - if not request.user.id: + if not hasattr(request, 'user') or not request.user.id: # We need username for stable bucketing and id for tracking, so just skip anonymous (not-logged-in) users return 0 diff --git a/openedx/core/djangoapps/courseware_api/serializers.py b/openedx/core/djangoapps/courseware_api/serializers.py index 7f47c763b2..6d8209f5f9 100644 --- a/openedx/core/djangoapps/courseware_api/serializers.py +++ b/openedx/core/djangoapps/courseware_api/serializers.py @@ -87,6 +87,7 @@ class CourseInfoSerializer(serializers.Serializer): # pylint: disable=abstract- can_load_courseware = serializers.DictField() notes = serializers.DictField() marketing_url = serializers.CharField() + celebrations = serializers.DictField() def __init__(self, *args, **kwargs): """ diff --git a/openedx/core/djangoapps/courseware_api/tests/test_views.py b/openedx/core/djangoapps/courseware_api/tests/test_views.py index 5379464f59..5c8df000f2 100644 --- a/openedx/core/djangoapps/courseware_api/tests/test_views.py +++ b/openedx/core/djangoapps/courseware_api/tests/test_views.py @@ -11,8 +11,8 @@ from django.conf import settings from lms.djangoapps.courseware.access_utils import ACCESS_DENIED, ACCESS_GRANTED from lms.djangoapps.courseware.tabs import ExternalLinkCourseTab -from student.models import CourseEnrollment -from student.tests.factories import UserFactory +from student.models import CourseEnrollment, CourseEnrollmentCelebration +from student.tests.factories import CourseEnrollmentCelebrationFactory, UserFactory from xmodule.modulestore.django import modulestore from xmodule.modulestore.tests.django_utils import TEST_DATA_SPLIT_MODULESTORE, SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import ItemFactory, ToyCourseFactory @@ -156,3 +156,54 @@ class ResumeApiTestViews(BaseCoursewareTests, CompletionWaffleTestMixin): assert response.data['block_id'] == str(self.unit.location) assert response.data['unit_id'] == str(self.unit.location) assert response.data['section_id'] == str(self.sequence.location) + + +@ddt.ddt +class CelebrationApiTestViews(BaseCoursewareTests): + """ + Tests for the celebration API + """ + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.url = '/api/courseware/celebration/{}'.format(cls.course.id) + + def setUp(self): + super().setUp() + self.enrollment = CourseEnrollment.enroll(self.user, self.course.id, 'verified') + + @ddt.data(True, False) + def test_happy_path(self, update): + if update: + CourseEnrollmentCelebrationFactory(enrollment=self.enrollment, celebrate_first_section=False) + + response = self.client.post(self.url, {'first_section': True}, content_type='application/json') + assert response.status_code == (200 if update else 201) + + celebration = CourseEnrollmentCelebration.objects.first() + assert celebration.celebrate_first_section + assert celebration.enrollment.id == self.enrollment.id + + def test_extra_data(self): + response = self.client.post(self.url, {'extra': True}, content_type='application/json') + assert response.status_code == 400 + + def test_no_data(self): + response = self.client.post(self.url, {}, content_type='application/json') + assert response.status_code == 200 + assert CourseEnrollmentCelebration.objects.count() == 0 + + def test_no_enrollment(self): + self.enrollment.delete() + response = self.client.post(self.url, {'first_section': True}, content_type='application/json') + assert response.status_code == 404 + + def test_no_login(self): + self.client.logout() + response = self.client.post(self.url, {'first_section': True}, content_type='application/json') + assert response.status_code == 401 + + def test_invalid_course(self): + response = self.client.post('/api/courseware/celebration/course-v1:does+not+exist', + {'first_section': True}, content_type='application/json') + assert response.status_code == 404 diff --git a/openedx/core/djangoapps/courseware_api/urls.py b/openedx/core/djangoapps/courseware_api/urls.py index fd2c9fd031..6175da19a1 100644 --- a/openedx/core/djangoapps/courseware_api/urls.py +++ b/openedx/core/djangoapps/courseware_api/urls.py @@ -18,4 +18,7 @@ urlpatterns = [ url(r'^resume/{}'.format(settings.COURSE_KEY_PATTERN), views.Resume.as_view(), name="resume-api"), + url(r'^celebration/{}'.format(settings.COURSE_KEY_PATTERN), + views.Celebration.as_view(), + name="celebration-api"), ] diff --git a/openedx/core/djangoapps/courseware_api/views.py b/openedx/core/djangoapps/courseware_api/views.py index 5bad9197ed..3f35294b4c 100644 --- a/openedx/core/djangoapps/courseware_api/views.py +++ b/openedx/core/djangoapps/courseware_api/views.py @@ -29,7 +29,7 @@ from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin from openedx.features.content_type_gating.models import ContentTypeGatingConfig from openedx.features.course_duration_limits.access import generate_course_expired_message from openedx.features.discounts.utils import generate_offer_html -from student.models import CourseEnrollment +from student.models import CourseEnrollment, CourseEnrollmentCelebration from xmodule.modulestore.django import modulestore from xmodule.modulestore.search import path_to_location @@ -48,6 +48,8 @@ class CoursewareMeta: ) self.effective_user = self.overview.effective_user self.course_key = course_key + self.enrollment_object = CourseEnrollment.get_enrollment(self.effective_user, self.course_key, + select_related=['celebration']) def __getattr__(self, name): return getattr(self.overview, name) @@ -90,8 +92,7 @@ class CoursewareMeta: @property def can_show_upgrade_sock(self): - enrollment = CourseEnrollment.get_enrollment(self.effective_user, self.course_key) - can_show = can_show_verified_upgrade(self.effective_user, enrollment) + can_show = can_show_verified_upgrade(self.effective_user, self.enrollment_object) return can_show @property @@ -146,6 +147,15 @@ class CoursewareMeta: 'visible': self.overview.edxnotes_visibility, } + @property + def celebrations(self): + """ + Returns a list of celebrations that should be performed. + """ + return { + 'first_section': CourseEnrollmentCelebration.should_celebrate_first_section(self.enrollment_object), + } + class CoursewareInformation(RetrieveAPIView): """ @@ -338,3 +348,59 @@ class Resume(DeveloperErrorViewMixin, APIView): pass return Response(resp) + + +class Celebration(DeveloperErrorViewMixin, APIView): + """ + **Use Cases** + + Marks a particular celebration as complete + + **Example Requests** + + POST /api/courseware/celebration/{course_key} + + **Request Parameters** + + Body consists of the following fields: + + * first_section (bool): whether we should celebrate when a user finishes their first section of a course + + **Returns** + + * 200 or 201 on success with above fields. + * 400 if an invalid parameter was sent. + * 404 if the course is not available or cannot be seen. + """ + + authentication_classes = ( + JwtAuthentication, + SessionAuthenticationAllowInactiveUser, + ) + permission_classes = (IsAuthenticated, ) + http_method_names = ['post'] + + def post(self, request, course_key_string, *args, **kwargs): + """ + Handle a POST request. + """ + course_key = CourseKey.from_string(course_key_string) + + data = dict(request.data) + first_section = data.pop('first_section', None) + if data: + return Response(status=400) # there were parameters we didn't recognize + + enrollment = CourseEnrollment.get_enrollment(request.user, course_key) + if not enrollment: + return Response(status=404) + + defaults = {} + if first_section is not None: + defaults['celebrate_first_section'] = first_section + + if defaults: + _, created = CourseEnrollmentCelebration.objects.update_or_create(enrollment=enrollment, defaults=defaults) + return Response(status=201 if created else 200) + else: + return Response(status=200) # just silently allow it