Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: support run-based assignments in can-redeem; ensure assigned policies have highest priority #562

Merged
merged 3 commits into from
Sep 13, 2024
Merged

feat: support run-based assignments in can-redeem; ensure assigned policies have highest priority #562

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
90a5b5e
feat: support run-based assignments in can-redeem; assigned policies …
adamstankiewicz Sep 13, 2024
a12390c
fix: only normalize course key if no assignment direct lookup is poss…
adamstankiewicz Sep 13, 2024
a77a2d6
chore: catch-log-raise LearnerContentAssignment.MultipleObjectsReturned
adamstankiewicz Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 37 additions & 15 deletions enterprise_access/apps/content_assignments/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -200,19 +201,40 @@ 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,
)
Comment on lines +209 to +213
Copy link
Contributor

@pwnage101 pwnage101 Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For completeness, should we catch-log-raise the "multiple records returned" exception too? I'm imagining the only time we'd see this is if there's simultaneously a legacy and new assignment for the same course & learner, which shouldn't happen because our business logic should prevent it. That said, explicitly handling the exception might answer any questions from future code readers about why it's not handled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pwnage101 Updated to include an except block as follows:

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

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}',
)

# 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
adamstankiewicz marked this conversation as resolved.
Show resolved Hide resolved
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


Expand Down Expand Up @@ -245,7 +267,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.

Expand Down Expand Up @@ -446,7 +468,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``,
Expand Down
107 changes: 99 additions & 8 deletions enterprise_access/apps/content_assignments/tests/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
)
Expand Down
1 change: 1 addition & 0 deletions enterprise_access/apps/subsidy_access_policy/constants.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
17 changes: 14 additions & 3 deletions enterprise_access/apps/subsidy_access_policy/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.

Expand Down Expand Up @@ -1737,15 +1748,15 @@ 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}')

return assignments_api.allocate_assignments(
assignment_configuration,
[user_record.email],
course_key,
content_key,
self.content_price_cents,
)

Expand Down
24 changes: 17 additions & 7 deletions enterprise_access/apps/subsidy_access_policy/tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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'
Expand All @@ -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):
"""
Expand All @@ -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):
Expand Down Expand Up @@ -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
Expand Down
Loading