From d16cd8c84eaba8424a8ffefb431fb6d913e3616b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 22 Nov 2024 16:54:58 +1100 Subject: [PATCH 01/11] Amend backorder completely Update every single order line to reflect local orders and stock levels. New cases supported: * Add lines for orders created by an admin. * Create backorder line after increase of local line item quantity. * Adjust local stock after variant has been removed from order cycle. --- app/jobs/amend_backorder_job.rb | 139 +++++++++++++++++--------- spec/jobs/amend_backorder_job_spec.rb | 30 +++++- 2 files changed, 114 insertions(+), 55 deletions(-) diff --git a/app/jobs/amend_backorder_job.rb b/app/jobs/amend_backorder_job.rb index 1e6ffd7854..8e5687249b 100644 --- a/app/jobs/amend_backorder_job.rb +++ b/app/jobs/amend_backorder_job.rb @@ -1,8 +1,9 @@ # frozen_string_literal: true -# When orders are cancelled, we need to amend +require 'open_food_network/order_cycle_permissions' + +# When orders are created, adjusted or cancelled, we need to amend # an existing backorder as well. -# We're not dealing with line item changes just yet. class AmendBackorderJob < ApplicationJob sidekiq_options retry: 0 @@ -15,81 +16,119 @@ class AmendBackorderJob < ApplicationJob # The following is a mix of the BackorderJob and the CompleteBackorderJob. # TODO: Move the common code into a re-usable service class. def amend_backorder(order) - order_cycle = order.order_cycle - distributor = order.distributor - user = distributor.owner - items = backorderable_items(order) + variants = distributed_linked_variants(order) + return unless variants.any? - return if items.empty? + user = order.distributor.owner + order_cycle = order.order_cycle # We are assuming that all variants are linked to the same wholesale # shop and its catalog: - reference_link = items[0].variant.semantic_links[0].semantic_id + reference_link = variants[0].semantic_links[0].semantic_id urls = FdcUrlBuilder.new(reference_link) orderer = FdcBackorderer.new(user, urls) + broker = FdcOfferBroker.new(user, urls) backorder = orderer.find_open_order(order) - variants = order_cycle.variants_distributed_by(distributor) - adjust_quantities(order_cycle, user, backorder, urls, variants) + updated_lines = update_order_lines(backorder, order_cycle, variants, broker, orderer) + unprocessed_lines = backorder.lines.to_set - updated_lines + cancel_stale_lines(unprocessed_lines, order, broker) + + # Clean up empty lines: + backorder.lines.reject! { |line| line.quantity.zero? } FdcBackorderer.new(user, urls).send_order(backorder) end - # Check if we have enough stock to reduce the backorder. - # - # Our local stock can increase when users cancel their orders. - # But stock levels could also have been adjusted manually. So we review all - # quantities before finalising the order. - def adjust_quantities(order_cycle, user, order, urls, variants) - broker = FdcOfferBroker.new(user, urls) + def update_order_lines(backorder, order_cycle, variants, broker, orderer) + variants.map do |variant| + link = variant.semantic_links[0].semantic_id + solution = broker.best_offer(link) + line = orderer.find_or_build_order_line(backorder, solution.offer) + if variant.on_demand + adjust_stock(variant, solution, line) + else + aggregate_final_quantities(order_cycle, line, variant, solution) + end - order.lines.each do |line| - line.quantity = line.quantity.to_i + line + end + end + + def cancel_stale_lines(unprocessed_lines, order, broker) + managed_variants = managed_linked_variants(order) + unprocessed_lines.each do |line| + wholesale_quantity = line.quantity.to_i wholesale_product_id = line.offer.offeredItem.semanticId transformation = broker.wholesale_to_retail(wholesale_product_id) - linked_variant = variants.linked_to(transformation.retail_product_id) + linked_variant = managed_variants.linked_to(transformation.retail_product_id) - # Assumption: If a transformation is present then we only sell the retail - # variant. If that can't be found, it was deleted and we'll ignore that - # for now. - next if linked_variant.nil? + if linked_variant.nil? + transformation.factor = 1 + linked_variant = managed_variants.linked_to(wholesale_product_id) + end - # Find all line items for this order cycle - # Update quantity accordingly - if linked_variant.on_demand - release_superfluous_stock(line, linked_variant, transformation) - else - aggregate_final_quantities(order_cycle, line, linked_variant, transformation) + # Adjust stock level back, we're not going to order this one. + if linked_variant&.on_demand + retail_quantity = wholesale_quantity * transformation.factor + linked_variant.on_hand -= retail_quantity + end + + # We don't have any active orders for this + line.quantity = 0 + end + end + + def adjust_stock(variant, solution, line) + if variant.on_hand.negative? + needed_quantity = -1 * variant.on_hand # We need to replenish it. + + # The number of wholesale packs we need to order to fulfill the + # needed quantity. + # For example, we order 2 packs of 12 cans if we need 15 cans. + wholesale_quantity = (needed_quantity.to_f / solution.factor).ceil + + # The number of individual retail items we get with the wholesale order. + # For example, if we order 2 packs of 12 cans, we will get 24 cans + # and we'll account for that in our stock levels. + retail_quantity = wholesale_quantity * solution.factor + + line.quantity = line.quantity.to_i + wholesale_quantity + variant.on_hand += retail_quantity + else + # Note that a division of integers dismisses the remainder, like `floor`: + wholesale_items_contained_in_stock = variant.on_hand / solution.factor + + # But maybe we didn't actually order that much: + deductable_quantity = [line.quantity, wholesale_items_contained_in_stock].min + + if deductable_quantity.positive? + line.quantity -= deductable_quantity + + retail_stock_change = deductable_quantity * solution.factor + variant.on_hand -= retail_stock_change end end - - # Clean up empty lines: - order.lines.reject! { |line| line.quantity.zero? } end - # We look at all linked variants. - def backorderable_items(order) - order.line_items.select do |item| - # TODO: scope variants to hub. - # We are only supporting producer stock at the moment. - item.variant.semantic_links.present? - end + def managed_linked_variants(order) + user = order.distributor.owner + order_cycle = order.order_cycle + + # These permissions may be too complex. Here may be scope to optimise. + permissions = OpenFoodNetwork::OrderCyclePermissions.new(user, order_cycle) + permissions.visible_variants_for_outgoing_exchanges_to(order.distributor) + .where.associated(:semantic_links) end - def release_superfluous_stock(line, linked_variant, transformation) - # Note that a division of integers dismisses the remainder, like `floor`: - wholesale_items_contained_in_stock = linked_variant.on_hand / transformation.factor - - # But maybe we didn't actually order that much: - deductable_quantity = [line.quantity, wholesale_items_contained_in_stock].min - line.quantity -= deductable_quantity - - retail_stock_changes = deductable_quantity * transformation.factor - linked_variant.on_hand -= retail_stock_changes + def distributed_linked_variants(order) + order.order_cycle.variants_distributed_by(order.distributor) + .where.associated(:semantic_links) end def aggregate_final_quantities(order_cycle, line, variant, transformation) + # We may want to query all these quantities in one go instead of this n+1. orders = order_cycle.orders.invoiceable quantity = Spree::LineItem.where(order: orders, variant:).sum(:quantity) wholesale_quantity = (quantity.to_f / transformation.factor).ceil diff --git a/spec/jobs/amend_backorder_job_spec.rb b/spec/jobs/amend_backorder_job_spec.rb index 9f7ba52615..94b6ca65ee 100644 --- a/spec/jobs/amend_backorder_job_spec.rb +++ b/spec/jobs/amend_backorder_job_spec.rb @@ -86,13 +86,33 @@ RSpec.describe AmendBackorderJob do expect(backorder.lines[0].quantity).to eq 1 # beans expect(backorder.lines[1].quantity).to eq 5 # chia - # We cancel the only order and that should reduce the order lines to 0. - expect { order.cancel! } - .to change { beans.reload.on_hand }.from(9).to(15) - .and change { chia_seed.reload.on_hand }.from(7).to(12) + # We increase quantities which should be reflected in the backorder: + beans.on_hand = -1 + chia_item.quantity += 3 + chia_item.save! expect { subject.amend_backorder(order) } - .to change { backorder.lines.count }.from(2).to(0) + .to change { beans.on_hand }.from(-1).to(11) + .and change { backorder.lines[0].quantity }.from(1).to(2) + .and change { backorder.lines[1].quantity }.from(5).to(8) + + # We cancel the only order. + expect { order.cancel! } + .to change { beans.reload.on_hand }.from(11).to(17) + .and change { chia_seed.reload.on_hand }.from(4).to(12) + + # But we decreased the stock of beans outside of orders above. + # So only the chia seeds are cancelled. The beans still need replenishing. + expect { subject.amend_backorder(order) } + .to change { backorder.lines.count }.from(2).to(1) + .and change { beans.reload.on_hand }.by(-12) + end + end + + describe "#distributed_linked_variants" do + it "selects available variants with semantic links" do + variants = subject.distributed_linked_variants(order) + expect(variants).to match_array [beans, chia_seed] end end end From 4fa4eb1b4e7f6e98cb1e0beeb2936881282b4c09 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 27 Nov 2024 13:25:28 +1100 Subject: [PATCH 02/11] Move backorder update code to re-usable class --- app/jobs/amend_backorder_job.rb | 122 +-------------------- app/services/backorder_updater.rb | 135 ++++++++++++++++++++++++ spec/jobs/amend_backorder_job_spec.rb | 7 -- spec/services/backorder_updater_spec.rb | 118 +++++++++++++++++++++ 4 files changed, 256 insertions(+), 126 deletions(-) create mode 100644 app/services/backorder_updater.rb create mode 100644 spec/services/backorder_updater_spec.rb diff --git a/app/jobs/amend_backorder_job.rb b/app/jobs/amend_backorder_job.rb index 8e5687249b..02429b6ef2 100644 --- a/app/jobs/amend_backorder_job.rb +++ b/app/jobs/amend_backorder_job.rb @@ -1,7 +1,5 @@ # frozen_string_literal: true -require 'open_food_network/order_cycle_permissions' - # When orders are created, adjusted or cancelled, we need to amend # an existing backorder as well. class AmendBackorderJob < ApplicationJob @@ -13,125 +11,11 @@ class AmendBackorderJob < ApplicationJob end end - # The following is a mix of the BackorderJob and the CompleteBackorderJob. - # TODO: Move the common code into a re-usable service class. def amend_backorder(order) - variants = distributed_linked_variants(order) - return unless variants.any? + backorder = BackorderUpdater.new.amend_backorder(order) user = order.distributor.owner - order_cycle = order.order_cycle - - # We are assuming that all variants are linked to the same wholesale - # shop and its catalog: - reference_link = variants[0].semantic_links[0].semantic_id - urls = FdcUrlBuilder.new(reference_link) - orderer = FdcBackorderer.new(user, urls) - broker = FdcOfferBroker.new(user, urls) - - backorder = orderer.find_open_order(order) - - updated_lines = update_order_lines(backorder, order_cycle, variants, broker, orderer) - unprocessed_lines = backorder.lines.to_set - updated_lines - cancel_stale_lines(unprocessed_lines, order, broker) - - # Clean up empty lines: - backorder.lines.reject! { |line| line.quantity.zero? } - - FdcBackorderer.new(user, urls).send_order(backorder) - end - - def update_order_lines(backorder, order_cycle, variants, broker, orderer) - variants.map do |variant| - link = variant.semantic_links[0].semantic_id - solution = broker.best_offer(link) - line = orderer.find_or_build_order_line(backorder, solution.offer) - if variant.on_demand - adjust_stock(variant, solution, line) - else - aggregate_final_quantities(order_cycle, line, variant, solution) - end - - line - end - end - - def cancel_stale_lines(unprocessed_lines, order, broker) - managed_variants = managed_linked_variants(order) - unprocessed_lines.each do |line| - wholesale_quantity = line.quantity.to_i - wholesale_product_id = line.offer.offeredItem.semanticId - transformation = broker.wholesale_to_retail(wholesale_product_id) - linked_variant = managed_variants.linked_to(transformation.retail_product_id) - - if linked_variant.nil? - transformation.factor = 1 - linked_variant = managed_variants.linked_to(wholesale_product_id) - end - - # Adjust stock level back, we're not going to order this one. - if linked_variant&.on_demand - retail_quantity = wholesale_quantity * transformation.factor - linked_variant.on_hand -= retail_quantity - end - - # We don't have any active orders for this - line.quantity = 0 - end - end - - def adjust_stock(variant, solution, line) - if variant.on_hand.negative? - needed_quantity = -1 * variant.on_hand # We need to replenish it. - - # The number of wholesale packs we need to order to fulfill the - # needed quantity. - # For example, we order 2 packs of 12 cans if we need 15 cans. - wholesale_quantity = (needed_quantity.to_f / solution.factor).ceil - - # The number of individual retail items we get with the wholesale order. - # For example, if we order 2 packs of 12 cans, we will get 24 cans - # and we'll account for that in our stock levels. - retail_quantity = wholesale_quantity * solution.factor - - line.quantity = line.quantity.to_i + wholesale_quantity - variant.on_hand += retail_quantity - else - # Note that a division of integers dismisses the remainder, like `floor`: - wholesale_items_contained_in_stock = variant.on_hand / solution.factor - - # But maybe we didn't actually order that much: - deductable_quantity = [line.quantity, wholesale_items_contained_in_stock].min - - if deductable_quantity.positive? - line.quantity -= deductable_quantity - - retail_stock_change = deductable_quantity * solution.factor - variant.on_hand -= retail_stock_change - end - end - end - - def managed_linked_variants(order) - user = order.distributor.owner - order_cycle = order.order_cycle - - # These permissions may be too complex. Here may be scope to optimise. - permissions = OpenFoodNetwork::OrderCyclePermissions.new(user, order_cycle) - permissions.visible_variants_for_outgoing_exchanges_to(order.distributor) - .where.associated(:semantic_links) - end - - def distributed_linked_variants(order) - order.order_cycle.variants_distributed_by(order.distributor) - .where.associated(:semantic_links) - end - - def aggregate_final_quantities(order_cycle, line, variant, transformation) - # We may want to query all these quantities in one go instead of this n+1. - orders = order_cycle.orders.invoiceable - quantity = Spree::LineItem.where(order: orders, variant:).sum(:quantity) - wholesale_quantity = (quantity.to_f / transformation.factor).ceil - line.quantity = wholesale_quantity + urls = nil # Not needed to send order. The backorder id is the URL. + FdcBackorderer.new(user, urls).send_order(backorder) if backorder end end diff --git a/app/services/backorder_updater.rb b/app/services/backorder_updater.rb new file mode 100644 index 0000000000..d8425a3ea7 --- /dev/null +++ b/app/services/backorder_updater.rb @@ -0,0 +1,135 @@ +# frozen_string_literal: true + +require 'open_food_network/order_cycle_permissions' + +# Update a backorder to reflect all local orders and stock levels +# connected to the associated order cycle. +class BackorderUpdater + # Given an OFN order was created, changed or cancelled, + # we re-calculate how much to order in for every variant. + def amend_backorder(order) + variants = distributed_linked_variants(order) + + # Temporary code: once we don't need a variant link to look up the + # backorder, we don't need this check anymore. + # Then we can adjust the backorder even though there are no linked variants + # in the order cycle right now. Some variants may have been in the order + # cycle before and got ordered before being removed from the order cycle. + return unless variants.any? + + user = order.distributor.owner + order_cycle = order.order_cycle + + # We are assuming that all variants are linked to the same wholesale + # shop and its catalog: + reference_link = variants[0].semantic_links[0].semantic_id + urls = FdcUrlBuilder.new(reference_link) + orderer = FdcBackorderer.new(user, urls) + broker = FdcOfferBroker.new(user, urls) + + backorder = orderer.find_open_order(order) + + updated_lines = update_order_lines(backorder, order_cycle, variants, broker, orderer) + unprocessed_lines = backorder.lines.to_set - updated_lines + cancel_stale_lines(unprocessed_lines, order, broker) + + # Clean up empty lines: + backorder.lines.reject! { |line| line.quantity.zero? } + + backorder + end + + def update_order_lines(backorder, order_cycle, variants, broker, orderer) + variants.map do |variant| + link = variant.semantic_links[0].semantic_id + solution = broker.best_offer(link) + line = orderer.find_or_build_order_line(backorder, solution.offer) + if variant.on_demand + adjust_stock(variant, solution, line) + else + aggregate_final_quantities(order_cycle, line, variant, solution) + end + + line + end + end + + def cancel_stale_lines(unprocessed_lines, order, broker) + managed_variants = managed_linked_variants(order) + unprocessed_lines.each do |line| + wholesale_quantity = line.quantity.to_i + wholesale_product_id = line.offer.offeredItem.semanticId + transformation = broker.wholesale_to_retail(wholesale_product_id) + linked_variant = managed_variants.linked_to(transformation.retail_product_id) + + if linked_variant.nil? + transformation.factor = 1 + linked_variant = managed_variants.linked_to(wholesale_product_id) + end + + # Adjust stock level back, we're not going to order this one. + if linked_variant&.on_demand + retail_quantity = wholesale_quantity * transformation.factor + linked_variant.on_hand -= retail_quantity + end + + # We don't have any active orders for this + line.quantity = 0 + end + end + + def adjust_stock(variant, solution, line) + if variant.on_hand.negative? + needed_quantity = -1 * variant.on_hand # We need to replenish it. + + # The number of wholesale packs we need to order to fulfill the + # needed quantity. + # For example, we order 2 packs of 12 cans if we need 15 cans. + wholesale_quantity = (needed_quantity.to_f / solution.factor).ceil + + # The number of individual retail items we get with the wholesale order. + # For example, if we order 2 packs of 12 cans, we will get 24 cans + # and we'll account for that in our stock levels. + retail_quantity = wholesale_quantity * solution.factor + + line.quantity = line.quantity.to_i + wholesale_quantity + variant.on_hand += retail_quantity + else + # Note that a division of integers dismisses the remainder, like `floor`: + wholesale_items_contained_in_stock = variant.on_hand / solution.factor + + # But maybe we didn't actually order that much: + deductable_quantity = [line.quantity, wholesale_items_contained_in_stock].min + + if deductable_quantity.positive? + line.quantity -= deductable_quantity + + retail_stock_change = deductable_quantity * solution.factor + variant.on_hand -= retail_stock_change + end + end + end + + def managed_linked_variants(order) + user = order.distributor.owner + order_cycle = order.order_cycle + + # These permissions may be too complex. Here may be scope to optimise. + permissions = OpenFoodNetwork::OrderCyclePermissions.new(user, order_cycle) + permissions.visible_variants_for_outgoing_exchanges_to(order.distributor) + .where.associated(:semantic_links) + end + + def distributed_linked_variants(order) + order.order_cycle.variants_distributed_by(order.distributor) + .where.associated(:semantic_links) + end + + def aggregate_final_quantities(order_cycle, line, variant, transformation) + # We may want to query all these quantities in one go instead of this n+1. + orders = order_cycle.orders.invoiceable + quantity = Spree::LineItem.where(order: orders, variant:).sum(:quantity) + wholesale_quantity = (quantity.to_f / transformation.factor).ceil + line.quantity = wholesale_quantity + end +end diff --git a/spec/jobs/amend_backorder_job_spec.rb b/spec/jobs/amend_backorder_job_spec.rb index 94b6ca65ee..9229db6d6c 100644 --- a/spec/jobs/amend_backorder_job_spec.rb +++ b/spec/jobs/amend_backorder_job_spec.rb @@ -108,11 +108,4 @@ RSpec.describe AmendBackorderJob do .and change { beans.reload.on_hand }.by(-12) end end - - describe "#distributed_linked_variants" do - it "selects available variants with semantic links" do - variants = subject.distributed_linked_variants(order) - expect(variants).to match_array [beans, chia_seed] - end - end end diff --git a/spec/services/backorder_updater_spec.rb b/spec/services/backorder_updater_spec.rb new file mode 100644 index 0000000000..da5e4aa6d2 --- /dev/null +++ b/spec/services/backorder_updater_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe BackorderUpdater do + let(:order) { create(:completed_order_with_totals) } + let(:distributor) { order.distributor } + let(:beans) { beans_item.variant } + let(:beans_item) { order.line_items[0] } + let(:chia_seed) { chia_item.variant } + let(:chia_item) { order.line_items[1] } + let(:user) { order.distributor.owner } + let(:catalog_json) { file_fixture("fdc-catalog.json").read } + let(:catalog_url) { + "https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts" + } + let(:product_link) { + "https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts/44519466467635" + } + let(:chia_seed_wholesale_link) { + "https://env-0105831.jcloud-ver-jpe.ik-server.com/api/dfc/Enterprises/test-hodmedod/SuppliedProducts/44519468433715" + } + + before do + # This ensures that callbacks adjust stock correctly. + # See: https://github.com/openfoodfoundation/openfoodnetwork/pull/12938 + order.reload + + user.oidc_account = build(:testdfc_account) + + beans.semantic_links << SemanticLink.new( + semantic_id: product_link + ) + chia_seed.semantic_links << SemanticLink.new( + semantic_id: chia_seed_wholesale_link + ) + order.order_cycle = create( + :simple_order_cycle, + distributors: [distributor], + variants: order.variants, + ) + order.save! + + beans.on_demand = true + beans_item.update!(quantity: 6) + beans.on_hand = -3 + + chia_item.update!(quantity: 5) + chia_seed.on_demand = false + chia_seed.on_hand = 7 + end + + describe "#amend_backorder" do + it "updates an order" do + stub_request(:get, catalog_url).to_return(body: catalog_json) + + # Record the placed backorder: + backorder = nil + allow_any_instance_of(FdcBackorderer).to receive(:find_order) do |*_args| + backorder + end + allow_any_instance_of(FdcBackorderer).to receive(:find_open_order) do |*_args| + backorder + end + allow_any_instance_of(FdcBackorderer).to receive(:send_order) do |*args| + backorder = args[1] + end + + BackorderJob.new.place_backorder(order) + + # We ordered a case of 12 cans: -3 + 12 = 9 + expect(beans.on_hand).to eq 9 + + # Stock controlled items don't change stock in backorder: + expect(chia_seed.on_hand).to eq 7 + + expect(backorder.lines[0].quantity).to eq 1 # beans + expect(backorder.lines[1].quantity).to eq 5 # chia + + # Without any change, the backorder shouldn't get changed either: + subject.amend_backorder(order) + + # Same as before: + expect(beans.on_hand).to eq 9 + expect(chia_seed.on_hand).to eq 7 + expect(backorder.lines[0].quantity).to eq 1 # beans + expect(backorder.lines[1].quantity).to eq 5 # chia + + # We increase quantities which should be reflected in the backorder: + beans.on_hand = -1 + chia_item.quantity += 3 + chia_item.save! + + expect { subject.amend_backorder(order) } + .to change { beans.on_hand }.from(-1).to(11) + .and change { backorder.lines[0].quantity }.from(1).to(2) + .and change { backorder.lines[1].quantity }.from(5).to(8) + + # We cancel the only order. + expect { order.cancel! } + .to change { beans.reload.on_hand }.from(11).to(17) + .and change { chia_seed.reload.on_hand }.from(4).to(12) + + # But we decreased the stock of beans outside of orders above. + # So only the chia seeds are cancelled. The beans still need replenishing. + expect { subject.amend_backorder(order) } + .to change { backorder.lines.count }.from(2).to(1) + .and change { beans.reload.on_hand }.by(-12) + end + end + + describe "#distributed_linked_variants" do + it "selects available variants with semantic links" do + variants = subject.distributed_linked_variants(order) + expect(variants).to match_array [beans, chia_seed] + end + end +end From 7d7253cf0e4728d2fcd542fe4dc04309ed14c080 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 27 Nov 2024 15:04:26 +1100 Subject: [PATCH 03/11] Re-use BackorderUpdater to complete backorder --- app/jobs/complete_backorder_job.rb | 54 +------------------------ app/services/backorder_updater.rb | 42 ++++++++++++------- spec/services/backorder_updater_spec.rb | 4 +- 3 files changed, 31 insertions(+), 69 deletions(-) diff --git a/app/jobs/complete_backorder_job.rb b/app/jobs/complete_backorder_job.rb index db177c5cf5..8737996a6e 100644 --- a/app/jobs/complete_backorder_job.rb +++ b/app/jobs/complete_backorder_job.rb @@ -24,8 +24,7 @@ class CompleteBackorderJob < ApplicationJob urls = FdcUrlBuilder.new(order.lines[0].offer.offeredItem.semanticId) - variants = order_cycle.variants_distributed_by(distributor) - adjust_quantities(order_cycle, user, order, urls, variants) + BackorderUpdater.new.update(order, user, distributor, order_cycle) FdcBackorderer.new(user, urls).complete_order(order) @@ -36,55 +35,4 @@ class CompleteBackorderJob < ApplicationJob raise end - - # Check if we have enough stock to reduce the backorder. - # - # Our local stock can increase when users cancel their orders. - # But stock levels could also have been adjusted manually. So we review all - # quantities before finalising the order. - def adjust_quantities(order_cycle, user, order, urls, variants) - broker = FdcOfferBroker.new(user, urls) - - order.lines.each do |line| - line.quantity = line.quantity.to_i - wholesale_product_id = line.offer.offeredItem.semanticId - transformation = broker.wholesale_to_retail(wholesale_product_id) - linked_variant = variants.linked_to(transformation.retail_product_id) - - # Assumption: If a transformation is present then we only sell the retail - # variant. If that can't be found, it was deleted and we'll ignore that - # for now. - next if linked_variant.nil? - - # Find all line items for this order cycle - # Update quantity accordingly - if linked_variant.on_demand - release_superfluous_stock(line, linked_variant, transformation) - else - aggregate_final_quantities(order_cycle, line, linked_variant, transformation) - end - end - - # Clean up empty lines: - order.lines.reject! { |line| line.quantity.zero? } - end - - def release_superfluous_stock(line, linked_variant, transformation) - # Note that a division of integers dismisses the remainder, like `floor`: - wholesale_items_contained_in_stock = linked_variant.on_hand / transformation.factor - - # But maybe we didn't actually order that much: - deductable_quantity = [line.quantity, wholesale_items_contained_in_stock].min - line.quantity -= deductable_quantity - - retail_stock_changes = deductable_quantity * transformation.factor - linked_variant.on_hand -= retail_stock_changes - end - - def aggregate_final_quantities(order_cycle, line, variant, transformation) - orders = order_cycle.orders.invoiceable - quantity = Spree::LineItem.where(order: orders, variant:).sum(:quantity) - wholesale_quantity = (quantity.to_f / transformation.factor).ceil - line.quantity = wholesale_quantity - end end diff --git a/app/services/backorder_updater.rb b/app/services/backorder_updater.rb index d8425a3ea7..ad84cec505 100644 --- a/app/services/backorder_updater.rb +++ b/app/services/backorder_updater.rb @@ -8,7 +8,9 @@ class BackorderUpdater # Given an OFN order was created, changed or cancelled, # we re-calculate how much to order in for every variant. def amend_backorder(order) - variants = distributed_linked_variants(order) + order_cycle = order.order_cycle + distributor = order.distributor + variants = distributed_linked_variants(order_cycle, distributor) # Temporary code: once we don't need a variant link to look up the # backorder, we don't need this check anymore. @@ -17,8 +19,21 @@ class BackorderUpdater # cycle before and got ordered before being removed from the order cycle. return unless variants.any? + # We are assuming that all variants are linked to the same wholesale + # shop and its catalog: + reference_link = variants[0].semantic_links[0].semantic_id user = order.distributor.owner - order_cycle = order.order_cycle + urls = FdcUrlBuilder.new(reference_link) + orderer = FdcBackorderer.new(user, urls) + + backorder = orderer.find_open_order(order) + + update(backorder, user, distributor, order_cycle) + end + + # Update a given backorder according to a distributor's order cycle. + def update(backorder, user, distributor, order_cycle) + variants = distributed_linked_variants(order_cycle, distributor) # We are assuming that all variants are linked to the same wholesale # shop and its catalog: @@ -27,11 +42,10 @@ class BackorderUpdater orderer = FdcBackorderer.new(user, urls) broker = FdcOfferBroker.new(user, urls) - backorder = orderer.find_open_order(order) - updated_lines = update_order_lines(backorder, order_cycle, variants, broker, orderer) unprocessed_lines = backorder.lines.to_set - updated_lines - cancel_stale_lines(unprocessed_lines, order, broker) + managed_variants = managed_linked_variants(user, order_cycle, distributor) + cancel_stale_lines(unprocessed_lines, managed_variants, broker) # Clean up empty lines: backorder.lines.reject! { |line| line.quantity.zero? } @@ -54,8 +68,7 @@ class BackorderUpdater end end - def cancel_stale_lines(unprocessed_lines, order, broker) - managed_variants = managed_linked_variants(order) + def cancel_stale_lines(unprocessed_lines, managed_variants, broker) unprocessed_lines.each do |line| wholesale_quantity = line.quantity.to_i wholesale_product_id = line.offer.offeredItem.semanticId @@ -79,6 +92,8 @@ class BackorderUpdater end def adjust_stock(variant, solution, line) + line.quantity = line.quantity.to_i + if variant.on_hand.negative? needed_quantity = -1 * variant.on_hand # We need to replenish it. @@ -92,7 +107,7 @@ class BackorderUpdater # and we'll account for that in our stock levels. retail_quantity = wholesale_quantity * solution.factor - line.quantity = line.quantity.to_i + wholesale_quantity + line.quantity += wholesale_quantity variant.on_hand += retail_quantity else # Note that a division of integers dismisses the remainder, like `floor`: @@ -110,18 +125,15 @@ class BackorderUpdater end end - def managed_linked_variants(order) - user = order.distributor.owner - order_cycle = order.order_cycle - + def managed_linked_variants(user, order_cycle, distributor) # These permissions may be too complex. Here may be scope to optimise. permissions = OpenFoodNetwork::OrderCyclePermissions.new(user, order_cycle) - permissions.visible_variants_for_outgoing_exchanges_to(order.distributor) + permissions.visible_variants_for_outgoing_exchanges_to(distributor) .where.associated(:semantic_links) end - def distributed_linked_variants(order) - order.order_cycle.variants_distributed_by(order.distributor) + def distributed_linked_variants(order_cycle, distributor) + order_cycle.variants_distributed_by(distributor) .where.associated(:semantic_links) end diff --git a/spec/services/backorder_updater_spec.rb b/spec/services/backorder_updater_spec.rb index da5e4aa6d2..48aa125fdc 100644 --- a/spec/services/backorder_updater_spec.rb +++ b/spec/services/backorder_updater_spec.rb @@ -110,8 +110,10 @@ RSpec.describe BackorderUpdater do end describe "#distributed_linked_variants" do + let(:order_cycle) { order.order_cycle } + it "selects available variants with semantic links" do - variants = subject.distributed_linked_variants(order) + variants = subject.distributed_linked_variants(order_cycle, distributor) expect(variants).to match_array [beans, chia_seed] end end From 7d27f46d68f907be603b00fa4f8b0522c454aa50 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 5 Dec 2024 16:31:37 +1100 Subject: [PATCH 04/11] Simplify negated expectation --- spec/system/admin/orders/bulk_actions_spec.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/spec/system/admin/orders/bulk_actions_spec.rb b/spec/system/admin/orders/bulk_actions_spec.rb index 1d1e861d11..9bf49bd932 100644 --- a/spec/system/admin/orders/bulk_actions_spec.rb +++ b/spec/system/admin/orders/bulk_actions_spec.rb @@ -447,6 +447,7 @@ RSpec.describe ' end end end + it "can bulk cancel 2 orders" do page.find("#listing_orders tbody tr:nth-child(1) input[name='bulk_ids[]']").click page.find("#listing_orders tbody tr:nth-child(2) input[name='bulk_ids[]']").click @@ -463,7 +464,7 @@ RSpec.describe ' uncheck "Send a cancellation email to the customer" expect { find_button("Cancel").click # Cancels the cancel action - }.not_to enqueue_job(ActionMailer::MailDeliveryJob).exactly(:twice) + }.not_to enqueue_mail end page.find("span.icon-reorder", text: "Actions").click @@ -474,7 +475,7 @@ RSpec.describe ' within ".reveal-modal" do expect { find_button("Confirm").click # Confirms the cancel action - }.not_to enqueue_job(ActionMailer::MailDeliveryJob).exactly(:twice) + }.not_to enqueue_mail end expect(page).to have_content("CANCELLED", count: 2) From e1febc6e0040f2be99bd190a2b82357a0faa5cb3 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 5 Dec 2024 16:41:36 +1100 Subject: [PATCH 05/11] Simplify page actions --- spec/system/admin/orders/bulk_actions_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/system/admin/orders/bulk_actions_spec.rb b/spec/system/admin/orders/bulk_actions_spec.rb index 9bf49bd932..cb77f6d414 100644 --- a/spec/system/admin/orders/bulk_actions_spec.rb +++ b/spec/system/admin/orders/bulk_actions_spec.rb @@ -463,7 +463,7 @@ RSpec.describe ' within ".reveal-modal" do uncheck "Send a cancellation email to the customer" expect { - find_button("Cancel").click # Cancels the cancel action + click_on "Cancel" # Cancels the cancel action }.not_to enqueue_mail end @@ -474,7 +474,7 @@ RSpec.describe ' within ".reveal-modal" do expect { - find_button("Confirm").click # Confirms the cancel action + click_on "Confirm" # Confirms the cancel action }.not_to enqueue_mail end From e76d6ad3df2ed6b018efc614d2c14ab74d2e1c24 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 5 Dec 2024 16:57:29 +1100 Subject: [PATCH 06/11] Spec current order cancellation amending backorder The cancellation happens async in Javascript. Therefore we need to wait for and outcome on the page to know that the action finished. The expectation needs to be around that whole block. We actually want only one job enqueued if the same backorder is affected. Time to fix that. --- spec/system/admin/orders/bulk_actions_spec.rb | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/spec/system/admin/orders/bulk_actions_spec.rb b/spec/system/admin/orders/bulk_actions_spec.rb index cb77f6d414..c8b1a66553 100644 --- a/spec/system/admin/orders/bulk_actions_spec.rb +++ b/spec/system/admin/orders/bulk_actions_spec.rb @@ -467,18 +467,21 @@ RSpec.describe ' }.not_to enqueue_mail end + expect(page).not_to have_content "This will cancel the current order." + page.find("span.icon-reorder", text: "Actions").click within ".ofn-drop-down .menu" do page.find("span", text: "Cancel Orders").click end - within ".reveal-modal" do - expect { + expect { + within ".reveal-modal" do click_on "Confirm" # Confirms the cancel action - }.not_to enqueue_mail - end - - expect(page).to have_content("CANCELLED", count: 2) + end + expect(page).to have_content("CANCELLED", count: 2) + }.to enqueue_job(AmendBackorderJob).exactly(:twice) + # You can't combine negative matchers. + .and enqueue_mail.exactly(0).times end end From 9ca1b48d2efc40d46045200049f19d62abb45112 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 6 Dec 2024 10:05:37 +1100 Subject: [PATCH 07/11] Move backorder amendment out of order callback Triggering it for each order is inefficient when we cancel them in bulk. The callback doesn't allow us to optimise this. --- app/controllers/spree/admin/orders_controller.rb | 1 + app/models/spree/order/checkout.rb | 2 -- app/services/orders/bulk_cancel_service.rb | 1 + app/services/orders/customer_cancellation_service.rb | 1 + 4 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/controllers/spree/admin/orders_controller.rb b/app/controllers/spree/admin/orders_controller.rb index a73adf58c4..ada793fc3f 100644 --- a/app/controllers/spree/admin/orders_controller.rb +++ b/app/controllers/spree/admin/orders_controller.rb @@ -70,6 +70,7 @@ module Spree @order.restock_items = params.fetch(:restock_items, "true") == "true" if @order.public_send(event.to_s) + AmendBackorderJob.perform_later(@order) if event == "cancel" flash[:success] = Spree.t(:order_updated) else flash[:error] = Spree.t(:cannot_perform_operation) diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb index a9705d7244..e51ffe442a 100644 --- a/app/models/spree/order/checkout.rb +++ b/app/models/spree/order/checkout.rb @@ -142,8 +142,6 @@ module Spree OrderMailer.cancel_email(id).deliver_later if send_cancellation_email update(payment_state: updater.update_payment_state) - - AmendBackorderJob.perform_later(self) end def after_resume diff --git a/app/services/orders/bulk_cancel_service.rb b/app/services/orders/bulk_cancel_service.rb index 0469907bf8..6eed24c7d9 100644 --- a/app/services/orders/bulk_cancel_service.rb +++ b/app/services/orders/bulk_cancel_service.rb @@ -15,6 +15,7 @@ module Orders order.send_cancellation_email = @send_cancellation_email order.restock_items = @restock_items order.cancel + AmendBackorderJob.perform_later(order) end # rubocop:enable Rails/FindEach end diff --git a/app/services/orders/customer_cancellation_service.rb b/app/services/orders/customer_cancellation_service.rb index 7385139db0..3371680cbc 100644 --- a/app/services/orders/customer_cancellation_service.rb +++ b/app/services/orders/customer_cancellation_service.rb @@ -10,6 +10,7 @@ module Orders return unless order.cancel Spree::OrderMailer.cancel_email_for_shop(order).deliver_later + AmendBackorderJob.perform_later(order) end private From fcbaefb2c81cabf0cd0dcf9351c6b8f1919871e7 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 6 Dec 2024 11:57:18 +1100 Subject: [PATCH 08/11] Update each backorder only once in bulk cancel --- app/jobs/amend_backorder_job.rb | 9 +++++++++ app/services/orders/bulk_cancel_service.rb | 3 +-- spec/jobs/amend_backorder_job_spec.rb | 23 ++++++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/app/jobs/amend_backorder_job.rb b/app/jobs/amend_backorder_job.rb index 02429b6ef2..b4d2bf0506 100644 --- a/app/jobs/amend_backorder_job.rb +++ b/app/jobs/amend_backorder_job.rb @@ -5,6 +5,15 @@ class AmendBackorderJob < ApplicationJob sidekiq_options retry: 0 + def self.schedule_bulk_update_for(orders) + # We can have one backorder per order cycle and distributor. + groups = orders.group_by { |order| [order.order_cycle, order.distributor] } + groups.each_value do |orders_with_same_backorder| + # We need to trigger only one update per backorder. + perform_later(orders_with_same_backorder.first) + end + end + def perform(order) OrderLocker.lock_order_and_variants(order) do amend_backorder(order) diff --git a/app/services/orders/bulk_cancel_service.rb b/app/services/orders/bulk_cancel_service.rb index 6eed24c7d9..86e7637b8e 100644 --- a/app/services/orders/bulk_cancel_service.rb +++ b/app/services/orders/bulk_cancel_service.rb @@ -15,8 +15,7 @@ module Orders order.send_cancellation_email = @send_cancellation_email order.restock_items = @restock_items order.cancel - AmendBackorderJob.perform_later(order) - end + end.tap { |orders| AmendBackorderJob.schedule_bulk_update_for(orders) } # rubocop:enable Rails/FindEach end diff --git a/spec/jobs/amend_backorder_job_spec.rb b/spec/jobs/amend_backorder_job_spec.rb index 9229db6d6c..bde82b0741 100644 --- a/spec/jobs/amend_backorder_job_spec.rb +++ b/spec/jobs/amend_backorder_job_spec.rb @@ -50,6 +50,29 @@ RSpec.describe AmendBackorderJob do chia_seed.on_hand = 7 end + describe ".schedule_bulk_update_for" do + let(:order_same_oc) { + create( + :completed_order_with_totals, + distributor: order.distributor, + order_cycle: order.order_cycle, + ) + } + let(:order_other_oc) { create(:completed_order_with_totals) } + + it "enqueues only one job per backorder" do + expect { + AmendBackorderJob.schedule_bulk_update_for([order, order_same_oc]) + }.to enqueue_job(AmendBackorderJob).exactly(:once) + end + + it "enqueues a job for each backorder" do + expect { + AmendBackorderJob.schedule_bulk_update_for([order, order_other_oc]) + }.to enqueue_job(AmendBackorderJob).exactly(:twice) + end + end + describe "#amend_backorder" do it "updates an order" do stub_request(:get, catalog_url).to_return(body: catalog_json) From 9c0a15f43113afdd6e1718570d7a7f9b90dfb83c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 6 Dec 2024 13:14:33 +1100 Subject: [PATCH 09/11] Amend backorders on admin update orders --- app/controllers/api/v0/shipments_controller.rb | 3 +++ app/controllers/spree/orders_controller.rb | 2 ++ 2 files changed, 5 insertions(+) diff --git a/app/controllers/api/v0/shipments_controller.rb b/app/controllers/api/v0/shipments_controller.rb index 4d3d469597..b041eacd97 100644 --- a/app/controllers/api/v0/shipments_controller.rb +++ b/app/controllers/api/v0/shipments_controller.rb @@ -24,6 +24,7 @@ module Api Orders::WorkflowService.new(@order).advance_to_payment if @order.line_items.any? @order.recreate_all_fees! + AmendBackorderJob.perform_later(@order) if @order.completed? render json: @shipment, serializer: Api::ShipmentSerializer, status: :ok end @@ -73,6 +74,7 @@ module Api @order.contents.add(variant, quantity, @shipment) @order.recreate_all_fees! + AmendBackorderJob.perform_later(@order) if @order.completed? render json: @shipment, serializer: Api::ShipmentSerializer, status: :ok end @@ -86,6 +88,7 @@ module Api @shipment.reload if @shipment.persisted? @order.recreate_all_fees! + AmendBackorderJob.perform_later(@order) if @order.completed? render json: @shipment, serializer: Api::ShipmentSerializer, status: :ok end diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb index a7a25700a2..f2b47f27ed 100644 --- a/app/controllers/spree/orders_controller.rb +++ b/app/controllers/spree/orders_controller.rb @@ -77,6 +77,8 @@ module Spree @order.create_tax_charge! end + AmendBackorderJob.perform_later(@order) if @order.completed? + respond_with(@order) do |format| format.html do if params.key?(:checkout) From 88837b55b9f40e5bd906b40e6af0ad369262a3af Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 6 Dec 2024 13:40:02 +1100 Subject: [PATCH 10/11] Amend backorder also when resuming order --- app/controllers/spree/admin/orders_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/orders_controller.rb b/app/controllers/spree/admin/orders_controller.rb index ada793fc3f..6c76bdf3d9 100644 --- a/app/controllers/spree/admin/orders_controller.rb +++ b/app/controllers/spree/admin/orders_controller.rb @@ -70,7 +70,7 @@ module Spree @order.restock_items = params.fetch(:restock_items, "true") == "true" if @order.public_send(event.to_s) - AmendBackorderJob.perform_later(@order) if event == "cancel" + AmendBackorderJob.perform_later(@order) if @order.completed? flash[:success] = Spree.t(:order_updated) else flash[:error] = Spree.t(:cannot_perform_operation) From bf41658d32b00a8cc927338b9900dd5e4778ed61 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 11 Dec 2024 12:40:35 +1100 Subject: [PATCH 11/11] Fix nil error when amending backorder --- app/services/fdc_backorderer.rb | 2 +- spec/services/fdc_backorderer_spec.rb | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/services/fdc_backorderer.rb b/app/services/fdc_backorderer.rb index 33318a8c83..4237ce090c 100644 --- a/app/services/fdc_backorderer.rb +++ b/app/services/fdc_backorderer.rb @@ -152,7 +152,7 @@ class FdcBackorderer end def new?(order) - order.semanticId == urls.orders_url + order.semanticId == urls&.orders_url end def build_sale_session(order) diff --git a/spec/services/fdc_backorderer_spec.rb b/spec/services/fdc_backorderer_spec.rb index 15310526e4..66f078a407 100644 --- a/spec/services/fdc_backorderer_spec.rb +++ b/spec/services/fdc_backorderer_spec.rb @@ -106,4 +106,20 @@ RSpec.describe FdcBackorderer do expect(found_line).to eq existing_line end end + + describe "#new?" do + describe "without knowing URLs" do + let(:subject) { FdcBackorderer.new(nil, nil) } + + it "recognises new orders" do + order = DataFoodConsortium::Connector::Order.new(nil) + expect(subject.new?(order)).to eq true + end + + it "recognises existing orders" do + order = DataFoodConsortium::Connector::Order.new("https://order") + expect(subject.new?(order)).to eq false + end + end + end end