dinoperovic / django-salesman

Headless e-commerce framework for Django and Wagtail.
https://django-salesman.rtfd.io
BSD 3-Clause "New" or "Revised" License
391 stars 48 forks source link

Inherited PaymentMethod not shown as choice #41

Closed thenewguy closed 7 months ago

thenewguy commented 7 months ago

I was refactoring a payment method and noticed that when I used a base class to hold common method that the payment method choices were no longer presented.

Here is a simple example using the documented payment methods:

Payment Methods:

from django.core.exceptions import ValidationError
from django.urls import reverse

from salesman.checkout.payment import PaymentMethod
from salesman.core.utils import get_salesman_model

Order = get_salesman_model('Order')

class PayOnDelivery(PaymentMethod):
    """
    Payment method that expects payment on delivery.

    Copied from https://github.com/dinoperovic/django-salesman/blob/8105e84c0ccca00f483122b91a74175131808399/example/shop/payment/on_delivery.py#L9
    """

    identifier = "pay-on-delivery"
    label = "Pay on delivery"

    def validate_basket(self, basket, request):
        """
        Payment only available when purchasing 10 items or less.
        """
        super().validate_basket(basket, request)
        if basket.quantity > 10:
            raise ValidationError("Can't pay for more than 10 items on delivery.")

    def basket_payment(self, basket, request):
        """
        Create order and mark it as shipped. Order status should be changed
        to `COMPLETED` and a new payment should be added manually by the merchant
        when the order items are received and paid for by the customer.
        """
        order = Order.objects.create_from_basket(basket, request, status="SHIPPED")
        basket.delete()
        url = reverse("salesman-order-last") + f"?token={order.token}"
        return request.build_absolute_uri(url)

class PayOnDelivery2(PayOnDelivery):
    identifier = "pay-on-delivery-2"
    label = "Pay on delivery 2"

class PayOnDelivery3(PayOnDelivery):
    identifier = "pay-on-delivery-3"
    label = "Pay on delivery 3"

    def basket_payment(self, *args, **kwargs):
        return super().basket_payment(*args, **kwargs)

Setting:

SALESMAN_PAYMENT_METHODS = [
    'path.to.PayOnDelivery',
    'path.to.PayOnDelivery2',
    'path.to.PayOnDelivery3',
]

When selected the payment method, PayOnDelivery2 is not presented as a choice.

dinoperovic commented 7 months ago

@thenewguy this is expected as we currently check for existence of basket_payment or order_payment to determine if PaymentMethod supports paying for basket/order.

So the method needs to be defined in the class registered in SALESMAN_PAYMENT_METHODS.

What you did in PayOnDelivery3 should do it:

def basket_payment(self, *args, **kwargs):
    return super().basket_payment(*args, **kwargs)
thenewguy commented 7 months ago

I've been working my way through a proof of concept. I really like how you've put this together! It offers enough to be very useful without getting in the way =)

However, this issue is not super intuitive and there is no error message or indication of why the payment method isn't shown. It took me a fair amount of time to track down the cause. It certainly wasn't expected that typical class inheritance would be unsupported and I would imagine that I will not be the only one to stumble here.

Is that the only way for the existence check to work? If so, what about a boolean flag instead?