feat: [TESIS-131] quote the manual order before creating it and dispatch the chosen option - #90
Conversation
…mplate Quoting and dispatching are two endpoints of the provider and, by convention, two Service rows. Nothing said they belonged to the same courier, so an option quoted by "Andreani - Cotización" could not be dispatched: ConfirmDispatch rejects a template that does not dispatch. `services.quote_service_id` hangs from the dispatch template, the same shape as `tracking_service_id` (TESIS-49), and is validated the same way: couriers only, never itself, and only a template that quotes. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuoteShipment read the destination and the lines straight from an order, so the only way to learn what a shipment costs was to create the order first, deducting the stock before the operator confirmed. It now takes the origin, the destination and `[product, quantity]` lines. `QuoteShipment.for_order` builds that context from an order, and the nested quote endpoint uses it, so its behaviour does not change. A new example pins what travels to the courier (weight, item count, origin and destination); it passes unchanged against the previous implementation. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each option now carries `dispatch_integration_id`, the company integration whose template dispatches for that courier. That is what the dispatch needs to confirm the option: the id the quote reported belongs to the template that answered the rate, and ConfirmDispatch rejects it. A quote template with no active dispatch integration behind it is not asked for a price at all. An option the operator cannot confirm is not an option, and asking would only make them wait for it. The option is named after the courier (the dispatch template), so the operator picks "Andreani" and not "Andreani - Cotización". Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST /api/v1/quotes takes what the manual order wizard already gathered (origin warehouse, destination and `[product_id, quantity]` lines) and answers the same options as the quote of an order. The order is created once, when the operator picks an option and confirms, so quoting no longer deducts stock nor leaves an order nobody can cancel. The warehouse and the products are looked up inside the tenant: an id of another company answers 404. A draft without origin, zip code or items, with a non-positive quantity or above the item limit of an order answers 400. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ConfirmDispatch never wrote `shipments.shipping_cost`, so the order detail showed the shipment as "to be quoted" forever and the cost the operator accepted was lost. POST /shipments/:id/dispatch now takes an optional `shipping_cost`, the price of the option the operator confirmed, and keeps it on the shipment. It is validated before asking the courier for a label (which the courier charges): a negative or non-numeric cost answers 400. A dispatch without it leaves the cost as it was. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…tches ADR-016 explains the three gaps the wizard hit (quoting needed an order, a quote could not be dispatched, the chosen cost was lost), why the parcel is quoted instead of adding a draft order status, why the link hangs from the dispatch template, and why the confirmed cost is kept instead of re-quoting. architecture.md points the synchronous quoting exception at the new endpoint too. ADR-016 and not 015: proyecto-api#86 (TESIS-107) already takes 015. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
los 6 commits de este PR salieron con la identidad sim x@x en vez de la tuya. Como GitHub asocia los commits a una cuenta por el email, los está atribuyendo a xxxxxxxxxxxxx, una cuenta de 2013 que no tiene nada que ver con el proyecto. Supongo que tu git config quedó con valores de relleno en el entorno desde el que pusheaste. ¿Podés confirmarlo? Para no dejar esa cuenta en el historial de master, habría que reescribir la autoría antes del merge |
TomasMartin2004
left a comment
There was a problem hiding this comment.
Revisión — TESIS-131 (PR #90) · Cotizar el borrador y despachar lo elegido
Revisado sobre TESIS-131-quote-the-draft-and-dispatch-it contra origin/master, con el ADR-016 y la card al lado.
| Check | Resultado |
|---|---|
| Suite completa (local) | 1283 ejemplos, 0 fallas |
| RuboCop · Brakeman (local) | 258 archivos sin ofensas · 0 warnings |
db/schema.rb |
Sólo la columna y el índice nuevos, sin ruido de la versión local de PostgreSQL |
🔴 Bloqueante, y no es de este PR solo: choca el número de migración con api#89
Las dos migraciones abiertas tienen el mismo timestamp:
#89 (TESIS-82) db/migrate/20260924120000_add_approved_to_users.rb
#90 (este) db/migrate/20260924120000_add_quote_service_to_services.rb
Simulé los dos mergeados sobre master y Rails no arranca:
$ bin/rails db:migrate
ActiveRecord::DuplicateMigrationVersionError:
Multiple migrations have the version number 20260924120000.
El que entre segundo deja master sin poder migrar hasta que alguien renumere, y db/schema.rb va a chocar igual en la línea de version:. Cualquiera de los dos puede renumerar —una migración que todavía no se aplicó en ningún lado se renombra sin costo—, pero hay que decidirlo antes de mergear, no descubrirlo después del merge.
Como este PR es el segundo en abrirse, me ofrezco a coordinarlo: si te parece, renumerá esta a 20260924130000 y listo.
✅ El diseño resuelve el problema real, y la alternativa descartada es la correcta
El hueco que describís es exacto: para mostrar tarifas había que crear la orden, y crearla descuenta stock. Cotizar el paquete en vez de la orden evita el estado draft que habría tocado estados, listado, KPIs y el momento del descuento. Es la decisión de menor alcance para el mismo resultado, y el ADR la justifica sin adornos.
Que el peso lo siga calculando el backend desde products.weight es lo que mantiene la regla vieja: el cliente dice qué lleva el paquete, no cuánto pesa.
✅ Verificado por HTTP, con sondas propias
cotizar un borrador -> 200, órdenes creadas: 0
producto de otra empresa -> 404
cantidad 0 -> 400 "each item needs a positive integer quantity"
101 líneas (tope del alta) -> 400
sin código postal -> 400
El 404 del producto ajeno sale del scope del tenant y no de un chequeo aparte, que es como corresponde. Y me gustó el detalle de required(): sin él, un id vacío llegaba a find('') y contestaba 404, diciéndole al cliente que algo no existe cuando lo que falta es el parámetro.
El filtro de cotizadores sin despacho tiene dientes: le saqué el dispatchers.key?(ci.service_id) y se pone en rojo is not even asked for a price, solo. Filtrar antes de abrir los hilos —y no después de cotizar— está bien argumentado: una opción que no se puede confirmar no es una opción.
🟡 Dos couriers que comparten la plantilla de cotización: uno desaparece en silencio
dispatchers indexa por quote_service_id:
.index_by { |ci| ci.service.quote_service_id }Si dos plantillas de despacho apuntan a la misma de cotización, index_by se queda con la última y la otra se pierde. Lo probé con «Andreani» y «Andreani Express» apuntando al mismo cotizador, las dos integraciones activas:
integraciones de despacho activas: 2
opciones devueltas: 1
dispatch_integration_id: una sola
No hay nada que lo impida hoy: quote_service_id no es único entre plantillas de despacho, y compartir el cotizador es plausible —dos servicios del mismo proveedor, o dos cuentas—. El resultado es que el operador ve una opción menos sin que nada lo avise, que es justo lo que esta card viene a evitar.
Dos salidas, cualquiera sirve:
- Validar que una plantilla de cotización sea de un solo despachador (unicidad de
quote_service_identre couriers que despachan), y que el error lo diga en Avo al configurar; o - Agrupar: cotizar una vez por plantilla y devolver una opción por despachador que la use.
La primera es más barata y coincide con lo que el ADR dice que es el modelo: una plantilla de cotización por courier.
🟡 El endpoint dispara llamadas externas sin crear nada
POST /quotes es el primer endpoint autenticado que sale a los proveedores sin dejar rastro en la base: antes cotizar exigía crear la orden, que era un freno natural. Un cliente en loop puede generar tantas llamadas a los couriers como quiera, con las credenciales de la empresa.
No lo bloqueo —hace falta sesión válida y el alcance es el propio tenant—, pero ahora que api#89 trae rate_limit para auth, el mecanismo ya está en el repo y aplicarlo acá es una línea. Vale al menos como nota en el ADR: es el tipo de cosa que no se nota hasta que un front con un useEffect mal puesto cotiza en cada tecla.
🟡 Menores
- El ADR dice que el costo se valida «negativo o no numérico responde 400 sin llamar a nadie». Lo verifiqué y es así. Buen orden: validar antes de pedir la etiqueta, que el courier cobra.
Service#quotes_shipping?deduce la capacidad delresponse_mapperen vez de una columna nueva. Coincido con el criterio data-driven, y me gusta que la contracara (dispatches_shipment?) explique por qué una plantilla de seguimiento no cuenta.
Veredicto
REQUEST CHANGES, por el choque de migraciones: es bloqueante para el par y se arregla renumerando, pero tiene que quedar resuelto antes de que entre cualquiera de los dos.
El resto del PR está muy bien: el problema está bien diagnosticado, la alternativa descartada es la que había que descartar, y las reglas nuevas vienen con ejemplos que fallan si se deshacen. Lo de los couriers que comparten cotizador lo dejo a tu criterio —con una validación alcanza—.
🤖 Generated with Claude Code
LoLoo03
left a comment
There was a problem hiding this comment.
Review — TESIS-131 (PR #90)
🔴 El nuevo: shipping_cost deja pasar valores que la base rechaza, y eso pasa después de emitir la etiqueta
El controller solo verifica que el costo sea un número y no sea negativo. La columna es decimal(10,2) (máximo 99.999.999,99), y NaN e Infinity también son BigDecimal válidos. Esos valores pasan la validación, se llama al courier, y el update! falla dentro de la transacción:
shipping_cost Resultado ¿Se llamó al courier? Envío después
1e9 / 100000000 ActiveRecord::RangeError → 500 1 vez pending, sin tracking
Infinity ActiveRecord::RangeError → 500 1 vez pending, sin tracking
NaN 422, lo rechaza la validación del modelo 1 vez pending, sin tracking
La etiqueta se pagó y el número de seguimiento se perdió en el rollback. Como el envío sigue en pending, reintentar emite una segunda etiqueta. Es justo lo que el comentario de shipments_controller.rb promete evitar («para no gastar una etiqueta en un despacho que después no se podría guardar»). El PR dice que un costo negativo o no numérico responde 400 sin llamar a nadie, y es verdad, pero no cubre estos casos.
Arreglo sugerido: que la regla viva en un solo lugar. Agregar less_than: 100_000_000 a la validación de Shipment#shipping_cost y, en ConfirmDispatch#call, validar el costo contra el modelo antes de request_label (por ejemplo assign_attributes + valid?). Así, si la columna cambia, la validación la sigue. También hace falta sumar ejemplos 400 para 1e8, NaN e Infinity que verifiquen que no se llamó al courier.
🔴 Confirmo el choque de migraciones con api#89
origin/TESIS-82-auth-multitenancy-qa-fixes tiene 20260924120000_add_approved_to_users.rb, con el mismo timestamp que la de este PR. Estoy de acuerdo con renumerar esta, que se abrió después, a 20260924130000.
🔴 Confirmo la autoría x@x
Los 6 commits dicen sim x@x. Hay que reescribir la autoría antes del merge. Por ejemplo, git rebase origin/master --exec "git commit --amend --no-edit --reset-author" con el user.email correcto y después un force-push.
🟡 Dos despachadores que comparten la plantilla de cotización
Estoy de acuerdo con el comentario de Tomás: el index_by { |ci| ci.service.quote_service_id } de QuoteShipment#dispatchers se queda con uno solo y el otro desaparece sin aviso. La unicidad de quote_service_id impide solo que la misma empresa tenga dos integraciones del mismo servicio. Nada impide que dos servicios de despacho apunten al mismo cotizador. Validar esa unicidad en Service alcanza.
🟡 Límite de requests en POST /quotes
Coincido en que no bloquea, pero vale al menos una línea en el ADR-016.
⚪ Menores
Service#quote_service_problem exige que la plantilla sea courier?, pero no que sea una plantilla que despacha. Una plantilla de cotización o de seguimiento puede tener quote_service cargado sin que nada lo marque. No rompe nada, porque dispatchers filtra por dispatches_shipment?, pero deja en Avo una configuración que no hace nada.
El costo lo manda el cliente y no se compara con lo que devolvió la cotización. El ADR-016 lo decide así a propósito y el operador ya está autenticado en su propio tenant. Lo menciono solo para que quede escrito.
✅ Lo que está bien
La respuesta { data: [...] } cumple el ADR-015 de #86.
El 404 para producto o depósito de otra empresa sale del scope del tenant, no de un chequeo aparte.
El helper required() evita que un id vacío termine en find('') y responda 404.
El refactor a .for_order no cambia lo que se le manda al courier.
El orden de validación en ConfirmDispatch ya estaba bien pensado. El problema es solo el rango del costo.
Veredicto: REQUEST CHANGES, por el rango de shipping_cost (hace perder etiquetas pagas) además de la migración y la autoría.
|
Aviso: Con renombrarla a El resto de mi review sigue igual: el fondo del PR está bien y lo único que lo bloqueaba era esto. 🤖 Generated with Claude Code |
TESIS-82 took 20260924120000 on master, and two migrations with the same version stop Rails from migrating. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
… the label The controller only checked that shipping_cost was a non-negative number. 1e8, NaN and Infinity passed, the courier issued the label, and update! failed afterwards: the shipment stayed pending and a retry paid for a second label. The range now lives in Shipment and ConfirmDispatch tries the cost against it before calling the courier. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
QuoteShipment indexes the dispatchers by quote template, so two dispatch templates sharing one dropped one of them from the options without notice. Service now rejects a quote template that another courier already uses, or one set on a template that does not dispatch, and a unique index backs it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
POST /quotes is the first authenticated endpoint that calls the couriers without writing anything, so a client in a loop could fire unlimited requests with the company credentials. Rails rate_limit, per user. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # db/schema.rb
Respuesta — TESIS-131 (reviews de @TomasMartin2004 y @LoLoo03, comentario de @LauAubert)Commits nuevos en
Suite 1387 examples, 0 failures · RuboCop 272 archivos sin ofensas · Brakeman 0. 🔴 Choque de migraciones: renumerada
Después entró TESIS-129 con 🔴
|
TomasMartin2004
left a comment
There was a problem hiding this comment.
Segunda vuelta — TESIS-131 (PR #90)
Verifiqué los tres puntos sobre la rama actualizada.
🔴 La migración, renumerada y sin choque
db/migrate/20260924120000_add_approved_to_users.rb (master, TESIS-82)
db/migrate/20260924130000_add_quote_service_to_services.rb (esta rama)
db/migrate/20260925120000_add_session_token_to_admin_users (master, TESIS-129)
$ bin/rails db:migrate:status
down 20260924120000 Add approved to users
down 20260924130000 Add quote service to services
down 20260925120000 Add session token to admin users
Las tres conviven y Rails las enumera sin el DuplicateMigrationVersionError. Que la de acá quede en el medio no molesta: son tablas distintas y Rails corre las pendientes aunque sean más viejas, como decís.
🟡 Una plantilla de cotización, un solo despachador
Reproduje mi caso —«Andreani» y «Andreani Express» compartiendo cotizador— y ahora no se puede ni configurar:
valido? false
error: "ya es la plantilla de cotización de otro courier"
La solución me gusta más que la que había propuesto: además de la unicidad, rechazás el cotizador cargado en una plantilla que no despacha, que era la otra mitad de la configuración que no hace nada. Y el índice único lo respalda en la base, así que no depende de que la validación corra.
🟡 El tope de cotizaciones
las primeras 20 -> 200
la 21 y la 22 -> 429, Retry-After: 60
otro usuario de la misma empresa -> 200
Por usuario y no por IP es la decisión correcta y no se me había ocurrido: todos los requests llegan autenticados, y un depósito detrás de un mismo NAT no tiene por qué compartir cupo. 20 por minuto deja lugar de sobra para entrar al paso 3 y reintentar.
(Igual que con el límite de auth: para medirlo hay que stubear increment y read del store real. Reemplazar el objeto cache_store no alcanza — me pasó de nuevo y la primera corrida me dio doce 200 seguidos.)
Verificación completa
rspec → 1387 ejemplos, 0 fallas · RuboCop 272 archivos sin ofensas · Brakeman 0 warnings.
Veredicto
APPROVE. Los tres quedaron cerrados y los dos 🟡 terminaron mejor resueltos de lo que estaban planteados.
Queda lo que marcó @LauAubert: los seis commits originales salieron con la identidad sim <x@x>. Los cuatro nuevos ya salen con la tuya, así que si querés dejar el historial prolijo conviene arreglarlo antes del merge; si preferís que entre así, no lo bloqueo.
🤖 Generated with Claude Code
|
Intenté mergearlo y GitHub lo rechaza: Es la review de @LoLoo03, que sigue en @LoLoo03 cuando puedas: los dos 🔴 que marcaste están resueltos y los verifiqué —la migración renumerada convive con las de TESIS-82 y TESIS-129 sin el Lo dejo listo para mergear apenas se destrabe. 🤖 Generated with Claude Code |
|
@LoLoo03 estado de tus tres 🔴, verificados por mí sobre la rama actual: 1. El rango de La validación quedó donde pediste: 2. La migración — resuelta. Renumerada a 3. La autoría — sigue pendiente. Seis commits todavía figuran como Los siete posteriores ya salen con la cuenta de @Sanntinat. Por eso te escribo en vez de mergear: los dos primeros los doy por cerrados, pero el tercero es tuyo y no me corresponde decidirlo. Si la autoría te bloquea, decilo y que se reescriba antes del merge; si preferís que entre así y quede anotado, actualizá la review y lo mergeo — el PR está Detrás viene 🤖 Generated with Claude Code |
Ticket de Jira
https://proyectofinalfrlp.atlassian.net/browse/TESIS-131
Descripción
Destraba el paso 3 del alta manual (TESIS-59, diseño S07): cotizar antes de crear la orden, despachar la opción que el operador elige y guardar lo que costó.
De dónde sale esta card. S07 muestra las tarifas al entrar al paso y un «Confirmar orden» que crea la orden y emite el despacho. Las tres piezas existían por separado (cotizar, TESIS-46; crear el envío, TESIS-105; despachar, TESIS-47), pero juntas no alcanzaban. Había tres huecos:
pendingque nadie puede cancelar.ConfirmDispatchexige una que despache («Andreani»). Lo verifiqué contra master: mandar el id de la cotización al despacho responde 422 (InvalidCourierIntegrationError). Nada vinculaba una plantilla con la otra.shipments.shipping_cost, y el detalle de la orden mostraba el envío «a cotizar» para siempre.Decisiones (las tres están en ADR-016, con sus alternativas):
POST /api/v1/quotesrecibe el borrador (depósito de origen, destino y líneas) y responde lo mismo que la cotización de una orden. El peso lo sigue calculando el backend conproducts.weight: el cliente dice qué lleva el paquete, no cuánto pesa. Se descartó un estadodraftde la orden, que tocaba estados, listado, KPIs y el momento del descuento para resolver un problema de secuencia.services.quote_service_id, con el mismo patrón y la misma validación quetracking_service_id(TESIS-49). Cada opción informadispatch_integration_idy se nombra por el courier («Andreani», no «Andreani - Cotización»).Lo que no cambia:
POST /orders/:id/quotesresponde igual que antes, másdispatch_integration_id, y un despacho sinshipping_costfunciona como hasta ahora.20260924120000:services.quote_service_id(FK aservices,on_delete: :nullify, con índice)Service#quote_service/#quoted_services, con validación; campo «Quote template» en Avo, junto al de seguimiento; seeds: «Andreani» → «Andreani - Cotización»Shipments::QuoteShipmentrecibe origen, destino y líneas;.for_orderarma ese contexto desde una orden (commit de refactor aparte, sin cambio de comportamiento)dispatch_integration_idy nombre del courier en cada opciónPOST /api/v1/quotes(Api::V1::DraftQuotesController): depósito y productos dentro del tenant (ajenos → 404); sin origen, código postal o líneas, cantidades no positivas o más líneas que el tope del alta → 400POST /shipments/:id/dispatchaceptashipping_costopcional y lo persistearchitecture.mdEvidencia visual
N/A: son endpoints. La pantalla que los consume es TESIS-59, en
proyecto-web.Cómo probar
Precondición:
bin/rails db:migrate,bin/rails db:seedy una sesión del tenantnorte.POST /api/v1/quotescon{ "quote": { "origin_warehouse_id": <id>, "destination_zip_code": "5000", "destination_address": "…", "items": [{ "product_id": <id>, "quantity": 2 }] } }→ 200 condata. Cada opción traedispatch_integration_id. No se crea ninguna orden.product_ido unorigin_warehouse_idde otra empresa → 404. Sinitems, o conquantity: 0→ 400 con{ "error": … }.POST /orders/:id/shipment) y despacharlo con{ "dispatch": { "company_integration_id": <dispatch_integration_id>, "origin_warehouse_id": <id>, "shipping_cost": 2500.5 } }→ 200, yshipping_costqueda en el envío (GET /shipments/:id)."shipping_cost": -1→ 400, sin llamada al courier.POST /orders/:id/quotessigue respondiendo como antes.Verificación:
bundle exec rspec(1283 ejemplos, 0 fallas),bundle exec rubocop(258 archivos, sin ofensas),bin/brakeman -q(0 warnings). Los seeds se corrieron dentro de una transacción revertida: «Andreani» queda vinculado a «Andreani - Cotización», y la empresanortecotiza con la segunda y despacha con la primera.Dientes. Rompí cada regla nueva de a una y en todos los casos se puso en rojo el ejemplo que la cubre:
is not even asked for a pricedispatch_integration_idcon el id de la cotizaciónnames each option after its courier…quantity: 0returns 400 with a quantity of zerois kept on the shipment and read back from itEl ejemplo que fija lo que viaja al courier (peso, bultos, origen y destino) pasa igual contra la implementación anterior de
QuoteShipment: el refactor no cambió el payload.Impacto y consideraciones
¿Introduce breaking changes?
Uno, acotado.
POST /orders/:id/quotesdeja de ofrecer los couriers que no tienen integración de despacho vinculada, porque esas opciones no se podían confirmar. Con los seeds, Andreani queda vinculado. Un courier cargado a mano necesita su «Quote template» en el panel de administración para aparecer. El front todavía no consume este endpoint.¿Requiere nuevas variables de entorno?
No.
¿Afecta la arquitectura o genera un nuevo patrón?
Sí, ADR-016. Es el 016 porque #86 (TESIS-107) ya usa el 015.
Con los otros PRs abiertos. Simulé el merge con #85, #86, #87 y #88: ninguno tiene conflictos de texto. #88 agrega ejemplos a
quote_shipment_spec.rbcon el helperquoting_servicey, con esta rama, al principio fallaban 5, porque una plantilla sin despacho ya no se ofrece. Lo resolví de este lado: el helper arma por defecto el courier completo. Combinado con #88, los specs de envíos pasan (158, 0 fallas), entre en el orden que entre.Bloquea TESIS-59 (
proyecto-web): el paso 3 cotiza conPOST /quotesy confirma con alta, envío y despacho.🤖 Generated with Claude Code