Merge pull request #7207 from andrewpbrett/fix-nil-price-subs

Report subscription failures

.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

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
@@ -13,8 +12,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,11 +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 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

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

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

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)

engines/order_management/spec/services/order_management/subscriptions/summarizer_spec.rb

@@ -94,6 +94,20 @@ module Subscriptions
             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

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,21 @@ 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) { 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

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(: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

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