diff --git a/app/controllers/api/v1/products_controller.rb b/app/controllers/api/v1/products_controller.rb index d27f8c1..9f530a3 100644 --- a/app/controllers/api/v1/products_controller.rb +++ b/app/controllers/api/v1/products_controller.rb @@ -6,6 +6,7 @@ class ProductsController < ApplicationController before_action :set_product, only: %i[show update destroy] rescue_from ActiveRecord::RecordNotUnique, with: :render_conflict rescue_from ActiveRecord::RecordNotSaved, with: :render_unprocessable + rescue_from Catalog::StaleProductError, with: :render_precondition_failed def index page = [params[:page].to_i, 1].max @@ -38,6 +39,7 @@ def index end def show + expose_version(@product) render json: ProductSerializer.render(@product) end @@ -65,9 +67,11 @@ def update product = Products::UpdateProduct.new( product: @product, params: product_params, - stocks: stock_params + stocks: stock_params, + expected_version: expected_version ).call + expose_version(product) render json: ProductSerializer.render(product), status: :ok end @@ -78,6 +82,33 @@ def destroy private + # La version del agregado viaja como ETag (TESIS-101). El cliente la + # devuelve en `If-Match` al guardar y el servidor rechaza la escritura si + # ya no es la vigente. + def expose_version(product) + response.set_header('ETag', %("#{Catalog::ProductVersion.new(product: product).call}")) + end + + # `If-Match` puede venir con comillas, con el prefijo debil `W/` o como + # `*`. `*` significa "cualquier version, siempre que exista": el producto + # ya se resolvio en set_product, asi que equivale a no poner precondicion. + def expected_version + raw = request.headers['If-Match'].to_s.strip + return nil if raw.blank? || raw == '*' + + raw.delete_prefix('W/').delete_prefix('"').delete_suffix('"') + end + + # 412 y no 409, apartandose de lo que pedia la card. Es el codigo que HTTP + # define para una precondicion que no se cumple, y de paso resuelve solo el + # requisito de distinguirlo: este endpoint ya devuelve 409 por SKU + # duplicado y por lock de stock ocupado, y un tercer 409 obligaria al front + # a leer el cuerpo para saber cual es. Con 412 alcanza el status. + def render_precondition_failed(exception) + render json: { error: exception.message, current_version: exception.current_version }, + status: :precondition_failed + end + def set_product # Eager load de stocks y sus warehouses para evitar N+1 en el detalle. @product = Product.includes(stocks: :warehouse).find(params.expect(:id)) diff --git a/app/controllers/api/v1/stock_transfers_controller.rb b/app/controllers/api/v1/stock_transfers_controller.rb new file mode 100644 index 0000000..b390ee0 --- /dev/null +++ b/app/controllers/api/v1/stock_transfers_controller.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +module Api + module V1 + class StockTransfersController < ApplicationController + before_action :set_transfer, only: %i[receive cancel] + rescue_from Catalog::InsufficientWarehouseStockError, with: :render_unprocessable + rescue_from Catalog::SettleTransfer::NotInFlightError, with: :render_conflict + + def index + transfers = policy_scope(StockTransfer) + .includes(:product, :origin_warehouse, :destination_warehouse) + .order(dispatched_at: :desc) + transfers = transfers.where(status: params[:status]) if params[:status].present? + transfers = transfers.where(product_id: params[:product_id]) if params[:product_id].present? + + render json: { data: StockTransferSerializer.render_as_hash(transfers) } + end + + def create + authorize StockTransfer + + transfer = Catalog::DispatchTransfer.new( + company: current_company, product: product_for_create, + origin_warehouse: warehouse_for(:origin_warehouse_id), + destination_warehouse: warehouse_for(:destination_warehouse_id), + quantity: transfer_params[:quantity] + ).call + + render json: StockTransferSerializer.render(transfer), status: :created + end + + def receive + settle(:received) + end + + def cancel + settle(:cancelled) + end + + private + + def settle(outcome) + transfer = Catalog::SettleTransfer.new(transfer: @transfer, outcome: outcome).call + + render json: StockTransferSerializer.render(transfer), status: :ok + end + + def set_transfer + @transfer = StockTransfer.find(params.expect(:id)) + authorize @transfer, :"#{action_name}?" + end + + # find y no find_by en el scope del tenant: el default_scope de + # CompanyScoped ya acota, así que un id de otra empresa levanta + # RecordNotFound -> 404, que es lo que corresponde (no revelar que existe). + def product_for_create + Product.find(transfer_params[:product_id]) + end + + def warehouse_for(key) + Warehouse.find(transfer_params[key]) + end + + def transfer_params + # permit y no expect, igual que en productos: un body con company_id se + # ignora en lugar de devolver 400. + # rubocop:disable Rails/StrongParametersExpect + params.require(:stock_transfer) + .permit(:product_id, :origin_warehouse_id, :destination_warehouse_id, :quantity) + # rubocop:enable Rails/StrongParametersExpect + end + + def render_conflict(exception) + render json: { error: exception.message }, status: :conflict + end + end + end +end diff --git a/app/models/product.rb b/app/models/product.rb index c4fab73..fedb811 100644 --- a/app/models/product.rb +++ b/app/models/product.rb @@ -11,8 +11,23 @@ class Product < ApplicationRecord # Sumar una categoría tiene que ser una línea acá, no una migración. CATEGORIES = %w[Electronics Machinery Cabling Power].freeze + # Unidades en vuelo hacia/desde depósitos, como subconsulta escalar. + # + # Subconsulta y no un segundo left_joins: `with_total_stock` ya hace join con + # `stocks` y agrupa por products.id. Sumar un join a `stock_transfers` daría + # producto cartesiano entre las dos tablas hijas y el SUM de stocks quedaría + # multiplicado por la cantidad de transferencias. Es una query igual —no N+1— + # pero sin contaminar la agregación existente. + IN_TRANSIT_SUBQUERY = <<~SQL.squish + SELECT COALESCE(SUM(st.quantity), 0) FROM stock_transfers st + WHERE st.product_id = products.id AND st.status = 'in_transit' + SQL + belongs_to :company has_many :stocks, dependent: :destroy + # restrict_with_error: una transferencia en vuelo son unidades reales ya + # descontadas del origen. Borrar el producto las haría desaparecer sin rastro. + has_many :stock_transfers, dependent: :restrict_with_error has_many :product_mappings, dependent: :destroy # Bloquea el borrado si hay ítems de órdenes: son registros financieros y no # deben evaporarse por un DELETE. destroy! levanta RecordNotDestroyed -> 409 (API). @@ -28,7 +43,8 @@ class Product < ApplicationRecord scope :with_total_stock, lambda { left_joins(:stocks) .group(:id) - .select('products.*', 'COALESCE(SUM(stocks.quantity), 0) AS total_stock') + .select('products.*', 'COALESCE(SUM(stocks.quantity), 0) AS total_stock', + "(#{IN_TRANSIT_SUBQUERY}) AS in_transit_quantity") } # Retorna el stock total consolidado. Si la fila fue cargada con el scope @@ -40,6 +56,17 @@ def total_stock has_attribute?(:total_stock) ? self[:total_stock].to_i : stocks.sum(:quantity) end + # Unidades que salieron de un depósito y todavía no llegaron a otro. No están + # en `total_stock` a propósito: no son stock disponible en ningún nodo. + # + # Misma mecánica que total_stock: si la fila vino del scope, el alias del + # SELECT ya trae el agregado; si no, se suma por asociación (detalle, alta). + def in_transit_quantity + return self[:in_transit_quantity].to_i if has_attribute?(:in_transit_quantity) + + stock_transfers.in_flight.sum(:quantity) + 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_transfer.rb b/app/models/stock_transfer.rb new file mode 100644 index 0000000..10c6f47 --- /dev/null +++ b/app/models/stock_transfer.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true + +# Unidades de un producto que salieron de un depósito y todavía no llegaron a +# otro. Existe para poder expresar algo que `stocks` no puede: mientras viajan, +# las unidades no están en ningún nodo. +# +# Por eso NO se cuentan en `Product#total_stock`. El catálogo las expone aparte +# (`in_transit_quantity`), que es lo que alimenta el tab "In Transit" y el +# "+N Incoming" de TESIS-62. +# +# No hay estado borrador a propósito: una transferencia creada y no despachada +# deja las unidades en el origen, así que no aporta a los números que este +# modelo existe para producir. Crear una transferencia ES despacharla. +class StockTransfer < ApplicationRecord + include CompanyScoped + + STATUSES = { in_transit: 'in_transit', received: 'received', cancelled: 'cancelled' }.freeze + + belongs_to :company + belongs_to :product + belongs_to :origin_warehouse, class_name: 'Warehouse' + belongs_to :destination_warehouse, class_name: 'Warehouse' + + # Mismo criterio que WebhookLog y FailedEvent: el enum da predicados y scopes, + # y `validate: true` invalida un estado desconocido en vez de explotar al + # asignarlo. + enum :status, STATUSES, validate: true + + validates :quantity, numericality: { only_integer: true, greater_than: 0 } + validate :warehouses_are_distinct + validate :product_and_warehouses_belong_to_company + + # Unidades en vuelo por producto. Se usa como subconsulta agregada desde el + # listado del catálogo: una sola query para toda la página, no una por fila. + scope :in_flight, -> { where(status: STATUSES[:in_transit]) } + + private + + def warehouses_are_distinct + return if origin_warehouse_id.blank? || destination_warehouse_id.blank? + return if origin_warehouse_id != destination_warehouse_id + + errors.add(:destination_warehouse, 'must be different from the origin warehouse') + end + + # El producto y los dos depósitos tienen que ser de la misma empresa que la + # transferencia: evita mover unidades entre tenants. Mismo criterio que Stock. + def product_and_warehouses_belong_to_company + return if company_id.blank? + + { product: product, origin_warehouse: origin_warehouse, + destination_warehouse: destination_warehouse }.each do |name, record| + next if record.blank? || record.company_id == company_id + + errors.add(name, 'must belong to the same company as the transfer') + end + end +end diff --git a/app/policies/stock_transfer_policy.rb b/app/policies/stock_transfer_policy.rb new file mode 100644 index 0000000..7ce239b --- /dev/null +++ b/app/policies/stock_transfer_policy.rb @@ -0,0 +1,25 @@ +# frozen_string_literal: true + +class StockTransferPolicy < ApplicationPolicy + def index? + user.present? + end + + def create? + user.present? + end + + def receive? + record.company_id == user.company_id + end + + def cancel? + record.company_id == user.company_id + end + + class Scope < ApplicationPolicy::Scope + def resolve + scope.where(company_id: user.company_id) + end + end +end diff --git a/app/poros/catalog/adjust_warehouse_stock.rb b/app/poros/catalog/adjust_warehouse_stock.rb new file mode 100644 index 0000000..882e0fc --- /dev/null +++ b/app/poros/catalog/adjust_warehouse_stock.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module Catalog + # Suma o resta unidades de un producto en un depósito concreto. + # + # Es el único lugar que escribe `stocks.quantity` para las transferencias, y + # las tres transiciones (despachar, recibir, cancelar) pasan por acá — por eso + # existe como pieza aparte y no repetida en cada una. + # + # NO toma el advisory lock: lo toma quien la invoca, porque una transferencia + # necesita que la lectura del saldo y la escritura estén dentro del MISMO lock + # que la creación de la fila de transferencia. Tomarlo acá lo cerraría antes de + # tiempo y dejaría la ventana que el lock existe para cerrar (ADR-009). + class AdjustWarehouseStock < ApplicationPoro + def initialize(product:, warehouse:, delta:) + super() + @product = product + @warehouse = warehouse + @delta = delta.to_i + end + + def call + stock = Stock.find_or_initialize_by(product_id: @product.id, warehouse_id: @warehouse.id) + resulting = stock.quantity.to_i + @delta + ensure_available!(stock, resulting) + + stock.quantity = resulting + stock.save! + stock + end + + private + + # El CHECK `stocks_quantity_non_negative` es la última línea de defensa, pero + # llegaría como CheckViolation genérica. Cortar acá deja un error que nombra + # el depósito, lo disponible y lo pedido. + def ensure_available!(stock, resulting) + return unless resulting.negative? + + raise InsufficientWarehouseStockError.new( + product_id: @product.id, warehouse_id: @warehouse.id, + available: stock.quantity.to_i, requested: @delta.abs + ) + end + end +end diff --git a/app/poros/catalog/dispatch_transfer.rb b/app/poros/catalog/dispatch_transfer.rb new file mode 100644 index 0000000..c7bf176 --- /dev/null +++ b/app/poros/catalog/dispatch_transfer.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +module Catalog + # Despacha unidades de un depósito a otro: descuenta del origen y deja la + # transferencia en vuelo. + # + # Crear la transferencia y mover el stock es una sola operación atómica, bajo + # el advisory lock del producto (ADR-009): la lectura del saldo del origen y su + # escritura tienen que estar dentro del mismo lock, o dos despachos + # simultáneos del mismo producto podrían descontar sobre el mismo saldo. + class DispatchTransfer < ApplicationPoro + def initialize(company:, product:, origin_warehouse:, destination_warehouse:, quantity:) + super() + @company = company + @product = product + @origin = origin_warehouse + @destination = destination_warehouse + @quantity = quantity + end + + def call + WithStockLock.new(product_id: @product.id, wait: false).call do + transfer = build_transfer + # Se valida antes de tocar stock: un origen igual al destino o una + # cantidad inválida no deben dejar unidades descontadas. + transfer.save! + AdjustWarehouseStock.new(product: @product, warehouse: @origin, + delta: -transfer.quantity).call + transfer + end + end + + private + + def build_transfer + StockTransfer.new(company: @company, product: @product, origin_warehouse: @origin, + destination_warehouse: @destination, quantity: @quantity, + status: :in_transit, dispatched_at: Time.current) + end + end +end diff --git a/app/poros/catalog/insufficient_warehouse_stock_error.rb b/app/poros/catalog/insufficient_warehouse_stock_error.rb new file mode 100644 index 0000000..cc790a9 --- /dev/null +++ b/app/poros/catalog/insufficient_warehouse_stock_error.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +module Catalog + # No hay unidades suficientes en el depósito para el movimiento pedido. + # El controller lo mapea a 422: es un dato del request, no un fallo del sistema. + # + # Hermano de InsufficientStockError y no el mismo error: aquel responde "el + # producto no alcanza en ningún lado" (lo levanta DeductStock al ingerir una + # orden) y éste "no alcanza en ESTE depósito", que es la pregunta de una + # transferencia entre nodos. Comparten la causa pero no los datos: uno nombra + # el producto, el otro el depósito, lo disponible y lo pedido. + class InsufficientWarehouseStockError < StandardError + attr_reader :product_id, :warehouse_id, :available, :requested + + def initialize(product_id:, warehouse_id:, available:, requested:) + @product_id = product_id + @warehouse_id = warehouse_id + @available = available + @requested = requested + super("warehouse #{warehouse_id} holds #{available} units of product " \ + "#{product_id}, cannot move #{requested}") + end + end +end diff --git a/app/poros/catalog/product_version.rb b/app/poros/catalog/product_version.rb new file mode 100644 index 0000000..cc6a83e --- /dev/null +++ b/app/poros/catalog/product_version.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'digest' + +module Catalog + # Huella del estado del producto tal como lo vio el cliente: los campos que el + # ABM edita, mas el stock de cada deposito. + # + # Es la version que viaja como ETag y vuelve en `If-Match`. Cubre el agregado + # completo y no solo la fila `products` a proposito: el modal edita nombre, + # medidas y cantidades a la vez, y una version que solo mirara `products` no + # detectaria que alguien movio el stock mientras el modal estaba abierto. + # + # Y no solo protege de otro operador: el caso mas peligroso es una venta. Si un + # webhook descuenta 5 unidades entre que el modal abre y guarda, guardar la + # cantidad ABSOLUTA que el usuario vio borraria ese descuento sin dejar rastro. + # Por eso la huella incluye el stock venga de donde venga el cambio. + class ProductVersion < ApplicationPoro + SEPARATOR = '|' + + def initialize(product:) + super() + @product = product + end + + def call + Digest::SHA256.hexdigest(fingerprint) + end + + private + + def fingerprint + (edited_fields + stock_pairs).join(SEPARATOR) + end + + def edited_fields + [@product.name.to_s, @product.description.to_s, + @product.weight.to_s, @product.dimensions.to_s] + end + + # Los depositos se ordenan antes de digerir: `product.stocks` no garantiza + # orden, y sin esto la misma fila daria huellas distintas entre requests. + def stock_pairs + @product.stocks.sort_by(&:warehouse_id).map { |s| "#{s.warehouse_id}:#{s.quantity}" } + end + end +end diff --git a/app/poros/catalog/settle_transfer.rb b/app/poros/catalog/settle_transfer.rb new file mode 100644 index 0000000..2a6949b --- /dev/null +++ b/app/poros/catalog/settle_transfer.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +module Catalog + # Cierra una transferencia en vuelo: la recibe en el destino o la cancela + # devolviendo las unidades al origen. + # + # Las dos transiciones comparten todo salvo a qué depósito vuelven las + # unidades y con qué estado queda la fila, así que viven en una sola pieza en + # lugar de dos casi idénticas. + class SettleTransfer < ApplicationPoro + class NotInFlightError < StandardError; end + + OUTCOMES = { + received: :destination_warehouse, + cancelled: :origin_warehouse + }.freeze + + def initialize(transfer:, outcome:) + super() + @transfer = transfer + @outcome = outcome.to_sym + end + + def call + warehouse_method = OUTCOMES.fetch(@outcome) do + raise ArgumentError, "unknown outcome '#{@outcome}'" + end + + WithStockLock.new(product_id: @transfer.product_id, wait: false).call do + # Se revisa adentro del lock: dos requests simultáneos sobre la misma + # transferencia podrían pasar los dos el chequeo si estuviera afuera, y + # las unidades entrarían al destino dos veces. + ensure_in_flight! + settle(@transfer.public_send(warehouse_method)) + end + end + + private + + def ensure_in_flight! + return if @transfer.reload.in_transit? + + raise NotInFlightError, + "transfer #{@transfer.id} is already #{@transfer.status}" + end + + def settle(warehouse) + AdjustWarehouseStock.new(product: @transfer.product, warehouse: warehouse, + delta: @transfer.quantity).call + @transfer.update!(status: @outcome, settled_at: Time.current) + @transfer + end + end +end diff --git a/app/poros/catalog/stale_product_error.rb b/app/poros/catalog/stale_product_error.rb new file mode 100644 index 0000000..9369e39 --- /dev/null +++ b/app/poros/catalog/stale_product_error.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +module Catalog + # El cliente mando `If-Match` con una version que ya no es la vigente: alguien + # toco el producto entre que lo leyo y lo guardo. + # + # Lleva la version actual para que el controller la devuelva en la respuesta: + # el cliente puede recargar y reintentar sin pedir el detalle de nuevo. + class StaleProductError < StandardError + attr_reader :current_version + + def initialize(current_version:) + @current_version = current_version + super('the product changed since it was loaded') + end + end +end diff --git a/app/poros/products/update_product.rb b/app/poros/products/update_product.rb index b168181..ff8d1e5 100644 --- a/app/poros/products/update_product.rb +++ b/app/poros/products/update_product.rb @@ -4,15 +4,17 @@ module Products class UpdateProduct < ApplicationPoro include Concerns::WarehouseValidation - def initialize(product:, params:, stocks:) + def initialize(product:, params:, stocks:, expected_version: nil) super() @product = product @params = params @stocks_params = stocks + @expected_version = expected_version end def call Product.transaction do + verify_version! @product.update!(@params) if @stocks_params.present? @@ -26,6 +28,27 @@ def call private + # Locking optimista (TESIS-101). El chequeo va DENTRO de la transaccion y + # detras de un `lock!` --o sea `SELECT ... FOR UPDATE` sobre la fila del + # producto-- y no antes: comparar afuera dejaria pasar a dos requests que + # leyeron la misma version, que es exactamente la carrera que esto cierra. + # Con la fila tomada, el segundo espera, relee el estado que dejo el primero + # y su version ya no coincide. + # + # Sin `expected_version` no hay precondicion que verificar y el update pasa + # como siempre: es la semantica de `If-Match` en HTTP, y mantiene el + # contrato anterior para cualquier cliente que no lo mande. + def verify_version! + return if @expected_version.blank? + + @product.lock! + @product.stocks.reload + current = Catalog::ProductVersion.new(product: @product).call + return if current == @expected_version + + raise Catalog::StaleProductError.new(current_version: current) + end + # Advisory lock y no sólo FOR UPDATE: el upsert puede crear filas de # stocks que todavía no existen, y ahí FOR UPDATE no tiene nada que # bloquear. Además la operación abarca varias filas del mismo producto, diff --git a/app/serializers/product_list_serializer.rb b/app/serializers/product_list_serializer.rb index 5494747..e032fb7 100644 --- a/app/serializers/product_list_serializer.rb +++ b/app/serializers/product_list_serializer.rb @@ -4,7 +4,7 @@ class ProductListSerializer < ApplicationSerializer identifier :id fields :sku, :name, :description, :category, :dimensions, :total_stock, - :created_at, :updated_at + :in_transit_quantity, :created_at, :updated_at # weight es decimal en la DB y BigDecimal se serializa como string por # defecto; exponerlo como número evita que el front tenga que parsear. diff --git a/app/serializers/product_serializer.rb b/app/serializers/product_serializer.rb index 07da6fd..694a8c3 100644 --- a/app/serializers/product_serializer.rb +++ b/app/serializers/product_serializer.rb @@ -4,7 +4,7 @@ class ProductSerializer < ApplicationSerializer identifier :id fields :sku, :name, :description, :category, :dimensions, :total_stock, - :created_at, :updated_at + :in_transit_quantity, :created_at, :updated_at # weight es decimal en la DB y BigDecimal se serializa como string por # defecto; exponerlo como número evita que el front tenga que parsear. diff --git a/app/serializers/stock_transfer_serializer.rb b/app/serializers/stock_transfer_serializer.rb new file mode 100644 index 0000000..8199f1b --- /dev/null +++ b/app/serializers/stock_transfer_serializer.rb @@ -0,0 +1,22 @@ +# frozen_string_literal: true + +class StockTransferSerializer < ApplicationSerializer + identifier :id + + fields :quantity, :status, :dispatched_at, :settled_at, :created_at, :updated_at + + # Sólo la identidad del producto: quien lista transferencias ya tiene el + # catálogo, y anidar el serializer completo traería total_stock —una agregación + # por fila— para nada. + field :product do |transfer| + { id: transfer.product_id, sku: transfer.product.sku, name: transfer.product.name } + end + + field :origin_warehouse do |transfer| + { id: transfer.origin_warehouse_id, name: transfer.origin_warehouse.name } + end + + field :destination_warehouse do |transfer| + { id: transfer.destination_warehouse_id, name: transfer.destination_warehouse.name } + end +end diff --git a/config/initializers/cors.rb b/config/initializers/cors.rb index ff47924..5acc094 100644 --- a/config/initializers/cors.rb +++ b/config/initializers/cors.rb @@ -10,6 +10,13 @@ origins '*' resource '*', headers: :any, - methods: %i[get post put patch delete options head] + methods: %i[get post put patch delete options head], + # Sin `expose`, el browser le oculta el ETag al JavaScript: CORS + # sólo deja leer los headers simples salvo que el servidor los + # liste. El front corre en otro origen (5173 contra 3000), así que + # sin esta línea `response.headers.etag` llega `undefined`, el + # modal no manda `If-Match` y el locking optimista de TESIS-101 + # queda desactivado sin que nada falle a la vista. + expose: %w[ETag] end end diff --git a/config/routes.rb b/config/routes.rb index 49ba42d..1d115c7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -19,6 +19,13 @@ resources :mappings, only: %i[index create destroy], controller: 'product_mappings' end + resources :stock_transfers, path: 'stock-transfers', only: %i[index create] do + member do + post :receive + post :cancel + end + end + # `only: []` a propósito: esta card sólo agrega la cotización anidada. El # ABM de órdenes es TESIS-42 y define ahí sus propias acciones. resources :orders, only: [] do diff --git a/db/migrate/20260825120000_create_stock_transfers.rb b/db/migrate/20260825120000_create_stock_transfers.rb new file mode 100644 index 0000000..48f76f2 --- /dev/null +++ b/db/migrate/20260825120000_create_stock_transfers.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +class CreateStockTransfers < ActiveRecord::Migration[8.1] + def change + create_table :stock_transfers do |t| + t.references :company, null: false, foreign_key: { on_delete: :cascade } + # RESTRICT y no cascade: una transferencia es el registro de un movimiento + # de unidades reales. Si se pudiera borrar el producto o el depósito con + # transferencias vivas, quedarían unidades descontadas del origen sin + # rastro de a dónde iban. Mismo criterio que order_items -> products. + t.references :product, null: false, foreign_key: { on_delete: :restrict } + t.references :origin_warehouse, null: false, + foreign_key: { to_table: :warehouses, on_delete: :restrict } + t.references :destination_warehouse, null: false, + foreign_key: { to_table: :warehouses, + on_delete: :restrict } + t.integer :quantity, null: false + t.string :status, null: false, default: 'in_transit' + # Cuándo salieron las unidades del origen. NOT NULL porque crear la + # transferencia ES despacharla: no existe el estado borrador. + t.datetime :dispatched_at, null: false + # Cuándo se resolvió (recibida o cancelada). Nulo mientras está en vuelo. + t.datetime :settled_at + + t.timestamps + end + + add_check_constraint :stock_transfers, 'quantity > 0', + name: 'stock_transfers_quantity_positive' + add_check_constraint :stock_transfers, + "status IN ('in_transit', 'received', 'cancelled')", + name: 'stock_transfers_status_check' + add_check_constraint :stock_transfers, + 'origin_warehouse_id <> destination_warehouse_id', + name: 'stock_transfers_distinct_warehouses' + + # El listado del catálogo suma las unidades en vuelo por producto, siempre + # dentro de una empresa y siempre filtrando por estado. + add_index :stock_transfers, %i[company_id status product_id] + end +end diff --git a/db/schema.rb b/db/schema.rb index ba274b7..56332d2 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_08_24_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_08_25_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -169,6 +169,27 @@ t.check_constraint "status::text = ANY (ARRAY['pending'::character varying, 'ready_to_ship'::character varying, 'in_transit'::character varying, 'delivered'::character varying]::text[])", name: "shipments_status_check" end + create_table "stock_transfers", force: :cascade do |t| + t.bigint "company_id", null: false + t.datetime "created_at", null: false + t.bigint "destination_warehouse_id", null: false + t.datetime "dispatched_at", null: false + t.bigint "origin_warehouse_id", null: false + t.bigint "product_id", null: false + t.integer "quantity", null: false + t.datetime "settled_at" + t.string "status", default: "in_transit", null: false + t.datetime "updated_at", null: false + t.index ["company_id", "status", "product_id"], name: "index_stock_transfers_on_company_id_and_status_and_product_id" + t.index ["company_id"], name: "index_stock_transfers_on_company_id" + t.index ["destination_warehouse_id"], name: "index_stock_transfers_on_destination_warehouse_id" + t.index ["origin_warehouse_id"], name: "index_stock_transfers_on_origin_warehouse_id" + t.index ["product_id"], name: "index_stock_transfers_on_product_id" + t.check_constraint "origin_warehouse_id <> destination_warehouse_id", name: "stock_transfers_distinct_warehouses" + t.check_constraint "quantity > 0", name: "stock_transfers_quantity_positive" + t.check_constraint "status::text = ANY (ARRAY['in_transit'::character varying, 'received'::character varying, 'cancelled'::character varying]::text[])", name: "stock_transfers_status_check" + end + create_table "stocks", force: :cascade do |t| t.datetime "created_at", null: false t.bigint "product_id", null: false @@ -235,6 +256,10 @@ add_foreign_key "shipments", "companies", on_delete: :cascade add_foreign_key "shipments", "company_integrations", on_delete: :nullify add_foreign_key "shipments", "orders", on_delete: :cascade + add_foreign_key "stock_transfers", "companies", on_delete: :cascade + add_foreign_key "stock_transfers", "products", on_delete: :restrict + add_foreign_key "stock_transfers", "warehouses", column: "destination_warehouse_id", on_delete: :restrict + add_foreign_key "stock_transfers", "warehouses", column: "origin_warehouse_id", on_delete: :restrict add_foreign_key "stocks", "products", on_delete: :cascade add_foreign_key "stocks", "warehouses", on_delete: :restrict add_foreign_key "users", "companies", on_delete: :cascade diff --git a/db/seeds.rb b/db/seeds.rb index c22371c..b2b384e 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -277,6 +277,21 @@ Stock.find_or_create_by!(product: mouse, warehouse: satelite) { |s| s.quantity = 30 } end + # Transferencia en vuelo (TESIS-103): unidades que ya salieron del Central y + # todavía no llegaron al Satélite. No se usa DispatchTransfer porque el stock + # sembrado arriba ya refleja el saldo posterior al despacho; acá sólo se + # registra el movimiento para que el catálogo tenga un producto con + # `in_transit_quantity > 0` y el tab "In Transit" muestre algo real. + if central && satelite + StockTransfer.find_or_create_by!(product: notebook, origin_warehouse: central, + destination_warehouse: satelite, + status: 'in_transit') do |t| + t.company = norte_company + t.quantity = 5 + t.dispatched_at = 2.days.ago + end + end + # Identity Mapping: vincula productos de Norte con Mercado Libre, usando la # integración de la plantilla de stock (la de órdenes no transmite stock). ml_stock_service = Service.find_by(service_name: 'Mercado Libre - Stock') diff --git a/docs/adr/ADR-009-bloqueos-distribuidos.md b/docs/adr/ADR-009-bloqueos-distribuidos.md index 1af3c6e..e88d828 100644 --- a/docs/adr/ADR-009-bloqueos-distribuidos.md +++ b/docs/adr/ADR-009-bloqueos-distribuidos.md @@ -85,7 +85,56 @@ Catalog::WithStockLock.new(product_id: product.id, wait: false).call { ... } - ✅ Sin infraestructura nueva: PostgreSQL ya es parte del stack y ya es la fuente de verdad del stock - ✅ No hay locks huérfanos que limpiar: el motor los libera solo, al terminar la transacción (commit o rollback) - ✅ El PORO es explícito y auditable: clave, timeout y manejo de errores están a la vista en ~40 líneas -- ⚠️ El lock ordena escrituras, no detecta ediciones concurrentes: el lost update del ABM sigue abierto hasta que se implemente el locking optimista +- ✅ El lost update del ABM lo cierra el locking optimista de TESIS-101 (ver sección siguiente): el lock ordena, el ETag detecta - ⚠️ La granularidad por producto serializa de más cuando una empresa escribe stock del mismo producto en depósitos distintos al mismo tiempo - ⚠️ Las escrituras batch (`update_all`, `upsert_all`, SQL crudo) se saltean el PORO por completo — por eso existe el `CHECK` de la base como red de seguridad independiente - ⚠️ Los specs de concurrencia necesitan desactivar el envoltorio transaccional de RSpec (`use_transactional_tests = false`): si no, las dos "conexiones" que se quieren probar en paralelo viven dentro de la misma transacción de test y nunca compiten de verdad por el lock + +--- + +## Complemento: locking optimista del ABM (TESIS-101) + +Este ADR dejaba abierto el *lost update* del ABM. Se cierra con un mecanismo +distinto, porque responde a otra pregunta: el advisory lock **ordena** las +escrituras, y acá hace falta **detectar** que dos personas editaron lo mismo. + +### Decisión: ETag + `If-Match`, no `lock_version` + +`GET /products/:id` devuelve un `ETag` con la huella del agregado, y +`PUT /products/:id` la exige de vuelta en `If-Match`. + +**Por qué no `lock_version`.** La columna versiona la fila `products`, y el modal +edita un agregado: nombre, medidas **y** las cantidades de cada depósito. Para +que `lock_version` cubriera el stock habría que bumpearlo desde los callbacks de +`Stock`, o sea escribir en `products` en cada venta — amplificación de escritura +y contención justo en el camino más caliente del sistema. El ETag cubre el +agregado completo sin tocar la base ni sumar una migración. + +**Qué cubre la huella** (`Catalog::ProductVersion`): los campos que el modal +edita más el par `warehouse_id:quantity` de cada depósito, ordenados. Que incluya +el stock no es un extra: el escenario más peligroso no son dos operadores sino +**una venta**. Si un webhook descuenta 5 unidades mientras el modal está abierto, +guardar la cantidad *absoluta* que el usuario vio borraría ese descuento sin +dejar rastro. + +**Dónde se verifica.** Dentro de la transacción y detrás de un `lock!` +(`SELECT ... FOR UPDATE` sobre la fila del producto). Comparar afuera dejaría +pasar a dos requests que leyeron la misma versión — exactamente la carrera que +esto cierra. + +### Desvío de la card: 412, no 409 + +La card pedía 409 «con un cuerpo que permita distinguirlo de los otros dos 409». +Se devuelve **412 Precondition Failed**, que es el código que HTTP define para +una precondición incumplida — y de paso resuelve solo el requisito: el endpoint +ya devuelve 409 por SKU duplicado y por lock de stock ocupado, y un tercer 409 +obligaría al front a leer el cuerpo para saber cuál es. Con 412 alcanza el +status. + +### Consecuencias + +- ✅ Sin migración y sin columna nueva +- ✅ Cubre cualquier escritor: otro operador, una venta por webhook, un job +- ✅ Sin `If-Match` el update pasa como siempre — semántica de HTTP, y no rompe el contrato anterior +- ⚠️ Esa misma compatibilidad hace que la protección sea **opt-in**: un cliente que no manda el header no está protegido +- ⚠️ La huella se recalcula en cada `show` y en cada `update`; es un SHA-256 sobre unas pocas decenas de bytes, pero no es gratis diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index 1acf53d..d7056d6 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -274,11 +274,24 @@ El proyecto está organizado en 6 dominios correspondientes a los epics de Jira: | ------------ | ---------- | -------------------------------------------------------- | | `auth` | TESIS-19 | Empresas, usuarios, depósitos, autenticación JWT | | `integrations` | TESIS-20 | Plantillas de APIs externas, credenciales por empresa | -| `catalog` | TESIS-21 | Productos, stock por depósito, sincronización multicanal | +| `catalog` | TESIS-21 | Productos, stock por depósito, transferencias entre depósitos, sincronización multicanal | | `webhooks` | TESIS-22 | Gateway de webhooks, cola de mensajes, reintentos | | `orders` | TESIS-23 | Órdenes de compra, ítems, consolidación multicanal | | `shipments` | TESIS-24 | Envíos, cotización de couriers, tracking | +> **Unidades en vuelo.** `stocks` responde "cuántas unidades hay en este +> depósito", y no puede expresar unidades que salieron de uno y todavía no +> llegaron a otro. Eso vive en `stock_transfers` (TESIS-103): al despachar se +> descuentan del origen, al recibir se suman al destino, y mientras viajan no +> pertenecen a ningún nodo — por eso **no** entran en `Product#total_stock` y se +> exponen aparte como `in_transit_quantity`. +> +> Las tres transiciones mueven stock real bajo el advisory lock del producto +> (§9), y el listado del catálogo agrega las unidades en vuelo con una +> **subconsulta escalar**, no con un segundo `left_joins`: `with_total_stock` ya +> hace join con `stocks` y agrupa, así que un segundo join a una tabla hija daría +> producto cartesiano y multiplicaría el `SUM` de stock. + Los POROs y Jobs se organizan por dominio: ``` diff --git a/spec/models/stock_transfer_spec.rb b/spec/models/stock_transfer_spec.rb new file mode 100644 index 0000000..f5cd1d8 --- /dev/null +++ b/spec/models/stock_transfer_spec.rb @@ -0,0 +1,88 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe StockTransfer, type: :model do + subject(:transfer) do + described_class.new(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: 5, + dispatched_at: Time.current) + end + + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Widget') } + let(:origin) { warehouse('Central', '1900') } + let(:destination) { warehouse('North', '1901') } + + def warehouse(name, zip, owner: company) + Warehouse.create!(company: owner, name: name, zip_code: zip, address: "Calle #{zip}") + end + + def other_company + @other_company ||= Company.create!(name: 'Other', tax_id: '30-22222222-2') + end + + it 'is valid with the required attributes' do + expect(transfer).to be_valid + end + + it 'defaults to in_transit' do + transfer.save! + expect(transfer.status).to eq('in_transit') + end + + it 'rejects a non-positive quantity' do + transfer.quantity = 0 + expect(transfer).not_to be_valid + end + + it 'rejects the same warehouse on both ends', :aggregate_failures do + transfer.destination_warehouse = origin + expect(transfer).not_to be_valid + expect(transfer.errors[:destination_warehouse]).to be_present + end + + # `validate: true` en el enum existe para que un estado desconocido invalide + # el registro en lugar de explotar en el asignador (mismo criterio que + # WebhookLog y FailedEvent). + it 'rejects an unknown status without raising on assignment' do + transfer.status = 'lost' + expect(transfer).not_to be_valid + end + + describe 'the same-company rule' do + it 'rejects a product from another company' do + transfer.product = Current.set(company_id: nil) do + Product.create!(company: other_company, sku: 'B-1', name: 'Other') + end + expect(transfer).not_to be_valid + end + + it 'rejects a warehouse from another company' do + transfer.destination_warehouse = Current.set(company_id: nil) do + warehouse('Foreign', '3000', owner: other_company) + end + expect(transfer).not_to be_valid + end + end + + describe 'the database constraints' do + # Las validaciones del modelo son la primera línea; el CHECK es la que no se + # puede saltear con update_all / SQL crudo. Mismo criterio que stocks.quantity. + it 'rejects a non-positive quantity at the database level' do + transfer.save! + + expect do + described_class.where(id: transfer.id).update_all(quantity: 0) # rubocop:disable Rails/SkipsModelValidations + end.to raise_error(ActiveRecord::StatementInvalid) + end + + it 'rejects an unknown status at the database level' do + transfer.save! + + expect do + described_class.where(id: transfer.id).update_all(status: 'lost') # rubocop:disable Rails/SkipsModelValidations + end.to raise_error(ActiveRecord::StatementInvalid) + end + end +end diff --git a/spec/poros/catalog/dispatch_transfer_spec.rb b/spec/poros/catalog/dispatch_transfer_spec.rb new file mode 100644 index 0000000..6d85694 --- /dev/null +++ b/spec/poros/catalog/dispatch_transfer_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Catalog::DispatchTransfer, type: :poro do + subject(:dispatch) do + described_class.new(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: quantity) + end + + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Widget') } + let(:origin) { Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'A') } + let(:destination) { Warehouse.create!(company: company, name: 'North', zip_code: '1901', address: 'B') } + let(:quantity) { 4 } + + before { Stock.create!(product: product, warehouse: origin, quantity: 10) } + + def origin_quantity = Stock.find_by(product: product, warehouse: origin).quantity + + it 'leaves the transfer in flight' do + expect(dispatch.call).to be_in_transit + end + + it 'deducts the units from the origin warehouse' do + dispatch.call + expect(origin_quantity).to eq(6) + end + + it 'does not put the units in the destination yet' do + dispatch.call + expect(Stock.find_by(product: product, warehouse: destination)).to be_nil + end + + it 'keeps the units out of total_stock while they travel' do + dispatch.call + expect(product.reload.total_stock).to eq(6) + end + + it 'reports them as in transit' do + dispatch.call + expect(product.reload.in_transit_quantity).to eq(4) + end + + context 'when the origin does not hold enough units' do + let(:quantity) { 99 } + + it 'raises instead of leaving a negative balance' do + expect { dispatch.call }.to raise_error(Catalog::InsufficientWarehouseStockError) + end + + context 'when the dispatch has already failed' do + before { suppress(Catalog::InsufficientWarehouseStockError) { dispatch.call } } + + it 'writes no transfer at all' do + expect(StockTransfer.count).to eq(0) + end + + it 'leaves the origin balance untouched' do + expect(origin_quantity).to eq(10) + end + end + end +end diff --git a/spec/poros/catalog/product_version_spec.rb b/spec/poros/catalog/product_version_spec.rb new file mode 100644 index 0000000..33977ed --- /dev/null +++ b/spec/poros/catalog/product_version_spec.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Catalog::ProductVersion, type: :poro do + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Widget', weight: 2) } + let(:central) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'A') + end + let(:north) do + Warehouse.create!(company: company, name: 'North', zip_code: '1901', address: 'B') + end + + # Metodo y no `subject`: RSpec memoiza el subject, y estos ejemplos necesitan + # leer la version dos veces con un cambio en el medio. + def version + described_class.new(product: product.reload).call + end + + it 'is stable when nothing changes' do + expect(version).to eq(described_class.new(product: product.reload).call) + end + + it 'changes when an edited field changes' do + before_change = version + product.update!(name: 'Renamed') + + expect(version).not_to eq(before_change) + end + + # El caso que motiva la card: el modal guarda cantidades absolutas, asi que un + # movimiento de stock ajeno tiene que invalidar lo que el usuario vio. + it 'changes when the stock of a warehouse moves' do + stock = Stock.create!(product: product, warehouse: central, quantity: 10) + before_change = version + stock.update!(quantity: 5) + + expect(version).not_to eq(before_change) + end + + it 'changes when a warehouse is added' do + Stock.create!(product: product, warehouse: central, quantity: 10) + before_change = version + Stock.create!(product: product, warehouse: north, quantity: 3) + + expect(version).not_to eq(before_change) + end + + # Sin ordenar, la misma fila daria huellas distintas segun como Postgres + # devuelva los stocks, y el guardado fallaria al azar. + it 'does not depend on the order the stocks come back in' do + Stock.create!(product: product, warehouse: north, quantity: 3) + Stock.create!(product: product, warehouse: central, quantity: 10) + reversed = Product.find(product.id) + allow(reversed).to receive(:stocks).and_return(reversed.stocks.to_a.reverse) + + expect(described_class.new(product: reversed).call).to eq(version) + end + + it 'is not affected by fields the modal does not edit' do + before_change = version + product.update_column(:sku, 'SKU-CHANGED') # rubocop:disable Rails/SkipsModelValidations + + expect(version).to eq(before_change) + end +end diff --git a/spec/poros/catalog/settle_transfer_spec.rb b/spec/poros/catalog/settle_transfer_spec.rb new file mode 100644 index 0000000..dbb0655 --- /dev/null +++ b/spec/poros/catalog/settle_transfer_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Catalog::SettleTransfer, type: :poro do + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:product) { Product.create!(company: company, sku: 'SKU-1', name: 'Widget') } + let(:origin) { Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'A') } + let(:destination) { Warehouse.create!(company: company, name: 'North', zip_code: '1901', address: 'B') } + + let(:transfer) do + Stock.create!(product: product, warehouse: origin, quantity: 10) + Catalog::DispatchTransfer.new(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: 4).call + end + + def quantity_in(warehouse) + Stock.find_by(product: product, warehouse: warehouse)&.quantity || 0 + end + + describe 'receiving' do + before { described_class.new(transfer: transfer, outcome: :received).call } + + it 'marks the transfer as received' do + expect(transfer.reload).to be_received + end + + it 'adds the units to the destination' do + expect(quantity_in(destination)).to eq(4) + end + + it 'does not give them back to the origin' do + expect(quantity_in(origin)).to eq(6) + end + + it 'stops counting them as in transit' do + expect(product.reload.in_transit_quantity).to eq(0) + end + + it 'returns them to total_stock' do + expect(product.reload.total_stock).to eq(10) + end + + it 'stamps when it was settled' do + expect(transfer.reload.settled_at).to be_present + end + end + + describe 'cancelling' do + before { described_class.new(transfer: transfer, outcome: :cancelled).call } + + it 'marks the transfer as cancelled' do + expect(transfer.reload).to be_cancelled + end + + it 'gives the units back to the origin' do + expect(quantity_in(origin)).to eq(10) + end + + it 'leaves nothing in the destination' do + expect(quantity_in(destination)).to eq(0) + end + end + + # Sin este corte, dos requests simultáneos sobre la misma transferencia + # sumarían las unidades al destino dos veces. + it 'refuses to settle a transfer that is no longer in flight' do + described_class.new(transfer: transfer, outcome: :received).call + + expect { described_class.new(transfer: transfer, outcome: :cancelled).call } + .to raise_error(described_class::NotInFlightError) + end + + it 'does not move stock twice when settled again' do + described_class.new(transfer: transfer, outcome: :received).call + suppress(described_class::NotInFlightError) do + described_class.new(transfer: transfer, outcome: :received).call + end + + expect(quantity_in(destination)).to eq(4) + end + + it 'rejects an unknown outcome' do + expect { described_class.new(transfer: transfer, outcome: :lost).call } + .to raise_error(ArgumentError) + end +end diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 2bffe44..9aefd70 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -159,6 +159,63 @@ def holding_advisory_lock_for(product) it 'exposes the category of each product' do expect(response.parsed_body['data'].pluck('category')).to all(be_nil) end + + it 'reports no units in transit when there are no transfers' do + expect(response.parsed_body['data'].pluck('in_transit_quantity')).to all(eq(0)) + end + end + + context 'with units travelling between warehouses' do + let(:warehouse) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') + end + + def north + @north ||= Warehouse.create!(company: company, name: 'North', + zip_code: '1901', address: 'Calle 2') + end + + def dispatch(product, quantity) + Catalog::DispatchTransfer.new(company: company, product: product, + origin_warehouse: warehouse, destination_warehouse: north, + quantity: quantity).call + end + + def row + get '/api/v1/products', headers: headers + response.parsed_body['data'].find { |item| item['sku'] == 'A-001' } + end + + def stocked_product + Product.create!(company: company, sku: 'A-001', name: 'Alpha').tap do |product| + Stock.create!(product: product, warehouse: warehouse, quantity: 10) + end + end + + it 'reports the units in flight apart from the stock', :aggregate_failures do + dispatch(stocked_product, 3) + + expect(row['in_transit_quantity']).to eq(3) + expect(row['total_stock']).to eq(7) + end + + # La subconsulta escalar existe para no romper el SUM de total_stock: un + # segundo left_joins daría producto cartesiano entre stocks y transfers. + it 'does not corrupt total_stock when a product has several transfers' do + product = stocked_product + 2.times { dispatch(product, 2) } + + expect(row['total_stock']).to eq(6) + end + + # 0 y no 1: va como subconsulta escalar dentro del SELECT del listado, así + # que no hay ninguna consulta separada contra stock_transfers. + it 'adds no query per row' do + create_products_with_stock(10) + queries = count_queries(matching: /FROM "stock_transfers"/) { get '/api/v1/products', headers: headers } + + expect(queries).to eq(0) + end end context 'with stock spread across warehouses' do @@ -289,6 +346,106 @@ def row end end + describe 'optimistic locking with If-Match' do + let(:warehouse) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') + end + let!(:product) do + p = Product.create!(company: company, sku: 'A-001', name: 'Alpha', weight: 1) + Stock.create!(product: p, warehouse: warehouse, quantity: 10) + p + end + + def current_version + get "/api/v1/products/#{product.id}", headers: headers + response.headers['ETag'] + end + + def save(version, quantity: 7, name: 'Alpha') + body = { product: { name: name, weight: 1, + stocks: stocks_for(warehouse.id, quantity: quantity) } } + put "/api/v1/products/#{product.id}", + params: body, headers: headers.merge('If-Match' => version.to_s), as: :json + end + + it 'exposes the version as an ETag on show' do + expect(current_version).to be_present + end + + it 'accepts the write when the version still matches' do + save(current_version) + expect(response).to have_http_status(:ok) + end + + it 'returns the new version after a successful write', :aggregate_failures do + before_write = current_version + save(before_write) + + expect(response.headers['ETag']).to be_present + expect(response.headers['ETag']).not_to eq(before_write) + end + + # La carrera real de la card: dos ediciones que partieron de la misma + # version. La segunda no puede pisar a la primera en silencio. + context 'when someone else already saved' do + # Metodo y no `let` para no pasar el tope de helpers memoizados del grupo. + def stale + @stale ||= current_version + end + + before { save(stale, quantity: 20, name: 'First writer') } + + it 'rejects the second write with 412' do + save(stale, quantity: 3, name: 'Second writer') + expect(response).to have_http_status(:precondition_failed) + end + + it 'does not apply the second write' do + save(stale, quantity: 3, name: 'Second writer') + expect(product.reload.name).to eq('First writer') + end + + it 'does not touch the stock either' do + save(stale, quantity: 3, name: 'Second writer') + expect(Stock.find_by(product: product, warehouse: warehouse).quantity).to eq(20) + end + + it 'hands back the current version so the client can reload' do + save(stale, quantity: 3) + expect(response.parsed_body['current_version']).to be_present + end + end + + # El caso mas peligroso no es otro operador: es una venta descontando stock + # mientras el modal esta abierto. Guardar la cantidad absoluta lo borraria. + it 'rejects the write when stock moved underneath, even if nobody edited' do + stale = current_version + Stock.find_by(product: product, warehouse: warehouse).update!(quantity: 5) + save(stale) + + expect(response).to have_http_status(:precondition_failed) + end + + # Semantica de HTTP: sin precondicion, no hay nada que verificar. Mantiene + # el contrato anterior para un cliente que no manda el header. + it 'writes without If-Match, as before' do + body = { product: { name: 'No header', weight: 1 } } + put "/api/v1/products/#{product.id}", params: body, headers: headers, as: :json + + expect(response).to have_http_status(:ok) + end + + it 'treats If-Match: * as no precondition' do + save('*') + expect(response).to have_http_status(:ok) + end + + it 'accepts a weak or quoted version' do + save("W/#{current_version}") + expect(response).to have_http_status(:ok) + end + end + describe 'POST /api/v1/products' do let(:warehouse) do Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') diff --git a/spec/requests/api/v1/stock_transfers_spec.rb b/spec/requests/api/v1/stock_transfers_spec.rb new file mode 100644 index 0000000..69ef31f --- /dev/null +++ b/spec/requests/api/v1/stock_transfers_spec.rb @@ -0,0 +1,123 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Stock transfers 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(:product) { Product.create!(company: company, sku: 'A-001', name: 'Alpha') } + + def auth_headers(user) + post '/api/v1/auth/login', params: { email: user.email, password: 'password123' } + { 'Authorization' => "Bearer #{response.parsed_body['token']}" } + end + + def warehouse(name, zip) + Warehouse.create!(company: company, name: name, zip_code: zip, address: "Calle #{zip}") + end + + def origin = @origin ||= warehouse('Central', '1900') + def destination = @destination ||= warehouse('North', '1901') + + def body_for(quantity) + { stock_transfer: { product_id: product.id, origin_warehouse_id: origin.id, + destination_warehouse_id: destination.id, quantity: quantity } } + end + + # El tenant se fuerza a nil para que el fixture nazca SIEMPRE en la otra + # empresa: assign_current_company pisaría el company: manual si Current + # quedó seteado por un request previo. + def foreign_product + @foreign_product ||= Current.set(company_id: nil) do + other = Company.create!(name: 'Tenant B', tax_id: '30-22222222-2') + Product.create!(company: other, sku: 'B-001', name: 'Other') + end + end + + def dispatch_one(quantity: 4) + Stock.find_or_create_by!(product: product, warehouse: origin) { |s| s.quantity = 10 } + Catalog::DispatchTransfer.new(company: company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: quantity).call + end + + describe 'POST /api/v1/stock-transfers' do + before { Stock.create!(product: product, warehouse: origin, quantity: 10) } + + it 'returns 401 without a token' do + post '/api/v1/stock-transfers', params: body_for(4), as: :json + expect(response).to have_http_status(:unauthorized) + end + + it 'creates the transfer', :aggregate_failures do + post '/api/v1/stock-transfers', params: body_for(4), headers: headers, as: :json + + expect(response).to have_http_status(:created) + expect(response.parsed_body['status']).to eq('in_transit') + end + + it 'returns 422 when the origin cannot cover the quantity' do + post '/api/v1/stock-transfers', params: body_for(99), headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'returns 422 when both ends are the same warehouse' do + params = body_for(1).deep_merge(stock_transfer: { destination_warehouse_id: origin.id }) + post '/api/v1/stock-transfers', params: params, headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'returns 404 for a product of another company' do + params = body_for(1).deep_merge(stock_transfer: { product_id: foreign_product.id }) + post '/api/v1/stock-transfers', params: params, headers: headers, as: :json + + expect(response).to have_http_status(:not_found) + end + end + + describe 'GET /api/v1/stock-transfers' do + it 'lists the transfers of the company' do + dispatch_one + get '/api/v1/stock-transfers', headers: headers + + expect(response.parsed_body['data'].length).to eq(1) + end + + it 'filters by status' do + dispatch_one + get '/api/v1/stock-transfers', params: { status: 'received' }, headers: headers + + expect(response.parsed_body['data']).to be_empty + end + end + + describe 'POST /api/v1/stock-transfers/:id/receive' do + it 'settles the transfer into the destination', :aggregate_failures do + transfer = dispatch_one + post "/api/v1/stock-transfers/#{transfer.id}/receive", headers: headers + + expect(response).to have_http_status(:ok) + expect(Stock.find_by(product: product, warehouse: destination).quantity).to eq(4) + end + + it 'returns 409 when it is no longer in flight' do + transfer = dispatch_one + Catalog::SettleTransfer.new(transfer: transfer, outcome: :received).call + post "/api/v1/stock-transfers/#{transfer.id}/receive", headers: headers + + expect(response).to have_http_status(:conflict) + end + end + + describe 'POST /api/v1/stock-transfers/:id/cancel' do + it 'gives the units back to the origin', :aggregate_failures do + transfer = dispatch_one + post "/api/v1/stock-transfers/#{transfer.id}/cancel", headers: headers + + expect(response).to have_http_status(:ok) + expect(Stock.find_by(product: product, warehouse: origin).quantity).to eq(10) + end + end +end