-
Notifications
You must be signed in to change notification settings - Fork 9
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.
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
feat: support run-based assignments in can-redeem
; ensure assigned policies have highest priority
#562
feat: support run-based assignments in can-redeem
; ensure assigned policies have highest priority
#562
Conversation
can-redeem
; ensure assigned policies have highest prioritycan-redeem
; ensure assigned policies have highest priority
9c1a368
to
e1d3961
Compare
…have highest priority
e1d3961
to
90a5b5e
Compare
return queryset.get( | ||
Q(content_key=content_key) | Q(parent_content_key=content_key), | ||
assignment_configuration=assignment_configuration, | ||
lms_user_id=lms_user_id, | ||
) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just nits!
Description
Make run-based assignments discoverable in
can-redeem
In
get_assignment_for_learner
, we perform a lookup of assignments based on a top-level course key, even for run-based assignments. Given this, a run-based assignment would never be found. To mitigate, the querysetget
now considered the assignment'sparent_content_key
as well.Ensuring assignment-based policies have highest priority
When a learner can redeem with both a
PerLearnerEnrollmentCreditAccessPolicy
and aAssignedLearnerCreditAccessPolicy
, thecan-redeem
API response does not prioritize the assignment-based policy for redemption over other policy types.The lack of priority for assignment-based policies was noticed when performing stage QA and observed that the learner portal Course page was suggesting the course was assigned (via a badge and the assignment-specific price messaging), but the redemption was attempting to go through the
PerLearnerEnrollmentCreditAccessPolicy
instead of the expectedAssignedLearnerCreditAccessPolicy
, not quite matching the UX.Jira:
N/A
Merge checklist:
./manage.py makemigrations
has been runPost merge: