From f0a7d06c5fbd7321b6a0067c901105d60c882ed6 Mon Sep 17 00:00:00 2001 From: LauAubert Date: Fri, 2 Oct 2026 02:05:29 -0300 Subject: [PATCH 1/5] fix: [TESIS-999022] bound the page number of every listing Paginatable only bounded the page number from below. A page of twenty digits made the OFFSET overflow bigint, PostgreSQL raised NumericValueOutOfRange and every listing answered 500. The page is now clamped between 1 and a million, far above any real listing and far from the limit of the database. A page that large reads as the last accepted one and answers empty, like any page past the end, with the real total. Co-Authored-By: Claude Opus 5.5 --- app/controllers/concerns/api/v1/paginatable.rb | 14 +++++++++++--- spec/requests/api/v1/pagination_spec.rb | 9 +++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/controllers/concerns/api/v1/paginatable.rb b/app/controllers/concerns/api/v1/paginatable.rb index 79f89e8..37d3be3 100644 --- a/app/controllers/concerns/api/v1/paginatable.rb +++ b/app/controllers/concerns/api/v1/paginatable.rb @@ -38,6 +38,12 @@ module Paginatable # preferible a que el default de 20 le esconda depósitos en silencio. WHOLE_LIST_PER_PAGE = MAX_PER_PAGE + # La página más alta que se acepta. Muy por encima de cualquier listado + # real (un millón de páginas de cien filas), y lejos del límite de + # `bigint` del OFFSET: un `?page=` de veinte dígitos lo desbordaba y la + # API respondía 500 (hallazgo de la auditoría de TESIS-89). + MAX_PAGE = 1_000_000 + private # Devuelve `[filas, meta]`. @@ -53,11 +59,13 @@ def paginate(scope, per_page: DEFAULT_PER_PAGE, total: nil) { page: page, per_page: size, total: total || scope.count }] end - # Página pedida, nunca menor que 1. `page=0` y `page=-3` se acotan en vez + # Página pedida, entre 1 y MAX_PAGE. `page=0` y `page=-3` se acotan en vez # de romper: un offset negativo es un error de SQL, y un 400 por un número - # que se puede interpretar sería antipático. + # que se puede interpretar sería antipático. Por arriba, el mismo criterio: + # una página enorme se lee como la última aceptada y responde vacía, como + # cualquier página pasada del final. def page_number - [scalar_param(:page).to_i, 1].max + scalar_param(:page).to_i.clamp(1, MAX_PAGE) end # `scalar_param` y no `params[...]`: `?per_page[]=1` entrega un Array y diff --git a/spec/requests/api/v1/pagination_spec.rb b/spec/requests/api/v1/pagination_spec.rb index 7534973..53933aa 100644 --- a/spec/requests/api/v1/pagination_spec.rb +++ b/spec/requests/api/v1/pagination_spec.rb @@ -56,6 +56,15 @@ def rows_for(params) end # Una página más allá del final no es un error: es una página vacía. + # Un número de veinte dígitos desbordaba el OFFSET (bigint) y respondía 500. + it 'answers an empty page for a page number too large for the database', :aggregate_failures do + get '/api/v1/orders', params: { page: '99999999999999999999' }, headers: headers + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['data']).to be_empty + expect(response.parsed_body.dig('meta', 'page')).to eq(Api::V1::Paginatable::MAX_PAGE) + end + it 'answers an empty page past the end, with the real total', :aggregate_failures do create_products(3) From ea885364eec4e24543a51fce696cafe0d65e6512 Mon Sep 17 00:00:00 2001 From: Lautaro Antuel Aubert <82173421+LauAubert@users.noreply.github.com> Date: Sun, 4 Oct 2026 20:45:54 -0300 Subject: [PATCH 2/5] feat: [TESIS-999001] expose the stock status and the incoming units per warehouse in the product detail (#99) The detail did not carry stock_status, so the front recomputed it with its own thresholds and the same product read "available" in the catalog and "critical" in the detail. The rule now lives in Product.stock_status_for and is applied to the product total and to each stock row. in_transit_by_warehouse breaks the units in flight down by destination with a single grouped query, including warehouses that have no stock row yet. The warehouse nested in each stock drops stored_units through a reference view: it cost one SUM per warehouse and nobody reads it there. Co-authored-by: Claude Opus 5.5 --- app/models/product.rb | 36 +++++- app/models/stock.rb | 5 + app/serializers/product_serializer.rb | 7 ++ app/serializers/stock_serializer.rb | 9 +- app/serializers/warehouse_serializer.rb | 7 ++ spec/models/product_spec.rb | 19 +++ spec/models/stock_spec.rb | 9 ++ spec/requests/api/v1/api_contract_spec.rb | 31 ++++- spec/requests/api/v1/products_spec.rb | 136 ++++++++++++++++++++++ 9 files changed, 252 insertions(+), 7 deletions(-) diff --git a/app/models/product.rb b/app/models/product.rb index 9a35391..2d97386 100644 --- a/app/models/product.rb +++ b/app/models/product.rb @@ -112,10 +112,20 @@ def total_stock # usa `by_stock_status` para filtrar: si se calculara en el cliente, el filtro # y el color de la fila podrían discrepar. def stock_status - total = total_stock - return 'out_of_stock' if total.zero? + self.class.stock_status_for(total_stock) + end + + # La regla de disponibilidad, en un solo lugar. La usan el producto (sobre su + # total) y cada fila de `stocks` (sobre lo que guarda ese depósito): si cada + # uno tuviera su copia, el badge del detalle y el del catálogo podían volver a + # discrepar, que es justo el bug que motivó exponer el estado desde acá. + # + # Por depósito es una regla provisoria: usa el mismo umbral global porque el + # modelo no tiene punto de reposición por depósito. Si aparece, cambia acá. + def self.stock_status_for(quantity) + return 'out_of_stock' if quantity.to_i <= 0 - total <= LOW_STOCK_THRESHOLD ? 'low' : 'available' + quantity <= LOW_STOCK_THRESHOLD ? 'low' : 'available' end # Unidades que salieron de un depósito y todavía no llegaron a otro. No están @@ -129,6 +139,26 @@ def in_transit_quantity stock_transfers.in_flight.sum(:quantity) end + # Unidades en vuelo hacia cada depósito: lo que todavía no figura en ningún + # número del destino. El saliente no va: ya está descontado del on hand del + # origen al despachar, y mostrarlo en esa fila se leería como si siguiera ahí. + # + # Va aparte y no por fila de `stocks` porque el destino puede no tener fila + # hasta que la transferencia se recibe (`AdjustWarehouseStock` la crea + # recién entonces). Una sola query agregada para todo el producto; cada + # transferencia tiene un único destino, así que la suma de todas las + # entradas es exactamente `in_transit_quantity`. + def in_transit_by_warehouse + stock_transfers.in_flight + .joins(:destination_warehouse) + .group(:destination_warehouse_id, 'warehouses.name') + .order(:destination_warehouse_id) + .sum(:quantity) + .map do |(warehouse_id, name), quantity| + { warehouse_id: warehouse_id, name: name, quantity: quantity.to_i } + end + end + # Depósito donde está el grueso de las unidades. Lo consume la columna # "Location Node" del listado, que muestra un nodo y no el desglose. # diff --git a/app/models/stock.rb b/app/models/stock.rb index ab53e1e..b812660 100644 --- a/app/models/stock.rb +++ b/app/models/stock.rb @@ -12,6 +12,11 @@ def display_name "#{product&.name} @ #{warehouse&.name}" end + # Disponibilidad de este depósito, con la misma regla que el producto. + def stock_status + Product.stock_status_for(quantity) + end + # El disparo del sync saliente vive acá y no en el ABM porque la condición es # "cambió la tabla stocks", no "alguien usó tal endpoint": así queda cubierto # todo camino que escriba stock (ABM, descuento por venta, importaciones, diff --git a/app/serializers/product_serializer.rb b/app/serializers/product_serializer.rb index 694a8c3..8294bef 100644 --- a/app/serializers/product_serializer.rb +++ b/app/serializers/product_serializer.rb @@ -12,5 +12,12 @@ class ProductSerializer < ApplicationSerializer product.weight.to_f end + # La disponibilidad, con la misma regla que el listado: antes el detalle no la + # traía y el front la recalculaba con otros umbrales. + field :stock_status + + # Entrante por depósito (ver `Product#in_transit_by_warehouse`). + field :in_transit_by_warehouse + association :stocks, blueprint: StockSerializer end diff --git a/app/serializers/stock_serializer.rb b/app/serializers/stock_serializer.rb index d1dbd59..81355eb 100644 --- a/app/serializers/stock_serializer.rb +++ b/app/serializers/stock_serializer.rb @@ -5,5 +5,12 @@ class StockSerializer < ApplicationSerializer fields :quantity, :warehouse_id, :created_at, :updated_at - association :warehouse, blueprint: WarehouseSerializer + # Calculado acá y no en el front: es la misma regla que el badge del producto + # (`Product.stock_status_for`), aplicada a lo que guarda este depósito. + field :stock_status + + # Vista `reference`: el depósito como referencia, sin `stored_units`. Ese + # campo hace un SUM por depósito y en el detalle de un producto nadie lo lee; + # con la vista por defecto, cada fila de stock agregaba una query. + association :warehouse, blueprint: WarehouseSerializer, view: :reference end diff --git a/app/serializers/warehouse_serializer.rb b/app/serializers/warehouse_serializer.rb index 0aff673..292a498 100644 --- a/app/serializers/warehouse_serializer.rb +++ b/app/serializers/warehouse_serializer.rb @@ -11,4 +11,11 @@ class WarehouseSerializer < ApplicationSerializer # Se expone como entero y nunca null: un deposito vacio guarda cero unidades, # que es un dato, no un dato faltante. field :stored_units + + # El deposito como referencia dentro de otro recurso (el stock de un + # producto). Sin `stored_units`: fuera del listado de depositos no viene del + # scope `with_stored_units` y costaria una query por deposito. + view :reference do + excludes :stored_units + end end diff --git a/spec/models/product_spec.rb b/spec/models/product_spec.rb index b673cb0..a3c342b 100644 --- a/spec/models/product_spec.rb +++ b/spec/models/product_spec.rb @@ -43,6 +43,25 @@ end end + describe '.stock_status_for' do + it 'maps a quantity to the three availability states', :aggregate_failures do + expect(described_class.stock_status_for(0)).to eq('out_of_stock') + expect(described_class.stock_status_for(1)).to eq('low') + expect(described_class.stock_status_for(Product::LOW_STOCK_THRESHOLD)).to eq('low') + expect(described_class.stock_status_for(Product::LOW_STOCK_THRESHOLD + 1)).to eq('available') + end + + it 'treats a missing quantity as no units' do + expect(described_class.stock_status_for(nil)).to eq('out_of_stock') + end + + it 'only answers values of the vocabulary' do + results = [0, 1, 1_000].map { |quantity| described_class.stock_status_for(quantity) } + + expect(results).to all(be_in(Product::STOCK_STATUSES)) + end + end + describe '#primary_stock' do let(:central) do Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') diff --git a/spec/models/stock_spec.rb b/spec/models/stock_spec.rb index 429f61f..1d0ad81 100644 --- a/spec/models/stock_spec.rb +++ b/spec/models/stock_spec.rb @@ -12,6 +12,15 @@ let(:product) { Product.create!(company: company, sku: 'SKU-001', name: 'Widget Alpha') } let(:warehouse) { Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') } + describe '#stock_status' do + it 'applies the product rule to the units of this warehouse', :aggregate_failures do + expect(described_class.new(quantity: 0).stock_status).to eq('out_of_stock') + expect(described_class.new(quantity: Product::LOW_STOCK_THRESHOLD).stock_status).to eq('low') + expect(described_class.new(quantity: Product::LOW_STOCK_THRESHOLD + 1).stock_status) + .to eq('available') + end + end + it 'is valid with required attributes' do expect(stock).to be_valid end diff --git a/spec/requests/api/v1/api_contract_spec.rb b/spec/requests/api/v1/api_contract_spec.rb index af16773..667ef79 100644 --- a/spec/requests/api/v1/api_contract_spec.rb +++ b/spec/requests/api/v1/api_contract_spec.rb @@ -35,9 +35,11 @@ module ContratoDeLaApi empresa: %w[id name], producto_fila: %w[id sku name description category weight dimensions total_stock stock_status in_transit_quantity primary_warehouse warehouse_count created_at updated_at], - producto: %w[id sku name description category weight dimensions total_stock - in_transit_quantity stocks created_at updated_at], - stock: %w[id quantity warehouse_id warehouse created_at updated_at], + producto: %w[id sku name description category weight dimensions total_stock stock_status + in_transit_quantity in_transit_by_warehouse stocks created_at updated_at], + stock: %w[id quantity warehouse_id warehouse stock_status created_at updated_at], + deposito_referencia: %w[id name address zip_code], + transito: %w[warehouse_id name quantity], deposito: %w[id name address zip_code stored_units], orden_fila: %w[id external_order_id customer_name customer_document customer_address customer_zip_code customer_city customer_province status courier total_amount @@ -83,6 +85,12 @@ def product end end + def send_units_to_another_warehouse + destino = Warehouse.create!(company: company, name: 'CD Sur', address: 'Av. 3', zip_code: '8000') + Catalog::DispatchTransfer.new(company: company, product: product, origin_warehouse: warehouse, + destination_warehouse: destino, quantity: 2).call + end + def order @order ||= begin o = Order.create!(company: company, customer_name: 'Cliente', customer_address: 'Av. 2', @@ -216,6 +224,23 @@ def shipment expect(response.parsed_body['stocks'].first.keys).to match_array(claves[:stock]) end + # El depósito anidado es una referencia: sin `stored_units`, que sólo + # trae el listado de depósitos (ahí sale agregado en la misma query). + it 'nests the warehouse of a stock as a reference, without its load' do + get "/api/v1/products/#{product.id}", headers: headers + + expect(response.parsed_body['stocks'].first['warehouse'].keys) + .to match_array(claves[:deposito_referencia]) + end + + it 'breaks the incoming units down by destination warehouse' do + send_units_to_another_warehouse + get "/api/v1/products/#{product.id}", headers: headers + + expect(response.parsed_body['in_transit_by_warehouse'].first.keys) + .to match_array(claves[:transito]) + end + it 'answers a warehouse with the fields the frontend declares' do warehouse diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 2c1a67a..439bebb 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -504,6 +504,142 @@ def row end end + # Lo que la pantalla de detalle necesita para pintar estados sin + # reimplementar reglas: antes el front calculaba el badge con sus propios + # umbrales y el mismo producto salía «Disponible» en el catálogo y «Crítico» + # en el detalle. + describe 'GET /api/v1/products/:id availability and units in flight' do + # Un método y no un `let` por depósito: el grupo ya hereda tres helpers del + # describe de arriba y RSpec/MultipleMemoizedHelpers corta en cinco. + def depot(name) + Warehouse.find_or_create_by!(company: company, name: name) do |warehouse| + warehouse.assign_attributes(zip_code: '1900', address: "Calle #{name}") + end + end + + def product_with(quantity, sku: 'D-001') + Product.create!(company: company, sku: sku, name: sku).tap do |product| + Stock.create!(product: product, warehouse: depot('Central'), quantity: quantity) + end + end + + def detail(product) + get "/api/v1/products/#{product.id}", headers: headers + response.parsed_body + end + + def statuses_in_both_screens(product) + status = detail(product)['stock_status'] + get '/api/v1/products', headers: headers + [status, response.parsed_body['data'].find { |row| row['id'] == product.id }['stock_status']] + end + + def transfer(product, to:, quantity:, settle: nil) + sent = Catalog::DispatchTransfer.new(company: company, product: product, + origin_warehouse: depot('Central'), + destination_warehouse: depot(to), quantity: quantity).call + settle ? Catalog::SettleTransfer.new(transfer: sent, outcome: settle).call : sent + end + + # Una fila de stock más y dos transferencias en vuelo hacia ese depósito. + def spread(product, to:) + Stock.create!(product: product, warehouse: depot(to), quantity: 5) + 2.times { transfer(product, to: to, quantity: 1) } + end + + def queries_for_detail(product) + count_queries(matching: /SELECT/) { detail(product) } + end + + it 'answers the same status as the catalog right at the threshold', :aggregate_failures do + at_threshold = product_with(Product::LOW_STOCK_THRESHOLD, sku: 'D-LOW') + above = product_with(Product::LOW_STOCK_THRESHOLD + 1, sku: 'D-OK') + + expect(statuses_in_both_screens(at_threshold)).to eq(%w[low low]) + expect(statuses_in_both_screens(above)).to eq(%w[available available]) + end + + it 'answers out_of_stock for a product with no units' do + expect(detail(product_with(0))['stock_status']).to eq('out_of_stock') + end + + # Ejemplo de la card con los números de NOR-003 en los seeds: 130 en total + # (available), repartidos 100 y 30 (low los dos). + it 'computes the status of each warehouse with the same rule', :aggregate_failures do + product = product_with(100) + Stock.create!(product: product, warehouse: depot('North'), quantity: 30) + + expect(detail(product)['stock_status']).to eq('available') + expect(detail(product)['stocks'].pluck('stock_status')).to eq(%w[low low]) + end + + it 'adds the status to the create answer as well' do + post '/api/v1/products', headers: headers, + params: { product: { sku: 'D-NEW', name: 'Nuevo' } }, as: :json + + expect(response.parsed_body['stock_status']).to eq('out_of_stock') + end + + it 'adds the status to the update answer as well' do + put "/api/v1/products/#{product_with(7).id}", + headers: headers, params: { product: { name: 'Renombrado' } }, as: :json + + expect(response.parsed_body['stock_status']).to eq('low') + end + + context 'with transfers in several states' do + let!(:product) { product_with(50) } + + before do + transfer(product, to: 'North', quantity: 4) + transfer(product, to: 'North', quantity: 1) + transfer(product, to: 'South', quantity: 6) + transfer(product, to: 'North', quantity: 2, settle: :received) + transfer(product, to: 'South', quantity: 3, settle: :cancelled) + end + + it 'counts only the incoming units still in flight, per destination' do + expect(detail(product)['in_transit_by_warehouse']).to eq( + [{ 'warehouse_id' => depot('North').id, 'name' => 'North', 'quantity' => 5 }, + { 'warehouse_id' => depot('South').id, 'name' => 'South', 'quantity' => 6 }] + ) + end + + it 'adds up to the in_transit_quantity of the product' do + body = detail(product) + + expect(body['in_transit_by_warehouse'].sum { |row| row['quantity'] }) + .to eq(body['in_transit_quantity']) + end + + # South nunca recibió nada (la de South se canceló), así que no tiene + # fila en `stocks`: el entrante igual aparece. + it 'includes a destination that has no stock row yet', :aggregate_failures do + body = detail(product) + + expect(body['stocks'].pluck('warehouse_id')).not_to include(depot('South').id) + expect(body['in_transit_by_warehouse'].pluck('name')).to include('South') + end + end + + it 'answers an empty breakdown when nothing is in flight' do + expect(detail(product_with(5))['in_transit_by_warehouse']).to eq([]) + end + + # Una query agregada para el entrante y ninguna por depósito: antes, cada + # fila de stock sumaba `stored_units` de su depósito con una query propia. + # Se compara un producto chico contra uno con más depósitos y + # transferencias: la cantidad de queries tiene que ser la misma. + it 'does not add queries as warehouses and transfers grow' do + small = product_with(50, sku: 'D-SMALL') + large = product_with(50, sku: 'D-LARGE') + %w[North South].each { |name| spread(large, to: name) } + detail(small) # el primer request crea el usuario y carga el esquema + + expect(queries_for_detail(large)).to eq(queries_for_detail(small)) + end + end + describe 'optimistic locking with If-Match' do let(:warehouse) do Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') From c1e7d25e6a247a5a15853e298db0f711e80b928d Mon Sep 17 00:00:00 2001 From: Lautaro Antuel Aubert <82173421+LauAubert@users.noreply.github.com> Date: Sun, 4 Oct 2026 20:49:15 -0300 Subject: [PATCH 3/5] fix: [TESIS-999005] let a warehouse go when its stock rows are all at zero (#100) Removing a warehouse from a product in the edit modal sends quantity 0, because Products::UpdateProduct upserts and never deletes. The row stays, and restrict_with_error on stocks then blocked deleting the warehouse forever, even though it held no units. The warehouse now drops its empty rows right before the restriction runs, in the same transaction: if orders or transfers still block the deletion, the rows come back. The 409 reason looks only at rows holding units, so it names the real blocker. Co-authored-by: Claude Opus 5.5 --- .../api/v1/warehouses_controller.rb | 5 +- app/models/stock.rb | 5 ++ app/models/warehouse.rb | 23 +++++++ spec/models/stock_spec.rb | 11 ++++ spec/models/warehouse_spec.rb | 23 +++++++ spec/requests/api/v1/warehouses_spec.rb | 60 +++++++++++++++++++ 6 files changed, 126 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/v1/warehouses_controller.rb b/app/controllers/api/v1/warehouses_controller.rb index f2187e8..2287ecc 100644 --- a/app/controllers/api/v1/warehouses_controller.rb +++ b/app/controllers/api/v1/warehouses_controller.rb @@ -77,8 +77,11 @@ def render_conflict(_exception) status: :conflict end + # `holding_units` y no `stocks`: las filas en cero no bloquean (ver + # `Warehouse#release_empty_stock_rows`), así que nombrarlas como el motivo + # escondería el verdadero cuando lo que frena son las ventas. def blocking_reason - return 'existing stock' if @warehouse.stocks.exists? + return 'existing stock' if @warehouse.stocks.holding_units.exists? return 'order lines taken from it' if @warehouse.order_items.exists? 'stock transfers from or to it' diff --git a/app/models/stock.rb b/app/models/stock.rb index b812660..7d4ef17 100644 --- a/app/models/stock.rb +++ b/app/models/stock.rb @@ -5,6 +5,11 @@ class Stock < ApplicationRecord belongs_to :warehouse validates :quantity, numericality: { greater_than_or_equal_to: 0 } + + # Filas que guardan unidades. Una fila en cero es una asignación sin stock: + # el modal de producto la deja así al «quitar» un depósito, porque + # `Products::UpdateProduct` hace upsert y nunca borra. + scope :holding_units, -> { where('quantity > 0') } validates :warehouse_id, uniqueness: { scope: :product_id } validate :product_and_warehouse_must_belong_to_same_company diff --git a/app/models/warehouse.rb b/app/models/warehouse.rb index fdd6eef..95fe087 100644 --- a/app/models/warehouse.rb +++ b/app/models/warehouse.rb @@ -4,6 +4,20 @@ class Warehouse < ApplicationRecord include CompanyScoped belongs_to :company + + # Antes que el `restrict_with_error` de `stocks` (por eso `prepend`): las + # filas en cero no son stock, son asignaciones vacías. Sin esto, un depósito + # al que el modal de producto le «quitó» todos los productos —que viajan como + # `quantity: 0`— no se podía borrar nunca, aunque no guardara nada. + # + # `delete_all` y no `destroy_all`: borrar una fila en cero no le cambia el + # total a ningún producto, así que no hay nada que sincronizar con los canales + # y el callback de `Stock` encolaría un job por producto para nada. + # + # Si otra cosa frena el borrado (ventas, transferencias), el `destroy` se + # aborta dentro de su transacción y estas filas vuelven: no se pierde nada. + before_destroy :release_empty_stock_rows, prepend: true + # Bloquea el borrado si hay stock: las unidades son dato de negocio y no deben # evaporarse por un DELETE. destroy! levanta RecordNotDestroyed -> 409 (API). has_many :stocks, dependent: :restrict_with_error @@ -45,4 +59,13 @@ class Warehouse < ApplicationRecord def stored_units has_attribute?(:stored_units) ? self[:stored_units].to_i : stocks.sum(:quantity) end + + private + + # `reset`: si la asociación ya estaba cargada, el `restrict_with_error` que + # corre después miraría la lista vieja, con las filas que ya no existen. + def release_empty_stock_rows + stocks.where(quantity: 0).delete_all + stocks.reset + end end diff --git a/spec/models/stock_spec.rb b/spec/models/stock_spec.rb index 1d0ad81..555add7 100644 --- a/spec/models/stock_spec.rb +++ b/spec/models/stock_spec.rb @@ -71,6 +71,17 @@ Current.reset end + describe '.holding_units' do + let(:other_product) { Product.create!(company: company, sku: 'SKU-002', name: 'Otro') } + + it 'leaves out the rows at zero' do + stock.save! + described_class.create!(product: other_product, warehouse: warehouse, quantity: 0) + + expect(described_class.holding_units).to contain_exactly(stock) + end + end + describe 'outbound sync trigger' do let(:sync_job) { Catalog::SyncStockToChannelsJob } diff --git a/spec/models/warehouse_spec.rb b/spec/models/warehouse_spec.rb index d0260bd..c5b610f 100644 --- a/spec/models/warehouse_spec.rb +++ b/spec/models/warehouse_spec.rb @@ -26,4 +26,27 @@ it 'belongs to a company' do expect(described_class.reflect_on_association(:company).macro).to eq(:belongs_to) end + + describe 'destroying it' do + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Producto') } + + before { warehouse.save! } + + # El `restrict_with_error` de `stocks` corre después y miraría la lista + # cargada si no se reseteara. + it 'goes through even when its empty stock rows were already loaded', :aggregate_failures do + Stock.create!(product: product, warehouse: warehouse, quantity: 0) + warehouse.stocks.load + + expect(warehouse.destroy).to be_truthy + expect(Stock.count).to eq(0) + end + + it 'is refused while a row holds units', :aggregate_failures do + Stock.create!(product: product, warehouse: warehouse, quantity: 3) + + expect(warehouse.destroy).to be(false) + expect(warehouse.errors.full_messages).to include(a_string_matching(/stocks/i)) + end + end end diff --git a/spec/requests/api/v1/warehouses_spec.rb b/spec/requests/api/v1/warehouses_spec.rb index 6e9594a..a947977 100644 --- a/spec/requests/api/v1/warehouses_spec.rb +++ b/spec/requests/api/v1/warehouses_spec.rb @@ -313,6 +313,58 @@ def stock_queries_while(&) expect(response.parsed_body['error']).to eq('Cannot delete warehouse with existing stock') end + # Una fila en cero es una asignación vacía: el modal de producto deja así + # al depósito que se «quita», porque el update hace upsert y nunca borra. + context 'when every stock row of the warehouse is at zero' do + before { assign_products_without_units(2) } + + it 'deletes the warehouse along with those empty rows', :aggregate_failures do + delete "/api/v1/warehouses/#{warehouse.id}", headers: headers + + expect(response).to have_http_status(:no_content) + expect(Warehouse.find_by(id: warehouse.id)).to be_nil + expect(Stock.where(warehouse_id: warehouse.id)).to be_empty + end + + # Nada que publicar: el total de esos productos no cambió. + it 'does not enqueue a stock sync for those products' do + expect do + delete "/api/v1/warehouses/#{warehouse.id}", headers: headers + end.not_to have_enqueued_job(Catalog::SyncStockToChannelsJob) + end + end + + context 'when an empty row sits next to a row with units' do + before do + assign_products_without_units(1) + create_warehouse_with_stock + end + + it 'keeps the warehouse and both rows', :aggregate_failures do + delete "/api/v1/warehouses/#{warehouse.id}", headers: headers + + expect(response).to have_http_status(:conflict) + expect(Stock.where(warehouse_id: warehouse.id).count).to eq(2) + end + end + + # El borrado se aborta dentro de su transacción: las filas en cero que se + # soltaron vuelven, y el motivo que se nombra es el verdadero. + context 'when order lines block it and it only has empty stock rows' do + before do + assign_products_without_units(1) + create_order_line_from_warehouse + end + + it 'keeps the empty rows and names the order lines', :aggregate_failures do + delete "/api/v1/warehouses/#{warehouse.id}", headers: headers + + expect(response.parsed_body['error']) + .to eq('Cannot delete warehouse with order lines taken from it') + expect(Stock.where(warehouse_id: warehouse.id, quantity: 0).count).to eq(1) + end + end + # TESIS-126: la línea recuerda su depósito para devolverle unidades al # modificar la orden. Borrarlo dejaría esa devolución sin destino. context 'when order lines were taken from it and it has no stock left' do @@ -371,6 +423,14 @@ def create_warehouse_with_stock Stock.create!(product: product, warehouse: warehouse, quantity: 10) end + # Productos asignados al depósito con cero unidades. + def assign_products_without_units(count) + count.times do |index| + product = Product.create!(company: company, sku: "EMPTY-#{index}", name: "Vacío #{index}") + Stock.create!(product: product, warehouse: warehouse, quantity: 0) + end + end + def create_order_line_from_warehouse product = Product.create!(company: company, sku: 'SKU-2', name: 'Vendido') order = Order.create!(company: company, customer_name: 'Cliente') From 2cefa7ba567ed4267acae114f44985e5a3d3c8eb Mon Sep 17 00:00:00 2001 From: Lautaro Antuel Aubert <82173421+LauAubert@users.noreply.github.com> Date: Sun, 4 Oct 2026 20:51:53 -0300 Subject: [PATCH 4/5] fix: [TESIS-999017] refuse fractional order quantities with 422 instead of 500 (#105) OrderItem validated the quantity as greater than zero on the raw value, and the integer column then cast it: 0.5 passed, was stored as 0 and made DeductStock raise ArgumentError, which nobody rescues, so POST /orders answered 500. 2.7 was silently truncated to 2. The edition already refused both through ReplaceOrderLines#positive_integer; the creation did not. The quantity of an order item is now validated as an integer, so the creation and the webhook ingestion answer 422 with the reason before any stock moves. Integers that come as text from a form are still accepted. Co-authored-by: Claude Opus 5.5 --- app/models/order_item.rb | 6 +++++- spec/models/order_item_spec.rb | 13 +++++++++++++ spec/requests/api/v1/orders_spec.rb | 15 +++++++++++++++ 3 files changed, 33 insertions(+), 1 deletion(-) diff --git a/app/models/order_item.rb b/app/models/order_item.rb index ee32c6c..c7346d8 100644 --- a/app/models/order_item.rb +++ b/app/models/order_item.rb @@ -8,7 +8,11 @@ class OrderItem < ApplicationRecord # una modificación que tenga que devolverle unidades no sabe a dónde. belongs_to :warehouse, optional: true - validates :quantity, numericality: { greater_than: 0 } + # Entero: la columna es integer y guardaba `0.5` como 0 y `2.7` como 2. El + # primero pasaba la validación (0,5 > 0) y reventaba después en DeductStock + # con un 500; el segundo se truncaba sin aviso. Se valida antes de escribir, + # así el alta y el webhook responden 422 con el motivo. + validates :quantity, numericality: { only_integer: true, greater_than: 0 } validates :unit_price, numericality: { greater_than_or_equal_to: 0 } validate :product_belongs_to_same_company_as_order validate :warehouse_belongs_to_same_company_as_order diff --git a/spec/models/order_item_spec.rb b/spec/models/order_item_spec.rb index d38b3ab..aa02c12 100644 --- a/spec/models/order_item_spec.rb +++ b/spec/models/order_item_spec.rb @@ -40,6 +40,19 @@ expect(order_item).not_to be_valid end + # La columna es integer: 0,5 se guardaba como 0 y 2,7 como 2. + it 'refuses a fractional quantity', :aggregate_failures do + [0.5, 2.7, '1.5'].each do |quantity| + order_item.quantity = quantity + expect(order_item).not_to be_valid, "#{quantity.inspect} should be refused" + end + end + + it 'accepts an integer that comes as text from a form' do + order_item.quantity = '3' + expect(order_item).to be_valid + end + it 'validates unit_price is not negative' do order_item.unit_price = -1 expect(order_item).not_to be_valid diff --git a/spec/requests/api/v1/orders_spec.rb b/spec/requests/api/v1/orders_spec.rb index 63f3ddb..3cafee1 100644 --- a/spec/requests/api/v1/orders_spec.rb +++ b/spec/requests/api/v1/orders_spec.rb @@ -396,6 +396,21 @@ def order_of_another_company expect(response).to have_http_status(:unprocessable_content) end + # Pasaba la validación (0,5 > 0), la columna integer la guardaba en 0 y + # DeductStock reventaba con ArgumentError: 500. + it 'rejects a quantity below one unit with 422, not 500', :aggregate_failures do + post_order(build_payload(items: [default_item.merge(quantity: 0.5)])) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to include('must be an integer') + end + + it 'rejects a fractional quantity instead of truncating it', :aggregate_failures do + expect { post_order(build_payload(items: [default_item.merge(quantity: 2.7)])) } + .not_to change(Order, :count) + expect(response).to have_http_status(:unprocessable_content) + end + it 'rejects when warehouse belongs to another company' do other_wh = other_company_warehouse post_order(build_payload(items: [default_item.merge(warehouse_id: other_wh.id)])) From 34a6aab00ed29195d0cbd1a54b629d13a38fc076 Mon Sep 17 00:00:00 2001 From: Lautaro Antuel Aubert <82173421+LauAubert@users.noreply.github.com> Date: Sun, 4 Oct 2026 20:54:31 -0300 Subject: [PATCH 5/5] fix: [TESIS-999018] validate the warehouse of each product stock row (#106) The products POROs compared the warehouse ids of the stock rows raw against the ids in the database. A row without warehouse_id made the sort raise ArgumentError (nil against Integer), so POST and PUT /products answered 500; ids that came as text ("5") never matched and gave a false 422 saying the warehouse did not belong to the company. The order edition had the same false 422 with the same warehouse sent as "1" and 1. The rows now need a positive integer warehouse_id, as a number or as digits, and are compared as unique integers by count. A missing one answers 422 naming the row. Warehouses of another company are still refused with the same generic message. Co-authored-by: Claude Opus 5.5 --- app/poros/orders/replace_order_lines.rb | 4 +- .../products/concerns/warehouse_validation.rb | 24 +++++++++-- spec/requests/api/v1/order_updates_spec.rb | 17 ++++++++ spec/requests/api/v1/products_spec.rb | 42 +++++++++++++++++++ 4 files changed, 82 insertions(+), 5 deletions(-) diff --git a/app/poros/orders/replace_order_lines.rb b/app/poros/orders/replace_order_lines.rb index 7e34fb0..bb6bd4d 100644 --- a/app/poros/orders/replace_order_lines.rb +++ b/app/poros/orders/replace_order_lines.rb @@ -115,8 +115,10 @@ def validate_lines_without_warehouse! # Warehouse es CompanyScoped, pero fuera de un request Current puede estar # en nil y el scope no aplica: el company_id va explícito, como en el alta. + # Enteros antes de comparar: `["1", 1]` son el mismo depósito y, crudos, + # contaban como dos y daban un 422 falso de «no pertenece a esta empresa». def validate_new_warehouses! - ids = new_items.pluck(:warehouse_id).uniq + ids = new_items.map { |item| item[:warehouse_id].to_s.to_i }.uniq return if Warehouse.where(id: ids, company_id: @order.company_id).count == ids.size raise ActiveRecord::RecordNotSaved, 'One or more warehouses do not belong to this company' diff --git a/app/poros/products/concerns/warehouse_validation.rb b/app/poros/products/concerns/warehouse_validation.rb index 82815c8..4825209 100644 --- a/app/poros/products/concerns/warehouse_validation.rb +++ b/app/poros/products/concerns/warehouse_validation.rb @@ -14,15 +14,31 @@ module WarehouseValidation # revierte todo). Warehouse.where(id:) ya filtra por empresa vía el # default_scope de CompanyScoped, así que no hace falta repetir company_id. def validate_warehouses_belong_to_company! - # rubocop:disable-next Rails/Pluck - warehouse_ids = @stocks_params.map { |s| s[:warehouse_id] }.uniq - owned = Warehouse.where(id: warehouse_ids).pluck(:id) - return if owned.sort == warehouse_ids.sort + warehouse_ids = normalized_warehouse_ids! + return if Warehouse.where(id: warehouse_ids).count == warehouse_ids.size # Mensaje genérico a propósito: no exponer IDs de depósitos de otro tenant. raise ActiveRecord::RecordNotSaved, 'One or more warehouses do not belong to this company' end + + # Los ids del request como enteros únicos. Antes se comparaban crudos: una + # fila sin `warehouse_id` hacía reventar el `sort` (nil contra Integer, un + # 500), y ids que llegaban como texto ("5") no coincidían con los de la base + # y daban un 422 falso de «no pertenece a esta empresa». + def normalized_warehouse_ids! + @stocks_params.map.with_index { |stock, index| warehouse_id_of!(stock, index) }.uniq + end + + def warehouse_id_of!(stock, index) + value = stock[:warehouse_id] + id = value if value.is_a?(Integer) + id ||= value.to_i if value.is_a?(String) && value.match?(/\A\d+\z/) + return id if id&.positive? + + raise ActiveRecord::RecordNotSaved, + "stocks[#{index}]: warehouse_id must be a positive integer" + end end end end diff --git a/spec/requests/api/v1/order_updates_spec.rb b/spec/requests/api/v1/order_updates_spec.rb index 089ad88..23c97da 100644 --- a/spec/requests/api/v1/order_updates_spec.rb +++ b/spec/requests/api/v1/order_updates_spec.rb @@ -283,4 +283,21 @@ def foreign_order expect(order.reload.customer_name).to eq('Juan Pérez') end end + + # Hallazgo de auditoría (TESIS-89): el mismo depósito en texto y en número + # contaba como dos y daba un 422 falso. + def stocked_second_product + Product.create!(company: company, sku: 'CEL-2', name: 'Funda').tap do |second| + Stock.create!(product: second, warehouse: warehouse, quantity: 5) + end + end + + it 'accepts new lines that name the same warehouse as text and as a number' do + second = stocked_second_product + put_order(items: [{ id: line.id, quantity: 4 }, + { product_id: second.id, warehouse_id: warehouse.id.to_s, quantity: 1, unit_price: 5 }, + { product_id: product.id, warehouse_id: warehouse.id, quantity: 1, unit_price: 100 }]) + + expect(response).to have_http_status(:ok) + end end diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 439bebb..09eebe8 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -977,6 +977,48 @@ def put_stock(quantity) end end + # Hallazgo de auditoría (TESIS-89): las filas de `stocks` se comparaban crudas + # contra los ids de la base. Una fila sin depósito hacía reventar el `sort` + # (500), y un id en texto daba un 422 falso de «no pertenece a la empresa». + describe 'the warehouse of each stock row' do + let(:product) { Product.create!(company: company, sku: 'ROW-001', name: 'Filas') } + let(:central) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') + end + + def put_stocks(stocks) + put "/api/v1/products/#{product.id}", + params: { product: { name: 'Filas', stocks: stocks } }, headers: headers, as: :json + end + + it 'answers 422 and names the row that has no warehouse', :aggregate_failures do + put_stocks([{ warehouse_id: central.id, quantity: 1 }, { quantity: 3 }]) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to eq('stocks[1]: warehouse_id must be a positive integer') + end + + it 'accepts a warehouse id that comes as text', :aggregate_failures do + put_stocks([{ warehouse_id: central.id.to_s, quantity: 4 }]) + + expect(response).to have_http_status(:ok) + expect(Stock.find_by(product: product, warehouse: central).quantity).to eq(4) + end + + it 'still refuses a warehouse of another company' do + put_stocks([{ warehouse_id: other_warehouse.id, quantity: 1 }]) + + expect(response.parsed_body['error']).to eq('One or more warehouses do not belong to this company') + end + + it 'refuses the same row on creation too' do + post '/api/v1/products', params: { product: { sku: 'ROW-002', name: 'Nuevo', stocks: [{ quantity: 1 }] } }, + headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + end + end + describe 'PATCH /api/v1/products/:id' do let!(:product) do Product.create!(company: company, sku: 'PROD-001', name: 'Original')