From 389883d4a7b4fd4508f7472b0ee127517bff8090 Mon Sep 17 00:00:00 2001 From: Adam Stankiewicz Date: Fri, 13 Sep 2024 14:00:36 -0400 Subject: [PATCH] feat: support run-based assignments in `can-redeem`; ensure assigned policies have highest priority (#562) --- .../apps/content_assignments/api.py | 59 +++++++--- .../content_assignments/tests/test_api.py | 107 ++++++++++++++++-- .../apps/subsidy_access_policy/constants.py | 1 + .../apps/subsidy_access_policy/models.py | 17 ++- .../tests/test_models.py | 24 ++-- 5 files changed, 175 insertions(+), 33 deletions(-) diff --git a/enterprise_access/apps/content_assignments/api.py b/enterprise_access/apps/content_assignments/api.py index bd707755..2645ba0b 100644 --- a/enterprise_access/apps/content_assignments/api.py +++ b/enterprise_access/apps/content_assignments/api.py @@ -142,7 +142,7 @@ def get_assignments_for_admin( assignment_configuration (AssignmentConfiguration): The assignment configuration within which to search for assignments. learner_emails (list of str): A list of emails for which the admin intends to find existing assignments. - content_key (str): A content key representing a course which the assignments are for. + content_key (str): A content key representing a course or course run which the assignments are for. Returns: queryset of ``LearnerContentAssignment``: Existing records relevant to an admin's allocation request. @@ -176,20 +176,21 @@ def get_assignment_for_learner( both course run keys, so a simple string comparison may not always suffice. Method of content_key comparison for assignment lookup: - +---+------------------------+-----------------------+----------------------------------------------+ - | # | assignment content_key | requested content_key | How to compare? | - +---+------------------------+-----------------------+----------------------------------------------+ - | 1 | course | course | Simple comparison. | - | 2 | course | course run | Convert everything to courses, then compare. | (most common) - | 3 | course run | course | Not supported. | - | 4 | course run | course run | Not supported. | - +---+------------------------+-----------------------+----------------------------------------------+ + +---+------------------------+-----------------------+-------------------------------------------------+ + | # | assignment content_key | requested content_key | How to compare? | + +---+------------------------+-----------------------+-------------------------------------------------+ + | 1 | course | course | Simple comparison. | + | 2 | course | course run | Convert course run to course key, then compare. | + | 3 | course run | course | Simple comparison via the parent_content_key, | + | | | | returning first run-based assignment match. | + | 4 | course run | course run | Simple comparison. | + +---+------------------------+-----------------------+-------------------------------------------------+ Args: assignment_configuration (AssignmentConfiguration): The assignment configuration within which to search for assignments. lms_user_id (int): One lms_user_id which the assignments are for. - content_key (str): A content key representing a course which the assignments are for. + content_key (str): A content key representing a course or course run which the assignments are for. Returns: ``LearnerContentAssignment``: Existing assignment relevant to a learner's redemption request, or None if not @@ -200,19 +201,47 @@ def get_assignment_for_learner( constraint across [assignment_configuration,lms_user_id,content_key]. BUT still technically possible if internal staff managed to create a duplicate assignment configuration for a single enterprise. """ + queryset = LearnerContentAssignment.objects.select_related('assignment_configuration') + + try: + # First, try to find a corresponding assignment based on the specific content key, + # considering both assignments' content key and parent content key. + return queryset.get( + Q(content_key=content_key) | Q(parent_content_key=content_key), + assignment_configuration=assignment_configuration, + lms_user_id=lms_user_id, + ) + except LearnerContentAssignment.DoesNotExist: + logger.info( + f'No assignment found with content_key or parent_content_key {content_key} ' + f'for {assignment_configuration} and lms_user_id {lms_user_id}', + ) + except LearnerContentAssignment.MultipleObjectsReturned as exc: + logger.error( + f'Multiple assignments found with content_key or parent_content_key {content_key} ' + f'for {assignment_configuration} and lms_user_id {lms_user_id}', + ) + raise exc + + # If no exact match was found, try to normalize the content key and find a match. This happens when + # the content_key is a course run key and the assignment's content_key is a course key, as depicted + # by row 2 in the above docstring matrix. content_key_to_match = _normalize_course_key_from_metadata(assignment_configuration, content_key) if not content_key_to_match: logger.error(f'Unable to normalize content_key {content_key} for {assignment_configuration} and {lms_user_id}') return None - queryset = LearnerContentAssignment.objects.select_related('assignment_configuration') + try: return queryset.get( + content_key=content_key_to_match, assignment_configuration=assignment_configuration, lms_user_id=lms_user_id, - # assignment content_key is assumed to always be a course with no namespace prefix. - content_key=content_key_to_match, ) except LearnerContentAssignment.DoesNotExist: + logger.info( + f'No assignment found with normalized content_key {content_key_to_match} ' + f'for {assignment_configuration} and lms_user_id {lms_user_id}', + ) return None @@ -245,7 +274,7 @@ def allocate_assignments(assignment_configuration, learner_emails, content_key, Params: - ``assignment_configuration``: The AssignmentConfiguration record under which assignments should be allocated. - ``learner_emails``: A list of learner email addresses to whom assignments should be allocated. - - ``content_key``: Typically a *course* key to which the learner is assigned. + - ``content_key``: Either a course or course run key, representing the content to be allocated. - ``content_price_cents``: The cost of redeeming the content, in USD cents, at the time of allocation. Should always be an integer >= 0. @@ -446,7 +475,7 @@ def _get_lms_user_ids_by_email(emails): def _get_existing_assignments_for_allocation( - assignment_configuration, learner_emails_to_allocate, content_key, lms_user_ids_by_email, + assignment_configuration, learner_emails_to_allocate, content_key, lms_user_ids_by_email, ): """ Finds any existing assignments records related to the provided ``assignment_cofiguration``, diff --git a/enterprise_access/apps/content_assignments/tests/test_api.py b/enterprise_access/apps/content_assignments/tests/test_api.py index ae464ece..ab75ecf2 100644 --- a/enterprise_access/apps/content_assignments/tests/test_api.py +++ b/enterprise_access/apps/content_assignments/tests/test_api.py @@ -116,57 +116,146 @@ def test_get_assignments_for_configuration_different_states(self): return_value=mock.MagicMock(), ) @ddt.data( - # Standard happy path. + # [course run] Standard happy path. { - 'assignment_content_key': 'test+course', + 'assignment_content_key': 'course-v1:test+course+run', + 'assignment_parent_content_key': 'test+course', + 'assignment_is_course_run': True, 'assignment_lms_user_id': 1, 'request_default_assignment_configuration': True, 'request_lms_user_id': 1, 'request_content_key': 'course-v1:test+course+run', 'expect_assignment_found': True, }, - # Happy path, requested content is a course (with prefix). + # [course] Standard happy path. { 'assignment_content_key': 'test+course', + 'assignment_parent_content_key': None, + 'assignment_is_course_run': False, + 'assignment_lms_user_id': 1, + 'request_default_assignment_configuration': True, + 'request_lms_user_id': 1, + 'request_content_key': 'course-v1:test+course+run', + 'expect_assignment_found': True, + }, + # [course run] Happy path, requested content is a course + { + 'assignment_content_key': 'course-v1:test+course+run', + 'assignment_parent_content_key': 'test+course', + 'assignment_is_course_run': True, 'assignment_lms_user_id': 1, 'request_default_assignment_configuration': True, 'request_lms_user_id': 1, - 'request_content_key': 'course-v1:test+course', # This is a course! With a prefix! + 'request_content_key': 'test+course', 'expect_assignment_found': True, }, - # Happy path, requested content is a course (without prefix). + # [course] Happy path, requested content is a course { 'assignment_content_key': 'test+course', + 'assignment_parent_content_key': None, + 'assignment_is_course_run': False, 'assignment_lms_user_id': 1, 'request_default_assignment_configuration': True, 'request_lms_user_id': 1, - 'request_content_key': 'test+course', # This is a course! Without a prefix! + 'request_content_key': 'test+course', 'expect_assignment_found': True, }, - # Different lms_user_id. + # [course run] Different lms_user_id, requested content is a course + { + 'assignment_content_key': 'course-v1:test+course+run', + 'assignment_parent_content_key': 'test+course', + 'assignment_is_course_run': True, + 'assignment_lms_user_id': 1, + 'request_default_assignment_configuration': True, + 'request_lms_user_id': 2, # Different lms_user_id! + 'request_content_key': 'test+course', + 'expect_assignment_found': False, + }, + # [course] Different lms_user_id, requested content is a course + { + 'assignment_content_key': 'test+course', + 'assignment_parent_content_key': None, + 'assignment_is_course_run': False, + 'assignment_lms_user_id': 1, + 'request_default_assignment_configuration': True, + 'request_lms_user_id': 2, # Different lms_user_id! + 'request_content_key': 'test+course', + 'expect_assignment_found': False, + }, + # [course run] Different lms_user_id + { + 'assignment_content_key': 'course-v1:test+course+run', + 'assignment_parent_content_key': 'test+course', + 'assignment_is_course_run': True, + 'assignment_lms_user_id': 1, + 'request_default_assignment_configuration': True, + 'request_lms_user_id': 2, # Different lms_user_id! + 'request_content_key': 'course-v1:test+course+run', + 'expect_assignment_found': False, + }, + # [course] Different lms_user_id { 'assignment_content_key': 'test+course', + 'assignment_parent_content_key': None, + 'assignment_is_course_run': False, 'assignment_lms_user_id': 1, 'request_default_assignment_configuration': True, 'request_lms_user_id': 2, # Different lms_user_id! + 'request_content_key': 'course-v1:test+course+run', + 'expect_assignment_found': False, + }, + # [course run] Different customer, requested content is a course + { + 'assignment_content_key': 'course-v1:test+course+run', + 'assignment_parent_content_key': 'test+course', + 'assignment_is_course_run': True, + 'assignment_lms_user_id': 1, + 'request_default_assignment_configuration': False, # Different customer! + 'request_lms_user_id': 1, 'request_content_key': 'test+course', 'expect_assignment_found': False, }, - # Different customer. + # [course] Different customer, requested content is a course { 'assignment_content_key': 'test+course', + 'assignment_parent_content_key': None, + 'assignment_is_course_run': False, 'assignment_lms_user_id': 1, 'request_default_assignment_configuration': False, # Different customer! 'request_lms_user_id': 1, 'request_content_key': 'test+course', 'expect_assignment_found': False, }, + # [course run] Different customer + { + 'assignment_content_key': 'course-v1:test+course+run', + 'assignment_parent_content_key': 'test+course', + 'assignment_is_course_run': True, + 'assignment_lms_user_id': 1, + 'request_default_assignment_configuration': False, # Different customer! + 'request_lms_user_id': 1, + 'request_content_key': 'course-v1:test+course+run', + 'expect_assignment_found': False, + }, + # [course] Different customer + { + 'assignment_content_key': 'test+course', + 'assignment_parent_content_key': None, + 'assignment_is_course_run': False, + 'assignment_lms_user_id': 1, + 'request_default_assignment_configuration': False, # Different customer! + 'request_lms_user_id': 1, + 'request_content_key': 'course-v1:test+course+run', + 'expect_assignment_found': False, + }, ) @ddt.unpack def test_get_assignment_for_learner( self, mock_get_and_cache_content_metadata, assignment_content_key, + assignment_parent_content_key, + assignment_is_course_run, assignment_lms_user_id, request_default_assignment_configuration, request_lms_user_id, @@ -182,6 +271,8 @@ def test_get_assignment_for_learner( LearnerContentAssignmentFactory.create( assignment_configuration=self.assignment_configuration, content_key=assignment_content_key, + parent_content_key=assignment_parent_content_key, + is_assigned_course_run=assignment_is_course_run, lms_user_id=assignment_lms_user_id, state=LearnerContentAssignmentStateChoices.ALLOCATED, ) diff --git a/enterprise_access/apps/subsidy_access_policy/constants.py b/enterprise_access/apps/subsidy_access_policy/constants.py index f92c6adb..a0e627f5 100644 --- a/enterprise_access/apps/subsidy_access_policy/constants.py +++ b/enterprise_access/apps/subsidy_access_policy/constants.py @@ -30,6 +30,7 @@ class SegmentEvents: # Configure the priority of each policy type here. When given multiple redeemable policies to select for redemption, # the policy resolution engine will select policies with the lowest priority number. +ASSIGNED_CREDIT_POLICY_TYPE_PRIORITY = 0 CREDIT_POLICY_TYPE_PRIORITY = 1 SUBSCRIPTION_POLICY_TYPE_PRIORITY = 2 diff --git a/enterprise_access/apps/subsidy_access_policy/models.py b/enterprise_access/apps/subsidy_access_policy/models.py index 46ff611b..bcb3bf1a 100644 --- a/enterprise_access/apps/subsidy_access_policy/models.py +++ b/enterprise_access/apps/subsidy_access_policy/models.py @@ -25,6 +25,7 @@ from ..content_assignments.models import AssignmentConfiguration from .constants import ( + ASSIGNED_CREDIT_POLICY_TYPE_PRIORITY, CREDIT_POLICY_TYPE_PRIORITY, FORCE_ENROLLMENT_KEYWORD, REASON_BEYOND_ENROLLMENT_DEADLINE, @@ -1107,6 +1108,16 @@ def priority(self): return CREDIT_POLICY_TYPE_PRIORITY +class AssignedCreditPolicyMixin: + """ + Mixin class for assigned credit type policies. + """ + + @property + def priority(self): + return ASSIGNED_CREDIT_POLICY_TYPE_PRIORITY + + class PerLearnerEnrollmentCreditAccessPolicy(CreditPolicyMixin, SubsidyAccessPolicy): """ Policy that limits the number of enrollments transactions for a learner in a subsidy. @@ -1273,7 +1284,7 @@ def remaining_balance_per_user(self, lms_user_id=None): return self.per_learner_spend_limit - positive_spent_amount -class AssignedLearnerCreditAccessPolicy(CreditPolicyMixin, SubsidyAccessPolicy): +class AssignedLearnerCreditAccessPolicy(AssignedCreditPolicyMixin, SubsidyAccessPolicy): """ Policy based on LearnerContentAssignments, backed by a learner credit type of subsidy. @@ -1737,7 +1748,7 @@ def create_assignment(self): assignment_configuration.enterprise_customer_uuid, self.course_run_key, ) - course_key = content_metadata.get('content_key') + content_key = content_metadata.get('content_key') user_record = User.objects.filter(lms_user_id=self.lms_user_id).first() if not user_record: raise Exception(f'No email could be found for lms_user_id {self.lms_user_id}') @@ -1745,7 +1756,7 @@ def create_assignment(self): return assignments_api.allocate_assignments( assignment_configuration, [user_record.email], - course_key, + content_key, self.content_price_cents, ) diff --git a/enterprise_access/apps/subsidy_access_policy/tests/test_models.py b/enterprise_access/apps/subsidy_access_policy/tests/test_models.py index eef43c77..a0c1a587 100644 --- a/enterprise_access/apps/subsidy_access_policy/tests/test_models.py +++ b/enterprise_access/apps/subsidy_access_policy/tests/test_models.py @@ -3,7 +3,7 @@ """ import contextlib from datetime import datetime, timedelta -from unittest.mock import ANY, PropertyMock, patch +from unittest.mock import ANY, patch from uuid import uuid4 import ddt @@ -900,6 +900,7 @@ def setUp(self): self.policy_two = PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory.create() self.policy_three = PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory.create() self.policy_four = PerLearnerSpendCapLearnerCreditAccessPolicyFactory.create() + self.policy_five = AssignedLearnerCreditAccessPolicyFactory.create() policy_one_subsidy_patcher = patch.object( self.policy_one, 'subsidy_record' @@ -925,10 +926,17 @@ def setUp(self): self.mock_policy_four_subsidy_record = policy_four_subsidy_patcher.start() self.mock_policy_four_subsidy_record.return_value = self.mock_subsidy_four + policy_five_subsidy_patcher = patch.object( + self.policy_five, 'subsidy_record' + ) + self.mock_policy_five_subsidy_record = policy_five_subsidy_patcher.start() + self.mock_policy_five_subsidy_record.return_value = self.mock_subsidy_one + self.addCleanup(policy_one_subsidy_patcher.stop) self.addCleanup(policy_two_subsidy_patcher.stop) self.addCleanup(policy_three_subsidy_patcher.stop) self.addCleanup(policy_four_subsidy_patcher.stop) + self.addCleanup(policy_five_subsidy_patcher.stop) def test_setup(self): """ @@ -937,6 +945,8 @@ def test_setup(self): assert self.policy_one.subsidy_record() == self.mock_subsidy_one assert self.policy_two.subsidy_record() == self.mock_subsidy_two assert self.policy_three.subsidy_record() == self.mock_subsidy_three + assert self.policy_four.subsidy_record() == self.mock_subsidy_four + assert self.policy_five.subsidy_record() == self.mock_subsidy_one @override_settings(MULTI_POLICY_RESOLUTION_ENABLED=True) def test_resolve_one_policy(self): @@ -965,16 +975,16 @@ def test_resolve_two_policies_by_expiration(self): assert SubsidyAccessPolicy.resolve_policy(policies) == self.policy_one @override_settings(MULTI_POLICY_RESOLUTION_ENABLED=True) - def test_resolve_two_policies_by_type_priority(self): + def test_resolve_policies_by_type_priority(self): """ Test resolve given a two policies with same balances, same expiration but different type-priority. """ - policies = [self.policy_four, self.policy_one] - # artificially set the priority attribute higher on one of the policies (lower priority takes precedent) - with patch.object(PerLearnerSpendCreditAccessPolicy, 'priority', new_callable=PropertyMock) as mock: - mock.return_value = 100 - assert SubsidyAccessPolicy.resolve_policy(policies) == self.policy_one + policies = [self.policy_one, self.policy_four, self.policy_five] + # Assert the AssignedLearnerCreditAccessPolicy is resolved with highest + # priority, regardless of the ordering of the input policies. + assert SubsidyAccessPolicy.resolve_policy(policies) == self.policy_five + assert SubsidyAccessPolicy.resolve_policy(list(reversed(policies))) == self.policy_five @ddt.ddt