proposition of solution on Create Order route

I try to use your proposed request and response serializers for the
creation of an Order.
All the cases keeps the same returned response, except the error on billing_address,
now it return a dict of error and not a single string, because it's a field with multiple
values.
All the old test past (modification of the billing_address error).
The generation of openAPI also works.
The usage of new request serializer add also check on billing_address
and credit_card_id values that was not done before.

I have also a question on the existing code, if paiement fail because we
didn't find the card, we didn't cancel the order ? (see commentary in the code)

Author: MorganeAD

src/backend/joanie/core/api.py

@@ -170,10 +170,14 @@ class OrderViewSet(
     lookup_field = "pk"
     pagination_class = Pagination
     permission_classes = [permissions.IsAuthenticated]
-    serializer_class = serializers.OrderSerializer
     filterset_class = filters.OrderViewSetFilter
     ordering = ["-created_on"]
 
+    def get_serializer_class(self):
+        if self.action == 'create':
+            return serializers.OrderCreateBodySerializer
+        return serializers.OrderSerializer
+
     def get_queryset(self):
         """Custom queryset to limit to orders owned by the logged-in user."""
         user = User.update_or_create_from_request_user(request_user=self.request.user)
@@ -182,7 +186,7 @@ def get_queryset(self):
     def perform_create(self, serializer):
         """Force the order's "owner" field to the logged-in user."""
         owner = User.update_or_create_from_request_user(request_user=self.request.user)
-        serializer.save(owner=owner)
+        return serializer.save(owner=owner)
 
     @swagger_auto_schema(
         request_body=serializers.OrderCreateBodySerializer,
@@ -192,37 +196,19 @@ def perform_create(self, serializer):
     def create(self, request, *args, **kwargs):
         """Try to create an order and a related payment if the payment is fee."""
         serializer = self.get_serializer(data=request.data)
+
+        # - Validate data
         if not serializer.is_valid():
             return Response(serializer.errors, status=400)
 
         product = serializer.validated_data.get("product")
         course = serializer.validated_data.get("course")
         billing_address = serializer.validated_data.get("billing_address")
+        credit_card_id = serializer.validated_data.get("credit_card_id")
 
-        # Populate organization field if it is not set and there is only one
-        # on the product
-        if not serializer.validated_data.get("organization"):
-            try:
-                organization = product.course_relations.get(
-                    course=course
-                ).organizations.get()
-            except (
-                models.Course.DoesNotExist,
-                models.Organization.DoesNotExist,
-                models.Organization.MultipleObjectsReturned,
-            ):
-                pass
-            else:
-                serializer.validated_data["organization"] = organization
-
-        # If product is not free, we have to create a payment.
-        # To create one, a billing address is mandatory
-        if product.price.amount > 0 and not billing_address:
-            return Response({"billing_address": "This field is required."}, status=400)
-
-        # - Validate data then create an order
+        # then create an order
         try:
-            self.perform_create(serializer)
+            order = self.perform_create(serializer)
         except (DRFValidationError, IntegrityError):
             return Response(
                 (
@@ -234,9 +220,7 @@ def create(self, request, *args, **kwargs):
 
         # Once order has been created, if product is not free, create a payment
         if product.price.amount > 0:
-            order = serializer.instance
             payment_backend = get_payment_backend()
-            credit_card_id = serializer.initial_data.get("credit_card_id")
 
             # if payment in one click
             if credit_card_id:
@@ -251,25 +235,20 @@ def create(self, request, *args, **kwargs):
                         credit_card_token=credit_card.token,
                     )
                 except (CreditCard.DoesNotExist, NotImplementedError):
+                    # TODO question: if paiement fail because we didn't find the card, we didn't cancel the order ?
                     pass
             else:
                 payment_info = payment_backend.create_payment(
                     request=request, order=order, billing_address=billing_address
                 )
 
             # Return the fresh new order with payment_info
+            order.payment_info = payment_info
             return Response(
-                # FIXME: this work but we'll need to make several change in test and clients
-                serializers.OrderCreateResponseSerializer(
-                    {
-                        "order": serializer.data,
-                        "payment_info": payment_info,
-                    }
-                ).data,
-                status=201,
+                serializers.OrderCreateResponseSerializer(order).data, status=201
             )
         # Else return the fresh new order
-        return Response(serializer.data, status=201)
+        return Response(serializers.OrderCreateResponseSerializer(order).data, status=201)
 
     @swagger_auto_schema(
         request_body=serializers.OrderAbortBodySerializer,

src/backend/joanie/core/serializers/model_serializers.py

@@ -423,7 +423,6 @@ class OrderSerializer(serializers.ModelSerializer):
     Order model serializer
     """
 
-    id = serializers.CharField()
     owner = serializers.CharField(
         source="owner.username", read_only=True, required=False
     )
@@ -470,7 +469,7 @@ class Meta:
             "certificate",
             "created_on",
             "enrollments",
-            # "id",
+            "id",
             "main_invoice",
             "owner",
             "total",

src/backend/joanie/core/serializers/order_create_body_serializer.py

@@ -1,15 +1,45 @@
 """Serializers for core.api.OrderViewSet.create Body"""
-
 from rest_framework import serializers
+from joanie.core import models
+from .model_serializers import OrderSerializer, AddressSerializer
+
+
+class OrderCreateBodySerializer(OrderSerializer):
+    billing_address = AddressSerializer(required=False)
+    credit_card_id = serializers.UUIDField(required=False)
 
-from .model_serializers import OrderSerializer, OrderCreateSerializer, AddressSerializer
+    class Meta(OrderSerializer.Meta):
+        fields = OrderSerializer.Meta.fields + ["billing_address", "credit_card_id"]
+        read_only_fields = OrderSerializer.Meta.fields + ["billing_address", "credit_card_id"]
 
+    def validate(self, data):
+        cleaned_data = super().validate(data)
+        # Populate organization field if it is not set and there is only one
+        # on the product
+        if "organization" not in cleaned_data or not cleaned_data["organization"]:
+            try:
+                organization = cleaned_data["product"].course_relations.get(
+                    course=cleaned_data["course"]
+                ).organizations.get()
+            except (
+                models.Organization.DoesNotExist,
+                models.Organization.MultipleObjectsReturned,
+            ):
+                pass
+            else:
+                cleaned_data["organization"] = organization
 
-class OrderCreateBodySerializer:
-    credit_card_id = serializers.CharField(required=True)
-    course = serializers.CharField(required=True)
-    product = serializers.CharField(required=True)
-    billing_address = AddressSerializer(required=True)
+        # If product is not free, we have to create a payment.
+        # To create one, a billing address is mandatory
+        if cleaned_data['product'].price.amount > 0 \
+                and ("billing_address" not in cleaned_data or not cleaned_data['billing_address']):
+            raise serializers.ValidationError({"billing_address": "This field is required."})
+        return cleaned_data
 
-    class Meta(OrderCreateSerializer.Meta):
-        fields = ["billing_address"]
+    def create(self, validated_data):
+        order_data = validated_data.copy()
+        if 'billing_address' in order_data:
+            order_data.pop('billing_address')
+        if 'credit_card_id' in order_data:
+            order_data.pop('credit_card_id')
+        return super().create(order_data)

src/backend/joanie/core/serializers/order_create_response_serializer.py

@@ -1,17 +1,10 @@
 """Serializers for core.api.OrderViewSet.create Response"""
+from .model_serializers import OrderSerializer, PaymentSerializer
 
-from rest_framework import serializers
-from djmoney.contrib.django_rest_framework import MoneyField
-from drf_yasg.utils import swagger_serializer_method
 
-from joanie.core import models
-from .model_serializers import OrderSerializer, PaymentSerializer, EnrollmentSerializer
-
-
-class OrderCreateResponseSerializer(serializers.Serializer):
-    order = OrderSerializer(required=True)
+class OrderCreateResponseSerializer(OrderSerializer):
     payment_info = PaymentSerializer(required=False)
 
     class Meta(OrderSerializer.Meta):
-        fields = ["order", "payment_info"]
-        read_only_fields = ["order", "payment_info"]
+        fields = OrderSerializer.Meta.fields + ["payment_info"]
+        read_only_fields = OrderSerializer.Meta.fields + ["payment_info"]

src/backend/joanie/payment/factories.py

@@ -83,6 +83,7 @@ class BillingAddressDictFactory(factory.DictFactory):
     Return a billing address dictionary
     """
 
+    title = factory.Faker("name")
     address = factory.Faker("street_address")
     city = factory.Faker("city")
     country = factory.Faker("country_code")

src/backend/joanie/tests/core/test_api_order.py

@@ -1111,7 +1111,7 @@ def test_api_order_create_payment_requires_billing_address(self):
         self.assertFalse(models.Order.objects.exists())
         self.assertEqual(response.status_code, 400)
         self.assertEqual(
-            response.json(), {"billing_address": "This field is required."}
+            response.json(), {"billing_address": ["This field is required."]}
         )
 
     @mock.patch.object(

src/backend/joanie/tests/payment/test_backend_payplug.py

@@ -408,6 +408,7 @@ def test_payment_backend_payplug_handle_notification_payment(
         order = OrderFactory(product=product)
         backend = PayplugBackend(self.configuration)
         billing_address = BillingAddressDictFactory()
+        del billing_address["title"]
         payplug_billing_address = billing_address.copy()
         payplug_billing_address["address1"] = payplug_billing_address["address"]
         del payplug_billing_address["address"]
@@ -485,6 +486,7 @@ def test_payment_backend_payplug_handle_notification_payment_register_card(
         order = OrderFactory(product=product)
         backend = PayplugBackend(self.configuration)
         billing_address = BillingAddressDictFactory()
+        del billing_address["title"]
         payplug_billing_address = billing_address.copy()
         payplug_billing_address["address1"] = payplug_billing_address["address"]
         del payplug_billing_address["address"]

src/openApiClientJs/gen/index.ts

@@ -21,7 +21,7 @@ export type { ErrorResponse } from './models/ErrorResponse';
 export { Order } from './models/Order';
 export type { OrderAbortBody } from './models/OrderAbortBody';
 export { OrderCreateBody } from './models/OrderCreateBody';
-export type { OrderCreateResponse } from './models/OrderCreateResponse';
+export { OrderCreateResponse } from './models/OrderCreateResponse';
 export type { Payment } from './models/Payment';
 export { Product } from './models/Product';
 

src/openApiClientJs/gen/models/Order.ts

@@ -10,6 +10,9 @@ export type Order = {
   readonly created_on?: string;
   readonly certificate?: string;
   readonly enrollments?: string;
+  /**
+   * primary key for the record as UUID
+   */
   readonly id?: string;
   readonly main_invoice?: string;
   organization?: string;

src/openApiClientJs/gen/models/OrderCreateBody.ts

@@ -12,6 +12,9 @@ export type OrderCreateBody = {
   readonly created_on?: string;
   readonly certificate?: string;
   readonly enrollments?: string;
+  /**
+   * primary key for the record as UUID
+   */
   readonly id?: string;
   readonly main_invoice?: string;
   organization?: string;
@@ -22,6 +25,7 @@ export type OrderCreateBody = {
   readonly state?: OrderCreateBody.state;
   readonly target_courses?: string;
   billing_address?: Address;
+  credit_card_id?: string;
 };
 
 export namespace OrderCreateBody {