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.

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: list_price is always populated in can_redeem #648

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pwnage101
Copy link
Contributor

@pwnage101 pwnage101 commented Feb 25, 2025

This basically adds a fallback branch when calculating the list_price to serialize into the can-redeem endpoint. The fallback logic relies on a net new call to enterprise-catalog to fetch content metadata containing price information agnostic of any catalog or customer.

Screenshot 2025-02-26 at 4 50 18 PM

ENT-9660

Local Dev Testing

For this manual test, I created an learner credit plan with only an assignable budget, but no assignments.

GET /api/v1/policy-redemption/enterprise-customer/298629a4-0fbf-44db-bcfb-9c604eca0bc8/can-redeem/?content_key=course-v1%3AedX%2BDemoX%2BDemo_Course
can-redeem response BEFORE commit has null list_price
[
    {
        "content_key": "course-v1:edX+DemoX+Demo_Course",
        "list_price": null,
        "redemptions": [],
        "has_successful_redemption": false,
        "redeemable_subsidy_access_policy": null,
        "can_redeem": false,
        "reasons": [
            {
                "reason": "reason_learner_not_assigned_content",
                "user_message": "You can't enroll right now because this course is not assigned to you.",
                "policy_uuids": [
                    "8b460a30-ff5d-4810-8b56-18689677d672"
                ],
                "metadata": {
                    "enterprise_administrators": [
                        {
                            "email": "[[email protected]](mailto:[email protected])",
                            "lms_user_id": 3
                        }
                    ]
                }
            }
        ]
    }
]
can-redeem response AFTER commit has populated list_price
[
    {
        "content_key": "course-v1:edX+DemoX+Demo_Course",
        "list_price": {
            "usd": 149.0,
            "usd_cents": 14900
        },
        "redemptions": [],
        "has_successful_redemption": false,
        "redeemable_subsidy_access_policy": null,
        "can_redeem": false,
        "reasons": [
            {
                "reason": "reason_learner_not_assigned_content",
                "user_message": "You can't enroll right now because this course is not assigned to you.",
                "policy_uuids": [
                    "8b460a30-ff5d-4810-8b56-18689677d672"
                ],
                "metadata": {
                    "enterprise_administrators": [
                        {
                            "email": "[[email protected]](mailto:[email protected])",
                            "lms_user_id": 3
                        }
                    ]
                }
            }
        ]
    }
]

Stage testing plan

This URL currently returns null list_price, but it should become non-null after merging:

https://enterprise-access.stage.edx.org/api/v1/policy-redemption/enterprise-customer/b705ea3b-f2ab-484f-83b9-78cf488f1704/can-redeem/?content_key=course-v1%3Aedx%2B101010%2B1T2025

Prod testing plan

This URL currently returns null list_price, but it should become non-null after merging:

https://enterprise-access.edx.org/api/v1/policy-redemption/enterprise-customer/89cbcdd9-c3cd-4dd7-8a33-2034b50b240e/can-redeem/?content_key=course-v1%3AUniversityofCambridge%2BSUP%2B3T2025

@pwnage101 pwnage101 force-pushed the pwnage101/ENT-9660 branch 5 times, most recently from 243db0e to 0d702b3 Compare February 27, 2025 00:29
# should not contain duplicates.
matching_policy = matching_policies[0] if matching_policies else None
if matching_policy:
result[content_key][matching_policy].append(redemption)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I slightly refactored this function to return different mapping:

  • old: dict(content_key -> dict(policy UUID -> list of redemptions)
  • new: dict(content_key -> dict(policy instance -> list of redemptions)

The purpose is to avoid another DB call in downstream code to fetch the policy instance. We already pulled it into memory here in this function, so just don't discard it yet.

Comment on lines -21 to +27
customer_uuid = uuid4()
catalog_uuid = uuid4()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A repetitive change in this file is replacing customer_uuid with catalog_uuid. It was just wrong before.

"""
Helper to isolate the task of fetching content metadata via our client.
"""
client = EnterpriseCatalogApiClient()
try:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAICT, this try was useless because the else block was literally just raise exc.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, given the caller(s) of this method's parent don't seem to try/except the re-raised exc.

@pwnage101 pwnage101 marked this pull request as ready for review February 27, 2025 00:52
"""
Helper to isolate the task of fetching content metadata via our client.
"""
client = EnterpriseCatalogApiClient()
try:
Copy link
Member

Choose a reason for hiding this comment

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

Agreed, given the caller(s) of this method's parent don't seem to try/except the re-raised exc.

@pwnage101 pwnage101 force-pushed the pwnage101/ENT-9660 branch 5 times, most recently from 85e2497 to a749cdc Compare February 28, 2025 17:40
Copy link
Member

@adamstankiewicz adamstankiewicz left a comment

Choose a reason for hiding this comment

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

LGTM! Just left a handful of non-blocking nits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants