From 153dbf6dc663dae192713c8dfc17817f3d91d142 Mon Sep 17 00:00:00 2001 From: James Kachel Date: Fri, 14 Feb 2025 15:21:50 -0600 Subject: [PATCH] Minor fixes for the regular payment workflow (#216) * Some fixes for the regular payment workflow Should hopefully be cherry pick-able out into its own PR. Fixes for a few things that were problematic while getting stuff ready for refunds: - Zero-value baskets caused errors on checkout - the code was expecting to redirect, we need to send status back to the React app instead - import_product and update_product_data commands both didn't manage product versions correctly - for update_product_data, this fix is in the underlying API call that gets queued - refactored PendingOrder._get_or_create a bit to check to make sure the lines and versions match, or fail (and to move checking into a separate function to keep Ruff happy) - Added a BasketItem inline so you can see what's in the basket in Django Admin more easily * Additional fixes - Fix some tests with payment APIs - Add hard system link to Orders - we're doing this in Basket anyway --- payments/admin.py | 27 +++ payments/api.py | 13 +- payments/api_test.py | 9 +- payments/factories.py | 1 + .../0015_order_integrated_system.py | 25 +++ payments/models.py | 184 +++++++++++------- payments/views/v0/__init__.py | 5 +- system_meta/api.py | 38 ++-- system_meta/fixtures.py | 11 ++ .../management/commands/import_product.py | 20 +- 10 files changed, 221 insertions(+), 112 deletions(-) create mode 100644 payments/migrations/0015_order_integrated_system.py create mode 100644 system_meta/fixtures.py diff --git a/payments/admin.py b/payments/admin.py index aee36c3d..9df3f610 100644 --- a/payments/admin.py +++ b/payments/admin.py @@ -9,6 +9,30 @@ from payments import models +class BasketItemInline(admin.TabularInline): + """Inline editor for lines""" + + model = models.BasketItem + list_display = [ + "product", + "quantity", + "base_price", + "discounted_price", + "tax_money", + "total_price", + ] + readonly_fields = [ + "product", + "quantity", + "base_price", + "discounted_price", + "tax_money", + "total_price", + ] + min_num = 0 + extra = 0 + + @admin.register(models.Basket) class BasketAdmin(VersionAdmin): """Admin for Basket""" @@ -17,6 +41,9 @@ class BasketAdmin(VersionAdmin): search_fields = ["user__email", "user__username"] list_display = ["id", "user"] raw_id_fields = ("user",) + inlines = [ + BasketItemInline, + ] @admin.register(models.BasketItem) diff --git a/payments/api.py b/payments/api.py index 265cdb5e..8aded70e 100644 --- a/payments/api.py +++ b/payments/api.py @@ -62,10 +62,8 @@ REDEMPTION_TYPE_ONE_TIME_PER_USER, REDEMPTION_TYPE_UNLIMITED, REFUND_SUCCESS_STATES, - USER_MSG_TYPE_PAYMENT_ACCEPTED_NOVALUE, ZERO_PAYMENT_DATA, ) -from unified_ecommerce.utils import redirect_with_user_message from users.api import determine_user_location, get_flagged_countries log = logging.getLogger(__name__) @@ -76,12 +74,12 @@ def generate_checkout_payload(request, system): """Generate the payload to send to the payment gateway.""" basket = Basket.establish_basket(request, system) + log.debug("established basket has %s lines", basket.basket_items.count()) + # Notes for future implementation: this used to check for - # * ~~Blocked products (by country)~~ (now handled in the basket_add hook) # * Re-purchases of the same product # * Purchasing a product that is expired # These are all cleared for now, but will need to go back here later. - # For completeness, this is also where discounts were checked for validity. order = PendingOrder.create_from_basket(basket) total_price = 0 @@ -121,13 +119,6 @@ def generate_checkout_payload(request, system): ) return { "no_checkout": True, - "response": redirect_with_user_message( - reverse("cart"), - { - "type": USER_MSG_TYPE_PAYMENT_ACCEPTED_NOVALUE, - "run": order.lines.first().purchased_object.course.title, - }, - ), } callback_uri = request.build_absolute_uri(reverse("v0:checkout-result-callback")) diff --git a/payments/api_test.py b/payments/api_test.py index fbc88560..32c26c3a 100644 --- a/payments/api_test.py +++ b/payments/api_test.py @@ -49,7 +49,7 @@ WebhookBasketAction, WebhookOrder, ) -from system_meta.factories import ProductFactory +from system_meta.factories import IntegratedSystemFactory, ProductFactory from system_meta.models import IntegratedSystem from unified_ecommerce.constants import ( DISCOUNT_TYPE_DOLLARS_OFF, @@ -664,6 +664,10 @@ def test_pre_sale_webhook(mocker, user, products): def test_post_sale_webhook_multisystem(mocker, fulfilled_complete_order, source): """Test fire the post-sale webhook with an order with lines from >1 system.""" + # Create an integrated system out of band to make sure this is actually + # getting called correctly. + _ = IntegratedSystemFactory.create() + with reversion.create_revision(): product = ProductFactory.create() @@ -688,6 +692,9 @@ def test_post_sale_webhook_multisystem(mocker, fulfilled_complete_order, source) ], ) + if len(order_info.lines) == 0: + continue + webhook_data = WebhookBase( type=PAYMENT_HOOK_ACTION_POST_SALE, system_slug=system.slug, diff --git a/payments/factories.py b/payments/factories.py index eafd8b23..aa733e2a 100644 --- a/payments/factories.py +++ b/payments/factories.py @@ -40,6 +40,7 @@ class OrderFactory(DjangoModelFactory): total_price_paid = fuzzy.FuzzyDecimal(10.00, 10.00) purchaser = SubFactory(UserFactory) + integrated_system = SubFactory(IntegratedSystemFactory) class Meta: """Meta options for BasketFactory""" diff --git a/payments/migrations/0015_order_integrated_system.py b/payments/migrations/0015_order_integrated_system.py new file mode 100644 index 00000000..d0b59214 --- /dev/null +++ b/payments/migrations/0015_order_integrated_system.py @@ -0,0 +1,25 @@ +# Generated by Django 4.2.18 on 2025-02-06 20:23 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("system_meta", "0009_product_details_url"), + ("payments", "0014_alter_discount_payment_type"), + ] + + operations = [ + migrations.AddField( + model_name="order", + name="integrated_system", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="+", + to="system_meta.integratedsystem", + ), + ), + ] diff --git a/payments/models.py b/payments/models.py index 0263fd39..826cc040 100644 --- a/payments/models.py +++ b/payments/models.py @@ -654,6 +654,13 @@ class STATE: ] state = models.CharField(default=STATE.PENDING, choices=STATE.choices) + integrated_system = models.ForeignKey( + IntegratedSystem, + on_delete=models.PROTECT, + related_name="+", + null=True, + blank=True, + ) purchaser = models.ForeignKey( settings.AUTH_USER_MODEL, on_delete=models.CASCADE, @@ -871,6 +878,40 @@ def delete_redeemed_discounts(self): class PendingOrder(Order): """An order that is pending payment""" + def _process_basket_products(self, basket): + """Process the basket products.""" + + products = basket.get_products() + # Get the details from each Product. + product_versions = [ + Version.objects.get_for_object(product).first() for product in products + ] + + if len(product_versions) == 0: + log.error( + ( + "PendingOrder._get_or_create: %s products are there " + "but no versions?" + ), + len(products), + ) + msg = "No product versions found" + raise ValidationError(msg) + + if len(product_versions) != len(products): + log.error( + ( + "PendingOrder._get_or_create: %s products are there " + "but only %s versions?" + ), + len(products), + len(product_versions), + ) + msg = "Product versions don't match lines added" + raise ValidationError(msg) + + return product_versions + @transaction.atomic def _get_or_create(self, basket: Basket): """ @@ -887,88 +928,77 @@ def _get_or_create(self, basket: Basket): Returns: PendingOrder: the retrieved or created PendingOrder. """ - try: - products = basket.get_products() - # Get the details from each Product. - product_versions = [ - Version.objects.get_for_object(product).first() for product in products - ] - - # Get or create a PendingOrder - # TODO: we prefetched the discounts here - orders = Order.objects.select_for_update().filter( - lines__product_version__in=product_versions, + product_versions = self._process_basket_products(self, basket) + + # Get or create a PendingOrder + orders = Order.objects.select_for_update().filter( + lines__product_version__in=product_versions, + state=Order.STATE.PENDING, + purchaser=basket.user, + integrated_system=basket.integrated_system, + ) + # Previously, multiple PendingOrders could be created for a single user + # for the same product, if multiple exist, grab the first. + if orders: + order = orders.first() + # TODO: this should clear discounts from the order here + + order.refresh_from_db() + else: + order = Order.objects.create( state=Order.STATE.PENDING, purchaser=basket.user, + integrated_system=basket.integrated_system, + total_price_paid=0, ) - # Previously, multiple PendingOrders could be created for a single user - # for the same product, if multiple exist, grab the first. - if orders: - order = orders.first() - # TODO: this should clear discounts from the order here - - order.refresh_from_db() - else: - order = Order.objects.create( - state=Order.STATE.PENDING, - purchaser=basket.user, - total_price_paid=0, - ) - # TODO: Apply any discounts to the PendingOrder + # TODO: Apply any discounts to the PendingOrder - # Manually set these so that we get updated data if it changes - # (this re-uses a PendingOrder if it exists, so it might now be wrong) - order.tax_rate = basket.tax_rate - order.purchaser_ip = basket.user_ip - order.purchaser_taxable_country_code = ( - basket.user_taxable_country_code - if basket.user_taxable_country_code - else "" - ) - order.purchaser_taxable_geolocation_type = ( - basket.user_taxable_geolocation_type + # Manually set these so that we get updated data if it changes + # (this re-uses a PendingOrder if it exists, so it might now be wrong) + order.tax_rate = basket.tax_rate + order.purchaser_ip = basket.user_ip + order.purchaser_taxable_country_code = ( + basket.user_taxable_country_code if basket.user_taxable_country_code else "" + ) + order.purchaser_taxable_geolocation_type = basket.user_taxable_geolocation_type + order.purchaser_blockable_country_code = ( + basket.user_blockable_country_code + if basket.user_blockable_country_code + else "" + ) + order.purchaser_blockable_geolocation_type = ( + basket.user_blockable_geolocation_type + ) + order.save() + + # Create or get Line for each product. + # Calculate the Order total based on Lines and discount. + total = Decimal(0) + used_discounts = [] + for product_version in product_versions: + basket_item = basket.basket_items.get( + product=product_version.field_dict["id"] ) - order.purchaser_blockable_country_code = ( - basket.user_blockable_country_code - if basket.user_blockable_country_code - else "" + line, created = order.lines.get_or_create( + order=order, + product_version=product_version, + defaults={ + "quantity": 1, + "discounted_price": basket_item.discounted_price, + }, ) - order.purchaser_blockable_geolocation_type = ( - basket.user_blockable_geolocation_type + used_discounts.append(basket_item.best_discount_for_item_from_basket) + total += line.total_price_money + log.debug( + "%s line %s product %s", + ("Created" if created else "Updated"), + line, + product_version.field_dict["sku"], ) - order.save() - - # Create or get Line for each product. - # Calculate the Order total based on Lines and discount. - total = Decimal(0) - used_discounts = [] - for product_version in product_versions: - basket_item = basket.basket_items.get( - product=product_version.field_dict["id"] - ) - line, created = order.lines.get_or_create( - order=order, - product_version=product_version, - defaults={ - "quantity": 1, - "discounted_price": basket_item.discounted_price, - }, - ) - used_discounts.append(basket_item.best_discount_for_item_from_basket) - total += line.total_price_money - log.debug( - "%s line %s product %s", - ("Created" if created else "Updated"), - line, - product_version.field_dict["sku"], - ) - line.save() + line.save() - order.total_price_paid = total - - except Exception: # pylint: disable=broad-except # noqa: BLE001 - order.state = Order.STATE.ERRORED + order.total_price_paid = total order.save() @@ -1259,6 +1289,14 @@ def product(self) -> Product: self.product_version, ) + @staticmethod + def from_product(product: Product, **kwargs): + """Create a Line for the most recent version of the product.""" + + kwargs["product_version"] = Version.objects.get_for_object(product).first() + + return Line(**kwargs) + def __str__(self): """Return string version of the line.""" return f"{self.product_version}" diff --git a/payments/views/v0/__init__.py b/payments/views/v0/__init__.py index 61c8a2e3..cd386c62 100644 --- a/payments/views/v0/__init__.py +++ b/payments/views/v0/__init__.py @@ -407,13 +407,16 @@ def start_checkout(request, system_slug: str): if ( "country_blocked" in payload - or "no_checkout" in payload or "purchased_same_courserun" in payload or "purchased_non_upgradeable_courserun" in payload or "invalid_discounts" in payload ): return Response(payload["response"], status=status.HTTP_406_NOT_ACCEPTABLE) + if "no_checkout" in payload: + # The user doesn't have to check out - we're done here. + return Response(payload, status=status.HTTP_201_CREATED) + return Response(CyberSourceCheckoutSerializer(payload).data) diff --git a/system_meta/api.py b/system_meta/api.py index 1dda8e36..91c1a8f4 100644 --- a/system_meta/api.py +++ b/system_meta/api.py @@ -4,6 +4,7 @@ import requests from django.conf import settings +from reversion import create_revision from system_meta.models import Product from unified_ecommerce.utils import parse_readable_id @@ -100,22 +101,25 @@ def update_product_metadata(product_id: int) -> None: """Get product metadata from the Learn API.""" try: - product = Product.objects.get(id=product_id) - fetched_metadata = get_product_metadata(product.system.slug, product.sku) - - if not fetched_metadata: - log.warning("No Learn results found for product %s", product) - return - - product.image_metadata = ( - fetched_metadata.get("image", None) or product.image_metadata - ) - - product.name = fetched_metadata.get("title", product.name) - product.description = fetched_metadata.get("description", product.description) - product.price = fetched_metadata.get("price", product.price) - product.details_url = fetched_metadata.get("url", product.details_url) - - product.save() + with create_revision(): + product = Product.objects.get(id=product_id) + fetched_metadata = get_product_metadata(product.system.slug, product.sku) + + if not fetched_metadata: + log.warning("No Learn results found for product %s", product) + return + + product.image_metadata = ( + fetched_metadata.get("image", None) or product.image_metadata + ) + + product.name = fetched_metadata.get("title", product.name) + product.description = fetched_metadata.get( + "description", product.description + ) + product.price = fetched_metadata.get("price", product.price) + product.details_url = fetched_metadata.get("url", product.details_url) + + product.save() except requests.RequestException: log.exception("Failed to update metadata for product %s", product.id) diff --git a/system_meta/fixtures.py b/system_meta/fixtures.py new file mode 100644 index 00000000..d93d330a --- /dev/null +++ b/system_meta/fixtures.py @@ -0,0 +1,11 @@ +"""Fixtures for system metadata.""" + +import pytest + +from system_meta.factories import IntegratedSystemFactory + + +@pytest.fixture() +def integrated_system(): + """Create an integrated system.""" + return IntegratedSystemFactory() diff --git a/system_meta/management/commands/import_product.py b/system_meta/management/commands/import_product.py index 1442b19b..0cadf469 100644 --- a/system_meta/management/commands/import_product.py +++ b/system_meta/management/commands/import_product.py @@ -3,6 +3,7 @@ import logging from django.core.management.base import BaseCommand +from reversion import create_revision from system_meta.api import get_product_metadata from system_meta.models import IntegratedSystem, Product @@ -34,15 +35,16 @@ def handle(self, *args, **kwargs): # noqa: ARG002 metadata = get_product_metadata(kwargs["system_slug"], kwargs["readable_id"]) system = IntegratedSystem.objects.get(slug=kwargs["system_slug"]) - product = Product.objects.create( - sku=metadata["sku"], - name=metadata["title"], - description=metadata["description"], - image_metadata=metadata["image"], - price=metadata["price"], - system=system, - details_url=metadata["url"], - ) + with create_revision(): + product = Product.objects.create( + sku=metadata["sku"], + name=metadata["title"], + description=metadata["description"], + image_metadata=metadata["image"], + price=metadata["price"], + system=system, + details_url=metadata["url"], + ) self.stdout.write( self.style.SUCCESS(f"Successfully imported product {product}")