From d46f6369ea3a4deca174fbe1df1c5cf944f02569 Mon Sep 17 00:00:00 2001 From: Dillon Dumesnil Date: Tue, 11 May 2021 17:08:12 -0400 Subject: [PATCH] feat: AA-768: Add history to Course Goals model and update events Also removes the remove_course_goal method as it is no longer used anywhere and removes the functionality of deleting course goals if a user unenrolls. Adds in fields for eventing to make them more useful. --- lms/djangoapps/course_goals/api.py | 39 +++++-------------- lms/djangoapps/course_goals/handlers.py | 32 +++++++-------- .../migrations/0003_historicalcoursegoal.py | 38 ++++++++++++++++++ lms/djangoapps/course_goals/models.py | 8 ++-- lms/djangoapps/course_goals/tests/test_api.py | 33 +++++++++------- lms/djangoapps/course_goals/views.py | 22 +---------- .../tests/views/test_course_home.py | 10 ++--- .../course_experience/views/course_home.py | 26 ++++++------- 8 files changed, 106 insertions(+), 102 deletions(-) create mode 100644 lms/djangoapps/course_goals/migrations/0003_historicalcoursegoal.py diff --git a/lms/djangoapps/course_goals/api.py b/lms/djangoapps/course_goals/api.py index 07ce8f5161..dda86f3513 100644 --- a/lms/djangoapps/course_goals/api.py +++ b/lms/djangoapps/course_goals/api.py @@ -8,10 +8,9 @@ from opaque_keys.edx.keys import CourseKey from rest_framework.reverse import reverse from common.djangoapps.course_modes.models import CourseMode +from lms.djangoapps.course_goals.models import CourseGoal, GOAL_KEY_CHOICES from openedx.features.course_experience import ENABLE_COURSE_GOALS -from . import models - def add_course_goal(user, course_id, goal_key): """ @@ -22,18 +21,11 @@ def add_course_goal(user, course_id, goal_key): user: The user that is setting the goal course_id (string): The id for the course the goal refers to goal_key (string): The goal key for the new goal. - """ course_key = CourseKey.from_string(str(course_id)) - current_goal = get_course_goal(user, course_key) - if current_goal: - # If a course goal already exists, simply update it. - current_goal.goal_key = goal_key - current_goal.save(update_fields=['goal_key']) - else: - # Otherwise, create and save a new course goal. - new_goal = models.CourseGoal(user=user, course_key=course_key, goal_key=goal_key) - new_goal.save() + CourseGoal.objects.update_or_create( + user=user, course_key=course_key, defaults={'goal_key': goal_key} + ) def get_course_goal(user, course_key): @@ -45,18 +37,7 @@ def get_course_goal(user, course_key): if user.is_anonymous: return None - course_goals = models.CourseGoal.objects.filter(user=user, course_key=course_key) - return course_goals[0] if course_goals else None - - -def remove_course_goal(user, course_id): - """ - Given a user and a course_id, remove the course goal. - """ - course_key = CourseKey.from_string(course_id) - course_goal = get_course_goal(user, course_key) - if course_goal: - course_goal.delete() + return CourseGoal.objects.filter(user=user, course_key=course_key).first() def get_goal_api_url(request): @@ -84,7 +65,7 @@ def get_course_goal_options(): Returns the valid options for goal keys, mapped to their translated strings, as defined by theCourseGoal model. """ - return {goal_key: goal_text for goal_key, goal_text in models.GOAL_KEY_CHOICES} # lint-amnesty, pylint: disable=unnecessary-comprehension + return {goal_key: goal_text for goal_key, goal_text in GOAL_KEY_CHOICES} # lint-amnesty, pylint: disable=unnecessary-comprehension def get_course_goal_text(goal_key): @@ -105,11 +86,11 @@ def valid_course_goals_ordered(include_unsure=False): goal_options = get_course_goal_options() ordered_goal_options = [] - ordered_goal_options.append((models.GOAL_KEY_CHOICES.certify, goal_options[models.GOAL_KEY_CHOICES.certify])) - ordered_goal_options.append((models.GOAL_KEY_CHOICES.complete, goal_options[models.GOAL_KEY_CHOICES.complete])) - ordered_goal_options.append((models.GOAL_KEY_CHOICES.explore, goal_options[models.GOAL_KEY_CHOICES.explore])) + ordered_goal_options.append((GOAL_KEY_CHOICES.certify, goal_options[GOAL_KEY_CHOICES.certify])) + ordered_goal_options.append((GOAL_KEY_CHOICES.complete, goal_options[GOAL_KEY_CHOICES.complete])) + ordered_goal_options.append((GOAL_KEY_CHOICES.explore, goal_options[GOAL_KEY_CHOICES.explore])) if include_unsure: - ordered_goal_options.append((models.GOAL_KEY_CHOICES.unsure, goal_options[models.GOAL_KEY_CHOICES.unsure])) + ordered_goal_options.append((GOAL_KEY_CHOICES.unsure, goal_options[GOAL_KEY_CHOICES.unsure])) return ordered_goal_options diff --git a/lms/djangoapps/course_goals/handlers.py b/lms/djangoapps/course_goals/handlers.py index de5d64f609..9fa10f1b13 100644 --- a/lms/djangoapps/course_goals/handlers.py +++ b/lms/djangoapps/course_goals/handlers.py @@ -3,25 +3,21 @@ Signal handlers for course goals. """ -from django.db import models +from django.db.models.signals import post_save from django.dispatch import receiver +from eventtracking import tracker -from common.djangoapps.course_modes.models import CourseMode -from common.djangoapps.student.models import CourseEnrollment - -from .api import add_course_goal, remove_course_goal -from .models import GOAL_KEY_CHOICES +from common.djangoapps.track import segment +from lms.djangoapps.course_goals.models import CourseGoal -@receiver(models.signals.post_save, sender=CourseEnrollment, dispatch_uid="update_course_goal_on_enroll_change") -def update_course_goal_on_enroll_change(sender, instance, **kwargs): # pylint: disable=unused-argument - """ - Updates goals as follows on enrollment changes: - 1) Set the course goal to 'certify' when the user enrolls as a verified user. - 2) Remove the course goal when the user's enrollment is no longer active. - """ - course_id = str(instance.course_id) - if not instance.is_active: - remove_course_goal(instance.user, course_id) - elif instance.mode == CourseMode.VERIFIED: - add_course_goal(instance.user, course_id, GOAL_KEY_CHOICES.certify) +@receiver(post_save, sender=CourseGoal, dispatch_uid="emit_course_goals_event") +def emit_course_goal_event(sender, instance, **kwargs): # lint-amnesty, pylint: disable=unused-argument + """Emit events for both tracking logs and for Segment.""" + name = 'edx.course.goal.added' if kwargs.get('created', False) else 'edx.course.goal.updated' + properties = { + 'courserun_key': str(instance.course_key), + 'goal_key': instance.goal_key, + } + tracker.emit(name, properties) + segment.track(instance.user.id, name, properties) diff --git a/lms/djangoapps/course_goals/migrations/0003_historicalcoursegoal.py b/lms/djangoapps/course_goals/migrations/0003_historicalcoursegoal.py new file mode 100644 index 0000000000..868477feb7 --- /dev/null +++ b/lms/djangoapps/course_goals/migrations/0003_historicalcoursegoal.py @@ -0,0 +1,38 @@ +# Generated by Django 2.2.20 on 2021-05-14 16:57 + +from django.conf import settings +from django.db import migrations, models +import django.db.models.deletion +import opaque_keys.edx.django.models +import simple_history.models + + +class Migration(migrations.Migration): + + dependencies = [ + migrations.swappable_dependency(settings.AUTH_USER_MODEL), + ('course_goals', '0002_auto_20171010_1129'), + ] + + operations = [ + migrations.CreateModel( + name='HistoricalCourseGoal', + fields=[ + ('id', models.IntegerField(auto_created=True, blank=True, db_index=True, verbose_name='ID')), + ('course_key', opaque_keys.edx.django.models.CourseKeyField(db_index=True, max_length=255)), + ('goal_key', models.CharField(choices=[('certify', 'Earn a certificate'), ('complete', 'Complete the course'), ('explore', 'Explore the course'), ('unsure', 'Not sure yet')], default='unsure', max_length=100)), + ('history_id', models.AutoField(primary_key=True, serialize=False)), + ('history_date', models.DateTimeField()), + ('history_change_reason', models.CharField(max_length=100, null=True)), + ('history_type', models.CharField(choices=[('+', 'Created'), ('~', 'Changed'), ('-', 'Deleted')], max_length=1)), + ('history_user', models.ForeignKey(null=True, on_delete=django.db.models.deletion.SET_NULL, related_name='+', to=settings.AUTH_USER_MODEL)), + ('user', models.ForeignKey(blank=True, db_constraint=False, null=True, on_delete=django.db.models.deletion.DO_NOTHING, related_name='+', to=settings.AUTH_USER_MODEL)), + ], + options={ + 'verbose_name': 'historical course goal', + 'ordering': ('-history_date', '-history_id'), + 'get_latest_by': 'history_date', + }, + bases=(simple_history.models.HistoricalChanges, models.Model), + ), + ] diff --git a/lms/djangoapps/course_goals/models.py b/lms/djangoapps/course_goals/models.py index 414d6ee98e..076d3dbb5f 100644 --- a/lms/djangoapps/course_goals/models.py +++ b/lms/djangoapps/course_goals/models.py @@ -3,12 +3,12 @@ Course Goals Models """ -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user +from django.contrib.auth import get_user_model from django.db import models -from django.utils.encoding import python_2_unicode_compatible from django.utils.translation import ugettext_lazy as _ from model_utils import Choices from opaque_keys.edx.django.models import CourseKeyField +from simple_history.models import HistoricalRecords # Each goal is represented by a goal key and a string description. GOAL_KEY_CHOICES = Choices( @@ -18,8 +18,9 @@ GOAL_KEY_CHOICES = Choices( ('unsure', _('Not sure yet')), ) +User = get_user_model() + -@python_2_unicode_compatible class CourseGoal(models.Model): """ Represents a course goal set by a user on the course home page. @@ -33,6 +34,7 @@ class CourseGoal(models.Model): user = models.ForeignKey(User, blank=False, on_delete=models.CASCADE) course_key = CourseKeyField(max_length=255, db_index=True) goal_key = models.CharField(max_length=100, choices=GOAL_KEY_CHOICES, default=GOAL_KEY_CHOICES.unsure) + history = HistoricalRecords() def __str__(self): return 'CourseGoal: {user} set goal to {goal} for course {course}'.format( diff --git a/lms/djangoapps/course_goals/tests/test_api.py b/lms/djangoapps/course_goals/tests/test_api.py index 00d4c4801a..1f7623411a 100644 --- a/lms/djangoapps/course_goals/tests/test_api.py +++ b/lms/djangoapps/course_goals/tests/test_api.py @@ -5,23 +5,23 @@ Unit tests for course_goals.api methods. from unittest import mock -from django.contrib.auth.models import User # lint-amnesty, pylint: disable=imported-auth-user +from django.contrib.auth import get_user_model from django.test.utils import override_settings from django.urls import reverse from rest_framework.test import APIClient from common.djangoapps.student.models import CourseEnrollment -from common.djangoapps.track.tests import EventTrackingTestCase from lms.djangoapps.course_goals.models import CourseGoal from xmodule.modulestore.tests.django_utils import SharedModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory -TEST_PASSWORD = 'test' EVENT_NAME_ADDED = 'edx.course.goal.added' EVENT_NAME_UPDATED = 'edx.course.goal.updated' +User = get_user_model() -class TestCourseGoalsAPI(EventTrackingTestCase, SharedModuleStoreTestCase): + +class TestCourseGoalsAPI(SharedModuleStoreTestCase): """ Testing the Course Goals API. """ @@ -40,13 +40,14 @@ class TestCourseGoalsAPI(EventTrackingTestCase, SharedModuleStoreTestCase): self.apiUrl = reverse('course_goals_api:v0:course_goal-list') - @mock.patch('lms.djangoapps.course_goals.views.segment.track') + @mock.patch('lms.djangoapps.course_goals.handlers.segment.track') @override_settings(LMS_SEGMENT_KEY="foobar") - def test_add_valid_goal(self, ga_call): + def test_add_valid_goal(self, segment_call): """ Ensures a correctly formatted post succeeds.""" response = self.post_course_goal(valid=True, goal_key='certify') - assert self.get_event(- 1)['name'] == EVENT_NAME_ADDED - ga_call.assert_called_with(self.user.id, EVENT_NAME_ADDED) + segment_call.assert_called_once_with(self.user.id, EVENT_NAME_ADDED, { + 'courserun_key': str(self.course.id), 'goal_key': 'certify' + }) assert response.status_code == 201 current_goals = CourseGoal.objects.filter(user=self.user, course_key=self.course.id) @@ -61,7 +62,6 @@ class TestCourseGoalsAPI(EventTrackingTestCase, SharedModuleStoreTestCase): def test_add_without_goal_key(self): """ Ensures if no goal key provided, post does not succeed. """ - response = self.post_course_goal(goal_key=None) assert len(CourseGoal.objects.filter(user=self.user, course_key=self.course.id)) == 0 self.assertContains( @@ -70,16 +70,23 @@ class TestCourseGoalsAPI(EventTrackingTestCase, SharedModuleStoreTestCase): status_code=400 ) - @mock.patch('lms.djangoapps.course_goals.views.segment.track') + @mock.patch('lms.djangoapps.course_goals.handlers.segment.track') @override_settings(LMS_SEGMENT_KEY="foobar") - def test_update_goal(self, ga_call): + def test_update_goal(self, segment_call): """ Ensures that repeated course goal post events do not create new instances of the goal. """ self.post_course_goal(valid=True, goal_key='explore') self.post_course_goal(valid=True, goal_key='certify') self.post_course_goal(valid=True, goal_key='unsure') - assert self.get_event(- 1)['name'] == EVENT_NAME_UPDATED - ga_call.assert_called_with(self.user.id, EVENT_NAME_UPDATED) + segment_call.assert_any_call(self.user.id, EVENT_NAME_ADDED, { + 'courserun_key': str(self.course.id), 'goal_key': 'explore' + }) + segment_call.assert_any_call(self.user.id, EVENT_NAME_UPDATED, { + 'courserun_key': str(self.course.id), 'goal_key': 'certify' + }) + segment_call.assert_any_call(self.user.id, EVENT_NAME_UPDATED, { + 'courserun_key': str(self.course.id), 'goal_key': 'unsure' + }) current_goals = CourseGoal.objects.filter(user=self.user, course_key=self.course.id) assert len(current_goals) == 1 assert current_goals[0].goal_key == 'unsure' diff --git a/lms/djangoapps/course_goals/views.py b/lms/djangoapps/course_goals/views.py index 81ef5f0680..e99e633a2a 100644 --- a/lms/djangoapps/course_goals/views.py +++ b/lms/djangoapps/course_goals/views.py @@ -4,22 +4,17 @@ Course Goals Views - includes REST API from django.contrib.auth import get_user_model -from django.db.models.signals import post_save -from django.dispatch import receiver from django.http import JsonResponse from edx_rest_framework_extensions.auth.jwt.authentication import JwtAuthentication -from eventtracking import tracker from opaque_keys.edx.keys import CourseKey from rest_framework import permissions, serializers, status, viewsets from rest_framework.authentication import SessionAuthentication from rest_framework.response import Response -from common.djangoapps.track import segment +from lms.djangoapps.course_goals.api import get_course_goal_options +from lms.djangoapps.course_goals.models import GOAL_KEY_CHOICES, CourseGoal from openedx.core.lib.api.permissions import IsStaffOrOwner -from .api import get_course_goal_options -from .models import GOAL_KEY_CHOICES, CourseGoal - User = get_user_model() @@ -106,16 +101,3 @@ class CourseGoalViewSet(viewsets.ModelViewSet): 'is_unsure': goal_key == GOAL_KEY_CHOICES.unsure, } return JsonResponse(data, content_type="application/json", status=(200 if goal else 201)) # lint-amnesty, pylint: disable=redundant-content-type-for-json-response - - -@receiver(post_save, sender=CourseGoal, dispatch_uid="emit_course_goals_event") -def emit_course_goal_event(sender, instance, **kwargs): # lint-amnesty, pylint: disable=unused-argument - """Emit events for both tracking logs and for Segment.""" - name = 'edx.course.goal.added' if kwargs.get('created', False) else 'edx.course.goal.updated' - tracker.emit( - name, - { - 'goal_key': instance.goal_key, - } - ) - segment.track(instance.user.id, name) diff --git a/openedx/features/course_experience/tests/views/test_course_home.py b/openedx/features/course_experience/tests/views/test_course_home.py index 84807b9667..18b9ce0620 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -29,7 +29,7 @@ from common.djangoapps.util.date_utils import strftime_localized_html from lms.djangoapps.experiments.models import ExperimentData from lms.djangoapps.commerce.models import CommerceConfiguration from lms.djangoapps.commerce.utils import EcommerceService -from lms.djangoapps.course_goals.api import add_course_goal, remove_course_goal +from lms.djangoapps.course_goals.api import add_course_goal, get_course_goal from lms.djangoapps.courseware.tests.helpers import get_expiration_banner_text from lms.djangoapps.courseware.utils import verified_upgrade_deadline_link from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory @@ -46,10 +46,13 @@ from openedx.core.djangolib.markup import HTML from openedx.features.course_duration_limits.models import CourseDurationLimitConfig from openedx.features.course_experience import ( COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, + COURSE_PRE_START_ACCESS_FLAG, DISABLE_UNIFIED_COURSE_TAB_FLAG, + ENABLE_COURSE_GOALS, SHOW_UPGRADE_MSG_ON_COURSE_HOME ) from openedx.features.course_experience.tests import BaseCourseUpdatesTestCase +from openedx.features.course_experience.tests.views.helpers import add_course_mode, remove_course_mode from openedx.features.discounts.applicability import get_discount_expiration_date from openedx.features.discounts.utils import REV1008_EXPERIMENT_ID, format_strikeout_price from common.djangoapps.student.models import CourseEnrollment, FBEEnrollmentExclusion @@ -60,9 +63,6 @@ from xmodule.modulestore import ModuleStoreEnum from xmodule.modulestore.tests.django_utils import CourseUserType, ModuleStoreTestCase from xmodule.modulestore.tests.factories import CourseFactory, ItemFactory, check_mongo_calls -from ... import COURSE_PRE_START_ACCESS_FLAG, ENABLE_COURSE_GOALS -from .helpers import add_course_mode, remove_course_mode - TEST_PASSWORD = 'test' TEST_CHAPTER_NAME = 'Test Chapter' TEST_COURSE_TOOLS = 'Course Tools' @@ -798,7 +798,7 @@ class TestCourseHomePageAccess(CourseHomePageTestCase): self.assertNotContains(response, TEST_COURSE_GOAL_OPTIONS) # Verify that enrolled and verified users are not shown the set course goal message. - remove_course_goal(user, str(verifiable_course.id)) + get_course_goal(user, verifiable_course.id).delete() CourseEnrollment.enroll(user, verifiable_course.id, CourseMode.VERIFIED) response = self.client.get(course_home_url(verifiable_course)) self.assertNotContains(response, TEST_COURSE_GOAL_OPTIONS) diff --git a/openedx/features/course_experience/views/course_home.py b/openedx/features/course_experience/views/course_home.py index 3718c4e8b4..55dbadb5df 100644 --- a/openedx/features/course_experience/views/course_home.py +++ b/openedx/features/course_experience/views/course_home.py @@ -3,7 +3,6 @@ Views for the course home page. """ -import six # lint-amnesty, pylint: disable=unused-import from django.conf import settings from django.template.context_processors import csrf from django.template.loader import render_to_string @@ -31,27 +30,26 @@ from openedx.core.djangoapps.content.course_overviews.models import CourseOvervi from openedx.core.djangoapps.plugin_api.views import EdxFragmentView from openedx.core.djangoapps.util.maintenance_banner import add_maintenance_banner from openedx.features.course_duration_limits.access import generate_course_expired_fragment +from openedx.features.course_experience import ( + COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, + LATEST_UPDATE_FLAG, + SHOW_UPGRADE_MSG_ON_COURSE_HOME, +) from openedx.features.course_experience.course_tools import CourseToolsPluginManager from openedx.features.course_experience.url_helpers import get_learning_mfe_home_url +from openedx.features.course_experience.utils import get_course_outline_block_tree, get_resume_block, get_start_block +from openedx.features.course_experience.views.course_dates import CourseDatesFragmentView +from openedx.features.course_experience.views.course_home_messages import CourseHomeMessageFragmentView +from openedx.features.course_experience.views.course_outline import CourseOutlineFragmentView +from openedx.features.course_experience.views.course_sock import CourseSockFragmentView +from openedx.features.course_experience.views.latest_update import LatestUpdateFragmentView +from openedx.features.course_experience.views.welcome_message import WelcomeMessageFragmentView from openedx.features.discounts.utils import get_first_purchase_offer_banner_fragment from openedx.features.discounts.utils import format_strikeout_price from common.djangoapps.student.models import CourseEnrollment from common.djangoapps.util.views import ensure_valid_course_key from xmodule.course_module import COURSE_VISIBILITY_PUBLIC, COURSE_VISIBILITY_PUBLIC_OUTLINE -from .. import ( - COURSE_ENABLE_UNENROLLED_ACCESS_FLAG, - LATEST_UPDATE_FLAG, - SHOW_UPGRADE_MSG_ON_COURSE_HOME, -) -from ..utils import get_course_outline_block_tree, get_resume_block, get_start_block -from .course_dates import CourseDatesFragmentView -from .course_home_messages import CourseHomeMessageFragmentView -from .course_outline import CourseOutlineFragmentView -from .course_sock import CourseSockFragmentView -from .latest_update import LatestUpdateFragmentView -from .welcome_message import WelcomeMessageFragmentView - EMPTY_HANDOUTS_HTML = '
    '