From 23d436831b017474fd27e99c7e8161c162d5250a Mon Sep 17 00:00:00 2001 From: Shafqat Farhan Date: Mon, 16 Jan 2023 21:35:30 +0500 Subject: [PATCH] fix: VAN-1247 - Add isControl property in recommendations response and update event properties --- .../recommendations/serializers.py | 5 +- .../recommendations/test_serializers.py | 7 +-- .../recommendations/test_views.py | 42 +++++++++++-- .../learner_home/recommendations/views.py | 60 ++++++++++--------- 4 files changed, 75 insertions(+), 39 deletions(-) diff --git a/lms/djangoapps/learner_home/recommendations/serializers.py b/lms/djangoapps/learner_home/recommendations/serializers.py index d2048e2ad2..1b13da552d 100644 --- a/lms/djangoapps/learner_home/recommendations/serializers.py +++ b/lms/djangoapps/learner_home/recommendations/serializers.py @@ -19,6 +19,7 @@ class CourseRecommendationSerializer(serializers.Serializer): courses = serializers.ListField( child=RecommendedCourseSerializer(), allow_empty=True ) - isPersonalizedRecommendation = serializers.BooleanField( - source="is_personalized_recommendation" + isControl = serializers.BooleanField( + source="is_control", + default=None ) diff --git a/lms/djangoapps/learner_home/recommendations/test_serializers.py b/lms/djangoapps/learner_home/recommendations/test_serializers.py index 8455d730ce..17a2e31cc8 100644 --- a/lms/djangoapps/learner_home/recommendations/test_serializers.py +++ b/lms/djangoapps/learner_home/recommendations/test_serializers.py @@ -41,7 +41,6 @@ class TestCourseRecommendationSerializer(TestCase): output_data = CourseRecommendationSerializer( { "courses": recommended_courses, - "is_personalized_recommendation": False, } ).data @@ -49,7 +48,7 @@ class TestCourseRecommendationSerializer(TestCase): output_data, { "courses": [], - "isPersonalizedRecommendation": False, + "isControl": None, }, ) @@ -61,7 +60,7 @@ class TestCourseRecommendationSerializer(TestCase): output_data = CourseRecommendationSerializer( { "courses": recommended_courses, - "is_personalized_recommendation": True, + "is_control": False, } ).data @@ -82,6 +81,6 @@ class TestCourseRecommendationSerializer(TestCase): "title": recommended_courses[1]["title"], }, ], - "isPersonalizedRecommendation": True, + "isControl": False, }, ) diff --git a/lms/djangoapps/learner_home/recommendations/test_views.py b/lms/djangoapps/learner_home/recommendations/test_views.py index 63256c322b..4e864f6d39 100644 --- a/lms/djangoapps/learner_home/recommendations/test_views.py +++ b/lms/djangoapps/learner_home/recommendations/test_views.py @@ -14,6 +14,7 @@ from common.djangoapps.student.tests.factories import ( CourseEnrollmentFactory, UserFactory, ) +from common.djangoapps.student.toggles import ENABLE_FALLBACK_RECOMMENDATIONS from lms.djangoapps.learner_home.test_utils import ( random_url, ) @@ -106,6 +107,7 @@ class TestCourseRecommendationApiView(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 404) self.assertEqual(response.data, None) + @override_waffle_flag(ENABLE_FALLBACK_RECOMMENDATIONS, active=True) @override_waffle_flag(ENABLE_LEARNER_HOME_AMPLITUDE_RECOMMENDATIONS, active=True) @mock.patch("django.conf.settings.GENERAL_RECOMMENDATIONS", GENERAL_RECOMMENDATIONS) @mock.patch( @@ -123,12 +125,13 @@ class TestCourseRecommendationApiView(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 200) response_content = json.loads(response.content) - self.assertEqual(response_content.get("isPersonalizedRecommendation"), False) + self.assertEqual(response_content.get("isControl"), False) self.assertEqual( response_content.get("courses"), self.SERIALIZED_GENERAL_RECOMMENDATIONS, ) + @override_waffle_flag(ENABLE_FALLBACK_RECOMMENDATIONS, active=True) @override_waffle_flag(ENABLE_LEARNER_HOME_AMPLITUDE_RECOMMENDATIONS, active=True) @mock.patch("django.conf.settings.GENERAL_RECOMMENDATIONS", GENERAL_RECOMMENDATIONS) @mock.patch( @@ -144,7 +147,7 @@ class TestCourseRecommendationApiView(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 200) response_content = json.loads(response.content) - self.assertEqual(response_content.get("isPersonalizedRecommendation"), False) + self.assertEqual(response_content.get("isControl"), None) self.assertEqual( response_content.get("courses"), self.SERIALIZED_GENERAL_RECOMMENDATIONS, @@ -173,11 +176,12 @@ class TestCourseRecommendationApiView(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 200) response_content = json.loads(response.content) - self.assertEqual(response_content.get("isPersonalizedRecommendation"), True) + self.assertEqual(response_content.get("isControl"), False) self.assertEqual( len(response_content.get("courses")), expected_recommendations_length ) + @override_waffle_flag(ENABLE_FALLBACK_RECOMMENDATIONS, active=True) @override_waffle_flag(ENABLE_LEARNER_HOME_AMPLITUDE_RECOMMENDATIONS, active=True) @mock.patch("django.conf.settings.GENERAL_RECOMMENDATIONS", GENERAL_RECOMMENDATIONS) @mock.patch( @@ -199,12 +203,37 @@ class TestCourseRecommendationApiView(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 200) response_content = json.loads(response.content) - self.assertEqual(response_content.get("isPersonalizedRecommendation"), False) + self.assertEqual(response_content.get("isControl"), True) self.assertEqual( response_content.get("courses"), self.SERIALIZED_GENERAL_RECOMMENDATIONS, ) + @override_waffle_flag(ENABLE_FALLBACK_RECOMMENDATIONS, active=False) + @override_waffle_flag(ENABLE_LEARNER_HOME_AMPLITUDE_RECOMMENDATIONS, active=True) + @mock.patch("django.conf.settings.GENERAL_RECOMMENDATIONS", GENERAL_RECOMMENDATIONS) + @mock.patch( + "lms.djangoapps.learner_home.recommendations.views.get_personalized_course_recommendations" + ) + def test_fallback_recommendations_disabled( + self, mocked_get_personalized_course_recommendations + ): + """ + Test that a user gets no recommendations for the control group. + """ + mocked_get_personalized_course_recommendations.return_value = [ + True, + True, + [], + ] + + response = self.client.get(self.url) + self.assertEqual(response.status_code, 200) + + response_content = json.loads(response.content) + self.assertEqual(response_content.get("isControl"), True) + self.assertEqual(response_content.get("courses"), []) + @override_waffle_flag(ENABLE_LEARNER_HOME_AMPLITUDE_RECOMMENDATIONS, active=True) @mock.patch( "lms.djangoapps.learner_home.recommendations.views.get_personalized_course_recommendations" @@ -231,9 +260,10 @@ class TestCourseRecommendationApiView(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 200) response_content = json.loads(response.content) - self.assertEqual(response_content.get("isPersonalizedRecommendation"), True) + self.assertEqual(response_content.get("isControl"), False) self.assertEqual(len(response_content.get("courses")), expected_recommendations) + @override_waffle_flag(ENABLE_FALLBACK_RECOMMENDATIONS, active=True) @override_waffle_flag(ENABLE_LEARNER_HOME_AMPLITUDE_RECOMMENDATIONS, active=True) @mock.patch("django.conf.settings.GENERAL_RECOMMENDATIONS", GENERAL_RECOMMENDATIONS) @mock.patch( @@ -262,7 +292,7 @@ class TestCourseRecommendationApiView(SharedModuleStoreTestCase): self.assertEqual(response.status_code, 200) response_content = json.loads(response.content) - self.assertEqual(response_content.get("isPersonalizedRecommendation"), False) + self.assertEqual(response_content.get("isControl"), False) self.assertEqual( response_content.get("courses"), self.SERIALIZED_GENERAL_RECOMMENDATIONS, diff --git a/lms/djangoapps/learner_home/recommendations/views.py b/lms/djangoapps/learner_home/recommendations/views.py index f5b290b900..a2339c79cf 100644 --- a/lms/djangoapps/learner_home/recommendations/views.py +++ b/lms/djangoapps/learner_home/recommendations/views.py @@ -14,6 +14,7 @@ from rest_framework.response import Response from rest_framework.views import APIView from common.djangoapps.student.models import CourseEnrollment +from common.djangoapps.student.toggles import show_fallback_recommendations from common.djangoapps.track import segment from lms.djangoapps.learner_home.recommendations.serializers import ( CourseRecommendationSerializer, @@ -52,29 +53,19 @@ class CourseRecommendationApiView(APIView): if not should_show_learner_home_amplitude_recommendations(): return Response(status=404) + user_id = request.user.id recommended_courses = [] - general_recommendations_response = Response( - CourseRecommendationSerializer( - { - "courses": settings.GENERAL_RECOMMENDATIONS, - "is_personalized_recommendation": False, - } - ).data, - status=200, - ) + fallback_recommendations = settings.GENERAL_RECOMMENDATIONS if show_fallback_recommendations() else [] try: - user_id = request.user.id is_control, has_is_control, course_keys = get_personalized_course_recommendations(user_id) except Exception as ex: # pylint: disable=broad-except logger.warning(f"Cannot get recommendations from Amplitude: {ex}") - return general_recommendations_response + return self._general_recommendations_response(user_id, None, fallback_recommendations) - if is_control or not course_keys: - self._emit_recommendations_viewed_event( - user_id, is_control, has_is_control, recommended_courses - ) - return general_recommendations_response + is_control = is_control if has_is_control else None + if is_control or is_control is None or not course_keys: + return self._general_recommendations_response(user_id, is_control, fallback_recommendations) user_enrolled_course_keys = set() fields = ["title", "owners", "marketing_url"] @@ -85,9 +76,9 @@ class CourseRecommendationApiView(APIView): user_enrolled_course_keys.add(course_key) # Pick 5 course keys, excluding the user's already enrolled course(s). - enrollable_course_keys = list( - set(course_keys).difference(user_enrolled_course_keys) - )[:5] + enrollable_course_keys = [ + course_key for course_key in course_keys if course_key not in user_enrolled_course_keys + ][:5] for course_id in enrollable_course_keys: course_data = get_course_data(course_id, fields) if course_data: @@ -99,34 +90,49 @@ class CourseRecommendationApiView(APIView): "marketing_url": course_data.get("marketing_url"), } ) - self._emit_recommendations_viewed_event( - user_id, is_control, has_is_control, recommended_courses - ) # If no courses are left after filtering already enrolled courses from # the list of amplitude recommendations, show general recommendations # to the user. if not recommended_courses: - return general_recommendations_response + return self._general_recommendations_response(user_id, is_control, fallback_recommendations) + self._emit_recommendations_viewed_event(user_id, is_control, recommended_courses) return Response( CourseRecommendationSerializer( { "courses": recommended_courses, - "is_personalized_recommendation": not is_control, + "is_control": is_control, } ).data, status=200, ) - def _emit_recommendations_viewed_event(self, user_id, is_control, has_is_control, recommended_courses): + def _emit_recommendations_viewed_event( + self, user_id, is_control, recommended_courses, amplitude_recommendations=True + ): """Emits an event to track Learner Home page visits.""" segment.track( user_id, "edx.bi.user.recommendations.viewed", { - "is_personalized_recommendation": not is_control, - "is_control": is_control if has_is_control else None, + "is_control": is_control, + "amplitude_recommendations": amplitude_recommendations, "course_key_array": [course["course_key"] for course in recommended_courses], }, ) + + def _general_recommendations_response(self, user_id, is_control, recommended_courses): + """ Helper method for general recommendations response. """ + self._emit_recommendations_viewed_event( + user_id, is_control, recommended_courses, amplitude_recommendations=False + ) + return Response( + CourseRecommendationSerializer( + { + "courses": recommended_courses, + "is_control": is_control, + } + ).data, + status=200, + )