fix: restructure can_redeem to gracefully handle error#108
fix: restructure can_redeem to gracefully handle error#108
Conversation
There was a problem hiding this comment.
Pull request overview
This PR restructures list-price fetching and error handling so can_redeem can treat missing catalog/metadata differently depending on whether the learner can redeem vs has already redeemed.
Changes:
SubsidyAccessPolicy.get_list_pricenow wrapsHTTPErrorfrom content-metadata fetch asContentPriceNullException.can_redeemnow uses separate error-handling paths forresolved_policy,successful_redemptions, and the non-redeemable/no-redemption case, avoiding request failure for already-redeemed content with missing catalog entries.- Adds warning logging for the already-redeemed + missing price scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| enterprise_access/apps/subsidy_access_policy/models.py | Normalizes metadata-fetch HTTP failures into ContentPriceNullException for list-price computation. |
| enterprise_access/apps/api/v1/views/subsidy_access_policy.py | Refactors can_redeem list-price logic to avoid failing requests for already-redeemed content when metadata is missing. |
| 'with enterprise customer %s. Content may no longer exist in the catalog.', | ||
| content_key, | ||
| enterprise_customer_uuid, | ||
| ) |
There was a problem hiding this comment.
In the successful_redemptions branch, a ContentPriceNullException now results in list_price_dict staying None (per the comment). However, the response serializer (SubsidyAccessPolicyCanRedeemElementResponseSerializer) defines list_price as a nested ListPriceResponseSerializer without allow_null=True, so returning None here will likely cause serialization errors/500s. Update the serializer to allow null list_price (and ensure the OpenAPI schema reflects this), or return a consistently-shaped list_price object that the serializer can represent.
| ) | |
| ) | |
| # Provide a structurally valid "null" list price so the response serializer | |
| # (which does not allow a null nested object) can still represent it. | |
| list_price_dict = make_list_price_dict(None, None) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #108 +/- ##
==========================================
+ Coverage 84.01% 84.03% +0.01%
==========================================
Files 144 144
Lines 12226 12236 +10
Branches 1170 1170
==========================================
+ Hits 10272 10282 +10
Misses 1625 1625
Partials 329 329 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
brobro10000
left a comment
There was a problem hiding this comment.
Once its merged, validate down stream consumers (course about page) is still receiving the expected messaging related to subsidy type, and catalog inclusion 👍🏽 . I would wait for either @pwnage101 or @iloveagent57 for final approval, but looks good on my end.
| try: | ||
| content_metadata = self.get_content_metadata(content_key) | ||
| except requests.exceptions.HTTPError as exc: | ||
| raise ContentPriceNullException( | ||
| f'Could not fetch content metadata to determine list price for {content_key}' | ||
| ) from exc | ||
| return get_list_price_for_content( | ||
| self.enterprise_customer_uuid, | ||
| content_key, | ||
| self.get_content_metadata(content_key), | ||
| content_metadata, | ||
| ) |
There was a problem hiding this comment.
get_list_price() now normalizes HTTPError into ContentPriceNullException, but get_list_price_for_content(..., content_metadata=...) can still raise KeyError/ValueError (e.g., missing content_price, content_price=None, or non-int types) which will bypass the intended normalized exception path and can surface as a 500 to callers. Consider validating the fetched metadata and/or catching these errors and re-raising ContentPriceNullException so callers only have to handle one failure type.
| except HTTPError: | ||
| # We might normally re-raise the exception here (and have in the past), but this would cause | ||
| # can-redeem requests to fail for courses which contain restricted runs where the customer does | ||
| # not have restricted access. The desired behavior is for the request to NOT fail, and the | ||
| # frontend is already coded to discard the restricted runs. | ||
| logger.warning( | ||
| ( | ||
| 'Failed to obtain content metadata from enterprise-catalog with enterprise customer ' | ||
| '%s. This can happen if the run is restricted.' | ||
| ), | ||
| enterprise_customer_uuid |
There was a problem hiding this comment.
In the HTTPError handler for the enterprise-catalog metadata fetch, the log message drops both content_key and the exception details (except HTTPError: without as exc). This makes it hard to diagnose which content caused the failure and why in production. Consider logging the exception (or exc_info=True) and including content_key in the message/args.
| except HTTPError: | |
| # We might normally re-raise the exception here (and have in the past), but this would cause | |
| # can-redeem requests to fail for courses which contain restricted runs where the customer does | |
| # not have restricted access. The desired behavior is for the request to NOT fail, and the | |
| # frontend is already coded to discard the restricted runs. | |
| logger.warning( | |
| ( | |
| 'Failed to obtain content metadata from enterprise-catalog with enterprise customer ' | |
| '%s. This can happen if the run is restricted.' | |
| ), | |
| enterprise_customer_uuid | |
| except HTTPError as exc: | |
| # We might normally re-raise the exception here (and have in the past), but this would cause | |
| # can-redeem requests to fail for courses which contain restricted runs where the customer does | |
| # not have restricted access. The desired behavior is for the request to NOT fail, and the | |
| # frontend is already coded to discard the restricted runs. | |
| logger.warning( | |
| ( | |
| 'Failed to obtain content metadata from enterprise-catalog for content_key %s with ' | |
| 'enterprise customer %s. This can happen if the run is restricted.' | |
| ), | |
| content_key, | |
| enterprise_customer_uuid, | |
| exc_info=True, |
Description:
models.py: Separate the
get_content_metadatafetch into its own try/except, catching HTTPError and raising ContentPriceNullException instead. This normalizes the exception type for all callers.subsidy_access_policy.py: The original single try/except ContentPriceNullException block covered all three branches (resolved_policy, successful_redemptions, else) and always raised a RedemptionRequestException on failure. For a learner who has already redeemed, a missing catalog entry is a data/catalog mismatch that should not fail the request.
Jira:
ENT-11649
Merge checklist:
./manage.py makemigrationshas been runPost merge: