refac: better reordering code

Author: cosmith

backend/trips/services.py

@@ -1,12 +1,13 @@
 from django.core.exceptions import ValidationError
-from django.db import models
+from django.db import transaction, models
 from typing import List, Dict, Any
 from .models import Trip, TripStop
 from orders.models import Stop, Order
 
 
 class TripValidationError(ValidationError):
     """Custom validation error for trip-related violations"""
+
     pass
 
 
@@ -50,7 +51,7 @@ def validate_new_trip_stop(trip: Trip, stop: Stop) -> None:
     existing_stop_ids = set(ts.stop.id for ts in trip_stops)
 
     # Get the paired stop (pickup if this is delivery, delivery if this is pickup)
-    paired_stop_type = 'pickup' if stop.stop_type == 'delivery' else 'delivery'
+    paired_stop_type = "pickup" if stop.stop_type == "delivery" else "delivery"
     paired_stop = order.stops.filter(stop_type=paired_stop_type).first()
 
     if not paired_stop:
@@ -78,25 +79,25 @@ def get_incomplete_orders(trip: Trip) -> List[Order]:
     Returns:
         List of Order instances that are missing either pickup or delivery stops
     """
-    trip_stops = trip.trip_stops.select_related('stop', 'stop__order').all()
+    trip_stops = trip.trip_stops.select_related("stop", "stop__order").all()
     orders_in_trip = {}
 
     # Group stops by order
     for trip_stop in trip_stops:
         if trip_stop.stop.order:
             order = trip_stop.stop.order
             if order.id not in orders_in_trip:
-                orders_in_trip[order.id] = {'order': order, 'stops': []}
-            orders_in_trip[order.id]['stops'].append(trip_stop.stop)
+                orders_in_trip[order.id] = {"order": order, "stops": []}
+            orders_in_trip[order.id]["stops"].append(trip_stop.stop)
 
     incomplete_orders = []
     for order_data in orders_in_trip.values():
-        order = order_data['order']
-        stops = order_data['stops']
+        order = order_data["order"]
+        stops = order_data["stops"]
         stop_types = {stop.stop_type for stop in stops}
 
         # Check if both pickup and delivery are present
-        if 'pickup' not in stop_types or 'delivery' not in stop_types:
+        if "pickup" not in stop_types or "delivery" not in stop_types:
             incomplete_orders.append(order)
 
     return incomplete_orders
@@ -116,8 +117,8 @@ def ensure_order_pair_in_trip(trip: Trip, order: Order) -> Dict[str, Any]:
     Raises:
         TripValidationError: If the order doesn't have both pickup and delivery stops
     """
-    pickup_stop = order.stops.filter(stop_type='pickup').first()
-    delivery_stop = order.stops.filter(stop_type='delivery').first()
+    pickup_stop = order.stops.filter(stop_type="pickup").first()
+    delivery_stop = order.stops.filter(stop_type="delivery").first()
 
     if not pickup_stop:
         raise TripValidationError(
@@ -129,13 +130,12 @@ def ensure_order_pair_in_trip(trip: Trip, order: Order) -> Dict[str, Any]:
             f"Order {order.order_number} does not have a delivery stop."
         )
 
