diff --git a/.gitignore b/.gitignore index 57d9b58f..7985eac3 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,5 @@ # Ignore key files for decrypting credentials and more. /config/*.key +# Reporte de cobertura de SimpleCov (TESIS-93). +/coverage/ diff --git a/Gemfile b/Gemfile index 700acf59..113672cf 100644 --- a/Gemfile +++ b/Gemfile @@ -60,6 +60,11 @@ end group :test do # Stubs de HTTP para testear el adaptador de integraciones sin red real. gem 'webmock' + + # Mide qué líneas ejecuta la suite (TESIS-93). require: false porque lo + # arranca spec_helper antes de cargar la aplicación: lo que se carga antes + # que SimpleCov no se mide. + gem 'simplecov', require: false end gem 'devise', '~> 5.0' diff --git a/Gemfile.lock b/Gemfile.lock index ef4e930f..549eb76a 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -363,6 +363,7 @@ GEM rubocop (~> 1.86, >= 1.86.2) ruby-progressbar (1.13.0) securerandom (0.4.1) + simplecov (1.3.0) solid_cable (4.0.2) actioncable (>= 7.2) activejob (>= 7.2) @@ -457,6 +458,7 @@ DEPENDENCIES rubocop-rails rubocop-rails-omakase rubocop-rspec + simplecov solid_cable solid_cache solid_queue @@ -591,6 +593,7 @@ CHECKSUMS rubocop-rspec (3.10.2) sha256=0b3e2ecc592cd10ecbf0095bb58d1e357905276e069643523cc19eb7495f65e2 ruby-progressbar (1.13.0) sha256=80fc9c47a9b640d6834e0dc7b3c94c9df37f08cb072b7761e4a71e22cff29b33 securerandom (0.4.1) sha256=cc5193d414a4341b6e225f0cb4446aceca8e50d5e1888743fac16987638ea0b1 + simplecov (1.3.0) sha256=d9886307863c1ead47657dcbed869bfffd82318808b09014d8f0ff77dcfe7492 solid_cable (4.0.2) sha256=084636a67679ad00d23088b33c84047e614bcf41ee559db24b414d83cdc42d03 solid_cache (1.0.10) sha256=bc05a2fb3ac78a6f43cbb5946679cf9db67dd30d22939ededc385cb93e120d41 solid_queue (1.7.0) sha256=6566b70b801d1c317c81bba7bcdd5677c019afac584a30374b4164002ca356d3 diff --git a/app/poros/shipments/poll_tracking_status.rb b/app/poros/shipments/poll_tracking_status.rb index 6cdd398a..8b2cc028 100644 --- a/app/poros/shipments/poll_tracking_status.rb +++ b/app/poros/shipments/poll_tracking_status.rb @@ -51,8 +51,13 @@ def tracking_service # Se revalida al ejecutar, no al encolar: entre el barrido y este job el # envío pudo entregarse (por otro ciclo, o a mano desde el panel). + # `order(:id)` no es cosmético: en la consulta masiva los números viajan + # concatenados en la URI, así que sin un orden fijo el mismo lote produce + # URLs distintas entre corridas. Postgres puede devolver las filas en + # cualquier orden, y eso ya hacía fallar de forma intermitente al spec que + # fija la URL del lote (TESIS-93). def shipments - @shipments ||= @integration.shipments.in_flight.where(id: @shipment_ids).to_a + @shipments ||= @integration.shipments.in_flight.where(id: @shipment_ids).order(:id).to_a end # Pares [envío, movimiento traducido] a registrar. diff --git a/lefthook.yml b/lefthook.yml index e0d76124..4a46b634 100644 --- a/lefthook.yml +++ b/lefthook.yml @@ -17,7 +17,10 @@ pre-push: parallel: true commands: rspec: - run: bundle exec rspec + # COVERAGE_FLOOR: acá se corre la suite entera, así que el piso de + # cobertura de spec_helper.rb tiene sentido. En una corrida parcial no se + # evalúa (ver el comentario ahí). + run: COVERAGE_FLOOR=1 bundle exec rspec branch-name: run: | branch=$(git rev-parse --abbrev-ref HEAD) diff --git a/spec/avo/filters_spec.rb b/spec/avo/filters_spec.rb new file mode 100644 index 00000000..67d26cb6 --- /dev/null +++ b/spec/avo/filters_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Los filtros del panel de administración (TESIS-93). +# +# Son tres clases de tres líneas y ninguna tenía un ejemplo: si un filtro +# ignorara el valor elegido, el panel mostraría la tabla entera y el +# administrador no tendría cómo notarlo —una lista más larga de lo que pidió no +# parece un error—. +# +# Se prueban por `apply`, que es el único método que Avo les llama, con la +# consulta sin scopear: el aislamiento por empresa es de la API, y el backoffice +# ve a propósito todas las empresas. +RSpec.describe 'Avo filters', type: :model do + let(:company) { Company.create!(name: 'Norte', tax_id: '30-11111111-1') } + let(:other_company) { Company.create!(name: 'Sur', tax_id: '30-22222222-2') } + let(:couriers) { {} } + + def order_for(a_company, status: 'pending') + Current.set(company_id: a_company.id) do + Order.create!(company: a_company, customer_name: 'Juan', status: status) + end + end + + # Una sola integración por empresa: un envío por orden, y dos integraciones de + # la misma empresa pedirían dos couriers distintos. + def courier_of(a_company) + couriers[a_company.id] ||= Current.set(company_id: a_company.id) do + courier_integration(company: a_company, name: "Andreani #{a_company.id}") + end + end + + def shipment_for(a_company, status: 'pending') + courier = courier_of(a_company) + + Current.set(company_id: a_company.id) do + order = Order.create!(company: a_company, customer_name: 'Juan', status: 'paid') + Shipment.create!(company: a_company, order: order, company_integration: courier, + status: status) + end + end + + describe Avo::Filters::CompanyFilter do + subject(:filter) { described_class.new } + + it 'keeps only the rows of the chosen company' do + mine = order_for(company) + order_for(other_company) + + expect(filter.apply(nil, Order.unscoped, company.id).pluck(:id)).to eq([mine.id]) + end + + it 'leaves the query untouched when nothing is chosen' do + order_for(company) + order_for(other_company) + + expect(filter.apply(nil, Order.unscoped, '').count).to eq(2) + end + + it 'offers one option per company, by name', :aggregate_failures do + company + other_company + + expect(filter.options.values).to include('Norte', 'Sur') + end + end + + describe Avo::Filters::OrderStatusFilter do + subject(:filter) { described_class.new } + + it 'keeps only the orders in the chosen status' do + paid = order_for(company, status: 'paid') + order_for(company, status: 'pending') + + expect(filter.apply(nil, Order.unscoped, 'paid').pluck(:id)).to eq([paid.id]) + end + + it 'leaves the query untouched when nothing is chosen' do + order_for(company, status: 'paid') + order_for(company, status: 'pending') + + expect(filter.apply(nil, Order.unscoped, nil).count).to eq(2) + end + + it 'offers exactly the statuses an order can be in' do + expect(filter.options.keys).to match_array(Order::STATUSES) + end + end + + describe Avo::Filters::ShipmentStatusFilter do + subject(:filter) { described_class.new } + + it 'keeps only the shipments in the chosen status' do + in_transit = shipment_for(company, status: 'in_transit') + shipment_for(company, status: 'pending') + + expect(filter.apply(nil, Shipment.unscoped, 'in_transit').pluck(:id)).to eq([in_transit.id]) + end + + it 'leaves the query untouched when nothing is chosen' do + shipment_for(company, status: 'in_transit') + shipment_for(company, status: 'pending') + + expect(filter.apply(nil, Shipment.unscoped, '').count).to eq(2) + end + end +end diff --git a/spec/jobs/shipments/scan_pull_tracking_job_spec.rb b/spec/jobs/shipments/scan_pull_tracking_job_spec.rb index e41c9baa..90523dfa 100644 --- a/spec/jobs/shipments/scan_pull_tracking_job_spec.rb +++ b/spec/jobs/shipments/scan_pull_tracking_job_spec.rb @@ -104,6 +104,24 @@ def run = described_class.new.perform .with(other.company_integration_id, [other.id], company_b.id) end + # Una integración con plantilla de seguimiento y sin nada en vuelo: el barrido + # la encuentra y tiene que salir sin encolar. Sin este ejemplo, dividir por la + # cantidad de grupos con la lista vacía —una división por cero— no la veía + # nadie (TESIS-93). + it 'enqueues nothing for a courier with no shipment in flight' do + integration_for(company_a) + + expect { run }.not_to have_enqueued_job(Shipments::PollTrackingJob) + end + + it 'still sweeps the couriers that do have shipments in flight' do + integration_for(company_a) + other = shipment_for(integration_for(company_b), 'CB-1') + + expect { run }.to have_enqueued_job(Shipments::PollTrackingJob) + .with(other.company_integration_id, [other.id], company_b.id) + end + it 'ignores couriers without a tracking template' do shipment_for(integration_for(company_a, service: courier_service('Andreani')), 'AND-1') expect { run }.not_to have_enqueued_job(Shipments::PollTrackingJob) diff --git a/spec/models/stock_transfer_spec.rb b/spec/models/stock_transfer_spec.rb index f5cd1d8a..9c099b93 100644 --- a/spec/models/stock_transfer_spec.rb +++ b/spec/models/stock_transfer_spec.rb @@ -45,6 +45,40 @@ def other_company # `validate: true` en el enum existe para que un estado desconocido invalide # el registro en lugar de explotar en el asignador (mismo criterio que # WebhookLog y FailedEvent). + # Las dos validaciones cruzadas arrancan con un guard: sin depósitos, o sin + # empresa, no hay nada que comparar y la que tiene que hablar es la + # presencia. Sin estos ejemplos el guard no lo ejercitaba nadie, y quitarlo + # —que convierte la falta de un depósito en un error confuso sobre el otro— + # no rompía la suite (TESIS-93). + context 'when a warehouse is missing' do + before { transfer.origin_warehouse = nil } + + it 'is invalid' do + expect(transfer).not_to be_valid + end + + it 'complains about the missing warehouse and not about the two being equal' do + transfer.valid? + + expect(transfer.errors[:destination_warehouse]).to be_empty + end + end + + context 'when the transfer has no company' do + before { transfer.company = nil } + + it 'is invalid' do + expect(transfer).not_to be_valid + end + + it 'does not blame the product for belonging elsewhere', :aggregate_failures do + transfer.valid? + + expect(transfer.errors[:product]).to be_empty + expect(transfer.errors[:origin_warehouse]).to be_empty + end + end + it 'rejects an unknown status without raising on assignment' do transfer.status = 'lost' expect(transfer).not_to be_valid diff --git a/spec/models/webhook_log_spec.rb b/spec/models/webhook_log_spec.rb index 65173022..a2d114c8 100644 --- a/spec/models/webhook_log_spec.rb +++ b/spec/models/webhook_log_spec.rb @@ -40,6 +40,17 @@ # Sin esta validación, un Current heredado haría que assign_current_company # pisara el company_id explícito y el log terminara en el tenant equivocado # en silencio. Acá falla fuerte en vez de escribir mal. + # El guard de la validación cruzada: sin integración no hay con qué comparar, + # y quien tiene que hablar es la presencia de la asociación. Nadie lo + # ejercitaba (TESIS-93). + it 'does not blame the integration when there is none', :aggregate_failures do + log = described_class.new(company: company) + + log.valid? + + expect(log.errors[:company_integration]).not_to include('must belong to the same company') + end + it 'refuses to write under a leaked tenant instead of doing it silently' do integration Current.company_id = Company.create!(name: 'Intruso', tax_id: '30-88888888-8').id diff --git a/spec/policies/application_policy_spec.rb b/spec/policies/application_policy_spec.rb new file mode 100644 index 00000000..3749ad2a --- /dev/null +++ b/spec/policies/application_policy_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# La base de la que heredan todas las policies (TESIS-93). +# +# Lo que se prueba acá no es una regla de negocio sino una postura: **nada está +# permitido salvo que una policy concreta lo permita**. Cada uno de estos +# defaults es lo que va a contestar una acción nueva cuya policy todavía no se +# escribió, o un recurso al que alguien le agregó un endpoint y se olvidó del +# permiso. Que respondan `false` es la diferencia entre un permiso faltante que +# se nota y uno que se filtra. +# +# Ninguna policy real ejercita estos métodos —todas los pisan—, así que sin +# estos ejemplos el comportamiento por defecto no lo verifica nadie. +RSpec.describe ApplicationPolicy do + subject(:policy) { described_class.new(user, record) } + + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:user) { User.create!(email: 'a@acme.com', password: 'pass123', company: company) } + let(:record) { Product.new(company: company, sku: 'SKU-1', name: 'Widget') } + + describe 'the default answer to every action' do + it 'refuses to list', :aggregate_failures do + expect(policy.index?).to be(false) + expect(policy.show?).to be(false) + end + + it 'refuses to write', :aggregate_failures do + expect(policy.create?).to be(false) + expect(policy.update?).to be(false) + expect(policy.destroy?).to be(false) + end + + # Con un usuario presente y un registro de su propia empresa: la negativa no + # depende de que falte el usuario, es el default. + it 'refuses even to an authenticated user over a record of their own company' do + expect(policy.index?).to be(false) + end + end + + # `new?` y `edit?` no tienen regla propia: son las vistas de `create?` y + # `update?`. Si alguien redefine `create?` y se olvida de `new?`, esto fija + # que no hacía falta acordarse. + describe 'the aliases of the write actions' do + it 'answers new? with create?' do + expect(policy.new?).to eq(policy.create?) + end + + it 'answers edit? with update?' do + expect(policy.edit?).to eq(policy.update?) + end + + context 'when a subclass allows creating and updating' do + let(:permissive) do + Class.new(described_class) do + def create? = true + def update? = true + end + end + + it 'follows the subclass instead of the default', :aggregate_failures do + subclass_policy = permissive.new(user, record) + + expect(subclass_policy.new?).to be(true) + expect(subclass_policy.edit?).to be(true) + end + end + end + + describe ApplicationPolicy::Scope do + # Un Scope que no define `resolve` no devuelve "todo": explota. Es el otro + # lado de la misma postura — una policy a medio escribir falla a la vista y + # no filtra la tabla entera del tenant equivocado. + it 'refuses to resolve a scope that did not define it' do + scope = described_class.new(user, Product.all) + + expect { scope.resolve }.to raise_error(NoMethodError, /must define #resolve/) + end + + it 'names the class that has to define it' do + incomplete = Class.new(described_class) + stub_const('IncompleteScope', incomplete) + + expect { incomplete.new(user, Product.all).resolve } + .to raise_error(NoMethodError, /IncompleteScope/) + end + end +end diff --git a/spec/policies/product_mapping_policy_spec.rb b/spec/policies/product_mapping_policy_spec.rb index 84e90104..b411637c 100644 --- a/spec/policies/product_mapping_policy_spec.rb +++ b/spec/policies/product_mapping_policy_spec.rb @@ -38,6 +38,18 @@ def foreign_product expect(policy).to be_destroy end + # `record.product` puede venir en nil: CompanyScoped filtra también las + # asociaciones, así que un mapping de otro tenant llega sin producto. El `&.` + # es lo que evita el NoMethodError, y su rama nil no la ejercitaba nadie + # (TESIS-93). + context 'when the mapped product is out of the scope of the tenant' do + before { allow(mapping).to receive(:product).and_return(nil) } + + it 'denies destroying it instead of raising' do + expect(policy).not_to be_destroy + end + end + context 'when the mapped product belongs to another company' do let(:mapping) { mapping_for(foreign_product, 'Shopify') } diff --git a/spec/policies/product_policy_spec.rb b/spec/policies/product_policy_spec.rb new file mode 100644 index 00000000..61a5b5b8 --- /dev/null +++ b/spec/policies/product_policy_spec.rb @@ -0,0 +1,90 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# El catálogo es la tabla que más endpoints toca, y hasta TESIS-93 su policy no +# tenía spec propio: el aislamiento se verificaba sólo de punta a punta, por +# HTTP. Esto fija la regla en el lugar donde vive. +RSpec.describe ProductPolicy do + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:other_company) { Company.create!(name: 'Rival', tax_id: '30-88888888-1') } + let(:user) { User.create!(email: 'a@acme.com', password: 'pass123', company: company) } + + def product_for(a_company, sku: 'SKU-1') + Current.set(company_id: a_company.id) do + Product.create!(company: a_company, sku: sku, name: 'Widget') + end + end + + describe '#index?' do + it 'allows an authenticated user' do + expect(described_class.new(user, Product).index?).to be(true) + end + + it 'refuses without a user' do + expect(described_class.new(nil, Product).index?).to be(false) + end + end + + describe '#create?' do + it 'allows an authenticated user' do + expect(described_class.new(user, Product).create?).to be(true) + end + + it 'refuses without a user' do + expect(described_class.new(nil, Product).create?).to be(false) + end + end + + describe '#show?' do + it 'allows a product of the same company' do + expect(described_class.new(user, product_for(company)).show?).to be(true) + end + + it 'refuses a product of another company' do + expect(described_class.new(user, product_for(other_company)).show?).to be(false) + end + end + + describe '#update?' do + it 'allows a product of the same company' do + expect(described_class.new(user, product_for(company)).update?).to be(true) + end + + it 'refuses a product of another company' do + expect(described_class.new(user, product_for(other_company)).update?).to be(false) + end + end + + describe '#destroy?' do + it 'allows a product of the same company' do + expect(described_class.new(user, product_for(company)).destroy?).to be(true) + end + + it 'refuses a product of another company' do + expect(described_class.new(user, product_for(other_company)).destroy?).to be(false) + end + end + + # Igual que en OrderPolicy: los ejemplos corren con `unscoped` porque si no el + # default_scope de CompanyScoped filtraría antes y el Scope pasaría sin hacer + # nada. + describe 'Scope#resolve' do + it 'returns the products of the company of the user' do + mine = product_for(company) + product_for(other_company, sku: 'SKU-2') + + resolved = described_class::Scope.new(user, Product.unscoped).resolve + + expect(resolved.pluck(:id)).to eq([mine.id]) + end + + it 'returns nothing when the company has no products' do + product_for(other_company, sku: 'SKU-2') + + resolved = described_class::Scope.new(user, Product.unscoped).resolve + + expect(resolved).to be_empty + end + end +end diff --git a/spec/policies/stock_transfer_policy_spec.rb b/spec/policies/stock_transfer_policy_spec.rb new file mode 100644 index 00000000..bf96e2f2 --- /dev/null +++ b/spec/policies/stock_transfer_policy_spec.rb @@ -0,0 +1,80 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Una transferencia mueve unidades entre depósitos. `receive?` y `cancel?` son +# las dos acciones que cambian dónde están esas unidades, así que son las que +# tienen que mirar el tenant — `index?` y `create?` no reciben un registro y +# sólo pueden pedir un usuario. +RSpec.describe StockTransferPolicy do + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:other_company) { Company.create!(name: 'Rival', tax_id: '30-88888888-1') } + let(:user) { User.create!(email: 'a@acme.com', password: 'pass123', company: company) } + + def transfer_for(a_company) + Current.set(company_id: a_company.id) do + product = Product.create!(company: a_company, sku: "SKU-#{a_company.id}", name: 'Widget') + origin = warehouse_for(a_company, 'Central', '1900') + destination = warehouse_for(a_company, 'North', '1901') + + StockTransfer.create!(company: a_company, product: product, origin_warehouse: origin, + destination_warehouse: destination, quantity: 5, + dispatched_at: Time.current) + end + end + + def warehouse_for(a_company, name, zip) + Warehouse.create!(company: a_company, name: name, zip_code: zip, address: "Calle #{zip}") + end + + describe '#index?' do + it 'allows an authenticated user' do + expect(described_class.new(user, StockTransfer).index?).to be(true) + end + + it 'refuses without a user' do + expect(described_class.new(nil, StockTransfer).index?).to be(false) + end + end + + describe '#create?' do + it 'allows an authenticated user' do + expect(described_class.new(user, StockTransfer).create?).to be(true) + end + + it 'refuses without a user' do + expect(described_class.new(nil, StockTransfer).create?).to be(false) + end + end + + describe '#receive?' do + it 'allows a transfer of the same company' do + expect(described_class.new(user, transfer_for(company)).receive?).to be(true) + end + + it 'refuses a transfer of another company' do + expect(described_class.new(user, transfer_for(other_company)).receive?).to be(false) + end + end + + describe '#cancel?' do + it 'allows a transfer of the same company' do + expect(described_class.new(user, transfer_for(company)).cancel?).to be(true) + end + + it 'refuses a transfer of another company' do + expect(described_class.new(user, transfer_for(other_company)).cancel?).to be(false) + end + end + + describe 'Scope#resolve' do + it 'returns the transfers of the company of the user' do + mine = transfer_for(company) + transfer_for(other_company) + + resolved = described_class::Scope.new(user, StockTransfer.unscoped).resolve + + expect(resolved.pluck(:id)).to eq([mine.id]) + end + end +end diff --git a/spec/policies/warehouse_policy_spec.rb b/spec/policies/warehouse_policy_spec.rb new file mode 100644 index 00000000..73ba9f8f --- /dev/null +++ b/spec/policies/warehouse_policy_spec.rb @@ -0,0 +1,79 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Los depósitos son el recurso que TESIS-108 pasó a devolver entero (hasta el +# techo) para llenar los selects del frontend, así que su Scope es lo único que +# separa los nodos de una empresa de los de otra en esa respuesta. +RSpec.describe WarehousePolicy do + let(:company) { Company.create!(name: 'Acme', tax_id: '20-12345678-9') } + let(:other_company) { Company.create!(name: 'Rival', tax_id: '30-88888888-1') } + let(:user) { User.create!(email: 'a@acme.com', password: 'pass123', company: company) } + + def warehouse_for(a_company, name: 'Central') + Current.set(company_id: a_company.id) do + Warehouse.create!(company: a_company, name: name, zip_code: '1900', address: 'Calle 1') + end + end + + describe '#index?' do + it 'allows an authenticated user' do + expect(described_class.new(user, Warehouse).index?).to be(true) + end + + it 'refuses without a user' do + expect(described_class.new(nil, Warehouse).index?).to be(false) + end + end + + describe '#create?' do + it 'allows an authenticated user' do + expect(described_class.new(user, Warehouse).create?).to be(true) + end + + it 'refuses without a user' do + expect(described_class.new(nil, Warehouse).create?).to be(false) + end + end + + describe '#show?' do + it 'allows a warehouse of the same company' do + expect(described_class.new(user, warehouse_for(company)).show?).to be(true) + end + + it 'refuses a warehouse of another company' do + expect(described_class.new(user, warehouse_for(other_company)).show?).to be(false) + end + end + + describe '#update?' do + it 'allows a warehouse of the same company' do + expect(described_class.new(user, warehouse_for(company)).update?).to be(true) + end + + it 'refuses a warehouse of another company' do + expect(described_class.new(user, warehouse_for(other_company)).update?).to be(false) + end + end + + describe '#destroy?' do + it 'allows a warehouse of the same company' do + expect(described_class.new(user, warehouse_for(company)).destroy?).to be(true) + end + + it 'refuses a warehouse of another company' do + expect(described_class.new(user, warehouse_for(other_company)).destroy?).to be(false) + end + end + + describe 'Scope#resolve' do + it 'returns the warehouses of the company of the user' do + mine = warehouse_for(company) + warehouse_for(other_company, name: 'Ajeno') + + resolved = described_class::Scope.new(user, Warehouse.unscoped).resolve + + expect(resolved.pluck(:id)).to eq([mine.id]) + end + end +end diff --git a/spec/poros/application_poro_spec.rb b/spec/poros/application_poro_spec.rb new file mode 100644 index 00000000..1c0a5d14 --- /dev/null +++ b/spec/poros/application_poro_spec.rb @@ -0,0 +1,51 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# La base de todos los casos de uso (TESIS-93). +# +# Sólo aporta el atajo de clase `.call`, que evita el `new(...).call` en cada +# invocación. Nadie lo usaba —todos los llamadores escriben las dos partes—, así +# que era código sin un solo ejemplo: si se rompiera, se enteraría el primero +# que lo use. +RSpec.describe ApplicationPoro, type: :poro do + let(:greeter) do + Class.new(described_class) do + def initialize(name:, greeting: 'Hola') + super() + @name = name + @greeting = greeting + end + + def call = "#{@greeting}, #{@name}" + end + end + + let(:adder) do + Class.new(described_class) do + def initialize(first, second) + super() + @first = first + @second = second + end + + def call = @first + @second + end + end + + it 'builds the instance and runs it in one step' do + expect(greeter.call(name: 'Ana')).to eq('Hola, Ana') + end + + it 'forwards every keyword to the constructor' do + expect(greeter.call(name: 'Ana', greeting: 'Buen día')).to eq('Buen día, Ana') + end + + it 'returns what the instance returns, not the instance' do + expect(greeter.call(name: 'Ana')).to be_a(String) + end + + it 'forwards positional arguments too' do + expect(adder.call(2, 3)).to eq(5) + end +end diff --git a/spec/poros/catalog/outbound_sync_spec.rb b/spec/poros/catalog/outbound_sync_spec.rb index 72079777..9e9f01b1 100644 --- a/spec/poros/catalog/outbound_sync_spec.rb +++ b/spec/poros/catalog/outbound_sync_spec.rb @@ -122,6 +122,25 @@ def stub_channel(host, external_id, status: 200) expect { sync.call }.to raise_error(Integrations::AdapterExecutionError, /status 500/) end + # Un timeout no trae status HTTP: el mensaje tiene que nombrar el canal sin + # inventar un código. Sólo estaba probado el fallo con respuesta (TESIS-93). + it 'names a channel that failed without answering, and no status', :aggregate_failures do + stub_request(:put, 'https://api.ml.test/items/MLA-1').to_timeout + + expect { sync.call }.to raise_error(Integrations::AdapterExecutionError) do |error| + expect(error.message).to include('Mercado Libre') + expect(error.message).not_to include('status') + end + end + + it 'still propagates to the healthy channel when the other times out' do + stub_request(:put, 'https://api.ml.test/items/MLA-1').to_timeout + + suppress(Integrations::AdapterExecutionError) { sync.call } + + expect(a_request(:put, 'https://api.tn.test/items/TN-2')).to have_been_made.once + end + it 'aggregates every failing channel when they all fail' do stub_channel('api.tn.test', 'TN-2', status: 503) expect { sync.call }.to raise_error(Integrations::AdapterExecutionError, /2 of 2 channels/) diff --git a/spec/poros/integrations/http_adapter_spec.rb b/spec/poros/integrations/http_adapter_spec.rb index 8ec11771..beb30912 100644 --- a/spec/poros/integrations/http_adapter_spec.rb +++ b/spec/poros/integrations/http_adapter_spec.rb @@ -80,6 +80,35 @@ def run_adapter end end + # El verbo HTTP lo elige la plantilla del Service, que sólo valida presencia: + # nada impide guardar un verbo que el adaptador no sabe construir. Sin este + # ejemplo, el día que alguien agregue 'HEAD' a la plantilla el fallo aparece + # como un NoMethodError adentro de un job, en vez del error del adaptador que + # el motor de reintentos sabe manejar (TESIS-93). + describe 'an http_method the adapter does not know' do + before { service.update!(http_method: 'HEAD') } + + it 'raises AdapterExecutionError naming the verb', :aggregate_failures do + expect { run_adapter }.to raise_error(Integrations::AdapterExecutionError) do |error| + expect(error.message).to include('Andreani', 'HEAD') + end + end + + it 'does not reach the external API' do + request = stub_request(:any, /andreani/) + + suppress(Integrations::AdapterExecutionError) { run_adapter } + + expect(request).not_to have_been_made + end + + it 'carries the payload so the failure can be replayed', :aggregate_failures do + expect { run_adapter }.to raise_error(Integrations::AdapterExecutionError) do |error| + expect(error.payload).to eq({ customer_zip_code: '1900' }) + end + end + end + describe 'credential keys used as header names' do def adapter_for(credentials) integration.update!(credentials: credentials) diff --git a/spec/poros/orders/process_webhook_order_spec.rb b/spec/poros/orders/process_webhook_order_spec.rb index a494fc04..4576afa7 100644 --- a/spec/poros/orders/process_webhook_order_spec.rb +++ b/spec/poros/orders/process_webhook_order_spec.rb @@ -320,6 +320,65 @@ def create_log(body = payload) end end + # La carrera de dos workers sobre el mismo evento (TESIS-93). + # + # Hay dos defensas contra el duplicado y sólo la primera estaba probada. La + # validación de unicidad del modelo atrapa el reintento normal —el proveedor + # reentrega el webhook y el segundo worker lo ve ya registrado—, pero entre + # esa consulta y el INSERT hay una ventana: si los dos workers la atraviesan a + # la vez, los dos pasan la validación y el que llega segundo choca contra el + # índice único (company_id, external_order_id). + # + # Eso es lo que se simula acá: el `find_by` de entrada devuelve `nil` (estamos + # dentro de la ventana) y el INSERT falla con la violación del índice, que es + # lo que contesta Postgres en producción. Lo que se verifica es qué hace el + # worker que pierde: devolver la venta ya registrada, no `nil` ni una segunda + # orden. + context 'when another worker registered the same sale first' do + subject(:losing_worker) { described_class.new(webhook_log: create_log) } + + let(:payload) { order_payload(items: [line('MLA-1', 2, 1500.5)]) } + let(:winner) { Order.find_by!(external_order_id: 'ML-1001') } + + before do + publish('SKU-1', 'MLA-1') + described_class.new(webhook_log: create_log).call + winner + + calls = 0 + allow(Order).to receive(:find_by).and_wrap_original do |original, *args| + calls += 1 + calls == 1 ? nil : original.call(*args) + end + allow(Order).to receive(:create!).and_raise(ActiveRecord::RecordNotUnique.new(unique_violation)) + end + + # El nombre del índice sale de la constante del PORO y no escrito a mano: si + # alguien renombra el índice, el guard y este stub cambian juntos en vez de + # desincronizarse en silencio y dejar el ejemplo probando otra cosa. + def unique_violation + %(PG::UniqueViolation: duplicate key value violates unique constraint "#{described_class::ORDERS_UNIQUE_INDEX}") + end + + it 'returns the order the other worker registered' do + expect(losing_worker.call).to eq(winner) + end + + it 'does not register the sale twice' do + expect { losing_worker.call }.not_to change(Order, :count) + end + + it 'does not deduct the stock a second time' do + expect { losing_worker.call }.not_to(change { stock_of('SKU-1') }) + end + + it 'marks its own log as processed, not failed' do + log_of_the_loser = losing_worker.instance_variable_get(:@log) + + expect { losing_worker.call }.to change { log_of_the_loser.reload.status }.to('processed') + end + end + context 'when the channel reports a status the OMS does not know' do before { publish('SKU-1', 'MLA-1') } diff --git a/spec/poros/shipments/poll_tracking_status_spec.rb b/spec/poros/shipments/poll_tracking_status_spec.rb index c3f9c9ac..2a390062 100644 --- a/spec/poros/shipments/poll_tracking_status_spec.rb +++ b/spec/poros/shipments/poll_tracking_status_spec.rb @@ -206,6 +206,28 @@ def poll(ids = [shipment.id]) expect(poll([shipment.id, other.id]).size).to eq(2) end + # La consulta masiva pregunta por varios envíos de una vez, así que cuando + # se cae se cae para todos. El camino de un envío por request ya estaba + # probado contra el timeout; este no, y es el que decide si el barrido + # sigue o se levanta una excepción con la ronda entera adentro (TESIS-93). + context 'when the batch query fails' do + before { stub_request(:get, batch_url).to_timeout } + + it 'returns nothing instead of raising' do + expect(poll([shipment.id, other.id])).to be_empty + end + + it 'registers no movement' do + expect { poll([shipment.id, other.id]) }.not_to change(ShipmentEvent, :count) + end + + it 'leaves the shipments as they were' do + poll([shipment.id, other.id]) + + expect(shipment.reload.status).to eq('ready_to_ship') + end + end + # Los números los mandamos nosotros: un elemento que no es de ninguno es la # pista de una plantilla mal configurada, y tiene que quedar en el log. it 'logs the element that matches no shipment of the query' do diff --git a/spec/poros/shipments/quote_shipment_spec.rb b/spec/poros/shipments/quote_shipment_spec.rb index 54ed89a6..383289a6 100644 --- a/spec/poros/shipments/quote_shipment_spec.rb +++ b/spec/poros/shipments/quote_shipment_spec.rb @@ -105,6 +105,56 @@ def integrate(service) end end + # Los días son opcionales: hay couriers que cotizan el precio y no prometen + # plazo. La opción tiene que llegar igual, con el plazo vacío, en vez de + # descartarse o de viajar con un cero que el front mostraría como "0 días" + # (TESIS-93). + context 'when a courier quotes a price but no delivery time' do + before do + integrate(quoting_service('Fast', 'https://fast.test/rates')) + stub_request(:post, 'https://fast.test/rates') + .to_return(status: 200, body: { precio: 2500.0 }.to_json) + end + + it 'keeps the option' do + expect(quotes.length).to eq(1) + end + + it 'leaves the estimate empty instead of inventing a zero' do + expect(quotes.first[:estimated_days]).to be_nil + end + + it 'still carries the price' do + expect(quotes.first[:shipping_cost]).to eq(BigDecimal('2500.0')) + end + end + + # Un costo que no es un número: el courier contestó 'gratis', o un texto de + # error en el campo del precio. La opción se descarta en vez de tumbar la + # cotización entera, que es lo mismo que se hace cuando no manda precio. + context 'when a courier answers a price that is not a number' do + before do + integrate(quoting_service('Fast', 'https://fast.test/rates')) + integrate(quoting_service('Cheap', 'https://cheap.test/rates')) + stub_request(:post, 'https://fast.test/rates') + .to_return(status: 200, body: { precio: 'gratis', dias: 1 }.to_json) + stub_request(:post, 'https://cheap.test/rates') + .to_return(status: 200, body: { precio: 1800.0, dias: 5 }.to_json) + end + + it 'drops that option' do + expect(quotes.pluck(:provider_name)).to eq(['Cheap']) + end + + it 'still returns the courier that answered a number' do + expect(quotes.length).to eq(1) + end + + it 'does not raise' do + expect { quotes }.not_to raise_error + end + end + context 'with a courier template that only knows how to dispatch' do before do dispatch = Service.create!(service_name: 'Andreani', type: 'courier', http_method: 'POST', diff --git a/spec/poros/shipments/translate_tracking_payload_spec.rb b/spec/poros/shipments/translate_tracking_payload_spec.rb index 4bc59471..477002bc 100644 --- a/spec/poros/shipments/translate_tracking_payload_spec.rb +++ b/spec/poros/shipments/translate_tracking_payload_spec.rb @@ -104,6 +104,25 @@ expect(result[:occurred_at]).to be_nil end + # 'no-es-una-fecha-valida' devuelve nil sin levantar nada, así que el rescue + # quedaba sin ejercitar. Estos dos sí lo pisan: una fecha imposible levanta + # ArgumentError y un objeto levanta TypeError. Un evento con la fecha rota + # tiene que entrar igual —el caso de uso tiene fallback— y no tumbar el job + # (TESIS-93). + it 'returns nil for an impossible date instead of raising' do + payload = { 'evento' => { 'fecha' => '2026-13-45T99:99:99' } } + result = described_class.new(service: service, payload: payload).call + expect(result[:occurred_at]).to be_nil + end + + it 'keeps the rest of the event when the date is broken', :aggregate_failures do + payload = { 'evento' => { 'fecha' => '2026-13-45T99:99:99', 'estado' => 'Entregado' } } + result = described_class.new(service: service, payload: payload).call + + expect(result[:occurred_at]).to be_nil + expect(result).to have_key(:external_status) + end + it 'returns nil when occurred_at is absent from the payload' do result = described_class.new(service: service, payload: {}).call expect(result[:occurred_at]).to be_nil diff --git a/spec/requests/admin/resources_spec.rb b/spec/requests/admin/resources_spec.rb new file mode 100644 index 00000000..ee39f09d --- /dev/null +++ b/spec/requests/admin/resources_spec.rb @@ -0,0 +1,256 @@ +# frozen_string_literal: true + +require 'rails_helper' + +# Barrido del panel de administración (TESIS-93). +# +# Los recursos de Avo son bloques `fields` que no se evalúan hasta que alguien +# abre la página: un campo mal nombrado, una asociación que ya no existe o un +# `self.title` que apunta a un método borrado no rompen ningún test de la suite +# —rompen la pantalla, y recién cuando se entra—. Hasta esta card, de los ocho +# recursos sólo se abría el de servicios. +# +# Esto abre el listado y el detalle de cada uno con una fila de verdad. No +# verifica cómo se ve: verifica que la página se arme, que es la parte que hoy +# nadie miraba y la que decide si la demo del panel funciona. +RSpec.describe 'Admin panel resources (Avo)', type: :request do + let(:admin_user) { AdminUser.create!(email: 'admin@backoffice.com', password: 'admin123') } + let(:company) { Company.create!(name: 'Distribuidora Norte', tax_id: '30-11111111-1') } + + # Una fila por tabla, todas de la misma empresa. El tenant se setea a mano: + # CompanyScoped toma el company_id del contexto y acá no hay request de API + # que lo haya puesto. + # + # Memoizado: cada ejemplo abre varias páginas sobre las MISMAS filas. + let(:rows) { seed } + + def seed + Current.company_id = company.id + + warehouse = Warehouse.create!(company: company, name: 'Central', zip_code: '1900', + address: 'Av. 1') + product = Product.create!(company: company, sku: 'NOR-001', name: 'Celular') + stock = Stock.create!(product: product, warehouse: warehouse, quantity: 10) + user = User.create!(email: 'operador@norte.test', password: 'password123', company: company) + integration = courier_integration(company: company) + order = Order.create!(company: company, customer_name: 'Juan Pérez', status: 'paid', + external_order_id: 'ML-1001') + shipment = Shipment.create!(company: company, company_integration: integration, order: order, + tracking_number: 'AND-123', status: 'in_transit') + + { companies: company, warehouses: warehouse, products: product, stocks: stock, users: user, + company_integrations: integration, orders: order, shipments: shipment } + ensure + Current.reset + end + + # Login por el formulario y no con el helper `sign_in`: ese helper usa + # `Warden.on_next_request`, que vale para UN request. Estos ejemplos abren + # ocho páginas seguidas, así que necesitan la sesión de verdad —la misma que + # usa un administrador— o la segunda página ya contesta un redirect al login. + before do + post '/admin/sign_in', + params: { admin_user: { email: admin_user.email, password: 'admin123' } } + end + + after { Current.reset } + + describe 'every resource of the panel' do + it 'renders the listing', :aggregate_failures do + rows.each_key do |resource| + get "/admin/resources/#{resource}" + + expect(response).to have_http_status(:ok), "el listado de #{resource} no abre" + end + end + + it 'renders the detail of a row', :aggregate_failures do + rows.each do |resource, record| + get "/admin/resources/#{resource}/#{record.id}" + + expect(response).to have_http_status(:ok), "el detalle de #{resource} no abre" + end + end + + it 'renders the create form', :aggregate_failures do + rows.each_key do |resource| + get "/admin/resources/#{resource}/new" + + expect(response).to have_http_status(:ok), "el alta de #{resource} no abre" + end + end + + it 'renders the edit form', :aggregate_failures do + rows.each do |resource, record| + get "/admin/resources/#{resource}/#{record.id}/edit" + + expect(response).to have_http_status(:ok), "el formulario de #{resource} no abre" + end + end + end + + # Los diccionarios de una plantilla se editan como JSON. La columna es jsonb, + # así que puede contener un objeto —lo normal— o un string, y el panel tiene + # que mostrar los dos: el string tal cual, el objeto formateado. Sólo se + # ejercitaba el segundo (TESIS-93). + describe 'the JSON dictionaries of a service' do + let(:service) do + Service.create!(service_name: 'Andreani', type: 'courier', http_method: 'POST', + uri: 'https://api.andreani.test/ordenes', + request_mapper: { 'destino.cp' => 'customer_zip_code' }) + end + + # La validación del modelo no deja guardar un diccionario que no sea objeto, + # así que la única forma de tener esa fila es saltearla —que es justo lo que + # pudo pasar con una fila vieja o un import—. + def store_bare_string_mapper + service.update_column(:request_mapper, 'un-string-suelto') # rubocop:disable Rails/SkipsModelValidations + end + + it 'shows a dictionary stored as an object' do + get "/admin/resources/services/#{service.id}" + + expect(response.body).to include('customer_zip_code') + end + + it 'opens the form of a dictionary that holds a bare string' do + store_bare_string_mapper + + get "/admin/resources/services/#{service.id}/edit" + + expect(response).to have_http_status(:ok) + end + + it 'shows that string as it is, without formatting it as JSON' do + store_bare_string_mapper + + get "/admin/resources/services/#{service.id}/edit" + + expect(response.body).to include('un-string-suelto') + end + end + + # El buscador de cada recurso. Esto es lo que destapó que las tres búsquedas + # del panel —empresas, productos y usuarios— levantaban NameError: el lambda + # usaba `search_term` y Avo 4 entrega lo tipeado como `q`. Como no había + # ningún ejemplo que buscara, el panel se veía sano hasta que alguien + # escribía en la caja (TESIS-93). + describe 'the search box of each resource' do + it 'answers instead of raising', :aggregate_failures do + rows + + %w[companies products users].each do |resource| + get "/admin/resources/#{resource}?q=zzz-no-existe" + + expect(response).to have_http_status(:ok), "la búsqueda de #{resource} no anda" + end + end + + it 'finds a product by a piece of its name' do + rows + + get '/admin/resources/products?q=Celu' + + expect(response.body).to include('NOR-001') + end + + it 'finds a product by its sku' do + rows + + get '/admin/resources/products?q=NOR-001' + + expect(response.body).to include('Celular') + end + + it 'finds a company by its tax id' do + rows + + get '/admin/resources/companies?q=30-11111111-1' + + expect(response.body).to include('Distribuidora Norte') + end + + it 'finds a user by a piece of its email' do + rows + + get '/admin/resources/users?q=operador' + + expect(response.body).to include('operador@norte.test') + end + + it 'leaves out what does not match' do + rows + + get '/admin/resources/products?q=zzz-no-existe' + + expect(response.body).not_to include('NOR-001') + end + end + + # Un envío recién creado: el número de seguimiento lo devuelve el courier al + # despachar, así que hasta entonces la fila no lo tiene. + def shipment_without_tracking + Current.company_id = company.id + order = Order.create!(company: company, customer_name: 'Ana', status: 'paid') + Shipment.create!(company: company, order: order, status: 'pending', + company_integration: rows[:company_integrations]) + end + + # El título de cada fila es un método del modelo, no una columna. Son los que + # arma Avo para el breadcrumb y para los selects de las asociaciones: si uno + # devuelve nil o levanta, la pantalla que lo muestra se cae. + describe 'the label each row shows in the panel' do + it 'names an order by its customer and external id' do + order = rows[:orders] + + expect(order.display_name).to eq('Juan Pérez (ML-1001)') + end + + # `customer_name` es NOT NULL en la base y tiene validación, así que una + # orden guardada siempre lo trae: el fallback es para la fila todavía sin + # guardar, que es la que el panel muestra en el formulario de alta. + it 'falls back to the id on an order that is not saved yet' do + expect(Order.new.display_name).to eq('Order #') + end + + it 'names an order without external id by its customer alone' do + Current.company_id = company.id + manual = Order.create!(company: company, customer_name: 'Ana', status: 'pending') + + expect(manual.display_name).to eq('Ana') + end + + it 'names a stock row by its product and warehouse' do + expect(rows[:stocks].display_name).to eq('Celular @ Central') + end + + it 'names a shipment by its tracking number' do + expect(rows[:shipments].display_name).to eq('AND-123') + end + + # Un envío recién creado todavía no tiene número: el courier lo devuelve al + # despachar. Hasta entonces el panel lo nombra por su id. + it 'falls back to the id when the shipment has no tracking number yet' do + pending_shipment = shipment_without_tracking + + expect(pending_shipment.display_name).to eq("Shipment ##{pending_shipment.id}") + end + + # Las filas todavía sin asociar: el formulario de alta las muestra antes de + # que el administrador elija empresa o servicio. El `&.` de cada etiqueta es + # lo que evita que esa pantalla se caiga. + it 'survives an integration that has no company nor service yet' do + expect(CompanyIntegration.new.display_name).to eq(' ↔ ') + end + + it 'survives a stock row that has no product nor warehouse yet' do + expect(Stock.new.display_name).to eq(' @ ') + end + + it 'names an integration by the company and the service' do + integration = rows[:company_integrations] + + expect(integration.display_name).to eq("Distribuidora Norte \u2194 Andreani") + end + end +end diff --git a/spec/requests/api/v1/failed_events_spec.rb b/spec/requests/api/v1/failed_events_spec.rb index d61dc40b..6a97738f 100644 --- a/spec/requests/api/v1/failed_events_spec.rb +++ b/spec/requests/api/v1/failed_events_spec.rb @@ -45,6 +45,29 @@ expect(response.parsed_body['meta']).to eq('page' => 1, 'per_page' => 2, 'total' => 4) end + # El filtro por tipo es el que usa el panel de la DLQ para separar las + # ingestas de webhook de los requests salientes. Estaba sin ejercitar: la + # suite pasaba igual si la línea no filtraba nada (TESIS-93). + it 'filters by event type' do + outbound = create_event(company, event_type: 'integrations.outbound_sync') + create_event(company, event_type: 'orders.ingestion') + + get '/api/v1/failed-events', params: { event_type: 'integrations.outbound_sync' }, + headers: headers + + expect(response.parsed_body['data'].pluck('id')).to contain_exactly(outbound.id) + end + + it 'combines the type filter with the status one' do + create_event(company, event_type: 'orders.ingestion', status: :pending) + dead = create_event(company, event_type: 'orders.ingestion', status: :dead) + + get '/api/v1/failed-events', params: { event_type: 'orders.ingestion', status: 'dead' }, + headers: headers + + expect(response.parsed_body['data'].pluck('id')).to contain_exactly(dead.id) + end + it 'ignores an unknown status filter' do get '/api/v1/failed-events', params: { status: 'exploded' }, headers: headers diff --git a/spec/requests/api/v1/integrations_spec.rb b/spec/requests/api/v1/integrations_spec.rb index 3305f018..373693de 100644 --- a/spec/requests/api/v1/integrations_spec.rb +++ b/spec/requests/api/v1/integrations_spec.rb @@ -95,6 +95,33 @@ def listed expect(raw).not_to include('SECRET-TOKEN') end + # `credentials` tiene que ser un objeto. Un string o un array pasan el + # `require` —que sólo mira que no venga vacío— y reventarían recién adentro + # del cifrado, como 500. El guard los corta antes; nadie lo ejercitaba + # (TESIS-93). + context 'when credentials is not an object' do + it 'rejects a string instead of failing inside the encryption' do + put "/api/v1/integrations/#{service.id}", + params: { credentials: 'ACCESS-TOKEN' }, headers: headers, as: :json + + expect(response).to have_http_status(:bad_request) + end + + it 'rejects an array' do + put "/api/v1/integrations/#{service.id}", + params: { credentials: ['ACCESS-TOKEN'] }, headers: headers, as: :json + + expect(response).to have_http_status(:bad_request) + end + + it 'stores nothing' do + expect do + put "/api/v1/integrations/#{service.id}", + params: { credentials: 'ACCESS-TOKEN' }, headers: headers, as: :json + end.not_to change(CompanyIntegration, :count) + end + end + it 'returns 404 for an unknown service' do put '/api/v1/integrations/999999', params: payload, headers: headers, as: :json expect(response).to have_http_status(:not_found) diff --git a/spec/requests/api/v1/order_updates_spec.rb b/spec/requests/api/v1/order_updates_spec.rb index 07658aaa..9fe7fc4a 100644 --- a/spec/requests/api/v1/order_updates_spec.rb +++ b/spec/requests/api/v1/order_updates_spec.rb @@ -103,6 +103,49 @@ def current_version = Orders::OrderVersion.new(order: Order.find(order.id)).call end end + # El `id` de una línea que se conserva decide si la línea se actualiza o si el + # pedido se rechaza: con un id que no existe se estaría editando la orden de + # otro. `positive_integer` es lo que lo filtra, y sus dos caminos —el string + # de dígitos que sí vale, y todo lo demás— no los ejercitaba nadie (TESIS-93). + describe 'the id of a line that is kept' do + it 'accepts it as a string of digits, like a form would send it' do + put_order(items: [{ id: line.id.to_s, quantity: 6 }]) + + expect(response).to have_http_status(:ok) + end + + it 'moves the stock as if the id had come as a number' do + put_order(items: [{ id: line.id.to_s, quantity: 6 }]) + + expect(stock_left).to eq(18) + end + + it 'rejects a string that is not a number', :aggregate_failures do + put_order(items: [{ id: 'abc', quantity: 6 }]) + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to include('id must be a positive integer') + end + + it 'rejects a zero' do + put_order(items: [{ id: 0, quantity: 6 }]) + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'rejects a decimal instead of truncating it' do + put_order(items: [{ id: 2.5, quantity: 6 }]) + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'leaves the stock untouched when the id is rejected' do + put_order(items: [{ id: 'abc', quantity: 6 }]) + + expect(stock_left).to eq(20) + end + end + describe 'the version' do it 'accepts the ETag the detail returned' do get "/api/v1/orders/#{order.id}", headers: headers diff --git a/spec/requests/api/v1/orders_spec.rb b/spec/requests/api/v1/orders_spec.rb index 43d1ffad..9acd2ca8 100644 --- a/spec/requests/api/v1/orders_spec.rb +++ b/spec/requests/api/v1/orders_spec.rb @@ -416,6 +416,34 @@ def order_of_another_company expect(response).to have_http_status(:unprocessable_content) end + # `items: ['SKU-1']` pasa el chequeo de Array y revienta recién en + # `item.permit`, como 500. El guard lo corta con 422 y nadie lo + # ejercitaba (TESIS-93). + it 'rejects when an item is not an object', :aggregate_failures do + post '/api/v1/orders', + params: { order: { customer_name: 'X', items: ['SKU-1'] } }, + headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to include('product_id', 'quantity', 'unit_price') + end + + it 'rejects when only one of the items is not an object' do + post '/api/v1/orders', + params: { order: { customer_name: 'X', items: [default_item, 42] } }, + headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + end + + it 'creates nothing when an item is not an object' do + expect do + post '/api/v1/orders', + params: { order: { customer_name: 'X', items: ['SKU-1'] } }, + headers: headers, as: :json + end.not_to change(Order, :count) + end + it 'rejects when items is empty' do post '/api/v1/orders', params: { order: { customer_name: 'X', items: [] } }, diff --git a/spec/requests/api/v1/products_spec.rb b/spec/requests/api/v1/products_spec.rb index 0376d57c..2b6682c2 100644 --- a/spec/requests/api/v1/products_spec.rb +++ b/spec/requests/api/v1/products_spec.rb @@ -886,6 +886,32 @@ def put_stock(quantity) # para ejercitar el handler `render_constraint_violation` de # ApplicationController con la excepción que dispararía PostgreSQL si esa # validación no existiera. + # La otra mitad del mismo handler: cualquier CHECK que no sea el de stock + # contesta un mensaje genérico a propósito. El de PostgreSQL nombra tabla y + # restricción, y eso no tiene por qué salir de la API. Sólo estaba probada + # la rama específica (TESIS-93). + context 'when the violated constraint is not the one about stock' do + it 'returns 422 with a generic message', :aggregate_failures do + violation = ActiveRecord::CheckViolation.new('PG::CheckViolation: ERROR: new row for relation "orders" violates check constraint "orders_total_amount_non_negative"') + allow(Products::UpdateProduct).to receive(:new).and_raise(violation) + + patch "/api/v1/products/#{product.id}", params: { product: { name: 'X' } }, headers: headers, as: :json + + expect(response).to have_http_status(:unprocessable_content) + expect(response.parsed_body['error']).to eq('invalid data: a database constraint was violated') + end + + it 'does not leak the table nor the name of the constraint', :aggregate_failures do + violation = ActiveRecord::CheckViolation.new('PG::CheckViolation: ERROR: new row for relation "orders" violates check constraint "orders_total_amount_non_negative"') + allow(Products::UpdateProduct).to receive(:new).and_raise(violation) + + patch "/api/v1/products/#{product.id}", params: { product: { name: 'X' } }, headers: headers, as: :json + + expect(response.parsed_body['error']).not_to include('orders') + expect(response.parsed_body['error']).not_to include('orders_total_amount_non_negative') + end + end + context 'when the underlying update raises a database CHECK violation' do it 'returns 422 with a stock-specific message', :aggregate_failures do violation = ActiveRecord::CheckViolation.new('PG::CheckViolation: ERROR: new row for relation "stocks" violates check constraint "stocks_quantity_non_negative"') diff --git a/spec/requests/api/v1/shipment_dispatches_spec.rb b/spec/requests/api/v1/shipment_dispatches_spec.rb index 61d17175..28b2e44d 100644 --- a/spec/requests/api/v1/shipment_dispatches_spec.rb +++ b/spec/requests/api/v1/shipment_dispatches_spec.rb @@ -137,6 +137,28 @@ def dispatch_shipment(id: shipment.id, integration_id: integration.id, expect(response.parsed_body['error']).to include('Service Unavailable') end + # Las otras dos formas del cuerpo de un tercero. Con el cuerpo vacío no hay + # nada que propagar y el mensaje tiene que quedar en el del adaptador —no en + # un ": " colgando—; con un JSON que no es objeto se propaga tal cual, porque + # no hay campo del que sacar el motivo. Ninguna de las dos se probaba + # (TESIS-93). + it 'keeps the adapter message when the courier answers with an empty body', + :aggregate_failures do + stub_request(:post, 'https://andreani.test/ordenes').to_return(status: 503, body: '') + dispatch_shipment + + expect(response.parsed_body['error']).to include('503') + expect(response.parsed_body['error']).not_to end_with(': ') + end + + it 'propagates a JSON body that is not an object' do + stub_request(:post, 'https://andreani.test/ordenes') + .to_return(status: 422, body: '["codigo postal invalido"]') + dispatch_shipment + + expect(response.parsed_body['error']).to include('codigo postal invalido') + end + it 'returns 502 when the courier answers without a tracking number' do stub_courier(body: { etiqueta: 'https://l.test/1.pdf' }) dispatch_shipment diff --git a/spec/requests/api/v1/stock_transfers_spec.rb b/spec/requests/api/v1/stock_transfers_spec.rb index 8fb2427a..a5870cdd 100644 --- a/spec/requests/api/v1/stock_transfers_spec.rb +++ b/spec/requests/api/v1/stock_transfers_spec.rb @@ -93,6 +93,37 @@ def dispatch_one(quantity: 4) end end + def dispatch_another_product + other = Product.create!(company: company, sku: 'A-002', name: 'Beta') + Stock.create!(product: other, warehouse: origin, quantity: 10) + Catalog::DispatchTransfer.new(company: company, product: other, origin_warehouse: origin, + destination_warehouse: destination, quantity: 2).call + end + + # El filtro por producto es el que alimenta el "+N Incoming" de la ficha de + # catálogo: sin él la ficha mostraría las unidades en vuelo de todos los + # productos. Estaba sin ejercitar (TESIS-93). + describe 'GET /api/v1/stock-transfers filtered by product' do + it 'returns only the transfers of the product asked for', :aggregate_failures do + mine = dispatch_one + dispatch_another_product + + get '/api/v1/stock-transfers', params: { product_id: product.id }, headers: headers + + expect(response.parsed_body['data'].pluck('id')).to contain_exactly(mine.id) + expect(StockTransfer.count).to eq(2) + end + + it 'combines the product filter with the status one' do + dispatch_one + + get '/api/v1/stock-transfers', params: { product_id: product.id, status: 'received' }, + headers: headers + + expect(response.parsed_body['data']).to be_empty + end + end + describe 'POST /api/v1/stock-transfers/:id/receive' do it 'settles the transfer into the destination', :aggregate_failures do transfer = dispatch_one diff --git a/spec/requests/api/webhooks/integrations_spec.rb b/spec/requests/api/webhooks/integrations_spec.rb index 5cd009a7..fd765945 100644 --- a/spec/requests/api/webhooks/integrations_spec.rb +++ b/spec/requests/api/webhooks/integrations_spec.rb @@ -96,6 +96,53 @@ def post_broken_body end end + # Las otras dos formas que puede tomar un body que no es un objeto. Un + # proveedor que hace ping con el cuerpo vacío, y uno que manda un JSON + # válido que no es ni objeto ni lista. Ninguna se probaba, y las dos tienen + # que terminar en un log guardado: el evento no se pierde (TESIS-93). + context 'when the body is empty' do + def post_empty_body + post "/api/webhooks/integrations/#{integration.id}", + params: '', headers: { 'CONTENT_TYPE' => 'application/json' } + end + + it 'accepts it' do + post_empty_body + expect(response).to have_http_status(:accepted) + end + + it 'files the event with an empty payload instead of failing' do + post_empty_body + expect(WebhookLog.unscoped.last.payload).to eq({}) + end + end + + context 'when the body is valid JSON but not an object' do + def post_scalar_body + post "/api/webhooks/integrations/#{integration.id}", + params: '"solo-un-string"', headers: { 'CONTENT_TYPE' => 'application/json' } + end + + it 'accepts it' do + post_scalar_body + expect(response).to have_http_status(:accepted) + end + + it 'keeps it raw, the same as an unparseable body' do + post_scalar_body + expect(WebhookLog.unscoped.last.payload).to eq('raw' => '"solo-un-string"') + end + end + + context 'when the body is a JSON array' do + it 'stores the list as it came' do + post "/api/webhooks/integrations/#{integration.id}", + params: '[{"id":1}]', headers: { 'CONTENT_TYPE' => 'application/json' } + + expect(WebhookLog.unscoped.last.payload).to eq([{ 'id' => 1 }]) + end + end + context 'when the integration does not exist' do it 'returns 404' do post_webhook(999_999) diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index dc507470..aec19eb3 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -13,6 +13,37 @@ # it. # # See https://rubydoc.info/gems/rspec-core/RSpec/Core/Configuration +# La medición de cobertura arranca antes que cualquier otra cosa: SimpleCov +# sólo ve los archivos que se cargan después de él, y `rails_helper` carga la +# aplicación entera (TESIS-93). +require 'simplecov' + +SimpleCov.start 'rails' do + enable_coverage :branch + + skip '/spec/' + skip '/config/' + skip '/db/' + + group 'POROs', 'app/poros' + group 'Policies', 'app/policies' + group 'Serializers', 'app/serializers' + group 'Concerns', %w[app/controllers/concerns app/models/concerns] + + # Un piso, no una meta. Existe para que un PR que deje código nuevo sin un + # solo ejemplo ponga la suite en rojo, no para perseguir el 100%: hoy la + # medición está en 99.88% de líneas y 100% de ramas, así que el margen es el + # que hace falta para que el piso avise y no moleste. + # + # Sólo se evalúa sobre la suite entera. Una corrida parcial —un archivo, un + # ejemplo suelto, `--only-failures`— mide una fracción de la aplicación y + # siempre quedaría debajo del piso: terminaría en error por algo que no es un + # test rojo, que es la forma más rápida de acostumbrar a todos a ignorar el + # exit code. El CI y el hook de pre-push corren la suite completa, y son los + # dos lugares donde el piso hace falta. + minimum_coverage line: 99, branch: 95 if ENV['CI'] || ENV['COVERAGE_FLOOR'] +end + RSpec.configure do |config| # rspec-expectations config goes here. You can use an alternate # assertion/expectation library such as wrong or the stdlib/minitest