diff --git a/.rubocop_manual_todo.yml b/.rubocop_manual_todo.yml index 4789dca9fe..740c3171aa 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/app/jobs/subscription_placement_job.rb b/app/jobs/subscription_placement_job.rb index c56df548d6..69bcaea69e 100644 --- a/app/jobs/subscription_placement_job.rb +++ b/app/jobs/subscription_placement_job.rb @@ -3,7 +3,6 @@ require 'order_management/subscriptions/summarizer' 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 @@ -13,8 +12,8 @@ class SubscriptionPlacementJob < ActiveJob::Base 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,11 +29,15 @@ class SubscriptionPlacementJob < ActiveJob::Base def place_order_for(proxy_order) JobLogger.logger.info("Placing Order for Proxy Order #{proxy_order.id}") initialise_order(proxy_order) + return unless proxy_order.order.present? + + proxy_order.update_column(:placed_at, Time.zone.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 diff --git a/app/views/subscription_mailer/_summary_detail.html.haml b/app/views/subscription_mailer/_summary_detail.html.haml index 9e99633dcb..a566b2396d 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 86c7deadea..32c925fc03 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 @@ module OrderManagement 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 @@ module OrderManagement 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 c37272c6d3..e16b116bf8 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 @@ module OrderManagement 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 @@ module OrderManagement end def issue_count - (@order_ids - @success_ids).count + (@order_ids - @success_ids).count + @subscription_issues.count end def orders_affected_by(type) 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 0c47277e47..8333eda906 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,20 @@ module OrderManagement end end end + + describe "#record_subscription_issue" do + let(:subscription) { double(:subscription, shop_id: 1) } + + 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 + 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 454e2e5ba3..dcfd214008 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 OrderManagement 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,21 @@ module OrderManagement 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) { 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 + expect(summary.issue_count).to eq 1 + 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 db88322ffc..0f43d81781 100644 --- a/spec/jobs/subscription_placement_job_spec.rb +++ b/spec/jobs/subscription_placement_job_spec.rb @@ -217,6 +217,18 @@ describe SubscriptionPlacementJob do end end end + + context "when the proxy order fails to generate an order" do + before do + 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_order_for, proxy_order) + end + end end describe "#send_placement_email" do diff --git a/spec/mailers/subscription_mailer_spec.rb b/spec/mailers/subscription_mailer_spec.rb index 3ea46f0d5a..58f6b849d4 100644 --- a/spec/mailers/subscription_mailer_spec.rb +++ b/spec/mailers/subscription_mailer_spec.rb @@ -229,7 +229,10 @@ describe SubscriptionMailer, type: :mailer do 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 @@ describe SubscriptionMailer, type: :mailer do 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