From 1e4330dca5d2f3588f622b7cbcfe0b784c94ac02 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Sun, 27 Sep 2026 22:44:07 -0300 Subject: [PATCH] fix: [TESIS-136] refuse to dispatch the shipment of a cancelled order MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `CreateShipment` refuses to open a shipment for a cancelled order. `ConfirmDispatch` had no equivalent check: it validated the shipment's own state and nothing else, so a shipment opened before the order was cancelled could still be dispatched through the API, paying a courier for the label of a sale that is not going out. The frontend of TESIS-134 does not offer the action, but the endpoint was open to any client. The status list is `CreateShipment::NON_SHIPPABLE_STATUSES`, not a copy: it is the same business rule, and when the state machine exists there is one place to change. It raises the same `UnshippableOrderError` as opening the shipment does, for the same 422. A 409 would be the shipment's own state conflict — that is `AlreadyDispatchedError` — and the frontend reads it that way: on a 409 the dispatch dialog says the shipment was dispatched meanwhile, which here would be false. The check runs only before the call to the courier, unlike the shipment's own state, which is validated again under the lock. If the order is cancelled while the courier answers, the label already exists, and dropping the tracking number would lose the trail of a parcel the courier already knows about. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- app/poros/shipments/confirm_dispatch.rb | 27 ++++++++++ spec/poros/shipments/confirm_dispatch_spec.rb | 49 ++++++++++++++++++- .../api/v1/shipment_dispatches_spec.rb | 36 ++++++++++++++ 3 files changed, 111 insertions(+), 1 deletion(-) diff --git a/app/poros/shipments/confirm_dispatch.rb b/app/poros/shipments/confirm_dispatch.rb index f2dcf16..c7d0215 100644 --- a/app/poros/shipments/confirm_dispatch.rb +++ b/app/poros/shipments/confirm_dispatch.rb @@ -43,6 +43,7 @@ def initialize(shipment:, company_integration:, origin_warehouse:, shipping_cost end def call + validate_order! validate_integration! validate_status!(@shipment) validate_cost! @@ -54,6 +55,32 @@ def call private + # Una orden cancelada no entra al circuito logístico, y que su envío ya esté + # abierto no lo cambia: emitir la etiqueta de una venta que no sale es pagar + # un despacho de más (TESIS-136). Hasta acá la regla vivía sólo en el alta + # del envío, así que un envío abierto antes de cancelar la orden se podía + # despachar igual por la API. + # + # La lista de estados es la de `CreateShipment` y no una copia: es la misma + # regla de negocio, y cuando exista la transición de estados hay un solo + # lugar que tocar. + # + # Mismo error y mismo 422 que el alta, a propósito. 409 sería el conflicto de + # estado del ENVÍO —eso es `AlreadyDispatchedError`— y el frontend lo lee + # así: ante un 409 el diálogo de despacho dice que el envío ya se despachó + # mientras tanto, que acá sería falso. + # + # Sólo antes de la llamada al courier, y no otra vez bajo el lock como el + # estado del envío: si la orden se cancela mientras el courier contesta, la + # etiqueta ya se emitió, y descartar el número de seguimiento perdería el + # rastro de un paquete que el courier ya conoce. Se guarda y la cancelación + # se resuelve por el canal que corresponda. + def validate_order! + return unless CreateShipment::NON_SHIPPABLE_STATUSES.include?(order.status) + + raise UnshippableOrderError.new(order: order) + end + # Se valida antes de llamar al courier para no gastar una etiqueta —que el # proveedor cobra— en un envío que después no vamos a poder guardar. # diff --git a/spec/poros/shipments/confirm_dispatch_spec.rb b/spec/poros/shipments/confirm_dispatch_spec.rb index 79b719d..9bc2031 100644 --- a/spec/poros/shipments/confirm_dispatch_spec.rb +++ b/spec/poros/shipments/confirm_dispatch_spec.rb @@ -43,7 +43,8 @@ def dispatch(target = shipment, using: integration) def attempt_dispatch(target = shipment, using: integration) dispatch(target, using: using) rescue Shipments::AlreadyDispatchedError, Shipments::DispatchResponseError, - Shipments::InvalidCourierIntegrationError, Integrations::AdapterExecutionError + Shipments::InvalidCourierIntegrationError, Shipments::UnshippableOrderError, + Integrations::AdapterExecutionError nil end @@ -151,6 +152,52 @@ def dispatch_costing(cost) end end + # El alta del envío ya excluye las órdenes canceladas; el despacho no lo hacía, + # así que un envío abierto ANTES de cancelar la orden se podía despachar igual + # (TESIS-136, de la review de TESIS-134). + describe 'when the order was cancelled after the shipment was opened' do + before { order.update!(status: 'cancelled') } + + it 'refuses to dispatch it' do + expect { dispatch }.to raise_error(Shipments::UnshippableOrderError, /cannot be shipped/) + end + + # La etiqueta se paga: el chequeo va antes de la llamada, no después. + it 'does not call the courier' do + stub = stub_courier + attempt_dispatch + + expect(stub).not_to have_been_requested + end + + it 'leaves the shipment untouched', :aggregate_failures do + stub_courier + attempt_dispatch + + expect(shipment.reload.status).to eq('pending') + expect(shipment.tracking_number).to be_nil + end + + it 'records nothing in the log' do + stub_courier + + expect { attempt_dispatch }.not_to change(ShipmentEvent, :count) + end + end + + # Control negativo del bloque de arriba: lo que se rechaza es el estado + # cancelado, no cualquier estado distinto del inicial. + describe 'when the order is paid' do + before do + order.update!(status: 'paid') + stub_courier + end + + it 'dispatches it as usual' do + expect(dispatch.reload.status).to eq('ready_to_ship') + end + end + describe 'when the shipment is not pending' do let(:shipment) do Shipment.create!(company: company, order: order, status: 'in_transit', diff --git a/spec/requests/api/v1/shipment_dispatches_spec.rb b/spec/requests/api/v1/shipment_dispatches_spec.rb index cf53e91..fe8e1e0 100644 --- a/spec/requests/api/v1/shipment_dispatches_spec.rb +++ b/spec/requests/api/v1/shipment_dispatches_spec.rb @@ -211,6 +211,42 @@ def dispatch_shipment(id: shipment.id, integration_id: integration.id, expect(response).to have_http_status(:bad_gateway) end + # El alta del envío ya rechaza las órdenes canceladas con un 422; el despacho + # no lo hacía, así que un envío abierto antes de cancelar la orden se podía + # despachar por la API aunque la UI no lo ofreciera (TESIS-136). + describe 'when the order was cancelled after the shipment was opened' do + before { order.update!(status: 'cancelled') } + + it 'returns 422, the same as opening the shipment of a cancelled order' do + stub_courier + dispatch_shipment + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'explains why it was rejected' do + stub_courier + dispatch_shipment + + expect(response.parsed_body['error']).to include('cannot be shipped') + end + + it 'does not call the courier' do + stub = stub_courier + dispatch_shipment + + expect(stub).not_to have_been_requested + end + + it 'leaves the shipment pending and without tracking', :aggregate_failures do + stub_courier + dispatch_shipment + + expect(shipment.reload.status).to eq('pending') + expect(shipment.tracking_number).to be_nil + end + end + describe 'request contract' do it 'returns 400 without the integration' do dispatch_shipment(integration_id: '')