-    return {
-        'pickup_stop': pickup_stop,
-        'delivery_stop': delivery_stop
-    }
+    return {"pickup_stop": pickup_stop, "delivery_stop": delivery_stop}
 
 
-def add_order_to_trip(trip: Trip, order: Order, pickup_time, delivery_time, notes: str = "") -> Dict[str, Any]:
+def add_order_to_trip(
+    trip: Trip, order: Order, pickup_time, delivery_time, notes: str = ""
+) -> Dict[str, Any]:
     """
     Add both pickup and delivery stops for an order to a trip atomically.
 
@@ -157,14 +157,17 @@ def add_order_to_trip(trip: Trip, order: Order, pickup_time, delivery_time, note
 
     # Validate the order has both stops
     order_stops = ensure_order_pair_in_trip(trip, order)
-    pickup_stop = order_stops['pickup_stop']
-    delivery_stop = order_stops['delivery_stop']
+    pickup_stop = order_stops["pickup_stop"]
+    delivery_stop = order_stops["delivery_stop"]
 
     with transaction.atomic():
         # Get the next available sequence numbers for the trip
-        last_sequence = trip.trip_stops.aggregate(
-            max_sequence=models.Max('sequence')
-        )['max_sequence'] or 0
+        last_sequence = (
+            trip.trip_stops.aggregate(max_sequence=models.Max("sequence"))[
+                "max_sequence"
+            ]
+            or 0
+        )
 
         pickup_sequence = last_sequence + 1
         delivery_sequence = last_sequence + 2
@@ -175,7 +178,7 @@ def add_order_to_trip(trip: Trip, order: Order, pickup_time, delivery_time, note
             stop=pickup_stop,
             sequence=pickup_sequence,
             planned_arrival_time=pickup_time,
-            notes=notes or f'Pickup for {order.order_number}'
+            notes=notes or f"Pickup for {order.order_number}",
         )
         pickup_trip_stop.save(skip_validation=True)
 
@@ -184,13 +187,13 @@ def add_order_to_trip(trip: Trip, order: Order, pickup_time, delivery_time, note
             stop=delivery_stop,
             sequence=delivery_sequence,
             planned_arrival_time=delivery_time,
-            notes=notes or f'Delivery for {order.order_number}'
+            notes=notes or f"Delivery for {order.order_number}",
         )
         delivery_trip_stop.save(skip_validation=True)
 
     return {
-        'pickup_trip_stop': pickup_trip_stop,
-        'delivery_trip_stop': delivery_trip_stop
+        "pickup_trip_stop": pickup_trip_stop,
+        "delivery_trip_stop": delivery_trip_stop,
     }
 
 
@@ -204,38 +207,46 @@ def validate_pickup_before_delivery(trip: Trip) -> None:
     Raises:
         TripValidationError: If any order has delivery before pickup
     """
-    trip_stops = trip.trip_stops.select_related('stop', 'stop__order').order_by('sequence')
+    trip_stops = trip.trip_stops.select_related("stop", "stop__order").order_by(
+        "sequence"
+    )
     orders_in_trip = {}
 
     # Group stops by order and track their positions
     for trip_stop in trip_stops:
         if trip_stop.stop.order:
             order = trip_stop.stop.order
             if order.id not in orders_in_trip:
-                orders_in_trip[order.id] = {'order': order, 'stops': []}
-            orders_in_trip[order.id]['stops'].append({
-                'trip_stop': trip_stop,
-                'stop_type': trip_stop.stop.stop_type,
-                'sequence_position': trip_stop.sequence
-            })
+                orders_in_trip[order.id] = {"order": order, "stops": []}
+            orders_in_trip[order.id]["stops"].append(
+                {
+                    "trip_stop": trip_stop,
+                    "stop_type": trip_stop.stop.stop_type,
+                    "sequence_position": trip_stop.sequence,
+                }
+            )
 
     # Check each order's pickup-delivery ordering
     for order_data in orders_in_trip.values():
-        order = order_data['order']
-        stops = order_data['stops']
+        order = order_data["order"]
+        stops = order_data["stops"]
 
-        pickup_positions = [s['sequence_position'] for s in stops if s['stop_type'] == 'pickup']
-        delivery_positions = [s['sequence_position'] for s in stops if s['stop_type'] == 'delivery']
+        pickup_positions = [
+            s["sequence_position"] for s in stops if s["stop_type"] == "pickup"
+        ]
+        delivery_positions = [
+            s["sequence_position"] for s in stops if s["stop_type"] == "delivery"
+        ]
 
         # If both pickup and delivery exist, pickup must come before delivery
         if pickup_positions and delivery_positions:
