Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion app/models/order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
17 changes: 13 additions & 4 deletions app/poros/orders/process_webhook_order.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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 %<count>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:)
Expand All @@ -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
Expand All @@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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
4 changes: 2 additions & 2 deletions db/schema.rb

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions docs/adr/ADR-010-ingesta-de-ordenes-de-webhooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
47 changes: 34 additions & 13 deletions spec/models/order_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
32 changes: 32 additions & 0 deletions spec/poros/orders/process_webhook_order_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }

Expand Down
Loading