From e6512dcc373c00b90d622c6d1e1b0e2cd665fed4 Mon Sep 17 00:00:00 2001 From: LauAubert Date: Fri, 2 Oct 2026 01:54:29 -0300 Subject: [PATCH] fix: [TESIS-999018] validate the warehouse of each product stock row 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 7e34fb0c..bb6bd4db 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 82815c8f..48252090 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 089ad883..23c97da4 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 2c1a67a5..c68eee4e 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -841,6 +841,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')