SONIC-704: Add CT discount availed check for outline tab #303
Conversation
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| result = CheckFirstTimeDiscountEligibility.run_filter(email=email) | ||
|
|
||
| output = { | ||
| "is_eligible": result.get('is_eligible', True) |
There was a problem hiding this comment.
should the default value be True?
There was a problem hiding this comment.
For the default case, the user is going to be eligible for the first-time discount.
|
|
||
| discounted_orders = self.base_client.orders.query( | ||
| where=[ | ||
| "customerEmail=:email", |
There was a problem hiding this comment.
We can maybe save two API calls from being made here.
First thought on my mind was to use reference expansion which I couldn't find any for discounts. I think you have also checked this.
The second option here is to store the discount IDs themselves in settings/edx-internal.
We are already following this pattern for line_item_transition: Reference
It is not normally suggested to store the database identifiers like this but as long as the discounts remain the same, the ids/keys are going to be the same so we can minimize the extra API call here which makes more sense to me.
@shafqatfarhan to weigh in on this.
There was a problem hiding this comment.
For the first point, we cannot query expanded objects. For the second option, I wanted the identifiers to be consistent across all environments, which is why I suggested using discount codes, as storing IDs didn’t seem ideal. However, if we're okay with this approach, we can certainly save an API call here
| "orderState=:orderState", | ||
| "discountCodes(discountCode(id in :discountIds))" | ||
| ], | ||
| predicate_var={'email': email, 'discountIds': discount_ids, 'orderState': 'Complete'} |
There was a problem hiding this comment.
Just making sure that we don't want to cater for refunded orders here right?
There was a problem hiding this comment.
Refunded orders are not handled by CT itself, and if I remember correctly, we had to check for their first usage only. Correct me if I am wrong
Merge checklist:
Check off if complete or not applicable:
Post-merge: