Fix v0/baskets/create_from_product so that baskets get discounts as we expect#3290
Conversation
OpenAPI ChangesShow/hide ## Changes for v0.yaml:Unexpected changes? Ensure your branch is up-to-date with |
c89d0de to
23e2637
Compare
| existing_basket_discounts = [bd.redeemed_discount for bd in basket.discounts.all()] | ||
| discounts_to_apply = [ | ||
| *existing_basket_discounts, | ||
| *list(get_auto_apply_discounts_for_basket(basket.id).all()), |
There was a problem hiding this comment.
Nit: I think the all() is superfluous here since get_auto_apply_discounts_for_basket always kicks back a new, unevaluated queryset.
dsubak
left a comment
There was a problem hiding this comment.
Does what it says on the testing plan. I validated this with James' help by going through the API directly.
It appears that there are some discrepancies between discount application logic here and in the learn cart, so for anyone else who plans on testing this, stick with hitting the API directly.
2d80389 to
07086d7
Compare
|
@dsubak this should be ready for re-review now. I went back and found what documentation is out there for discount assignments, and the ruleset that was here didn't match up with that (which is why the cart page kept moving things around on you). So, I changed the new code so that it matches up. I would like to be able to - in a future PR - strip out the existing discount application stuff out of the The workflow for discount application is:
Codes are applied if:
In the case that there's a user discount and a financial assistance one, the financial assistance discount wins. This does mean that if the user discount is overridden, then it effectively doesn't count anymore and the user can supply a better discount by hand. The discount product bit was also wrong, so that too was corrected - we don't apply discounts automatically based on products. We do gatekeep discounts, though, so we do check to make sure the discount's products are in the basket if it has any. (This still doesn't touch B2B or verified program discounts because they do their own thing.) FWIW, this is the info I found (in chronological order): It'll be a task to get some documentation written for this to consolidate it all into one place (including the special case stuff that we have for B2B and verified programs). |
…I, add more tests
- User discounts are automatically applied to the cart, regardless of the "automatic" flag - The discount/product association is _not_ for automatically applying a discount - it's for validation on application So now get_auto_apply_discounts_for_basket looks for discounts attached to the user, discounts that have the automatic flag set, or applicable financial assistance discounts.
User discounts apply whether or not there are other discounts other than financial aid.
…bably never causing an exception
…naid discount is being applied regardless
7e227de to
bc23e5a
Compare
dsubak
left a comment
There was a problem hiding this comment.
I think we're good! James and I worked through some hiccups where finaid wasn't being selected over user discounts in certain cases, but after his fixes it's behaving as expected.
What are the relevant tickets?
Closes mitodl/hq#10103
Description (What does it do?)
The initial implementation/move of the
v0/baskets/create_from_productAPI was pretty simple. When it created a basket, it was at least able to go try to fetch the applicable automatic discounts for the user and apply them to the basket. However, it did not do this correctly, and would attempt to apply all the auto discounts available to the basket. In Unified Ecommerce, this was fine; MITx Online only allows a single discount per basket, so if there were more than one, it would fail.In working through this, I found too that it wasn't grabbing the financial assistance discounts and applying those, either. These get applied according to their own ruleset, but nevertheless count as "automatic" and should be applied to any normal basket. These would get applied at some point in the process - at worst, when the learner checks out - but they should be applied when the basket is created so the checkout page display has the correct pricing on it.
So, this PR does a handful of things:
get_auto_apply_discounts_for_basketinternal API to also grab financial assistance discounts. (Previously, this didn't consider financial assistance discounts at all.) Additionally, this has been updated to respect the activation and expiration dates on the discount.apply_discount_to_basketinternal API to make sure the discount being applied is better (the logic needed some help)._create_basket_from_productfunction so that it grabs existing applied discounts, applicable automatic discounts (including financial aid), and any discount provided to it, clears any discounts applied to the basket, and applies all of the collected discounts.How can this be tested?
Automated tests should pass.
To test the API itself, you will need a number of things set up:
The regular discounts should not have their Automatic flag set to True. Ideally, start testing with the discounts set to have no activation or expiration dates. All these discounts should ideally be for different amounts. For ease of testing, it's easier to set them all to fixed-price unlimited-use discounts.
The externally accessible part of this code is the
/api/v0/baskets/create_from_productAPI. Test by both adding a product to the basket and adding a product and a discount to the basket.The result of hitting the API should be a basket that has the specified product in it. The discount applied should be:
So, for testing, hit the API after setting the user-attached discount to automatic, then turning it off and setting the product-attached one to automatic, etc. Finally, apply for financial assistance and approve your request to ensure that discount is applied as expected.
Additional Context
This is one of the APis that got moved over from UE - some of the things that MITx Online does UE didn't do by design, like financial assistance. So, this PR is mostly shifting the process that's in the legacy API for discounts into APIs and then calling them from the new
v0API.