Addresses as strings assumption

[Timusan]

This is somewhat related to #29. Currently it is assumed that addresses (billing and shipping) are given as strings. However, in our case (and I boldly assume in many) addresses are stored as address objects with separate components. To store proper address objects on an Order I override the Order model first:

from salesman.orders.models import BaseOrder

class Order(BaseOrder):
    shipping_address = ForeignKey(
        "webshop.OrderAddress",
        null=True,
        blank=True,
        on_delete=SET_NULL,
        related_name="order_shipping_address",
    )
    billing_address = ForeignKey(
        "webshop.OrderAddress",
        null=True,
        blank=True,
        on_delete=SET_NULL,
        related_name="order_billing_address",
    )

Making shipping_address and billing_address foreign keys to the addresses table. Then on checkout I submit them like this:

{
   "email": "[email protected]",
   "shipping_address": {
	"street": "Shippingstreet",
        "house_number": 1,
	"postal_code": "1234",
	"locality": "Some Locality",
	"country": "BE"
   },
   "billing_address": {
	"street": "Billingstreet",
        "house_number": 1,
	"postal_code": "1234",
	"locality": "Some Locality",
	"country": "BE"
   },
  "payment_method": "pay-later",
}

Now to be able for Salesman to handle this I first need to override the Checkout serializer:

from salesman.checkout.serializers import CheckoutSerializer

class CustomCheckoutSerializer(CheckoutSerializer):
    shipping_address = OrderAddressSerializer()
    billing_address = OrderAddressSerializer()
  
    def validate_shipping_address(self, value: OrderAddressSerializer) -> OrderAddressSerializer:
        context = self.context.copy()
        context["address"] = "shipping"
        return validate_address(value, context=context)

    def validate_billing_address(self, value: OrderAddressSerializer) -> OrderAddressSerializer:
        context = self.context.copy()
        context["address"] = "billing"
        return validate_address(value, context=context)

    def save(self, **kwargs: Any) -> None:
        basket, request = self.context["basket"], self.context["request"]
       
       ....

        # Add in address fields.
        basket.extra["shipping_address"] = self.validated_data["shipping_address"]
        basket.extra["billing_address"] =  self.validated_data["billing_address"]

The OrderAddressSerializer is a simple ModelSerializer which serializes the address objects. I have overridden both the validate_shipping_address() and validate_billing_address() functions to accept the object data instead of simple string data. The actual validator function needs to be written as well:

from webshop.serializers.orders.order_address import OrderAddressSerializer

def validate_address(value: OrderAddressSerializer, context: dict[str, Any] = {}) -> str:
    if not value:
        raise ValidationError(_("Address is required."))
    return value

And set this in the config:

SALESMAN_ADDRESS_VALIDATOR = "webshop.utils.validate_address"

And finally I assign the validated data to the extra object on the basket.

Next the populate_from_basket() method on the custom Order model needs to be modified:

  @transaction.atomic
    def populate_from_basket(
        self,
        basket: BaseBasket,
        request: HttpRequest,
        **kwargs: Any,
    ) -> None:
        from webshop.models import OrderAddress
        from salesman.basket.serializers import ExtraRowsField

        if not hasattr(basket, "total"):
            basket.update(request)

        self.user = basket.user
        self.email = basket.extra.pop("email", "")

        self.shipping_address = OrderAddress.objects.create(**basket.extra["shipping_address"])
        self.billing_address = OrderAddress.objects.create(**basket.extra["billing_address"])

Here the data found on the extra object is expanded into actual Address objects and assigned to the Order.

To round things up I need to make sure that Salesman will use my CustomCheckoutSerializer instead of the default. As this is not configurable I am forced to override the routing by defining a new View:

from salesman.checkout.views import CheckoutViewSet
from webshop.serializers.checkout.checkout import CustomCheckoutSerializer

class CustomCheckoutViewSet(SalesmanCheckoutViewSet):
    serializer_class = CustomCheckoutSerializer

And then wire this view into my route:

router.register("checkout", CustomCheckoutViewSet, basename="salesman-checkout")

This feels a bit convoluted, or is this the intended way?

I think making the checkout serializer configurable (eg. SALESMAN_CHECKOUT_SERIALIZER) would at least take care of not having to override the view and routing. But we are still left with the assumption of addresses being strings and a "hacky" way of trying to change this via the extra object on a basket.

[dinoperovic]

Adding SALESMAN_CHECKOUT_SERIALIZER seems like a good idea to me. I will look to add it as part of #29.

For the addresses, I did not include them to keep it simple (hence the strings for addresses) and figured we could pass the shipping_address_id param instead of full text address (which you can validate in SALESMAN_ADDRESS_VALIDATOR and return the address as text if you want).

You would setup a custom endpoint for addresses and handle them separately as this is usually handled by the user/account outside of the order context anyway.

Another reason for not using foreign keys for addresses is that you wish to keep the snapshot of an address that was used during the checkout, in a case that user has changed the address after the order was created. This of course depends on your system requirements.

I was considering adding address as a module to salesman, but also realise that they could be implemented in many different ways and would make it difficult to support.

[Timusan]

It makes sense to keep kind of a "point-in-time" data on the order, which cannot be altered by the customer itself once the order is complete. But I think this should still be something left up to the implementer. Using foreign keys would not negate point-in-time as the addresses referred to by those keys could be made non-manageable.

For example, I have created an OrderAddress model just for this on which the different address components are stored in a single model. An end user cannot edit this. If they would have an address stored on their profile those values would simply be copied over to an OrderAddress during checkout.

Having a shipping_address_id and billing_address_id as separate params in the JSON body would be a nice addition (instead of/or accompanying) full text addresses. Possible together with SALESMAN_ADDRESS_MODEL and/or SALESMAN_ADDRESS_SERIALIZER to point to the custom model/serializer that is used? Address objects are managed outside of Salesman (as this is indeed out of scope).

Also ... thanks a bunch for the awesome work already done on Salesman! And for keeping up with my ramblings ;)

[dinoperovic]

I will look into adding such a feature, as soon as I get some free time :)