-            min_pickup_pos = min(pickup_positions)
-            min_delivery_pos = min(delivery_positions)
+            max_pickup_pos = max(pickup_positions)
+            max_delivery_pos = max(delivery_positions)
 
-            if min_pickup_pos >= min_delivery_pos:
+            if max_pickup_pos >= max_delivery_pos:
                 raise TripValidationError(
-                    f"Order {order.order_number} has delivery stop (position {min_delivery_pos}) "
-                    f"before or at same position as pickup stop (position {min_pickup_pos}). "
+                    f"Order {order.order_number} has delivery stop (position {max_delivery_pos}) "
+                    f"before or at same position as pickup stop (position {max_pickup_pos}). "
                     f"Pickup must occur before delivery."
                 )
 
@@ -250,11 +261,47 @@ def get_orders_requiring_both_stops() -> List[Order]:
     """
     orders_with_both_stops = []
 
-    for order in Order.objects.prefetch_related('stops').all():
+    for order in Order.objects.prefetch_related("stops").all():
         stops = order.stops.all()
         stop_types = {stop.stop_type for stop in stops}
 
-        if 'pickup' in stop_types and 'delivery' in stop_types:
+        if "pickup" in stop_types and "delivery" in stop_types:
             orders_with_both_stops.append(order)
 
     return orders_with_both_stops
+
+
+def update_trip_stop_sequences(trip: Trip, new_sequences: List[Dict[str, Any]]) -> None:
+    """
+    Update the sequence order of trip stops.
+
+    Args:
+        trip: The Trip instance to update
+        new_sequences: List of dicts with 'id' and 'sequence' keys
+
+    Raises:
+        TripValidationError: If some trip stops don't belong to this trip
+    """
+
+    with transaction.atomic():
+        # Validate that all provided stop IDs belong to this trip
+        provided_stop_ids = [item["id"] for item in new_sequences]
+        existing_stops = TripStop.objects.filter(trip=trip, id__in=provided_stop_ids)
+
+        if len(existing_stops) != len(provided_stop_ids):
+            raise TripValidationError("Some trip stops do not belong to this trip")
+
+        # First, set all orders to very high values to avoid constraint conflicts
+        # Use values starting from 10000 to avoid conflicts with existing orders
+        for i, sequence_item in enumerate(new_sequences):
+            trip_stop = TripStop.objects.get(id=sequence_item["id"], trip=trip)
+            trip_stop.sequence = 10000 + i  # Use high values temporarily
+            trip_stop.save()
+
+        # Then set the final order values
+        for sequence_item in new_sequences:
+            trip_stop = TripStop.objects.get(id=sequence_item["id"], trip=trip)
+            sequence_value = sequence_item.get("sequence")
+            if sequence_value is not None:
+                trip_stop.sequence = sequence_value
+                trip_stop.save()

backend/trips/views.py

@@ -1,4 +1,3 @@
-from django.db import transaction
 import logging
 from django.http import JsonResponse
 from django.views.decorators.csrf import csrf_exempt
@@ -8,7 +7,12 @@
 import json
 from datetime import datetime
 from .models import Trip, TripStop
-from .services import add_order_to_trip, validate_pickup_before_delivery, TripValidationError
+from .services import (
+    add_order_to_trip,
+    validate_pickup_before_delivery,
+    update_trip_stop_sequences,
+    TripValidationError,
+)
 from orders.models import Order
 
 logger = logging.getLogger(__name__)
@@ -504,8 +508,7 @@ def post(self, request, trip_pk):
         """Reorder trip stops for a given trip"""
         try:
             data = json.loads(request.body)
-            # Support both 'orders' (legacy) and 'sequences' (new) parameter names
-            new_sequences = data.get("sequences") or data.get("orders", [])
+            new_sequences = data.get("sequences")
 
             logger.info(f"Reordering stops for trip {trip_pk}: {new_sequences}")
 
@@ -518,36 +521,12 @@ def post(self, request, trip_pk):
             except Trip.DoesNotExist:
                 return JsonResponse({"error": "Trip not found"}, status=404)
 
-            with transaction.atomic():
-                # Validate that all provided stop IDs belong to this trip
-                provided_stop_ids = [item["id"] for item in new_sequences]
-                existing_stops = TripStop.objects.filter(
-                    trip=trip, id__in=provided_stop_ids
-                )
-
-                if len(existing_stops) != len(provided_stop_ids):
-                    return JsonResponse(
-                        {"error": "Some trip stops do not belong to this trip"},
-                        status=400,
-                    )
-
-                # First, set all orders to very high values to avoid constraint conflicts
-                # Use values starting from 10000 to avoid conflicts with existing orders
-                for i, sequence_item in enumerate(new_sequences):
-                    trip_stop = TripStop.objects.get(id=sequence_item["id"], trip=trip)
-                    trip_stop.sequence = 10000 + i  # Use high values temporarily
-                    trip_stop.save()
-
-                # Then set the final order values
-                for sequence_item in new_sequences:
-                    trip_stop = TripStop.objects.get(id=sequence_item["id"], trip=trip)
-                    # Support both 'sequence' (new) and 'order' (legacy) field names in data
-                    new_seq = sequence_item.get("sequence") or sequence_item.get("order")
-                    trip_stop.sequence = new_seq
-                    trip_stop.save()
-
-                # Validate that pickups happen before deliveries
+            # Update trip stop sequence
+            try:
+                update_trip_stop_sequences(trip, new_sequences)
                 validate_pickup_before_delivery(trip)
+            except TripValidationError as e:
+                return JsonResponse({"error": str(e)}, status=400)
 
             # Return updated trip stops
             updated_stops = TripStop.objects.filter(trip=trip).order_by("sequence")

frontend/src/components/trips/TripDetailsDrawer.tsx

@@ -228,8 +228,8 @@ export const TripDetailsDrawer: React.FC<TripDetailsDrawerProps> = ({
 
     // Create a copy of the trip stops array
     const reorderedStops = Array.from(tripStops)
-    const [removed] = reorderedStops.splice(sourceIndex, 1)
-    reorderedStops.splice(destinationIndex, 0, removed)
+    const removed = reorderedStops.splice(sourceIndex, 2)
+    reorderedStops.splice(destinationIndex, 0, removed[1])
 
     // Update local state immediately for better UX
     setTripStops(reorderedStops)
@@ -240,16 +240,14 @@ export const TripDetailsDrawer: React.FC<TripDetailsDrawerProps> = ({
       // Create the sequences array for the API call
       const sequences = reorderedStops.map((stop, index) => ({
         id: stop.id,
-        sequence: index + 1, // API expects 1-based sequencing
+        sequence: index,
       }))
 
       // Call the reorder API
-      await post(`/trips/${trip.id}/reorder-stops/`, {
+      const stops: TripStop[] = await post(`/trips/${trip.id}/reorder-stops/`, {
         sequences,
       })
-
-      // Refresh trip details to ensure we have the latest data
-      await fetchTripDetails()
+      setTripStops(stops)
 
       toast({
         title: 'Stops reordered',
@@ -264,17 +262,6 @@ export const TripDetailsDrawer: React.FC<TripDetailsDrawerProps> = ({
       }
     } catch (err) {
       console.error('Error reordering stops:', err)
-      const errorMessage =
-        (err as { response?: { data?: { error?: string } } }).response?.data
-          ?.error || 'Failed to reorder stops'
-
-      toast({
-        title: 'Error reordering stops',
-        description: errorMessage,
-        status: 'error',
-        duration: 5000,
-        isClosable: true,
-      })
 
       // Revert to original order on error
       await fetchTripDetails()