From 4f4f1fefa405eaf2cfa9955f8920fdc2a5485d87 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Sun, 4 Oct 2026 17:06:04 -0300 Subject: [PATCH 1/5] feat: [TESIS-162] model stock reservations, pickup orders, catalog counts and recent activity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the system audit of 04/10. Half of the findings are screen-level but could not be fixed without data behind them, so this is the backend half. **Stock reservations.** The product detail showed "Comprometido" and "Disponible para prometer" as em dashes with a note saying the API does not model them. It does now, and nothing new had to be stored: DeductStock removes the units from `stocks` when the order is created, ConfirmDispatch never touches `stocks`, and `order_items.warehouse_id` has recorded the origin since TESIS-126. So what was sold and has not shipped is still on the shelf and can be derived: - committed: lines of non-cancelled orders whose shipment was not dispatched, by warehouse; - available to promise: `stocks.quantity`, because what was sold is already out of it; - on hand: the two together, which is what somebody counts if they walk in. A product nobody reserved answers 0, not null: the figure exists and is zero. Lines with no warehouse stay out of both the breakdown and the total, so the rows of the screen always add up to its header. **Pickup orders.** `orders.requires_shipping` makes the shipment optional, for manual sales and for channel ones. The template reads it from the payload when the channel reports it; when it does not, the order is assumed to be shipped — assuming a shipment that turns out unnecessary is visible on screen, assuming a pickup that was really a shipment is a sale nobody sends. CreateShipment refuses a pickup with its own error, separate from the cancelled one: a cancelled order is a dead end, a pickup is a healthy sale that does not ship. **Catalog counts.** `/inventory` fired four requests, one per tab, each reading `meta.total` of a page of one row. `GET /products/counts` answers the four in one call, honouring the same search and category filter as the listing. It is its own endpoint and not part of the listing's `meta` because the counts do not change when paging. **Recent activity.** `GET /activity` feeds the bell: orders created, shipments dispatched and events that fell to the retry queue, in one list ordered by date. No notifications table and no per-user read state — that is another domain and nobody asked for it. Derived from what the system already records, so it cannot drift from the facts. Also: `warehouses.capacity` for the occupancy bar, which cannot be derived from any other data, and `packaging` / `technical_standard` on products. Seeds carry a pickup order, a warehouse with its capacity declared and one without, so both paths can be seen. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- app/controllers/api/v1/activity_controller.rb | 21 ++ app/controllers/api/v1/orders_controller.rb | 3 +- app/controllers/api/v1/products_controller.rb | 32 ++- .../api/v1/shipments_controller.rb | 1 + .../api/v1/warehouses_controller.rb | 2 +- app/models/order.rb | 5 + app/models/product.rb | 52 +++++ app/models/warehouse.rb | 5 + app/policies/activity_policy.rb | 10 + app/poros/activity/build_feed.rb | 103 +++++++++ app/poros/orders/process_webhook_order.rb | 19 +- app/poros/orders/translate_webhook_payload.rb | 2 +- app/poros/shipments/create_shipment.rb | 1 + app/poros/shipments/pickup_order_error.rb | 23 ++ app/serializers/order_list_serializer.rb | 3 +- app/serializers/order_serializer.rb | 3 +- app/serializers/product_serializer.rb | 14 +- app/serializers/warehouse_serializer.rb | 4 +- config/routes.rb | 11 + ...261004194406_add_capacity_to_warehouses.rb | 14 ++ ...ging_and_technical_standard_to_products.rb | 15 ++ ...4194416_add_requires_shipping_to_orders.rb | 15 ++ db/schema.rb | 6 +- db/seeds.rb | 45 +++- ...R-015-convencion-de-respuesta-de-la-api.md | 2 + docs/guidelines/architecture.md | 2 +- spec/models/product_spec.rb | 96 ++++++++ .../orders/process_webhook_order_spec.rb | 65 ++++++ spec/requests/api/v1/activity_spec.rb | 208 ++++++++++++++++++ spec/requests/api/v1/api_contract_spec.rb | 16 +- spec/requests/api/v1/orders_spec.rb | 18 ++ spec/requests/api/v1/products_spec.rb | 132 +++++++++++ spec/requests/api/v1/shipments_spec.rb | 20 ++ spec/requests/api/v1/warehouses_spec.rb | 37 ++++ 34 files changed, 984 insertions(+), 21 deletions(-) create mode 100644 app/controllers/api/v1/activity_controller.rb create mode 100644 app/policies/activity_policy.rb create mode 100644 app/poros/activity/build_feed.rb create mode 100644 app/poros/shipments/pickup_order_error.rb create mode 100644 db/migrate/20261004194406_add_capacity_to_warehouses.rb create mode 100644 db/migrate/20261004194411_add_packaging_and_technical_standard_to_products.rb create mode 100644 db/migrate/20261004194416_add_requires_shipping_to_orders.rb create mode 100644 spec/requests/api/v1/activity_spec.rb diff --git a/app/controllers/api/v1/activity_controller.rb b/app/controllers/api/v1/activity_controller.rb new file mode 100644 index 00000000..9c594e0d --- /dev/null +++ b/app/controllers/api/v1/activity_controller.rb @@ -0,0 +1,21 @@ +# frozen_string_literal: true + +module Api + module V1 + # Lo último que pasó en la empresa, para el panel de la campanita + # (TESIS-162). Colección: viaja en `data` sin `meta`, porque no pagina —es + # un desplegable con un techo, no un listado (ADR-015). + class ActivityController < ApplicationController + def index + authorize :activity, :index? + # El feed no sale de un scope: son tres consultas a tres modelos, y los + # tres ya filtran por tenant —`Order` y `FailedEvent` con CompanyScoped, + # los eventos de envío por el join con `Shipment`—. No hay un + # `policy_scope` que aplicar sin inventar un modelo que no existe. + skip_policy_scope + + render json: { data: Activity::BuildFeed.new(limit: scalar_param(:limit)).call } + end + end + end +end diff --git a/app/controllers/api/v1/orders_controller.rb b/app/controllers/api/v1/orders_controller.rb index d6161fb5..6e6d5fff 100644 --- a/app/controllers/api/v1/orders_controller.rb +++ b/app/controllers/api/v1/orders_controller.rb @@ -18,7 +18,8 @@ class OrdersController < ApplicationController # Los datos del cliente que acepta el body. `items` queda afuera: se arma # aparte, línea por línea, en `items_params`. ORDER_FIELDS = %i[customer_name customer_document customer_address - customer_zip_code customer_city customer_province].freeze + customer_zip_code customer_city customer_province + requires_shipping].freeze # Campos sobre los que corre el buscador del listado (TESIS-52). Son las # formas en que un operador nombra una venta: el id con el que la conoce el diff --git a/app/controllers/api/v1/products_controller.rb b/app/controllers/api/v1/products_controller.rb index c8faae63..099e8fb9 100644 --- a/app/controllers/api/v1/products_controller.rb +++ b/app/controllers/api/v1/products_controller.rb @@ -8,7 +8,14 @@ class ProductsController < ApplicationController # Qué acepta el body del producto. `stocks` no entra: se arma aparte en # `stock_params`, línea por línea. - PRODUCT_FIELDS = %i[sku name description category weight dimensions].freeze + PRODUCT_FIELDS = %i[sku name description category packaging technical_standard + weight dimensions].freeze + + # Las pestañas del catálogo: cómo se llama cada una en la respuesta y con + # qué estado se filtra. «Todos» no filtra, por eso su estado es nil. El + # orden es el que muestra la pantalla. + CATALOG_TABS = { all: nil, available: 'available', low: 'low', + out_of_stock: 'out_of_stock' }.freeze before_action :set_product, only: %i[show update destroy] rescue_from ActiveRecord::RecordNotUnique, with: :render_conflict @@ -49,6 +56,17 @@ def categories render json: { data: Product::CATEGORIES } end + # Cuántos productos tiene cada pestaña del catálogo, respetando el mismo + # buscador y el mismo filtro de categoría que el listado: si no, el número + # de la pestaña y las filas que se ven dirían cosas distintas. + # + # Una consulta por estado y no cuatro requests: es lo que la pantalla + # hacía desde el cliente (TESIS-162), y cada una era un round trip. + def counts + skip_authorization + render json: { data: CATALOG_TABS.transform_values { |status| tab_count(status) } } + end + def create authorize Product @@ -83,6 +101,18 @@ def destroy # Las tres pestañas del catálogo más el buscador y la categoría. El orden # importa: `with_total_stock` arma el GROUP BY y `by_stock_status` cuelga # su HAVING de esa agregación. + # Cuántas filas matchean cada pestaña. `nil` es «Todos»: el scope sin + # filtro de estado. + def tab_count(status) + count_of( + policy_scope(Product) + .search_catalog(scalar_param(:search)) + .by_category(scalar_param(:category)) + .with_total_stock + .by_stock_status(status) + ) + end + def filtered_products policy_scope(Product) .search_catalog(scalar_param(:search)) diff --git a/app/controllers/api/v1/shipments_controller.rb b/app/controllers/api/v1/shipments_controller.rb index 6cba8a17..0af8e4bd 100644 --- a/app/controllers/api/v1/shipments_controller.rb +++ b/app/controllers/api/v1/shipments_controller.rb @@ -11,6 +11,7 @@ class ShipmentsController < ApplicationController before_action :set_shipment, only: %i[show] rescue_from Shipments::UnshippableOrderError, with: :render_unprocessable + rescue_from Shipments::PickupOrderError, with: :render_unprocessable rescue_from Shipments::DuplicateShipmentError, with: :render_conflict rescue_from Shipments::AlreadyDispatchedError, with: :render_conflict rescue_from Shipments::InvalidCourierIntegrationError, with: :render_unprocessable diff --git a/app/controllers/api/v1/warehouses_controller.rb b/app/controllers/api/v1/warehouses_controller.rb index f2187e84..5497feeb 100644 --- a/app/controllers/api/v1/warehouses_controller.rb +++ b/app/controllers/api/v1/warehouses_controller.rb @@ -66,7 +66,7 @@ def set_warehouse # silencio las claves que no permite. (El comentario anterior decía que # respondía 400; en Rails 8.1 no es así, y el spec de company_id lo fija.) def warehouse_params - params.expect(warehouse: %i[name zip_code address]) + params.expect(warehouse: %i[name zip_code address capacity]) end # Tres cosas bloquean el borrado y el mensaje tiene que decir cuál. Se diff --git a/app/models/order.rb b/app/models/order.rb index 4a65a8cb..6e166fd3 100644 --- a/app/models/order.rb +++ b/app/models/order.rb @@ -5,6 +5,11 @@ class Order < ApplicationRecord STATUSES = %w[pending paid cancelled].freeze + # El estado final de una venta que no va a salir. Con nombre porque son varios + # los lugares que lo preguntan —el alta del envío, el despacho, el cálculo de + # lo comprometido— y un literal repetido en cada uno es un typo esperando. + CANCELLED = 'cancelled' + # Las 24 jurisdicciones de la Argentina, con el nombre con el que se muestran # en el select de provincia del alta manual (TESIS-58). No es una lista # citable como nombres oficiales: Tierra del Fuego, por ejemplo, se llama diff --git a/app/models/product.rb b/app/models/product.rb index 9a353914..811e1d91 100644 --- a/app/models/product.rb +++ b/app/models/product.rb @@ -24,6 +24,12 @@ class Product < ApplicationRecord # Los tres estados de disponibilidad, en el vocabulario de la pantalla. STOCK_STATUSES = %w[out_of_stock low available].freeze + # Un envío en este estado todavía no salió, así que sus unidades siguen + # físicamente en el depósito aunque ya estén vendidas. Una orden sin envío + # —todavía no se abrió, o es un retiro en el local— está en la misma + # situación, y por eso el filtro acepta también el NULL del LEFT JOIN. + UNDISPATCHED_SHIPMENT_STATUSES = [nil, 'pending'].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 @@ -129,6 +135,40 @@ def in_transit_quantity stock_transfers.in_flight.sum(:quantity) end + # Unidades vendidas que todavía no salieron del depósito, por depósito. + # + # El stock se descuenta al **crear** la orden (`Catalog::DeductStock`) y el + # despacho no vuelve a tocar `stocks`, así que estas unidades ya no figuran en + # ninguna fila de stock pero siguen estando en el estante hasta que el courier + # se las lleva. Son las que el detalle muestra como «Comprometido». + # + # Las líneas sin depósito (anteriores a TESIS-126) quedan afuera: no se pueden + # atribuir a ninguno, y contarlas en el total pero en ningún depósito dejaría + # una pantalla cuyas filas no suman el encabezado. + def committed_by_warehouse + @committed_by_warehouse ||= committed_scope + .group(:warehouse_id, 'warehouses.name') + .order(:warehouse_id) + .sum(:quantity) + .map do |(warehouse_id, name), quantity| + { warehouse_id: warehouse_id, name: name, quantity: quantity.to_i } + end + end + + # Lo comprometido de todo el producto. Suma el desglose en vez de volver a la + # base: es el mismo número por definición, y así no puede discrepar con las + # filas que muestra la pantalla. + def committed_quantity + committed_by_warehouse.sum { |row| row[:quantity] } + end + + # Lo que hay físicamente: lo que queda libre más lo vendido sin despachar. + def on_hand_quantity = total_stock + committed_quantity + + # Lo que se puede prometer es lo que queda libre: lo vendido ya se descontó de + # `stocks` al crear la orden, así que no hay que volver a restarlo. + def available_to_promise = total_stock + # 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. # @@ -144,4 +184,16 @@ def primary_stock stocks.reject { |stock| stock.quantity.zero? } .min_by { |stock| [-stock.quantity, stock.warehouse_id] } end + + private + + # Las líneas que cuentan como comprometidas. El LEFT JOIN con `shipments` es + # lo que deja entrar a las órdenes que todavía no tienen envío; un INNER las + # dejaría afuera, que es justo el caso más común apenas entra una venta. + def committed_scope + order_items.joins(:order, :warehouse) + .left_outer_joins(order: :shipment) + .where.not(orders: { status: Order::CANCELLED }) + .where(shipments: { status: UNDISPATCHED_SHIPMENT_STATUSES }) + end end diff --git a/app/models/warehouse.rb b/app/models/warehouse.rb index fdd6eef5..c2487233 100644 --- a/app/models/warehouse.rb +++ b/app/models/warehouse.rb @@ -25,6 +25,11 @@ class Warehouse < ApplicationRecord dependent: :restrict_with_error validates :name, presence: true + # Capacidad declarada del depósito, en unidades. Opcional: un depósito sin + # capacidad cargada no es un error, es uno del que nadie declaró el techo + # todavía, y la pantalla no dibuja su barra de ocupación. Mayor que cero + # porque un depósito de capacidad cero no podría guardar nada. + validates :capacity, numericality: { only_integer: true, greater_than: 0 }, allow_nil: true validates :zip_code, presence: true validates :address, presence: true diff --git a/app/policies/activity_policy.rb b/app/policies/activity_policy.rb new file mode 100644 index 00000000..0f0ab9d6 --- /dev/null +++ b/app/policies/activity_policy.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +# El feed no tiene modelo propio: se arma con órdenes, eventos de envío y +# eventos fallidos, y los tres ya están scopeados por tenant. La policy existe +# igual para que el controller no tenga que saltear la verificación de Pundit +# —`verify_authorized` está activo en toda la API— y para que quede un lugar +# donde apretar el permiso si algún día el feed deja de ser para cualquiera. +class ActivityPolicy < ApplicationPolicy + def index? = user.present? +end diff --git a/app/poros/activity/build_feed.rb b/app/poros/activity/build_feed.rb new file mode 100644 index 00000000..41da8ae6 --- /dev/null +++ b/app/poros/activity/build_feed.rb @@ -0,0 +1,103 @@ +# frozen_string_literal: true + +module Activity + # Lo último que pasó en la empresa, en una sola lista ordenada por fecha: es lo + # que muestra el panel de la campanita (TESIS-162) y el «feed de actividad + # reciente» que RF-26 compromete para el panel de control. + # + # No hay tabla de notificaciones ni estado de leído por usuario a propósito: + # eso es otro dominio —leídas por persona, vencimiento, preferencias— y nadie + # lo pidió. Acá la actividad se **deriva** de lo que el sistema ya registra, + # así que no puede desincronizarse de los hechos ni hay nada que mantener. + # + # Un endpoint y no tres: cada fuente es una consulta, pero la pantalla hace un + # solo request y recibe la lista ya ordenada. Pedir tres y mezclarlas en el + # cliente obligaría a que el front supiera cómo se ordena una actividad. + class BuildFeed < ApplicationPoro + DEFAULT_LIMIT = 20 + # Techo del `limit`: es un panel desplegable, no un listado paginado. Sin + # tope, un `?limit=` grande haría tres consultas sin límite. + MAX_LIMIT = 50 + + # Las ventas que entraron, los envíos que salieron y lo que se cayó a la + # cola de reintentos. Son las tres cosas sobre las que el operador actúa. + TYPES = %w[order_created shipment_dispatched event_failed].freeze + + DISPATCH_STATUS = Shipments::ConfirmDispatch::DISPATCHED_STATUS + + def initialize(limit: nil) + super() + @limit = normalize_limit(limit) + end + + # Se traen las últimas `limit` de cada fuente y recién después se mezclan: + # cualquiera de las tres podría ser toda la lista, así que ninguna puede + # traer menos. + def call + (recent_orders + recent_dispatches + recent_failures) + .sort_by { |event| [-event[:occurred_at].to_f, event[:id]] } + .first(@limit) + end + + private + + # Un `limit` que no es un número vuelve al default y no a uno: `'muchas'.to_i` + # es 0, y acotarlo devolvería una sola fila, que es una respuesta rara para + # un parámetro que nadie escribió a propósito. Lo que sí es un número se + # acota, con el mismo criterio que la paginación (TESIS-155). + def normalize_limit(limit) + value = limit.to_s.strip + return DEFAULT_LIMIT unless value.match?(/\A\d+\z/) + + value.to_i.clamp(1, MAX_LIMIT) + end + + # `Order` es CompanyScoped: el feed es del tenant del request sin filtrar a + # mano, igual que el resto de la API. + def recent_orders + Order.order(created_at: :desc, id: :desc).limit(@limit).map do |order| + { + id: "order-#{order.id}", type: 'order_created', occurred_at: order.created_at, + order_id: order.id, customer_name: order.customer_name, + external_order_id: order.external_order_id, + total_amount: order.total_amount&.to_f + } + end + end + + # El despacho no deja una marca de tiempo propia en `shipments`: lo dice el + # primer evento `ready_to_ship` de la bitácora, que es el que escribe + # `ConfirmDispatch`. Mismo criterio que usan los reportes. + # + # `ShipmentEvent` no es CompanyScoped —no tiene company_id— así que el + # aislamiento entra por el join con `Shipment`, que sí lo es. + def recent_dispatches + ShipmentEvent.where(internal_status: DISPATCH_STATUS) + .joins(:shipment).merge(Shipment.all) + .includes(shipment: { company_integration: :service }) + .order(occurred_at: :desc, id: :desc).limit(@limit) + .map { |event| dispatch_entry(event, event.shipment) } + end + + def dispatch_entry(event, shipment) + { + id: "dispatch-#{event.id}", type: 'shipment_dispatched', occurred_at: event.occurred_at, + shipment_id: shipment.id, order_id: shipment.order_id, + tracking_number: shipment.tracking_number, + courier: shipment.company_integration&.service_name + } + end + + # Igual que las órdenes: `FailedEvent` es CompanyScoped. + def recent_failures + FailedEvent.includes(company_integration: :service) + .order(created_at: :desc, id: :desc).limit(@limit).map do |failure| + { + id: "failure-#{failure.id}", type: 'event_failed', occurred_at: failure.created_at, + failed_event_id: failure.id, event_type: failure.event_type, + status: failure.status, integration: failure.company_integration&.service_name + } + end + end + end +end diff --git a/app/poros/orders/process_webhook_order.rb b/app/poros/orders/process_webhook_order.rb index 49a79b54..7fb769d7 100644 --- a/app/poros/orders/process_webhook_order.rb +++ b/app/poros/orders/process_webhook_order.rb @@ -127,7 +127,24 @@ def order_attributes .slice(:external_order_id, :customer_name, :customer_document, :customer_address, :customer_zip_code, :customer_city) .merge(company_id: @log.company_id, company_integration: @log.company_integration, - status: status, customer_province: province) + status: status, customer_province: province, + requires_shipping: requires_shipping?) + end + + # Si la venta se despacha o la retira el cliente (TESIS-162). Lo informa la + # plantilla del canal cuando el payload lo trae —Shopify lo manda, Mercado + # Libre no siempre—; cuando no dice nada, se asume que hay envío. + # + # El default es `true` y no `false` a propósito: asumir envío y que sobre + # deja una orden lista para despachar que nadie despacha, y eso se ve en la + # pantalla. Asumir retiro y que falte deja una venta que había que mandar + # sin ninguna señal de que falta hacerlo, y eso no se ve hasta que reclama + # el comprador. + def requires_shipping? + value = translated[:order][:requires_shipping] + return true if value.nil? + + ActiveModel::Type::Boolean.new.cast(value) != false end # La provincia se valida contra Order::PROVINCES, y cada canal la escribe a diff --git a/app/poros/orders/translate_webhook_payload.rb b/app/poros/orders/translate_webhook_payload.rb index e780c88c..f7edd409 100644 --- a/app/poros/orders/translate_webhook_payload.rb +++ b/app/poros/orders/translate_webhook_payload.rb @@ -14,7 +14,7 @@ class TranslateWebhookPayload < ApplicationPoro # flujos (ej. tracking_number en la respuesta de un courier). ORDER_KEYS = %w[external_order_id customer_name customer_document customer_address customer_zip_code customer_city - customer_province status].freeze + customer_province status requires_shipping].freeze ITEM_KEYS = %w[external_product_id quantity unit_price].freeze def initialize(service:, payload:) diff --git a/app/poros/shipments/create_shipment.rb b/app/poros/shipments/create_shipment.rb index 003d708b..b0552b29 100644 --- a/app/poros/shipments/create_shipment.rb +++ b/app/poros/shipments/create_shipment.rb @@ -39,6 +39,7 @@ def call private def validate_status! + raise PickupOrderError.new(order: @order) unless @order.requires_shipping? return unless NON_SHIPPABLE_STATUSES.include?(@order.status) raise UnshippableOrderError.new(order: @order) diff --git a/app/poros/shipments/pickup_order_error.rb b/app/poros/shipments/pickup_order_error.rb new file mode 100644 index 00000000..f9950826 --- /dev/null +++ b/app/poros/shipments/pickup_order_error.rb @@ -0,0 +1,23 @@ +# frozen_string_literal: true + +module Shipments + # La orden la retira el cliente en el local, así que no entra al circuito + # logístico: no hay envío que abrir ni etiqueta que emitir (TESIS-162). + # + # Es un error de datos y no transitorio, igual que `UnshippableOrderError`: + # reintentar no cambia nada mientras la orden siga siendo un retiro. El + # controller lo mapea a 422. + # + # Va aparte y no como otro estado de `UnshippableOrderError` porque el motivo + # es distinto y la pantalla hace cosas distintas con cada uno: una orden + # cancelada es un callejón sin salida, un retiro es una venta sana que + # simplemente no se despacha. + class PickupOrderError < StandardError + attr_reader :order_id + + def initialize(order:) + @order_id = order.id + super('an order picked up at the store has no shipment') + end + end +end diff --git a/app/serializers/order_list_serializer.rb b/app/serializers/order_list_serializer.rb index e9594d8f..bc33c49b 100644 --- a/app/serializers/order_list_serializer.rb +++ b/app/serializers/order_list_serializer.rb @@ -10,7 +10,8 @@ class OrderListSerializer < ApplicationSerializer # vienen desde el principio; la ciudad y la provincia desde TESIS-128, así que # las órdenes anteriores y las de webhook las traen en null. fields :customer_name, :customer_document, :customer_address, :customer_zip_code, - :customer_city, :customer_province, :external_order_id, :status, :created_at, :updated_at + :customer_city, :customer_province, :external_order_id, :status, :requires_shipping, + :created_at, :updated_at # La columna Total del listado (TESIS-52). Sale de la columna persistida y no # de sumar las líneas: sumarlas por fila sería un SELECT por orden, y además diff --git a/app/serializers/order_serializer.rb b/app/serializers/order_serializer.rb index a44b895a..2de3167f 100644 --- a/app/serializers/order_serializer.rb +++ b/app/serializers/order_serializer.rb @@ -4,7 +4,8 @@ class OrderSerializer < ApplicationSerializer identifier :id fields :customer_name, :customer_document, :customer_address, :customer_zip_code, - :customer_city, :customer_province, :external_order_id, :status, :created_at, :updated_at + :customer_city, :customer_province, :external_order_id, :status, :requires_shipping, + :created_at, :updated_at # Mismo criterio que unit_price: decimal en la DB, número en el JSON. Puede # ser nil en órdenes anteriores a TESIS-114 que no tienen líneas. diff --git a/app/serializers/product_serializer.rb b/app/serializers/product_serializer.rb index 694a8c34..9fb361b2 100644 --- a/app/serializers/product_serializer.rb +++ b/app/serializers/product_serializer.rb @@ -3,8 +3,18 @@ class ProductSerializer < ApplicationSerializer identifier :id - fields :sku, :name, :description, :category, :dimensions, :total_stock, - :in_transit_quantity, :created_at, :updated_at + fields :sku, :name, :description, :category, :packaging, :technical_standard, + :dimensions, :total_stock, :in_transit_quantity, :created_at, :updated_at + + # Los tres números del stock que muestra el detalle (TESIS-162). Van los tres + # desde el backend y ninguno se deriva en el cliente: `on_hand` no es + # `total_stock` —lo vendido sin despachar sigue en el estante pero ya salió de + # `stocks`— y restarlos mal del lado del front es el error que esta card viene + # a cerrar. + fields :committed_quantity, :on_hand_quantity, :available_to_promise + + # Lo comprometido por depósito, con la misma forma que `in_transit_by_warehouse`. + field :committed_by_warehouse # 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/warehouse_serializer.rb b/app/serializers/warehouse_serializer.rb index 0aff6732..9aeac199 100644 --- a/app/serializers/warehouse_serializer.rb +++ b/app/serializers/warehouse_serializer.rb @@ -2,7 +2,9 @@ class WarehouseSerializer < ApplicationSerializer identifier :id - fields :name, :zip_code, :address + # `capacity` puede ser null y eso significa algo: nadie declaró el techo de + # este depósito todavía. No es cero, que diría que no entra nada. + fields :name, :zip_code, :address, :capacity # Unidades guardadas en el deposito. Alimenta el widget de capacidad del panel # (TESIS-55): la barra compara depositos entre si, no contra una capacidad diff --git a/config/routes.rb b/config/routes.rb index 19fced7c..c98cdf8a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -27,6 +27,12 @@ # Vocabulario de categorías: ruta de colección, no depende de un producto. get :categories, on: :collection + # Cuántos productos cae en cada pestaña del catálogo, en una sola + # respuesta (TESIS-162). Ruta propia y no dentro del `meta` del listado: + # los contadores no cambian al pasar de página, así que el cliente los + # pide una vez y los invalida recién cuando algo muta. + get :counts, on: :collection + resources :mappings, only: %i[index create destroy], controller: 'product_mappings' end @@ -66,6 +72,11 @@ post :dispatch, on: :member, action: :confirm end + # Actividad reciente de la empresa (TESIS-162): la lista que abre la + # campanita. Recurso singular en plural por costumbre REST, pero sin id: + # es una vista agregada, no una colección de filas propias. + resources :activity, only: %i[index] + resources :failed_events, path: 'failed-events', only: %i[index] do member do post :retry, action: :requeue diff --git a/db/migrate/20261004194406_add_capacity_to_warehouses.rb b/db/migrate/20261004194406_add_capacity_to_warehouses.rb new file mode 100644 index 00000000..c31b032d --- /dev/null +++ b/db/migrate/20261004194406_add_capacity_to_warehouses.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +# Capacidad del depósito, en unidades. La barra de ocupación del detalle de +# producto compara lo guardado contra un techo, y ese techo no se puede derivar +# de ningún dato: lo sabe quien conoce el depósito. +# +# Nullable a propósito: los depósitos que ya existen no tienen una capacidad que +# alguien haya declarado, y poner un número por defecto sería inventarlo. Sin +# capacidad cargada, la pantalla no dibuja la barra (TESIS-163). +class AddCapacityToWarehouses < ActiveRecord::Migration[8.1] + def change + add_column :warehouses, :capacity, :integer + end +end diff --git a/db/migrate/20261004194411_add_packaging_and_technical_standard_to_products.rb b/db/migrate/20261004194411_add_packaging_and_technical_standard_to_products.rb new file mode 100644 index 00000000..8ab0eed8 --- /dev/null +++ b/db/migrate/20261004194411_add_packaging_and_technical_standard_to_products.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +# Empaque y norma técnica del producto. El detalle los muestra desde el diseño +# y hasta ahora decía que no existían en la API. +# +# Texto libre y no un vocabulario cerrado como `category`: una norma técnica es +# un código de un organismo externo (IRAM, IEC) y el empaque describe el bulto +# con las palabras del rubro. Encerrarlos en una lista nuestra dejaría afuera el +# caso que importa el día que aparezca. +class AddPackagingAndTechnicalStandardToProducts < ActiveRecord::Migration[8.1] + def change + add_column :products, :packaging, :string + add_column :products, :technical_standard, :string + end +end diff --git a/db/migrate/20261004194416_add_requires_shipping_to_orders.rb b/db/migrate/20261004194416_add_requires_shipping_to_orders.rb new file mode 100644 index 00000000..11667070 --- /dev/null +++ b/db/migrate/20261004194416_add_requires_shipping_to_orders.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +# Si la orden se despacha o la retira el cliente. Hasta ahora toda orden se +# asumía despachada, y no había forma de registrar una venta con retiro en el +# local (TESIS-162). +# +# `default: true` y `null: false`: las órdenes que ya existen se despachan —es +# lo único que el sistema sabía hacer— y el default cubre el caso en que el +# canal no informa nada. Asumir envío y que sobre es recuperable; asumir retiro +# y que falte deja una venta sin despachar sin que nadie se entere. +class AddRequiresShippingToOrders < ActiveRecord::Migration[8.1] + def change + add_column :orders, :requires_shipping, :boolean, default: true, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 95ee4433..b96361bf 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_10_01_211235) do +ActiveRecord::Schema[8.1].define(version: 2026_10_04_194416) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" @@ -108,6 +108,7 @@ t.string "customer_province" t.string "customer_zip_code" t.string "external_order_id" + t.boolean "requires_shipping", default: true, null: false t.string "status", default: "pending", null: false t.decimal "total_amount", precision: 10, scale: 2 t.datetime "updated_at", null: false @@ -139,7 +140,9 @@ t.text "description" t.string "dimensions" t.string "name", null: false + t.string "packaging" t.string "sku", null: false + t.string "technical_standard" t.datetime "updated_at", null: false t.decimal "weight", precision: 10, scale: 2, default: "0.0" t.index ["company_id", "category"], name: "index_products_on_company_id_and_category" @@ -259,6 +262,7 @@ create_table "warehouses", force: :cascade do |t| t.string "address", null: false + t.integer "capacity" t.bigint "company_id", null: false t.datetime "created_at", null: false t.string "name", null: false diff --git a/db/seeds.rb b/db/seeds.rb index 0628d761..b84fb6a9 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -35,7 +35,12 @@ { email: 'operador@norte.com', password: 'password123' } ], warehouses: [ - { name: 'Depósito Central', zip_code: '1900', address: 'Av. 7 N° 1234, La Plata' }, + # `capacity` es la capacidad declarada (TESIS-162): la barra de ocupación + # del detalle de producto compara lo guardado contra este techo. El + # satélite va sin declarar a propósito, para que se vea el caso en que la + # pantalla no dibuja la barra porque nadie cargó el número. + { name: 'Depósito Central', zip_code: '1900', address: 'Av. 7 N° 1234, La Plata', + capacity: 6_000 }, { name: 'Depósito Satélite Norte', zip_code: '1602', address: 'Calle 25 N° 456, Florida' } ] }, @@ -119,10 +124,17 @@ end attrs[:warehouses].each do |warehouse_attrs| - Warehouse.find_or_create_by!(name: warehouse_attrs[:name], company: company) do |w| + warehouse = Warehouse.find_or_create_by!(name: warehouse_attrs[:name], company: company) do |w| w.zip_code = warehouse_attrs[:zip_code] w.address = warehouse_attrs[:address] end + + # Fuera del bloque de creación, por el mismo motivo que la categoría de los + # productos: así las bases ya sembradas antes de que existiera la columna + # también quedan con capacidad. El `if` no pisa una cargada a mano. + if warehouse.capacity.nil? && warehouse_attrs[:capacity] + warehouse.update!(capacity: warehouse_attrs[:capacity]) + end end end @@ -573,6 +585,15 @@ { celular => 'Electronics', notebook => 'Electronics', mouse => 'Electronics' } .each { |product, category| product.update!(category: category) if product.category.nil? } + # Empaque y norma técnica (TESIS-162). Mismo criterio que la categoría: fuera + # del bloque de creación, y sólo si no están, para no pisar lo cargado a mano. + { celular => ['Caja individual', 'IRAM 4220'], + notebook => ['Caja con separadores x4', 'IEC 62368-1'], + mouse => ['Blíster x12', 'IRAM 2063'] }.each do |product, (packaging, standard)| + product.update!(packaging: packaging) if product.packaging.nil? + product.update!(technical_standard: standard) if product.technical_standard.nil? + end + # Stock en depósitos de Norte central = Warehouse.find_by(company: norte_company, name: 'Depósito Central') satelite = Warehouse.find_by(company: norte_company, name: 'Depósito Satélite Norte') @@ -794,6 +815,26 @@ end end +# Venta de Norte que el cliente retira en el local (TESIS-162): no entra al +# circuito logístico, así que el detalle no ofrece crearle un envío y la API +# rechaza el alta con 422. Es el caso que distingue «esta orden no lleva envío» +# de «a esta orden le falta el envío», que hasta ahora se veían igual. +if norte_company && celular + retiro = Order.find_or_create_by!( + company: norte_company, external_order_id: nil, customer_name: 'Retiro en Mostrador' + ) do |o| + o.customer_document = '27-35123456-4' + o.status = 'paid' + o.requires_shipping = false + end + + OrderItem.find_or_create_by!(order: retiro, product: celular) do |i| + i.quantity = 1 + i.unit_price = 285_000.00 + i.warehouse = Warehouse.find_by(company: norte_company, name: 'Depósito Central') + end +end + if sur_company # Venta manual de Comercial Sur: cubre el caso borde de una orden sin # dirección de envío (retiro en sucursal) y con status 'cancelled'. diff --git a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md index 0e2354f7..0602b805 100644 --- a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md +++ b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md @@ -45,6 +45,8 @@ cualquier error → { "error": "..." } | Respuesta | Por qué no pagina | | --- | --- | | `GET /orders/provinces`, `GET /products/categories` | Vocabularios fijos del dominio, no filas de una tabla: su tamaño lo fija el código, no los datos de la empresa | +| `GET /products/counts` | Los cuatro contadores de las pestañas del catálogo: un agregado de largo fijo, no una colección (TESIS-162) | +| `GET /activity` | La actividad reciente de la empresa, con un techo propio (`BuildFeed::MAX_LIMIT`): es un panel desplegable, no un listado que se pagine (TESIS-162) | | `POST /orders/:id/quotes` | El resultado de una acción —una cotización por courier configurado—, no una consulta | La distinción importa para el consumidor: leer `meta.total` en cualquiera de esas tres devuelve `undefined`. La regla corta es **si el largo lo decide la empresa, pagina; si lo decide el código, no**. diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index 9f516033..c7f2c5cd 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -145,7 +145,7 @@ POST /api/v1/products → { "id": 1, "sku": "...", ... } cualquier error → { "error": "..." } ``` -`meta` cuenta el scope **ya filtrado**, no la tabla entera, y lo lleva todo **listado de registros**: ninguno devuelve una cantidad ilimitada de filas. Los vocabularios fijos (`/orders/provinces`, `/products/categories`) y el resultado de una acción (`/orders/:id/quotes`) viajan en `data` sin `meta`, porque su largo lo decide el código y no los datos de la empresa (ver ADR-015). +`meta` cuenta el scope **ya filtrado**, no la tabla entera, y lo lleva todo **listado de registros**: ninguno devuelve una cantidad ilimitada de filas. Los vocabularios fijos (`/orders/provinces`, `/products/categories`), los agregados (`/products/counts`, `/activity`) y el resultado de una acción (`/orders/:id/quotes`) viajan en `data` sin `meta`, porque su largo lo decide el código y no los datos de la empresa (ver ADR-015). El cálculo vive en un solo lugar, el concern `Api::V1::Paginatable`, con el techo (`MAX_PER_PAGE = 100`) y los dos defaults: 20 para una pantalla paginada y 100 para los listados que el consumidor lee enteros —depósitos, mapeos, integraciones— y usa para llenar un select. `page` y `per_page` fuera de rango se acotan en vez de romper. diff --git a/spec/models/product_spec.rb b/spec/models/product_spec.rb index b673cb0c..87338a89 100644 --- a/spec/models/product_spec.rb +++ b/spec/models/product_spec.rb @@ -85,6 +85,102 @@ end end + + # TESIS-162: el stock se descuenta al CREAR la orden y el despacho no vuelve a + # tocar `stocks`, así que lo vendido y no despachado sigue en el estante + # aunque ya no figure en ninguna fila de stock. + describe 'the units already sold that did not leave yet' do + let(:central) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') + end + let(:north) do + Warehouse.create!(company: company, name: 'North', zip_code: '1901', address: 'Calle 2') + end + + before do + product.save! + Stock.create!(product: product, warehouse: central, quantity: 10) + end + + def sell(quantity, warehouse: central, status: 'paid', shipment_status: nil) + order = Order.create!(company: company, customer_name: 'Cliente', status: status) + OrderItem.create!(order: order, product: product, warehouse: warehouse, + quantity: quantity, unit_price: 100) + ship(order, shipment_status) unless shipment_status.nil? + order + end + + def ship(order, status) + Shipment.create!(company: company, order: order, status: status, + tracking_number: status == 'pending' ? nil : 'AND-1') + end + + it 'counts a sale that has no shipment yet' do + sell(3) + + expect(product.committed_quantity).to eq(3) + end + + it 'counts a sale whose shipment was opened but not dispatched' do + sell(2, shipment_status: 'pending') + + expect(product.committed_quantity).to eq(2) + end + + # Ya salió del depósito: deja de estar comprometida porque deja de estar. + it 'stops counting it once the shipment was dispatched' do + sell(4, shipment_status: 'ready_to_ship') + + expect(product.committed_quantity).to be_zero + end + + it 'ignores a cancelled order' do + sell(5, status: 'cancelled') + + expect(product.committed_quantity).to be_zero + end + + # Las anteriores a TESIS-126 no saben de qué depósito salieron: contarlas en + # el total pero en ningún depósito dejaría filas que no suman el encabezado. + it 'leaves out a line that does not record its warehouse' do + order = Order.create!(company: company, customer_name: 'Cliente', status: 'paid') + OrderItem.create!(order: order, product: product, quantity: 7, unit_price: 100) + + expect(product.committed_quantity).to be_zero + end + + it 'breaks it down by warehouse, with the name of each one' do + sell(3) && sell(1, warehouse: north) + + expect(product.committed_by_warehouse).to eq( + [{ warehouse_id: central.id, name: 'Central', quantity: 3 }, + { warehouse_id: north.id, name: 'North', quantity: 1 }] + ) + end + + # El encabezado del detalle muestra estos tres, y tienen que cerrar entre sí. + describe 'the three figures of the detail' do + before { sell(3) } + + it 'promises what is free' do + expect(product.available_to_promise).to eq(10) + end + + it 'commits what was sold' do + expect(product.committed_quantity).to eq(3) + end + + it 'holds both on the shelf' do + expect(product.on_hand_quantity).to eq(13) + end + end + + it 'promises zero and not nil for a product nobody reserved', :aggregate_failures do + expect(product.committed_quantity).to eq(0) + expect(product.committed_by_warehouse).to eq([]) + end + end + %i[sku name].each do |attribute| it "is invalid without #{attribute}" do product.public_send("#{attribute}=", nil) diff --git a/spec/poros/orders/process_webhook_order_spec.rb b/spec/poros/orders/process_webhook_order_spec.rb index 738d20d0..ca80a623 100644 --- a/spec/poros/orders/process_webhook_order_spec.rb +++ b/spec/poros/orders/process_webhook_order_spec.rb @@ -481,4 +481,69 @@ def payload_in(state) end end end + + # TESIS-162: el envío pasa a ser opcional, también para las ventas de canal. + # + # Se pisa `integration` con un `let` —el mismo patrón que usa el bloque de la + # provincia— para que `publish` mapee el producto contra la integración cuyo + # servicio trae la clave nueva en la plantilla. + describe 'whether the sale is shipped or picked up at the store' do + subject(:ingest) { described_class.new(webhook_log: create_log(body)).call } + + let(:integration) { CompanyIntegration.create!(company: company, service: pickup_service) } + let(:body) { order_payload(items: [line('MLA-1', 1, 100)]) } + + def pickup_service + Service.create!(service_name: "Canal #{SecureRandom.hex(4)}", type: 'ecommerce', + http_method: 'GET', uri: 'https://api.canal.test/orders', + response_mapper: order_mapper.merge( + 'delivery.requires_shipping' => 'requires_shipping' + ), + response_value_mapper: { 'pagado' => 'paid' }) + end + + before { publish('SKU-1', 'MLA-1', stock: 20) } + + # El default: asumir envío y que sobre deja una orden lista para despachar + # que se ve en la pantalla. Asumir retiro y que falte deja una venta que + # había que mandar, sin ninguna señal de que falta hacerlo. + context 'when the payload does not carry the flag' do + it 'assumes there is a shipment' do + expect(ingest.requires_shipping).to be(true) + end + end + + context 'when the channel reports a pickup' do + let(:body) do + order_payload(items: [line('MLA-1', 1, 100)]) + .merge('delivery' => { 'requires_shipping' => false }) + end + + it 'records it as a pickup' do + expect(ingest.requires_shipping).to be(false) + end + end + + context 'when the channel sends the flag as text' do + let(:body) do + order_payload(items: [line('MLA-1', 1, 100)]) + .merge('delivery' => { 'requires_shipping' => 'false' }) + end + + it 'reads it all the same' do + expect(ingest.requires_shipping).to be(false) + end + end + + # La plantilla mapea la clave pero el canal no la manda en esta venta. + context 'when the mapped key is missing from this payload' do + let(:body) do + order_payload(items: [line('MLA-1', 1, 100)]).merge('delivery' => {}) + end + + it 'falls back to a shipment' do + expect(ingest.requires_shipping).to be(true) + end + end + end end diff --git a/spec/requests/api/v1/activity_spec.rb b/spec/requests/api/v1/activity_spec.rb new file mode 100644 index 00000000..316b74c0 --- /dev/null +++ b/spec/requests/api/v1/activity_spec.rb @@ -0,0 +1,208 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# TESIS-162: lo que abre la campanita. No hay tabla de notificaciones: la +# actividad se deriva de lo que el sistema ya registra, así que no puede +# desincronizarse de los hechos. +RSpec.describe 'Activity 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) } + + def auth_headers(for_user) + post '/api/v1/auth/login', params: { email: for_user.email, password: 'password123' }, + headers: { 'X-Tenant-Slug' => for_user.company.slug } + { 'Authorization' => "Bearer #{response.parsed_body['token']}" } + end + + def create_order(owner: company, customer: 'Ana', at: Time.current, external: nil) + Current.set(company_id: owner.id) do + Order.create!(company: owner, customer_name: customer, status: 'paid', + external_order_id: external, created_at: at) + end + end + + def courier_integration(owner: company) + service = Service.create!(service_name: "Andreani #{SecureRandom.hex(4)}", type: 'courier', + http_method: 'POST', uri: 'https://api.andreani.test') + Current.set(company_id: owner.id) { CompanyIntegration.create!(company: owner, service: service) } + end + + def dispatch_order(order, at: Time.current, tracking: 'AND-1', owner: company) + Current.set(company_id: owner.id) do + shipment = Shipment.create!(company: owner, order: order, status: 'ready_to_ship', + tracking_number: tracking, + company_integration: courier_integration(owner: owner)) + ShipmentEvent.create!(shipment: shipment, internal_status: 'ready_to_ship', + external_status: 'Despachado', occurred_at: at) + shipment + end + end + + def fail_event(at: Time.current, owner: company) + Current.set(company_id: owner.id) do + FailedEvent.create!(company: owner, event_type: 'order_ingestion', direction: 'inbound', + payload: {}, created_at: at) + end + end + + def feed + get '/api/v1/activity', headers: headers + response.parsed_body['data'] + end + + it 'returns 401 without a token' do + get '/api/v1/activity' + expect(response).to have_http_status(:unauthorized) + end + + it 'answers with data and no meta: it is a panel, not a paginated listing' do + get '/api/v1/activity', headers: headers + + expect(response.parsed_body.keys).to eq(['data']) + end + + describe 'what it reports' do + it 'reports a sale that came in', :aggregate_failures do + order = create_order(customer: 'Distribuidora Sur', external: 'SHOP-99') + + entry = feed.first + expect(entry).to include('type' => 'order_created', 'order_id' => order.id, + 'customer_name' => 'Distribuidora Sur', + 'external_order_id' => 'SHOP-99') + end + + # Una venta cargada a mano no tiene id externo, y eso es lo que la distingue + # de una de canal en la pantalla. + it 'tells a manual sale apart by its missing external id' do + create_order + + expect(feed.first['external_order_id']).to be_nil + end + + # `shipments` no guarda cuándo se despachó: lo dice el primer evento + # `ready_to_ship` de la bitácora, igual que en los reportes. + it 'reports a dispatch with its courier and its tracking', :aggregate_failures do + shipment = dispatch_order(create_order, tracking: 'AND-500') + + entry = feed.find { |event| event['type'] == 'shipment_dispatched' } + expect(entry).to include('shipment_id' => shipment.id, 'order_id' => shipment.order_id, + 'tracking_number' => 'AND-500') + expect(entry['courier']).to start_with('Andreani') + end + + it 'reports an event that fell to the retry queue', :aggregate_failures do + failure = fail_event + + entry = feed.find { |event| event['type'] == 'event_failed' } + expect(entry).to include('failed_event_id' => failure.id, + 'event_type' => 'order_ingestion', 'status' => 'pending') + end + + + # Los tres datos que el feed deja en null cuando no hay: la orden sin total + # (las anteriores a TESIS-114 no tienen líneas con qué calcularlo), el envío + # sin courier y el evento fallido sin integración (los de la ingesta por + # webhook pueden no tener una todavía). Null y no un texto de relleno: la + # pantalla decide cómo se muestra lo que falta. + # Una orden sin líneas no tiene con qué calcular su total (TESIS-114). + it 'leaves the total of an order that has none in null' do + create_order + + expect(feed.first['total_amount']).to be_nil + end + + def dispatch_without_courier + Current.set(company_id: company.id) do + shipment = Shipment.create!(company: company, order: create_order, + status: 'ready_to_ship', tracking_number: 'SIN-COURIER') + ShipmentEvent.create!(shipment: shipment, internal_status: 'ready_to_ship', + external_status: 'Despachado', occurred_at: Time.current) + end + end + + it 'leaves the courier in null for a shipment that has none' do + dispatch_without_courier + + expect(feed.find { |event| event['type'] == 'shipment_dispatched' }['courier']).to be_nil + end + + it 'leaves the integration in null for a failure that has none' do + fail_event + + expect(feed.find { |event| event['type'] == 'event_failed' }['integration']).to be_nil + end + + it 'has nothing to report for a company where nothing happened' do + expect(feed).to eq([]) + end + end + + describe 'how it is ordered and bounded' do + it 'puts the most recent first, mixing the three sources', :aggregate_failures do + create_order(at: 3.days.ago) + dispatch_order(create_order(at: 5.days.ago), at: 1.day.ago) + fail_event(at: 2.days.ago) + + expect(feed.pluck('type')) + .to eq(%w[shipment_dispatched event_failed order_created order_created]) + end + + it 'defaults to the last twenty' do + 25.times { |i| create_order(at: i.minutes.ago) } + + expect(feed.size).to eq(Activity::BuildFeed::DEFAULT_LIMIT) + end + + it 'honours a smaller limit' do + 5.times { |i| create_order(at: i.minutes.ago) } + + get '/api/v1/activity', params: { limit: 2 }, headers: headers + + expect(response.parsed_body['data'].size).to eq(2) + end + + # Es un desplegable con techo, no un listado: sin tope, un `?limit=` grande + # haría tres consultas sin límite. + it 'caps a limit that asks for too much' do + 60.times { |i| create_order(at: i.minutes.ago) } + + get '/api/v1/activity', params: { limit: 1_000 }, headers: headers + + expect(response.parsed_body['data'].size).to eq(Activity::BuildFeed::MAX_LIMIT) + end + + it 'falls back to the default for a limit that is not a number' do + 3.times { |i| create_order(at: i.minutes.ago) } + + get '/api/v1/activity', params: { limit: 'muchas' }, headers: headers + + expect(response.parsed_body['data'].size).to eq(3) + end + end + + describe 'tenant isolation' do + let(:other_company) { Company.create!(name: 'Tenant B', tax_id: '30-22222222-2') } + + it 'leaves out the orders of another company' do + create_order(owner: other_company, customer: 'Ajena') + + expect(feed).to eq([]) + end + + # `ShipmentEvent` no tiene company_id: su aislamiento entra por el join con + # `Shipment`, así que vale la pena probarlo aparte. + it 'leaves out the dispatches of another company' do + dispatch_order(create_order(owner: other_company), owner: other_company) + + expect(feed).to eq([]) + end + + it 'leaves out the failed events of another company' do + fail_event(owner: other_company) + + expect(feed).to eq([]) + end + end +end diff --git a/spec/requests/api/v1/api_contract_spec.rb b/spec/requests/api/v1/api_contract_spec.rb index af16773f..57c8723c 100644 --- a/spec/requests/api/v1/api_contract_spec.rb +++ b/spec/requests/api/v1/api_contract_spec.rb @@ -35,16 +35,18 @@ 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], + producto: %w[id sku name description category packaging technical_standard weight + dimensions total_stock in_transit_quantity committed_quantity + on_hand_quantity available_to_promise committed_by_warehouse stocks + created_at updated_at], stock: %w[id quantity warehouse_id warehouse created_at updated_at], - deposito: %w[id name address zip_code stored_units], + deposito: %w[id name address zip_code capacity 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 - item_count created_at updated_at], + customer_zip_code customer_city customer_province status requires_shipping + courier total_amount item_count created_at updated_at], orden: %w[id external_order_id customer_name customer_document customer_address - customer_zip_code customer_city customer_province status total_amount order_items - created_at updated_at], + customer_zip_code customer_city customer_province status requires_shipping + total_amount order_items created_at updated_at], linea: %w[id product_id warehouse_id quantity unit_price product created_at updated_at], courier: %w[id service_id name], envio: %w[id order_id status tracking_number shipping_label_url shipping_cost courier events diff --git a/spec/requests/api/v1/orders_spec.rb b/spec/requests/api/v1/orders_spec.rb index 63f3ddb5..1b1802c6 100644 --- a/spec/requests/api/v1/orders_spec.rb +++ b/spec/requests/api/v1/orders_spec.rb @@ -396,6 +396,24 @@ def order_of_another_company expect(response).to have_http_status(:unprocessable_content) end + + # TESIS-162: hasta ahora toda orden se asumía despachada y no había forma + # de registrar una venta que el cliente retira en el local. + it 'is shipped when the body says nothing about it', :aggregate_failures do + post_order + + expect(response).to have_http_status(:created) + expect(response.parsed_body['requires_shipping']).to be(true) + end + + it 'records a pickup when the body asks for one', :aggregate_failures do + post_order(build_payload.deep_merge(order: { requires_shipping: false })) + + expect(response).to have_http_status(:created) + expect(response.parsed_body['requires_shipping']).to be(false) + expect(Order.last.requires_shipping).to be(false) + 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)])) diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 2c1a67a5..40f22b36 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -460,6 +460,78 @@ def row end end + + # TESIS-162: la pantalla pedía un request por pestaña, sólo para leer el total + # de cada una. + describe 'GET /api/v1/products/counts' do + let(:warehouse) do + Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') + end + + def stocked(sku, quantity, category: nil) + product = Product.create!(company: company, sku: sku, name: sku, category: category) + Stock.create!(product: product, warehouse: warehouse, quantity: quantity) + product + end + + # Uno de cada pestaña, para que los cuatro contadores digan algo distinto. + def one_product_per_tab + stocked('OUT-1', 0) + stocked('LOW-1', Product::LOW_STOCK_THRESHOLD) + stocked('OK-1', Product::LOW_STOCK_THRESHOLD + 1) + end + + it 'returns 401 without a token' do + get '/api/v1/products/counts' + expect(response).to have_http_status(:unauthorized) + end + + it 'counts every tab of the catalog in one answer', :aggregate_failures do + one_product_per_tab + + get '/api/v1/products/counts', headers: headers + + expect(response).to have_http_status(:ok) + expect(response.parsed_body['data']) + .to eq('all' => 3, 'available' => 1, 'low' => 1, 'out_of_stock' => 1) + end + + # Si los contadores ignoraran el buscador, el número de la pestaña y las + # filas que se ven dirían cosas distintas. + it 'honours the same search as the listing' do + stocked('ALPHA-1', 5) + stocked('BETA-1', 5) + + get '/api/v1/products/counts', params: { search: 'ALPHA' }, headers: headers + + expect(response.parsed_body['data']).to include('all' => 1, 'low' => 1) + end + + it 'honours the category filter too' do + stocked('CAT-1', 5, category: 'Cabling') + stocked('CAT-2', 5, category: 'Power') + + get '/api/v1/products/counts', params: { category: 'Cabling' }, headers: headers + + expect(response.parsed_body['data']).to include('all' => 1) + end + + it 'does not count the products of another company' do + other_product + + get '/api/v1/products/counts', headers: headers + + expect(response.parsed_body['data']['all']).to be_zero + end + + # Vocabulario, no colección paginada: `data` sola (ADR-015). + it 'answers without a meta envelope' do + get '/api/v1/products/counts', headers: headers + + expect(response.parsed_body.keys).to eq(['data']) + end + end + describe 'GET /api/v1/products/:id' do let!(:product) do wh = Warehouse.create!(company: company, name: 'Central', zip_code: '1900', address: 'Calle 1') @@ -479,6 +551,66 @@ def row expect(response.parsed_body['total_stock']).to eq(7) end + + # TESIS-162: los tres números del detalle dejan de ser «—» y pasan a ser + # dato. Se exponen desde acá y no se derivan en el cliente: `on_hand` no es + # `total_stock`, y restarlos mal del lado del front era el riesgo. + describe 'the three stock figures' do + def sell(quantity) + warehouse = Warehouse.find_by(name: 'Central') + order = Order.create!(company: company, customer_name: 'Cliente', status: 'paid') + OrderItem.create!(order: order, product: product, warehouse: warehouse, + quantity: quantity, unit_price: 100) + end + + it 'answers zero and not null for a product nobody reserved', :aggregate_failures do + get "/api/v1/products/#{product.id}", headers: headers + + expect(response.parsed_body) + .to include('committed_quantity' => 0, 'committed_by_warehouse' => [], + 'available_to_promise' => 7, 'on_hand_quantity' => 7) + end + + it 'separates what is free from what is on the shelf' do + sell(2) + + get "/api/v1/products/#{product.id}", headers: headers + + expect(response.parsed_body) + .to include('committed_quantity' => 2, 'available_to_promise' => 7, + 'on_hand_quantity' => 9) + end + + it 'breaks the commitment down by warehouse' do + sell(3) + + get "/api/v1/products/#{product.id}", headers: headers + + expect(response.parsed_body['committed_by_warehouse'].first) + .to include('name' => 'Central', 'quantity' => 3) + end + + # Una consulta agregada y no una por depósito. + it 'does not add a query per warehouse' do + add_extra_stocks(product) && sell(1) + + queries = count_queries(matching: /FROM "order_items"/) do + get "/api/v1/products/#{product.id}", headers: headers + end + + expect(queries).to eq(1) + end + end + + it 'returns the packaging and the technical standard', :aggregate_failures do + product.update!(packaging: 'Caja x12', technical_standard: 'IRAM 2063') + + get "/api/v1/products/#{product.id}", headers: headers + + expect(response.parsed_body['packaging']).to eq('Caja x12') + expect(response.parsed_body['technical_standard']).to eq('IRAM 2063') + end + it 'returns 404 for a product from another company', :aggregate_failures do expect(other_product.company_id).to eq(other_company.id) get "/api/v1/products/#{other_product.id}", headers: headers diff --git a/spec/requests/api/v1/shipments_spec.rb b/spec/requests/api/v1/shipments_spec.rb index c9621b24..5c955fa7 100644 --- a/spec/requests/api/v1/shipments_spec.rb +++ b/spec/requests/api/v1/shipments_spec.rb @@ -303,6 +303,26 @@ def create_shipment(order_id, auth: headers) post "/api/v1/orders/#{order_id}/shipment", headers: auth, as: :json end + # TESIS-162: una venta con retiro en el local no entra al circuito + # logístico. Es un 422 propio y no el de la orden cancelada: una cancelada + # es un callejón sin salida, un retiro es una venta sana que no se despacha. + context 'when the customer picks the order up at the store' do + let(:pickup) do + order_with('paid').tap { |o| o.update!(requires_shipping: false) } + end + + it 'refuses to open a shipment', :aggregate_failures do + expect { create_shipment(pickup.id) }.not_to change(Shipment, :count) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to include('picked up') + end + + it 'still opens one for an order that is shipped' do + expect { create_shipment(order_with('paid').id) }.to change(Shipment, :count).by(1) + end + end + # Mismo motivo que foreign_shipment: assign_current_company pisa el company: # manual si Current.company_id quedó seteado por un request previo. def foreign_order diff --git a/spec/requests/api/v1/warehouses_spec.rb b/spec/requests/api/v1/warehouses_spec.rb index 6e9594a2..720c823c 100644 --- a/spec/requests/api/v1/warehouses_spec.rb +++ b/spec/requests/api/v1/warehouses_spec.rb @@ -184,6 +184,43 @@ def stock_queries_while(&) expect(response).to have_http_status(:created) end + + # TESIS-162: la barra de ocupación del detalle de producto compara lo + # guardado contra un techo que nadie puede derivar de otro dato. + describe 'the declared capacity' do + it 'is saved when it comes in the body', :aggregate_failures do + post '/api/v1/warehouses', params: { warehouse: warehouse_attrs.merge(capacity: 6000) }, + headers: headers, as: :json + + expect(response).to have_http_status(:created) + expect(response.parsed_body['capacity']).to eq(6000) + end + + # Null y no cero: nadie declaró el techo todavía, que no es lo mismo que + # decir que no entra nada. + it 'stays null when nobody declared it' do + post '/api/v1/warehouses', params: { warehouse: warehouse_attrs }, + headers: headers, as: :json + + expect(response.parsed_body['capacity']).to be_nil + end + + it 'refuses a capacity that could not hold anything', :aggregate_failures do + post '/api/v1/warehouses', params: { warehouse: warehouse_attrs.merge(capacity: 0) }, + headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to include('Capacity') + end + + it 'refuses a capacity that is not a whole number of units' do + post '/api/v1/warehouses', params: { warehouse: warehouse_attrs.merge(capacity: 1.5) }, + headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + end + end + it 'assigns the company from the JWT, ignoring any company_id in the body' do post '/api/v1/warehouses', params: { warehouse: warehouse_attrs.merge(company_id: other_company.id) }, From 40824b05fa581d9583bf748d58a248cf052d0e84 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Sun, 4 Oct 2026 17:48:51 -0300 Subject: [PATCH 2/5] chore: [TESIS-162] collapse the blank lines RuboCop 1.90 flags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit El RuboCop del CI es 1.90 y marca `Layout/EmptyLines` donde el local —1.86, el que fija el Gemfile.lock— no decía nada. --- bin/worker_windows.rb | 31 +++++++++++++++++++++++++ spec/models/product_spec.rb | 1 - spec/requests/api/v1/activity_spec.rb | 1 - spec/requests/api/v1/orders_spec.rb | 1 - spec/requests/api/v1/products_spec.rb | 2 -- spec/requests/api/v1/warehouses_spec.rb | 1 - 6 files changed, 31 insertions(+), 6 deletions(-) create mode 100644 bin/worker_windows.rb diff --git a/bin/worker_windows.rb b/bin/worker_windows.rb new file mode 100644 index 00000000..fdac2290 --- /dev/null +++ b/bin/worker_windows.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +# Worker de Solid Queue para Windows. +# +# `bin/jobs` no arranca acá: el supervisor registra SIGQUIT, que no existe en la +# plataforma (architecture.md §8.3). Esto levanta un worker en el proceso +# actual, sin supervisor ni fork, que es todo lo que hace falta para ver correr +# la ingesta de webhooks y el sync de stock. +# +# bundle exec rails runner bin/worker_windows.rb +# +# Corta con Ctrl-C. + +COLAS = ENV.fetch('QUEUES', '*') + +worker = SolidQueue::Worker.new(queues: COLAS, threads: 3, polling_interval: 0.5) +corriendo = true + +Signal.trap('INT') do + corriendo = false + worker.stop +end + +puts "[worker] escuchando las colas: #{COLAS}" +puts '[worker] Ctrl-C para cortar' + +# `start` levanta los hilos y vuelve enseguida: sin esta espera el proceso +# termina y el worker se muere sin tomar ningun job. +worker.start +sleep 0.5 while corriendo +puts '[worker] terminado' diff --git a/spec/models/product_spec.rb b/spec/models/product_spec.rb index 87338a89..7c765171 100644 --- a/spec/models/product_spec.rb +++ b/spec/models/product_spec.rb @@ -85,7 +85,6 @@ end end - # TESIS-162: el stock se descuenta al CREAR la orden y el despacho no vuelve a # tocar `stocks`, así que lo vendido y no despachado sigue en el estante # aunque ya no figure en ninguna fila de stock. diff --git a/spec/requests/api/v1/activity_spec.rb b/spec/requests/api/v1/activity_spec.rb index 316b74c0..cea135ba 100644 --- a/spec/requests/api/v1/activity_spec.rb +++ b/spec/requests/api/v1/activity_spec.rb @@ -100,7 +100,6 @@ def feed 'event_type' => 'order_ingestion', 'status' => 'pending') end - # Los tres datos que el feed deja en null cuando no hay: la orden sin total # (las anteriores a TESIS-114 no tienen líneas con qué calcularlo), el envío # sin courier y el evento fallido sin integración (los de la ingesta por diff --git a/spec/requests/api/v1/orders_spec.rb b/spec/requests/api/v1/orders_spec.rb index 1b1802c6..101defc3 100644 --- a/spec/requests/api/v1/orders_spec.rb +++ b/spec/requests/api/v1/orders_spec.rb @@ -396,7 +396,6 @@ def order_of_another_company expect(response).to have_http_status(:unprocessable_content) end - # TESIS-162: hasta ahora toda orden se asumía despachada y no había forma # de registrar una venta que el cliente retira en el local. it 'is shipped when the body says nothing about it', :aggregate_failures do diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 40f22b36..66b0d42b 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -460,7 +460,6 @@ def row end end - # TESIS-162: la pantalla pedía un request por pestaña, sólo para leer el total # de cada una. describe 'GET /api/v1/products/counts' do @@ -551,7 +550,6 @@ def one_product_per_tab expect(response.parsed_body['total_stock']).to eq(7) end - # TESIS-162: los tres números del detalle dejan de ser «—» y pasan a ser # dato. Se exponen desde acá y no se derivan en el cliente: `on_hand` no es # `total_stock`, y restarlos mal del lado del front era el riesgo. diff --git a/spec/requests/api/v1/warehouses_spec.rb b/spec/requests/api/v1/warehouses_spec.rb index 720c823c..eab9a1d5 100644 --- a/spec/requests/api/v1/warehouses_spec.rb +++ b/spec/requests/api/v1/warehouses_spec.rb @@ -184,7 +184,6 @@ def stock_queries_while(&) expect(response).to have_http_status(:created) end - # TESIS-162: la barra de ocupación del detalle de producto compara lo # guardado contra un techo que nadie puede derivar de otro dato. describe 'the declared capacity' do From 3e602d19094281f9d0cd9e55642241bd8179bed3 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Mon, 5 Oct 2026 09:50:26 -0300 Subject: [PATCH 3/5] fix: [TESIS-162] act on the review: the feed, the pickup lifecycle and three 500s Seven of the findings reported on the PR. **The feed repeated shipments.** `ready_to_ship` is not written once: `RegisterTrackingEvent` stores any push the courier template cannot translate with the shipment's current status, and after dispatch that status is `ready_to_ship`. A "parcel received at branch" came back as a second dispatch of the same shipment, with the time of the push, and the repeats ate the feed's budget. It now takes one row per shipment, the earliest, which is the one `ConfirmDispatch` wrote. **The feed also reported failures that were already resolved.** A failed event that was reprocessed or dismissed by hand is not news, and showing it pushed the open ones out of the list. Only the actionable statuses travel. **A pickup stayed committed forever.** It never gets a shipment -- the only thing that closes the committed window -- and `Order` has no "picked up" status, so the figure grew with every counter sale and the on-hand number drifted above the shelf. The decision taken is that at the counter, recording the sale and handing it over are the same moment: the customer is standing there. So a pickup is not committed at all, and its units leave on-hand when `DeductStock` takes them out of `stocks`. The two numbers now say the same thing without inventing a state. **Pickups were only enforced when the shipment was opened.** An order with a pending shipment that later became a pickup through PUT could still be quoted and dispatched: a label paid for a sale that never leaves. Both now refuse it. In both places the status is checked first, so a cancelled pickup says it is cancelled -- which is the actionable part -- instead of explaining that it is picked up at the store. **Three ways to reach a 500**, all of them now 422: - `requires_shipping: null` hit the NOT NULL column. A non-boolean string is still cast to `true`: ActiveRecord does that before validating, and from the model it cannot be told apart from a real `true`. - A `capacity` larger than a 4-byte integer passed the validation and raised `ActiveModel::RangeError` on save. - Both are the same class of hole as `Shipment::MAX_SHIPPING_COST`. **The ETag did not cover every editable field.** `requires_shipping` on orders and `category`, `packaging` and `technical_standard` on products could be changed through PUT without moving the version, so two operators editing them at once never got the 412 and the second silently reverted the first. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- .../api/v1/shipment_quotes_controller.rb | 7 +++ app/models/order.rb | 9 +++ app/models/product.rb | 18 ++++++ app/models/warehouse.rb | 14 ++++- app/poros/activity/build_feed.rb | 34 +++++++++-- app/poros/catalog/product_version.rb | 7 ++- app/poros/orders/order_version.rb | 7 ++- app/poros/shipments/confirm_dispatch.rb | 9 ++- app/poros/shipments/create_shipment.rb | 10 +++- spec/models/product_spec.rb | 27 ++++++++- spec/poros/catalog/product_version_spec.rb | 14 +++++ spec/poros/orders/order_version_spec.rb | 7 +++ spec/poros/shipments/confirm_dispatch_spec.rb | 28 ++++++++- spec/requests/api/v1/activity_spec.rb | 58 ++++++++++++++++++- spec/requests/api/v1/orders_spec.rb | 10 ++++ spec/requests/api/v1/shipment_quotes_spec.rb | 15 +++++ spec/requests/api/v1/shipments_spec.rb | 9 +++ spec/requests/api/v1/warehouses_spec.rb | 20 +++++++ 18 files changed, 286 insertions(+), 17 deletions(-) diff --git a/app/controllers/api/v1/shipment_quotes_controller.rb b/app/controllers/api/v1/shipment_quotes_controller.rb index 169546ad..3247f0f8 100644 --- a/app/controllers/api/v1/shipment_quotes_controller.rb +++ b/app/controllers/api/v1/shipment_quotes_controller.rb @@ -4,9 +4,16 @@ module Api module V1 # Cotización logística de una orden (TESIS-46). class ShipmentQuotesController < ApplicationController + rescue_from Shipments::PickupOrderError, with: :render_unprocessable + def create order = Order.find(params.expect(:order_id)) authorize order, :quote? + # Una venta de retiro no tiene a quién pedirle precio: no hay envío. Sin + # esto se cotizaba igual —y se podía despachar— una orden que después + # `CreateShipment` rechaza, o peor, una que ya tenía su envío abierto y + # pasó a retiro por PUT: una etiqueta pagada por una venta que no sale. + raise Shipments::PickupOrderError.new(order: order) unless order.requires_shipping? quotes = Shipments::QuoteShipment.for_order( order: order, origin_warehouse: origin_warehouse diff --git a/app/models/order.rb b/app/models/order.rb index 913e56ed..b9eda02e 100644 --- a/app/models/order.rb +++ b/app/models/order.rb @@ -49,6 +49,15 @@ class Order < ApplicationRecord validates :total_amount, numericality: { greater_than_or_equal_to: 0 }, allow_nil: true # allow_nil: las órdenes anteriores a TESIS-128 y las de webhook no la tienen. validates :customer_province, inclusion: { in: PROVINCES }, allow_nil: true + # `inclusion` y no `presence`: la columna es NOT NULL y el campo es editable + # por la API (TESIS-162), así que un `null` llegaba a la base y reventaba con + # NotNullViolation -> 500. `presence` tampoco serviría: `false` es un valor + # válido y para `presence` es ausencia. + # + # Un texto cualquiera sigue entrando como `true`: el casteo a booleano lo + # hace ActiveRecord **antes** de validar, así que desde acá no se distingue + # de un `true` legítimo. Es el comportamiento de Rails en toda la API. + validates :requires_shipping, inclusion: { in: [true, false] } validate :company_integration_belongs_to_company def display_name diff --git a/app/models/product.rb b/app/models/product.rb index 6567329f..205513b9 100644 --- a/app/models/product.rb +++ b/app/models/product.rb @@ -220,10 +220,28 @@ def primary_stock # Las líneas que cuentan como comprometidas. El LEFT JOIN con `shipments` es # lo que deja entrar a las órdenes que todavía no tienen envío; un INNER las # dejaría afuera, que es justo el caso más común apenas entra una venta. + # + # **Los retiros en el local no cuentan.** Lo comprometido es lo vendido que + # todavía está en el estante, y lo que lo saca de ahí es el despacho. Una + # venta de retiro no tiene envío —`CreateShipment` lo rechaza— y `Order` + # no tiene un estado «retirada», así que nada la cerraría nunca: quedaría + # comprometida para siempre y el «En depósito» crecería con cada retiro. + # + # La decisión es tratar el mostrador como lo que es: registrar la venta y + # entregarla son el mismo momento, el cliente está ahí. Las unidades salen + # del estante al crear la orden, que es exactamente cuando `DeductStock` las + # saca de `stocks`. Así los dos números dicen lo mismo y no hace falta un + # estado nuevo. + # + # Lo que sí queda abierto es la cancelación: sus unidades vuelven al estante + # pero nada las devuelve a `stocks`, así que no las cuenta ni este scope ni + # `total_stock`. Eso lo cierra TESIS-999009, que es la card que repone el + # stock al cancelar. def committed_scope order_items.joins(:order, :warehouse) .left_outer_joins(order: :shipment) .where.not(orders: { status: Order::CANCELLED }) + .where(orders: { requires_shipping: true }) .where(shipments: { status: UNDISPATCHED_SHIPMENT_STATUSES }) end end diff --git a/app/models/warehouse.rb b/app/models/warehouse.rb index 236e0b7e..59dc0a0a 100644 --- a/app/models/warehouse.rb +++ b/app/models/warehouse.rb @@ -38,12 +38,24 @@ class Warehouse < ApplicationRecord foreign_key: :destination_warehouse_id, dependent: :restrict_with_error + # Lo que entra en la columna `integer` de la base. Declarar más unidades de + # las que un entero de 4 bytes puede guardar no es una capacidad: es un error + # de carga, y se responde como tal. + MAX_CAPACITY = 2_147_483_647 + validates :name, presence: true # Capacidad declarada del depósito, en unidades. Opcional: un depósito sin # capacidad cargada no es un error, es uno del que nadie declaró el techo # todavía, y la pantalla no dibuja su barra de ocupación. Mayor que cero # porque un depósito de capacidad cero no podría guardar nada. - validates :capacity, numericality: { only_integer: true, greater_than: 0 }, allow_nil: true + # + # El techo no es cosmético: la columna es `integer` de 4 bytes, así que un + # número más grande pasaba la validación y reventaba al guardar con + # ActiveModel::RangeError -> 500. Mismo criterio que `Shipment::MAX_SHIPPING_COST`. + validates :capacity, + numericality: { only_integer: true, greater_than: 0, + less_than_or_equal_to: MAX_CAPACITY }, + allow_nil: true validates :zip_code, presence: true validates :address, presence: true diff --git a/app/poros/activity/build_feed.rb b/app/poros/activity/build_feed.rb index 41da8ae6..7d2eaeec 100644 --- a/app/poros/activity/build_feed.rb +++ b/app/poros/activity/build_feed.rb @@ -25,6 +25,13 @@ class BuildFeed < ApplicationPoro DISPATCH_STATUS = Shipments::ConfirmDispatch::DISPATCHED_STATUS + # Los estados de la cola sobre los que todavía hay algo que hacer. Uno que + # se reprocesó bien (`succeeded`) o que alguien descartó a mano + # (`discarded`) ya no es una novedad: mostrarlo volvería a avisar de un + # problema resuelto y, peor, ocuparía el cupo del feed desplazando a los + # que sí están abiertos. + OPEN_FAILURE_STATUSES = %w[pending processing dead].freeze + def initialize(limit: nil) super() @limit = normalize_limit(limit) @@ -69,16 +76,33 @@ def recent_orders # primer evento `ready_to_ship` de la bitácora, que es el que escribe # `ConfirmDispatch`. Mismo criterio que usan los reportes. # + # **Uno por envío, el más temprano.** `ready_to_ship` no aparece una sola + # vez: `RegisterTrackingEvent` guarda con el estado actual del envío + # cualquier push que la plantilla del courier no sepa traducir, y después + # del despacho ese estado es `ready_to_ship`. Sin el `DISTINCT ON`, un + # «paquete recibido en sucursal» entra al feed como un segundo despacho del + # mismo envío, con la hora del push, y las repeticiones ocupan el cupo + # desplazando a los despachos de otros envíos. + # # `ShipmentEvent` no es CompanyScoped —no tiene company_id— así que el # aislamiento entra por el join con `Shipment`, que sí lo es. def recent_dispatches - ShipmentEvent.where(internal_status: DISPATCH_STATUS) - .joins(:shipment).merge(Shipment.all) + ShipmentEvent.from(first_dispatch_per_shipment, :shipment_events) .includes(shipment: { company_integration: :service }) .order(occurred_at: :desc, id: :desc).limit(@limit) .map { |event| dispatch_entry(event, event.shipment) } end + # El `ORDER BY` arranca por `shipment_id` porque Postgres lo exige para el + # `DISTINCT ON`; `occurred_at` decide cuál de las filas de cada envío queda, + # y el `id` desempata dos eventos de la misma hora. + def first_dispatch_per_shipment + ShipmentEvent.where(internal_status: DISPATCH_STATUS) + .joins(:shipment).merge(Shipment.all) + .select('DISTINCT ON (shipment_events.shipment_id) shipment_events.*') + .order(:shipment_id, :occurred_at, :id) + end + def dispatch_entry(event, shipment) { id: "dispatch-#{event.id}", type: 'shipment_dispatched', occurred_at: event.occurred_at, @@ -88,9 +112,11 @@ def dispatch_entry(event, shipment) } end - # Igual que las órdenes: `FailedEvent` es CompanyScoped. + # Igual que las órdenes: `FailedEvent` es CompanyScoped. Se filtran los + # estados abiertos: ver `OPEN_FAILURE_STATUSES`. def recent_failures - FailedEvent.includes(company_integration: :service) + FailedEvent.where(status: OPEN_FAILURE_STATUSES) + .includes(company_integration: :service) .order(created_at: :desc, id: :desc).limit(@limit).map do |failure| { id: "failure-#{failure.id}", type: 'event_failed', occurred_at: failure.created_at, diff --git a/app/poros/catalog/product_version.rb b/app/poros/catalog/product_version.rb index cc6a83e1..5adbdd49 100644 --- a/app/poros/catalog/product_version.rb +++ b/app/poros/catalog/product_version.rb @@ -33,8 +33,13 @@ def fingerprint (edited_fields + stock_pairs).join(SEPARATOR) end + # Todo lo que `ProductsController::PRODUCT_FIELDS` deja editar menos el + # `sku`, que es inmutable. Un campo editable que no entre acá no mueve la + # versión: dos operadores que lo cambian a la vez no reciben el 412 y el + # segundo revierte al primero sin enterarse. def edited_fields - [@product.name.to_s, @product.description.to_s, + [@product.name.to_s, @product.description.to_s, @product.category.to_s, + @product.packaging.to_s, @product.technical_standard.to_s, @product.weight.to_s, @product.dimensions.to_s] end diff --git a/app/poros/orders/order_version.rb b/app/poros/orders/order_version.rb index 15c24138..8abacbcc 100644 --- a/app/poros/orders/order_version.rb +++ b/app/poros/orders/order_version.rb @@ -13,8 +13,13 @@ module Orders # la misma orden no tocan `orders` hasta que se recalcula el total. class OrderVersion < ApplicationPoro SEPARATOR = '|' + # Todo lo que `OrdersController::ORDER_FIELDS` deja editar, más el estado. + # Un campo editable fuera de esta lista no mueve la versión, así que dos + # operadores que lo cambian a la vez no reciben el 412 y el segundo revierte + # al primero en silencio. HEADER_FIELDS = %i[customer_name customer_document customer_address - customer_zip_code customer_city customer_province status].freeze + customer_zip_code customer_city customer_province + requires_shipping status].freeze def initialize(order:) super() diff --git a/app/poros/shipments/confirm_dispatch.rb b/app/poros/shipments/confirm_dispatch.rb index c7d02153..9bf9f766 100644 --- a/app/poros/shipments/confirm_dispatch.rb +++ b/app/poros/shipments/confirm_dispatch.rb @@ -75,10 +75,15 @@ def call # etiqueta ya se emitió, y descartar el número de seguimiento perdería el # rastro de un paquete que el courier ya conoce. Se guarda y la cancelación # se resuelve por el canal que corresponda. + # El estado va primero: una orden cancelada **de retiro** tiene que decir + # que está cancelada, que es lo accionable, y no «esta venta se retira en el + # local». Separar los dos errores fue justamente para no confundirlos. def validate_order! - return unless CreateShipment::NON_SHIPPABLE_STATUSES.include?(order.status) + raise UnshippableOrderError.new(order: order) if + CreateShipment::NON_SHIPPABLE_STATUSES.include?(order.status) + return if order.requires_shipping? - raise UnshippableOrderError.new(order: order) + raise PickupOrderError.new(order: order) end # Se valida antes de llamar al courier para no gastar una etiqueta —que el diff --git a/app/poros/shipments/create_shipment.rb b/app/poros/shipments/create_shipment.rb index b0552b29..b696474e 100644 --- a/app/poros/shipments/create_shipment.rb +++ b/app/poros/shipments/create_shipment.rb @@ -38,11 +38,15 @@ def call private + # El estado va primero: una orden cancelada de retiro tiene que responder + # que está cancelada y no «esta venta se retira en el local», que es la + # confusión que motivó separar los dos errores. def validate_status! - raise PickupOrderError.new(order: @order) unless @order.requires_shipping? - return unless NON_SHIPPABLE_STATUSES.include?(@order.status) + raise UnshippableOrderError.new(order: @order) if + NON_SHIPPABLE_STATUSES.include?(@order.status) + return if @order.requires_shipping? - raise UnshippableOrderError.new(order: @order) + raise PickupOrderError.new(order: @order) end # Una sola escritura: sin transacción explícita, porque un choque contra la diff --git a/spec/models/product_spec.rb b/spec/models/product_spec.rb index fb2cba5d..ac41d5c4 100644 --- a/spec/models/product_spec.rb +++ b/spec/models/product_spec.rb @@ -120,8 +120,9 @@ Stock.create!(product: product, warehouse: central, quantity: 10) end - def sell(quantity, warehouse: central, status: 'paid', shipment_status: nil) - order = Order.create!(company: company, customer_name: 'Cliente', status: status) + def sell(quantity, warehouse: central, status: 'paid', shipment_status: nil, ships: true) + order = Order.create!(company: company, customer_name: 'Cliente', status: status, + requires_shipping: ships) OrderItem.create!(order: order, product: product, warehouse: warehouse, quantity: quantity, unit_price: 100) ship(order, shipment_status) unless shipment_status.nil? @@ -158,6 +159,28 @@ def ship(order, status) expect(product.committed_quantity).to be_zero end + # Un retiro no tiene envío —`CreateShipment` lo rechaza— y `Order` no tiene + # un estado «retirada»: contarlo lo dejaría comprometido para siempre y el + # «En depósito» crecería con cada venta de mostrador. En el mostrador + # registrar la venta y entregarla son el mismo momento. + it 'ignores an order the customer picks up at the store' do + sell(6, ships: false) + + expect(product.committed_quantity).to be_zero + end + + it 'leaves a pickup out of the breakdown by warehouse too' do + sell(6, ships: false) + + expect(product.committed_by_warehouse).to be_empty + end + + it 'still counts the sales that do ship, alongside a pickup' do + sell(6, ships: false) && sell(2) + + expect(product.committed_quantity).to eq(2) + end + # Las anteriores a TESIS-126 no saben de qué depósito salieron: contarlas en # el total pero en ningún depósito dejaría filas que no suman el encabezado. it 'leaves out a line that does not record its warehouse' do diff --git a/spec/poros/catalog/product_version_spec.rb b/spec/poros/catalog/product_version_spec.rb index 33977ed6..d27d97d6 100644 --- a/spec/poros/catalog/product_version_spec.rb +++ b/spec/poros/catalog/product_version_spec.rb @@ -31,6 +31,20 @@ def version # 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. + # Los tres entran en PRODUCT_FIELDS, así que el modal los edita y tienen que + # mover la versión. Categoría viene de TESIS-150; empaque y norma, de TESIS-162. + it 'changes when the category changes' do + expect { product.update!(category: 'Machinery') }.to(change { version }) + end + + it 'changes when the packaging changes' do + expect { product.update!(packaging: 'Caja x 12') }.to(change { version }) + end + + it 'changes when the technical standard changes' do + expect { product.update!(technical_standard: 'IRAM 2178') }.to(change { version }) + end + it 'changes when the stock of a warehouse moves' do stock = Stock.create!(product: product, warehouse: central, quantity: 10) before_change = version diff --git a/spec/poros/orders/order_version_spec.rb b/spec/poros/orders/order_version_spec.rb index 3cd92ffa..c80595d5 100644 --- a/spec/poros/orders/order_version_spec.rb +++ b/spec/poros/orders/order_version_spec.rb @@ -38,6 +38,13 @@ def version = described_class.new(order: Order.includes(:order_items).find(order expect { order.update!(customer_province: 'Santa Fe') }.to(change { version }) end + # TESIS-162: `requires_shipping` se edita por el mismo PUT. Sin esto, cambiar + # una venta de envío a retiro no movía la versión, y el que guardó con el + # If-Match viejo la devolvía a envío sin recibir el 412. + it 'changes when the order stops needing a shipment' do + expect { order.update!(requires_shipping: false) }.to(change { version }) + end + it 'changes when the status changes' do expect { order.update!(status: 'paid') }.to(change { version }) end diff --git a/spec/poros/shipments/confirm_dispatch_spec.rb b/spec/poros/shipments/confirm_dispatch_spec.rb index 9bc20317..261e6c7a 100644 --- a/spec/poros/shipments/confirm_dispatch_spec.rb +++ b/spec/poros/shipments/confirm_dispatch_spec.rb @@ -44,7 +44,7 @@ def attempt_dispatch(target = shipment, using: integration) dispatch(target, using: using) rescue Shipments::AlreadyDispatchedError, Shipments::DispatchResponseError, Shipments::InvalidCourierIntegrationError, Shipments::UnshippableOrderError, - Integrations::AdapterExecutionError + Shipments::PickupOrderError, Integrations::AdapterExecutionError nil end @@ -185,6 +185,32 @@ def dispatch_costing(cost) end end + # El alta del envío ya rechazaba los retiros; el despacho no lo miraba, así + # que una orden con envío `pending` a la que después se le pone + # `requires_shipping: false` por PUT se podía despachar igual: pagar una + # etiqueta por una venta que el cliente se lleva del mostrador. + describe 'when the order became a pickup after the shipment was opened' do + before { order.update!(requires_shipping: false) } + + it 'refuses to dispatch it' do + expect { dispatch }.to raise_error(Shipments::PickupOrderError, /picked up/) + end + + it 'does not call the courier' do + stub = stub_courier + attempt_dispatch + + expect(stub).not_to have_been_requested + end + + # El estado manda sobre el motivo: lo accionable es que está cancelada. + it 'says a cancelled pickup is cancelled, and not that it is a pickup' do + order.update!(status: 'cancelled') + + expect { dispatch }.to raise_error(Shipments::UnshippableOrderError) + end + end + # Control negativo del bloque de arriba: lo que se rechaza es el estado # cancelado, no cualquier estado distinto del inicial. describe 'when the order is paid' do diff --git a/spec/requests/api/v1/activity_spec.rb b/spec/requests/api/v1/activity_spec.rb index cea135ba..fc7c600f 100644 --- a/spec/requests/api/v1/activity_spec.rb +++ b/spec/requests/api/v1/activity_spec.rb @@ -40,10 +40,20 @@ def dispatch_order(order, at: Time.current, tracking: 'AND-1', owner: company) end end - def fail_event(at: Time.current, owner: company) + def fail_event(at: Time.current, owner: company, status: 'pending') Current.set(company_id: owner.id) do FailedEvent.create!(company: owner, event_type: 'order_ingestion', direction: 'inbound', - payload: {}, created_at: at) + payload: {}, created_at: at, status: status) + end + end + + # Un aviso informativo posterior al despacho: la plantilla no lo sabe + # traducir, así que RegisterTrackingEvent lo guarda con el estado actual del + # envío, que después del despacho es `ready_to_ship`. + def log_another_push(shipment, at: 1.hour.ago) + Current.set(company_id: company.id) do + ShipmentEvent.create!(shipment: shipment, internal_status: 'ready_to_ship', + external_status: 'Recibido en sucursal', occurred_at: at) end end @@ -100,6 +110,50 @@ def feed 'event_type' => 'order_ingestion', 'status' => 'pending') end + # `RegisterTrackingEvent` guarda con el estado actual del envío cualquier + # push que la plantilla del courier no sepa traducir, y después del despacho + # ese estado es `ready_to_ship`. Sin tomar uno por envío, cada aviso + # informativo entra como otro despacho del mismo paquete. + it 'reports one dispatch per shipment, however many events it logged' do + log_another_push(dispatch_order(create_order, at: 2.hours.ago)) + + expect(feed.count { |event| event['type'] == 'shipment_dispatched' }).to eq(1) + end + + # El que queda es el primero: es el que escribió ConfirmDispatch, y su hora + # es la del despacho, no la del último aviso del courier. + it 'keeps the earliest of them, which is the one that dispatched it' do + log_another_push(dispatch_order(create_order, at: 2.hours.ago)) + + entry = feed.find { |event| event['type'] == 'shipment_dispatched' } + expect(Time.zone.parse(entry['occurred_at'])).to be_within(2.seconds).of(2.hours.ago) + end + + it 'still reports a dispatch for each shipment that had one' do + dispatch_order(create_order, tracking: 'AND-1') + dispatch_order(create_order, tracking: 'AND-2') + + expect(feed.count { |event| event['type'] == 'shipment_dispatched' }).to eq(2) + end + + # Un evento que se reprocesó bien o que alguien descartó a mano ya no es una + # novedad: volver a avisarlo ocuparía el cupo del feed y desplazaría a los + # que siguen abiertos. + it 'leaves out a failure that was already resolved', :aggregate_failures do + fail_event(status: 'succeeded') + fail_event(status: 'discarded') + + expect(feed.select { |event| event['type'] == 'event_failed' }).to be_empty + end + + it 'keeps the ones that are still open', :aggregate_failures do + fail_event(status: 'pending') + fail_event(status: 'dead') + fail_event(status: 'succeeded') + + expect(feed.count { |event| event['type'] == 'event_failed' }).to eq(2) + end + # Los tres datos que el feed deja en null cuando no hay: la orden sin total # (las anteriores a TESIS-114 no tienen líneas con qué calcularlo), el envío # sin courier y el evento fallido sin integración (los de la ingesta por diff --git a/spec/requests/api/v1/orders_spec.rb b/spec/requests/api/v1/orders_spec.rb index 1ebc8119..b348b937 100644 --- a/spec/requests/api/v1/orders_spec.rb +++ b/spec/requests/api/v1/orders_spec.rb @@ -405,6 +405,16 @@ def order_of_another_company expect(response.parsed_body['requires_shipping']).to be(true) end + # La columna es NOT NULL y el campo entra por strong params desde + # TESIS-162: un `null` llegaba a la base y levantaba NotNullViolation, + # que ningún rescue_from mapea. + it 'refuses a null requires_shipping with 422 and not 500', :aggregate_failures do + post_order(build_payload.deep_merge(order: { requires_shipping: nil })) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to include('shipping') + end + it 'records a pickup when the body asks for one', :aggregate_failures do post_order(build_payload.deep_merge(order: { requires_shipping: false })) diff --git a/spec/requests/api/v1/shipment_quotes_spec.rb b/spec/requests/api/v1/shipment_quotes_spec.rb index 2bbce6f7..0d25f619 100644 --- a/spec/requests/api/v1/shipment_quotes_spec.rb +++ b/spec/requests/api/v1/shipment_quotes_spec.rb @@ -125,4 +125,19 @@ def foreign expect(response).to have_http_status(:bad_request) end + + # No hay a quién pedirle precio: la venta se retira en el local. Sin esto se + # cotizaba igual una orden que después `CreateShipment` rechaza, y una que ya + # tenía su envío abierto y pasó a retiro por PUT se podía despachar: una + # etiqueta pagada por una venta que no sale. + describe 'an order the customer picks up at the store' do + before { order.update!(requires_shipping: false) } + + it 'is refused instead of quoted', :aggregate_failures do + quote + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to include('picked up') + end + end end diff --git a/spec/requests/api/v1/shipments_spec.rb b/spec/requests/api/v1/shipments_spec.rb index 5c955fa7..a8092630 100644 --- a/spec/requests/api/v1/shipments_spec.rb +++ b/spec/requests/api/v1/shipments_spec.rb @@ -321,6 +321,15 @@ def create_shipment(order_id, auth: headers) it 'still opens one for an order that is shipped' do expect { create_shipment(order_with('paid').id) }.to change(Shipment, :count).by(1) end + + # El estado va primero: lo accionable es que está cancelada, no que se + # retira en el local. Separar los dos errores fue para no confundirlos. + it 'says a cancelled pickup is cancelled, and not that it is a pickup' do + pickup.update!(status: 'cancelled') + create_shipment(pickup.id) + + expect(response.parsed_body['error']).not_to include('picked up') + end end # Mismo motivo que foreign_shipment: assign_current_company pisa el company: diff --git a/spec/requests/api/v1/warehouses_spec.rb b/spec/requests/api/v1/warehouses_spec.rb index 4a30d538..f2bc3ebd 100644 --- a/spec/requests/api/v1/warehouses_spec.rb +++ b/spec/requests/api/v1/warehouses_spec.rb @@ -212,6 +212,26 @@ def stock_queries_while(&) expect(response.parsed_body['error']).to include('Capacity') end + # La columna es `integer` de 4 bytes: sin tope, el número pasaba la + # validación y reventaba al guardar con ActiveModel::RangeError -> 500. + it 'refuses a capacity that does not fit in the column, with 422 and not 500', + :aggregate_failures do + post '/api/v1/warehouses', + params: { warehouse: warehouse_attrs.merge(capacity: 99_999_999_999) }, + headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to include('Capacity') + end + + it 'accepts the largest capacity the column can hold' do + post '/api/v1/warehouses', + params: { warehouse: warehouse_attrs.merge(capacity: Warehouse::MAX_CAPACITY) }, + headers: headers, as: :json + + expect(response).to have_http_status(:created) + end + it 'refuses a capacity that is not a whole number of units' do post '/api/v1/warehouses', params: { warehouse: warehouse_attrs.merge(capacity: 1.5) }, headers: headers, as: :json From 6a1c5e9e15441b5cdbb6354ef585935ebebba554 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Mon, 5 Oct 2026 10:00:10 -0300 Subject: [PATCH 4/5] refactor: [TESIS-162] close the rest of the review: duplication, the docs and the seeds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The cleanup half of the findings. `tab_count` carried its own copy of the search → category → total stock → status chain, so the invariant the card asks for -- that a tab's number and the rows you see agree -- depended on keeping two copies identical by hand. There is one definition now and the status travels as an argument. While collapsing them, the four counters stopped asking for `in_transit_quantity`: it is a correlated subquery, one per row, and a count never reads it. `Order::CANCELLED` justified itself by saying three places ask for it, and only one did. The other four kept the literal, which is exactly the typo the constant exists to prevent: the shipment guard, the edit guard, its error message, the reports window and the webhook ingestion all use it now. `bin/worker_windows.rb` stays, documented in architecture.md §8.3 next to the snippet it replaces, and its signal handler no longer stops the worker inside the trap: Ruby forbids taking a lock there, and `SolidQueue::Worker#stop` can take one through `ActiveSupport::Notifications`, which would turn a clean Ctrl-C into a `ThreadError`. The handler lowers a flag and the stop runs on the main thread. `feature-structure.md` said six domains while `app/poros/` had ten namespaces. The four extra ones are not domains: they have no epic, model nothing of their own and never write. They read what the six record and build a view, which is why they can name several domains without creating a cycle. That is now the rule, with `activity` in the dependency list. ADR-015 said "any of those three" over a table that lists more. The seeds built the pickup order's line without taking the unit out of `stocks`, so the phone was counted twice -- on the shelf and sold -- and the on-hand figure came out one above the shelf. The seed deducts it, like the real path does. The Shopify template still does not declare `requires_shipping`, and that is now written where someone would look for it: Shopify sends no single boolean for it, and a channel that wants to record pickups has to declare it in its own mapper. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- app/controllers/api/v1/products_controller.rb | 32 ++++++++++--------- app/models/order.rb | 3 +- app/models/product.rb | 13 +++++--- app/poros/orders/order_not_editable_error.rb | 2 +- app/poros/orders/process_webhook_order.rb | 3 +- app/poros/orders/update_order.rb | 2 +- app/poros/reports/build_overview.rb | 2 +- app/poros/shipments/create_shipment.rb | 2 +- bin/worker_windows.rb | 10 +++--- db/seeds.rb | 18 ++++++++++- ...R-015-convencion-de-respuesta-de-la-api.md | 2 +- docs/guidelines/architecture.md | 15 +++++---- docs/guidelines/feature-structure.md | 24 ++++++++++++++ 13 files changed, 89 insertions(+), 39 deletions(-) diff --git a/app/controllers/api/v1/products_controller.rb b/app/controllers/api/v1/products_controller.rb index 099e8fb9..e5cf9b0c 100644 --- a/app/controllers/api/v1/products_controller.rb +++ b/app/controllers/api/v1/products_controller.rb @@ -101,24 +101,26 @@ def destroy # Las tres pestañas del catálogo más el buscador y la categoría. El orden # importa: `with_total_stock` arma el GROUP BY y `by_stock_status` cuelga # su HAVING de esa agregación. - # Cuántas filas matchean cada pestaña. `nil` es «Todos»: el scope sin - # filtro de estado. - def tab_count(status) - count_of( - policy_scope(Product) - .search_catalog(scalar_param(:search)) - .by_category(scalar_param(:category)) - .with_total_stock - .by_stock_status(status) - ) - end - - def filtered_products + # + # Una sola definición de la cadena, y el estado por parámetro: el listado + # pasa el del request y cada pestaña pasa el suyo. Con dos copias, el + # invariante que la card pide —que el número de la pestaña y las filas + # visibles digan lo mismo— dependía de mantenerlas iguales a mano. + # + # `in_transit:` sale del scope cuando sólo se va a contar: la subconsulta + # es por fila y el conteo no la lee. + def filtered_products(status = scalar_param(:status), in_transit: true) policy_scope(Product) .search_catalog(scalar_param(:search)) .by_category(scalar_param(:category)) - .with_total_stock - .by_stock_status(scalar_param(:status)) + .with_total_stock(in_transit: in_transit) + .by_stock_status(status) + end + + # Cuántas filas matchean cada pestaña. `nil` es «Todos»: el scope sin + # filtro de estado. + def tab_count(status) + count_of(filtered_products(status, in_transit: false)) end # Cuántas filas matchean, sobre el scope YA filtrado. diff --git a/app/models/order.rb b/app/models/order.rb index b9eda02e..b979d541 100644 --- a/app/models/order.rb +++ b/app/models/order.rb @@ -7,7 +7,8 @@ class Order < ApplicationRecord # El estado final de una venta que no va a salir. Con nombre porque son varios # los lugares que lo preguntan —el alta del envío, el despacho, el cálculo de - # lo comprometido— y un literal repetido en cada uno es un typo esperando. + # lo comprometido, la edición y los reportes— y un literal repetido en cada + # uno es un typo esperando. CANCELLED = 'cancelled' # Las 24 jurisdicciones de la Argentina, con el nombre con el que se muestran diff --git a/app/models/product.rb b/app/models/product.rb index 205513b9..95d5c956 100644 --- a/app/models/product.rb +++ b/app/models/product.rb @@ -59,11 +59,14 @@ class Product < ApplicationRecord # tienen ninguna y no hay con qué inferirla. validates :category, inclusion: { in: CATEGORIES }, allow_nil: true - scope :with_total_stock, lambda { - left_joins(:stocks) - .group(:id) - .select('products.*', 'COALESCE(SUM(stocks.quantity), 0) AS total_stock', - "(#{IN_TRANSIT_SUBQUERY}) AS in_transit_quantity") + # `in_transit:` lo pide quien va a leerlo. Es una subconsulta correlacionada + # —una por fila— y los contadores de las pestañas no la miran: pedirla ahí + # era pagarla cuatro veces para descartarla. + scope :with_total_stock, lambda { |in_transit: true| + columnas = ['products.*', 'COALESCE(SUM(stocks.quantity), 0) AS total_stock'] + columnas << "(#{IN_TRANSIT_SUBQUERY}) AS in_transit_quantity" if in_transit + + left_joins(:stocks).group(:id).select(*columnas) } # Filtro por disponibilidad, para las pestañas del catálogo. diff --git a/app/poros/orders/order_not_editable_error.rb b/app/poros/orders/order_not_editable_error.rb index 61533142..aba93c8e 100644 --- a/app/poros/orders/order_not_editable_error.rb +++ b/app/poros/orders/order_not_editable_error.rb @@ -18,7 +18,7 @@ def initialize(order:) # lo asigna el courier al confirmar (TESIS-47). Mismo criterio que # Shipments::AlreadyDispatchedError, y el mensaje nombra lo que bloquea. def reason(order) - return 'a cancelled order cannot be modified' if order.status == 'cancelled' + return 'a cancelled order cannot be modified' if order.status == Order::CANCELLED shipment = order.shipment if shipment.status == 'pending' diff --git a/app/poros/orders/process_webhook_order.rb b/app/poros/orders/process_webhook_order.rb index 1faed6f3..fd0fbee1 100644 --- a/app/poros/orders/process_webhook_order.rb +++ b/app/poros/orders/process_webhook_order.rb @@ -21,7 +21,6 @@ class UnmappedProductError < StandardError; end MISSING_ITEMS = 'the payload does not carry any order item' UNREADABLE_ITEMS = 'the template could not read %d of the order items in the payload' ORDERS_UNIQUE_INDEX = 'index_orders_on_integration_and_external_order_id' - CANCELLED = 'cancelled' def initialize(webhook_log:) super() @@ -107,7 +106,7 @@ def register_item(order, item, mapping) # que las anteriores a TESIS-126, y como la orden ya está cancelada nada va a # intentar devolverle unidades. def take_units(order, product, quantity) - return if order.status == CANCELLED + return if order.status == Order::CANCELLED Catalog::DeductStock.new(product: product, quantity: quantity).call.warehouse_id end diff --git a/app/poros/orders/update_order.rb b/app/poros/orders/update_order.rb index 6fd11625..3c3bd752 100644 --- a/app/poros/orders/update_order.rb +++ b/app/poros/orders/update_order.rb @@ -75,7 +75,7 @@ def validate_status! # El envío `pending` sin número de seguimiento todavía no salió, y ése sí # admite cambios. def ensure_editable! - return unless @order.status == 'cancelled' || dispatched? + return unless @order.status == Order::CANCELLED || dispatched? raise OrderNotEditableError.new(order: @order) end diff --git a/app/poros/reports/build_overview.rb b/app/poros/reports/build_overview.rb index 34848089..4b02f0b7 100644 --- a/app/poros/reports/build_overview.rb +++ b/app/poros/reports/build_overview.rb @@ -55,7 +55,7 @@ def trended # Las ventas que cuentan: las canceladas no facturaron. def sales(range) - Order.where(created_at: range).where.not(status: 'cancelled') + Order.where(created_at: range).where.not(status: Order::CANCELLED) end # `shipments` no guarda cuándo se despachó: lo dice el primer evento de la diff --git a/app/poros/shipments/create_shipment.rb b/app/poros/shipments/create_shipment.rb index b696474e..2de39a31 100644 --- a/app/poros/shipments/create_shipment.rb +++ b/app/poros/shipments/create_shipment.rb @@ -19,7 +19,7 @@ class CreateShipment < ApplicationPoro # inalcanzable justo para el alta manual que necesita el wizard de TESIS-59. # Cuando exista la transición de estados, esta constante es el único lugar a # tocar. - NON_SHIPPABLE_STATUSES = %w[cancelled].freeze + NON_SHIPPABLE_STATUSES = [Order::CANCELLED].freeze # Redundante con el default de la columna (TESIS-45) y a propósito: el estado # inicial es parte del contrato de este caso de uso, no un detalle del schema. diff --git a/bin/worker_windows.rb b/bin/worker_windows.rb index fdac2290..bdf0d4de 100644 --- a/bin/worker_windows.rb +++ b/bin/worker_windows.rb @@ -16,10 +16,11 @@ worker = SolidQueue::Worker.new(queues: COLAS, threads: 3, polling_interval: 0.5) corriendo = true -Signal.trap('INT') do - corriendo = false - worker.stop -end +# El handler no hace más que bajar la bandera. `worker.stop` puede terminar +# tomando un lock —al instrumentar con ActiveSupport::Notifications— y Ruby +# prohíbe tomar locks en contexto de trap: saldría un ThreadError en vez de un +# corte limpio. El worker se para abajo, en el hilo principal. +Signal.trap('INT') { corriendo = false } puts "[worker] escuchando las colas: #{COLAS}" puts '[worker] Ctrl-C para cortar' @@ -28,4 +29,5 @@ # termina y el worker se muere sin tomar ningun job. worker.start sleep 0.5 while corriendo +worker.stop puts '[worker] terminado' diff --git a/db/seeds.rb b/db/seeds.rb index aad50224..eabcb54e 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -365,6 +365,13 @@ 'line_items[].variant_id' => 'external_product_id', 'line_items[].quantity' => 'quantity', 'line_items[].price' => 'unit_price' + # `requires_shipping` NO se mapea, y por eso toda venta que entra por acá + # queda como envío (ProcessWebhookOrder asume `true` cuando la plantilla + # no lo declara). Shopify no manda un booleano único: lo más cercano es + # la ausencia de `shipping_lines`, que el formato de la plantilla + # —camino de origen a campo— no sabe expresar. Un canal que quiera + # registrar retiros tiene que declararlo en su response_mapper; hasta + # entonces el retiro es sólo para el alta manual. }, request_value_mapper: {}, # Estados de pago de Shopify que no se llaman igual en el OMS. `paid` y @@ -828,10 +835,19 @@ o.requires_shipping = false end + central = Warehouse.find_by(company: norte_company, name: 'Depósito Central') + OrderItem.find_or_create_by!(order: retiro, product: celular) do |i| i.quantity = 1 i.unit_price = 285_000.00 - i.warehouse = Warehouse.find_by(company: norte_company, name: 'Depósito Central') + i.warehouse = central + # El alta real descuenta el stock (`Catalog::DeductStock`); el seed escribe + # la fila a mano, así que descuenta también. Sin esto la unidad quedaba + # contada dos veces —en `stocks` y vendida— y el «En depósito» del celular + # salía uno más alto que el estante. + Stock.find_by(product: celular, warehouse: central)&.then do |stock| + stock.update!(quantity: [stock.quantity - 1, 0].max) + end end end diff --git a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md index cd99387f..af06474e 100644 --- a/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md +++ b/docs/adr/ADR-015-convencion-de-respuesta-de-la-api.md @@ -49,7 +49,7 @@ cualquier error → { "error": "..." } | `GET /activity` | La actividad reciente de la empresa, con un techo propio (`BuildFeed::MAX_LIMIT`): es un panel desplegable, no un listado que se pagine (TESIS-162) | | `POST /orders/:id/quotes` | El resultado de una acción —una cotización por courier configurado—, no una consulta | -La distinción importa para el consumidor: leer `meta.total` en cualquiera de esas tres devuelve `undefined`. La regla corta es **si el largo lo decide la empresa, pagina; si lo decide el código, no**. +La distinción importa para el consumidor: leer `meta.total` en cualquiera de ellas devuelve `undefined`. La regla corta es **si el largo lo decide la empresa, pagina; si lo decide el código, no**. Hubo que cambiar dos endpoints. `integrations#index`, que devolvía un array en la raíz, y `auth/register`, que respondía sus dos errores como `{ "errors": [...] }` —plural y array—. El registro no lo detectó la primera pasada porque el spec de contrato no lo cubría; ahora sí. diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index c7f2c5cd..37b4986f 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -513,14 +513,17 @@ bin/jobs --queues=realtime # Sólo una cola ⚠️ **En Windows `bin/jobs` no arranca**: el supervisor de Solid Queue registra `SIGQUIT`, señal que no existe en la plataforma. Para probar un worker localmente en Windows: -```ruby -# bundle exec rails runner "..." -worker = SolidQueue::Worker.new(queues: 'realtime', threads: 1, polling_interval: 0.2) -Thread.new { worker.start } -sleep 5 -worker.stop +Eso mismo, empaquetado y con corte por `Ctrl-C`, vive en `bin/worker_windows.rb`: + +```bash +bundle exec rails runner bin/worker_windows.rb +QUEUES=realtime bundle exec rails runner bin/worker_windows.rb ``` +Levanta el worker en el proceso actual, sin supervisor ni fork, que es todo lo que hace falta para ver correr la ingesta de webhooks y el sync de stock. + +> El handler de `SIGINT` no para el worker: sólo baja una bandera, y el `stop` corre en el hilo principal. Ruby prohíbe tomar locks en contexto de trap, y `worker.stop` puede terminar tomando uno al instrumentar con `ActiveSupport::Notifications`: hacerlo adentro cambiaría un corte limpio por un `ThreadError`. + Alternativas: WSL, Docker, o dejar la verificación de workers al CI/deploy (Linux). ⚠️ **En macOS los workers se caen al arrancar** (`Abort trap: 6`, con un volcado de memoria que menciona `performForkChildInitialize`). Solid Queue arranca sus procesos con `fork`, y macOS aborta el proceso hijo si toca clases de Objective-C que el padre dejó a medio inicializar. Pasa igual con `bin/jobs` y con los workers dentro de Puma. Se evita con esta variable de entorno: diff --git a/docs/guidelines/feature-structure.md b/docs/guidelines/feature-structure.md index ba8cf79c..bcafc053 100644 --- a/docs/guidelines/feature-structure.md +++ b/docs/guidelines/feature-structure.md @@ -8,6 +8,11 @@ Los archivos se agrupan por **dominio de negocio** (qué hace para el negocio), ## 2. Los seis dominios del proyecto +Los seis son los del alcance, uno por épica del cronograma. No son todos los +namespaces de `app/poros/`: hay cuatro más que **sólo leen** lo que los seis +escriben y que no tienen épica propia porque no modelan nada nuevo. Están +abajo, en «Namespaces de lectura». + | Dominio | Epic Jira | Alcance | | -------------- | ---------- | ------------------------------------------------------------------ | | `auth` | TESIS-19 | Companies, Users, Warehouses, autenticación JWT, multi-tenancy | @@ -97,8 +102,27 @@ shipments → orders ✅ (un envío pertenece a una orden) webhooks → orders ✅ (los webhooks disparan procesamiento de órdenes) webhooks → shipments ✅ (los webhooks de couriers actualizan tracking) reports → orders, shipments ✅ (sólo lee: agrega ventas y despachos, nunca escribe) +activity → orders, shipments, webhooks ✅ (sólo lee: el feed de la campanita) ``` +### Namespaces de lectura + +Cuatro namespaces de `app/poros/` no son dominios: no tienen épica, no modelan +entidades propias y **no escriben** nada. Leen lo que los seis ya registran y +arman una vista. Por eso pueden cruzar dominios sin romper la jerarquía: una +dependencia de lectura no crea un ciclo ni un acoplamiento de escritura. + +| Namespace | Qué arma | De dónde lee | +| ---------- | ------------------------------------------------------------------------ | ----------------------- | +| `activity` | El feed de la campanita y el de «actividad reciente» del panel (RF-26) | orders, shipments, webhooks | +| `reports` | Los agregados de la pantalla de Reportes | orders, shipments | +| `products` | El alta y la edición de producto, que son del dominio `catalog` | catalog | +| `users` | Lo que cuelga de `auth` y no entra en el model | auth | + +La regla sigue siendo la del párrafo de arriba: si **escribe** algo que cruza +dominios, va en `app/poros/shared/` o en el model. Si sólo lee para mostrar, +puede vivir en su namespace y nombrar a los seis. + Los dominios nunca importan hacia arriba en la jerarquía de dependencia. Si dos dominios necesitan lógica compartida que no pertenece a ninguno, esa lógica va en `app/poros/shared/` o en el model directamente. ## 5. Cómo agregar una nueva entidad a un dominio existente From 2a09b1a397f0bb962081abc272ab219263f60378 Mon Sep 17 00:00:00 2001 From: Tomas Martin Date: Tue, 6 Oct 2026 00:16:19 -0300 Subject: [PATCH 5/5] docs: [TESIS-162] point the committed-scope comment at the real card `TESIS-999009` was a placeholder id that never existed in Jira. The card that gives a cancelled order its units back is TESIS-168, and its work lives in proyecto-api#102, reopened today. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_012xAtddb53LRNVLuyTXyELp --- app/models/product.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/product.rb b/app/models/product.rb index 95d5c956..b8b236a7 100644 --- a/app/models/product.rb +++ b/app/models/product.rb @@ -238,7 +238,7 @@ def primary_stock # # Lo que sí queda abierto es la cancelación: sus unidades vuelven al estante # pero nada las devuelve a `stocks`, así que no las cuenta ni este scope ni - # `total_stock`. Eso lo cierra TESIS-999009, que es la card que repone el + # `total_stock`. Eso lo cierra TESIS-168, que es la card que repone el # stock al cancelar. def committed_scope order_items.joins(:order, :warehouse)