fix: Enable enrollment code purchase with mobile seats #4130
Conversation
| @@ -553,6 +554,7 @@ def fulfill_product(self, order, lines, email_opt_in=False): | |||
| attributes__name='course_key', | |||
| attribute_values__value_text=line.product.attr.course_key | |||
| ).get( | |||
| ~Q(stockrecords__partner_sku__icontains="mobile"), | |||
There was a problem hiding this comment.
It would be good to have a unit test for this. Ideally, this should have been caught on unit test level. But since it was not, better to add it for the future.
There was a problem hiding this comment.
+1 to what Dawoud says. we should have a unit test accompany this change
There was a problem hiding this comment.
follow up question for @moeez96: if i am understanding this, is the only way we know whether or not a seat is mobile is if 'mobile' is in the sku? icontains tends to be a slower operation, which is likely fine given this is done in a job, but wanted to ask to see if there's something other than text search that indicates whether something is a mobile seat
additionally, what was the failure mode that prompted us to make this change? it looks like the Q() predicate you added will filter out mobile seats, which is good, but this get() could still fail if there are multiple objects returned. are you comfortable with this approach? or is there reason for us to be even more defensive here? (i am not familiar enough with Seats to have an definitive opinion, so i'm asking)
There was a problem hiding this comment.
@christopappas That's right. We have a standard set for mobile SKUS format and it must contain the keyword mobile. This standard is integrated with the App Store and Plat Store product identifiers. Unfortunately there is nothing other than text search that indicates a seat is a mobile seat.
About the get(), we can replace this get() with a filter().first() but fulfilling a product requires a seat. If you go ahead in the method, the logic can not put up without a seat object. So if we do not have a seat, we want this to throw out an error/exception rather than fail silently.
There was a problem hiding this comment.
great, thank you for answering my questions. in that case, this looks good to me. thank you for adding the test
| @@ -553,6 +554,7 @@ def fulfill_product(self, order, lines, email_opt_in=False): | |||
| attributes__name='course_key', | |||
| attribute_values__value_text=line.product.attr.course_key | |||
| ).get( | |||
| ~Q(stockrecords__partner_sku__icontains="mobile"), | |||
There was a problem hiding this comment.
+1 to what Dawoud says. we should have a unit test accompany this change
|
@christopappas Unit test added, Please review. |
fix: sorted imports Fix: Support create-mobile-skus to run again if failed (#4129) * fix: Support create-mobile-skus to run again if we found an error previously fix: Enable enrollment code purchase with mobile seats (#4130) * fix: Enable enrollment code purchase with mobile seats * test: Add unit test --------- Co-authored-by: Abdul Moeez Zahid <[email protected]> fix: PEP8 conventions fix: import issue chore: added more test cases fix: added new line at the end of file fix: blank line issue fix: long line issue fix: pylint issues fix: import issues fix: removed extra blank line fix: updated test fix: issues in command fix:: isort issue
* fix: Enable enrollment code purchase with mobile seats * test: Add unit test --------- Co-authored-by: Abdul Moeez Zahid <[email protected]>
* fix: Enable enrollment code purchase with mobile seats * test: Add unit test --------- Co-authored-by: Abdul Moeez Zahid <[email protected]>
* fix: Enable enrollment code purchase with mobile seats * test: Add unit test --------- Co-authored-by: Abdul Moeez Zahid <[email protected]>
* fix: Enable enrollment code purchase with mobile seats * test: Add unit test --------- Co-authored-by: Abdul Moeez Zahid <[email protected]>
…ported#4130) * fix: Enable enrollment code purchase with mobile seats * test: Add unit test --------- Co-authored-by: Abdul Moeez Zahid <[email protected]>
⛔️ MAIN BRANCH WARNING! 2U EMPLOYEES must make branches against the 2u/main BRANCH
⛔️ DEPRECATION WARNING
This repository is deprecated and in maintainence-only operation while we work on a replacement, please see this announcement for more information.
Although we have stopped integrating new contributions, we always appreciate security disclosures and patches sent to [email protected]
Anyone internally merging to this repository is expected to release and monitor their changes; if you are not able to do this DO NOT MERGE, please coordinate with someone who can to ensure that the changes are released.
Description
Enable purchasing enrollment codes in presence of multiple enrollment codes for mobile.
Related Jira links:
https://2u-internal.atlassian.net/browse/REV-3857
https://2u-internal.atlassian.net/browse/LEARNER-9837