From 9d2fd373cef9d6c19bed41be217084e5712904ee Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Wed, 24 Mar 2021 12:16:25 -0700 Subject: [PATCH 1/6] record subscription issue if no order is generated --- app/jobs/subscription_placement_job.rb | 6 ++++-- .../subscription_mailer/_summary_detail.html.haml | 8 ++++++++ .../order_management/subscriptions/summarizer.rb | 11 +++++++++-- .../order_management/subscriptions/summary.rb | 9 +++++++-- 4 files changed, 28 insertions(+), 6 deletions(-) diff --git a/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb index c56df548d6a..3131c21051f 100644 --- a/app/jobs/subscription_placement_job.rb +++ b/app/jobs/subscription_placement_job.rb @@ -13,8 +13,8 @@ def perform private - delegate :record_order, :record_success, :record_issue, to: :summarizer - delegate :record_and_log_error, :send_placement_summary_emails, to: :summarizer + delegate :record_success, :record_issue, :record_subscription_issue, to: :summarizer + delegate :record_order, :record_and_log_error, :send_placement_summary_emails, to: :summarizer def summarizer @summarizer ||= OrderManagement::Subscriptions::Summarizer.new @@ -30,6 +30,8 @@ def proxy_orders def place_order_for(proxy_order) JobLogger.logger.info("Placing Order for Proxy Order #{proxy_order.id}") initialise_order(proxy_order) + return record_subscription_issue(proxy_order.subscription) if proxy_order.order.nil? + place_order(proxy_order.order) end diff --git a/app/views/subscription_mailer/_summary_detail.html.haml b/app/views/subscription_mailer/_summary_detail.html.haml index 9e99633dcb2..a566b2396dd 100644 --- a/app/views/subscription_mailer/_summary_detail.html.haml +++ b/app/views/subscription_mailer/_summary_detail.html.haml @@ -19,3 +19,11 @@ - orders.each_with_index do |order, i| %a{ href: order_url(order) }>= order.number = ", " if i < orders.count - 1 + +- if summary.subscription_issues.any? + - subscription_issues = summary.subscription_issues + %h4= t(".other.title", count: subscription_issues.count) + %p= t(".other.explainer") + - subscription_issues.each_with_index do |subscription_id, i| + %a{ href: edit_admin_subscription_url(subscription_id) }>= subscription_id + = ", " if i < subscription_issues.count - 1 diff --git a/engines/order_management/app/services/order_management/subscriptions/summarizer.rb b/engines/order_management/app/services/order_management/subscriptions/summarizer.rb index 86c7deadea5..32c925fc03f 100644 --- a/engines/order_management/app/services/order_management/subscriptions/summarizer.rb +++ b/engines/order_management/app/services/order_management/subscriptions/summarizer.rb @@ -22,6 +22,10 @@ def record_issue(type, order, message = nil) summary_for(order).record_issue(type, order, message) end + def record_subscription_issue(subscription) + summary_for_shop_id(subscription.shop_id).record_subscription_issue(subscription) + end + def record_and_log_error(type, order, error_message = nil) return record_issue(type, order) unless order.errors.any? @@ -51,10 +55,13 @@ def send_confirmation_summary_emails private - def summary_for(order) - shop_id = order.distributor_id + def summary_for_shop_id(shop_id) @summaries[shop_id] ||= Summary.new(shop_id) end + + def summary_for(order) + summary_for_shop_id(order.distributor_id) + end end end end diff --git a/engines/order_management/app/services/order_management/subscriptions/summary.rb b/engines/order_management/app/services/order_management/subscriptions/summary.rb index c37272c6d3d..e16b116bf8e 100644 --- a/engines/order_management/app/services/order_management/subscriptions/summary.rb +++ b/engines/order_management/app/services/order_management/subscriptions/summary.rb @@ -3,13 +3,14 @@ module OrderManagement module Subscriptions class Summary - attr_reader :shop_id, :issues + attr_reader :shop_id, :issues, :subscription_issues def initialize(shop_id) @shop_id = shop_id @order_ids = [] @success_ids = [] @issues = {} + @subscription_issues = [] end def record_order(order) @@ -25,6 +26,10 @@ def record_issue(type, order, message) issues[type][order.id] = message end + def record_subscription_issue(subscription) + @subscription_issues << subscription.id + end + def order_count @order_ids.count end @@ -34,7 +39,7 @@ def success_count end def issue_count - (@order_ids - @success_ids).count + (@order_ids - @success_ids).count + @subscription_issues.count end def orders_affected_by(type) From d0d3b73257ddaf17f306132f1df4dc0a13f76af6 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Wed, 24 Mar 2021 13:17:09 -0700 Subject: [PATCH 2/6] add specs --- .../subscriptions/summarizer_spec.rb | 11 ++++++++++ .../subscriptions/summary_spec.rb | 20 +++++++++++++++++++ spec/jobs/subscription_placement_job_spec.rb | 12 +++++++++++ 3 files changed, 43 insertions(+) diff --git a/engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb index 0c47277e47a..f80b27e85f9 100644 --- a/engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb +++ b/engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb @@ -94,6 +94,17 @@ module Subscriptions end end end + + describe "#record_subscription_issue" do + let(:subscription) { double(:subscription, shop_id: 1) } + + before { allow(summarizer).to receive(:summary_for_shop_id).with(subscription.shop_id) { summary } } + + it "records a subscription issue" do + expect(summary).to receive(:record_subscription_issue).with(subscription).once + summarizer.record_subscription_issue(subscription) + end + end end describe "#send_placement_summary_emails" do diff --git a/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb index 454e2e5ba39..731a123d17a 100644 --- a/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb +++ b/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb @@ -56,6 +56,15 @@ module Subscriptions end end + describe "record_subscription_issue" do + let(:subscription) { double(:subscription, id: 101) } + + it "stores a new subscription issue" do + summary.record_subscription_issue(subscription) + expect(summary.subscription_issues).to eq [101] + end + end + describe "#order_count" do let(:order_ids) { [1, 2, 3, 4, 5, 6, 7] } it "counts the number of items in the order_ids instance_variable" do @@ -81,6 +90,17 @@ module Subscriptions summary.instance_variable_set(:@success_ids, success_ids) expect(summary.issue_count).to be 2 # 7 & 9 end + + context "when there are also subscription issues" do + let(:subscription_issues) { [101, 212] } + + it "includes subscription issues in the count" do + summary.instance_variable_set(:@order_ids, order_ids) + summary.instance_variable_set(:@success_ids, success_ids) + summary.instance_variable_set(:@subscription_issues, subscription_issues) + expect(summary.issue_count).to eq 4 # 7, 9, 101, 212 + end + end end describe "#orders_affected_by" do diff --git a/spec/jobs/subscription_placement_job_spec.rb b/spec/jobs/subscription_placement_job_spec.rb index db88322ffc6..bf4b79c59a9 100644 --- a/spec/jobs/subscription_placement_job_spec.rb +++ b/spec/jobs/subscription_placement_job_spec.rb @@ -217,6 +217,18 @@ end end end + + context "when the proxy order fails to generate an order" do + before do + allow(proxy_order).to receive(:initialise_order!) { nil } + end + + it "records an error " do + expect(job).to receive(:record_subscription_issue) + expect(job).to_not receive(:place_order) + job.send(:place_proxy_order, proxy_order) + end + end end describe "#send_placement_email" do From 3664b86e0334c59aaccb6e3c48bc9fd1923ec1b8 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Wed, 24 Mar 2021 14:10:12 -0700 Subject: [PATCH 3/6] ignore long module in rubocop --- .rubocop_manual_todo.yml | 1 + .../order_management/subscriptions/summarizer_spec.rb | 5 ++++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 4789dca9feb..740c3171aa3 100644 --- a/.rubocop_manual_todo.yml +++ b/.rubocop_manual_todo.yml @@ -849,6 +849,7 @@ Metrics/ModuleLength: - app/models/spree/payment/processing.rb - engines/order_management/spec/services/order_management/subscriptions/proxy_order_syncer_spec.rb - engines/order_management/spec/services/order_management/subscriptions/validator_spec.rb + - engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb - lib/open_food_network/column_preference_defaults.rb - spec/controllers/admin/order_cycles_controller_spec.rb - spec/controllers/api/orders_controller_spec.rb diff --git a/engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb index f80b27e85f9..8333eda9062 100644 --- a/engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb +++ b/engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb @@ -98,7 +98,10 @@ module Subscriptions describe "#record_subscription_issue" do let(:subscription) { double(:subscription, shop_id: 1) } - before { allow(summarizer).to receive(:summary_for_shop_id).with(subscription.shop_id) { summary } } + before do + allow(summarizer).to receive(:summary_for_shop_id). + with(subscription.shop_id) { summary } + end it "records a subscription issue" do expect(summary).to receive(:record_subscription_issue).with(subscription).once From 8867f1ef7a5c8a92086fdac460dbc1e553b00ea6 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 29 Mar 2021 15:37:30 -0700 Subject: [PATCH 4/6] refactor responsibilities and order of operations --- app/jobs/subscription_placement_job.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb index 3131c21051f..793c0b7f7a5 100644 --- a/app/jobs/subscription_placement_job.rb +++ b/app/jobs/subscription_placement_job.rb @@ -3,7 +3,6 @@ class SubscriptionPlacementJob < ActiveJob::Base def perform ids = proxy_orders.pluck(:id) - proxy_orders.update_all(placed_at: Time.zone.now) ProxyOrder.where(id: ids).each do |proxy_order| place_order_for(proxy_order) end @@ -30,13 +29,15 @@ def proxy_orders def place_order_for(proxy_order) JobLogger.logger.info("Placing Order for Proxy Order #{proxy_order.id}") initialise_order(proxy_order) - return record_subscription_issue(proxy_order.subscription) if proxy_order.order.nil? + return unless proxy_order.order.present? + proxy_order.update_column(:placed_at, Time.now) place_order(proxy_order.order) end def initialise_order(proxy_order) proxy_order.initialise_order! + record_subscription_issue(proxy_order.subscription) if proxy_order.order.nil? rescue StandardError => e Bugsnag.notify(e, subscription: proxy_order.subscription, proxy_order: proxy_order) end From 61a39311b47aab852a51eef4f5581761751b3266 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 29 Mar 2021 16:08:27 -0700 Subject: [PATCH 5/6] improve specs --- .../order_management/subscriptions/summary_spec.rb | 14 +++++++++----- spec/jobs/subscription_placement_job_spec.rb | 4 ++-- spec/mailers/subscription_mailer_spec.rb | 10 ++++++++-- 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb b/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb index 731a123d17a..dcfd2140087 100644 --- a/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb +++ b/engines/order_management/spec/services/order_management/subscriptions/summary_spec.rb @@ -92,13 +92,17 @@ module Subscriptions end context "when there are also subscription issues" do - let(:subscription_issues) { [101, 212] } + let(:subscription) { double(:subscription, id: 101) } + let(:order) { double(:order, id: 1) } + + before do + summary.record_order(order) + summary.record_success(order) + summary.record_subscription_issue(subscription) + end it "includes subscription issues in the count" do - summary.instance_variable_set(:@order_ids, order_ids) - summary.instance_variable_set(:@success_ids, success_ids) - summary.instance_variable_set(:@subscription_issues, subscription_issues) - expect(summary.issue_count).to eq 4 # 7, 9, 101, 212 + expect(summary.issue_count).to eq 1 end end end diff --git a/spec/jobs/subscription_placement_job_spec.rb b/spec/jobs/subscription_placement_job_spec.rb index bf4b79c59a9..0f43d81781c 100644 --- a/spec/jobs/subscription_placement_job_spec.rb +++ b/spec/jobs/subscription_placement_job_spec.rb @@ -220,13 +220,13 @@ context "when the proxy order fails to generate an order" do before do - allow(proxy_order).to receive(:initialise_order!) { nil } + allow(proxy_order).to receive(:order) { nil } end it "records an error " do expect(job).to receive(:record_subscription_issue) expect(job).to_not receive(:place_order) - job.send(:place_proxy_order, proxy_order) + job.send(:place_order_for, proxy_order) end end end diff --git a/spec/mailers/subscription_mailer_spec.rb b/spec/mailers/subscription_mailer_spec.rb index 0cd77c047a2..6134946db62 100644 --- a/spec/mailers/subscription_mailer_spec.rb +++ b/spec/mailers/subscription_mailer_spec.rb @@ -229,7 +229,10 @@ let(:body) { strip_tags(SubscriptionMailer.deliveries.last.body.encoded) } let(:scope) { "subscription_mailer" } - before { allow(summary).to receive(:unrecorded_ids) { [] } } + before do + allow(summary).to receive(:unrecorded_ids) { [] } + allow(summary).to receive(:subscription_issues) { [] } + end context "when no issues were encountered while processing subscriptions" do before do @@ -339,7 +342,10 @@ let(:body) { strip_tags(SubscriptionMailer.deliveries.last.body.encoded) } let(:scope) { "subscription_mailer" } - before { allow(summary).to receive(:unrecorded_ids) { [] } } + before do + allow(summary).to receive(:unrecorded_ids) { [] } + allow(summary).to receive(:subscription_issues) { [] } + end context "when no issues were encountered while processing subscriptions" do before do From c30ced20601173afdce0d8d07cc94d43418e7666 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Sun, 4 Apr 2021 12:27:46 -0700 Subject: [PATCH 6/6] put Time.zone back --- app/jobs/subscription_placement_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb index 793c0b7f7a5..69bcaea69e8 100644 --- a/app/jobs/subscription_placement_job.rb +++ b/app/jobs/subscription_placement_job.rb @@ -31,7 +31,7 @@ def place_order_for(proxy_order) initialise_order(proxy_order) return unless proxy_order.order.present? - proxy_order.update_column(:placed_at, Time.now) + proxy_order.update_column(:placed_at, Time.zone.now) place_order(proxy_order.order) end