diff --git a/lms/djangoapps/course_goals/api.py b/lms/djangoapps/course_goals/api.py index f132d894a8..0415ae5685 100644 --- a/lms/djangoapps/course_goals/api.py +++ b/lms/djangoapps/course_goals/api.py @@ -3,7 +3,6 @@ Course Goals Python API """ -from django.conf import settings from opaque_keys.edx.keys import CourseKey from rest_framework.reverse import reverse @@ -12,9 +11,26 @@ from lms.djangoapps.course_goals.models import CourseGoal, GOAL_KEY_CHOICES from openedx.features.course_experience import ENABLE_COURSE_GOALS -def add_course_goal(user, course_id, goal_key=None, - number_of_days_with_visits_per_week_goal=None, - subscribed_to_goal_reminders=None): +def add_course_goal_deprecated(user, course_id, goal_key): + """ + Add a new course goal for the provided user and course. If the goal + already exists, simply update and save the goal. + This method is for the deprecated version of course goals and will be removed as soon + as the newer number of days version of course goals is fully implemented. + + Arguments: + 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)) + CourseGoal.objects.update_or_create( + user=user, course_key=course_key, defaults={'goal_key': goal_key} + ) + + +def add_course_goal(user, course_id, days_per_week, + subscribed_to_reminders): """ Add a new course goal for the provided user and course. If the goal already exists, simply update and save the goal. @@ -22,22 +38,16 @@ def add_course_goal(user, course_id, goal_key=None, Arguments: 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. - number_of_days_with_visits_per_week_goal (int): number of days learner wants to learn per week - subscribed_to_goal_reminders (bool): whether the learner wants to receive email reminders about their goal + days_per_week (int): number of days learner wants to learn per week + subscribed_to_reminders (bool): whether the learner wants to receive email reminders about their goal """ course_key = CourseKey.from_string(str(course_id)) - if goal_key: - CourseGoal.objects.update_or_create( - user=user, course_key=course_key, defaults={'goal_key': goal_key} - ) - else: - CourseGoal.objects.update_or_create( - user=user, course_key=course_key, defaults={ - 'number_of_days_with_visits_per_week_goal': number_of_days_with_visits_per_week_goal, - 'subscribed_to_goal_reminders': subscribed_to_goal_reminders, - } - ) + CourseGoal.objects.update_or_create( + user=user, course_key=course_key, defaults={ + 'days_per_week': days_per_week, + 'subscribed_to_reminders': subscribed_to_reminders, + } + ) def get_course_goal(user, course_key): @@ -68,8 +78,7 @@ def has_course_goal_permission(request, course_id, user_access): """ course_key = CourseKey.from_string(course_id) has_verified_mode = CourseMode.has_verified_mode(CourseMode.modes_for_course_dict(course_key)) - return user_access['is_enrolled'] and has_verified_mode and ENABLE_COURSE_GOALS.is_enabled(course_key) \ - and settings.FEATURES.get('ENABLE_COURSE_GOALS') + return user_access['is_enrolled'] and has_verified_mode and ENABLE_COURSE_GOALS.is_enabled(course_key) def get_course_goal_options(): diff --git a/lms/djangoapps/course_goals/handlers.py b/lms/djangoapps/course_goals/handlers.py index a97072241e..661d3220f5 100644 --- a/lms/djangoapps/course_goals/handlers.py +++ b/lms/djangoapps/course_goals/handlers.py @@ -18,8 +18,8 @@ def emit_course_goal_event(sender, instance, **kwargs): # lint-amnesty, pylint: properties = { 'courserun_key': str(instance.course_key), 'goal_key': instance.goal_key, - 'number_of_days_with_visits_per_week_goal': instance.number_of_days_with_visits_per_week_goal, - 'subscribed_to_goal_reminders': instance.subscribed_to_goal_reminders, + 'days_per_week': instance.days_per_week, + 'subscribed_to_reminders': instance.subscribed_to_reminders, } tracker.emit(name, properties) segment.track(instance.user.id, name, properties) diff --git a/lms/djangoapps/course_goals/migrations/0004_auto_20210726_1338.py b/lms/djangoapps/course_goals/migrations/0004_auto_20210806_0137.py similarity index 73% rename from lms/djangoapps/course_goals/migrations/0004_auto_20210726_1338.py rename to lms/djangoapps/course_goals/migrations/0004_auto_20210806_0137.py index 86ff6f25ec..fcd51c2b6e 100644 --- a/lms/djangoapps/course_goals/migrations/0004_auto_20210726_1338.py +++ b/lms/djangoapps/course_goals/migrations/0004_auto_20210806_0137.py @@ -1,4 +1,4 @@ -# Generated by Django 2.2.24 on 2021-07-26 13:38 +# Generated by Django 2.2.24 on 2021-08-06 01:37 from django.db import migrations, models @@ -12,22 +12,22 @@ class Migration(migrations.Migration): operations = [ migrations.AddField( model_name='coursegoal', - name='number_of_days_with_visits_per_week_goal', + name='days_per_week', field=models.PositiveIntegerField(default=0), ), migrations.AddField( model_name='coursegoal', - name='subscribed_to_goal_reminders', + name='subscribed_to_reminders', field=models.BooleanField(default=False), ), migrations.AddField( model_name='historicalcoursegoal', - name='number_of_days_with_visits_per_week_goal', + name='days_per_week', field=models.PositiveIntegerField(default=0), ), migrations.AddField( model_name='historicalcoursegoal', - name='subscribed_to_goal_reminders', + name='subscribed_to_reminders', field=models.BooleanField(default=False), ), ] diff --git a/lms/djangoapps/course_goals/models.py b/lms/djangoapps/course_goals/models.py index 13a51a42c4..62a80c0483 100644 --- a/lms/djangoapps/course_goals/models.py +++ b/lms/djangoapps/course_goals/models.py @@ -18,8 +18,6 @@ GOAL_KEY_CHOICES = Choices( ('unsure', _('Not sure yet')), ) -NUMBER_OF_DAYS_OPTIONS = [1, 3, 5] - User = get_user_model() @@ -36,15 +34,15 @@ class CourseGoal(models.Model): user = models.ForeignKey(User, blank=False, on_delete=models.CASCADE) course_key = CourseKeyField(max_length=255, db_index=True) # The goal a user has set for the number of days they want to learn per week - number_of_days_with_visits_per_week_goal = models.PositiveIntegerField(default=0) + days_per_week = models.PositiveIntegerField(default=0) # Controls whether a user will receive emails reminding them to stay on track with their learning goal - subscribed_to_goal_reminders = models.BooleanField(default=False) + subscribed_to_reminders = models.BooleanField(default=False) 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} days per week for course {course}'.format( user=self.user.username, - goal=self.number_of_days_with_visits_per_week_goal, + goal=self.days_per_week, course=self.course_key, ) diff --git a/lms/djangoapps/course_goals/tests/test_api.py b/lms/djangoapps/course_goals/tests/test_api.py index 99983f6027..a3582ba36d 100644 --- a/lms/djangoapps/course_goals/tests/test_api.py +++ b/lms/djangoapps/course_goals/tests/test_api.py @@ -48,8 +48,8 @@ class TestCourseGoalsAPI(SharedModuleStoreTestCase): segment_call.assert_called_once_with(self.user.id, EVENT_NAME_ADDED, { 'courserun_key': str(self.course.id), 'goal_key': 'certify', - 'number_of_days_with_visits_per_week_goal': 0, - 'subscribed_to_goal_reminders': False, + 'days_per_week': 0, + 'subscribed_to_reminders': False, }) assert response.status_code == 201 @@ -83,18 +83,18 @@ class TestCourseGoalsAPI(SharedModuleStoreTestCase): segment_call.assert_any_call(self.user.id, EVENT_NAME_ADDED, { 'courserun_key': str(self.course.id), 'goal_key': 'explore', - 'number_of_days_with_visits_per_week_goal': 0, - 'subscribed_to_goal_reminders': False, + 'days_per_week': 0, + 'subscribed_to_reminders': False, }) segment_call.assert_any_call(self.user.id, EVENT_NAME_UPDATED, { 'courserun_key': str(self.course.id), 'goal_key': 'certify', - 'number_of_days_with_visits_per_week_goal': 0, - 'subscribed_to_goal_reminders': False, + 'days_per_week': 0, + 'subscribed_to_reminders': False, }) segment_call.assert_any_call(self.user.id, EVENT_NAME_UPDATED, { 'courserun_key': str(self.course.id), 'goal_key': 'unsure', - 'number_of_days_with_visits_per_week_goal': 0, - 'subscribed_to_goal_reminders': False, + 'days_per_week': 0, + 'subscribed_to_reminders': False, }) current_goals = CourseGoal.objects.filter(user=self.user, course_key=self.course.id) assert len(current_goals) == 1 diff --git a/lms/djangoapps/course_goals/tests/test_goals.py b/lms/djangoapps/course_goals/tests/test_goals.py index e7c49502d5..e6b88c0b17 100644 --- a/lms/djangoapps/course_goals/tests/test_goals.py +++ b/lms/djangoapps/course_goals/tests/test_goals.py @@ -2,6 +2,7 @@ Unit tests for course_goals djangoapp """ +import json from unittest import mock from django.contrib.auth import get_user_model @@ -49,15 +50,15 @@ class TestCourseGoalsAPI(SharedModuleStoreTestCase): Sends a post request to set a course goal and returns the response. """ post_data = { - 'course_id': self.course.id, + 'course_id': str(self.course.id), 'user': self.user.username, } if number is not None: - post_data['number_of_days_with_visits_per_week_goal'] = number + post_data['days_per_week'] = number if subscribed is not None: - post_data['subscribed_to_goal_reminders'] = subscribed + post_data['subscribed_to_reminders'] = subscribed - response = self.client.post(self.apiUrl, post_data) + response = self.client.post(self.apiUrl, json.dumps(post_data), content_type='application/json') return response @mock.patch('lms.djangoapps.course_goals.handlers.segment.track') @@ -67,15 +68,15 @@ class TestCourseGoalsAPI(SharedModuleStoreTestCase): self.save_course_goal(1, True) segment_call.assert_called_once_with(self.user.id, EVENT_NAME_ADDED, { 'courserun_key': str(self.course.id), - 'number_of_days_with_visits_per_week_goal': 1, - 'subscribed_to_goal_reminders': True, + 'days_per_week': 1, + 'subscribed_to_reminders': True, '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].number_of_days_with_visits_per_week_goal == 1 - assert current_goals[0].subscribed_to_goal_reminders is True + assert current_goals[0].days_per_week == 1 + assert current_goals[0].subscribed_to_reminders is True @mock.patch('lms.djangoapps.course_goals.handlers.segment.track') @override_settings(LMS_SEGMENT_KEY="foobar") @@ -84,31 +85,31 @@ class TestCourseGoalsAPI(SharedModuleStoreTestCase): self.save_course_goal(1, True) segment_call.assert_called_with(self.user.id, EVENT_NAME_ADDED, { 'courserun_key': str(self.course.id), - 'number_of_days_with_visits_per_week_goal': 1, - 'subscribed_to_goal_reminders': True, + 'days_per_week': 1, + 'subscribed_to_reminders': True, 'goal_key': 'unsure', }) self.save_course_goal(3, True) segment_call.assert_called_with(self.user.id, EVENT_NAME_UPDATED, { 'courserun_key': str(self.course.id), - 'number_of_days_with_visits_per_week_goal': 3, - 'subscribed_to_goal_reminders': True, + 'days_per_week': 3, + 'subscribed_to_reminders': True, 'goal_key': 'unsure', }) self.save_course_goal(5, False) segment_call.assert_called_with(self.user.id, EVENT_NAME_UPDATED, { 'courserun_key': str(self.course.id), - 'number_of_days_with_visits_per_week_goal': 5, - 'subscribed_to_goal_reminders': False, + 'days_per_week': 5, + 'subscribed_to_reminders': False, '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].number_of_days_with_visits_per_week_goal == 5 - assert current_goals[0].subscribed_to_goal_reminders is False + assert current_goals[0].days_per_week == 5 + assert current_goals[0].subscribed_to_reminders is False def test_add_without_required_arguments(self): """ Ensures if required arguments are not provided, post does not succeed. """ @@ -116,7 +117,7 @@ class TestCourseGoalsAPI(SharedModuleStoreTestCase): assert len(CourseGoal.objects.filter(user=self.user, course_key=self.course.id)) == 0 self.assertContains( response=response, - text="'number_of_days_with_visits_per_week_goal' and 'subscribed_to_goal_reminders' are required.", + text="'days_per_week' and 'subscribed_to_reminders' are required.", status_code=400 ) diff --git a/lms/djangoapps/course_home_api/outline/v1/serializers.py b/lms/djangoapps/course_home_api/outline/v1/serializers.py index 5b59637f0a..1cab4cd27f 100644 --- a/lms/djangoapps/course_home_api/outline/v1/serializers.py +++ b/lms/djangoapps/course_home_api/outline/v1/serializers.py @@ -66,7 +66,7 @@ class CourseGoalsSerializer(serializers.Serializer): """ Serializer for Course Goal data """ - goal_options = serializers.ListField() + goal_options = serializers.ListField(default=[]) selected_goal = serializers.DictField() diff --git a/lms/djangoapps/course_home_api/outline/v1/tests/test_views.py b/lms/djangoapps/course_home_api/outline/v1/tests/test_views.py index 1edfafa962..a52609f3c7 100644 --- a/lms/djangoapps/course_home_api/outline/v1/tests/test_views.py +++ b/lms/djangoapps/course_home_api/outline/v1/tests/test_views.py @@ -8,6 +8,7 @@ from lms.djangoapps.grades.course_grade_factory import CourseGradeFactory from unittest.mock import Mock, patch import ddt +import json from django.conf import settings from django.urls import reverse from edx_toggles.toggles.testutils import override_waffle_flag @@ -244,12 +245,16 @@ class OutlineTabTestViews(BaseCourseHomeTests): """ Test that the api returns the correct response when saving a goal """ CourseEnrollment.enroll(self.user, self.course.id, CourseMode.AUDIT) - post_data = { - 'course_id': self.course.id, - 'number_of_days_with_visits_per_week_goal': 1, - 'subscribed_to_goal_reminders': True, - } - post_course_goal_response = self.client.post(reverse('course-home-save-course-goal'), post_data) + post_data = json.dumps({ + 'course_id': str(self.course.id), + 'days_per_week': 1, + 'subscribed_to_reminders': True, + }) + post_course_goal_response = self.client.post( + reverse('course-home-save-course-goal'), + post_data, + content_type='application/json', + ) assert post_course_goal_response.status_code == 200 response = self.client.get(self.url) @@ -257,10 +262,10 @@ class OutlineTabTestViews(BaseCourseHomeTests): course_goals = response.json()['course_goals'] expected_course_goals = { - 'goal_options': [1, 3, 5], + 'goal_options': [], 'selected_goal': { - 'number_of_days_with_visits_per_week_goal': 1, - 'subscribed_to_goal_reminders': True + 'days_per_week': 1, + 'subscribed_to_reminders': True } } assert course_goals == expected_course_goals diff --git a/lms/djangoapps/course_home_api/outline/v1/views.py b/lms/djangoapps/course_home_api/outline/v1/views.py index acc5c64e52..f842275a37 100644 --- a/lms/djangoapps/course_home_api/outline/v1/views.py +++ b/lms/djangoapps/course_home_api/outline/v1/views.py @@ -22,16 +22,17 @@ from rest_framework.response import Response from common.djangoapps.course_modes.models import CourseMode from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.util.json_request import expect_json from common.djangoapps.util.views import expose_header from lms.djangoapps.course_goals.api import ( add_course_goal, + add_course_goal_deprecated, get_course_goal, get_course_goal_text, has_course_goal_permission, valid_course_goals_ordered ) from lms.djangoapps.course_goals.toggles import COURSE_GOALS_NUMBER_OF_DAYS_GOALS -from lms.djangoapps.course_goals.models import NUMBER_OF_DAYS_OPTIONS from lms.djangoapps.course_home_api.outline.v1.serializers import OutlineTabSerializer from lms.djangoapps.course_home_api.toggles import ( course_home_legacy_is_active, @@ -108,10 +109,9 @@ class OutlineTabView(RetrieveAPIView): resume_block: (bool) Whether the block is the resume block has_scheduled_content: (bool) Whether the block has more content scheduled for the future course_goals: - goal_options: (list) A list of number of days options for the number of days goal selected_goal: - number_of_days_with_visits_per_week_goal: (int) The number of days the learner wants to learn per week - subscribed_to_goal_reminders: (bool) Whether the learner wants email reminders about their goal + days_per_week: (int) The number of days the learner wants to learn per week + subscribed_to_reminders: (bool) Whether the learner wants email reminders about their goal course_tools: List of serialized Course Tool objects. Each serialization has the following fields: analytics_id: (str) The unique id given to the tool. title: (str) The display title of the tool. @@ -250,19 +250,17 @@ class OutlineTabView(RetrieveAPIView): cert_data = get_cert_data(request.user, course, enrollment.mode) if is_enrolled else None if COURSE_GOALS_NUMBER_OF_DAYS_GOALS.is_enabled(): - if (is_enrolled and ENABLE_COURSE_GOALS.is_enabled(course_key) - and settings.FEATURES.get('ENABLE_COURSE_GOALS')): + if (is_enrolled and ENABLE_COURSE_GOALS.is_enabled(course_key)): course_goals = { - 'goal_options': NUMBER_OF_DAYS_OPTIONS, 'selected_goal': None } selected_goal = get_course_goal(request.user, course_key) if selected_goal: course_goals['selected_goal'] = { - 'number_of_days_with_visits_per_week_goal': selected_goal.number_of_days_with_visits_per_week_goal, - 'subscribed_to_goal_reminders': selected_goal.subscribed_to_goal_reminders, + 'days_per_week': selected_goal.days_per_week, + 'subscribed_to_reminders': selected_goal.subscribed_to_reminders, } else: # Only show the set course goal message for enrolled, unverified @@ -409,45 +407,39 @@ def dismiss_welcome_message(request): @authentication_classes((JwtAuthentication, SessionAuthenticationAllowInactiveUser,)) @permission_classes((IsAuthenticated,)) def save_course_goal(request): - course_id = request.data.get('course_id', None) - goal_key = request.data.get('goal_key', None) - number_of_days_with_visits_per_week_goal = request.data.get('number_of_days_with_visits_per_week_goal', None) - subscribed_to_goal_reminders = request.data.get('subscribed_to_goal_reminders', None) + course_id = request.data.get('course_id') + goal_key = request.data.get('goal_key') + days_per_week = request.data.get('days_per_week') + subscribed_to_reminders = request.data.get('subscribed_to_reminders') # If body doesn't contain 'course_id', return 400 to client. if not course_id: - raise ParseError(_("'course_id' is required.")) + raise ParseError("'course_id' is required.") if COURSE_GOALS_NUMBER_OF_DAYS_GOALS.is_enabled(): # If body doesn't contain the required goals fields, return 400 to client. - if number_of_days_with_visits_per_week_goal is None or subscribed_to_goal_reminders is None: - raise ParseError(_("'number_of_days_with_visits_per_week_goal' and 'subscribed_to_goal_reminders' are required.")) + if days_per_week is None or subscribed_to_reminders is None: + raise ParseError("'days_per_week' and 'subscribed_to_reminders' are required.") try: - number_of_days_with_visits_per_week_goal = int(number_of_days_with_visits_per_week_goal) - except ValueError: - raise ParseError(_("'number_of_days_with_visits_per_week_goal' needs to be an integer")) - subscribed_to_goal_reminders = subscribed_to_goal_reminders == 'True' - - kwargs = { - "number_of_days_with_visits_per_week_goal": number_of_days_with_visits_per_week_goal, - "subscribed_to_goal_reminders": subscribed_to_goal_reminders, - } + add_course_goal(request.user, course_id, days_per_week, subscribed_to_reminders) + return Response({ + 'header': _('Your course goal has been successfully set.'), + 'message': _('Course goal updated successfully.'), + }) + except Exception: + raise UnableToSaveCourseGoal else: # If body doesn't contain 'goal', return 400 to client. if not goal_key: - raise ParseError(_("'goal_key' is required.")) + raise ParseError("'goal_key' is required.") - kwargs = { - "goal_key": goal_key, - } - - try: - add_course_goal(request.user, course_id, **kwargs) - return Response({ - 'header': _('Your course goal has been successfully set.'), - 'message': _('Course goal updated successfully.'), - }) - except Exception: - raise UnableToSaveCourseGoal + try: + add_course_goal_deprecated(request.user, course_id, goal_key) + return Response({ + 'header': _('Your course goal has been successfully set.'), + 'message': _('Course goal updated successfully.'), + }) + except Exception: + raise UnableToSaveCourseGoal diff --git a/lms/envs/common.py b/lms/envs/common.py index e7b81e4d93..4d4d09bca7 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -801,9 +801,6 @@ FEATURES = { # .. toggle_tickets: https://github.com/edx/edx-platform/pull/15006 'ENABLE_BULK_ENROLLMENT_VIEW': False, - # Whether course goals is enabled. - 'ENABLE_COURSE_GOALS': True, - # Set to enable Enterprise integration 'ENABLE_ENTERPRISE_INTEGRATION': False, 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 358e7e6c88..dc8c29f058 100644 --- a/openedx/features/course_experience/tests/views/test_course_home.py +++ b/openedx/features/course_experience/tests/views/test_course_home.py @@ -27,7 +27,7 @@ from common.djangoapps.student.tests.factories import OrgStaffFactory from common.djangoapps.student.tests.factories import StaffFactory from lms.djangoapps.commerce.models import CommerceConfiguration from lms.djangoapps.commerce.utils import EcommerceService -from lms.djangoapps.course_goals.api import add_course_goal, get_course_goal +from lms.djangoapps.course_goals.api import add_course_goal_deprecated, get_course_goal from lms.djangoapps.course_home_api.toggles import COURSE_HOME_USE_LEGACY_FRONTEND from lms.djangoapps.courseware.tests.helpers import get_expiration_banner_text from lms.djangoapps.discussion.django_comment_client.tests.factories import RoleFactory @@ -755,7 +755,7 @@ class TestCourseHomePageAccess(CourseHomePageTestCase): self.assertContains(response, TEST_COURSE_GOAL_OPTIONS) # Verify that enrolled users that have set a course goal are not shown the set course goal message. - add_course_goal(user, verifiable_course.id, COURSE_GOAL_DISMISS_OPTION) + add_course_goal_deprecated(user, verifiable_course.id, COURSE_GOAL_DISMISS_OPTION) response = self.client.get(course_home_url(verifiable_course)) self.assertNotContains(response, TEST_COURSE_GOAL_OPTIONS) @@ -797,7 +797,7 @@ class TestCourseHomePageAccess(CourseHomePageTestCase): self.assertContains(response, TEST_COURSE_GOAL_UPDATE_FIELD_HIDDEN) # Verify that enrolled users that have set a course goal are shown a visible update goal selection field. - add_course_goal(user, verifiable_course.id, COURSE_GOAL_DISMISS_OPTION) + add_course_goal_deprecated(user, verifiable_course.id, COURSE_GOAL_DISMISS_OPTION) response = self.client.get(course_home_url(verifiable_course)) self.assertContains(response, TEST_COURSE_GOAL_UPDATE_FIELD) self.assertNotContains(response, TEST_COURSE_GOAL_UPDATE_FIELD_HIDDEN)