From 6cc403cd929ab72bc1efa4e3fbf907df7da5d25f Mon Sep 17 00:00:00 2001 From: Rob Harrington Date: Fri, 10 Jul 2015 11:17:51 +0800 Subject: [PATCH] Preventing double creation of invoices by recognising completed invoices within the specifed period --- app/jobs/finalize_user_invoices.rb | 5 +- app/jobs/update_user_invoices.rb | 26 ++++-- app/models/spree/user_decorator.rb | 8 +- app/views/admin/account/show.html.haml | 6 +- spec/jobs/finalize_user_invoices_spec.rb | 16 ++-- spec/jobs/update_user_invoices_spec.rb | 105 +++++++++++++++++++---- spec/models/spree/user_spec.rb | 16 ++-- 7 files changed, 127 insertions(+), 55 deletions(-) diff --git a/app/jobs/finalize_user_invoices.rb b/app/jobs/finalize_user_invoices.rb index 18d5d3b8b9..88e163f01e 100644 --- a/app/jobs/finalize_user_invoices.rb +++ b/app/jobs/finalize_user_invoices.rb @@ -14,8 +14,8 @@ class FinalizeUserInvoices def perform return unless settings_are_valid? - invoices = Spree::Order.where('distributor_id = (?) AND created_at >= (?) AND created_at <= (?) AND completed_at IS NULL', - @accounts_distributor, start_date + 1.day, end_date + 1.day) + invoices = Spree::Order.where('distributor_id = (?) AND created_at >= (?) AND created_at < (?) AND completed_at IS NULL', + @accounts_distributor, start_date, end_date) invoices.each do |invoice| finalize(invoice) @@ -27,7 +27,6 @@ class FinalizeUserInvoices # we can update these to read from those preferences invoice.payments.create(payment_method_id: Spree::Config.default_accounts_payment_method_id, amount: invoice.total) invoice.update_attribute(:shipping_method_id, Spree::Config.default_accounts_shipping_method_id) - while invoice.state != "complete" invoice.next end diff --git a/app/jobs/update_user_invoices.rb b/app/jobs/update_user_invoices.rb index b062d12fdf..9567c1e0e9 100644 --- a/app/jobs/update_user_invoices.rb +++ b/app/jobs/update_user_invoices.rb @@ -26,13 +26,25 @@ class UpdateUserInvoices def update_invoice_for(user, billable_periods) current_adjustments = [] - invoice = user.current_invoice + invoice = user.invoice_for(start_date, end_date) - billable_periods.reject{ |bp| bp.turnover == 0 }.each do |billable_period| - adjustment = invoice.adjustments.where(source_id: billable_period).first - adjustment ||= invoice.adjustments.new( adjustment_attrs_from(billable_period), :without_protection => true) - adjustment.update_attributes( label: adjustment_label_from(billable_period), amount: billable_period.bill ) - current_adjustments << adjustment + if invoice.persisted? && invoice.created_at != start_date + Bugsnag.notify(RuntimeError.new("InvoiceDateConflict"), { + start_date: start_date, + end_date: end_date, + existing_invoice: invoice.as_json + }) + elsif invoice.complete? + Bugsnag.notify(RuntimeError.new("InvoiceAlreadyFinalized"), { + invoice: invoice.as_json + }) + else + billable_periods.reject{ |bp| bp.turnover == 0 }.each do |billable_period| + adjustment = invoice.adjustments.where(source_id: billable_period).first + adjustment ||= invoice.adjustments.new( adjustment_attrs_from(billable_period), :without_protection => true) + adjustment.update_attributes( label: adjustment_label_from(billable_period), amount: billable_period.bill ) + current_adjustments << adjustment + end end clean_up_and_save(invoice, current_adjustments) @@ -74,6 +86,8 @@ class UpdateUserInvoices end if current_adjustments.any? + # Invoices should be "created" at the beginning of the period to which they apply + invoice.created_at = start_date unless invoice.persisted? invoice.save else Bugsnag.notify(RuntimeError.new("Empty Persisted Invoice"), { diff --git a/app/models/spree/user_decorator.rb b/app/models/spree/user_decorator.rb index fd61e1e709..acaea63d7f 100644 --- a/app/models/spree/user_decorator.rb +++ b/app/models/spree/user_decorator.rb @@ -18,7 +18,6 @@ Spree.user_class.class_eval do validate :limit_owned_enterprises - def known_users if admin? Spree::User.scoped @@ -49,10 +48,9 @@ Spree.user_class.class_eval do owned_enterprises(:reload).size < enterprise_limit end - def current_invoice - start_of_current_billing_period = (Time.now - 1.day).beginning_of_month - existing = orders.where('distributor_id = (?) AND created_at >= (?) AND completed_at IS NULL', - Spree::Config[:accounts_distributor_id], start_of_current_billing_period).first + def invoice_for(start_date, end_date) + existing = orders.where('distributor_id = (?) AND created_at >= (?) AND created_at < (?)', + Spree::Config[:accounts_distributor_id], start_date, end_date).first existing || orders.new(distributor_id: Spree::Config[:accounts_distributor_id]) end diff --git a/app/views/admin/account/show.html.haml b/app/views/admin/account/show.html.haml index 30d9eab0b3..acf4255d12 100644 --- a/app/views/admin/account/show.html.haml +++ b/app/views/admin/account/show.html.haml @@ -8,8 +8,6 @@ - @invoices.order('created_at DESC').each do |invoice| - month = (invoice.created_at.localtime - 1.day) - - range = "#{month.beginning_of_month.strftime("%d/%m/%y")}" - - range += " - #{[month.end_of_month, Time.now].min.strftime("%d/%m/%y")}" .row.invoice_title .eight.columns.alpha %h4= "#{month.strftime("%b %Y")}#{( invoice.completed_at ? '' : '*' )}" @@ -32,11 +30,11 @@ %td= adjustment.display_amount - invoice.adjustments.where('source_type <> (?)', "BillablePeriod").each do |adjustment| %tr - %td= range + %td   %td= adjustment.label %td= adjustment.display_amount %tr.total - %td= range + %td   %td TOTAL %td= invoice.display_total diff --git a/spec/jobs/finalize_user_invoices_spec.rb b/spec/jobs/finalize_user_invoices_spec.rb index 00a9387fdc..f474b83d13 100644 --- a/spec/jobs/finalize_user_invoices_spec.rb +++ b/spec/jobs/finalize_user_invoices_spec.rb @@ -14,9 +14,9 @@ describe FinalizeUserInvoices do let!(:accounts_distributor) { create(:distributor_enterprise) } let!(:invoice1) { create(:order, distributor: accounts_distributor, created_at: start_of_july - 10.days, completed_at: nil) } let!(:invoice2) { create(:order, distributor: accounts_distributor, created_at: start_of_july - 10.days, completed_at: start_of_july - 10.days) } - let!(:invoice3) { create(:order, distributor: accounts_distributor, created_at: start_of_july + 3.hours, completed_at: nil) } + let!(:invoice3) { create(:order, distributor: accounts_distributor, created_at: start_of_july, completed_at: nil) } let!(:invoice4) { create(:order, distributor: accounts_distributor, created_at: start_of_july + 10.days, completed_at: nil) } - let!(:invoice5) { create(:order, distributor: accounts_distributor, created_at: start_of_july - 30.days + 3.hours, completed_at: nil) } + let!(:invoice5) { create(:order, distributor: accounts_distributor, created_at: start_of_july - 30.days, completed_at: nil) } before do allow(Enterprise).to receive(:find_by_id) { accounts_distributor } @@ -70,13 +70,13 @@ describe FinalizeUserInvoices do context "and no date arguments are passed to the job" do travel_to(3.days) - it "finalizes the uncompleted orders for accounts_distributor created in the previous calendar month (or on the 1st of this month)" do + it "finalizes the uncompleted orders for accounts_distributor created in the previous calendar month" do finalizer.perform expect(finalizer).to have_received(:finalize).with(invoice1) - expect(finalizer).to have_received(:finalize).with(invoice3) + expect(finalizer).to_not have_received(:finalize).with(invoice3) expect(finalizer).to_not have_received(:finalize).with(invoice2) expect(finalizer).to_not have_received(:finalize).with(invoice4) - expect(finalizer).to_not have_received(:finalize).with(invoice5) + expect(finalizer).to have_received(:finalize).with(invoice5) end end @@ -90,13 +90,13 @@ describe FinalizeUserInvoices do context "that ends in the past" do travel_to(3.hours) - it "finalizes the uncompleted orders for accounts_distributor created in the specified calendar month (or on the first of the following month)" do + it "finalizes the uncompleted orders for accounts_distributor created in the specified calendar month" do finalizer.perform expect(finalizer).to have_received(:finalize).with(invoice1) - expect(finalizer).to have_received(:finalize).with(invoice3) + expect(finalizer).to_not have_received(:finalize).with(invoice3) expect(finalizer).to_not have_received(:finalize).with(invoice2) expect(finalizer).to_not have_received(:finalize).with(invoice4) - expect(finalizer).to_not have_received(:finalize).with(invoice5) + expect(finalizer).to have_received(:finalize).with(invoice5) end end diff --git a/spec/jobs/update_user_invoices_spec.rb b/spec/jobs/update_user_invoices_spec.rb index dd131e623f..233aa31359 100644 --- a/spec/jobs/update_user_invoices_spec.rb +++ b/spec/jobs/update_user_invoices_spec.rb @@ -94,9 +94,10 @@ describe UpdateUserInvoices do let(:invoice) { create(:order, user: user) } before do - allow(user).to receive(:current_invoice) { invoice } + allow(user).to receive(:invoice_for) { invoice } allow(updater).to receive(:clean_up_and_save) allow(updater).to receive(:finalize) + allow(Bugsnag).to receive(:notify) end context "on the first of the month" do @@ -105,18 +106,52 @@ describe UpdateUserInvoices do before do allow(updater).to receive(:adjustment_label_from).exactly(1).times.and_return("Old Item") allow(old_billable_period).to receive(:bill) { 666.66 } - updater.update_invoice_for(user, [old_billable_period]) end - it "creates adjustments for each billing item" do - adjustments = invoice.adjustments - expect(adjustments.map(&:source_id)).to eq [old_billable_period.id] - expect(adjustments.map(&:amount)).to eq [666.66] - expect(adjustments.map(&:label)).to eq ["Old Item"] + context "where the invoice was not created at start_date" do + before do + invoice.update_attribute(:created_at, start_of_july - 1.month + 1.day) + updater.update_invoice_for(user, [old_billable_period]) + end + + it "snags a bug" do + expect(Bugsnag).to have_received(:notify) + end end - it "cleans up and saves the invoice" do - expect(updater).to have_received(:clean_up_and_save).with(invoice, anything).once + context "where the invoice was created at start_date" do + before do + invoice.update_attribute(:created_at, start_of_july - 1.month) + end + + context "where the invoice is already complete" do + before do + allow(invoice).to receive(:complete?) { true } + updater.update_invoice_for(user, [old_billable_period]) + end + + it "snags a bug" do + expect(Bugsnag).to have_received(:notify) + end + end + + context "where the invoice is not complete" do + before do + allow(invoice).to receive(:complete?) { false } + updater.update_invoice_for(user, [old_billable_period]) + end + + it "creates adjustments for each billing item" do + adjustments = invoice.adjustments + expect(adjustments.map(&:source_id)).to eq [old_billable_period.id] + expect(adjustments.map(&:amount)).to eq [666.66] + expect(adjustments.map(&:label)).to eq ["Old Item"] + end + + it "cleans up and saves the invoice" do + expect(updater).to have_received(:clean_up_and_save).with(invoice, anything).once + end + end end end @@ -127,18 +162,52 @@ describe UpdateUserInvoices do allow(updater).to receive(:adjustment_label_from).exactly(2).times.and_return("BP1 Item", "BP2 Item") allow(billable_period1).to receive(:bill) { 123.45 } allow(billable_period2).to receive(:bill) { 543.21 } - updater.update_invoice_for(user, [billable_period1, billable_period2]) end - it "creates adjustments for each billing item" do - adjustments = invoice.adjustments - expect(adjustments.map(&:source_id)).to eq [billable_period1.id, billable_period2.id] - expect(adjustments.map(&:amount)).to eq [123.45, 543.21] - expect(adjustments.map(&:label)).to eq ["BP1 Item", "BP2 Item"] + context "where the invoice was not created at start_date" do + before do + invoice.update_attribute(:created_at, start_of_july + 1.day) + updater.update_invoice_for(user, [billable_period1, billable_period2]) + end + + it "snags a bug" do + expect(Bugsnag).to have_received(:notify) + end end - it "cleans up and saves the invoice" do - expect(updater).to have_received(:clean_up_and_save).with(invoice, anything).once + context "where the invoice was created at start_date" do + before do + invoice.update_attribute(:created_at, start_of_july) + end + + context "where the invoice is already complete" do + before do + allow(invoice).to receive(:complete?) { true } + updater.update_invoice_for(user, [billable_period1, billable_period2]) + end + + it "snags a bug" do + expect(Bugsnag).to have_received(:notify) + end + end + + context "where the invoice is not complete" do + before do + allow(invoice).to receive(:complete?) { false } + updater.update_invoice_for(user, [billable_period1, billable_period2]) + end + + it "creates adjustments for each billing item" do + adjustments = invoice.adjustments + expect(adjustments.map(&:source_id)).to eq [billable_period1.id, billable_period2.id] + expect(adjustments.map(&:amount)).to eq [123.45, 543.21] + expect(adjustments.map(&:label)).to eq ["BP1 Item", "BP2 Item"] + end + + it "cleans up and saves the invoice" do + expect(updater).to have_received(:clean_up_and_save).with(invoice, anything).once + end + end end end end @@ -282,7 +351,7 @@ describe UpdateUserInvoices do end context "when an invoice currently exists" do - let!(:invoice) { create(:order, user: user, distributor: accounts_distributor, created_at: start_of_july + 3.days) } + let!(:invoice) { create(:order, user: user, distributor: accounts_distributor, created_at: start_of_july) } let!(:billable_adjustment) { create(:adjustment, adjustable: invoice, source_type: 'BillablePeriod') } before do diff --git a/spec/models/spree/user_spec.rb b/spec/models/spree/user_spec.rb index b096055d85..e2eecb5b64 100644 --- a/spec/models/spree/user_spec.rb +++ b/spec/models/spree/user_spec.rb @@ -80,7 +80,7 @@ describe Spree.user_class do end end - describe "current_invoice" do + describe "invoice_for" do let!(:user) { create(:user) } let!(:accounts_distributor) { create(:distributor_enterprise) } let!(:start_of_month) { Time.now.beginning_of_month } @@ -89,20 +89,16 @@ describe Spree.user_class do Spree::Config.accounts_distributor_id = accounts_distributor.id end - context "where no relevant invoice exists for the current month" do + context "where no relevant invoice exists for the given period" do # Created during previous month - let!(:order1) { create(:order, user: user, created_at: start_of_month - 20.days, completed_at: nil, distributor: accounts_distributor) } - # Already Completed - let!(:order2) { create(:order, user: user, created_at: start_of_month + 3.hours, completed_at: start_of_month + 3.days, distributor: accounts_distributor) } + let!(:order1) { create(:order, user: user, created_at: start_of_month - 3.hours, completed_at: nil, distributor: accounts_distributor) } # Incorrect distributor let!(:order3) { create(:order, user: user, created_at: start_of_month + 3.hours, completed_at: nil, distributor: create(:distributor_enterprise)) } # Incorrect user let!(:order4) { create(:order, user: create(:user), created_at: start_of_month + 3.hours, completed_at: nil, distributor: accounts_distributor) } - around { |example| Timecop.travel(start_of_month + 20.days) { example.run } } - it "creates a new invoice" do - current_invoice = user.current_invoice + current_invoice = user.invoice_for(start_of_month, start_of_month + 20.days) expect(current_invoice).to be_a_new Spree::Order expect(current_invoice.completed_at).to be nil expect(current_invoice.distributor).to eq accounts_distributor @@ -113,10 +109,8 @@ describe Spree.user_class do context "where an invoice exists for the current month" do let!(:order) { create(:order, user: user, created_at: start_of_month + 3.hours, completed_at: nil, distributor: accounts_distributor) } - around { |example| Timecop.travel(start_of_month + 20.days) { example.run } } - it "returns the existing invoice" do - current_invoice = user.current_invoice + current_invoice = user.invoice_for(start_of_month, start_of_month + 20.days) expect(current_invoice).to eq order end end