diff --git a/app/controllers/api/v1/failed_events_controller.rb b/app/controllers/api/v1/failed_events_controller.rb index 93d113c..20769bd 100644 --- a/app/controllers/api/v1/failed_events_controller.rb +++ b/app/controllers/api/v1/failed_events_controller.rb @@ -3,20 +3,14 @@ module Api module V1 class FailedEventsController < ApplicationController + include Paginatable + before_action :set_failed_event, only: %i[requeue discard] def index - page = [params[:page].to_i, 1].max - per_page = params.fetch(:per_page, 20).to_i.clamp(1, 100) - - events = filtered_events.order(created_at: :desc) - .offset((page - 1) * per_page) - .limit(per_page) + events, meta = paginate(filtered_events.order(created_at: :desc)) - render json: { - data: FailedEventSerializer.render_as_hash(events), - meta: { page: page, per_page: per_page, total: filtered_events.count } - } + render json: { data: FailedEventSerializer.render_as_hash(events), meta: meta } end # POST /api/v1/failed-events/:id/retry (`retry` es palabra reservada en Ruby) diff --git a/app/controllers/api/v1/integrations_controller.rb b/app/controllers/api/v1/integrations_controller.rb index f679bd6..5ef01b0 100644 --- a/app/controllers/api/v1/integrations_controller.rb +++ b/app/controllers/api/v1/integrations_controller.rb @@ -3,22 +3,34 @@ module Api module V1 class IntegrationsController < ApplicationController + include Paginatable + # El listado no pasa por Pundit: lo usa el widget de nodos del panel # aunque la empresa no tenga la feature `integrations`, y sólo muestra las # plantillas globales con el estado de la propia empresa. El alta y la # modificación sí: ver CompanyIntegrationPolicy. skip_after_action :verify_policy_scoped - # Envuelto en `data` como el resto de las colecciones (ADR-015). Era el + # Envuelto en `data` como el resto de las colecciones (ADR-015): era el # único listado que devolvía un array pelado, y un array en la raíz no - # deja lugar para agregarle `meta` el día que pagine sin romper a quien - # lo consume. + # deja lugar para agregarle `meta`. + # + # Y pagina como todo listado (TESIS-108), aunque hoy `services` tenga pocas + # filas: es una tabla global que sólo crece cuando el administrador carga + # una plantilla nueva. Dejarla afuera sería una excepción que habría que + # justificar, y la regla vale más que el ahorro. + # + # `WHOLE_LIST_PER_PAGE` porque el panel la lee entera para dibujar sus + # nodos, no de a páginas. def index integrations = current_company.company_integrations.index_by(&:service_id) + services, meta = paginate(Service.order(:id), per_page: WHOLE_LIST_PER_PAGE) + render json: { data: IntegrationStatusSerializer.render_as_hash( - Service.order(:id), integrations_by_service_id: integrations - ) + services, integrations_by_service_id: integrations + ), + meta: meta } end diff --git a/app/controllers/api/v1/orders_controller.rb b/app/controllers/api/v1/orders_controller.rb index 69dff9d..f160174 100644 --- a/app/controllers/api/v1/orders_controller.rb +++ b/app/controllers/api/v1/orders_controller.rb @@ -4,6 +4,7 @@ module Api module V1 class OrdersController < ApplicationController include OptimisticLocking + include Paginatable rescue_from ActiveRecord::RecordNotSaved, with: :render_unprocessable rescue_from Catalog::InsufficientStockError, with: :render_insufficient_stock @@ -31,28 +32,20 @@ class OrdersController < ApplicationController ].freeze def index - # `scalar_param` y no `params[...]` directo: una query con `?page[]=1` - # entrega un Array y `to_i` sale con NoMethodError → 500. Ver - # ApplicationController. - page = [scalar_param(:page).to_i, 1].max - per_page = (scalar_param(:per_page) || 20).to_i.clamp(1, 100) - # La precarga alimenta dos columnas del serializer: `item_count` sale de # order_items y `courier` de la cadena envío → integración → servicio. # Sin ella, cada fila de la página dispara sus propias consultas. - orders = filtered_orders.preload(:order_items, shipment: { company_integration: :service }) - .order(created_at: :desc, id: :desc) - .offset((page - 1) * per_page) - .limit(per_page) - - render json: { - data: OrderListSerializer.render_as_hash(orders), - # El total se cuenta sobre el scope YA FILTRADO, no sobre la tabla de - # la empresa: de este número salen los KPIs de TESIS-53, que los pide - # con `?status=pending&per_page=1` y lee sólo el meta. Si contara de - # más, los KPIs mentirían. - meta: { page: page, per_page: per_page, total: filtered_orders.count } - } + # + # El `total` del meta lo cuenta el concern sobre el scope YA FILTRADO, + # no sobre la tabla de la empresa: de ese número salen los KPIs de + # TESIS-53, que los piden con `?status=pending&per_page=1` y leen sólo + # el meta. Si contara de más, los KPIs mentirían. + orders, meta = paginate( + filtered_orders.preload(:order_items, shipment: { company_integration: :service }) + .order(created_at: :desc, id: :desc) + ) + + render json: { data: OrderListSerializer.render_as_hash(orders), meta: meta } end # Vocabulario del select de provincia del alta manual (TESIS-58). Mismo diff --git a/app/controllers/api/v1/product_mappings_controller.rb b/app/controllers/api/v1/product_mappings_controller.rb index 63c404d..d4a572c 100644 --- a/app/controllers/api/v1/product_mappings_controller.rb +++ b/app/controllers/api/v1/product_mappings_controller.rb @@ -3,6 +3,8 @@ module Api module V1 class ProductMappingsController < ApplicationController + include Paginatable + MISSING_INTEGRATION = 'company_integration_id is required' ALREADY_LINKED = 'external product already linked to another product in this integration' @@ -12,15 +14,18 @@ class ProductMappingsController < ApplicationController rescue_from ActiveRecord::RecordNotUnique, with: :render_conflict def index - mappings = policy_scope(ProductMapping) - .where(product_id: @product.id) - .includes(company_integration: :service) - .order(:created_at) - - # Se envuelve en `data` para que el front trate una sola shape en todo - # el árbol de /products. No lleva `meta` como el index de productos: - # los mappings son tantos como canales de venta y no se paginan. - render json: { data: ProductMappingSerializer.render_as_hash(mappings) } + scope = policy_scope(ProductMapping) + .where(product_id: @product.id) + .includes(company_integration: :service) + .order(:created_at) + + # Los mapeos de un producto son tantos como canales de venta tenga la + # empresa: se leen enteros, así que van con `WHOLE_LIST_PER_PAGE`. Lo + # que cambia respecto de antes es que ahora hay un techo, que es el + # punto de TESIS-108: ningún listado devuelve una cantidad ilimitada. + mappings, meta = paginate(scope, per_page: WHOLE_LIST_PER_PAGE) + + render json: { data: ProductMappingSerializer.render_as_hash(mappings), meta: meta } end def create diff --git a/app/controllers/api/v1/products_controller.rb b/app/controllers/api/v1/products_controller.rb index 46cb77c..5ed6b69 100644 --- a/app/controllers/api/v1/products_controller.rb +++ b/app/controllers/api/v1/products_controller.rb @@ -4,6 +4,7 @@ module Api module V1 class ProductsController < ApplicationController include OptimisticLocking + include Paginatable before_action :set_product, only: %i[show update destroy] rescue_from ActiveRecord::RecordNotUnique, with: :render_conflict @@ -11,9 +12,6 @@ class ProductsController < ApplicationController rescue_from Catalog::StaleProductError, with: :render_precondition_failed def index - page = [scalar_param(:page).to_i, 1].max - per_page = (scalar_param(:per_page) || 20).to_i.clamp(1, 100) - # La precarga es load-bearing: ProductListSerializer lee el depósito # principal de cada fila, y sin ella son dos queries por producto # (stocks + warehouse) en vez de dos para toda la página. @@ -24,17 +22,14 @@ def index # como eager_load, sumaría las columnas de stocks y warehouses a ese # SELECT y Postgres rechazaría la consulta por columnas fuera del # GROUP BY. preload garantiza las consultas separadas. - products = filtered_products.preload(stocks: :warehouse) - .order(created_at: :desc) - .offset((page - 1) * per_page) - .limit(per_page) - - total = count_of(filtered_products) - - render json: { - data: ProductListSerializer.render_as_hash(products), - meta: { page: page, per_page: per_page, total: total } - } + # `total:` explícito: el scope viene agrupado por products.id, así que + # su `.count` devolvería un Hash y no un entero (ver `count_of`). + products, meta = paginate( + filtered_products.preload(stocks: :warehouse).order(created_at: :desc), + total: count_of(filtered_products) + ) + + render json: { data: ProductListSerializer.render_as_hash(products), meta: meta } end def show diff --git a/app/controllers/api/v1/shipments_controller.rb b/app/controllers/api/v1/shipments_controller.rb index 17b36b1..009402a 100644 --- a/app/controllers/api/v1/shipments_controller.rb +++ b/app/controllers/api/v1/shipments_controller.rb @@ -19,27 +19,24 @@ class ShipmentsController < ApplicationController # El parámetro que falta es un 400 de contrato, no un 422 de negocio. rescue_from ActionController::ParameterMissing, with: :render_bad_request + include Paginatable + # Cuánto del cuerpo del courier se propaga en el mensaje de error. COURIER_ERROR_LIMIT = 300 def index - page = [params[:page].to_i, 1].max - per_page = params.fetch(:per_page, 20).to_i.clamp(1, 100) - # La precarga es load-bearing: ShipmentListSerializer lee el nombre del # courier a través de la plantilla del Service, y sin ella son dos # queries por fila (company_integrations + services). - shipments = filtered_shipments.preload(company_integration: :service) - .order(created_at: :desc, id: :desc) - .offset((page - 1) * per_page) - .limit(per_page) - - render json: { - data: ShipmentListSerializer.render_as_hash(shipments), - # El total se cuenta sobre el scope filtrado, no sobre el total de la - # empresa: de acá sale el KPI de envíos activos (TESIS-53). - meta: { page: page, per_page: per_page, total: filtered_shipments.count } - } + # + # El total lo cuenta el concern sobre el scope filtrado, no sobre el + # total de la empresa: de acá sale el KPI de envíos activos (TESIS-53). + shipments, meta = paginate( + filtered_shipments.preload(company_integration: :service) + .order(created_at: :desc, id: :desc) + ) + + render json: { data: ShipmentListSerializer.render_as_hash(shipments), meta: meta } end def show diff --git a/app/controllers/api/v1/stock_transfers_controller.rb b/app/controllers/api/v1/stock_transfers_controller.rb index b1b21f1..28d4dc5 100644 --- a/app/controllers/api/v1/stock_transfers_controller.rb +++ b/app/controllers/api/v1/stock_transfers_controller.rb @@ -3,18 +3,16 @@ module Api module V1 class StockTransfersController < ApplicationController + include Paginatable + before_action :set_transfer, only: %i[receive cancel] rescue_from Catalog::InsufficientWarehouseStockError, with: :render_unprocessable rescue_from Catalog::SettleTransfer::NotInFlightError, with: :render_conflict def index - transfers = policy_scope(StockTransfer) - .includes(:product, :origin_warehouse, :destination_warehouse) - .order(dispatched_at: :desc) - transfers = transfers.where(status: params[:status]) if params[:status].present? - transfers = transfers.where(product_id: params[:product_id]) if params[:product_id].present? + transfers, meta = paginate(filtered_transfers) - render json: { data: StockTransferSerializer.render_as_hash(transfers) } + render json: { data: StockTransferSerializer.render_as_hash(transfers), meta: meta } end def create @@ -40,6 +38,17 @@ def cancel private + # Las unidades en vuelo, filtrables por estado y por producto. El listado + # del catálogo pide las de un producto para el «+N Incoming» de su ficha. + def filtered_transfers + transfers = policy_scope(StockTransfer) + .includes(:product, :origin_warehouse, :destination_warehouse) + .order(dispatched_at: :desc) + transfers = transfers.where(status: params[:status]) if params[:status].present? + transfers = transfers.where(product_id: params[:product_id]) if params[:product_id].present? + transfers + end + def settle(outcome) transfer = Catalog::SettleTransfer.new(transfer: @transfer, outcome: outcome).call diff --git a/app/controllers/api/v1/warehouses_controller.rb b/app/controllers/api/v1/warehouses_controller.rb index 3eaad3d..f2187e8 100644 --- a/app/controllers/api/v1/warehouses_controller.rb +++ b/app/controllers/api/v1/warehouses_controller.rb @@ -3,15 +3,27 @@ module Api module V1 class WarehousesController < ApplicationController + include Paginatable + before_action :set_warehouse, only: %i[show update destroy] rescue_from ActiveRecord::RecordNotDestroyed, with: :render_conflict def index # with_stored_units agrega la suma de stocks en la misma consulta: sin # el scope, el serializer pediria las unidades deposito por deposito. - warehouses = policy_scope(Warehouse).with_stored_units.order(created_at: :desc) - - render json: { data: WarehouseSerializer.render_as_hash(warehouses) } + # + # `WHOLE_LIST_PER_PAGE` y no el default: el front usa este listado para + # llenar selects —el picker de origen del alta manual, el de los modales + # de producto—, no una tabla con paginador. Con 20 le faltarían depósitos + # sin que nada se lo diga; el techo sigue existiendo y `meta.total` le + # avisa si alguna vez lo pasa. + # + # `total:` explícito: with_stored_units agrupa por warehouses.id. + scope = policy_scope(Warehouse).with_stored_units.order(created_at: :desc) + warehouses, meta = paginate(scope, per_page: WHOLE_LIST_PER_PAGE, + total: policy_scope(Warehouse).count) + + render json: { data: WarehouseSerializer.render_as_hash(warehouses), meta: meta } end def show diff --git a/app/controllers/concerns/api/v1/paginatable.rb b/app/controllers/concerns/api/v1/paginatable.rb new file mode 100644 index 0000000..79f89e8 --- /dev/null +++ b/app/controllers/concerns/api/v1/paginatable.rb @@ -0,0 +1,71 @@ +# frozen_string_literal: true + +module Api + module V1 + # Paginación de los listados de la API (TESIS-108). + # + # Existía copiada literal en cuatro controllers —el mismo `[page.to_i, 1].max` + # y el mismo `clamp(1, 100)`— y ausente en otros cuatro, que devolvían la + # tabla entera. Esto es la única definición de las dos cosas: cuántas filas + # se devuelven y cómo se arma el `meta` que las acompaña. + # + # Todo listado de registros pasa por acá. Los vocabularios fijos + # (`/orders/provinces`, `/products/categories`) y el resultado de una acción + # (`/orders/:id/quotes`) no: su largo lo decide el código, no la empresa. + # + # La forma de la respuesta la fija ADR-015: la colección va en `data` y el + # `meta` al lado, con `page`, `per_page` y `total`. El `total` cuenta el + # scope **ya filtrado**, no la tabla: de ahí salen los contadores de las + # pestañas y los KPIs del panel. + module Paginatable + extend ActiveSupport::Concern + + # Techo duro. Nadie puede pedir más, venga el número de donde venga: es lo + # que impide que un listado devuelva una cantidad ilimitada de filas. + MAX_PER_PAGE = 100 + + # Cuántas filas devuelve un listado que nadie acotó. Es el tamaño de una + # pantalla paginada. + DEFAULT_PER_PAGE = 20 + + # Para los listados que el consumidor lee enteros —depósitos, mapeos, + # integraciones: los usa para llenar un select, no una tabla con + # paginador—. Siguen teniendo techo; lo que cambia es que el default no + # los recorta antes de tiempo. + # + # Si alguna empresa pasa de acá, el consumidor se entera por `meta.total`, + # que va a ser mayor que las filas recibidas, y ahí le toca paginar. Es + # preferible a que el default de 20 le esconda depósitos en silencio. + WHOLE_LIST_PER_PAGE = MAX_PER_PAGE + + private + + # Devuelve `[filas, meta]`. + # + # `total:` se puede pasar cuando contar el scope no es directo: el catálogo + # viene agrupado por `products.id`, así que su `.count` devuelve un Hash y + # el controller ya sabe cómo contarlo (ver `count_of`). + def paginate(scope, per_page: DEFAULT_PER_PAGE, total: nil) + page = page_number + size = page_size(per_page) + + [scope.offset((page - 1) * size).limit(size), + { page: page, per_page: size, total: total || scope.count }] + end + + # Página pedida, nunca menor que 1. `page=0` y `page=-3` se acotan en vez + # de romper: un offset negativo es un error de SQL, y un 400 por un número + # que se puede interpretar sería antipático. + def page_number + [scalar_param(:page).to_i, 1].max + end + + # `scalar_param` y no `params[...]`: `?per_page[]=1` entrega un Array y + # `Array#to_i` no existe (TESIS-124). Un valor no numérico cae en `to_i` + # a 0 y el `clamp` lo lleva al mínimo. + def page_size(default) + (scalar_param(:per_page) || default).to_i.clamp(1, MAX_PER_PAGE) + end + 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 b333a1e..0e2354f 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 @@ -26,8 +26,8 @@ Mientras hubo una sola pantalla consumiendo la API la inconsistencia era barata. ``` GET /api/v1/products → { "data": [ {...}, {...} ], "meta": { page, per_page, total } } -GET /api/v1/warehouses → { "data": [ {...}, {...} ] } -GET /api/v1/integrations → { "data": [ {...}, {...} ] } +GET /api/v1/warehouses → { "data": [ {...}, {...} ], "meta": { ... } } +GET /api/v1/integrations → { "data": [ {...}, {...} ], "meta": { ... } } GET /api/v1/products/:id → { "id": 1, "sku": "...", ... } POST /api/v1/products → { "id": 1, "sku": "...", ... } @@ -38,7 +38,16 @@ cualquier error → { "error": "..." } `error` es una sola clave y un solo string, y **siempre está**. Puede venir acompañado de datos para recuperarse: el 409 del locking optimista agrega `current_version`, que es lo que el frontend necesita para reintentar. Lo que no se admite es otra clave en su lugar —`errors` en plural, un array, un objeto por campo—, porque entonces el consumidor tiene que probar dos formas. -`meta` aparece sólo si el listado pagina, y es siempre `page`, `per_page` y `total`, contando el scope **ya filtrado**. +`meta` es siempre `page`, `per_page` y `total`, contando el scope **ya filtrado**. + +**Qué lleva `meta` y qué no.** Desde TESIS-108 pagina todo **listado de registros** —productos, depósitos, órdenes, envíos, transferencias, eventos fallidos, mapeos, integraciones—, así que ahí el consumidor puede leer `total` sin preguntarse cuál lo trae. Van envueltos en `data` **sin** `meta`, en cambio, los que no son listados de registros: + +| 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 | +| `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**. 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í. @@ -59,7 +68,7 @@ La puerta queda abierta: pasar de esta convención a la otra es aditivo del lado **A favor** - La regla se enuncia en una línea y no tiene excepciones que justificar. -- Ninguna colección queda con un array en la raíz, así que cualquiera puede empezar a paginar sin romper su contrato. Es la precondición de TESIS-108. +- Ninguna colección queda con un array en la raíz, así que todas pudieron empezar a paginar sin romper su contrato. Fue la precondición de TESIS-108, que se hizo justo encima. - El comentario-trampa del frontend se borra: lo que explicaba ya no pasa. - `spec/requests/api/v1/api_contract_spec.rb` (TESIS-90) fija las tres formas **sobre los endpoints que enumera**, hoy incluido el registro. La lista está escrita a mano: un endpoint nuevo con otra forma no rompe nada hasta que se lo agrega ahí. Recorrer todas las rutas sería otra card; mientras tanto, sumar el endpoint al spec es parte de agregarlo. diff --git a/docs/guidelines/architecture.md b/docs/guidelines/architecture.md index faba770..5195994 100644 --- a/docs/guidelines/architecture.md +++ b/docs/guidelines/architecture.md @@ -139,15 +139,17 @@ Una sola regla, fijada en [ADR-015](../adr/ADR-015-convencion-de-respuesta-de-la ``` GET /api/v1/products → { "data": [ ... ], "meta": { page, per_page, total } } -GET /api/v1/warehouses → { "data": [ ... ] } +GET /api/v1/warehouses → { "data": [ ... ], "meta": { page, per_page, total } } GET /api/v1/products/:id → { "id": 1, "sku": "...", ... } POST /api/v1/products → { "id": 1, "sku": "...", ... } cualquier error → { "error": "..." } ``` -`meta` aparece sólo si el listado pagina, y cuenta el scope **ya filtrado**, no la tabla entera. +`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). -Ninguna colección devuelve un array en la raíz: un array pelado no admite agregarle `meta` sin romper a quien lo consume. `spec/requests/api/v1/api_contract_spec.rb` fija las tres formas, así que un endpoint nuevo que invente una cuarta rompe la suite. +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. + +`spec/requests/api/v1/pagination_spec.rb` fija los bordes una vez y recorre los listados que enumera, verificando que cada uno traiga `meta` y respete el techo; `api_contract_spec.rb` fija las formas de los endpoints que enumera. Las dos listas están escritas a mano: **un listado nuevo hay que sumarlo ahí**, o la suite no se entera de que existe. ### 4.1 Flujo de un webhook entrante diff --git a/spec/requests/api/v1/api_contract_spec.rb b/spec/requests/api/v1/api_contract_spec.rb index 8a7f86c..af16773 100644 --- a/spec/requests/api/v1/api_contract_spec.rb +++ b/spec/requests/api/v1/api_contract_spec.rb @@ -104,9 +104,12 @@ def shipment # ───────────────────────────────────────────────────────── forma del sobre # - # La regla la fija ADR-015 (TESIS-107) y es una sola: **una colección viaja - # envuelta en `data`** —más `meta` si pagina— y **un recurso solo viaja - # pelado**. Los errores, siempre `{ "error": "..." }`. + # La regla la fija ADR-015 (TESIS-107, TESIS-108) y es una sola: **una + # colección viaja en `data` + `meta`** y **un recurso solo viaja pelado**. + # Los errores, siempre `{ "error": "..." }`. + # + # No hay colección sin `meta`: desde TESIS-108 todas paginan, así que el + # consumidor puede leer `total` en cualquiera sin preguntarse cuál lo trae. # # Antes eran cuatro formas distintas, porque cada card eligió la suya y nadie # la escribió. Estos ejemplos son lo que impide que vuelva a pasar: agregar @@ -121,13 +124,15 @@ def shipment expect(response.parsed_body['meta'].keys).to match_array(claves[:meta]) end - # Sin `meta`: el listado no pagina. Es la mitad de TESIS-108. - it 'wraps an unpaginated collection in data alone' do + # Antes devolvía `data` sola porque no paginaba. Desde TESIS-108 no queda + # ninguna así: un listado sin techo puede devolver la tabla entera. + it 'wraps every collection in data plus meta, with no exception', :aggregate_failures do warehouse get '/api/v1/warehouses', headers: headers - expect(response.parsed_body.keys).to eq(['data']) + expect(response.parsed_body.keys).to match_array(%w[data meta]) + expect(response.parsed_body['meta'].keys).to match_array(claves[:meta]) end it 'returns a single resource with no envelope at all' do @@ -139,10 +144,10 @@ def shipment # Era el único que devolvía un array pelado. Un array en la raíz no admite # `meta` sin romper a quien lo consume, así que ninguna colección puede # quedar así. - it 'wraps the integrations listing too, with no exception' do + it 'wraps the integrations listing too, which used to be a bare array' do get '/api/v1/integrations', headers: headers - expect(response.parsed_body.keys).to eq(['data']) + expect(response.parsed_body.keys).to match_array(%w[data meta]) end # Los errores sí son consistentes en toda la API, y conviene que siga así. diff --git a/spec/requests/api/v1/integrations_spec.rb b/spec/requests/api/v1/integrations_spec.rb index 373693d..154688a 100644 --- a/spec/requests/api/v1/integrations_spec.rb +++ b/spec/requests/api/v1/integrations_spec.rb @@ -27,10 +27,10 @@ def listed # Era el único listado que contestaba un array pelado. Un array en la raíz # no deja lugar para `meta` sin romper a quien lo consume, y obligaba al # front a recordar que éste es la excepción (ADR-015, TESIS-107). - it 'wraps the collection in data, like every other listing' do + it 'wraps the collection in data and meta, like every other listing' do get '/api/v1/integrations', headers: headers - expect(response.parsed_body.keys).to eq(['data']) + expect(response.parsed_body.keys).to match_array(%w[data meta]) end context 'when the company has the service configured' do diff --git a/spec/requests/api/v1/pagination_spec.rb b/spec/requests/api/v1/pagination_spec.rb new file mode 100644 index 0000000..7534973 --- /dev/null +++ b/spec/requests/api/v1/pagination_spec.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Los bordes de la paginación, una vez y no por endpoint (TESIS-108). +# +# El cálculo vive en `Api::V1::Paginatable` y lo comparten todos los listados, +# así que se prueba sobre uno —productos, que es el que más filas tiene en los +# ejemplos— y aparte se verifica que los demás lo usen. +# +# Lo que estos ejemplos protegen es que un número raro se **acote** en vez de +# romper: un `page=0` se traduce a un offset negativo, que es un error de SQL, y +# un `per_page=9999` es una respuesta que nadie pidió. +RSpec.describe 'Pagination', type: :request do + let(:company) { Company.create!(name: 'Norte', tax_id: '30-11111111-1') } + let(:user) { User.create!(email: 'n@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_products(count) + count.times { |i| Product.create!(company: company, sku: "NOR-#{i}", name: "Producto #{i}") } + end + + def meta_for(params) + get '/api/v1/products', params: params, headers: headers + response.parsed_body['meta'] + end + + def rows_for(params) + get '/api/v1/products', params: params, headers: headers + response.parsed_body['data'] + end + + describe 'the page number' do + it 'defaults to the first page' do + expect(meta_for({})['page']).to eq(1) + end + + # Un offset negativo es un error de SQL: se acota en vez de romper. + it 'clamps page zero to the first page' do + expect(meta_for(page: 0)['page']).to eq(1) + end + + it 'clamps a negative page to the first page' do + expect(meta_for(page: -3)['page']).to eq(1) + end + + it 'reads a page that is not a number as the first one' do + expect(meta_for(page: 'dos')['page']).to eq(1) + end + + # Una página más allá del final no es un error: es una página vacía. + it 'answers an empty page past the end, with the real total', :aggregate_failures do + create_products(3) + + expect(rows_for(page: 99)).to be_empty + expect(meta_for(page: 99)['total']).to eq(3) + end + end + + describe 'the page size' do + it 'defaults to twenty rows' do + expect(meta_for({})['per_page']).to eq(Api::V1::Paginatable::DEFAULT_PER_PAGE) + end + + it 'honours a size the caller asked for' do + expect(meta_for(per_page: 5)['per_page']).to eq(5) + end + + # El techo es lo que impide que un listado devuelva la tabla entera. + it 'never goes above the ceiling, however large the request' do + expect(meta_for(per_page: 9999)['per_page']).to eq(Api::V1::Paginatable::MAX_PER_PAGE) + end + + it 'clamps a size of zero to one row' do + expect(meta_for(per_page: 0)['per_page']).to eq(1) + end + + it 'clamps a negative size to one row' do + expect(meta_for(per_page: -5)['per_page']).to eq(1) + end + + it 'reads a size that is not a number as one row' do + expect(meta_for(per_page: 'todas')['per_page']).to eq(1) + end + + it 'returns exactly the rows it says it returns' do + create_products(7) + + expect(rows_for(per_page: 3).length).to eq(3) + end + end + + describe 'the total' do + it 'counts the whole scope and not the page', :aggregate_failures do + create_products(7) + + get '/api/v1/products', params: { per_page: 3 }, headers: headers + + expect(response.parsed_body['data'].length).to eq(3) + expect(response.parsed_body['meta']['total']).to eq(7) + end + end + + # Antes esto era el cálculo copiado en cuatro controllers y ausente en otros + # tres. Si alguien agrega un listado que no pagina, o vuelve a copiar el + # cálculo, este bloque lo delata. + describe 'every listing of the API' do + def warehouse + Warehouse.create!(company: company, name: 'CD Norte', address: 'Av. 1', zip_code: '1900') + end + + def listings + product = Product.create!(company: company, sku: 'NOR-X', name: 'Producto') + warehouse + ['/api/v1/products', '/api/v1/warehouses', '/api/v1/orders', '/api/v1/shipments', + '/api/v1/failed-events', '/api/v1/integrations', '/api/v1/stock-transfers', + "/api/v1/products/#{product.id}/mappings"] + end + + def meta_of(path) + get path, headers: headers + response.parsed_body['meta'] + end + + it 'answers with a meta that carries page, per_page and total', :aggregate_failures do + listings.each do |path| + expect(meta_of(path)&.keys).to match_array(%w[page per_page total]), + "#{path} no trae el meta esperado" + end + end + + it 'respects the ceiling everywhere', :aggregate_failures do + listings.each do |path| + get path, params: { per_page: 9999 }, headers: headers + + expect(response.parsed_body['meta']['per_page']).to eq(Api::V1::Paginatable::MAX_PER_PAGE), + "#{path} se pasa del techo" + end + end + end +end diff --git a/spec/requests/api/v1/product_mappings_spec.rb b/spec/requests/api/v1/product_mappings_spec.rb index fbfe289..0780d45 100644 --- a/spec/requests/api/v1/product_mappings_spec.rb +++ b/spec/requests/api/v1/product_mappings_spec.rb @@ -72,12 +72,12 @@ def link_external_id_to_another_product(external_id) # Misma envoltura que GET /api/v1/products (TESIS-33): el front no tiene que # tratar dos shapes distintas en endpoints vecinos del mismo árbol. - it 'wraps the collection in a data key', :aggregate_failures do + it 'wraps the collection in data and meta', :aggregate_failures do mapping_for(product, meli_integration, 'MLA-123') get mappings_url(product.id), headers: headers expect(response.parsed_body).to be_a(Hash) - expect(response.parsed_body.keys).to eq(['data']) + expect(response.parsed_body.keys).to match_array(%w[data meta]) expect(response.parsed_body['data']).to be_an(Array) end