From 799d35f187d1770b8899818c9ac24c4905537e34 Mon Sep 17 00:00:00 2001 From: Konrad Date: Mon, 1 Mar 2021 20:44:22 +0100 Subject: [PATCH 01/61] Updated stylesheets for clean footer Updated the stylesheets to have precise alignment fo elements and consistent font sizes throughout the front-end footer. --- app/assets/stylesheets/darkswarm/footer.scss | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/app/assets/stylesheets/darkswarm/footer.scss b/app/assets/stylesheets/darkswarm/footer.scss index 9826970141..4893489635 100644 --- a/app/assets/stylesheets/darkswarm/footer.scss +++ b/app/assets/stylesheets/darkswarm/footer.scss @@ -5,7 +5,7 @@ footer { .row { p a { - font-size: 0.875rem; + font-size: 1rem; } a, a * { @@ -108,8 +108,8 @@ footer { } .social-icons { - margin-bottom: 0.25rem; - margin-top: 0.75rem; + margin-bottom: 0.9rem; + padding-top: 0.1rem; a { i { @@ -128,5 +128,9 @@ footer { } } } + + .legal a { + font-size: 0.875rem; + } } } From 3d8cfc4ccd5fa9aa942288cbce368f0769ef7062 Mon Sep 17 00:00:00 2001 From: Konrad Date: Mon, 1 Mar 2021 20:47:36 +0100 Subject: [PATCH 02/61] Updated footer for clean footer Updated the footer to have precise alignment of elements and consistent font sizes throughout the front-end footer. --- app/views/shared/_footer.html.haml | 50 +++++++++++++++--------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/app/views/shared/_footer.html.haml b/app/views/shared/_footer.html.haml index 2ba6278bec..dd7cd4a21c 100644 --- a/app/views/shared/_footer.html.haml +++ b/app/views/shared/_footer.html.haml @@ -31,25 +31,26 @@ // This is the instance-managed set of links: %h4 = t '.footer_contact_headline' - %p.social-icons - - if ContentConfig.footer_facebook_url.present? - %a{href: ContentConfig.footer_facebook_url} - %i.ofn-i_044-facebook - - if ContentConfig.footer_twitter_url.present? - %a{href: ContentConfig.footer_twitter_url} - %i.ofn-i_041-twitter - - if ContentConfig.footer_instagram_url.present? - %a{href: ContentConfig.footer_instagram_url} - %i.ofn-i_043-instagram - - if ContentConfig.footer_linkedin_url.present? - %a{href: ContentConfig.footer_linkedin_url} - %i.ofn-i_042-linkedin - - if ContentConfig.footer_googleplus_url.present? - %a{href: ContentConfig.footer_googleplus_url} - %i.ofn-i_046-g - - if ContentConfig.footer_pinterest_url.present? - %a{href: ContentConfig.footer_pinterest_url} - %i.ofn-i_045-pintrest + - if ContentConfig.footer_facebook_url.present? || ContentConfig.footer_twitter_url.present? || ContentConfig.footer_instagram_url.present? || ContentConfig.footer_linkedin_url.present? || ContentConfig.footer_googleplus_url.present? || ContentConfig.footer_pinterest_url.present? + %p.social-icons + - if ContentConfig.footer_facebook_url.present? + %a{href: ContentConfig.footer_facebook_url} + %i.ofn-i_044-facebook + - if ContentConfig.footer_twitter_url.present? + %a{href: ContentConfig.footer_twitter_url} + %i.ofn-i_041-twitter + - if ContentConfig.footer_instagram_url.present? + %a{href: ContentConfig.footer_instagram_url} + %i.ofn-i_043-instagram + - if ContentConfig.footer_linkedin_url.present? + %a{href: ContentConfig.footer_linkedin_url} + %i.ofn-i_042-linkedin + - if ContentConfig.footer_googleplus_url.present? + %a{href: ContentConfig.footer_googleplus_url} + %i.ofn-i_046-g + - if ContentConfig.footer_pinterest_url.present? + %a{href: ContentConfig.footer_pinterest_url} + %i.ofn-i_045-pintrest - if ContentConfig.footer_email.present? %p %a{href: ContentConfig.footer_email.reverse, mailto: true, target: '_blank'} @@ -92,7 +93,7 @@ %hr.hr-light %br - .row + .row.legal .small-12.medium-3.medium-offset-2.columns.text-left %a{href: main_app.root_path} %img{src: ContentConfig.footer_logo.url, width: "220"} @@ -107,10 +108,9 @@ %p.text-small = t '.footer_legal_text_html', {content_license: link_to('CC BY-SA 3.0', 'https://creativecommons.org/licenses/by-sa/3.0/'), code_license: link_to('AGPL 3', 'https://tldrlegal.com/license/gnu-affero-general-public-license-v3-(agpl-3.0)' )} %p.text-small - %div - - if Spree::Config.privacy_policy_url.present? - = t '.footer_data_text_with_privacy_policy_html', {cookies_policy: cookies_policy_link.html_safe, privacy_policy: privacy_policy_link.html_safe } - - else - = t '.footer_data_text_without_privacy_policy_html', {cookies_policy: cookies_policy_link.html_safe } + - if Spree::Config.privacy_policy_url.present? + = t '.footer_data_text_with_privacy_policy_html', {cookies_policy: cookies_policy_link.html_safe, privacy_policy: privacy_policy_link.html_safe } + - else + = t '.footer_data_text_without_privacy_policy_html', {cookies_policy: cookies_policy_link.html_safe } .medium-2.columns.text-center / Placeholder From a3b2a25ccf14eb79181bb316f4bdb287892569e9 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 9 Feb 2021 15:19:33 +0100 Subject: [PATCH 03/61] Refactor and reuse :not_state scope --- app/models/spree/order.rb | 2 +- lib/open_food_network/order_cycle_management_report.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 71d76036df..c89c5572e0 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -131,7 +131,7 @@ module Spree } scope :not_state, lambda { |state| - where("state != ?", state) + where.not(state: state) } # All the states an order can be in after completing the checkout diff --git a/lib/open_food_network/order_cycle_management_report.rb b/lib/open_food_network/order_cycle_management_report.rb index 1858469c6e..2e562c876a 100644 --- a/lib/open_food_network/order_cycle_management_report.rb +++ b/lib/open_food_network/order_cycle_management_report.rb @@ -49,7 +49,7 @@ module OpenFoodNetwork if FeatureToggle.enabled?(:customer_balance, @user) Spree::Order. finalized. - where.not(spree_orders: { state: :canceled }). + not_state(:canceled). distributed_by_user(@user). managed_by(@user). search(params[:q]) From 49dfccfb519de5543c8071964660fee21f33c40f Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 15 Feb 2021 12:47:45 +0100 Subject: [PATCH 04/61] Unit test #columns method At least, this covers what we're investigating now. --- .../reports/bulk_coop/bulk_coop_report.rb | 1 + .../bulk_coop/bulk_coop_report_spec.rb | 26 ++++++++++++++++--- 2 files changed, 24 insertions(+), 3 deletions(-) diff --git a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb index c9c8b822f1..a826407179 100644 --- a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb +++ b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb @@ -14,6 +14,7 @@ module OrderManagement ].freeze attr_reader :params + def initialize(user, params = {}, render_table = false) @params = params @user = user diff --git a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb index 5a580481de..c7141b6796 100644 --- a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb +++ b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb @@ -3,7 +3,12 @@ require 'spec_helper' describe OrderManagement::Reports::BulkCoop::BulkCoopReport do + subject { OrderManagement::Reports::BulkCoop::BulkCoopReport.new user, params, true } + let(:user) { create(:admin_user) } + describe "fetching orders" do + let(:params) { {} } + let(:d1) { create(:distributor_enterprise) } let(:oc1) { create(:simple_order_cycle) } let(:o1) { create(:order, completed_at: 1.day.ago, order_cycle: oc1, distributor: d1) } @@ -12,9 +17,6 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do before { o1.line_items << li1 } context "as a site admin" do - let(:user) { create(:admin_user) } - subject { OrderManagement::Reports::BulkCoop::BulkCoopReport.new user, {}, true } - it "fetches completed orders" do o2 = create(:order) o2.line_items << build(:line_item) @@ -124,4 +126,22 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do end end end + + describe '#columns' do + context 'when report type is bulk_coop_customer_payments' do + let(:params) { { report_type: 'bulk_coop_customer_payments' } } + + it 'returns' do + expect(subject.columns).to eq( + [ + :order_billing_address_name, + :order_completed_at, + :customer_payments_total_cost, + :customer_payments_amount_owed, + :customer_payments_amount_paid, + ] + ) + end + end + end end From 58ffd00f4abf7fcca7436227977b2df3bb731a7f Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 15 Feb 2021 12:49:31 +0100 Subject: [PATCH 05/61] Extract private method This was initially intended to cache the result of the `#map` and `#uniq` calls but we're not confident enough and don't want to scopecreep this. It's still worth to point out that this is what we need, line items' `unique orders`. Hopefully, next time we find a way to optimize it. --- .../reports/bulk_coop/bulk_coop_report.rb | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb index a826407179..ac318c00dc 100644 --- a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb +++ b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb @@ -159,15 +159,19 @@ module OrderManagement end def customer_payments_total_cost(line_items) - line_items.map(&:order).uniq.sum(&:total) + unique_orders(line_items).sum(&:total) end def customer_payments_amount_owed(line_items) - line_items.map(&:order).uniq.sum(&:outstanding_balance) + unique_orders(line_items).sum(&:outstanding_balance) end def customer_payments_amount_paid(line_items) - line_items.map(&:order).uniq.sum(&:payment_total) + unique_orders(line_items).sum(&:payment_total) + end + + def unique_orders(line_items) + line_items.map(&:order).uniq end def empty_cell(_line_items) From 197d7873965bdf77b481d284d6e9b11a27f3ca5d Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 15 Feb 2021 18:26:23 +0100 Subject: [PATCH 06/61] Unit-test OrderDataMasker --- spec/services/order_data_masker_spec.rb | 90 +++++++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 spec/services/order_data_masker_spec.rb diff --git a/spec/services/order_data_masker_spec.rb b/spec/services/order_data_masker_spec.rb new file mode 100644 index 0000000000..70aeab7bd9 --- /dev/null +++ b/spec/services/order_data_masker_spec.rb @@ -0,0 +1,90 @@ +require 'spec_helper' + +describe OrderDataMasker do + describe '#call' do + let(:distributor) { create(:enterprise) } + let(:order) { create(:order, distributor: distributor, ship_address: create(:address)) } + + context 'when displaying customer names is allowed' do + before { distributor.preferences[:show_customer_names_to_suppliers] = true } + + it 'masks personal addresses and email' do + described_class.new(order).call + + expect(order.bill_address.attributes).to include( + 'phone' => '', + 'address1' => '', + 'address2' => '', + 'city' => '', + 'zipcode' => '', + 'state_id' => nil + ) + + expect(order.ship_address.attributes).to include( + 'phone' => '', + 'address1' => '', + 'address2' => '', + 'city' => '', + 'zipcode' => '', + 'state_id' => nil + ) + + expect(order.email).to eq(I18n.t('admin.reports.hidden')) + end + + it 'does not mask the full name' do + described_class.new(order).call + + expect(order.bill_address.attributes).not_to include( + firstname: I18n.t('admin.reports.hidden'), + lastname: '' + ) + expect(order.ship_address.attributes).not_to include( + firstname: I18n.t('admin.reports.hidden'), + lastname: '' + ) + end + end + + context 'when displaying customer names is not allowed' do + before { distributor.preferences[:show_customer_names_to_suppliers] = false } + + it 'masks personal addresses and email' do + described_class.new(order).call + + expect(order.bill_address.attributes).to include( + 'phone' => '', + 'address1' => '', + 'address2' => '', + 'city' => '', + 'zipcode' => '', + 'state_id' => nil + ) + + expect(order.ship_address.attributes).to include( + 'phone' => '', + 'address1' => '', + 'address2' => '', + 'city' => '', + 'zipcode' => '', + 'state_id' => nil + ) + + expect(order.email).to eq(I18n.t('admin.reports.hidden')) + end + + it 'masks the full name' do + described_class.new(order).call + + expect(order.bill_address.attributes).to include( + 'firstname' => I18n.t('admin.reports.hidden'), + 'lastname' => '' + ) + expect(order.ship_address.attributes).to include( + 'firstname' => I18n.t('admin.reports.hidden'), + 'lastname' => '' + ) + end + end + end +end From b7335e12e9d2b447045c858170662615d1247542 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 15 Feb 2021 19:03:16 +0100 Subject: [PATCH 07/61] Add first basic unit test to Reports::LineItems This is quite hard and tedious due to its tight coupling with Permissions::Order but sets the path to adding more of these and eventually refactoring this class in the future. --- .../reports/line_items_spec.rb | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 spec/lib/open_food_network/reports/line_items_spec.rb diff --git a/spec/lib/open_food_network/reports/line_items_spec.rb b/spec/lib/open_food_network/reports/line_items_spec.rb new file mode 100644 index 0000000000..0cd21534a1 --- /dev/null +++ b/spec/lib/open_food_network/reports/line_items_spec.rb @@ -0,0 +1,54 @@ +require 'spec_helper' +require 'open_food_network/reports/line_items' + +describe OpenFoodNetwork::Reports::LineItems do + subject(:reports_line_items) { described_class.new(order_permissions, params) } + + # This object lets us add some test coverage despite the very deep coupling between the class + # under test and the various objects it depends on. Other more common moking strategies where very + # hard. + class FakeOrderPermissions + def initialize(line_item) + @relation = Spree::LineItem.where(id: line_item.id) + end + + def visible_line_items + relation + end + + def editable_line_items + line_item = FactoryBot.create(:line_item) + Spree::LineItem.where(id: line_item.id) + end + + private + + attr_reader :relation + end + + class FakeRansackResult + attr_reader :result + + def initialize(result) + @result = result + end + end + + describe '#list' do + let!(:order) { create(:order, distributor: create(:enterprise)) } + let!(:line_item) { create(:line_item, order: order) } + + let(:order_permissions) { FakeOrderPermissions.new(line_item) } + let(:params) { {} } + + before do + orders_relation = Spree::Order.where(id: order.id) + allow(reports_line_items).to receive(:search_orders) { FakeRansackResult.new(orders_relation) } + end + + it 'returns masked data' do + line_items = reports_line_items.list + expect(line_items.first.order.email).to eq(I18n.t('admin.reports.hidden')) + end + end +end From d00a35e12cbb51201544a042f053e376bb291e39 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 15 Feb 2021 19:11:19 +0100 Subject: [PATCH 08/61] Split long statement --- lib/open_food_network/reports/line_items.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lib/open_food_network/reports/line_items.rb b/lib/open_food_network/reports/line_items.rb index 7476dc85bf..e1c1dfeaae 100644 --- a/lib/open_food_network/reports/line_items.rb +++ b/lib/open_food_network/reports/line_items.rb @@ -23,10 +23,9 @@ module OpenFoodNetwork end editable_line_items = editable_line_items(line_items) + without_editable_line_items = line_items.reject { |li| editable_line_items.include? li } - line_items.reject{ |li| - editable_line_items.include? li - }.each do |line_item| + without_editable_line_items.each do |line_item| OrderDataMasker.new(line_item.order).call end From c69f0baf9f79306a9a281be4501fc753077a21e3 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 15 Feb 2021 19:21:55 +0100 Subject: [PATCH 09/61] Extract specs related to outstanding_balance --- .../spree/order/outstanding_balance_spec.rb | 220 ++++++++++++++++++ spec/models/spree/order_spec.rb | 31 --- 2 files changed, 220 insertions(+), 31 deletions(-) create mode 100644 spec/models/spree/order/outstanding_balance_spec.rb diff --git a/spec/models/spree/order/outstanding_balance_spec.rb b/spec/models/spree/order/outstanding_balance_spec.rb new file mode 100644 index 0000000000..d4ba269d30 --- /dev/null +++ b/spec/models/spree/order/outstanding_balance_spec.rb @@ -0,0 +1,220 @@ +require 'spec_helper' + +describe Spree::Order do + let(:order) { build(:order) } + + context "#outstanding_balance" do + context 'when orders are in cart state' do + before do + create(:order, total: order_total, payment_total: 0, state: 'cart') + create(:order, total: order_total, payment_total: 0, state: 'cart') + end + + it 'returns the order balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(-order_total) + end + end + + context 'when orders are in address state' do + before do + create(:order, total: order_total, payment_total: 0, state: 'address') + create(:order, total: order_total, payment_total: 50, state: 'address') + end + + it 'returns the order balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(-order_total) + end + end + + context 'when orders are in delivery state' do + before do + create(:order, total: order_total, payment_total: 0, state: 'delivery') + create(:order, total: order_total, payment_total: 50, state: 'delivery') + end + + it 'returns the order balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(-order_total) + end + end + + context 'when orders are in payment state' do + before do + create(:order, total: order_total, payment_total: 0, state: 'payment') + create(:order, total: order_total, payment_total: 50, state: 'payment') + end + + it 'returns the order balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(-order_total) + end + end + + context 'when no orders where paid' do + before do + order = create(:order, total: order_total, payment_total: 0) + order.update_attribute(:state, 'complete') + order = create(:order, total: order_total, payment_total: 0) + order.update_attribute(:state, 'complete') + end + + it 'returns the customer balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(-order_total) + end + end + + context 'when an order was paid' do + let(:payment_total) { order_total } + + before do + order = create(:order, total: order_total, payment_total: 0) + order.update_attribute(:state, 'complete') + order = create(:order, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'complete') + end + + it 'returns the customer balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(payment_total - 200.0) + end + end + + context 'when an order is canceled' do + let(:payment_total) { order_total } + let(:non_canceled_orders_total) { order_total } + + before do + create(:order, total: order_total, payment_total: order_total, state: 'canceled') + order = create(:order, total: order_total, payment_total: 0) + order.update_attribute(:state, 'complete') + end + + it 'returns the customer balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(payment_total) + end + end + + context 'when an order is resumed' do + let(:payment_total) { order_total } + + before do + order = create(:order, total: order_total, payment_total: 0) + order.update_attribute(:state, 'complete') + order = create(:order, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'resumed') + end + + it 'returns the customer balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(payment_total - 200.0) + end + end + + context 'when an order is in payment' do + let(:payment_total) { order_total } + + before do + order = create(:order, total: order_total, payment_total: 0) + order.update_attribute(:state, 'complete') + order = create(:order, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'payment') + end + + it 'returns the customer balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(payment_total - 200.0) + end + end + + context 'when an order is awaiting_return' do + let(:payment_total) { order_total } + + before do + order = create(:order, total: order_total, payment_total: 0) + order.update_attribute(:state, 'complete') + order = create(:order, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'awaiting_return') + end + + it 'returns the customer balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(payment_total - 200.0) + end + end + + context 'when an order is returned' do + let(:payment_total) { order_total } + let(:non_returned_orders_total) { order_total } + + before do + order = create(:order, total: order_total, payment_total: payment_total) + order.update_attribute(:state, 'returned') + order = create(:order, total: order_total, payment_total: 0) + order.update_attribute(:state, 'complete') + end + + it 'returns the customer balance' do + order = outstanding_balance.query.first + expect(order.balance_value).to eq(payment_total) + end + end + + context 'when there are no orders' do + it 'returns the order balance' do + orders = outstanding_balance.query + expect(orders).to be_empty + end + end + + it "should return positive amount when payment_total is less than total" do + order.payment_total = 20.20 + order.total = 30.30 + expect(order.outstanding_balance).to eq 10.10 + end + + it "should return negative amount when payment_total is greater than total" do + order.total = 8.20 + order.payment_total = 10.20 + expect(order.outstanding_balance).to be_within(0.001).of(-2.00) + end + end + + context "#outstanding_balance?" do + context 'when total is greater than payment_total' do + before do + order.total = 10.10 + order.payment_total = 9.50 + end + + it "returns true" do + expect(order.outstanding_balance?).to eq(true) + end + end + + context "when total is less than payment_total" do + before do + order.total = 8.25 + order.payment_total = 10.44 + end + + it "returns true" do + expect(order.outstanding_balance?).to eq(true) + end + end + + context "when total equals payment_total" do + before do + order.total = 10.10 + order.payment_total = 10.10 + end + + it 'returns false' do + expect(order.outstanding_balance?).to eq(false) + end + end + end +end diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index c886d5677d..f042e81c99 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -237,37 +237,6 @@ describe Spree::Order do end end - context "#outstanding_balance" do - it "should return positive amount when payment_total is less than total" do - order.payment_total = 20.20 - order.total = 30.30 - expect(order.outstanding_balance).to eq 10.10 - end - it "should return negative amount when payment_total is greater than total" do - order.total = 8.20 - order.payment_total = 10.20 - expect(order.outstanding_balance).to be_within(0.001).of(-2.00) - end - end - - context "#outstanding_balance?" do - it "should be true when total greater than payment_total" do - order.total = 10.10 - order.payment_total = 9.50 - expect(order.outstanding_balance?).to be_truthy - end - it "should be true when total less than payment_total" do - order.total = 8.25 - order.payment_total = 10.44 - expect(order.outstanding_balance?).to be_truthy - end - it "should be false when total equals payment_total" do - order.total = 10.10 - order.payment_total = 10.10 - expect(order.outstanding_balance?).to be_falsy - end - end - context "#completed?" do it "should indicate if order is completed" do order.completed_at = nil From d1fde07535ac7c63ddffa4f8be180e285f016f6c Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 15 Feb 2021 19:47:08 +0100 Subject: [PATCH 10/61] Extend #outstanding_balance to mimic OustandingBalance --- app/models/spree/order.rb | 7 +- .../spree/order/outstanding_balance_spec.rb | 164 +++++------------- 2 files changed, 46 insertions(+), 125 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index c89c5572e0..3d8559af35 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -136,6 +136,7 @@ module Spree # All the states an order can be in after completing the checkout FINALIZED_STATES = %w(complete canceled resumed awaiting_return returned).freeze + FINALIZED_NON_SUCCESSFUL_STATES = %w(canceled returned).freeze scope :finalized, -> { where(state: FINALIZED_STATES) } @@ -394,7 +395,11 @@ module Spree end def outstanding_balance - total - payment_total + if state.in?(FINALIZED_NON_SUCCESSFUL_STATES) + -payment_total + else + total - payment_total + end end def outstanding_balance? diff --git a/spec/models/spree/order/outstanding_balance_spec.rb b/spec/models/spree/order/outstanding_balance_spec.rb index d4ba269d30..4963fe273d 100644 --- a/spec/models/spree/order/outstanding_balance_spec.rb +++ b/spec/models/spree/order/outstanding_balance_spec.rb @@ -1,216 +1,132 @@ require 'spec_helper' describe Spree::Order do - let(:order) { build(:order) } context "#outstanding_balance" do context 'when orders are in cart state' do - before do - create(:order, total: order_total, payment_total: 0, state: 'cart') - create(:order, total: order_total, payment_total: 0, state: 'cart') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'cart') } it 'returns the order balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(-order_total) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when orders are in address state' do - before do - create(:order, total: order_total, payment_total: 0, state: 'address') - create(:order, total: order_total, payment_total: 50, state: 'address') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'address') } it 'returns the order balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(-order_total) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when orders are in delivery state' do - before do - create(:order, total: order_total, payment_total: 0, state: 'delivery') - create(:order, total: order_total, payment_total: 50, state: 'delivery') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'delivery') } it 'returns the order balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(-order_total) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when orders are in payment state' do - before do - create(:order, total: order_total, payment_total: 0, state: 'payment') - create(:order, total: order_total, payment_total: 50, state: 'payment') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'payment') } it 'returns the order balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(-order_total) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when no orders where paid' do - before do - order = create(:order, total: order_total, payment_total: 0) - order.update_attribute(:state, 'complete') - order = create(:order, total: order_total, payment_total: 0) - order.update_attribute(:state, 'complete') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') } it 'returns the customer balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(-order_total) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when an order was paid' do - let(:payment_total) { order_total } - - before do - order = create(:order, total: order_total, payment_total: 0) - order.update_attribute(:state, 'complete') - order = create(:order, total: order_total, payment_total: payment_total) - order.update_attribute(:state, 'complete') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') } it 'returns the customer balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(payment_total - 200.0) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when an order is canceled' do - let(:payment_total) { order_total } - let(:non_canceled_orders_total) { order_total } - - before do - create(:order, total: order_total, payment_total: order_total, state: 'canceled') - order = create(:order, total: order_total, payment_total: 0) - order.update_attribute(:state, 'complete') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'canceled') } it 'returns the customer balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(payment_total) + expect(order.outstanding_balance).to eq(-10) end end context 'when an order is resumed' do - let(:payment_total) { order_total } - - before do - order = create(:order, total: order_total, payment_total: 0) - order.update_attribute(:state, 'complete') - order = create(:order, total: order_total, payment_total: payment_total) - order.update_attribute(:state, 'resumed') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'resumed') } it 'returns the customer balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(payment_total - 200.0) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when an order is in payment' do - let(:payment_total) { order_total } - - before do - order = create(:order, total: order_total, payment_total: 0) - order.update_attribute(:state, 'complete') - order = create(:order, total: order_total, payment_total: payment_total) - order.update_attribute(:state, 'payment') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'payment') } it 'returns the customer balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(payment_total - 200.0) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when an order is awaiting_return' do - let(:payment_total) { order_total } - - before do - order = create(:order, total: order_total, payment_total: 0) - order.update_attribute(:state, 'complete') - order = create(:order, total: order_total, payment_total: payment_total) - order.update_attribute(:state, 'awaiting_return') - end + let(:order) { build(:order, total: 100, payment_total: 10, state: 'awaiting_return') } it 'returns the customer balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(payment_total - 200.0) + expect(order.outstanding_balance).to eq(100 - 10) end end context 'when an order is returned' do - let(:payment_total) { order_total } - let(:non_returned_orders_total) { order_total } + let(:order) { build(:order, total: 100, payment_total: 10, state: 'returned') } - before do - order = create(:order, total: order_total, payment_total: payment_total) - order.update_attribute(:state, 'returned') - order = create(:order, total: order_total, payment_total: 0) - order.update_attribute(:state, 'complete') - end - - it 'returns the customer balance' do - order = outstanding_balance.query.first - expect(order.balance_value).to eq(payment_total) + it 'returns the balance' do + expect(order.outstanding_balance).to eq(-10) end end - context 'when there are no orders' do - it 'returns the order balance' do - orders = outstanding_balance.query - expect(orders).to be_empty + context 'when payment_total is less than total' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') } + + it "returns positive" do + expect(order.outstanding_balance).to eq(100 - 10) end end - it "should return positive amount when payment_total is less than total" do - order.payment_total = 20.20 - order.total = 30.30 - expect(order.outstanding_balance).to eq 10.10 - end + context 'when payment_total is greater than total' do + let(:order) { create(:order, total: 8.20, payment_total: 10.20, state: 'complete') } - it "should return negative amount when payment_total is greater than total" do - order.total = 8.20 - order.payment_total = 10.20 - expect(order.outstanding_balance).to be_within(0.001).of(-2.00) + it "returns negative amount" do + expect(order.outstanding_balance).to eq(-2.00) + end end end - context "#outstanding_balance?" do + context '#outstanding_balance?' do context 'when total is greater than payment_total' do - before do - order.total = 10.10 - order.payment_total = 9.50 - end + let(:order) { build(:order, total: 10.10, payment_total: 9.50) } - it "returns true" do + it 'returns true' do expect(order.outstanding_balance?).to eq(true) end end - context "when total is less than payment_total" do - before do - order.total = 8.25 - order.payment_total = 10.44 - end + context 'when total is less than payment_total' do + let(:order) { build(:order, total: 8.25, payment_total: 10.44) } - it "returns true" do + it 'returns true' do expect(order.outstanding_balance?).to eq(true) end end context "when total equals payment_total" do - before do - order.total = 10.10 - order.payment_total = 10.10 - end + let(:order) { build(:order, total: 10.10, payment_total: 10.10) } it 'returns false' do expect(order.outstanding_balance?).to eq(false) From cd60cea5de4484d697dbe8e20c50dd14e03e54d7 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 16 Feb 2021 16:44:37 +0100 Subject: [PATCH 11/61] Extract balance-related methods into module This model concerns helps us put together this related methods. Although it doesn't provide any encapsulation yet, it makes a bit easier to consider them all next time we need to change this implementation somehow. It's a bit of an illusion but it feels like we are making this God object model a bit smaller. It also gives more room for documentation that will aid future devs. --- app/models/concerns/balance.rb | 32 +++++++++++++++++++ app/models/spree/order.rb | 19 ++--------- app/queries/outstanding_balance.rb | 3 ++ .../balance_spec.rb} | 3 +- 4 files changed, 38 insertions(+), 19 deletions(-) create mode 100644 app/models/concerns/balance.rb rename spec/models/{spree/order/outstanding_balance_spec.rb => concerns/balance_spec.rb} (99%) diff --git a/app/models/concerns/balance.rb b/app/models/concerns/balance.rb new file mode 100644 index 0000000000..5de06109d5 --- /dev/null +++ b/app/models/concerns/balance.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +require 'active_support/concern' + +# Contains the methods to compute an order balance form the point of view of the enterprise and not +# the individual shopper. +module Balance + FINALIZED_NON_SUCCESSFUL_STATES = %w(canceled returned).freeze + + # Returns the order balance by considering the total as money owed to the order distributor aka. + # the shop, and as a positive balance of said enterprise. If the customer pays it all, they + # distributor and customer are even. + # + # Note however, this is meant to be used only in the context of a single order object. When + # working with a collection of orders, such an index controller action, please consider using + # `app/queries/oustanding_balance.rb` instead so we avoid potential N+1s. + def outstanding_balance + if state.in?(FINALIZED_NON_SUCCESSFUL_STATES) + -payment_total + else + total - payment_total + end + end + + def outstanding_balance? + !outstanding_balance.zero? + end + + def display_outstanding_balance + Spree::Money.new(outstanding_balance, currency: currency) + end +end diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 3d8559af35..19e617e26b 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -9,7 +9,9 @@ require 'concerns/order_shipment' module Spree class Order < ActiveRecord::Base prepend OrderShipment + include Checkout + include Balance checkout_flow do go_to_state :address @@ -136,7 +138,6 @@ module Spree # All the states an order can be in after completing the checkout FINALIZED_STATES = %w(complete canceled resumed awaiting_return returned).freeze - FINALIZED_NON_SUCCESSFUL_STATES = %w(canceled returned).freeze scope :finalized, -> { where(state: FINALIZED_STATES) } @@ -173,10 +174,6 @@ module Spree self[:currency] || Spree::Config[:currency] end - def display_outstanding_balance - Spree::Money.new(outstanding_balance, currency: currency) - end - def display_item_total Spree::Money.new(item_total, currency: currency) end @@ -394,18 +391,6 @@ module Spree Spree::TaxRate.adjust(self) end - def outstanding_balance - if state.in?(FINALIZED_NON_SUCCESSFUL_STATES) - -payment_total - else - total - payment_total - end - end - - def outstanding_balance? - outstanding_balance != 0 - end - def name address = bill_address || ship_address return unless address diff --git a/app/queries/outstanding_balance.rb b/app/queries/outstanding_balance.rb index ce4d4d78f4..48ee451009 100644 --- a/app/queries/outstanding_balance.rb +++ b/app/queries/outstanding_balance.rb @@ -8,6 +8,9 @@ # cases. # # See CompleteOrdersWithBalance or CustomersWithBalance as examples. +# +# Note this query object and `app/models/concerns/balance.rb` should implement the same behavior +# until we find a better way. If you change one, please, change the other too. class OutstandingBalance # All the states of a finished order but that shouldn't count towards the balance (the customer # didn't get the order for whatever reason). Note it does not include complete diff --git a/spec/models/spree/order/outstanding_balance_spec.rb b/spec/models/concerns/balance_spec.rb similarity index 99% rename from spec/models/spree/order/outstanding_balance_spec.rb rename to spec/models/concerns/balance_spec.rb index 4963fe273d..8e60eeae89 100644 --- a/spec/models/spree/order/outstanding_balance_spec.rb +++ b/spec/models/concerns/balance_spec.rb @@ -1,7 +1,6 @@ require 'spec_helper' -describe Spree::Order do - +describe Balance do context "#outstanding_balance" do context 'when orders are in cart state' do let(:order) { build(:order, total: 100, payment_total: 10, state: 'cart') } From 3b7f45516c6923f260d62a77edd212280c095f75 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 17 Feb 2021 17:27:57 +0100 Subject: [PATCH 12/61] Toggle bulk coop report balance calculation This will make users hit the new method that implements the new calculation we are aiming for, only if they have the feature enabled. --- app/models/concerns/balance.rb | 13 +- .../reports/bulk_coop/bulk_coop_report.rb | 6 +- .../bulk_coop/bulk_coop_report_spec.rb | 34 ++++ spec/models/concerns/balance_spec.rb | 162 +++++++++++++++--- 4 files changed, 185 insertions(+), 30 deletions(-) diff --git a/app/models/concerns/balance.rb b/app/models/concerns/balance.rb index 5de06109d5..91019d401c 100644 --- a/app/models/concerns/balance.rb +++ b/app/models/concerns/balance.rb @@ -14,7 +14,7 @@ module Balance # Note however, this is meant to be used only in the context of a single order object. When # working with a collection of orders, such an index controller action, please consider using # `app/queries/oustanding_balance.rb` instead so we avoid potential N+1s. - def outstanding_balance + def new_outstanding_balance if state.in?(FINALIZED_NON_SUCCESSFUL_STATES) -payment_total else @@ -22,6 +22,17 @@ module Balance end end + # This method is the one we're gradually replacing with `#new_outstanding_balance`. Having them + # separate enables us to choose which implementation we want depending on the context using + # a feature toggle. This avoids incosistent behavior across the app during that incremental + # refactoring. + # + # It is meant to be removed as soon as we get product approval that the new implementation has + # been working correctly in production. + def outstanding_balance + total - payment_total + end + def outstanding_balance? !outstanding_balance.zero? end diff --git a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb index ac318c00dc..eb1f3ca60a 100644 --- a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb +++ b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb @@ -163,7 +163,11 @@ module OrderManagement end def customer_payments_amount_owed(line_items) - unique_orders(line_items).sum(&:outstanding_balance) + if OpenFoodNetwork::FeatureToggle.enabled?(:customer_balance, @user) + unique_orders(line_items).sum(&:new_outstanding_balance) + else + unique_orders(line_items).sum(&:outstanding_balance) + end end def customer_payments_amount_paid(line_items) diff --git a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb index c7141b6796..a6cb1666b3 100644 --- a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb +++ b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb @@ -144,4 +144,38 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do end end end + + # Yes, I know testing a private method is bad practice but report's design, tighly coupling + # OpenFoodNetwork::OrderGrouper and OrderManagement::Reports::BulkCoop::BulkCoopReport, makes it + # very hard to make things testeable without ending up in a wormwhole. This is a trade-off. + describe '#customer_payments_amount_owed' do + let(:params) { {} } + let(:user) { build(:user) } + let!(:line_item) { create(:line_item) } + let(:order) { line_item.order } + + context 'when the customer_balance feature is enabled' do + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, user) { true } + end + + it 'calls #new_outstanding_balance' do + expect_any_instance_of(Spree::Order).to receive(:new_outstanding_balance) + subject.send(:customer_payments_amount_owed, [line_item]) + end + end + + context 'when the customer_balance feature is disabled' do + before do + allow(OpenFoodNetwork::FeatureToggle) + .to receive(:enabled?).with(:customer_balance, user) { false } + end + + it 'calls #outstanding_balance' do + expect_any_instance_of(Spree::Order).to receive(:outstanding_balance) + subject.send(:customer_payments_amount_owed, [line_item]) + end + end + end end diff --git a/spec/models/concerns/balance_spec.rb b/spec/models/concerns/balance_spec.rb index 8e60eeae89..331e521442 100644 --- a/spec/models/concerns/balance_spec.rb +++ b/spec/models/concerns/balance_spec.rb @@ -1,6 +1,138 @@ require 'spec_helper' describe Balance do + context "#new_outstanding_balance" do + context 'when orders are in cart state' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'cart') } + + it 'returns the order balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when orders are in address state' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'address') } + + it 'returns the order balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when orders are in delivery state' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'delivery') } + + it 'returns the order balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when orders are in payment state' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'payment') } + + it 'returns the order balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when no orders where paid' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') } + + it 'returns the customer balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when an order was paid' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') } + + it 'returns the customer balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when an order is canceled' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'canceled') } + + it 'returns the customer balance' do + expect(order.new_outstanding_balance).to eq(-10) + end + end + + context 'when an order is resumed' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'resumed') } + + it 'returns the customer balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when an order is in payment' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'payment') } + + it 'returns the customer balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when an order is awaiting_return' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'awaiting_return') } + + it 'returns the customer balance' do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when an order is returned' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'returned') } + + it 'returns the balance' do + expect(order.new_outstanding_balance).to eq(-10) + end + end + + context 'when payment_total is less than total' do + let(:order) { build(:order, total: 100, payment_total: 10, state: 'complete') } + + it "returns positive" do + expect(order.new_outstanding_balance).to eq(100 - 10) + end + end + + context 'when payment_total is greater than total' do + let(:order) { create(:order, total: 8.20, payment_total: 10.20, state: 'complete') } + + it "returns negative amount" do + expect(order.new_outstanding_balance).to eq(-2.00) + end + end + end + + context '#outstanding_balance?' do + context 'when total is greater than payment_total' do + let(:order) { build(:order, total: 10.10, payment_total: 9.50) } + + it 'returns true' do + expect(order.outstanding_balance?).to eq(true) + end + end + + context 'when total is less than payment_total' do + let(:order) { build(:order, total: 8.25, payment_total: 10.44) } + + it 'returns true' do + expect(order.outstanding_balance?).to eq(true) + end + end + + context "when total equals payment_total" do + let(:order) { build(:order, total: 10.10, payment_total: 10.10) } + + it 'returns false' do + expect(order.outstanding_balance?).to eq(false) + end + end + end + context "#outstanding_balance" do context 'when orders are in cart state' do let(:order) { build(:order, total: 100, payment_total: 10, state: 'cart') } @@ -54,7 +186,7 @@ describe Balance do let(:order) { build(:order, total: 100, payment_total: 10, state: 'canceled') } it 'returns the customer balance' do - expect(order.outstanding_balance).to eq(-10) + expect(order.outstanding_balance).to eq(100 - 10) end end @@ -86,7 +218,7 @@ describe Balance do let(:order) { build(:order, total: 100, payment_total: 10, state: 'returned') } it 'returns the balance' do - expect(order.outstanding_balance).to eq(-10) + expect(order.outstanding_balance).to eq(100 - 10) end end @@ -106,30 +238,4 @@ describe Balance do end end end - - context '#outstanding_balance?' do - context 'when total is greater than payment_total' do - let(:order) { build(:order, total: 10.10, payment_total: 9.50) } - - it 'returns true' do - expect(order.outstanding_balance?).to eq(true) - end - end - - context 'when total is less than payment_total' do - let(:order) { build(:order, total: 8.25, payment_total: 10.44) } - - it 'returns true' do - expect(order.outstanding_balance?).to eq(true) - end - end - - context "when total equals payment_total" do - let(:order) { build(:order, total: 10.10, payment_total: 10.10) } - - it 'returns false' do - expect(order.outstanding_balance?).to eq(false) - end - end - end end From fce98da88d680ccadaec8887ce5f3c441fff60a2 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 24 Feb 2021 15:41:47 +0100 Subject: [PATCH 13/61] Reject line items in a more succinct way Thanks for the suggestion @mkllnk! --- lib/open_food_network/reports/line_items.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/lib/open_food_network/reports/line_items.rb b/lib/open_food_network/reports/line_items.rb index e1c1dfeaae..eaca042e16 100644 --- a/lib/open_food_network/reports/line_items.rb +++ b/lib/open_food_network/reports/line_items.rb @@ -22,8 +22,7 @@ module OpenFoodNetwork line_items = line_items.includes(*line_item_includes).references(:line_items) end - editable_line_items = editable_line_items(line_items) - without_editable_line_items = line_items.reject { |li| editable_line_items.include? li } + without_editable_line_items = line_items - editable_line_items(line_items) without_editable_line_items.each do |line_item| OrderDataMasker.new(line_item.order).call From b6ce9ca3cab21f0fc73f9a9e58ac4c168911e52d Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 2 Mar 2021 17:25:12 +0100 Subject: [PATCH 14/61] Test bulk coop report includes canceled orders Without them numbers in the report don't match with /admin/customers and /account where their order total is considered towards the customer balance. --- .../reports/bulk_coop/bulk_coop_report_spec.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb index a6cb1666b3..9adc4b047a 100644 --- a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb +++ b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb @@ -18,15 +18,16 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do context "as a site admin" do it "fetches completed orders" do - o2 = create(:order) + o2 = create(:order, state: 'cart') o2.line_items << build(:line_item) expect(subject.table_items).to eq([li1]) end - it "does not show cancelled orders" do - o2 = create(:order, state: "canceled", completed_at: 1.day.ago) - o2.line_items << build(:line_item_with_shipment) - expect(subject.table_items).to eq([li1]) + it "shows cancelled orders" do + o2 = create(:order, state: 'canceled', completed_at: 1.day.ago, order_cycle: oc1, distributor: d1) + line_item = build(:line_item_with_shipment) + o2.line_items << line_item + expect(subject.table_items).to include(line_item) end end From 2ead2ad417405e18517df50002f41be40fb2238e Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 3 Mar 2021 09:29:30 +0100 Subject: [PATCH 15/61] Replace private stub with fake collaborator object --- .../reports/line_items_spec.rb | 34 +++++++++---------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/spec/lib/open_food_network/reports/line_items_spec.rb b/spec/lib/open_food_network/reports/line_items_spec.rb index 0cd21534a1..63d9696f4b 100644 --- a/spec/lib/open_food_network/reports/line_items_spec.rb +++ b/spec/lib/open_food_network/reports/line_items_spec.rb @@ -8,8 +8,9 @@ describe OpenFoodNetwork::Reports::LineItems do # under test and the various objects it depends on. Other more common moking strategies where very # hard. class FakeOrderPermissions - def initialize(line_item) + def initialize(line_item, orders_relation) @relation = Spree::LineItem.where(id: line_item.id) + @orders_relation = orders_relation end def visible_line_items @@ -21,31 +22,30 @@ describe OpenFoodNetwork::Reports::LineItems do Spree::LineItem.where(id: line_item.id) end + def visible_orders + orders_relation + end + private - attr_reader :relation - end - - class FakeRansackResult - attr_reader :result - - def initialize(result) - @result = result - end + attr_reader :relation, :orders_relation end describe '#list' do - let!(:order) { create(:order, distributor: create(:enterprise)) } + let!(:order) do + create( + :order, + distributor: create(:enterprise), + completed_at: 1.day.ago, + shipments: [build(:shipment)] + ) + end let!(:line_item) { create(:line_item, order: order) } - let(:order_permissions) { FakeOrderPermissions.new(line_item) } + let(:orders_relation) { Spree::Order.where(id: order.id) } + let(:order_permissions) { FakeOrderPermissions.new(line_item, orders_relation) } let(:params) { {} } - before do - orders_relation = Spree::Order.where(id: order.id) - allow(reports_line_items).to receive(:search_orders) { FakeRansackResult.new(orders_relation) } - end - it 'returns masked data' do line_items = reports_line_items.list expect(line_items.first.order.email).to eq(I18n.t('admin.reports.hidden')) From 72597ea3f9d639bbdd5bf0fcdf7c5e572c576684 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 3 Mar 2021 09:32:42 +0100 Subject: [PATCH 16/61] Optionally inject orders relation So we can fetch them differently if we need to. Spoiler: we do in the bulk coop report. --- lib/open_food_network/reports/line_items.rb | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/lib/open_food_network/reports/line_items.rb b/lib/open_food_network/reports/line_items.rb index eaca042e16..ed7d93d1bc 100644 --- a/lib/open_food_network/reports/line_items.rb +++ b/lib/open_food_network/reports/line_items.rb @@ -2,9 +2,10 @@ module OpenFoodNetwork module Reports # shared code to search and list line items class LineItems - def initialize(order_permissions, params) + def initialize(order_permissions, params, orders_relation = nil) @order_permissions = order_permissions @params = params + @orders_relation = orders_relation || complete_not_canceled_visible_orders end def orders @@ -33,8 +34,14 @@ module OpenFoodNetwork private + attr_reader :orders_relation + + def complete_not_canceled_visible_orders + @order_permissions.visible_orders.complete.not_state(:canceled) + end + def search_orders - @order_permissions.visible_orders.complete.not_state(:canceled).search(@params[:q]) + orders_relation.search(@params[:q]) end # From the line_items given, returns the ones that are editable by the user From 21fb3f3da6b1a137d73dc820e9af4e62b8bfd2f5 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 3 Mar 2021 08:51:22 +0100 Subject: [PATCH 17/61] Extract orders relation as a query object --- app/queries/complete_visible_orders.rb | 15 +++++++ .../reports/bulk_coop/bulk_coop_report.rb | 10 +++-- spec/queries/complete_visible_orders_spec.rb | 40 +++++++++++++++++++ 3 files changed, 61 insertions(+), 4 deletions(-) create mode 100644 app/queries/complete_visible_orders.rb create mode 100644 spec/queries/complete_visible_orders_spec.rb diff --git a/app/queries/complete_visible_orders.rb b/app/queries/complete_visible_orders.rb new file mode 100644 index 0000000000..10eabe4f6d --- /dev/null +++ b/app/queries/complete_visible_orders.rb @@ -0,0 +1,15 @@ +# frozen_string_literal: true + +class CompleteVisibleOrders + def initialize(order_permissions) + @order_permissions = order_permissions + end + + def query + order_permissions.visible_orders.complete + end + + private + + attr_reader :order_permissions +end diff --git a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb index eb1f3ca60a..ef9cfdccc8 100644 --- a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb +++ b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb @@ -149,13 +149,15 @@ module OrderManagement end def order_permissions - return @order_permissions unless @order_permissions.nil? - - @order_permissions = ::Permissions::Order.new(@user, @params[:q]) + @order_permissions ||= ::Permissions::Order.new(@user, @params[:q]) end def report_line_items - @report_line_items ||= OpenFoodNetwork::Reports::LineItems.new(order_permissions, @params) + @report_line_items ||= OpenFoodNetwork::Reports::LineItems.new( + order_permissions, + @params, + CompleteVisibleOrders.new(order_permissions).query + ) end def customer_payments_total_cost(line_items) diff --git a/spec/queries/complete_visible_orders_spec.rb b/spec/queries/complete_visible_orders_spec.rb new file mode 100644 index 0000000000..82f66093ff --- /dev/null +++ b/spec/queries/complete_visible_orders_spec.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe CompleteVisibleOrders do + subject(:complete_visible_orders) { described_class.new(order_permissions) } + let(:filter_canceled) { false } + + describe '#query' do + let(:user) { create(:user) } + let(:enterprise) { create(:enterprise) } + let(:order_permissions) { ::Permissions::Order.new(user, filter_canceled) } + + before do + user.enterprises << enterprise + user.save! + end + + context 'when an order has no completed_at' do + let(:cart_order) { create(:order, distributor: enterprise) } + + it 'does not return it' do + expect(complete_visible_orders.query).not_to include(cart_order) + end + end + + context 'when an order has complete_at' do + let(:complete_order) { create(:order, completed_at: 1.day.ago, distributor: enterprise) } + + it 'does not return it' do + expect(complete_visible_orders.query).to include(complete_order) + end + end + + it 'calls #visible_orders' do + expect(order_permissions).to receive(:visible_orders).and_call_original + complete_visible_orders.query + end + end +end From f67b45a58059f02f25afb444ac61b6553a1b720a Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 3 Mar 2021 10:05:59 +0100 Subject: [PATCH 18/61] Reuse query object in Reports::LineItems This almost removes the responsibility of fetching orders from this class, that has too many. Ideally, I'd go on and leave this up to the caller of this class making `Reports::LineItems` rely completely on the passed in `orders_relation`. Not today. --- lib/open_food_network/reports/line_items.rb | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/lib/open_food_network/reports/line_items.rb b/lib/open_food_network/reports/line_items.rb index ed7d93d1bc..88841001dd 100644 --- a/lib/open_food_network/reports/line_items.rb +++ b/lib/open_food_network/reports/line_items.rb @@ -5,6 +5,7 @@ module OpenFoodNetwork def initialize(order_permissions, params, orders_relation = nil) @order_permissions = order_permissions @params = params + complete_not_canceled_visible_orders = CompleteVisibleOrders.new(order_permissions).query.not_state(:canceled) @orders_relation = orders_relation || complete_not_canceled_visible_orders end @@ -13,7 +14,7 @@ module OpenFoodNetwork end def list(line_item_includes = nil) - line_items = @order_permissions.visible_line_items.in_orders(orders.result) + line_items = order_permissions.visible_line_items.in_orders(orders.result) if @params[:supplier_id_in].present? line_items = line_items.supplied_by_any(@params[:supplier_id_in]) @@ -34,11 +35,7 @@ module OpenFoodNetwork private - attr_reader :orders_relation - - def complete_not_canceled_visible_orders - @order_permissions.visible_orders.complete.not_state(:canceled) - end + attr_reader :orders_relation, :order_permissions def search_orders orders_relation.search(@params[:q]) @@ -46,7 +43,7 @@ module OpenFoodNetwork # From the line_items given, returns the ones that are editable by the user def editable_line_items(line_items) - editable_line_items_ids = @order_permissions.editable_line_items.select(:id) + editable_line_items_ids = order_permissions.editable_line_items.select(:id) # Although merge could take a relation, here we convert line_items to array # because, if we pass a relation, merge will overwrite the conditions on the same field From b49777062b297a9fa2ab14d2dcce653265800c3e Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 3 Mar 2021 10:57:17 +0100 Subject: [PATCH 19/61] Include canceled orders in report when searching It's a code smell to have a boolean control argument. `Permissions::Order` goes too far and assumes we want to filter out canceled orders when using report's search params and this is not the case. --- .../reports/bulk_coop/bulk_coop_report.rb | 5 ++- .../bulk_coop/bulk_coop_report_spec.rb | 41 ++++++++++++++----- 2 files changed, 35 insertions(+), 11 deletions(-) diff --git a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb index ef9cfdccc8..1f658b189a 100644 --- a/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb +++ b/engines/order_management/app/services/order_management/reports/bulk_coop/bulk_coop_report.rb @@ -22,6 +22,7 @@ module OrderManagement @supplier_report = BulkCoopSupplierReport.new @allocation_report = BulkCoopAllocationReport.new + @filter_canceled = false end def header @@ -138,6 +139,8 @@ module OrderManagement private + attr_reader :filter_canceled + def line_item_includes [ { @@ -149,7 +152,7 @@ module OrderManagement end def order_permissions - @order_permissions ||= ::Permissions::Order.new(@user, @params[:q]) + @order_permissions ||= ::Permissions::Order.new(@user, filter_canceled) end def report_line_items diff --git a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb index 9adc4b047a..f667c8fdc6 100644 --- a/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb +++ b/engines/order_management/spec/services/order_management/reports/bulk_coop/bulk_coop_report_spec.rb @@ -6,7 +6,7 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do subject { OrderManagement::Reports::BulkCoop::BulkCoopReport.new user, params, true } let(:user) { create(:admin_user) } - describe "fetching orders" do + describe '#table_items' do let(:params) { {} } let(:d1) { create(:distributor_enterprise) } @@ -17,17 +17,38 @@ describe OrderManagement::Reports::BulkCoop::BulkCoopReport do before { o1.line_items << li1 } context "as a site admin" do - it "fetches completed orders" do - o2 = create(:order, state: 'cart') - o2.line_items << build(:line_item) - expect(subject.table_items).to eq([li1]) + context 'when searching' do + let(:params) { { q: { completed_at_gt: '', completed_at_lt: '', distributor_id_in: [] } } } + + it "fetches completed orders" do + o2 = create(:order, state: 'cart') + o2.line_items << build(:line_item) + expect(subject.table_items).to eq([li1]) + end + + it 'shows canceled orders' do + o2 = create(:order, state: 'canceled', completed_at: 1.day.ago, order_cycle: oc1, distributor: d1) + line_item = build(:line_item_with_shipment) + o2.line_items << line_item + expect(subject.table_items).to include(line_item) + end end - it "shows cancelled orders" do - o2 = create(:order, state: 'canceled', completed_at: 1.day.ago, order_cycle: oc1, distributor: d1) - line_item = build(:line_item_with_shipment) - o2.line_items << line_item - expect(subject.table_items).to include(line_item) + context 'when not searching' do + let(:params) { {} } + + it "fetches completed orders" do + o2 = create(:order, state: 'cart') + o2.line_items << build(:line_item) + expect(subject.table_items).to eq([li1]) + end + + it 'shows canceled orders' do + o2 = create(:order, state: 'canceled', completed_at: 1.day.ago, order_cycle: oc1, distributor: d1) + line_item = build(:line_item_with_shipment) + o2.line_items << line_item + expect(subject.table_items).to include(line_item) + end end end From 553954f8bf4aa73ac3f41265c849457b948c89a7 Mon Sep 17 00:00:00 2001 From: Konrad Date: Wed, 3 Mar 2021 20:35:16 +0100 Subject: [PATCH 20/61] Removed "footer .row a" and added "#secure" --- app/assets/stylesheets/darkswarm/footer.scss | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/darkswarm/footer.scss b/app/assets/stylesheets/darkswarm/footer.scss index 4893489635..e63f7f8c5b 100644 --- a/app/assets/stylesheets/darkswarm/footer.scss +++ b/app/assets/stylesheets/darkswarm/footer.scss @@ -4,7 +4,7 @@ footer { .row { - p a { + p { font-size: 1rem; } @@ -107,6 +107,11 @@ footer { color: rgba($disabled-med, 0.35); } + #secure { + font-size: 1.5rem; + font-weight: 300; + } + .social-icons { margin-bottom: 0.9rem; padding-top: 0.1rem; @@ -129,7 +134,7 @@ footer { } } - .legal a { + .legal p { font-size: 0.875rem; } } From e0108431d9725e80a3976c2942f5531923533203 Mon Sep 17 00:00:00 2001 From: Konrad Date: Wed, 3 Mar 2021 20:36:22 +0100 Subject: [PATCH 21/61] Added method to check for social icons --- app/helpers/footer_links_helper.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/helpers/footer_links_helper.rb b/app/helpers/footer_links_helper.rb index a4caad2abd..9bb79cffd7 100644 --- a/app/helpers/footer_links_helper.rb +++ b/app/helpers/footer_links_helper.rb @@ -15,4 +15,8 @@ module FooterLinksHelper target: '_blank', rel: 'noopener' ) end + + def show_social_icons? + ContentConfig.footer_facebook_url.present? || ContentConfig.footer_twitter_url.present? || ContentConfig.footer_instagram_url.present? || ContentConfig.footer_linkedin_url.present? || ContentConfig.footer_googleplus_url.present? || ContentConfig.footer_pinterest_url.present? + end end From 07b23602cd4acd9b88399467546a64fe9f58f0b6 Mon Sep 17 00:00:00 2001 From: Konrad Date: Wed, 3 Mar 2021 20:38:12 +0100 Subject: [PATCH 22/61] Included #secure and method show_social_icons? --- app/views/shared/_footer.html.haml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/shared/_footer.html.haml b/app/views/shared/_footer.html.haml index dd7cd4a21c..7ffa59b113 100644 --- a/app/views/shared/_footer.html.haml +++ b/app/views/shared/_footer.html.haml @@ -15,7 +15,7 @@ %p.secure-icon %i.ofn-i_017-locked .small-12.medium-6.columns.text-center - %p.text-big.secure-text + %p.secure-text#secure = t '.footer_secure' %p.secure-text = t '.footer_secure_text' @@ -31,7 +31,7 @@ // This is the instance-managed set of links: %h4 = t '.footer_contact_headline' - - if ContentConfig.footer_facebook_url.present? || ContentConfig.footer_twitter_url.present? || ContentConfig.footer_instagram_url.present? || ContentConfig.footer_linkedin_url.present? || ContentConfig.footer_googleplus_url.present? || ContentConfig.footer_pinterest_url.present? + - if show_social_icons? %p.social-icons - if ContentConfig.footer_facebook_url.present? %a{href: ContentConfig.footer_facebook_url} From aa761d6d6759f2ea1fb2aa89761b50e2a69d03d7 Mon Sep 17 00:00:00 2001 From: Konrad Date: Wed, 3 Mar 2021 20:57:55 +0100 Subject: [PATCH 23/61] Removed #secure and replaced with class --- app/assets/stylesheets/darkswarm/footer.scss | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/stylesheets/darkswarm/footer.scss b/app/assets/stylesheets/darkswarm/footer.scss index e63f7f8c5b..43b5898c7c 100644 --- a/app/assets/stylesheets/darkswarm/footer.scss +++ b/app/assets/stylesheets/darkswarm/footer.scss @@ -107,10 +107,10 @@ footer { color: rgba($disabled-med, 0.35); } - #secure { + p.text-big { font-size: 1.5rem; font-weight: 300; - } + } .social-icons { margin-bottom: 0.9rem; From db6eaede14f8d1d5dc634575b283a8c49dc86e78 Mon Sep 17 00:00:00 2001 From: Konrad Date: Wed, 3 Mar 2021 20:58:45 +0100 Subject: [PATCH 24/61] Shortened long line --- app/helpers/footer_links_helper.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/helpers/footer_links_helper.rb b/app/helpers/footer_links_helper.rb index 9bb79cffd7..e084cca277 100644 --- a/app/helpers/footer_links_helper.rb +++ b/app/helpers/footer_links_helper.rb @@ -17,6 +17,8 @@ module FooterLinksHelper end def show_social_icons? - ContentConfig.footer_facebook_url.present? || ContentConfig.footer_twitter_url.present? || ContentConfig.footer_instagram_url.present? || ContentConfig.footer_linkedin_url.present? || ContentConfig.footer_googleplus_url.present? || ContentConfig.footer_pinterest_url.present? + ContentConfig.footer_facebook_url.present? || ContentConfig.footer_twitter_url.present? || + ContentConfig.footer_instagram_url.present? || ContentConfig.footer_linkedin_url.present? || + ContentConfig.footer_googleplus_url.present? || ContentConfig.footer_pinterest_url.present? end end From 748b2b8223bbf5622537feb79d3d002227c3f62d Mon Sep 17 00:00:00 2001 From: Konrad Date: Wed, 3 Mar 2021 20:59:30 +0100 Subject: [PATCH 25/61] Removed #secure and replaced with class --- app/views/shared/_footer.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/_footer.html.haml b/app/views/shared/_footer.html.haml index 7ffa59b113..e7fb4e7dcb 100644 --- a/app/views/shared/_footer.html.haml +++ b/app/views/shared/_footer.html.haml @@ -15,7 +15,7 @@ %p.secure-icon %i.ofn-i_017-locked .small-12.medium-6.columns.text-center - %p.secure-text#secure + %p.text-big.secure-text = t '.footer_secure' %p.secure-text = t '.footer_secure_text' From 4e5945f9effb763a254cfe024f2f4de19d0c3695 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 4 Mar 2021 11:24:39 -0800 Subject: [PATCH 26/61] add resumed to list of allowable order states --- app/controllers/spree/admin/payments_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/payments_controller.rb b/app/controllers/spree/admin/payments_controller.rb index f818022dfd..4dfb826972 100644 --- a/app/controllers/spree/admin/payments_controller.rb +++ b/app/controllers/spree/admin/payments_controller.rb @@ -134,7 +134,7 @@ module Spree # # Otherwise redirect user to that step def can_transition_to_payment - return if @order.payment? || @order.complete? || @order.canceled? + return if @order.payment? || @order.complete? || @order.canceled? || @order.resumed? flash[:notice] = Spree.t(:fill_in_customer_info) redirect_to spree.edit_admin_order_customer_url(@order) From a431c03eb160cb90f7f89d215a172bceb43fde64 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Thu, 4 Mar 2021 14:32:55 -0800 Subject: [PATCH 27/61] only show primary producers on shopfront list of producers --- app/models/enterprise.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/enterprise.rb b/app/models/enterprise.rb index b3a5669f03..135358fdc7 100644 --- a/app/models/enterprise.rb +++ b/app/models/enterprise.rb @@ -250,7 +250,7 @@ class Enterprise < ActiveRecord::Base def plus_relatives_and_oc_producers(order_cycles) oc_producer_ids = Exchange.in_order_cycle(order_cycles).incoming.pluck :sender_id - Enterprise.relatives_of_one_union_others(id, oc_producer_ids | [id]) + Enterprise.is_primary_producer.relatives_of_one_union_others(id, oc_producer_ids | [id]) end def relatives_including_self From a6707710375442d3f8c303a931e9e572407dc53b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 5 Mar 2021 15:12:41 +0000 Subject: [PATCH 28/61] Reduce duplicate updates in Payment#revoke_adjustment_eligibility This was triggering two separate updates, and each of those updates could trigger callbacks, and those could trigger other callbacks. Here we're doing the same thing, but with one update. --- app/models/spree/payment.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/spree/payment.rb b/app/models/spree/payment.rb index 0896defdc3..6e3333bac2 100644 --- a/app/models/spree/payment.rb +++ b/app/models/spree/payment.rb @@ -154,8 +154,10 @@ module Spree return unless adjustment.try(:reload) return if adjustment.finalized? - adjustment.update_attribute(:eligible, false) - adjustment.finalize! + adjustment.update( + eligible: false, + state: "finalized" + ) end def validate_source From 365700615ad3bf5f1d993523824259d3876b8c19 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 5 Mar 2021 16:03:07 +0000 Subject: [PATCH 29/61] Remove dead code: Spree::Config.auto_capture We set this value to `true` unconditionally in an initializer, and then check the value in various places via Spree::Config. It's never false, and it's not configurable, so we can just drop it and remove the related conditionals. :fire: --- app/models/spree/app_configuration.rb | 2 -- app/models/spree/gateway/pay_pal_express.rb | 4 ---- app/models/spree/payment/processing.rb | 12 ++---------- app/models/spree/payment_method.rb | 4 ---- config/initializers/spree.rb | 4 ---- spec/models/spree/order/payment_spec.rb | 3 --- spec/models/spree/payment_spec.rb | 10 +--------- spec/spec_helper.rb | 1 - 8 files changed, 3 insertions(+), 37 deletions(-) diff --git a/app/models/spree/app_configuration.rb b/app/models/spree/app_configuration.rb index 67b843a4ce..6b4a57aa76 100644 --- a/app/models/spree/app_configuration.rb +++ b/app/models/spree/app_configuration.rb @@ -36,8 +36,6 @@ module Spree preference :allow_ssl_in_development_and_test, :boolean, default: false preference :allow_ssl_in_production, :boolean, default: true preference :allow_ssl_in_staging, :boolean, default: true - # Automatically capture the credit card (as opposed to just authorize and capture later) - preference :auto_capture, :boolean, default: false # Replace with the name of a zone if you would like to limit the countries preference :checkout_zone, :string, default: nil preference :currency, :string, default: "USD" diff --git a/app/models/spree/gateway/pay_pal_express.rb b/app/models/spree/gateway/pay_pal_express.rb index 357d563264..bc7d8cef64 100644 --- a/app/models/spree/gateway/pay_pal_express.rb +++ b/app/models/spree/gateway/pay_pal_express.rb @@ -31,10 +31,6 @@ module Spree provider_class.new end - def auto_capture? - true - end - def method_type 'paypal' end diff --git a/app/models/spree/payment/processing.rb b/app/models/spree/payment/processing.rb index 80d9d2dc0f..7be884411d 100644 --- a/app/models/spree/payment/processing.rb +++ b/app/models/spree/payment/processing.rb @@ -6,22 +6,14 @@ module Spree def process! return unless validate! - if payment_method.auto_capture? - purchase! - else - authorize! - end + purchase! end def process_offline! return unless validate! return if authorization_action_required? - if payment_method.auto_capture? - charge_offline! - else - authorize! - end + charge_offline! end def authorize!(return_url = nil) diff --git a/app/models/spree/payment_method.rb b/app/models/spree/payment_method.rb index d519e3284a..4204632b91 100644 --- a/app/models/spree/payment_method.rb +++ b/app/models/spree/payment_method.rb @@ -89,10 +89,6 @@ module Spree true end - def auto_capture? - Spree::Config[:auto_capture] - end - def supports?(_source) true end diff --git a/config/initializers/spree.rb b/config/initializers/spree.rb index 09e404fc9f..2d8a5bbe6a 100644 --- a/config/initializers/spree.rb +++ b/config/initializers/spree.rb @@ -22,10 +22,6 @@ Spree.config do |config| config.address_requires_state = true config.admin_interface_logo = '/default_images/ofn-logo.png' - # -- spree_paypal_express - # Auto-capture payments. Without this option, payments must be manually captured in the paypal interface. - config.auto_capture = true - # S3 settings config.s3_bucket = ENV['S3_BUCKET'] if ENV['S3_BUCKET'] config.s3_access_key = ENV['S3_ACCESS_KEY'] if ENV['S3_ACCESS_KEY'] diff --git a/spec/models/spree/order/payment_spec.rb b/spec/models/spree/order/payment_spec.rb index 44e434bd5e..224f0be0dc 100644 --- a/spec/models/spree/order/payment_spec.rb +++ b/spec/models/spree/order/payment_spec.rb @@ -9,9 +9,6 @@ module Spree let(:bogus) { create(:bogus_payment_method, distributors: [create(:enterprise)]) } before do - # So that Payment#purchase! is called during processing - Spree::Config[:auto_capture] = true - allow(order).to receive_message_chain(:line_items, :empty?).and_return(false) allow(order).to receive_messages total: 100 end diff --git a/spec/models/spree/payment_spec.rb b/spec/models/spree/payment_spec.rb index 9a0ab803d0..64d9c04afe 100644 --- a/spec/models/spree/payment_spec.rb +++ b/spec/models/spree/payment_spec.rb @@ -81,20 +81,12 @@ describe Spree::Payment do end context "#process!" do - it "should purchase if with auto_capture" do + it "should call purchase!" do payment = build_stubbed(:payment, payment_method: gateway) - expect(payment.payment_method).to receive(:auto_capture?).and_return(true) expect(payment).to receive(:purchase!) payment.process! end - it "should authorize without auto_capture" do - payment = build_stubbed(:payment, payment_method: gateway) - expect(payment.payment_method).to receive(:auto_capture?).and_return(false) - expect(payment).to receive(:authorize!) - payment.process! - end - it "should make the state 'processing'" do expect(payment).to receive(:started_processing!) payment.process! diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 55b6586f47..777b7cfcbf 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -172,7 +172,6 @@ RSpec.configure do |config| spree_config.checkout_zone = checkout_zone spree_config.currency = currency spree_config.shipping_instructions = true - spree_config.auto_capture = true end end From 5c7466872621c6add738b2f3c2f3a8007a82dd16 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 8 Mar 2021 16:20:07 +0100 Subject: [PATCH 30/61] Make test rely on container rather than parent This decouples it a bit from the actual HTML markup and makes it a bit more resilient. --- engines/web/spec/features/consumer/cookies_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engines/web/spec/features/consumer/cookies_spec.rb b/engines/web/spec/features/consumer/cookies_spec.rb index 5b55ece1fb..e447364fe2 100644 --- a/engines/web/spec/features/consumer/cookies_spec.rb +++ b/engines/web/spec/features/consumer/cookies_spec.rb @@ -155,7 +155,7 @@ feature "Cookies", js: true do end def click_footer_cookies_policy_link_and_wait - find("div > a", text: "cookies policy").click + find(".legal a", text: "cookies policy").click sleep 2 end From 95c232dfe7477c92e9db1382f50455d42f7cd790 Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 8 Mar 2021 08:54:28 -0800 Subject: [PATCH 31/61] add spec for a resumed order --- .../admin/orders/payments/payments_controller_spec.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb index cc743c1f4b..79479bf284 100644 --- a/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb +++ b/spec/controllers/spree/admin/orders/payments/payments_controller_spec.rb @@ -297,6 +297,17 @@ describe Spree::Admin::PaymentsController, type: :controller do spree_get :index, order_id: order.number expect(response.status).to eq 200 end + + context "order is then resumed" do + before do + order.resume + end + + it "still renders the payments tab" do + spree_get :index, order_id: order.number + expect(response.status).to eq 200 + end + end end end end From 0125b5f10e2167a61a59d166b27beee6533a26ab Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Mon, 8 Mar 2021 09:10:40 -0800 Subject: [PATCH 32/61] add spec for plus_relatives_and_oc_producers --- spec/models/enterprise_spec.rb | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/spec/models/enterprise_spec.rb b/spec/models/enterprise_spec.rb index 930358a350..313148e599 100644 --- a/spec/models/enterprise_spec.rb +++ b/spec/models/enterprise_spec.rb @@ -599,4 +599,19 @@ describe Enterprise do end end end + + describe "#plus_relatives_and_oc_producers" do + it "does not find non-produders " do + supplier = create(:supplier_enterprise) + distributor = create(:distributor_enterprise, is_primary_producer: false) + product = create(:product) + order_cycle = create( + :simple_order_cycle, + suppliers: [supplier], + distributors: [distributor], + variants: [product.master] + ) + expect(distributor.plus_relatives_and_oc_producers(order_cycle)).to eq([supplier]) + end + end end From aeefd068bf55e770b72c3157f100f12bc1cd820d Mon Sep 17 00:00:00 2001 From: Transifex-Openfoodnetwork Date: Tue, 16 Mar 2021 03:58:49 +1100 Subject: [PATCH 33/61] Updating translations for config/locales/en_CA.yml --- config/locales/en_CA.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/config/locales/en_CA.yml b/config/locales/en_CA.yml index c07d8cb4a5..4775d5fd43 100644 --- a/config/locales/en_CA.yml +++ b/config/locales/en_CA.yml @@ -1186,6 +1186,7 @@ en_CA: mailers: powered_by: open_food_network: "Open Food Network" + powered_html: "%{open_food_network}" menu: cart: cart: "Cart" @@ -2882,6 +2883,7 @@ en_CA: name_or_sku: "Name or SKU (enter at least first 4 characters of product name)" resend: "Resend" back_to_orders_list: "Back to Orders List" + back_to_payments_list: "Back To Payments List" return_authorizations: "Return Authoriations" cannot_create_returns: "Cannot create returns as this order has no shipped units" select_stock: "Select stock" @@ -3451,6 +3453,7 @@ en_CA: pending: pending ready: ready shipped: shipped + canceled: canceled payment_states: balance_due: balance due completed: completed @@ -3572,6 +3575,8 @@ en_CA: past_orders: Past Orders transactions: transaction_history: Transaction History + authorisation_required: Authorisation Required + authorise: Authorize open_orders: order: Order shop: Shop From e55457d9ddc5c8dd0cee1da079e4faf65952059c Mon Sep 17 00:00:00 2001 From: Transifex-Openfoodnetwork Date: Tue, 16 Mar 2021 04:13:10 +1100 Subject: [PATCH 34/61] Updating translations for config/locales/en_CA.yml --- config/locales/en_CA.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/config/locales/en_CA.yml b/config/locales/en_CA.yml index 4775d5fd43..999346951b 100644 --- a/config/locales/en_CA.yml +++ b/config/locales/en_CA.yml @@ -1253,7 +1253,7 @@ en_CA: invoice_tax_total: "Sales Tax Total:" tax_invoice: "TAX INVOICE" tax_total: "Total tax (%{rate}):" - total_excl_tax: "Total (Excl. tax):" + total_excl_tax: "Total (incl. shipping tax if applicable)" total_incl_tax: "Total (Incl. tax):" abn: "Business Number:" acn: "Business Number:" @@ -3181,7 +3181,7 @@ en_CA: code: "Code" from: "From" to: "Bill to" - shipping: "Shipping" + shipping: "Delivery/Pick-up" form: distribution_fields: title: "Distribution" From eb9cf04071c9ad6b85337f6f395540672b7129cd Mon Sep 17 00:00:00 2001 From: Transifex-Openfoodnetwork Date: Tue, 16 Mar 2021 04:22:30 +1100 Subject: [PATCH 35/61] Updating translations for config/locales/fr_CA.yml --- config/locales/fr_CA.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/config/locales/fr_CA.yml b/config/locales/fr_CA.yml index c340aa827d..aea564acf9 100644 --- a/config/locales/fr_CA.yml +++ b/config/locales/fr_CA.yml @@ -1188,6 +1188,7 @@ fr_CA: mailers: powered_by: open_food_network: "Open Food Network " + powered_html: "Cette commande a été rendue possible par %{open_food_network}." menu: cart: cart: "Panier" @@ -2896,6 +2897,7 @@ fr_CA: name_or_sku: "Nom ou Ref Produit (entrer au moins les 4 premiers caractères du nom du produit)" resend: "Renvoyer" back_to_orders_list: "Retour à la liste des commandes" + back_to_payments_list: "Retour à la liste des paiements" return_authorizations: "Autorisations de retours" cannot_create_returns: "Impossible de créer une autorisation de retour car aucun produit n'a été livré pour cette commande." select_stock: "Sélectionner le stock" @@ -3465,6 +3467,7 @@ fr_CA: pending: en attente ready: prêt shipped: envoyé + canceled: Annulé payment_states: balance_due: solde dû completed: effectué @@ -3587,6 +3590,8 @@ fr_CA: past_orders: Commandes Passées transactions: transaction_history: Historique des Transactions + authorisation_required: Autorisation nécessaire + authorise: Autorise open_orders: order: Commander shop: Faire mes courses From f9f1bf8331aa1a943b1e6b40c8ae5666663c8c1d Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Tue, 16 Mar 2021 12:00:20 -0700 Subject: [PATCH 36/61] specify UTC explicitly --- .github/workflows/build.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index b0daa298ba..867205b8e4 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -7,6 +7,7 @@ on: env: DISABLE_KNAPSACK: true + TIMEZONE: UTC jobs: test-models: From 046a2077f212f437322a6ec9eb865fa82fac841d Mon Sep 17 00:00:00 2001 From: Andy Brett Date: Tue, 16 Mar 2021 12:14:27 -0700 Subject: [PATCH 37/61] check the time, not the printed zone, in the spec --- spec/features/admin/enterprises/terms_and_conditions_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/admin/enterprises/terms_and_conditions_spec.rb b/spec/features/admin/enterprises/terms_and_conditions_spec.rb index 311cf6e9cf..d465a7e651 100644 --- a/spec/features/admin/enterprises/terms_and_conditions_spec.rb +++ b/spec/features/admin/enterprises/terms_and_conditions_spec.rb @@ -54,7 +54,7 @@ feature "Uploading Terms and Conditions PDF" do go_to_business_details expect(page).to have_selector "a[href*='logo-white.pdf'][target=\"_blank\"]" - expect(page).to have_content time.strftime("%F %T %z") + expect(page).to have_content time.strftime("%F %T") # Replace PDF attach_file "enterprise[terms_and_conditions]", black_pdf_file_name From 6ebcff9a4a68628e782eac8d56abf556a5b45f1f Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 16 Mar 2021 19:32:53 +0000 Subject: [PATCH 38/61] Bump i18n from 1.8.5 to 1.8.9 Bumps [i18n](https://github.com/ruby-i18n/i18n) from 1.8.5 to 1.8.9. - [Release notes](https://github.com/ruby-i18n/i18n/releases) - [Changelog](https://github.com/ruby-i18n/i18n/blob/master/CHANGELOG.md) - [Commits](https://github.com/ruby-i18n/i18n/compare/v1.8.5...v1.8.9) Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 91d9975730..762223f0bf 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -266,7 +266,7 @@ GEM tilt hashdiff (1.0.1) highline (2.0.3) - i18n (1.8.5) + i18n (1.8.9) concurrent-ruby (~> 1.0) i18n-js (3.8.1) i18n (>= 0.6.6) From 87e4b5e49d60f0656d7c10acc831fd2942e7e9e0 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Sun, 14 Mar 2021 23:55:45 +0000 Subject: [PATCH 39/61] covers payment_state changes from item deletion --- spec/controllers/line_items_controller_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/controllers/line_items_controller_spec.rb b/spec/controllers/line_items_controller_spec.rb index fbe0d74b7a..176edb3a33 100644 --- a/spec/controllers/line_items_controller_spec.rb +++ b/spec/controllers/line_items_controller_spec.rb @@ -84,6 +84,19 @@ describe LineItemsController, type: :controller do expect(response.status).to eq 204 expect { item.reload }.to raise_error ActiveRecord::RecordNotFound end + + context "after a payment is captured" do + let(:payment) { create(:check_payment, amount: order.total, order: order, state: 'completed') } + before { payment.capture! } + + it 'updates the payment state' do + expect(order.payment_state).to eq 'paid' + delete :destroy, params + expect(response.status).to eq 204 + order.update! + expect(order.payment_state).to eq 'credit_owed' + end + end end end end From 2e870ed7bc923a21d4f3cccf6ec2eb0a9ec21560 Mon Sep 17 00:00:00 2001 From: filipefurtad0 Date: Mon, 15 Mar 2021 19:24:31 +0000 Subject: [PATCH 40/61] replace update! w/ reload; remove assertion --- spec/controllers/line_items_controller_spec.rb | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/spec/controllers/line_items_controller_spec.rb b/spec/controllers/line_items_controller_spec.rb index 176edb3a33..ce91ad74df 100644 --- a/spec/controllers/line_items_controller_spec.rb +++ b/spec/controllers/line_items_controller_spec.rb @@ -91,9 +91,8 @@ describe LineItemsController, type: :controller do it 'updates the payment state' do expect(order.payment_state).to eq 'paid' - delete :destroy, params - expect(response.status).to eq 204 - order.update! + delete :destroy, params + order.reload expect(order.payment_state).to eq 'credit_owed' end end From 7f594026cd4ba91638443f6b58a927ea296a48be Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 16 Mar 2021 22:32:03 +0000 Subject: [PATCH 41/61] Bump awesome_print from 1.8.0 to 1.9.2 Bumps [awesome_print](https://github.com/awesome-print/awesome_print) from 1.8.0 to 1.9.2. - [Release notes](https://github.com/awesome-print/awesome_print/releases) - [Changelog](https://github.com/awesome-print/awesome_print/blob/master/CHANGELOG.md) - [Commits](https://github.com/awesome-print/awesome_print/compare/v1.8.0...v1.9.2) Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 91d9975730..efb9a82cdf 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -123,7 +123,7 @@ GEM atomic (1.1.101) awesome_nested_set (3.4.0) activerecord (>= 4.0.0, < 7.0) - awesome_print (1.8.0) + awesome_print (1.9.2) aws-sdk (1.67.0) aws-sdk-v1 (= 1.67.0) aws-sdk-v1 (1.67.0) From 4adaa6d28d52f69ab78c0e1f79f4989c176867d7 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Tue, 16 Mar 2021 22:33:54 +0000 Subject: [PATCH 42/61] Bump rspec-rails from 4.0.2 to 4.1.2 Bumps [rspec-rails](https://github.com/rspec/rspec-rails) from 4.0.2 to 4.1.2. - [Release notes](https://github.com/rspec/rspec-rails/releases) - [Changelog](https://github.com/rspec/rspec-rails/blob/main/Changelog.md) - [Commits](https://github.com/rspec/rspec-rails/compare/v4.0.2...v4.1.2) Signed-off-by: dependabot[bot] --- Gemfile.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index 91d9975730..4cc2ba2214 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -450,10 +450,10 @@ GEM rspec-expectations (3.10.1) diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.10.0) - rspec-mocks (3.10.0) + rspec-mocks (3.10.2) diff-lcs (>= 1.2.0, < 2.0) rspec-support (~> 3.10.0) - rspec-rails (4.0.2) + rspec-rails (4.1.2) actionpack (>= 4.2) activesupport (>= 4.2) railties (>= 4.2) @@ -463,7 +463,7 @@ GEM rspec-support (~> 3.10) rspec-retry (0.6.2) rspec-core (> 3.3) - rspec-support (3.10.1) + rspec-support (3.10.2) rswag (2.4.0) rswag-api (= 2.4.0) rswag-specs (= 2.4.0) From d4cbf472264adeb1d2246bb4a7c76fda8280d2c8 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 12 Mar 2021 21:43:35 +0000 Subject: [PATCH 43/61] Add T and Cs helper to checkout controller --- app/controllers/checkout_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 4022925385..ce1e88aa9f 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -10,6 +10,8 @@ class CheckoutController < ::BaseController include OrderCyclesHelper include EnterprisesHelper + helper 'terms_and_conditions' + ssl_required # We need pessimistic locking to avoid race conditions. From f55150745ef2a5b646c81866d74bf231c269a91a Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Fri, 12 Mar 2021 22:07:50 +0000 Subject: [PATCH 44/61] Add checkout helper to checkout controller --- app/controllers/checkout_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index ce1e88aa9f..1374c1ccb0 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -11,7 +11,8 @@ class CheckoutController < ::BaseController include EnterprisesHelper helper 'terms_and_conditions' - + helper 'checkout' + ssl_required # We need pessimistic locking to avoid race conditions. From 32c68f59514c6e81394573a20af2bd36a0c107ca Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 13 Mar 2021 10:56:01 +0000 Subject: [PATCH 45/61] Adapt helper call to make it work in rails 5.2 --- app/mailers/spree/order_mailer.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/mailers/spree/order_mailer.rb b/app/mailers/spree/order_mailer.rb index 9fbed9f02b..00002715c3 100644 --- a/app/mailers/spree/order_mailer.rb +++ b/app/mailers/spree/order_mailer.rb @@ -2,7 +2,7 @@ module Spree class OrderMailer < BaseMailer - helper ::CheckoutHelper + helper 'checkout' helper SpreeCurrencyHelper helper Spree::Admin::PaymentsHelper helper OrderHelper From c4c5bbc9a3fd1287d8f07494285e91d7eaf1d62a Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Sat, 13 Mar 2021 11:22:39 +0000 Subject: [PATCH 46/61] Adapt helpers to rails 5.2 --- app/controllers/checkout_controller.rb | 3 --- app/helpers/injection_helper.rb | 2 ++ app/mailers/subscription_mailer.rb | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 1374c1ccb0..1b8cd07d67 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -6,9 +6,6 @@ class CheckoutController < ::BaseController layout 'darkswarm' include OrderStockCheck - include CheckoutHelper - include OrderCyclesHelper - include EnterprisesHelper helper 'terms_and_conditions' helper 'checkout' diff --git a/app/helpers/injection_helper.rb b/app/helpers/injection_helper.rb index 7addd06135..3af44f885f 100644 --- a/app/helpers/injection_helper.rb +++ b/app/helpers/injection_helper.rb @@ -2,6 +2,8 @@ require 'open_food_network/enterprise_injection_data' module InjectionHelper include SerializerHelper + include EnterprisesHelper + include OrderCyclesHelper def inject_enterprises(enterprises = nil) inject_json_array( diff --git a/app/mailers/subscription_mailer.rb b/app/mailers/subscription_mailer.rb index 0f60ccbeab..dd20008bfc 100644 --- a/app/mailers/subscription_mailer.rb +++ b/app/mailers/subscription_mailer.rb @@ -1,5 +1,5 @@ class SubscriptionMailer < Spree::BaseMailer - helper CheckoutHelper + helper 'checkout' helper MailerHelper helper ShopMailHelper helper OrderHelper From f56678bcd98363246855ce3e41948ab00deab3f4 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Tue, 16 Mar 2021 18:12:14 +0100 Subject: [PATCH 47/61] Rearrange and extend some OrderMailer unit-tests This moves them to a more unit-like style where the mailer methods are the subjects. However, I did so only for the methods that show order balance and thus we want to be extra sure of their coverage. --- spec/mailers/order_mailer_spec.rb | 70 ++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 10 deletions(-) diff --git a/spec/mailers/order_mailer_spec.rb b/spec/mailers/order_mailer_spec.rb index fb3e22cd4b..d23b920c55 100644 --- a/spec/mailers/order_mailer_spec.rb +++ b/spec/mailers/order_mailer_spec.rb @@ -5,27 +5,77 @@ require 'spec_helper' describe Spree::OrderMailer do include OpenFoodNetwork::EmailHelper - context "basic behaviour" do + describe '#confirm_email_for_customer' do + subject(:email) { described_class.confirm_email_for_customer(order) } + let(:order) { build(:order_with_totals_and_distribution) } - context ":from not set explicitly" do - it "falls back to spree config" do - message = Spree::OrderMailer.confirm_email_for_customer(order) - expect(message.from).to eq [Spree::Config[:mails_from]] + it 'renders the shared/_payment.html.haml partial' do + expect(email.body).to include(I18n.t(:email_payment_summary)) + end + + context 'when the order has outstanding balance' do + before { allow(order).to receive(:outstanding_balance) { 123 } } + + it 'renders the amount as money' do + expect(email.body).to include('$123') end end - it "doesn't aggressively escape double quotes in confirmation body" do - confirmation_email = Spree::OrderMailer.confirm_email_for_customer(order) - expect(confirmation_email.body).to_not include(""") + context 'when the order has no outstanding balance' do + before { allow(order).to receive(:outstanding_balance) { 0 } } + + it 'displays the payment status' do + expect(email.body).to include(I18n.t(:email_payment_not_paid)) + end end - it "confirm_email_for_customer accepts an order id as an alternative to an Order object" do + context "when :from is not set explicitly" do + it "falls back to spree config" do + expect(email.from).to eq [Spree::Config[:mails_from]] + end + end + + it "doesn't aggressively escape double quotes body" do + expect(email.body).to_not include(""") + end + + it "accepts an order id as an alternative to an Order object" do expect(Spree::Order).to receive(:find).with(order.id).and_return(order) expect { - Spree::OrderMailer.confirm_email_for_customer(order.id).deliver_now + described_class.confirm_email_for_customer(order.id).deliver_now }.to_not raise_error end + end + + describe '#confirm_email_for_shop' do + subject(:email) { described_class.confirm_email_for_shop(order) } + + let(:order) { build(:order_with_totals_and_distribution) } + + it 'renders the shared/_payment.html.haml partial' do + expect(email.body).to include(I18n.t(:email_payment_summary)) + end + + context 'when the order has outstanding balance' do + before { allow(order).to receive(:outstanding_balance) { 123 } } + + it 'renders the amount as money' do + expect(email.body).to include('$123') + end + end + + context 'when the order has no outstanding balance' do + before { allow(order).to receive(:outstanding_balance) { 0 } } + + it 'displays the payment status' do + expect(email.body).to include(I18n.t(:email_payment_not_paid)) + end + end + end + + context "basic behaviour" do + let(:order) { build(:order_with_totals_and_distribution) } it "cancel_email accepts an order id as an alternative to an Order object" do expect(Spree::Order).to receive(:find).with(order.id).and_return(order) From 736a6280820e1dd97df441c00414685111b7cfee Mon Sep 17 00:00:00 2001 From: Transifex-Openfoodnetwork Date: Wed, 17 Mar 2021 22:31:32 +1100 Subject: [PATCH 48/61] Updating translations for config/locales/en_IE.yml --- config/locales/en_IE.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/config/locales/en_IE.yml b/config/locales/en_IE.yml index 86b30bee73..6f3ef2d189 100644 --- a/config/locales/en_IE.yml +++ b/config/locales/en_IE.yml @@ -1186,6 +1186,7 @@ en_IE: mailers: powered_by: open_food_network: "Open Food Network" + powered_html: "Your shopping experience is powered by the %{open_food_network}." menu: cart: cart: "Basket" @@ -1552,6 +1553,7 @@ en_IE: set_a_password: "You will then be prompted to set a password before you are able to administer the enterprise." mistakenly_sent: "Not sure why you have received this email? Please contact %{owner_email} for more information." producer_mail_greeting: "Dear" + producer_mail_text_before: "Please find below an update about the order cycle ready for:" producer_mail_order_text: "Here is a summary of the orders for your products:" producer_mail_delivery_instructions: "Stock pickup/delivery instructions:" producer_mail_signoff: "Thanks and best wishes" @@ -2888,6 +2890,7 @@ en_IE: name_or_sku: "Name or SKU (enter at least first 4 characters of product name)" resend: "Resend" back_to_orders_list: "Back To Orders List" + back_to_payments_list: "Back To Payments List" return_authorizations: "Return Authorisations" cannot_create_returns: "Cannot create returns as this order has no shipped units." select_stock: "Select stock" @@ -3457,6 +3460,7 @@ en_IE: pending: pending ready: ready shipped: shipped + canceled: canceled payment_states: balance_due: balance due completed: completed @@ -3578,6 +3582,8 @@ en_IE: past_orders: Past Orders transactions: transaction_history: Transaction History + authorisation_required: Authorisation Required + authorise: Authorize open_orders: order: Order shop: Shop From 8c57ccdaf9e02ba2233c4116a3481d3d85639df9 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Mon, 15 Mar 2021 11:48:28 +0100 Subject: [PATCH 49/61] Replace assets test config with public_file_server This fixes the following deprecation warnings ``` DEPRECATION WARNING: `config.serve_static_files` is deprecated and will be removed in Rails 5.1. Please use `config.public_file_server.enabled = true` instead. (called from block in at /home/runner/work/openfoodnetwork/openfoodnetwork/config/environments/test.rb:13) DEPRECATION WARNING: `config.static_cache_control` is deprecated and will be removed in Rails 5.1. Please use `config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' }` instead. (called from block in at /home/runner/work/openfoodnetwork/openfoodnetwork/config/environments/test.rb:14) ``` --- config/environments/production.rb | 2 +- config/environments/staging.rb | 2 +- config/environments/test.rb | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/config/environments/production.rb b/config/environments/production.rb index 695f61a0a1..826a4de616 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -11,7 +11,7 @@ Openfoodnetwork::Application.configure do config.action_controller.perform_caching = true # Disable Rails's static asset server (Apache or nginx will already do this) - config.serve_static_files = false + config.public_file_server.enabled = false # Compress JavaScripts and CSS config.assets.compress = true diff --git a/config/environments/staging.rb b/config/environments/staging.rb index 695f61a0a1..826a4de616 100644 --- a/config/environments/staging.rb +++ b/config/environments/staging.rb @@ -11,7 +11,7 @@ Openfoodnetwork::Application.configure do config.action_controller.perform_caching = true # Disable Rails's static asset server (Apache or nginx will already do this) - config.serve_static_files = false + config.public_file_server.enabled = false # Compress JavaScripts and CSS config.assets.compress = true diff --git a/config/environments/test.rb b/config/environments/test.rb index 665525bb13..dd68bc0459 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -10,8 +10,8 @@ Openfoodnetwork::Application.configure do config.eager_load = false # Configure static asset server for tests with Cache-Control for performance - config.serve_static_files = true - config.static_cache_control = "public, max-age=3600" + config.public_file_server.enabled = true + config.public_file_server.headers = { 'Cache-Control' => 'public, max-age=3600' } # Separate cache stores when running in parallel config.cache_store = :file_store, Rails.root.join("tmp", "cache", "paralleltests#{ENV['TEST_ENV_NUMBER']}") From cf730f8b0cb8727f71146f98cc3bbd511bf64763 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 17 Mar 2021 16:06:48 +0100 Subject: [PATCH 50/61] Temporarily skip embedded shopfront flaky spec Closes #7129 as discussed on that issue. --- spec/features/consumer/shopping/embedded_shopfronts_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/consumer/shopping/embedded_shopfronts_spec.rb b/spec/features/consumer/shopping/embedded_shopfronts_spec.rb index 2d2d3113f1..ff15d4a46d 100644 --- a/spec/features/consumer/shopping/embedded_shopfronts_spec.rb +++ b/spec/features/consumer/shopping/embedded_shopfronts_spec.rb @@ -90,7 +90,7 @@ feature "Using embedded shopfront functionality", js: true do end end - it "redirects to embedded hub on logout when embedded" do + xit "redirects to embedded hub on logout when embedded" do on_embedded_page do wait_for_cart find('#login-link a').click From 6c19baeab39da96855f17b121c400759e903403c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 4 Mar 2021 13:16:15 +0000 Subject: [PATCH 51/61] Remove transactional callback config --- config/application.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/config/application.rb b/config/application.rb index 73d9e78184..33638102f5 100644 --- a/config/application.rb +++ b/config/application.rb @@ -200,7 +200,5 @@ module Openfoodnetwork config.active_support.escape_html_entities_in_json = true config.active_job.queue_adapter = :delayed_job - - config.active_record.raise_in_transactional_callbacks = true end end From 237075dd471476b6f03d6b00110069398cd7eccf Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 4 Mar 2021 13:19:40 +0000 Subject: [PATCH 52/61] Replace deprecated before_filter syntax --- .../order_management/reports/bulk_coop_controller.rb | 4 ++-- .../reports/enterprise_fee_summaries_controller.rb | 4 ++-- lib/spree/core/controller_helpers/auth.rb | 2 +- lib/spree/core/controller_helpers/common.rb | 2 +- lib/spree/core/controller_helpers/order.rb | 2 +- lib/spree/core/controller_helpers/ssl.rb | 2 +- spec/controllers/spree/admin/base_controller_spec.rb | 2 +- 7 files changed, 9 insertions(+), 9 deletions(-) diff --git a/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb b/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb index c1810a92b6..3de16eaf61 100644 --- a/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb +++ b/engines/order_management/app/controllers/order_management/reports/bulk_coop_controller.rb @@ -3,8 +3,8 @@ module OrderManagement module Reports class BulkCoopController < Spree::Admin::BaseController - before_filter :load_report_parameters - before_filter :load_permissions + before_action :load_report_parameters + before_action :load_permissions def new; end diff --git a/engines/order_management/app/controllers/order_management/reports/enterprise_fee_summaries_controller.rb b/engines/order_management/app/controllers/order_management/reports/enterprise_fee_summaries_controller.rb index 7cc1376034..9f52ab3e9b 100644 --- a/engines/order_management/app/controllers/order_management/reports/enterprise_fee_summaries_controller.rb +++ b/engines/order_management/app/controllers/order_management/reports/enterprise_fee_summaries_controller.rb @@ -3,8 +3,8 @@ module OrderManagement module Reports class EnterpriseFeeSummariesController < Spree::Admin::BaseController - before_filter :load_report_parameters - before_filter :load_permissions + before_action :load_report_parameters + before_action :load_permissions def new; end diff --git a/lib/spree/core/controller_helpers/auth.rb b/lib/spree/core/controller_helpers/auth.rb index c6d05deaeb..fc2c1201e3 100644 --- a/lib/spree/core/controller_helpers/auth.rb +++ b/lib/spree/core/controller_helpers/auth.rb @@ -7,7 +7,7 @@ module Spree extend ActiveSupport::Concern included do - before_filter :ensure_api_key + before_action :ensure_api_key rescue_from CanCan::AccessDenied do unauthorized diff --git a/lib/spree/core/controller_helpers/common.rb b/lib/spree/core/controller_helpers/common.rb index c6978382e7..2655aea13b 100644 --- a/lib/spree/core/controller_helpers/common.rb +++ b/lib/spree/core/controller_helpers/common.rb @@ -12,7 +12,7 @@ module Spree layout :get_layout - before_filter :set_user_language + before_action :set_user_language protected diff --git a/lib/spree/core/controller_helpers/order.rb b/lib/spree/core/controller_helpers/order.rb index 460bd8e15f..d3f093cfd2 100644 --- a/lib/spree/core/controller_helpers/order.rb +++ b/lib/spree/core/controller_helpers/order.rb @@ -10,7 +10,7 @@ module Spree base.class_eval do helper_method :current_order helper_method :current_currency - before_filter :set_current_order + before_action :set_current_order end end diff --git a/lib/spree/core/controller_helpers/ssl.rb b/lib/spree/core/controller_helpers/ssl.rb index 6c923e4977..07a9ad5ccd 100644 --- a/lib/spree/core/controller_helpers/ssl.rb +++ b/lib/spree/core/controller_helpers/ssl.rb @@ -7,7 +7,7 @@ module Spree extend ActiveSupport::Concern included do - before_filter :force_non_ssl_redirect, if: proc { Spree::Config[:redirect_https_to_http] } + before_action :force_non_ssl_redirect, if: proc { Spree::Config[:redirect_https_to_http] } def self.ssl_allowed(*actions) class_attribute :ssl_allowed_actions diff --git a/spec/controllers/spree/admin/base_controller_spec.rb b/spec/controllers/spree/admin/base_controller_spec.rb index 5cbcfbfc58..2947bbf698 100644 --- a/spec/controllers/spree/admin/base_controller_spec.rb +++ b/spec/controllers/spree/admin/base_controller_spec.rb @@ -5,7 +5,7 @@ require 'spec_helper' describe Spree::Admin::BaseController, type: :controller do controller(Spree::Admin::BaseController) do def index - before_filter :unauthorized + before_action :unauthorized render text: "" end end From 55b0751c1a0a0fbe79124750a48d79060ff540f5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 4 Mar 2021 13:20:58 +0000 Subject: [PATCH 53/61] Use strings for class names on associations --- app/models/customer.rb | 8 ++++---- app/models/distributor_shipping_method.rb | 4 ++-- app/models/enterprise_role.rb | 2 +- app/models/subscription.rb | 4 ++-- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/models/customer.rb b/app/models/customer.rb index 256fc1b61c..b1ba0f2651 100644 --- a/app/models/customer.rb +++ b/app/models/customer.rb @@ -2,15 +2,15 @@ class Customer < ActiveRecord::Base acts_as_taggable belongs_to :enterprise - belongs_to :user, class_name: Spree.user_class - has_many :orders, class_name: Spree::Order + belongs_to :user, class_name: Spree.user_class.to_s + has_many :orders, class_name: "Spree::Order" before_destroy :check_for_orders - belongs_to :bill_address, foreign_key: :bill_address_id, class_name: Spree::Address + belongs_to :bill_address, foreign_key: :bill_address_id, class_name: "Spree::Address" alias_attribute :billing_address, :bill_address accepts_nested_attributes_for :bill_address - belongs_to :ship_address, foreign_key: :ship_address_id, class_name: Spree::Address + belongs_to :ship_address, foreign_key: :ship_address_id, class_name: "Spree::Address" alias_attribute :shipping_address, :ship_address accepts_nested_attributes_for :ship_address diff --git a/app/models/distributor_shipping_method.rb b/app/models/distributor_shipping_method.rb index a2c81506dd..5c69cefbc0 100644 --- a/app/models/distributor_shipping_method.rb +++ b/app/models/distributor_shipping_method.rb @@ -1,5 +1,5 @@ class DistributorShippingMethod < ActiveRecord::Base self.table_name = "distributors_shipping_methods" - belongs_to :shipping_method, class_name: Spree::ShippingMethod, touch: true - belongs_to :distributor, class_name: Enterprise, touch: true + belongs_to :shipping_method, class_name: "Spree::ShippingMethod", touch: true + belongs_to :distributor, class_name: "Enterprise", touch: true end diff --git a/app/models/enterprise_role.rb b/app/models/enterprise_role.rb index 3709f7e004..0b686e2e6e 100644 --- a/app/models/enterprise_role.rb +++ b/app/models/enterprise_role.rb @@ -1,5 +1,5 @@ class EnterpriseRole < ActiveRecord::Base - belongs_to :user, class_name: Spree.user_class + belongs_to :user, class_name: Spree.user_class.to_s belongs_to :enterprise validates :user_id, :enterprise_id, presence: true diff --git a/app/models/subscription.rb b/app/models/subscription.rb index ca1fd676e8..dbb5a8bf23 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -8,8 +8,8 @@ class Subscription < ActiveRecord::Base belongs_to :schedule belongs_to :shipping_method, class_name: 'Spree::ShippingMethod' belongs_to :payment_method, class_name: 'Spree::PaymentMethod' - belongs_to :bill_address, foreign_key: :bill_address_id, class_name: Spree::Address - belongs_to :ship_address, foreign_key: :ship_address_id, class_name: Spree::Address + belongs_to :bill_address, foreign_key: :bill_address_id, class_name: "Spree::Address" + belongs_to :ship_address, foreign_key: :ship_address_id, class_name: "Spree::Address" has_many :subscription_line_items, inverse_of: :subscription has_many :order_cycles, through: :schedule has_many :proxy_orders From b48677c624dc658cc296de1de0e4c79eef418d9b Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 17 Mar 2021 16:25:50 +0100 Subject: [PATCH 54/61] Rearrange tests to make them method-centric If these are unit tests, it's much easier to find a `describe` with the method under test and putting all the tests exercising that method together. It turns out that `#update_payment_state` is by far the method that we test the most which leads me to think: a) this class might be doing too many things. b) other methods might not be that well covered. --- .../order_management/order/updater_spec.rb | 173 +++++++++--------- 1 file changed, 89 insertions(+), 84 deletions(-) diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb index f0df8269af..50c95d5eba 100644 --- a/engines/order_management/spec/services/order_management/order/updater_spec.rb +++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb @@ -141,114 +141,119 @@ module OrderManagement updater.update end - it "is failed if no valid payments" do - allow(order).to receive_message_chain(:payments, :valid, :empty?).and_return(true) + describe "#update_payment_state" do + context "when the order has no valid payments" do + it "is failed" do + allow(order).to receive_message_chain(:payments, :valid, :empty?).and_return(true) - updater.update_payment_state - expect(order.payment_state).to eq('failed') - end - - context "payment total is greater than order total" do - it "is credit_owed" do - order.payment_total = 2 - order.total = 1 - - expect { updater.update_payment_state - }.to change { order.payment_state }.to 'credit_owed' - end - end - - context "order total is greater than payment total" do - it "is credit_owed" do - order.payment_total = 1 - order.total = 2 - - expect { - updater.update_payment_state - }.to change { order.payment_state }.to 'balance_due' - end - end - - context "order total equals payment total" do - it "is paid" do - order.payment_total = 30 - order.total = 30 - - expect { - updater.update_payment_state - }.to change { order.payment_state }.to 'paid' - end - end - - context "order is canceled" do - before do - order.state = 'canceled' - end - - context "and is still unpaid" do - it "is void" do - order.payment_total = 0 - order.total = 30 - expect { - updater.update_payment_state - }.to change { order.payment_state }.to 'void' + expect(order.payment_state).to eq('failed') end end - context "and is paid" do + context "payment total is greater than order total" do it "is credit_owed" do - order.payment_total = 30 - order.total = 30 - allow(order).to receive_message_chain(:payments, :valid, :empty?).and_return(false) - allow(order).to receive_message_chain(:payments, :completed, :empty?).and_return(false) + order.payment_total = 2 + order.total = 1 + expect { updater.update_payment_state }.to change { order.payment_state }.to 'credit_owed' end end - context "and payment is refunded" do - it "is void" do - order.payment_total = 0 - order.total = 30 - allow(order).to receive_message_chain(:payments, :valid, :empty?).and_return(false) - allow(order).to receive_message_chain(:payments, :completed, :empty?).and_return(false) + context "order total is greater than payment total" do + it "is credit_owed" do + order.payment_total = 1 + order.total = 2 + expect { updater.update_payment_state - }.to change { order.payment_state }.to 'void' - end - end - end - - context 'when the set payment_state does not match the last payment_state' do - before { order.payment_state = 'previous_to_paid' } - - context 'and the order is being updated' do - before { allow(order).to receive(:persisted?) { true } } - - it 'creates a new state_change for the order' do - expect { updater.update_payment_state } - .to change { order.state_changes.size }.by(1) + }.to change { order.payment_state }.to 'balance_due' end end - context 'and the order is being created' do - before { allow(order).to receive(:persisted?) { false } } + context "order total equals payment total" do + it "is paid" do + order.payment_total = 30 + order.total = 30 - it 'creates a new state_change for the order' do + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'paid' + end + end + + context "order is canceled" do + before { order.state = 'canceled' } + + context "and is still unpaid" do + it "is void" do + order.payment_total = 0 + order.total = 30 + + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'void' + end + end + + context "and is paid" do + it "is credit_owed" do + order.payment_total = 30 + order.total = 30 + allow(order).to receive_message_chain(:payments, :valid, :empty?).and_return(false) + allow(order).to receive_message_chain(:payments, :completed, :empty?).and_return(false) + + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'credit_owed' + end + end + + context "and payment is refunded" do + it "is void" do + order.payment_total = 0 + order.total = 30 + allow(order).to receive_message_chain(:payments, :valid, :empty?).and_return(false) + allow(order).to receive_message_chain(:payments, :completed, :empty?).and_return(false) + + expect { + updater.update_payment_state + }.to change { order.payment_state }.to 'void' + end + end + end + + context 'when the set payment_state matches the last payment_state' do + before { order.payment_state = 'paid' } + + it 'does not create any state_change' do expect { updater.update_payment_state } .not_to change { order.state_changes.size } end end - end - context 'when the set payment_state matches the last payment_state' do - before { order.payment_state = 'paid' } + context 'when the set payment_state does not match the last payment_state' do + before { order.payment_state = 'previous_to_paid' } - it 'does not create any state_change' do - expect { updater.update_payment_state } - .not_to change { order.state_changes.size } + context 'and the order is being updated' do + before { allow(order).to receive(:persisted?) { true } } + + it 'creates a new state_change for the order' do + expect { updater.update_payment_state } + .to change { order.state_changes.size }.by(1) + end + end + + context 'and the order is being created' do + before { allow(order).to receive(:persisted?) { false } } + + it 'creates a new state_change for the order' do + expect { updater.update_payment_state } + .not_to change { order.state_changes.size } + end + end end end From 826515874b480b7768d07809d76566402fb59ccf Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 11 Mar 2021 14:36:30 +0000 Subject: [PATCH 55/61] Replace some uses of #alias_method_chain --- app/controllers/admin/enterprise_groups_controller.rb | 5 ++--- app/controllers/admin/enterprises_controller.rb | 5 ++--- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/app/controllers/admin/enterprise_groups_controller.rb b/app/controllers/admin/enterprise_groups_controller.rb index ad1489e654..0691d9eeaa 100644 --- a/app/controllers/admin/enterprise_groups_controller.rb +++ b/app/controllers/admin/enterprise_groups_controller.rb @@ -25,15 +25,14 @@ module Admin protected - def build_resource_with_address - enterprise_group = build_resource_without_address + def build_resource + enterprise_group = super enterprise_group.address = Spree::Address.new enterprise_group.address.country = Spree::Country.find_by( id: Spree::Config[:default_country_id] ) enterprise_group end - alias_method_chain :build_resource, :address # Overriding method on Spree's resource controller, # so that resources are found using permalink. diff --git a/app/controllers/admin/enterprises_controller.rb b/app/controllers/admin/enterprises_controller.rb index 87a3dff224..9b42b39174 100644 --- a/app/controllers/admin/enterprises_controller.rb +++ b/app/controllers/admin/enterprises_controller.rb @@ -114,13 +114,12 @@ module Admin protected - def build_resource_with_address - enterprise = build_resource_without_address + def build_resource + enterprise = super enterprise.address ||= Spree::Address.new enterprise.address.country ||= Spree::Country.find_by(id: Spree::Config[:default_country_id]) enterprise end - alias_method_chain :build_resource, :address # Overriding method on Spree's resource controller, # so that resources are found using permalink From 064f7582cc746477d68d2cca1f5f0fe54dd94f55 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 28 Feb 2021 10:48:28 +0000 Subject: [PATCH 56/61] Update line_item included taxes Drops use of the `spree_adjustments.included_tax` database field (when summing line item tax), which we are slowly deprecating before eventual removal --- app/models/spree/line_item.rb | 4 ++-- app/models/spree/order.rb | 4 +++- app/models/spree/tax_rate.rb | 2 +- .../app/services/order_management/order/updater.rb | 2 +- lib/open_food_network/sales_tax_report.rb | 2 +- spec/models/spree/adjustment_spec.rb | 8 ++++---- spec/models/spree/line_item_spec.rb | 5 ++++- 7 files changed, 16 insertions(+), 11 deletions(-) diff --git a/app/models/spree/line_item.rb b/app/models/spree/line_item.rb index cda88aa395..d224b154a8 100644 --- a/app/models/spree/line_item.rb +++ b/app/models/spree/line_item.rb @@ -174,11 +174,11 @@ module Spree end def has_tax? - adjustments.included_tax.any? + adjustments.tax.any? end def included_tax - adjustments.included_tax.sum(:included_tax) + adjustments.tax.inclusive.sum(:amount) end def tax_rates diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index 7a1eff9e03..b1731b42d4 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -662,7 +662,9 @@ module Spree end def total_tax - all_adjustments.sum(:included_tax) + adjustments.sum(:included_tax) + + shipment_adjustments.sum(:included_tax) + + line_item_adjustments.tax.sum(:amount) end def has_taxes_included diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index e0fe37a776..eb543306f7 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -84,7 +84,7 @@ module Spree order.line_items.reload # TaxRate adjustments (order.adjustments.tax) # and line item adjustments (tax included on line items) consist of 100% tax - (order.adjustments.tax + order.line_item_adjustments.reload).each do |adjustment| + order.adjustments.tax.additional.each do |adjustment| adjustment.set_absolute_included_tax! adjustment.amount end end diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb index 7ff69ceeaa..4aac1f7843 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -59,7 +59,7 @@ module OrderManagement def update_adjustment_total order.adjustment_total = all_adjustments.additional.eligible.sum(:amount) order.additional_tax_total = all_adjustments.tax.additional.sum(:amount) - order.included_tax_total = order.line_item_adjustments.tax.sum(:included_tax) + + order.included_tax_total = order.line_item_adjustments.tax.inclusive.sum(:amount) + all_adjustments.enterprise_fee.sum(:included_tax) + all_adjustments.shipping.sum(:included_tax) + adjustments.admin.sum(:included_tax) diff --git a/lib/open_food_network/sales_tax_report.rb b/lib/open_food_network/sales_tax_report.rb index 8abce8c0e4..83bfadd8b6 100644 --- a/lib/open_food_network/sales_tax_report.rb +++ b/lib/open_food_network/sales_tax_report.rb @@ -100,7 +100,7 @@ module OpenFoodNetwork end def tax_included_in(line_item) - line_item.adjustments.sum(:included_tax) + line_item.adjustments.tax.inclusive.sum(:amount) end def shipment_inc_vat diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 1df48296ca..b46a431041 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -185,9 +185,9 @@ module Spree tax_rate.adjust(order) end - it "has 100% tax included" do + it "has tax included" do expect(adjustment.amount).to be > 0 - expect(adjustment.included_tax).to eq(adjustment.amount) + expect(adjustment.included).to be true end it "does not crash when order data has been updated previously" do @@ -365,7 +365,7 @@ module Spree # so tax on the enterprise_fee adjustment will be 0 # Tax on line item is: 0.2/1.2 x $10 = $1.67 expect(adjustment.included_tax).to eq(0.0) - expect(line_item.adjustments.first.included_tax).to eq(1.67) + expect(line_item.adjustments.tax.first.amount).to eq(1.67) end end @@ -395,7 +395,7 @@ module Spree # gives tax on fee of 0.2/1.2 x $50 = $8.33 # Tax on line item is: 0.2/1.2 x $10 = $1.67 expect(adjustment.included_tax).to eq(8.33) - expect(line_item.adjustments.first.included_tax).to eq(1.67) + expect(line_item.adjustments.tax.first.amount).to eq(1.67) end end diff --git a/spec/models/spree/line_item_spec.rb b/spec/models/spree/line_item_spec.rb index 0ac72a7f4b..0c0277f00a 100644 --- a/spec/models/spree/line_item_spec.rb +++ b/spec/models/spree/line_item_spec.rb @@ -443,7 +443,10 @@ module Spree let(:li_no_tax) { create(:line_item) } let(:li_tax) { create(:line_item) } let(:tax_rate) { create(:tax_rate, calculator: ::Calculator::DefaultTax.new) } - let!(:adjustment) { create(:adjustment, adjustable: li_tax, originator: tax_rate, label: "TR", amount: 123, included_tax: 10.00) } + let!(:adjustment) { + create(:adjustment, adjustable: li_tax, originator: tax_rate, label: "TR", + amount: 10.00, included: true) + } context "checking if a line item has tax included" do it "returns true when it does" do From baaee1baab4aaddc8db8419c0bb05f81c34bec5a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 28 Feb 2021 16:04:01 +0000 Subject: [PATCH 57/61] Update OrderAdjustmentsFetcher --- app/services/order_tax_adjustments_fetcher.rb | 19 +++++++++++++++++-- .../order_tax_adjustments_fetcher_spec.rb | 12 +++++++----- 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 23b5dde065..686efff9a4 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -21,7 +21,6 @@ class OrderTaxAdjustmentsFetcher def all Spree::Adjustment - .with_tax .where(order_adjustments.or(line_item_adjustments).or(shipment_adjustments)) .order('created_at ASC') end @@ -50,11 +49,27 @@ class OrderTaxAdjustmentsFetcher Hash[tax_rates.collect do |tax_rate| tax_amount = if tax_rates.one? - adjustment.included_tax + adjustment_tax_amount(adjustment) else tax_rate.compute_tax(adjustment.amount) end [tax_rate, tax_amount] end] end + + def adjustment_tax_amount(adjustment) + if no_tax_adjustments?(adjustment) + adjustment.included_tax + else + adjustment.amount + end + end + + def no_tax_adjustments?(adjustment) + # Enterprise Fees, Admin Adjustments, and Shipping Fees currently do not have tax adjustments. + # The tax amount is stored in the included_tax attribute. + adjustment.originator_type == "EnterpriseFee" || + adjustment.originator_type == "Spree::ShippingMethod" || + (adjustment.source_type.nil? && adjustment.originator_type.nil?) + end end diff --git a/spec/services/order_tax_adjustments_fetcher_spec.rb b/spec/services/order_tax_adjustments_fetcher_spec.rb index de7f788230..f5edd0d848 100644 --- a/spec/services/order_tax_adjustments_fetcher_spec.rb +++ b/spec/services/order_tax_adjustments_fetcher_spec.rb @@ -44,8 +44,9 @@ describe OrderTaxAdjustmentsFetcher do tax_category: tax_category20, calculator: Calculator::FlatRate.new(preferred_amount: 48.0)) end - let(:additional_adjustment) do - create(:adjustment, amount: 50.0, included_tax: tax_rate25.compute_tax(50.0)) + let(:admin_adjustment) do + create(:adjustment, order: order, amount: 50.0, included_tax: tax_rate25.compute_tax(50.0), + source: nil, label: "Admin Adjustment") end let(:order_cycle) do @@ -62,8 +63,7 @@ describe OrderTaxAdjustmentsFetcher do line_items: [line_item1, line_item2], bill_address: create(:address), order_cycle: order_cycle, - distributor: coordinator, - adjustments: [additional_adjustment] + distributor: coordinator ) end @@ -80,6 +80,8 @@ describe OrderTaxAdjustmentsFetcher do end before do + order.reload + order.adjustments << admin_adjustment order.create_tax_charge! order.recreate_all_fees! end @@ -102,7 +104,7 @@ describe OrderTaxAdjustmentsFetcher do expect(subject[tax_rate20]).to eq(8.0) end - it "contains tax on order adjustment" do + it "contains tax on admin adjustment" do expect(subject[tax_rate25]).to eq(10.0) end end From e62cf67be574c8cbab84720b60a895ccb9c1816d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 28 Feb 2021 20:41:47 +0000 Subject: [PATCH 58/61] Add more defensive code in TaxRateFinder --- app/services/tax_rate_finder.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/tax_rate_finder.rb b/app/services/tax_rate_finder.rb index f27a42ef85..01b955d3d0 100644 --- a/app/services/tax_rate_finder.rb +++ b/app/services/tax_rate_finder.rb @@ -75,7 +75,7 @@ class TaxRateFinder # to the included tax. def find_closest_tax_rates_from_included_tax(amount, included_tax) approximation = (included_tax / (amount - included_tax)) - return [] if approximation.infinite? || approximation.zero? + return [] if approximation.infinite? || approximation.zero? || approximation.nan? [Spree::TaxRate.order("ABS(amount - #{approximation})").first] end From ae9d8020a15b07e401dcfaf2d11119d5757c6965 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 25 Feb 2021 16:11:41 +0100 Subject: [PATCH 59/61] Better test InvoiceRenderer This required a tiny refactoring to enable injecting the renderer. It'll make it easier to later add the relevant specs related to the order balance. --- app/services/invoice_renderer.rb | 8 +++-- spec/services/invoice_renderer_spec.rb | 43 +++++++++++++++++++++----- 2 files changed, 40 insertions(+), 11 deletions(-) diff --git a/app/services/invoice_renderer.rb b/app/services/invoice_renderer.rb index 2f8663d24c..bbb1886a5b 100644 --- a/app/services/invoice_renderer.rb +++ b/app/services/invoice_renderer.rb @@ -1,4 +1,8 @@ class InvoiceRenderer + def initialize(renderer = ApplicationController.new) + @renderer = renderer + end + def render_to_string(order) renderer.instance_variable_set(:@order, order) renderer.render_to_string(args(order)) @@ -15,9 +19,7 @@ class InvoiceRenderer private - def renderer - @renderer ||= ApplicationController.new - end + attr_reader :renderer def invoice_template if Spree::Config.invoice_style2? diff --git a/spec/services/invoice_renderer_spec.rb b/spec/services/invoice_renderer_spec.rb index 87e654eb3f..d78f5e97a8 100644 --- a/spec/services/invoice_renderer_spec.rb +++ b/spec/services/invoice_renderer_spec.rb @@ -4,19 +4,46 @@ require 'spec_helper' describe InvoiceRenderer do let(:service) { described_class.new } - - it "creates a PDF invoice with two different templates" do + let(:order) do order = create(:completed_order_with_fees) order.bill_address = order.ship_address order.save! + order + end - result = service.render_to_string(order) - expect(result).to match /^%PDF/ + context 'when invoice_style2 is configured' do + before { allow(Spree::Config).to receive(:invoice_style2?).and_return(true) } - allow(Spree::Config).to receive(:invoice_style2?).and_return true + it 'uses the invoice2 template' do + renderer = instance_double(ApplicationController) + expect(renderer) + .to receive(:render_to_string) + .with(include(template: 'spree/admin/orders/invoice2')) - alternative = service.render_to_string(order) - expect(alternative).to match /^%PDF/ - expect(alternative).to_not eq result + described_class.new(renderer).render_to_string(order) + end + + it 'creates a PDF invoice' do + result = service.render_to_string(order) + expect(result).to match /^%PDF/ + end + end + + context 'when invoice_style2 is not configured' do + before { allow(Spree::Config).to receive(:invoice_style2?).and_return(false) } + + it 'uses the invoice template' do + renderer = instance_double(ApplicationController) + expect(renderer) + .to receive(:render_to_string) + .with(include(template: 'spree/admin/orders/invoice')) + + described_class.new(renderer).render_to_string(order) + end + + it 'creates a PDF invoice' do + result = service.render_to_string(order) + expect(result).to match /^%PDF/ + end end end From 85446c0ddef1b4fbd67e5e81d69e2d455e7e50cc Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Wed, 17 Mar 2021 11:28:05 +0100 Subject: [PATCH 60/61] Rearrange and cover balance in subs mailer specs This moves them to a more unit-like style where the mailer methods are the subjects. However, I did so only for the methods that show order balance and thus we want to be extra sure of their coverage. --- spec/mailers/subscription_mailer_spec.rb | 85 +++++++++++++++--------- 1 file changed, 53 insertions(+), 32 deletions(-) diff --git a/spec/mailers/subscription_mailer_spec.rb b/spec/mailers/subscription_mailer_spec.rb index f50f58b5e4..0cd77c047a 100644 --- a/spec/mailers/subscription_mailer_spec.rb +++ b/spec/mailers/subscription_mailer_spec.rb @@ -8,7 +8,10 @@ describe SubscriptionMailer, type: :mailer do before { setup_email } - describe "order placement" do + describe '#placement_email' do + subject(:email) { SubscriptionMailer.placement_email(order, changes) } + let(:changes) { {} } + let(:shop) { create(:enterprise) } let(:customer) { create(:customer, enterprise: shop) } let(:subscription) { create(:subscription, shop: shop, customer: customer, with_items: true) } @@ -16,31 +19,24 @@ describe SubscriptionMailer, type: :mailer do let!(:order) { proxy_order.initialise_order! } context "when changes have been made to the order" do - let(:changes) { {} } - - before do - changes[order.line_items.first.id] = 2 - expect do - SubscriptionMailer.placement_email(order, changes).deliver_now - end.to change{ SubscriptionMailer.deliveries.count }.by(1) - end + before { changes[order.line_items.first.id] = 2 } it "sends the email, which notifies the customer of changes made" do + expect { email.deliver_now }.to change { SubscriptionMailer.deliveries.count }.by(1) + body = SubscriptionMailer.deliveries.last.body.encoded + expect(body).to include "This order was automatically created for you." expect(body).to include "Unfortunately, not all products that you requested were available." end end context "and changes have not been made to the order" do - before do - expect do - SubscriptionMailer.placement_email(order, {}).deliver_now - end.to change{ SubscriptionMailer.deliveries.count }.by(1) - end - it "sends the email" do + expect { email.deliver_now }.to change { SubscriptionMailer.deliveries.count }.by(1) + body = SubscriptionMailer.deliveries.last.body.encoded + expect(body).to include "This order was automatically created for you." expect(body).to_not include "Unfortunately, not all products that you requested were available." end @@ -52,12 +48,9 @@ describe SubscriptionMailer, type: :mailer do let(:shop) { create(:enterprise, allow_order_changes: true) } - let(:email) { SubscriptionMailer.deliveries.last } - let(:body) { email.body.encoded } + let(:body) { SubscriptionMailer.deliveries.last.body.encoded } - before do - SubscriptionMailer.placement_email(order, {}).deliver_now - end + before { email.deliver_now } context "when the customer has a user account" do let(:customer) { create(:customer, enterprise: shop) } @@ -93,21 +86,36 @@ describe SubscriptionMailer, type: :mailer do end end end + + context 'when the order has outstanding balance' do + before { allow(order).to receive(:outstanding_balance) { 123 } } + + it 'renders the amount as money' do + expect(email.body).to include('$123') + end + end + + context 'when the order has no outstanding balance' do + before { allow(order).to receive(:outstanding_balance) { 0 } } + + it 'displays the payment status' do + expect(email.body).to include(I18n.t(:email_payment_not_paid)) + end + end end - describe "order confirmation" do + describe '#confirmation_email' do + subject(:email) { SubscriptionMailer.confirmation_email(order) } + let(:customer) { create(:customer) } let(:subscription) { create(:subscription, customer: customer, with_items: true) } let(:proxy_order) { create(:proxy_order, subscription: subscription) } let!(:order) { proxy_order.initialise_order! } - - before do - expect do - SubscriptionMailer.confirmation_email(order).deliver_now - end.to change{ SubscriptionMailer.deliveries.count }.by(1) - end + let(:user) { order.user } it "sends the email" do + expect { email.deliver_now }.to change{ SubscriptionMailer.deliveries.count }.by(1) + body = SubscriptionMailer.deliveries.last.body.encoded expect(body).to include "This order was automatically placed for you" end @@ -115,14 +123,11 @@ describe SubscriptionMailer, type: :mailer do describe "linking to order page" do let(:order_link_href) { "href=\"#{order_url(order)}\"" } - let(:email) { SubscriptionMailer.deliveries.last } - let(:body) { email.body.encoded } - context "when the customer has a user account" do let(:customer) { create(:customer) } it "provides link to view details" do - expect(body).to match /#{order_link_href}/ + expect(email.body.encoded).to include(order_url(order)) end end @@ -130,10 +135,26 @@ describe SubscriptionMailer, type: :mailer do let(:customer) { create(:customer, user: nil) } it "does not provide link" do - expect(body).to_not match /#{order_link_href}/ + expect(email.body).to_not match /#{order_link_href}/ end end end + + context 'when the order has outstanding balance' do + before { allow(order).to receive(:outstanding_balance) { 123 } } + + it 'renders the amount as money' do + expect(email.body).to include('$123') + end + end + + context 'when the order has no outstanding balance' do + before { allow(order).to receive(:outstanding_balance) { 0 } } + + it 'displays the payment status' do + expect(email.body).to include(I18n.t(:email_payment_not_paid)) + end + end end describe "empty order notification" do From 93bc60664ac9936ae913ba8ff46ce4fba1250d38 Mon Sep 17 00:00:00 2001 From: Pau Perez Date: Thu, 18 Mar 2021 10:00:33 +0100 Subject: [PATCH 61/61] Fix long lines --- .../spec/services/order_management/order/updater_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb index 50c95d5eba..47a92a520c 100644 --- a/engines/order_management/spec/services/order_management/order/updater_spec.rb +++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb @@ -202,8 +202,8 @@ module OrderManagement it "is credit_owed" do order.payment_total = 30 order.total = 30 - allow(order).to receive_message_chain(:payments, :valid, :empty?).and_return(false) - allow(order).to receive_message_chain(:payments, :completed, :empty?).and_return(false) + allow(order).to receive_message_chain(:payments, :valid, :empty?) { false } + allow(order).to receive_message_chain(:payments, :completed, :empty?) { false } expect { updater.update_payment_state @@ -215,8 +215,8 @@ module OrderManagement it "is void" do order.payment_total = 0 order.total = 30 - allow(order).to receive_message_chain(:payments, :valid, :empty?).and_return(false) - allow(order).to receive_message_chain(:payments, :completed, :empty?).and_return(false) + allow(order).to receive_message_chain(:payments, :valid, :empty?) { false } + allow(order).to receive_message_chain(:payments, :completed, :empty?) { false } expect { updater.update_payment_state