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

Show file tree
Hide file tree
Changes from all commits
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
48 changes: 46 additions & 2 deletions ecommerce/extensions/fulfillment/tests/test_modules.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,10 +672,42 @@ def format_hubspot_request_url(self):
settings.HUBSPOT_PORTAL_ID,
settings.HUBSPOT_SALES_LEAD_FORM_GUID)

def create_mobile_seat_for_course(self, sku_prefix):
""" Create a mobile seat for a course given the sku_prefix """
web_seat = Product.objects.filter(
parent__isnull=False,
course=self.course,
attributes__name="id_verification_required",
parent__product_class__name="Seat"
).first()
web_stock_record = web_seat.stockrecords.first()

mobile_seat = Product.objects.create(
course=self.course,
parent=web_seat.parent,
structure=web_seat.structure,
expires=web_seat.expires,
is_public=web_seat.is_public,
title="{} {}".format(sku_prefix.capitalize(), web_seat.title.lower())
)

mobile_seat.attr.certificate_type = web_seat.attr.certificate_type
mobile_seat.attr.course_key = web_seat.attr.course_key
mobile_seat.attr.id_verification_required = web_seat.attr.id_verification_required
mobile_seat.attr.save()

StockRecord.objects.create(
partner=web_stock_record.partner,
product=mobile_seat,
partner_sku="mobile.{}.{}".format(sku_prefix.lower(), web_stock_record.partner_sku.lower()),
price_currency=web_stock_record.price_currency,
)
return mobile_seat

def setUp(self):
super(EnrollmentCodeFulfillmentModuleTests, self).setUp()
course = CourseFactory(partner=self.partner)
course.create_or_update_seat('verified', True, 50, create_enrollment_code=True)
self.course = CourseFactory(partner=self.partner)
self.course.create_or_update_seat('verified', True, 50, create_enrollment_code=True)
enrollment_code = Product.objects.get(product_class__name=ENROLLMENT_CODE_PRODUCT_CLASS_NAME)
user = UserFactory()
basket = factories.BasketFactory(owner=user, site=self.site)
Expand Down Expand Up @@ -709,6 +741,18 @@ def test_fulfill_product(self):
self.assertEqual(OrderLineVouchers.objects.first().vouchers.count(), self.QUANTITY)
self.assertIsNotNone(OrderLineVouchers.objects.first().vouchers.first().benefit.range.catalog)

def test_fulfill_product_with_existing_mobile_seats(self):
"""Test fulfilling an Enrollment code product with mobile seats for the same course."""
self.assertEqual(OrderLineVouchers.objects.count(), 0)
lines = self.order.lines.all()
self.create_mobile_seat_for_course('android')
self.create_mobile_seat_for_course('ios')
__, completed_lines = EnrollmentCodeFulfillmentModule().fulfill_product(self.order, lines)
self.assertEqual(completed_lines[0].status, LINE.COMPLETE)
self.assertEqual(OrderLineVouchers.objects.count(), 1)
self.assertEqual(OrderLineVouchers.objects.first().vouchers.count(), self.QUANTITY)
self.assertIsNotNone(OrderLineVouchers.objects.first().vouchers.first().benefit.range.catalog)

def test_revoke_line(self):
line = self.order.lines.first()
with self.assertRaises(NotImplementedError):
Expand Down
Loading