From 90378c7d88cfd73b55d4ed3714824bfd6453ff2e Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 24 Sep 2026 21:08:26 -0300 Subject: [PATCH 01/11] feat: [TESIS-131] link each courier dispatch template to its quote template MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/avo/resources/service.rb | 4 ++ app/models/service.rb | 23 +++++++++ ...924120000_add_quote_service_to_services.rb | 16 ++++++ db/schema.rb | 5 +- db/seeds.rb | 6 +++ spec/models/service_spec.rb | 49 +++++++++++++++++++ 6 files changed, 102 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20260924120000_add_quote_service_to_services.rb diff --git a/app/avo/resources/service.rb b/app/avo/resources/service.rb index 397243b..a5379b2 100644 --- a/app/avo/resources/service.rb +++ b/app/avo/resources/service.rb @@ -16,6 +16,10 @@ def fields # consulta periódica pregunta por sus envíos (TESIS-49). field :tracking_service, as: :belongs_to, use_resource: Avo::Resources::Service, name: 'Tracking template', only_on: %i[show forms] + # En la plantilla que despacha: la que le pide tarifas al mismo + # proveedor. Sin ella, el courier no se ofrece al cotizar (TESIS-131). + field :quote_service, as: :belongs_to, use_resource: Avo::Resources::Service, + name: 'Quote template', only_on: %i[show forms] mapper_fields end diff --git a/app/models/service.rb b/app/models/service.rb index db5589c..89e0ebe 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -24,12 +24,22 @@ class Service < ApplicationRecord has_many :tracked_services, class_name: 'Service', foreign_key: :tracking_service_id, inverse_of: :tracking_service, dependent: :nullify + # Plantilla con la que se le piden tarifas a este courier (TESIS-131). Cuelga + # de la plantilla que despacha, igual que la de seguimiento: es lo que permite + # pasar de una opción cotizada a su despacho, porque la cotización la contesta + # una plantilla y la etiqueta la emite otra. + belongs_to :quote_service, class_name: 'Service', optional: true, + inverse_of: :quoted_services + has_many :quoted_services, class_name: 'Service', foreign_key: :quote_service_id, + inverse_of: :quote_service, dependent: :nullify + validates :service_name, presence: true, uniqueness: true validates :uri, presence: true validates :http_method, presence: true validates :type, presence: true, inclusion: { in: TYPES } validate :mappers_are_valid_json validate :tracking_service_answers_tracking + validate :quote_service_quotes_shipping # Sólo los canales de e-commerce generan ventas: el gateway lo usa para decidir # si un webhook entrante va al procesador de órdenes (TESIS-43) o queda a la @@ -146,4 +156,17 @@ def tracking_service_problem 'no es una plantilla de consulta de tracking' unless tracking_service.answers_tracking? end + + def quote_service_quotes_shipping + reason = quote_service_problem + errors.add(:quote_service, reason) if reason + end + + def quote_service_problem + return if quote_service.nil? + return 'solo aplica a couriers' unless courier? + return 'no puede ser la misma plantilla' if quote_service == self + + 'no es una plantilla de cotización' unless quote_service.quotes_shipping? + end end diff --git a/db/migrate/20260924120000_add_quote_service_to_services.rb b/db/migrate/20260924120000_add_quote_service_to_services.rb new file mode 100644 index 0000000..048929a --- /dev/null +++ b/db/migrate/20260924120000_add_quote_service_to_services.rb @@ -0,0 +1,16 @@ +# frozen_string_literal: true + +class AddQuoteServiceToServices < ActiveRecord::Migration[8.1] + # Plantilla con la que se le piden tarifas al courier que despacha con esta + # plantilla (TESIS-131). Cotizar y despachar son dos endpoints del proveedor y, + # por convención, dos `Service`; esto es lo que dice que son del mismo + # proveedor, para que una opción cotizada se pueda despachar. + # + # Mismo criterio que `tracking_service_id` (TESIS-49): es una relación entre + # plantillas, no entre integraciones, y cuelga de la plantilla que despacha. + # nullable: un courier sin plantilla de cotización simplemente no se ofrece. + def change + add_reference :services, :quote_service, + foreign_key: { to_table: :services, on_delete: :nullify } + end +end diff --git a/db/schema.rb b/db/schema.rb index 338cf76..e50fea3 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_09_21_130000) do +ActiveRecord::Schema[8.1].define(version: 2026_09_24_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -147,6 +147,7 @@ create_table "services", force: :cascade do |t| t.datetime "created_at", null: false t.string "http_method", null: false + t.bigint "quote_service_id" t.jsonb "request_mapper", default: {}, null: false t.jsonb "request_value_mapper", default: {}, null: false t.jsonb "response_mapper", default: {}, null: false @@ -156,6 +157,7 @@ t.string "type", null: false t.datetime "updated_at", null: false t.string "uri", null: false + t.index ["quote_service_id"], name: "index_services_on_quote_service_id" t.index ["service_name"], name: "index_services_on_service_name", unique: true t.index ["tracking_service_id"], name: "index_services_on_tracking_service_id" t.check_constraint "type::text = ANY (ARRAY['ecommerce'::character varying, 'courier'::character varying]::text[])", name: "services_type_check" @@ -274,6 +276,7 @@ add_foreign_key "product_mappings", "company_integrations", on_delete: :cascade add_foreign_key "product_mappings", "products", on_delete: :cascade add_foreign_key "products", "companies", on_delete: :cascade + add_foreign_key "services", "services", column: "quote_service_id", on_delete: :nullify add_foreign_key "services", "services", column: "tracking_service_id", on_delete: :nullify add_foreign_key "shipment_events", "shipments", on_delete: :cascade add_foreign_key "shipments", "companies", on_delete: :cascade diff --git a/db/seeds.rb b/db/seeds.rb index 35fa217..29820e2 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -307,6 +307,12 @@ correo_tracking = Service.find_by(service_name: 'Correo Argentino - Seguimiento') correo_service&.update!(tracking_service: correo_tracking) if correo_tracking +# Andreani cotiza con una plantilla y despacha con otra (TESIS-131): el vínculo +# es lo que deja despachar la opción que el operador eligió al cotizar. +andreani_dispatch = Service.find_by(service_name: 'Andreani') +andreani_quote = Service.find_by(service_name: 'Andreani - Cotización') +andreani_dispatch&.update!(quote_service: andreani_quote) if andreani_quote + # Vincula la primera empresa activa con Mercado Libre (integración de ejemplo). # La variable ml_integration la consume la orden de webhook de la sección TESIS-40 # más abajo (sin ella, `db:seed` cortaba con NameError: undefined ml_integration). diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 0942e95..3164217 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -205,6 +205,55 @@ def template(name, uri) end end + # La plantilla que cotiza por el courier que despacha con ésta (TESIS-131). + describe 'quote_service' do + subject(:courier) do + described_class.new(service_name: 'Andreani', type: 'courier', http_method: 'POST', + uri: 'https://api.andreani.test/ordenes', + response_mapper: { 'numero' => 'tracking_number' }) + end + + let(:quote_template) do + described_class.create!(service_name: 'Andreani - Cotización', type: 'courier', + http_method: 'POST', uri: 'https://api.andreani.test/tarifas', + response_mapper: { 'total' => 'shipping_cost' }) + end + + it 'accepts a template that quotes shipping' do + courier.quote_service = quote_template + expect(courier).to be_valid + end + + it 'rejects a template that does not quote shipping' do + courier.quote_service = described_class.create!( + service_name: 'Andreani - Seguimiento', type: 'courier', http_method: 'GET', + uri: 'https://api.andreani.test/envios/:tracking_number' + ) + expect(courier).not_to be_valid + end + + it 'rejects pointing a template at itself' do + quote_template.quote_service = quote_template + expect(quote_template).not_to be_valid + end + + it 'rejects a quote template on a sales channel' do + service.quote_service = quote_template + expect(service).not_to be_valid + end + + it 'lets the quote template reach the courier it quotes for' do + courier.update!(quote_service: quote_template) + expect(quote_template.quoted_services).to contain_exactly(courier) + end + + it 'is released when the quote template is destroyed' do + courier.update!(quote_service: quote_template) + quote_template.destroy! + expect(courier.reload.quote_service).to be_nil + end + end + it 'persists nested JSONB mappers', :aggregate_failures do service.update!(request_mapper: { 'order' => { 'id' => 'external_id' } }) expect(service.reload.request_mapper).to eq('order' => { 'id' => 'external_id' }) From b8a6aeb80c93170959e9667846c403acda9f4bb5 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 24 Sep 2026 21:12:02 -0300 Subject: [PATCH 02/11] refactor: [TESIS-131] quote the parcel instead of the order 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 --- .../api/v1/shipment_quotes_controller.rb | 2 +- app/poros/shipments/quote_shipment.rb | 30 +++++++++++++---- spec/poros/shipments/quote_shipment_spec.rb | 32 ++++++++++++++++++- 3 files changed, 55 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/v1/shipment_quotes_controller.rb b/app/controllers/api/v1/shipment_quotes_controller.rb index 7ffcad3..169546a 100644 --- a/app/controllers/api/v1/shipment_quotes_controller.rb +++ b/app/controllers/api/v1/shipment_quotes_controller.rb @@ -8,7 +8,7 @@ def create order = Order.find(params.expect(:order_id)) authorize order, :quote? - quotes = Shipments::QuoteShipment.new( + quotes = Shipments::QuoteShipment.for_order( order: order, origin_warehouse: origin_warehouse ).call diff --git a/app/poros/shipments/quote_shipment.rb b/app/poros/shipments/quote_shipment.rb index eb956f3..4c99fd3 100644 --- a/app/poros/shipments/quote_shipment.rb +++ b/app/poros/shipments/quote_shipment.rb @@ -17,10 +17,26 @@ class QuoteShipment < ApplicationPoro # request y el usuario está esperando. La card pide 3-5s. TIMEOUTS = { open: 4, read: 4 }.freeze - def initialize(order:, origin_warehouse:) + # La cotización de una orden que ya existe (TESIS-46): el destino y las + # líneas salen de la orden. + def self.for_order(order:, origin_warehouse:) + new(origin_warehouse: origin_warehouse, + destination: { zip_code: order.customer_zip_code, address: order.customer_address }, + lines: order.order_items.includes(:product).map { |item| [item.product, item.quantity] }) + end + + # Lo que se cotiza es el paquete, no la orden: de dónde sale, a dónde va y + # qué lleva. Así se puede cotizar también un alta que todavía no se confirmó + # (TESIS-131), sin crear la orden —y descontar el stock— para averiguar + # cuánto cuesta enviarla. + # + # `lines` son pares `[producto, cantidad]`, y `destination` lleva + # `:zip_code` y `:address`. + def initialize(origin_warehouse:, destination:, lines:) super() - @order = order @origin = origin_warehouse + @destination = destination + @lines = lines end def call @@ -100,18 +116,18 @@ def payload { 'origin_zip_code' => @origin.zip_code, 'origin_address' => @origin.address, - 'destination_zip_code' => @order.customer_zip_code, - 'destination_address' => @order.customer_address, + 'destination_zip_code' => @destination[:zip_code], + 'destination_address' => @destination[:address], 'total_weight' => total_weight, - 'total_items' => @order.order_items.sum(:quantity) + 'total_items' => @lines.sum { |_product, quantity| quantity } } end - # Peso del paquete: la suma de peso × cantidad de cada ítem. `products.weight` + # Peso del paquete: la suma de peso × cantidad de cada línea. `products.weight` # es decimal y arranca en 0, así que un producto sin peso cargado no rompe la # cotización — suma cero. def total_weight - @order.order_items.includes(:product).sum { |item| item.product.weight * item.quantity } + @lines.sum { |product, quantity| product.weight * quantity } end end end diff --git a/spec/poros/shipments/quote_shipment_spec.rb b/spec/poros/shipments/quote_shipment_spec.rb index 54ed89a..52f14b0 100644 --- a/spec/poros/shipments/quote_shipment_spec.rb +++ b/spec/poros/shipments/quote_shipment_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' RSpec.describe Shipments::QuoteShipment, type: :poro do - subject(:quotes) { described_class.new(order: order, origin_warehouse: origin).call } + subject(:quotes) { described_class.for_order(order: order, origin_warehouse: origin).call } let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } let(:origin) do @@ -120,6 +120,36 @@ def integrate(service) end end + # Lo que viaja es el paquete de la orden: peso × cantidad de cada línea y el + # total de bultos, además de origen y destino. + context 'when quoting an order' do + let(:rates) { 'https://fast.test/rates' } + + before do + sensor = Product.create!(company: company, sku: 'S-1', name: 'Sensor', weight: 0.5) + cable = Product.create!(company: company, sku: 'C-1', name: 'Cable', weight: 2) + order.order_items.create!(product: sensor, quantity: 3, unit_price: 100) + order.order_items.create!(product: cable, quantity: 2, unit_price: 100) + + integrate(Service.create!(service_name: 'Fast', type: 'courier', http_method: 'POST', + uri: rates, request_value_mapper: {}, response_value_mapper: {}, + request_mapper: { 'desde' => 'origin_zip_code', + 'hasta' => 'destination_zip_code', + 'kilos' => 'total_weight', + 'bultos' => 'total_items' }, + response_mapper: { 'precio' => 'shipping_cost' })) + stub_request(:post, rates).to_return(status: 200, body: { precio: 900 }.to_json) + end + + it 'sends its weight, its item count, its origin and its destination' do + quotes + + expect(WebMock).to have_requested(:post, rates) + .with(body: hash_including('desde' => '1900', 'hasta' => '5000', + 'kilos' => '5.5', 'bultos' => 5)) + end + end + context 'with an inactive courier' do before do integration = integrate(quoting_service('Fast', 'https://fast.test/rates')) From 375e412ef1f6be66fb53f1e20243d84f35c9194c Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 24 Sep 2026 21:17:18 -0300 Subject: [PATCH 03/11] feat: [TESIS-131] offer only the quotes that can be dispatched MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- app/poros/shipments/quote_shipment.rb | 45 +++++++-- spec/poros/shipments/quote_shipment_spec.rb | 101 +++++++++++++++---- spec/requests/api/v1/shipment_quotes_spec.rb | 26 ++++- 3 files changed, 139 insertions(+), 33 deletions(-) diff --git a/app/poros/shipments/quote_shipment.rb b/app/poros/shipments/quote_shipment.rb index 4c99fd3..0f20ed9 100644 --- a/app/poros/shipments/quote_shipment.rb +++ b/app/poros/shipments/quote_shipment.rb @@ -56,15 +56,35 @@ def call private - # Sólo las integraciones activas cuyo template sabe cotizar. Un courier puede - # tener también una plantilla de despacho: ésa no contesta tarifas y pedírsela - # sería llamar al endpoint equivocado (ver Service#quotes_shipping?). + # Las integraciones a las que se les piden tarifas: activas, con un template + # que sabe cotizar (una plantilla de despacho no contesta tarifas, ver + # Service#quotes_shipping?) y con una integración que despache por el mismo + # courier (TESIS-131). + # + # Lo último se filtra acá y no después de cotizar: una opción que no se puede + # despachar no es una opción, y pedirle la tarifa sería gastar una llamada al + # proveedor —y hacer esperar al operador— por algo que no se va a mostrar. def integrations - @integrations ||= CompanyIntegration.where(is_active: true) - .joins(:service) - .where(services: { type: Service::COURIER }) - .includes(:service) - .select { |ci| ci.service.quotes_shipping? } + @integrations ||= active_couriers.select do |ci| + ci.service.quotes_shipping? && dispatchers.key?(ci.service_id) + end + end + + # La integración que despacha por cada plantilla de cotización, indexada por + # el id de esa plantilla. La cotización la contesta una plantilla y la + # etiqueta la emite otra; el vínculo lo declara `Service#quote_service`. + def dispatchers + @dispatchers ||= active_couriers.select { |ci| ci.service.dispatches_shipment? } + .select { |ci| ci.service.quote_service_id.present? } + .index_by { |ci| ci.service.quote_service_id } + end + + def active_couriers + @active_couriers ||= CompanyIntegration.where(is_active: true) + .joins(:service) + .where(services: { type: Service::COURIER }) + .includes(:service) + .to_a end # El rescate va DENTRO del hilo: `Thread#value` re-levanta la excepción del @@ -98,12 +118,19 @@ def quote_with(integration, body) # Una respuesta sin costo no es una opción que el usuario pueda elegir: se # descarta como si el operador no hubiera contestado, en vez de ofrecer una # tarifa vacía. `estimated_days` sí puede faltar — es informativo. + # + # El nombre es el del courier (la plantilla que despacha) y no el de su + # plantilla de cotización: el operador elige «Andreani», no + # «Andreani - Cotización». `dispatch_integration_id` es lo que se le manda al + # despacho para confirmar esta opción. def normalize(integration, parsed) cost = parsed[COST_KEY] return nil if cost.blank? + dispatcher = dispatchers.fetch(integration.service_id) { company_integration_id: integration.id, - provider_name: integration.service.service_name, + dispatch_integration_id: dispatcher.id, + provider_name: dispatcher.service.service_name, shipping_cost: BigDecimal(cost.to_s), estimated_days: parsed[DAYS_KEY]&.to_i } rescue ArgumentError diff --git a/spec/poros/shipments/quote_shipment_spec.rb b/spec/poros/shipments/quote_shipment_spec.rb index 52f14b0..9213e7c 100644 --- a/spec/poros/shipments/quote_shipment_spec.rb +++ b/spec/poros/shipments/quote_shipment_spec.rb @@ -14,11 +14,25 @@ customer_address: 'Av. Siempreviva 742') end - def quoting_service(name, uri) - Service.create!(service_name: name, type: 'courier', http_method: 'POST', uri: uri, - request_mapper: { 'cp' => 'destination_zip_code' }, - response_mapper: { 'precio' => 'shipping_cost', 'dias' => 'estimated_days' }, - request_value_mapper: {}, response_value_mapper: {}) + # La plantilla que cotiza por un courier. Salvo que se pida lo contrario, el + # courier queda completo: su plantilla de despacho vinculada a ésta + # (TESIS-131) y ya integrada, porque sin ella la opción no se ofrecería. + def quoting_service(name, uri, request_mapper: { 'cp' => 'destination_zip_code' }, + dispatchable: true) + quote = Service.create!(service_name: "#{name} - Cotización", type: 'courier', + http_method: 'POST', uri: uri, request_mapper: request_mapper, + response_mapper: { 'precio' => 'shipping_cost', + 'dias' => 'estimated_days' }, + request_value_mapper: {}, response_value_mapper: {}) + integrate(dispatch_service(name, quote_service: quote)) if dispatchable + quote + end + + def dispatch_service(name, quote_service: nil) + Service.create!(service_name: name, type: 'courier', http_method: 'POST', + uri: "https://#{name.downcase}.test/ordenes", quote_service: quote_service, + response_mapper: { 'numero' => 'tracking_number' }, + request_mapper: {}, request_value_mapper: {}, response_value_mapper: {}) end def integrate(service) @@ -26,14 +40,19 @@ def integrate(service) credentials: { 'access_token' => 'T' }, is_active: true) end + # Un courier listo para cotizar. Devuelve la integración que cotiza. + def courier(name, uri, **quote_options) + integrate(quoting_service(name, uri, **quote_options)) + end + before { Current.company_id = company.id } after { Current.company_id = nil } context 'with two couriers that answer' do before do - integrate(quoting_service('Fast', 'https://fast.test/rates')) - integrate(quoting_service('Cheap', 'https://cheap.test/rates')) + courier('Fast', 'https://fast.test/rates') + courier('Cheap', 'https://cheap.test/rates') stub_request(:post, 'https://fast.test/rates') .to_return(status: 200, body: { precio: 2500.0, dias: 1 }.to_json) stub_request(:post, 'https://cheap.test/rates') @@ -56,6 +75,53 @@ def integrate(service) it 'reports which integration produced each option' do expect(quotes.pluck(:company_integration_id)).to all(be_present) end + + # Lo que el despacho necesita para confirmar la opción elegida (TESIS-131): + # la integración que emite la etiqueta, no la que contestó la tarifa. + it 'reports the integration that dispatches each option' do + dispatcher = CompanyIntegration.joins(:service).find_by!(services: { service_name: 'Cheap' }) + + expect(quotes.first[:dispatch_integration_id]).to eq(dispatcher.id) + end + + # El operador elige «Cheap», no «Cheap - Cotización». + it 'names each option after its courier, not after its quote template' do + expect(quotes.pluck(:provider_name)).not_to include(a_string_ending_with('Cotización')) + end + end + + # Una opción que no se puede despachar no se ofrece, y ni siquiera se cotiza: + # sería hacer esperar al operador por algo que no va a poder confirmar. + context 'with a quote template that no dispatch template points at' do + before do + integrate(quoting_service('Orphan', 'https://orphan.test/rates', dispatchable: false)) + stub_request(:post, 'https://orphan.test/rates') + .to_return(status: 200, body: { precio: 900.0 }.to_json) + end + + it 'does not offer it' do + expect(quotes).to eq([]) + end + + it 'is not even asked for a price' do + quotes + + expect(WebMock).not_to have_requested(:post, 'https://orphan.test/rates') + end + end + + context 'when the integration that dispatches is inactive' do + before do + courier('Fast', 'https://fast.test/rates') + CompanyIntegration.joins(:service).find_by!(services: { service_name: 'Fast' }) + .update!(is_active: false) + stub_request(:post, 'https://fast.test/rates') + .to_return(status: 200, body: { precio: 900.0 }.to_json) + end + + it 'does not offer the option' do + expect(quotes).to eq([]) + end end # El paralelismo NO se verifica acá, y no por olvido: WebMock no es @@ -64,8 +130,8 @@ def integrate(service) # está hecha contra un servidor HTTP real y documentada en el PR. context 'when one courier is down' do before do - integrate(quoting_service('Fast', 'https://fast.test/rates')) - integrate(quoting_service('Broken', 'https://broken.test/rates')) + courier('Fast', 'https://fast.test/rates') + courier('Broken', 'https://broken.test/rates') stub_request(:post, 'https://fast.test/rates') .to_return(status: 200, body: { precio: 2500.0, dias: 1 }.to_json) stub_request(:post, 'https://broken.test/rates').to_return(status: 500) @@ -82,7 +148,7 @@ def integrate(service) context 'when every courier fails' do before do - integrate(quoting_service('Broken', 'https://broken.test/rates')) + courier('Broken', 'https://broken.test/rates') stub_request(:post, 'https://broken.test/rates').to_timeout end @@ -93,7 +159,7 @@ def integrate(service) context 'when a courier answers without a price' do before do - integrate(quoting_service('Empty', 'https://empty.test/rates')) + courier('Empty', 'https://empty.test/rates') stub_request(:post, 'https://empty.test/rates') .to_return(status: 200, body: { dias: 3 }.to_json) end @@ -131,13 +197,10 @@ def integrate(service) order.order_items.create!(product: sensor, quantity: 3, unit_price: 100) order.order_items.create!(product: cable, quantity: 2, unit_price: 100) - integrate(Service.create!(service_name: 'Fast', type: 'courier', http_method: 'POST', - uri: rates, request_value_mapper: {}, response_value_mapper: {}, - request_mapper: { 'desde' => 'origin_zip_code', - 'hasta' => 'destination_zip_code', - 'kilos' => 'total_weight', - 'bultos' => 'total_items' }, - response_mapper: { 'precio' => 'shipping_cost' })) + courier('Fast', rates, request_mapper: { 'desde' => 'origin_zip_code', + 'hasta' => 'destination_zip_code', + 'kilos' => 'total_weight', + 'bultos' => 'total_items' }) stub_request(:post, rates).to_return(status: 200, body: { precio: 900 }.to_json) end @@ -152,7 +215,7 @@ def integrate(service) context 'with an inactive courier' do before do - integration = integrate(quoting_service('Fast', 'https://fast.test/rates')) + integration = courier('Fast', 'https://fast.test/rates') integration.update!(is_active: false) end diff --git a/spec/requests/api/v1/shipment_quotes_spec.rb b/spec/requests/api/v1/shipment_quotes_spec.rb index da5e2fc..2bbce6f 100644 --- a/spec/requests/api/v1/shipment_quotes_spec.rb +++ b/spec/requests/api/v1/shipment_quotes_spec.rb @@ -19,12 +19,22 @@ def auth_headers(user) { 'Authorization' => "Bearer #{response.parsed_body['token']}" } end + # Un courier con sus dos plantillas: la que cotiza y la que despacha, vinculada + # a la primera (TESIS-131). Sin la de despacho la opción no se ofrecería. def courier(name, uri) - service = Service.create!(service_name: name, type: 'courier', http_method: 'POST', uri: uri, - request_mapper: { 'cp' => 'destination_zip_code' }, - response_mapper: { 'precio' => 'shipping_cost', - 'dias' => 'estimated_days' }, - request_value_mapper: {}, response_value_mapper: {}) + quote = Service.create!(service_name: "#{name} - Cotización", type: 'courier', + http_method: 'POST', uri: uri, + request_mapper: { 'cp' => 'destination_zip_code' }, + response_mapper: { 'precio' => 'shipping_cost', + 'dias' => 'estimated_days' }, + request_value_mapper: {}, response_value_mapper: {}) + integrate(Service.create!(service_name: name, type: 'courier', http_method: 'POST', + uri: "#{uri}/ordenes", quote_service: quote, + response_mapper: { 'numero' => 'tracking_number' })) + integrate(quote) + end + + def integrate(service) CompanyIntegration.create!(company: company, service: service, credentials: { 'access_token' => 'T' }, is_active: true) end @@ -58,6 +68,12 @@ def quote(warehouse_id: warehouse.id, order_id: order.id, auth: headers) expect(option['shipping_cost'].to_f).to eq(2500.0) expect(option['estimated_days']).to eq(3) end + + it 'says which integration dispatches the option' do + dispatcher = CompanyIntegration.joins(:service).find_by!(services: { service_name: 'Fast' }) + + expect(response.parsed_body['data'].first['dispatch_integration_id']).to eq(dispatcher.id) + end end # Sin opciones no es un error: el front distingue "ningún operador contestó" From c84fbada9b7bbdb7de1560467993891fadbe48d4 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 24 Sep 2026 21:20:45 -0300 Subject: [PATCH 04/11] feat: [TESIS-131] quote a draft before the order exists 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 --- .../api/v1/draft_quotes_controller.rb | 97 ++++++++++++ config/routes.rb | 4 + spec/requests/api/v1/draft_quotes_spec.rb | 143 ++++++++++++++++++ 3 files changed, 244 insertions(+) create mode 100644 app/controllers/api/v1/draft_quotes_controller.rb create mode 100644 spec/requests/api/v1/draft_quotes_spec.rb diff --git a/app/controllers/api/v1/draft_quotes_controller.rb b/app/controllers/api/v1/draft_quotes_controller.rb new file mode 100644 index 0000000..053f12c --- /dev/null +++ b/app/controllers/api/v1/draft_quotes_controller.rb @@ -0,0 +1,97 @@ +# frozen_string_literal: true + +module Api + module V1 + # Cotización de un alta que todavía no se confirmó (TESIS-131). + # + # La cotización de TESIS-46 cuelga de una orden, y crear la orden descuenta + # el stock. El paso 3 del alta manual (TESIS-59) necesita mostrar las tarifas + # ANTES de que el operador confirme, así que acá se cotiza el paquete con lo + # que el asistente ya juntó: depósito de origen, destino y líneas. La orden se + # crea una sola vez, cuando el operador elige y confirma. + class DraftQuotesController < ApplicationController + # El parámetro que falta es un 400 de contrato, con el mismo cuerpo + # `{ error }` que el resto de la API. + rescue_from ActionController::ParameterMissing, with: :render_bad_request + + def create + # Cotizar un borrador es el paso previo a darlo de alta: se autoriza como + # crear una orden. Cada request va a los couriers con las credenciales de + # la empresa, pero no lee ni toca nada que no sea del propio tenant. + authorize Order, :create? + + quotes = Shipments::QuoteShipment.new( + origin_warehouse: origin_warehouse, destination: destination, lines: lines + ).call + + # 200 y una lista vacía cuando nadie contestó, igual que la cotización de + # una orden: el front distingue «sin opciones» de «falló la cotización». + render json: { data: quotes }, status: :ok + end + + private + + def quote_params + params.expect(quote: [:origin_warehouse_id, :destination_zip_code, :destination_address, + { items: [%i[product_id quantity]] }]) + end + + # find y no find_by: Warehouse es CompanyScoped, así que un id de otra + # empresa levanta RecordNotFound -> 404 en vez de revelar que existe. + def origin_warehouse + Warehouse.find(required(:origin_warehouse_id)) + end + + # El código postal es lo que cotizan todas las plantillas: sin él no hay + # tarifa posible. La dirección viaja si está, porque algunas la piden. + def destination + { zip_code: required(:destination_zip_code).to_s.strip, + address: quote_params[:destination_address].to_s.strip.presence } + end + + # Pares [producto, cantidad]. Los productos se buscan dentro del tenant: uno + # ajeno no aparece y responde 404, como el resto de la API. + def lines + products = Product.where(id: items.map(&:first)).index_by(&:id) + items.map do |product_id, quantity| + [products.fetch(product_id) { raise ActiveRecord::RecordNotFound }, quantity] + end + end + + # El mismo tope que el alta (OrdersController::MAX_ITEMS): cotizar algo que + # después no se podría crear no le sirve a nadie. + def items + @items ||= begin + raw = quote_params[:items] + raise ActionController::ParameterMissing, :items if raw.blank? + + limit = OrdersController::MAX_ITEMS + raise MalformedParameterError, "items exceeds maximum of #{limit}" if raw.size > limit + + raw.map do |item| + [positive_integer(item, :product_id), positive_integer(item, :quantity)] + end + end + end + + # «Bien formado» acá es un entero positivo. Un 0, un negativo o un texto + # no son un producto ni una cantidad que se pueda enviar. + def positive_integer(item, key) + value = Integer(item[key].to_s, exception: false) + return value if value&.positive? + + raise MalformedParameterError, "each item needs a positive integer #{key}" + end + + # `expect` cubre la clave ausente, no el valor vacío: sin esto un id en + # blanco llegaba a `find('')` y salía como 404, diciéndole al cliente que el + # recurso no existe cuando lo que falta es el parámetro. + def required(name) + value = quote_params[name] + raise ActionController::ParameterMissing, name if value.blank? + + value + end + end + end +end diff --git a/config/routes.rb b/config/routes.rb index 10de6e3..c5e5dab 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -35,6 +35,10 @@ end end + # Cotización de un alta que todavía no existe (TESIS-131): cuelga de la + # raíz y no de una orden, porque lo que se cotiza es el borrador. + resources :quotes, only: %i[create], controller: 'draft_quotes' + resources :orders, only: %i[index show create update] do resources :quotes, only: %i[create], controller: 'shipment_quotes' diff --git a/spec/requests/api/v1/draft_quotes_spec.rb b/spec/requests/api/v1/draft_quotes_spec.rb new file mode 100644 index 0000000..2f26e38 --- /dev/null +++ b/spec/requests/api/v1/draft_quotes_spec.rb @@ -0,0 +1,143 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Cotizar un alta antes de crear la orden (TESIS-131). +RSpec.describe 'Draft quotes API', type: :request do + let(:company) { Company.create!(name: 'Tenant A', tax_id: '30-11111111-1') } + let(:user) { User.create!(email: 'a@example.com', password: 'password123', company: company) } + let(:headers) { auth_headers(user) } + let(:rates) { 'https://fast.test/rates' } + + def warehouse + @warehouse ||= Warehouse.create!(company: company, name: 'Central', zip_code: '1900', + address: 'Calle 1') + end + + def product + @product ||= Product.create!(company: company, sku: 'S-1', name: 'Sensor', weight: 1.5) + end + + def auth_headers(user) + post '/api/v1/auth/login', params: { email: user.email, password: 'password123' }, + headers: { 'X-Tenant-Slug' => user.company.slug } + { 'Authorization' => "Bearer #{response.parsed_body['token']}" } + end + + # Un courier con sus dos plantillas vinculadas: la que cotiza y la que despacha. + def courier + quote = Service.create!(service_name: 'Fast - Cotización', type: 'courier', + http_method: 'POST', uri: rates, + request_mapper: { 'cp' => 'destination_zip_code', + 'kilos' => 'total_weight' }, + response_mapper: { 'precio' => 'shipping_cost', + 'dias' => 'estimated_days' }) + dispatch = Service.create!(service_name: 'Fast', type: 'courier', http_method: 'POST', + uri: 'https://fast.test/ordenes', quote_service: quote, + response_mapper: { 'numero' => 'tracking_number' }) + [quote, dispatch].map do |service| + CompanyIntegration.create!(company: company, service: service, + credentials: { 'access_token' => 'T' }, is_active: true) + end + end + + def draft(**overrides) + { origin_warehouse_id: warehouse.id, destination_zip_code: '5000', + destination_address: 'Av. Siempreviva 742', + items: [{ product_id: product.id, quantity: 2 }] }.merge(overrides) + end + + def quote_draft(body = draft, auth: headers) + post '/api/v1/quotes', params: { quote: body }, headers: auth, as: :json + end + + it 'returns 401 without a token' do + post '/api/v1/quotes', params: { quote: draft }, as: :json + + expect(response).to have_http_status(:unauthorized) + end + + context 'with a courier that answers' do + before do + courier + stub_request(:post, rates).to_return(status: 200, body: { precio: 2500.0, dias: 3 }.to_json) + end + + it 'returns the options in the same shape as the quote of an order', :aggregate_failures do + quote_draft + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['data'].first.keys) + .to match_array(%w[company_integration_id dispatch_integration_id provider_name + shipping_cost estimated_days]) + end + + it 'quotes the parcel of the draft: its weight and its destination' do + quote_draft + + expect(WebMock).to have_requested(:post, rates) + .with(body: hash_including('cp' => '5000', 'kilos' => '3.0')) + end + + # El punto de la card: cotizar no crea la orden ni toca el stock. + it 'does not create an order' do + expect { quote_draft }.not_to change(Order, :count) + end + end + + it 'returns 200 with an empty list when no courier answers' do + quote_draft + + expect(response.parsed_body).to eq('data' => []) + end + + describe 'isolation between companies' do + let(:other) { Company.create!(name: 'Tenant B', tax_id: '30-22222222-2') } + + it 'answers 404 for a warehouse of another company' do + foreign = Warehouse.create!(company: other, name: 'Ajeno', zip_code: '1', address: 'x') + quote_draft(draft(origin_warehouse_id: foreign.id)) + + expect(response).to have_http_status(:not_found) + end + + it 'answers 404 for a product of another company' do + foreign = Product.create!(company: other, sku: 'X-1', name: 'Ajeno') + quote_draft(draft(items: [{ product_id: foreign.id, quantity: 1 }])) + + expect(response).to have_http_status(:not_found) + end + end + + describe 'a malformed draft' do + { + 'without the origin warehouse' => { origin_warehouse_id: nil }, + 'without the destination zip code' => { destination_zip_code: '' }, + 'without items' => { items: [] }, + 'with a quantity of zero' => { items: [{ product_id: 1, quantity: 0 }] }, + 'with a quantity that is not a number' => { items: [{ product_id: 1, quantity: 'dos' }] }, + 'with an item that is not an object' => { items: ['S-1'] } + }.each do |label, overrides| + it "returns 400 #{label}" do + quote_draft(draft(**overrides)) + + expect(response).to have_http_status(:bad_request) + end + end + + it 'says which parameter is missing, like the rest of the API' do + quote_draft(draft(destination_zip_code: '')) + + expect(response.parsed_body['error']).to include('destination_zip_code') + end + + it 'returns 400 above the item limit of an order' do + items = Array.new(Api::V1::OrdersController::MAX_ITEMS + 1) do + { product_id: product.id, quantity: 1 } + end + quote_draft(draft(items: items)) + + expect(response.parsed_body['error']).to include('maximum') + end + end +end From c7e25b7ceb1897e791d681271fe9cdbef21d8a5f Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 24 Sep 2026 21:24:48 -0300 Subject: [PATCH 05/11] feat: [TESIS-131] keep the confirmed shipping cost on dispatch 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 --- .../api/v1/shipments_controller.rb | 18 ++++++++-- app/poros/shipments/confirm_dispatch.rb | 15 ++++++-- .../api/v1/shipment_dispatches_spec.rb | 36 +++++++++++++++++-- 3 files changed, 63 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/v1/shipments_controller.rb b/app/controllers/api/v1/shipments_controller.rb index 17b36b1..c62caf6 100644 --- a/app/controllers/api/v1/shipments_controller.rb +++ b/app/controllers/api/v1/shipments_controller.rb @@ -71,7 +71,7 @@ def confirm dispatched = Shipments::ConfirmDispatch.new( shipment: shipment, company_integration: courier_integration, - origin_warehouse: origin_warehouse + origin_warehouse: origin_warehouse, shipping_cost: shipping_cost ).call render json: ShipmentSerializer.render(dispatched), status: :ok @@ -112,7 +112,21 @@ def origin_warehouse end def dispatch_params - params.expect(dispatch: %i[company_integration_id origin_warehouse_id]) + params.expect(dispatch: %i[company_integration_id origin_warehouse_id shipping_cost]) + end + + # El costo de la opción que el operador confirmó al cotizar (TESIS-131). Se + # valida acá, antes de llamar al courier, para no gastar una etiqueta en un + # despacho que después no se podría guardar. Opcional: sin él, el despacho + # funciona como antes. + def shipping_cost + raw = dispatch_params[:shipping_cost] + return nil if raw.blank? + + cost = BigDecimal(raw.to_s, exception: false) + return cost if cost && !cost.negative? + + raise MalformedParameterError, 'shipping_cost must be a number, zero or greater' end # `expect` cubre la clave ausente, no el valor vacío: sin esto un id en diff --git a/app/poros/shipments/confirm_dispatch.rb b/app/poros/shipments/confirm_dispatch.rb index 764d2fc..fdcf180 100644 --- a/app/poros/shipments/confirm_dispatch.rb +++ b/app/poros/shipments/confirm_dispatch.rb @@ -31,11 +31,15 @@ class ConfirmDispatch < ApplicationPoro # es NOT NULL y es lo que la pantalla muestra como lo que pasó (TESIS-60). INITIAL_EXTERNAL_STATUS = 'Etiqueta generada' - def initialize(shipment:, company_integration:, origin_warehouse:) + # `shipping_cost` es el de la opción que el operador confirmó al cotizar + # (TESIS-131). Es opcional: un despacho que no lo trae deja el costo como + # estaba. + def initialize(shipment:, company_integration:, origin_warehouse:, shipping_cost: nil) super() @shipment = shipment @integration = company_integration @origin = origin_warehouse + @shipping_cost = shipping_cost end def call @@ -140,11 +144,18 @@ def persist(parsed) @shipment.update!(company_integration: @integration, tracking_number: tracking_number!(parsed), shipping_label_url: parsed[LABEL_KEY], - status: DISPATCHED_STATUS) + status: DISPATCHED_STATUS, + **confirmed_cost) register_event end end + # El costo se escribe sólo si vino: sin él, el despacho no tiene por qué + # borrar uno que ya estuviera cargado. + def confirmed_cost + @shipping_cost.nil? ? {} : { shipping_cost: @shipping_cost } + end + # Sin número de seguimiento el despacho no sirve para nada: no se puede # seguir el paquete ni emparejar los eventos que el courier empuje después. # La etiqueta, en cambio, puede faltar — no todos los proveedores devuelven diff --git a/spec/requests/api/v1/shipment_dispatches_spec.rb b/spec/requests/api/v1/shipment_dispatches_spec.rb index 61d1717..0ee0340 100644 --- a/spec/requests/api/v1/shipment_dispatches_spec.rb +++ b/spec/requests/api/v1/shipment_dispatches_spec.rb @@ -43,10 +43,10 @@ def stub_courier(status: 200, body: { numero: 'AND-999', etiqueta: 'https://l.te end def dispatch_shipment(id: shipment.id, integration_id: integration.id, - warehouse_id: warehouse.id, auth: headers) + warehouse_id: warehouse.id, auth: headers, **extra) post "/api/v1/shipments/#{id}/dispatch", params: { dispatch: { company_integration_id: integration_id, - origin_warehouse_id: warehouse_id } }, + origin_warehouse_id: warehouse_id, **extra } }, headers: auth, as: :json end @@ -85,6 +85,38 @@ def dispatch_shipment(id: shipment.id, integration_id: integration.id, end end + # El costo de la opción que el operador confirmó al cotizar (TESIS-131). Sin + # esto el detalle de la orden mostraba el envío «a cotizar» para siempre. + describe 'the confirmed shipping cost' do + before { stub_courier } + + it 'is kept on the shipment and read back from it', :aggregate_failures do + dispatch_shipment(shipping_cost: 2500.5) + expect(response.parsed_body['shipping_cost']).to eq(2500.5) + + get "/api/v1/shipments/#{shipment.id}", headers: headers + expect(response.parsed_body['shipping_cost']).to eq(2500.5) + end + + it 'leaves the cost as it was when the dispatch does not bring one' do + shipment.update!(shipping_cost: 900) + dispatch_shipment + + expect(shipment.reload.shipping_cost).to eq(900) + end + + # Se valida antes de pedir la etiqueta: el courier la cobra, y gastarla en + # un despacho que después no se puede guardar es plata tirada. + { 'negative' => -1, 'not a number' => 'mucho' }.each do |label, cost| + it "answers 400 for a cost that is #{label}, without asking the courier", :aggregate_failures do + dispatch_shipment(shipping_cost: cost) + + expect(response).to have_http_status(:bad_request) + expect(WebMock).not_to have_requested(:post, 'https://andreani.test/ordenes') + end + end + end + # Criterio de la card: no se puede despachar dos veces el mismo paquete. it 'returns 409 when the shipment was already dispatched' do stub_courier From 9213133f755c869c1ceabac3efb088c7cad7b513 Mon Sep 17 00:00:00 2001 From: sim Date: Thu, 24 Sep 2026 21:26:33 -0300 Subject: [PATCH 06/11] docs: [TESIS-131] record how the manual order wizard quotes and dispatches 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 --- .../adr/ADR-016-cotizacion-del-alta-manual.md | 84 +++++++++++++++++++ docs/guidelines/architecture.md | 5 +- 2 files changed, 88 insertions(+), 1 deletion(-) create mode 100644 docs/adr/ADR-016-cotizacion-del-alta-manual.md diff --git a/docs/adr/ADR-016-cotizacion-del-alta-manual.md b/docs/adr/ADR-016-cotizacion-del-alta-manual.md new file mode 100644 index 0000000..f3d4efb --- /dev/null +++ b/docs/adr/ADR-016-cotizacion-del-alta-manual.md @@ -0,0 +1,84 @@ +# ADR-016: Cotización del alta manual y despacho de la opción elegida + +**Fecha:** 2026-09-24 +**Estado:** Aceptado + +--- + +## Contexto + +El paso 3 del alta manual de órdenes (TESIS-59, diseño S07) sigue este recorrido: al entrar se cotiza contra todos los operadores, el operador elige una opción y **«Confirmar orden»** crea la orden y emite el despacho. Las piezas de la API existían por separado —cotizar (TESIS-46), crear el envío (TESIS-105, [ADR-012](ADR-012-creacion-de-envios.md)) y despacharlo (TESIS-47)—, pero juntas no alcanzaban para ese recorrido. Había tres huecos: + +1. **La cotización necesitaba una orden** (`POST /orders/:id/quotes`). Para mostrar las tarifas antes de confirmar había que crear la orden, y eso descuenta el stock. Si el operador abandonaba el paso, quedaba una orden `pending` que nadie podía cancelar: `Orders::UpdateOrder` deja la cancelación afuera a propósito ([ADR-013](ADR-013-modificacion-de-ordenes.md)). +2. **Una cotización no se podía despachar.** Cotizar y despachar son dos endpoints del proveedor y, por convención, dos `Service` («Andreani - Cotización» y «Andreani»). Cada opción informaba la integración de la plantilla que **cotiza**, y `Shipments::ConfirmDispatch` exige una que **despache**. Nada en el modelo decía que eran del mismo courier. +3. **El costo elegido se perdía.** El despacho no escribía `shipments.shipping_cost`, así que el detalle de la orden mostraba el envío «a cotizar» para siempre. + +## Decisión + +``` +Paso 3 del alta + │ + ├─ POST /api/v1/quotes cotiza el borrador, sin crear nada + │ { origin_warehouse_id, destination_zip_code, destination_address, + │ items: [{ product_id, quantity }] } + │ -> [{ company_integration_id, dispatch_integration_id, + │ provider_name, shipping_cost, estimated_days }] + │ + └─ «Confirmar orden» + ├─ POST /api/v1/orders crea la orden y descuenta el stock + ├─ POST /api/v1/orders/:id/shipment abre el envío en `pending` + └─ POST /api/v1/shipments/:id/dispatch + { company_integration_id: , + origin_warehouse_id, shipping_cost } +``` + +| Pieza | Rol | +| --------------------------------------- | ----------------------------------------------------------------------------------------- | +| `Api::V1::DraftQuotesController` | `POST /api/v1/quotes`: arma el paquete del borrador dentro del tenant y delega | +| `Shipments::QuoteShipment` | Cotiza un paquete (origen, destino y líneas); `.for_order` lo arma desde una orden | +| `Service#quote_service` | En la plantilla que despacha: la plantilla con la que se cotiza al mismo courier | +| `Shipments::ConfirmDispatch` | Guarda además el `shipping_cost` confirmado, si viene | + +### Se cotiza el paquete, no la orden + +`QuoteShipment` recibe el origen, el destino y pares `[producto, cantidad]`. El peso sigue saliendo de `products.weight` del lado del backend: el cliente manda qué lleva el paquete, no cuánto pesa. La cotización de una orden (`POST /orders/:id/quotes`) arma ese mismo contexto con `QuoteShipment.for_order` y responde igual que antes. + +**Alternativas descartadas:** + +- **Un estado `draft` de la orden** que no descuente stock hasta confirmar. Resolvía el hueco 1, pero tocaba los estados, el listado, los KPIs del panel y el momento del descuento: un cambio de dominio para un problema de secuencia. +- **Que el front mande el peso total.** Duplica en el cliente un dato que ya es del backend, y deja que el cliente decida sobre qué se cotiza. + +### El vínculo entre plantillas cuelga de la que despacha + +`services.quote_service_id` sigue el patrón de `tracking_service_id` ([ADR-014](ADR-014-pull-tracking-de-couriers.md)). El `Service` del proveedor es el que despacha, y sus plantillas auxiliares (la de seguimiento, la de cotización) cuelgan de él. La validación también es la misma: sólo couriers, nunca la propia plantilla, y sólo una plantilla que cotice (`Service#quotes_shipping?`). + +Con el vínculo, cada opción informa `dispatch_integration_id` y se nombra por el courier («Andreani», no «Andreani - Cotización»). + +**Una plantilla de cotización sin integración de despacho activa no se consulta.** Se filtra antes de abrir los hilos, no después de cotizar: una opción que el operador no puede confirmar no es una opción, y pedirle la tarifa sería hacerlo esperar por algo que no se va a mostrar. + +### El costo que se guarda es el que se confirmó + +El despacho acepta `shipping_cost` opcional y lo escribe en el envío. Se validó la alternativa de volver a cotizar al despachar y guardar lo que conteste el courier, y se descartó por dos motivos: es una segunda llamada externa dentro del request, y el precio podría no coincidir con el que el operador aceptó segundos antes. + +El valor se valida antes de pedir la etiqueta (que el courier cobra): un costo negativo o que no es un número responde 400 sin haber llamado a nadie. Un despacho sin costo deja el que hubiera. + +## Consecuencias + +**A favor** + +- El stock se descuenta una sola vez, cuando el operador confirma. Cotizar no crea nada. +- Una opción cotizada siempre se puede despachar: la que no tendría con qué, no aparece. +- El detalle de la orden muestra el costo que se eligió. + +**En contra** + +- **Confirmar son tres requests encadenados, y no es atómico.** El despacho llama a un courier externo y no puede ir en la misma transacción que el alta. Si falla, la orden queda creada con su envío `pending`, que es un estado válido y se puede volver a despachar. Lo resuelve el front, reintentando el despacho sobre la orden ya creada. +- **El precio puede cambiar entre la cotización y el despacho.** Se guarda el que se confirmó, no el que el courier cobre al emitir la etiqueta. Si un proveedor empieza a devolver el costo en la respuesta del despacho, esa es la fuente mejor, y este ADR se revisa. +- Un courier cargado sin su `quote_service` no se ofrece al cotizar. En el panel de administración, el vínculo se edita en la plantilla que despacha, junto a la de seguimiento. + +## Referencias + +- TESIS-131 — la card de este ADR +- TESIS-59 — el paso 3 del alta manual, que consume este flujo +- TESIS-46 / TESIS-47 / TESIS-105 — cotización, despacho y alta del envío +- [ADR-012](ADR-012-creacion-de-envios.md), [ADR-014](ADR-014-pull-tracking-de-couriers.md) diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index cddf155..27b0df8 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -388,7 +388,10 @@ respuesta un dato que no sirve más tarde. del request**, no desde un job. El motivo es el producto: el usuario está esperando la lista de tarifas para elegir una. Devolverla por un job obligaría a sondear o a abrir un canal de tiempo real para un dato que se consume en el acto -y que caduca enseguida. +y que caduca enseguida. `POST /api/v1/quotes` (TESIS-131) es la misma cotización +sobre un alta que todavía no se confirmó, y corre por el mismo caso de uso: la +excepción es una sola, no dos (ver +[ADR-016](../adr/ADR-016-cotizacion-del-alta-manual.md)). `POST /api/v1/shipments/:id/dispatch` (TESIS-47) hace lo propio con el operador que el usuario eligió: le pide la etiqueta y devuelve el número de seguimiento y From 758e70728d505159a03463d3b356f9961759f63b Mon Sep 17 00:00:00 2001 From: Santiago Natalichio Bestosini Date: Fri, 25 Sep 2026 20:12:32 -0300 Subject: [PATCH 07/11] fix: [TESIS-131] renumber the quote service migration after TESIS-82 TESIS-82 took 20260924120000 on master, and two migrations with the same version stop Rails from migrating. Co-Authored-By: Claude Opus 5.5 --- ...vices.rb => 20260924130000_add_quote_service_to_services.rb} | 0 db/schema.rb | 2 +- 2 files changed, 1 insertion(+), 1 deletion(-) rename db/migrate/{20260924120000_add_quote_service_to_services.rb => 20260924130000_add_quote_service_to_services.rb} (100%) diff --git a/db/migrate/20260924120000_add_quote_service_to_services.rb b/db/migrate/20260924130000_add_quote_service_to_services.rb similarity index 100% rename from db/migrate/20260924120000_add_quote_service_to_services.rb rename to db/migrate/20260924130000_add_quote_service_to_services.rb diff --git a/db/schema.rb b/db/schema.rb index 9177511..a171bca 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_09_24_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_09_24_130000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" From 95d00fabee04b50a68264fba57244db54a12cd83 Mon Sep 17 00:00:00 2001 From: Santiago Natalichio Bestosini Date: Fri, 25 Sep 2026 20:17:51 -0300 Subject: [PATCH 08/11] fix: [TESIS-131] check the confirmed cost against the shipment before 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 --- .../api/v1/shipments_controller.rb | 15 ++++++------ app/models/shipment.rb | 9 +++++++- app/poros/shipments/confirm_dispatch.rb | 18 +++++++++++++++ .../shipments/invalid_shipping_cost_error.rb | 13 +++++++++++ spec/models/shipment_spec.rb | 13 +++++++++++ spec/poros/shipments/confirm_dispatch_spec.rb | 23 +++++++++++++++++++ .../api/v1/shipment_dispatches_spec.rb | 15 +++++++++++- 7 files changed, 96 insertions(+), 10 deletions(-) create mode 100644 app/poros/shipments/invalid_shipping_cost_error.rb diff --git a/app/controllers/api/v1/shipments_controller.rb b/app/controllers/api/v1/shipments_controller.rb index c62caf6..0d08417 100644 --- a/app/controllers/api/v1/shipments_controller.rb +++ b/app/controllers/api/v1/shipments_controller.rb @@ -15,6 +15,7 @@ class ShipmentsController < ApplicationController rescue_from Shipments::AlreadyDispatchedError, with: :render_conflict rescue_from Shipments::InvalidCourierIntegrationError, with: :render_unprocessable rescue_from Shipments::DispatchResponseError, with: :render_bad_gateway + rescue_from Shipments::InvalidShippingCostError, with: :render_bad_request rescue_from Integrations::AdapterExecutionError, with: :render_courier_failure # El parámetro que falta es un 400 de contrato, no un 422 de negocio. rescue_from ActionController::ParameterMissing, with: :render_bad_request @@ -115,18 +116,16 @@ def dispatch_params params.expect(dispatch: %i[company_integration_id origin_warehouse_id shipping_cost]) end - # El costo de la opción que el operador confirmó al cotizar (TESIS-131). Se - # valida acá, antes de llamar al courier, para no gastar una etiqueta en un - # despacho que después no se podría guardar. Opcional: sin él, el despacho - # funciona como antes. + # El costo de la opción que el operador confirmó al cotizar (TESIS-131). + # Opcional: sin él, el despacho funciona como antes. Acá sólo se lee como + # número; el rango lo valida ConfirmDispatch contra el modelo, antes de + # llamar al courier, para que la regla viva en un solo lugar. def shipping_cost raw = dispatch_params[:shipping_cost] return nil if raw.blank? - cost = BigDecimal(raw.to_s, exception: false) - return cost if cost && !cost.negative? - - raise MalformedParameterError, 'shipping_cost must be a number, zero or greater' + BigDecimal(raw.to_s, exception: false) || + raise(MalformedParameterError, 'shipping_cost must be a number') end # `expect` cubre la clave ausente, no el valor vacío: sin esto un id en diff --git a/app/models/shipment.rb b/app/models/shipment.rb index 937456e..6cb352d 100644 --- a/app/models/shipment.rb +++ b/app/models/shipment.rb @@ -12,6 +12,12 @@ class Shipment < ApplicationRecord # cuota del proveedor. IN_FLIGHT_STATUSES = %w[ready_to_ship in_transit].freeze + # Tope de `shipping_cost`: la columna es decimal(10,2), así que lo más grande + # que entra es 99.999.999,99. Validarlo en el modelo hace que un costo fuera de + # rango sea un error de validación y no un RangeError de la base; el despacho + # lo prueba contra esta regla antes de pedir la etiqueta (TESIS-131). + MAX_SHIPPING_COST = 100_000_000 + belongs_to :company # La integración se asigna al inicializar el envío y puede no existir todavía # (se completa al confirmar el despacho con un courier). @@ -26,7 +32,8 @@ class Shipment < ApplicationRecord scope :in_flight, -> { where(status: IN_FLIGHT_STATUSES).where.not(tracking_number: nil) } validates :status, presence: true, inclusion: { in: STATUSES } - validates :shipping_cost, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true + validates :shipping_cost, numericality: { greater_than_or_equal_to: 0, + less_than: MAX_SHIPPING_COST }, allow_nil: true # Restricción 1 a 1 de la card: una orden no puede tener dos envíos. El índice # único sobre order_id (migración) es la garantía a nivel motor; la validación # del modelo da un mensaje de error limpio antes de llegar a la DB. diff --git a/app/poros/shipments/confirm_dispatch.rb b/app/poros/shipments/confirm_dispatch.rb index fdcf180..f2dcf16 100644 --- a/app/poros/shipments/confirm_dispatch.rb +++ b/app/poros/shipments/confirm_dispatch.rb @@ -45,6 +45,7 @@ def initialize(shipment:, company_integration:, origin_warehouse:, shipping_cost def call validate_integration! validate_status!(@shipment) + validate_cost! parsed = request_label persist(parsed) @@ -66,6 +67,23 @@ def validate_status!(shipment) raise AlreadyDispatchedError.new(shipment: shipment) end + # El costo se prueba contra la regla del modelo —la misma que aplica el + # `update!` de `persist`— y no contra una copia: si la columna cambia, la + # validación la sigue. Sin esto, un costo que el modelo rechaza (fuera de + # rango, NaN, infinito) pasaba, se pedía la etiqueta y el `update!` fallaba + # después: el envío seguía en `pending` y un reintento emitía otra etiqueta. + # + # Se valida un envío nuevo con sólo el costo para no tocar `@shipment` antes + # de la llamada externa; del resultado se lee únicamente `shipping_cost`. + def validate_cost! + return if @shipping_cost.nil? + + probe = Shipment.new(shipping_cost: @shipping_cost) + probe.validate + reasons = probe.errors.messages_for(:shipping_cost) + raise InvalidShippingCostError, reasons if reasons.any? + end + def validate_integration! reason = integration_problem return if reason.nil? diff --git a/app/poros/shipments/invalid_shipping_cost_error.rb b/app/poros/shipments/invalid_shipping_cost_error.rb new file mode 100644 index 0000000..bf68585 --- /dev/null +++ b/app/poros/shipments/invalid_shipping_cost_error.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +module Shipments + # El costo confirmado es un número, pero no uno que el envío pueda guardar: + # negativo, fuera del rango de la columna, NaN o infinito. Se detecta antes de + # pedir la etiqueta, así que el courier no se llegó a llamar. Es un dato del + # request, y el controller lo mapea a 400. + class InvalidShippingCostError < StandardError + def initialize(reasons) + super("shipping_cost #{reasons.to_sentence}") + end + end +end diff --git a/spec/models/shipment_spec.rb b/spec/models/shipment_spec.rb index 73feef6..415ee6b 100644 --- a/spec/models/shipment_spec.rb +++ b/spec/models/shipment_spec.rb @@ -40,6 +40,19 @@ expect(shipment.errors[:shipping_cost]).to include('must be greater than or equal to 0') end + # decimal(10,2): lo que no entra en la columna es un error de validación, no + # un RangeError de la base (TESIS-131). + it 'accepts the largest shipping_cost the column holds' do + shipment.shipping_cost = BigDecimal('99999999.99') + expect(shipment).to be_valid + end + + it 'rejects a shipping_cost that does not fit the column', :aggregate_failures do + shipment.shipping_cost = Shipment::MAX_SHIPPING_COST + expect(shipment).not_to be_valid + expect(shipment.errors[:shipping_cost]).to include('must be less than 100000000') + end + it 'is invalid without a company' do shipment.company = nil expect(shipment).not_to be_valid diff --git a/spec/poros/shipments/confirm_dispatch_spec.rb b/spec/poros/shipments/confirm_dispatch_spec.rb index f3472b4..79b719d 100644 --- a/spec/poros/shipments/confirm_dispatch_spec.rb +++ b/spec/poros/shipments/confirm_dispatch_spec.rb @@ -128,6 +128,29 @@ def attempt_dispatch(target = shipment, using: integration) expect(dispatch.reload.shipping_label_url).to be_nil end + # La regla del costo es la del modelo: lo que el `update!` rechazaría se + # rechaza antes de pedir la etiqueta, que el courier cobra (TESIS-131). + describe 'when the confirmed cost does not fit the shipment' do + def dispatch_costing(cost) + described_class.new(shipment: shipment, company_integration: integration, + origin_warehouse: warehouse, shipping_cost: cost).call + end + + it 'refuses a cost out of the range of the column, saying why' do + expect { dispatch_costing(BigDecimal('1e8')) } + .to raise_error(Shipments::InvalidShippingCostError, 'shipping_cost must be less than 100000000') + end + + it 'does not call the courier', :aggregate_failures do + stub = stub_courier + %w[1e8 NaN Infinity -1].each do |cost| + expect { dispatch_costing(BigDecimal(cost)) }.to raise_error(Shipments::InvalidShippingCostError) + end + + expect(stub).not_to have_been_requested + 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 0ee0340..d7ada4e 100644 --- a/spec/requests/api/v1/shipment_dispatches_spec.rb +++ b/spec/requests/api/v1/shipment_dispatches_spec.rb @@ -107,14 +107,27 @@ def dispatch_shipment(id: shipment.id, integration_id: integration.id, # Se valida antes de pedir la etiqueta: el courier la cobra, y gastarla en # un despacho que después no se puede guardar es plata tirada. - { 'negative' => -1, 'not a number' => 'mucho' }.each do |label, cost| + # + # Los tres últimos son números que BigDecimal acepta y la columna no + # (decimal(10,2)): antes pasaban el chequeo del controller, se pedía la + # etiqueta y el `update!` fallaba después, con el envío todavía en `pending` + # y el número de seguimiento perdido en el rollback. + { 'negative' => -1, 'not a number' => 'mucho', 'too big for the column' => 100_000_000, + 'NaN' => 'NaN', 'infinite' => 'Infinity' }.each do |label, cost| it "answers 400 for a cost that is #{label}, without asking the courier", :aggregate_failures do dispatch_shipment(shipping_cost: cost) expect(response).to have_http_status(:bad_request) expect(WebMock).not_to have_requested(:post, 'https://andreani.test/ordenes') + expect(shipment.reload).to have_attributes(status: 'pending', tracking_number: nil) end end + + it 'takes the largest cost the column holds' do + dispatch_shipment(shipping_cost: '99999999.99') + + expect(shipment.reload.shipping_cost).to eq(BigDecimal('99999999.99')) + end end # Criterio de la card: no se puede despachar dos veces el mismo paquete. From c3696c8bb2c385fa05f708c8fff4f5145dae789d Mon Sep 17 00:00:00 2001 From: Santiago Natalichio Bestosini Date: Fri, 25 Sep 2026 20:30:26 -0300 Subject: [PATCH 09/11] fix: [TESIS-131] give each quote template a single dispatcher 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 --- app/models/service.rb | 14 ++++++- app/poros/shipments/quote_shipment.rb | 2 + ...924130000_add_quote_service_to_services.rb | 5 +++ db/schema.rb | 2 +- spec/models/service_spec.rb | 42 +++++++++++++++++++ 5 files changed, 63 insertions(+), 2 deletions(-) diff --git a/app/models/service.rb b/app/models/service.rb index 89e0ebe..53572e8 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -162,11 +162,23 @@ def quote_service_quotes_shipping errors.add(:quote_service, reason) if reason end + # Las dos últimas reglas sostienen lo que la cotización asume: cada opción + # cotizada se despacha con UNA integración (QuoteShipment#dispatchers indexa + # por plantilla de cotización). Si dos plantillas de despacho compartieran el + # cotizador, una de las dos desaparecía de las opciones sin aviso; y una + # plantilla que no despacha con cotizador cargado es una configuración que no + # hace nada. El índice único de `quote_service_id` lo respalda en la base. def quote_service_problem return if quote_service.nil? return 'solo aplica a couriers' unless courier? return 'no puede ser la misma plantilla' if quote_service == self + return 'solo aplica a la plantilla con la que el courier despacha' unless dispatches_shipment? + return 'no es una plantilla de cotización' unless quote_service.quotes_shipping? - 'no es una plantilla de cotización' unless quote_service.quotes_shipping? + 'ya es la plantilla de cotización de otro courier' if quote_service_taken? + end + + def quote_service_taken? + self.class.where(quote_service_id: quote_service_id).where.not(id: id).exists? end end diff --git a/app/poros/shipments/quote_shipment.rb b/app/poros/shipments/quote_shipment.rb index 0f20ed9..6dcd9a3 100644 --- a/app/poros/shipments/quote_shipment.rb +++ b/app/poros/shipments/quote_shipment.rb @@ -73,6 +73,8 @@ def integrations # La integración que despacha por cada plantilla de cotización, indexada por # el id de esa plantilla. La cotización la contesta una plantilla y la # etiqueta la emite otra; el vínculo lo declara `Service#quote_service`. + # `index_by` no pierde a nadie porque una plantilla de cotización es de un + # solo despachador: lo valida Service y lo respalda un índice único. def dispatchers @dispatchers ||= active_couriers.select { |ci| ci.service.dispatches_shipment? } .select { |ci| ci.service.quote_service_id.present? } diff --git a/db/migrate/20260924130000_add_quote_service_to_services.rb b/db/migrate/20260924130000_add_quote_service_to_services.rb index 048929a..c00556d 100644 --- a/db/migrate/20260924130000_add_quote_service_to_services.rb +++ b/db/migrate/20260924130000_add_quote_service_to_services.rb @@ -9,8 +9,13 @@ class AddQuoteServiceToServices < ActiveRecord::Migration[8.1] # Mismo criterio que `tracking_service_id` (TESIS-49): es una relación entre # plantillas, no entre integraciones, y cuelga de la plantilla que despacha. # nullable: un courier sin plantilla de cotización simplemente no se ofrece. + # + # Único: una plantilla de cotización es de un solo despachador. La cotización + # devuelve una opción por cotizador y la despacha con UNA integración; si dos + # plantillas de despacho compartieran la de cotización, una se perdería. def change add_reference :services, :quote_service, + index: { unique: true }, foreign_key: { to_table: :services, on_delete: :nullify } end end diff --git a/db/schema.rb b/db/schema.rb index a171bca..c8bdc18 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -157,7 +157,7 @@ t.string "type", null: false t.datetime "updated_at", null: false t.string "uri", null: false - t.index ["quote_service_id"], name: "index_services_on_quote_service_id" + t.index ["quote_service_id"], name: "index_services_on_quote_service_id", unique: true t.index ["service_name"], name: "index_services_on_service_name", unique: true t.index ["tracking_service_id"], name: "index_services_on_tracking_service_id" t.check_constraint "type::text = ANY (ARRAY['ecommerce'::character varying, 'courier'::character varying]::text[])", name: "services_type_check" diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 3164217..10f427c 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -237,6 +237,48 @@ def template(name, uri) expect(quote_template).not_to be_valid end + # Otra plantilla de despacho del mismo proveedor, como «Andreani Express». + def other_dispatcher(**attrs) + described_class.create!(service_name: 'Andreani Express', type: 'courier', http_method: 'POST', + uri: 'https://api.andreani.test/express', + response_mapper: { 'numero' => 'tracking_number' }, **attrs) + end + + # Sin esto QuoteShipment#dispatchers se quedaba con uno de los dos y el otro + # desaparecía de las opciones sin aviso: lo encontró la review de TESIS-131. + it 'rejects a quote template that another courier already dispatches with', :aggregate_failures do + other_dispatcher(quote_service: quote_template) + courier.quote_service = quote_template + + expect(courier).not_to be_valid + expect(courier.errors[:quote_service]).to include('ya es la plantilla de cotización de otro courier') + end + + it 'lets the courier that has the quote template keep it' do + courier.update!(quote_service: quote_template) + courier.uri = 'https://api.andreani.test/v2/ordenes' + + expect(courier).to be_valid + end + + it 'backs the rule with a unique index' do + courier.update!(quote_service: quote_template) + + expect { other_dispatcher.update_column(:quote_service_id, quote_template.id) } # rubocop:disable Rails/SkipsModelValidations + .to raise_error(ActiveRecord::RecordNotUnique) + end + + # Una plantilla de cotización o de seguimiento con cotizador cargado es una + # configuración que no hace nada: la cotización sólo mira a los que despachan. + it 'rejects a quote template on a template that does not dispatch', :aggregate_failures do + tracking = described_class.new(service_name: 'Andreani - Seguimiento', type: 'courier', + http_method: 'GET', quote_service: quote_template, + uri: 'https://api.andreani.test/envios/:tracking_number') + + expect(tracking).not_to be_valid + expect(tracking.errors[:quote_service]).to include('solo aplica a la plantilla con la que el courier despacha') + end + it 'rejects a quote template on a sales channel' do service.quote_service = quote_template expect(service).not_to be_valid From 56711fcef25424a3ccca8f6bba4e33fdbb955be2 Mon Sep 17 00:00:00 2001 From: Santiago Natalichio Bestosini Date: Fri, 25 Sep 2026 20:32:41 -0300 Subject: [PATCH 10/11] feat: [TESIS-131] cap the draft quotes each user can ask for 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 --- .../api/v1/draft_quotes_controller.rb | 19 ++++++++ spec/requests/api/v1/draft_quotes_spec.rb | 45 +++++++++++++++++++ 2 files changed, 64 insertions(+) diff --git a/app/controllers/api/v1/draft_quotes_controller.rb b/app/controllers/api/v1/draft_quotes_controller.rb index 053f12c..2c31313 100644 --- a/app/controllers/api/v1/draft_quotes_controller.rb +++ b/app/controllers/api/v1/draft_quotes_controller.rb @@ -14,6 +14,20 @@ class DraftQuotesController < ApplicationController # `{ error }` que el resto de la API. rescue_from ActionController::ParameterMissing, with: :render_bad_request + # Es el primer endpoint autenticado que sale a los couriers sin dejar nada + # en la base: antes cotizar exigía crear la orden, que era un freno natural. + # Sin tope, un cliente en loop —un `useEffect` mal puesto que cotiza en + # cada tecla— genera tantas llamadas a los proveedores como quiera, con las + # credenciales de la empresa. Se cuenta por usuario y no por IP: todos los + # requests llegan autenticados, y un depósito detrás de un mismo NAT no + # debería compartir el cupo. El asistente cotiza al entrar al paso 3 y en + # cada reintento, así que 20 por minuto sobra para el uso normal. + QUOTES_PER_WINDOW = 20 + QUOTE_WINDOW = 1.minute + + rate_limit to: QUOTES_PER_WINDOW, within: QUOTE_WINDOW, only: :create, + by: -> { current_user.id }, with: :render_too_many_quotes + def create # Cotizar un borrador es el paso previo a darlo de alta: se autoriza como # crear una orden. Cada request va a los couriers con las credenciales de @@ -83,6 +97,11 @@ def positive_integer(item, key) raise MalformedParameterError, "each item needs a positive integer #{key}" end + def render_too_many_quotes + response.set_header('Retry-After', QUOTE_WINDOW.to_i.to_s) + render json: { error: 'Too many quotes, try again later' }, status: :too_many_requests + end + # `expect` cubre la clave ausente, no el valor vacío: sin esto un id en # blanco llegaba a `find('')` y salía como 404, diciéndole al cliente que el # recurso no existe cuando lo que falta es el parámetro. diff --git a/spec/requests/api/v1/draft_quotes_spec.rb b/spec/requests/api/v1/draft_quotes_spec.rb index 2f26e38..3b949eb 100644 --- a/spec/requests/api/v1/draft_quotes_spec.rb +++ b/spec/requests/api/v1/draft_quotes_spec.rb @@ -140,4 +140,49 @@ def quote_draft(body = draft, auth: headers) expect(response.parsed_body['error']).to include('maximum') end end + + # Cada cotización sale a los couriers sin crear nada (TESIS-131). En test la + # cache es :null_store y el límite nunca se alcanza; acá el contador usa una + # cache de verdad, sólo durante cada ejemplo. + describe 'rate limit' do + let(:counter) { ActiveSupport::Cache::MemoryStore.new } + + # Agota el cupo del usuario y olvida los requests que hizo para eso: lo que + # importa es qué pasa con el siguiente. + def use_up_quotes + Api::V1::DraftQuotesController::QUOTES_PER_WINDOW.times { quote_draft } + WebMock.reset_executed_requests! + end + + before do + courier + stub_request(:post, rates).to_return(status: 200, body: { precio: 2500.0, dias: 3 }.to_json) + store = Api::V1::DraftQuotesController.cache_store + allow(store).to receive(:increment) { |*args, **options| counter.increment(*args, **options) } + end + + it 'lets the quotes of the window through' do + use_up_quotes + + expect(response).to have_http_status(:ok) + end + + it 'answers 429 past the limit, without asking the couriers', :aggregate_failures do + use_up_quotes + quote_draft + + expect(response).to have_http_status(:too_many_requests) + expect(response.headers['Retry-After']).to eq('60') + expect(WebMock).not_to have_requested(:post, rates) + end + + it 'counts each user apart' do + other = User.create!(email: 'b@example.com', password: 'password123', company: company) + other_headers = auth_headers(other) + use_up_quotes + quote_draft(auth: other_headers) + + expect(response).to have_http_status(:ok) + end + end end From 931951d5644d4e9a60f47021e2941cbefdcbaf80 Mon Sep 17 00:00:00 2001 From: Santiago Natalichio Bestosini Date: Fri, 25 Sep 2026 20:33:01 -0300 Subject: [PATCH 11/11] docs: [TESIS-131] record the review decisions in ADR-016 Co-Authored-By: Claude Opus 5.5 --- docs/adr/ADR-016-cotizacion-del-alta-manual.md | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/docs/adr/ADR-016-cotizacion-del-alta-manual.md b/docs/adr/ADR-016-cotizacion-del-alta-manual.md index f3d4efb..f6c8dde 100644 --- a/docs/adr/ADR-016-cotizacion-del-alta-manual.md +++ b/docs/adr/ADR-016-cotizacion-del-alta-manual.md @@ -52,6 +52,8 @@ Paso 3 del alta `services.quote_service_id` sigue el patrón de `tracking_service_id` ([ADR-014](ADR-014-pull-tracking-de-couriers.md)). El `Service` del proveedor es el que despacha, y sus plantillas auxiliares (la de seguimiento, la de cotización) cuelgan de él. La validación también es la misma: sólo couriers, nunca la propia plantilla, y sólo una plantilla que cotice (`Service#quotes_shipping?`). +Además, **una plantilla de cotización es de un solo despachador**, y sólo la plantilla que despacha puede tener una. La cotización devuelve una opción por plantilla de cotización y la despacha con una integración; si dos plantillas de despacho («Andreani» y «Andreani Express») compartieran el cotizador, una desaparecería de las opciones sin aviso. `Service` lo valida con un mensaje para el panel, y un índice único sobre `quote_service_id` lo respalda en la base. Si algún día dos servicios del mismo proveedor necesitan el mismo endpoint de tarifas, se carga una plantilla de cotización por cada uno: son filas, no código. + Con el vínculo, cada opción informa `dispatch_integration_id` y se nombra por el courier («Andreani», no «Andreani - Cotización»). **Una plantilla de cotización sin integración de despacho activa no se consulta.** Se filtra antes de abrir los hilos, no después de cotizar: una opción que el operador no puede confirmar no es una opción, y pedirle la tarifa sería hacerlo esperar por algo que no se va a mostrar. @@ -60,7 +62,13 @@ Con el vínculo, cada opción informa `dispatch_integration_id` y se nombra por El despacho acepta `shipping_cost` opcional y lo escribe en el envío. Se validó la alternativa de volver a cotizar al despachar y guardar lo que conteste el courier, y se descartó por dos motivos: es una segunda llamada externa dentro del request, y el precio podría no coincidir con el que el operador aceptó segundos antes. -El valor se valida antes de pedir la etiqueta (que el courier cobra): un costo negativo o que no es un número responde 400 sin haber llamado a nadie. Un despacho sin costo deja el que hubiera. +El valor se valida antes de pedir la etiqueta (que el courier cobra), y contra la regla del modelo, no contra una copia: `ConfirmDispatch` prueba el costo con las validaciones de `Shipment` —no negativo y menor a 100.000.000, lo que entra en `decimal(10,2)`— antes de llamar al courier. Un costo que no es un número, negativo, fuera de rango, `NaN` o infinito responde 400 sin haber llamado a nadie. Validarlo sólo en el controller dejaba pasar los tres últimos: se emitía la etiqueta, el `update!` fallaba después y el envío seguía `pending`, así que un reintento pagaba una segunda etiqueta. Un despacho sin costo deja el que hubiera. + +El costo lo manda el cliente y no se compara con lo que devolvió la cotización: el operador está autenticado dentro de su propio tenant, y el costo que se guarda es informativo —no se le cobra a nadie a partir de él—. Si algún día se factura con este dato, la comparación pasa a ser necesaria. + +### Cotizar tiene un tope por usuario + +`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 funcionaba como freno. Sin tope, un cliente en loop —un `useEffect` mal puesto que cotiza en cada tecla— generaría tantas llamadas a los couriers como quisiera, con las credenciales de la empresa. Se aplica el `rate_limit` de Rails (el mismo mecanismo que el registro, TESIS-82): **20 cotizaciones por minuto por usuario**, con 429 y `Retry-After`. Se cuenta por usuario y no por IP porque todos los requests llegan autenticados, y los operarios de un depósito detrás de un mismo NAT no deberían compartir el cupo. ## Consecuencias @@ -75,6 +83,7 @@ El valor se valida antes de pedir la etiqueta (que el courier cobra): un costo n - **Confirmar son tres requests encadenados, y no es atómico.** El despacho llama a un courier externo y no puede ir en la misma transacción que el alta. Si falla, la orden queda creada con su envío `pending`, que es un estado válido y se puede volver a despachar. Lo resuelve el front, reintentando el despacho sobre la orden ya creada. - **El precio puede cambiar entre la cotización y el despacho.** Se guarda el que se confirmó, no el que el courier cobre al emitir la etiqueta. Si un proveedor empieza a devolver el costo en la respuesta del despacho, esa es la fuente mejor, y este ADR se revisa. - Un courier cargado sin su `quote_service` no se ofrece al cotizar. En el panel de administración, el vínculo se edita en la plantilla que despacha, junto a la de seguimiento. +- El contador del tope vive en la cache de la app (Solid Cache en producción). En test es `:null_store` y el tope nunca se alcanza, salvo en el spec que lo prueba. ## Referencias