diff --git a/app/models/order.rb b/app/models/order.rb index 4a65a8c..8e9d15e 100644 --- a/app/models/order.rb +++ b/app/models/order.rb @@ -35,7 +35,9 @@ class Order < ApplicationRecord validates :customer_name, presence: true validates :status, presence: true, inclusion: { in: STATUSES } - validates :external_order_id, uniqueness: { scope: :company_id }, allow_nil: true + # Único por canal y no por empresa: dos canales de la misma empresa pueden + # usar el mismo id. Es el índice `index_orders_on_integration_and_external_order_id`. + validates :external_order_id, uniqueness: { scope: :company_integration_id }, allow_nil: true # allow_nil: las órdenes anteriores a TESIS-114 que no tienen líneas no tienen # con qué calcularlo, y la orden vive un instante sin total dentro de la # transacción que la crea. diff --git a/app/poros/orders/process_webhook_order.rb b/app/poros/orders/process_webhook_order.rb index adb5106..0801355 100644 --- a/app/poros/orders/process_webhook_order.rb +++ b/app/poros/orders/process_webhook_order.rb @@ -20,7 +20,7 @@ class UnmappedProductError < StandardError; end MISSING_ORDER_ID = 'the payload does not carry an external order id' MISSING_ITEMS = 'the payload does not carry any order item' UNREADABLE_ITEMS = 'the template could not read %d of the order items in the payload' - ORDERS_UNIQUE_INDEX = 'index_orders_on_company_id_and_external_order_id' + ORDERS_UNIQUE_INDEX = 'index_orders_on_integration_and_external_order_id' CANCELLED = 'cancelled' def initialize(webhook_log:) @@ -45,7 +45,7 @@ def call def ingest validate_payload! - duplicate = Order.find_by(external_order_id: external_order_id) + duplicate = already_registered return duplicate if duplicate items = resolve_items @@ -59,10 +59,19 @@ def ingest # log quedaría en `processed` sin ninguna orden creada. raise unless e.message.include?(ORDERS_UNIQUE_INDEX) - # Dos workers con el mismo evento: el índice único (company_id, + # Dos workers con el mismo evento: el índice único (company_integration_id, # external_order_id) deja pasar a uno solo. El que perdió la carrera no # tiene nada que hacer, la venta ya está registrada. - Order.find_by(external_order_id: external_order_id) + already_registered + end + + # La misma venta es el mismo id **en el mismo canal**. Antes se buscaba por + # empresa: si dos canales de una empresa usaban el mismo id (cada uno numera + # por su lado), la segunda venta se tomaba por duplicada, el log quedaba + # `processed` sin orden, sin stock descontado y sin nada en la DLQ. + def already_registered + Order.find_by(company_integration_id: @log.company_integration_id, + external_order_id: external_order_id) end def create_order(items) diff --git a/db/migrate/20261002045858_scope_order_external_id_to_integration.rb b/db/migrate/20261002045858_scope_order_external_id_to_integration.rb new file mode 100644 index 0000000..5aaf38d --- /dev/null +++ b/db/migrate/20261002045858_scope_order_external_id_to_integration.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +# La venta externa se identifica por el canal que la mandó, no por la empresa: +# dos canales de la misma empresa pueden usar el mismo id (Mercado Libre y +# Tiendanube numeran cada uno por su lado). Con el índice por empresa, la +# segunda venta se tomaba por duplicada y se perdía sin dejar error. +# +# `company_integration_id` ya implica la empresa: cada integración es de una +# sola. Las órdenes manuales no tienen id externo (el alta no lo permite), así +# que el NULL de la integración no deja nada sin cubrir. +class ScopeOrderExternalIdToIntegration < ActiveRecord::Migration[8.1] + def change + remove_index :orders, %i[company_id external_order_id], unique: true, + name: 'index_orders_on_company_id_and_external_order_id' + add_index :orders, %i[company_integration_id external_order_id], + unique: true, name: 'index_orders_on_integration_and_external_order_id' + end +end diff --git a/db/schema.rb b/db/schema.rb index 95ee443..1b64168 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_10_01_211235) do +ActiveRecord::Schema[8.1].define(version: 2026_10_02_045858) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -111,7 +111,7 @@ t.string "status", default: "pending", null: false t.decimal "total_amount", precision: 10, scale: 2 t.datetime "updated_at", null: false - t.index ["company_id", "external_order_id"], name: "index_orders_on_company_id_and_external_order_id", unique: true + t.index ["company_integration_id", "external_order_id"], name: "index_orders_on_integration_and_external_order_id", unique: true t.index ["company_id"], name: "index_orders_on_company_id" t.index ["company_integration_id"], name: "index_orders_on_company_integration_id" t.check_constraint "status::text = ANY (ARRAY['pending'::character varying, 'paid'::character varying, 'cancelled'::character varying]::text[])", name: "orders_status_check" diff --git a/docs/adr/ADR-010-ingesta-de-ordenes-de-webhooks.md b/docs/adr/ADR-010-ingesta-de-ordenes-de-webhooks.md index 59910c7..70ea24d 100644 --- a/docs/adr/ADR-010-ingesta-de-ordenes-de-webhooks.md +++ b/docs/adr/ADR-010-ingesta-de-ordenes-de-webhooks.md @@ -125,8 +125,8 @@ La excepción **no se propaga** desde el job: si subiera, Active Job reintentar Las plataformas reenvían webhooks y Solid Queue garantiza *at-least-once*: el mismo evento puede llegar a procesarse más de una vez, y descontar el stock dos veces por una sola venta es el peor error posible acá. Tres barreras, en orden: 1. Un log ya `processed` no se vuelve a procesar. -2. Si ya existe una `Order` con ese `external_order_id`, se marca el log como procesado y no se crea nada. -3. Si dos workers corren a la vez, el índice único `(company_id, external_order_id)` deja pasar a uno solo; el que pierde la carrera captura el `RecordNotUnique` y termina como duplicado. El rescate verifica que la violación sea **la de ese índice** por nombre: cubre toda la transacción, y un índice único que aparezca más adelante en `order_items` o en `stocks` es un fallo real que tiene que llegar a la DLQ, no un duplicado ya registrado. +2. Si ya existe una `Order` con ese `external_order_id` **en la misma integración**, se marca el log como procesado y no se crea nada. La clave es el canal y no la empresa: cada canal numera sus ventas por su lado, y con la clave por empresa la venta de un segundo canal con un id repetido se descartaba en silencio (TESIS-999020). +3. Si dos workers corren a la vez, el índice único `(company_integration_id, external_order_id)` deja pasar a uno solo; el que pierde la carrera captura el `RecordNotUnique` y termina como duplicado. El rescate verifica que la violación sea **la de ese índice** por nombre: cubre toda la transacción, y un índice único que aparezca más adelante en `order_items` o en `stocks` es un fallo real que tiene que llegar a la DLQ, no un duplicado ya registrado. ### Ruteo en el gateway diff --git a/spec/models/order_spec.rb b/spec/models/order_spec.rb index 5ef0783..616cb3a 100644 --- a/spec/models/order_spec.rb +++ b/spec/models/order_spec.rb @@ -76,20 +76,41 @@ expect(another).to be_valid end - it 'enforces external_order_id uniqueness scoped to company' do - order.external_order_id = 'ML-123' - order.save! - duplicate = described_class.new(company: company, customer_name: 'Otro', - external_order_id: 'ML-123') - expect(duplicate).not_to be_valid - end + # Única por canal: la misma venta es el mismo id en la misma integración. + describe 'the external order id' do + def channel(owner, name) + CompanyIntegration.create!( + company: owner, + service: Service.create!(service_name: name, type: 'ecommerce', + uri: "https://#{name.downcase}.test", http_method: 'GET') + ) + end - it 'allows the same external_order_id across different companies' do - order.external_order_id = 'ML-123' - order.save! - other = described_class.new(company: other_company, customer_name: 'Otro', - external_order_id: 'ML-123') - expect(other).to be_valid + def sale(owner, integration, id = 'ML-123') + described_class.new(company: owner, customer_name: 'Cliente', company_integration: integration, + external_order_id: id) + end + + it 'is unique within the same channel' do + ml = channel(company, 'ML') + sale(company, ml).save! + + expect(sale(company, ml)).not_to be_valid + end + + # Cada canal numera por su lado: antes la segunda venta se tomaba por + # duplicada y se perdía sin dejar error. + it 'repeats freely between two channels of the same company' do + sale(company, channel(company, 'ML')).save! + + expect(sale(company, channel(company, 'TN'))).to be_valid + end + + it 'repeats freely between companies' do + sale(company, channel(company, 'ML')).save! + + expect(sale(other_company, channel(other_company, 'ML2'))).to be_valid + end end it 'rejects a company_integration from another company', :aggregate_failures do diff --git a/spec/poros/orders/process_webhook_order_spec.rb b/spec/poros/orders/process_webhook_order_spec.rb index ab7c326..02fcc0b 100644 --- a/spec/poros/orders/process_webhook_order_spec.rb +++ b/spec/poros/orders/process_webhook_order_spec.rb @@ -197,6 +197,38 @@ def create_log(body = payload) end end + # Hallazgo de auditoría (TESIS-89): la idempotencia buscaba el id externo en + # toda la empresa. La venta de otro canal con el mismo id se tomaba por + # duplicada: log `processed`, sin orden, sin stock descontado, sin DLQ. + context 'when another channel of the company already used the same external id' do + let(:other_channel) { CompanyIntegration.create!(company: company, service: create_service) } + + before do + publish('SKU-1', 'MLA-1', stock: 20) + ProductMapping.create!(product: Product.find_by(sku: 'SKU-1'), company_integration: other_channel, + external_product_id: 'TN-1') + described_class.new(webhook_log: create_log(order_payload(items: [line('MLA-1', 2, 10)]))).call + end + + def sale_from_other_channel + WebhookLog.create!(company_id: company.id, company_integration: other_channel, + payload: order_payload(items: [line('TN-1', 3, 10)])) + end + + it 'registers it as a sale of its own', :aggregate_failures do + order = described_class.new(webhook_log: sale_from_other_channel).call + + expect(order.company_integration).to eq(other_channel) + expect(Order.where(external_order_id: 'ML-1001').count).to eq(2) + end + + it 'takes its units from the stock' do + described_class.new(webhook_log: sale_from_other_channel).call + + expect(stock_of('SKU-1')).to eq(15) + end + end + context 'when the log was already processed' do before { log.update!(status: :processed) }