Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

fix: Enable enrollment code purchase with mobile seats #4130

Merged
merged 2 commits into from
Feb 16, 2024
Merged

fix: Enable enrollment code purchase with mobile seats #4130

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3973965
fix: Enable enrollment code purchase with mobile seats
Feb 14, 2024
265befb
test: Add unit test
Feb 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ecommerce/extensions/fulfillment/modules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import requests
import waffle
from django.conf import settings
from django.db.models import Q
from django.urls import reverse
from getsmarter_api_clients.geag import GetSmarterEnterpriseApiClient
from oscar.core.loading import get_model
Expand Down Expand Up @@ -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"),
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to what Dawoud says. we should have a unit test accompany this change

Copy link
Contributor

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

great, thank you for answering my questions. in that case, this looks good to me. thank you for adding the test

attributes__name='certificate_type',
attribute_values__value_text=line.product.attr.seat_type
)
Expand Down
Loading