From 90bf4f312bdf624a38475bbacad639c91b836e19 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 12 Aug 2020 16:14:51 +1000 Subject: [PATCH 01/10] Document and spec current controller behaviour When we imported and merged Spree's controller modules with our decorators, Rails started using Spree's original code again. This was first included in v3.2.0 and deployed on 28 July 2020. --- lib/spree/core/controller_helpers/order.rb | 4 ++ spec/controllers/base_controller_spec.rb | 64 ++++++++++++++++++++++ 2 files changed, 68 insertions(+) diff --git a/lib/spree/core/controller_helpers/order.rb b/lib/spree/core/controller_helpers/order.rb index 1df01e286a..afa609e975 100644 --- a/lib/spree/core/controller_helpers/order.rb +++ b/lib/spree/core/controller_helpers/order.rb @@ -6,6 +6,10 @@ module Spree module Core module ControllerHelpers module Order + # WARNING: This code seems to be overridden by Spree's original in: + # spree_core/lib/spree/core/controller_helpers/order.rb + # + # None of our patches are active. def self.included(base) base.class_eval do helper_method :current_order diff --git a/spec/controllers/base_controller_spec.rb b/spec/controllers/base_controller_spec.rb index ee7dcb4bcc..70fce385cd 100644 --- a/spec/controllers/base_controller_spec.rb +++ b/spec/controllers/base_controller_spec.rb @@ -9,6 +9,70 @@ describe BaseController, type: :controller do end end + describe "#current_order" do + let(:user) { create(:user) } + + it "doesn't change anything without a user" do + expect { + get :index + }.to_not change { Spree::Order.count } + end + + it "creates a new order" do + allow(controller).to receive(:try_spree_current_user).and_return(user) + + expect { + get :index + }.to change { Spree::Order.count }.by(1) + + expect(user.orders.count).to eq 1 + end + + it "uses the last incomplete order" do + last_cart = create(:order, user: user, created_by: user, state: "cart", completed_at: nil) + allow(controller).to receive(:try_spree_current_user).and_return(user) + + expect { + get :index + }.to_not change { Spree::Order.count } + + expect(session[:order_id]).to eq last_cart.id + end + + # This behaviour comes from spree_core/lib/spree/core/controller_helpers/order.rb. + # It should be overridden by our lib/spree/core/controller_helpers/order.rb. + # But this test case proves that the original Spree logic is active. + # + # We originally overrode the Spree logic because merging orders from different + # shops was confusing. Incomplete orders would also re-appear after checkout, + # confusing as well. + # + # When we brought this code from Spree and merged it with our overrides, + # we accidentally started using Spree's version again. + it "merges the last incomplete order into the current one" do + last_cart = create(:order, user: user, created_by: user, state: "cart", completed_at: nil) + last_cart.line_items << create(:line_item) + + current_cart = create( + :order, + user: user, + created_by: user, + state: "cart", + completed_at: nil, + created_at: 1.week.ago + ) + session[:order_id] = current_cart.id + + allow(controller).to receive(:try_spree_current_user).and_return(user) + + expect { + get :index + }.to_not change { session[:order_id] } + + expect(current_cart.line_items.pluck(:id)).to match_array last_cart.line_items.map(&:id) + end + end + it "redirects to home with message if order cycle is expired" do expect(controller).to receive(:current_order_cycle).and_return(oc).twice expect(controller).to receive(:current_order).and_return(order).twice From 57610142053f61568a87f8d3a6cc914362c3d3cc Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 13 Aug 2020 16:57:02 +1000 Subject: [PATCH 02/10] Restore Spree customisations for controllers --- app/controllers/base_controller.rb | 4 ++++ lib/spree/core/controller_helpers/order.rb | 4 ---- spec/controllers/base_controller_spec.rb | 22 ++++++---------------- 3 files changed, 10 insertions(+), 20 deletions(-) diff --git a/app/controllers/base_controller.rb b/app/controllers/base_controller.rb index 8fcdac8fe4..9c364e9e9d 100644 --- a/app/controllers/base_controller.rb +++ b/app/controllers/base_controller.rb @@ -1,3 +1,7 @@ +require 'spree/core/controller_helpers' +require 'spree/core/controller_helpers/auth' +require 'spree/core/controller_helpers/common' +require 'spree/core/controller_helpers/order' require 'spree/core/controller_helpers/respond_with' require 'open_food_network/tag_rule_applicator' diff --git a/lib/spree/core/controller_helpers/order.rb b/lib/spree/core/controller_helpers/order.rb index afa609e975..1df01e286a 100644 --- a/lib/spree/core/controller_helpers/order.rb +++ b/lib/spree/core/controller_helpers/order.rb @@ -6,10 +6,6 @@ module Spree module Core module ControllerHelpers module Order - # WARNING: This code seems to be overridden by Spree's original in: - # spree_core/lib/spree/core/controller_helpers/order.rb - # - # None of our patches are active. def self.included(base) base.class_eval do helper_method :current_order diff --git a/spec/controllers/base_controller_spec.rb b/spec/controllers/base_controller_spec.rb index 70fce385cd..145f0c7cf8 100644 --- a/spec/controllers/base_controller_spec.rb +++ b/spec/controllers/base_controller_spec.rb @@ -19,7 +19,7 @@ describe BaseController, type: :controller do end it "creates a new order" do - allow(controller).to receive(:try_spree_current_user).and_return(user) + allow(controller).to receive(:spree_current_user).and_return(user) expect { get :index @@ -30,7 +30,7 @@ describe BaseController, type: :controller do it "uses the last incomplete order" do last_cart = create(:order, user: user, created_by: user, state: "cart", completed_at: nil) - allow(controller).to receive(:try_spree_current_user).and_return(user) + allow(controller).to receive(:spree_current_user).and_return(user) expect { get :index @@ -39,17 +39,7 @@ describe BaseController, type: :controller do expect(session[:order_id]).to eq last_cart.id end - # This behaviour comes from spree_core/lib/spree/core/controller_helpers/order.rb. - # It should be overridden by our lib/spree/core/controller_helpers/order.rb. - # But this test case proves that the original Spree logic is active. - # - # We originally overrode the Spree logic because merging orders from different - # shops was confusing. Incomplete orders would also re-appear after checkout, - # confusing as well. - # - # When we brought this code from Spree and merged it with our overrides, - # we accidentally started using Spree's version again. - it "merges the last incomplete order into the current one" do + it "deletes the last incomplete order" do last_cart = create(:order, user: user, created_by: user, state: "cart", completed_at: nil) last_cart.line_items << create(:line_item) @@ -63,13 +53,13 @@ describe BaseController, type: :controller do ) session[:order_id] = current_cart.id - allow(controller).to receive(:try_spree_current_user).and_return(user) + allow(controller).to receive(:spree_current_user).and_return(user) expect { get :index - }.to_not change { session[:order_id] } + }.to change { Spree::Order.count }.by(-1) - expect(current_cart.line_items.pluck(:id)).to match_array last_cart.line_items.map(&:id) + expect(current_cart.line_items.count).to eq 0 end end From e8139d394890f3ec60c861726886953c9e341e7f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Aug 2020 10:02:48 +1000 Subject: [PATCH 03/10] Keep old incomplete (cart) orders We used to delete old cart orders so that they wouldn't re-appear after a successful checkout of another order. Keeping them ensures that we don't remove an order that is still used by another device. It also makes sure that we keep references of failed payments. --- lib/spree/core/controller_helpers/order.rb | 10 +++---- spec/controllers/base_controller_spec.rb | 34 ++++++++++++++++++++-- 2 files changed, 36 insertions(+), 8 deletions(-) diff --git a/lib/spree/core/controller_helpers/order.rb b/lib/spree/core/controller_helpers/order.rb index 1df01e286a..e95499bef2 100644 --- a/lib/spree/core/controller_helpers/order.rb +++ b/lib/spree/core/controller_helpers/order.rb @@ -68,8 +68,7 @@ module Spree session[:guest_token] = nil end - # Do not attempt to merge incomplete and current orders. - # Instead, destroy the incomplete orders. + # Recover incomplete orders from other sessions after logging in. def set_current_order return unless (user = spree_current_user) @@ -77,11 +76,10 @@ module Spree if session[:order_id].nil? && last_incomplete_order session[:order_id] = last_incomplete_order.id - elsif current_order(true) && - last_incomplete_order && - current_order != last_incomplete_order - last_incomplete_order.destroy end + + # Load current order and create a new one if necessary. + current_order(true) end def current_currency diff --git a/spec/controllers/base_controller_spec.rb b/spec/controllers/base_controller_spec.rb index 145f0c7cf8..fb1d399335 100644 --- a/spec/controllers/base_controller_spec.rb +++ b/spec/controllers/base_controller_spec.rb @@ -39,7 +39,11 @@ describe BaseController, type: :controller do expect(session[:order_id]).to eq last_cart.id end - it "deletes the last incomplete order" do + it "ignores the last incomplete order" do + # Spree used to merge the last order with the current one. + # And we used to override that logic to delete old incomplete orders. + # Now we are checking here that none of that is happening. + last_cart = create(:order, user: user, created_by: user, state: "cart", completed_at: nil) last_cart.line_items << create(:line_item) @@ -57,10 +61,36 @@ describe BaseController, type: :controller do expect { get :index - }.to change { Spree::Order.count }.by(-1) + }.to_not change { Spree::Order.count } expect(current_cart.line_items.count).to eq 0 end + + it "doesn't recover old orders after checkout, a new empty one is created" do + last_cart = create(:order, user: user, created_by: user, state: "cart", completed_at: nil) + last_cart.line_items << create(:line_item) + + just_completed_order = create( + :order, + user: user, + created_by: user, + state: "complete", + completed_at: Time.zone.now, + created_at: 1.week.ago + ) + expect(just_completed_order.completed_at).to be_present + session[:order_id] = just_completed_order.id + + allow(controller).to receive(:spree_current_user).and_return(user) + + expect { + get :index + }.to change { Spree::Order.count }.by(1) + + expect(session[:order_id]).to_not eq just_completed_order.id + expect(session[:order_id]).to_not eq last_cart.id + expect(controller.current_order.line_items.count).to eq 0 + end end it "redirects to home with message if order cycle is expired" do From b79c568b08268de1aa89a728d980c2ca63fef5cb Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Aug 2020 10:34:50 +1000 Subject: [PATCH 04/10] Load our spree overrides instead of the originals We changed some of Spree's logic and want to use that. And once we remove the spree_core gem, we need to load those files before using them. --- app/controllers/api/base_controller.rb | 1 + app/controllers/spree/user_passwords_controller.rb | 7 +++++++ app/controllers/spree/user_registrations_controller.rb | 7 +++++++ app/controllers/spree/user_sessions_controller.rb | 7 +++++++ 4 files changed, 22 insertions(+) diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index 423a4fb104..e9d82d19ad 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -1,5 +1,6 @@ # Base controller for OFN's API require_dependency 'spree/api/controller_setup' +require "spree/core/controller_helpers/ssl" module Api class BaseController < ActionController::Metal diff --git a/app/controllers/spree/user_passwords_controller.rb b/app/controllers/spree/user_passwords_controller.rb index bf8b13d379..4e7fc6e824 100644 --- a/app/controllers/spree/user_passwords_controller.rb +++ b/app/controllers/spree/user_passwords_controller.rb @@ -1,3 +1,10 @@ +# frozen_string_literal: true + +require "spree/core/controller_helpers/auth" +require "spree/core/controller_helpers/common" +require "spree/core/controller_helpers/order" +require "spree/core/controller_helpers/ssl" + module Spree class UserPasswordsController < Devise::PasswordsController helper 'spree/base', 'spree/store' diff --git a/app/controllers/spree/user_registrations_controller.rb b/app/controllers/spree/user_registrations_controller.rb index 98ad3c8d1c..1a2d740da7 100644 --- a/app/controllers/spree/user_registrations_controller.rb +++ b/app/controllers/spree/user_registrations_controller.rb @@ -1,3 +1,10 @@ +# frozen_string_literal: true + +require "spree/core/controller_helpers/auth" +require "spree/core/controller_helpers/common" +require "spree/core/controller_helpers/order" +require "spree/core/controller_helpers/ssl" + module Spree class UserRegistrationsController < Devise::RegistrationsController helper 'spree/base', 'spree/store' diff --git a/app/controllers/spree/user_sessions_controller.rb b/app/controllers/spree/user_sessions_controller.rb index 7ff26a3437..c17eafb892 100644 --- a/app/controllers/spree/user_sessions_controller.rb +++ b/app/controllers/spree/user_sessions_controller.rb @@ -1,3 +1,10 @@ +# frozen_string_literal: true + +require "spree/core/controller_helpers/auth" +require "spree/core/controller_helpers/common" +require "spree/core/controller_helpers/order" +require "spree/core/controller_helpers/ssl" + module Spree class UserSessionsController < Devise::SessionsController helper 'spree/base', 'spree/store' From 0a1947ae340f2dcee4826214b5c2b6c0a19a3887 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Aug 2020 10:56:20 +1000 Subject: [PATCH 05/10] Remove unused module from lib I was looking for library files that may be used but are not loaded. I would then add the missing `require` statements. But I found that this module isn't used any more. Usage removed in: 310d1b37260f49588c4302eaf529d5834005adb9 --- .rubocop_todo.yml | 1 - lib/open_food_network/model_class_from_controller_name.rb | 8 -------- 2 files changed, 9 deletions(-) delete mode 100644 lib/open_food_network/model_class_from_controller_name.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index e95d9e89c6..21d7bd8851 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1037,7 +1037,6 @@ Style/FrozenStringLiteralComment: - 'lib/open_food_network/i18n_config.rb' - 'lib/open_food_network/lettuce_share_report.rb' - 'lib/open_food_network/locking.rb' - - 'lib/open_food_network/model_class_from_controller_name.rb' - 'lib/open_food_network/order_and_distributor_report.rb' - 'lib/open_food_network/order_cycle_form_applicator.rb' - 'lib/open_food_network/order_cycle_management_report.rb' diff --git a/lib/open_food_network/model_class_from_controller_name.rb b/lib/open_food_network/model_class_from_controller_name.rb deleted file mode 100644 index 97b92a18d7..0000000000 --- a/lib/open_food_network/model_class_from_controller_name.rb +++ /dev/null @@ -1,8 +0,0 @@ -module OpenFoodNetwork - module ModelClassFromControllerName - # Equivalent to CanCan's "authorize_resource :class => false" (see https://github.com/ryanb/cancan/blob/master/lib/cancan/controller_resource.rb#L146) - def model_class - self.class.to_s.sub("Controller", "").underscore.split('/').last.singularize.to_sym - end - end -end From c3e0f45f1a49b2c68e508d5a0fb4f07bfb734afd Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Aug 2020 11:20:05 +1000 Subject: [PATCH 06/10] Remove unused Report class from lib Also removing related unused classes and their specs. --- .rubocop_todo.yml | 12 ------- knapsack_rspec_report.json | 3 -- lib/open_food_network/reports/report.rb | 35 ------------------- lib/open_food_network/reports/row.rb | 15 -------- lib/open_food_network/reports/rule.rb | 21 ----------- .../orders_and_fulfillments_report_spec.rb | 1 + .../open_food_network/reports/report_spec.rb | 15 -------- .../lib/open_food_network/reports/row_spec.rb | 19 ---------- .../open_food_network/reports/rule_spec.rb | 21 ----------- 9 files changed, 1 insertion(+), 141 deletions(-) delete mode 100644 lib/open_food_network/reports/report.rb delete mode 100644 lib/open_food_network/reports/row.rb delete mode 100644 lib/open_food_network/reports/rule.rb delete mode 100644 spec/lib/open_food_network/reports/report_spec.rb delete mode 100644 spec/lib/open_food_network/reports/row_spec.rb delete mode 100644 spec/lib/open_food_network/reports/rule_spec.rb diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 21d7bd8851..aec1f024b9 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -600,13 +600,7 @@ Style/ClassAndModuleChildren: - 'app/serializers/api/taxon_serializer.rb' - 'app/serializers/api/variant_serializer.rb' - 'lib/open_food_network/locking.rb' - - 'lib/open_food_network/reports/report.rb' - - 'lib/open_food_network/reports/row.rb' - - 'lib/open_food_network/reports/rule.rb' - 'spec/controllers/spree/admin/base_controller_spec.rb' - - 'spec/lib/open_food_network/reports/report_spec.rb' - - 'spec/lib/open_food_network/reports/row_spec.rb' - - 'spec/lib/open_food_network/reports/rule_spec.rb' # Offense count: 2 Style/ClassVars: @@ -1060,9 +1054,6 @@ Style/FrozenStringLiteralComment: - 'lib/open_food_network/referer_parser.rb' - 'lib/open_food_network/reports/line_items.rb' - 'lib/open_food_network/reports/list.rb' - - 'lib/open_food_network/reports/report.rb' - - 'lib/open_food_network/reports/row.rb' - - 'lib/open_food_network/reports/rule.rb' - 'lib/open_food_network/sales_tax_report.rb' - 'lib/open_food_network/scope_product_to_hub.rb' - 'lib/open_food_network/scope_variant_to_hub.rb' @@ -1293,9 +1284,6 @@ Style/FrozenStringLiteralComment: - 'spec/lib/open_food_network/products_and_inventory_report_spec.rb' - 'spec/lib/open_food_network/property_merge_spec.rb' - 'spec/lib/open_food_network/referer_parser_spec.rb' - - 'spec/lib/open_food_network/reports/report_spec.rb' - - 'spec/lib/open_food_network/reports/row_spec.rb' - - 'spec/lib/open_food_network/reports/rule_spec.rb' - 'spec/lib/open_food_network/sales_tax_report_spec.rb' - 'spec/lib/open_food_network/scope_variant_to_hub_spec.rb' - 'spec/lib/open_food_network/scope_variants_to_search_spec.rb' diff --git a/knapsack_rspec_report.json b/knapsack_rspec_report.json index 91499ba40e..f745a4438d 100644 --- a/knapsack_rspec_report.json +++ b/knapsack_rspec_report.json @@ -113,7 +113,6 @@ "spec/models/subscription_line_item_spec.rb": 0.021193265914916992, "spec/controllers/api/statuses_controller_spec.rb": 0.02451467514038086, "spec/lib/open_food_network/referer_parser_spec.rb": 0.015799283981323242, - "spec/lib/open_food_network/reports/rule_spec.rb": 0.01628732681274414, "spec/helpers/serializer_helper_spec.rb": 0.004682064056396484, "spec/jobs/heartbeat_job_spec.rb": 0.013271570205688477, "spec/services/mail_configuration_spec.rb": 0.01050567626953125, @@ -246,7 +245,6 @@ "spec/validators/integer_array_validator_spec.rb": 0.04994392395019531, "spec/validators/date_time_string_validator_spec.rb": 0.05316734313964844, "spec/models/product_import/reset_absent_spec.rb": 0.04071307182312012, - "spec/lib/open_food_network/reports/report_spec.rb": 0.04329681396484375, "spec/jobs/confirm_signup_job_spec.rb": 0.03060293197631836, "spec/services/order_cycle_distributed_variants_spec.rb": 0.02808237075805664, "spec/lib/open_food_network/feature_toggle_spec.rb": 0.0240786075592041, @@ -255,7 +253,6 @@ "spec/models/spree/calculator/flat_rate_spec.rb": 0.014808177947998047, "spec/models/spree/image_spec.rb": 0.014715909957885742, "spec/helpers/spree/admin/base_helper_spec.rb": 0.008042573928833008, - "spec/lib/open_food_network/reports/row_spec.rb": 0.005358695983886719, "engines/order_management/spec/controllers/order_management/reports/enterprise_fee_summaries_controller_spec.rb": 0.7100100517272949, "engines/order_management/spec/services/order_management/reports/enterprise_fee_summary/parameters_spec.rb": 4.190261602401733, "engines/order_management/spec/services/order_management/reports/enterprise_fee_summary/report_data/enterprise_fee_type_total_spec.rb": 0.006247282028198242, diff --git a/lib/open_food_network/reports/report.rb b/lib/open_food_network/reports/report.rb deleted file mode 100644 index 2c4e5900f2..0000000000 --- a/lib/open_food_network/reports/report.rb +++ /dev/null @@ -1,35 +0,0 @@ -require 'open_food_network/reports/row' -require 'open_food_network/reports/rule' - -module OpenFoodNetwork::Reports - class Report - class_attribute :_header, :_columns, :_rules_head - - # -- API - def header - _header - end - - def columns - _columns.to_a - end - - def rules - # Flatten linked list and return as hashes - rules = [] - - rule = _rules_head - while rule - rules << rule - rule = rule.next - end - - rules.map(&:to_h) - end - - # -- DSL - def self.header(*columns) - self._header = columns - end - end -end diff --git a/lib/open_food_network/reports/row.rb b/lib/open_food_network/reports/row.rb deleted file mode 100644 index 35b4bffe33..0000000000 --- a/lib/open_food_network/reports/row.rb +++ /dev/null @@ -1,15 +0,0 @@ -module OpenFoodNetwork::Reports - class Row - def initialize - @columns = [] - end - - def column(&block) - @columns << block - end - - def to_a - @columns - end - end -end diff --git a/lib/open_food_network/reports/rule.rb b/lib/open_food_network/reports/rule.rb deleted file mode 100644 index cfb8b7e67f..0000000000 --- a/lib/open_food_network/reports/rule.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'open_food_network/reports/row' - -module OpenFoodNetwork::Reports - class Rule - attr_reader :next - - def group(&block) - @group = block - end - - def sort(&block) - @sort = block - end - - def to_h - h = { group_by: @group, sort_by: @sort } - h[:summary_columns] = @summary_row.to_a if @summary_row - h - end - end -end diff --git a/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb b/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb index af91f3a917..f970c4b83a 100644 --- a/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb +++ b/spec/lib/open_food_network/orders_and_fulfillments_report_spec.rb @@ -1,5 +1,6 @@ require 'spec_helper' require 'open_food_network/orders_and_fulfillments_report' +require 'open_food_network/order_grouper' describe OpenFoodNetwork::OrdersAndFulfillmentsReport do include AuthenticationHelper diff --git a/spec/lib/open_food_network/reports/report_spec.rb b/spec/lib/open_food_network/reports/report_spec.rb deleted file mode 100644 index 86e76bcec5..0000000000 --- a/spec/lib/open_food_network/reports/report_spec.rb +++ /dev/null @@ -1,15 +0,0 @@ -require 'open_food_network/reports/report' - -module OpenFoodNetwork::Reports - class TestReport < Report - header 'One', 'Two', 'Three', 'Four' - end - - describe Report do - let(:report) { TestReport.new } - - it "returns the header" do - expect(report.header).to eq(%w(One Two Three Four)) - end - end -end diff --git a/spec/lib/open_food_network/reports/row_spec.rb b/spec/lib/open_food_network/reports/row_spec.rb deleted file mode 100644 index c541832edf..0000000000 --- a/spec/lib/open_food_network/reports/row_spec.rb +++ /dev/null @@ -1,19 +0,0 @@ -require 'spec_helper' -require 'open_food_network/reports/row' - -module OpenFoodNetwork::Reports - describe Row do - let(:row) { Row.new } - # rubocop:disable Style/Proc - let(:proc) { Proc.new {} } - # rubocop:enable Style/Proc - - it "can define a number of columns and return them as an array" do - row.column(&proc) - row.column(&proc) - row.column(&proc) - - expect(row.to_a).to eq([proc, proc, proc]) - end - end -end diff --git a/spec/lib/open_food_network/reports/rule_spec.rb b/spec/lib/open_food_network/reports/rule_spec.rb deleted file mode 100644 index eae5b26045..0000000000 --- a/spec/lib/open_food_network/reports/rule_spec.rb +++ /dev/null @@ -1,21 +0,0 @@ -require 'spec_helper' -require 'open_food_network/reports/rule' - -module OpenFoodNetwork::Reports - describe Rule do - let(:rule) { Rule.new } - # rubocop:disable Style/Proc - let(:proc) { Proc.new {} } - # rubocop:enable Style/Proc - - it "can define a group proc and return it in a hash" do - rule.group(&proc) - expect(rule.to_h).to eq(group_by: proc, sort_by: nil) - end - - it "can define a sort proc and return it in a hash" do - rule.sort(&proc) - expect(rule.to_h).to eq(group_by: nil, sort_by: proc) - end - end -end From 23706ec1d6a1bcf4aa5022e72bc7ddc5477f4eac Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Fri, 14 Aug 2020 13:01:51 +1000 Subject: [PATCH 07/10] Load our version of the Spree environment We didn't actually change any logic in our version of the Spree environment file but if we do that in the future, we want to be sure that it takes effect. Our file was ignored and not loaded before. --- config/initializers/spree.rb | 1 + spec/lib/spree/core/environment_spec.rb | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) create mode 100644 spec/lib/spree/core/environment_spec.rb diff --git a/config/initializers/spree.rb b/config/initializers/spree.rb index 6ff230c5f5..2e80f978ae 100644 --- a/config/initializers/spree.rb +++ b/config/initializers/spree.rb @@ -6,6 +6,7 @@ # In order to initialize a setting do: # config.setting_name = 'new value' +require "spree/core/environment" require 'spree/product_filters' # Due to a bug in ActiveRecord we need to load the tagging code in Gateway which diff --git a/spec/lib/spree/core/environment_spec.rb b/spec/lib/spree/core/environment_spec.rb new file mode 100644 index 0000000000..7eec8dc329 --- /dev/null +++ b/spec/lib/spree/core/environment_spec.rb @@ -0,0 +1,19 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Spree::Core::Environment do + # Our version doesn't add any features we could test. + # So we just check that our file is loaded correctly. + let(:our_file) { Rails.root.join("lib/spree/core/environment.rb").to_s } + + it "is defined in our code" do + file = subject.method(:initialize).source_location.first + expect(file).to eq our_file + end + + it "used by Spree" do + file = Spree::Core::Engine.config.spree.method(:initialize).source_location.first + expect(file).to eq our_file + end +end From bb3f958dd2cd907195f5702dbb93e5e6db119f4c Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 19 Aug 2020 10:37:18 +1000 Subject: [PATCH 08/10] Remove redundant includes --- app/controllers/base_controller.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/controllers/base_controller.rb b/app/controllers/base_controller.rb index 9c364e9e9d..b039a2447d 100644 --- a/app/controllers/base_controller.rb +++ b/app/controllers/base_controller.rb @@ -1,4 +1,3 @@ -require 'spree/core/controller_helpers' require 'spree/core/controller_helpers/auth' require 'spree/core/controller_helpers/common' require 'spree/core/controller_helpers/order' @@ -6,7 +5,6 @@ require 'spree/core/controller_helpers/respond_with' require 'open_food_network/tag_rule_applicator' class BaseController < ApplicationController - include Spree::Core::ControllerHelpers include Spree::Core::ControllerHelpers::Auth include Spree::Core::ControllerHelpers::Common include Spree::Core::ControllerHelpers::Order From 355c5f5c55ceffce9ffb958ea72be8f33eb89e97 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Wed, 19 Aug 2020 10:47:13 +1000 Subject: [PATCH 09/10] Remove unreachable order recovery code Every page load creates a cart order if none is present. So when a user logs in, they always have an order stored in their session. And therefore, we never got to recover an old order. We could have fixed the code to restore old orders. But as far as I can tell, order recovery hasn't been working for years and I couldn't find any issue requesting this feature. If we wanted to implement order recovery, it should probably be designed more carefully and included in the `current_order` method. --- lib/spree/core/controller_helpers/order.rb | 13 ++----------- spec/controllers/base_controller_spec.rb | 11 ----------- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/lib/spree/core/controller_helpers/order.rb b/lib/spree/core/controller_helpers/order.rb index e95499bef2..fe99381c7f 100644 --- a/lib/spree/core/controller_helpers/order.rb +++ b/lib/spree/core/controller_helpers/order.rb @@ -68,18 +68,9 @@ module Spree session[:guest_token] = nil end - # Recover incomplete orders from other sessions after logging in. + # Load current order and create a new one if necessary. def set_current_order - return unless (user = spree_current_user) - - last_incomplete_order = user.last_incomplete_spree_order - - if session[:order_id].nil? && last_incomplete_order - session[:order_id] = last_incomplete_order.id - end - - # Load current order and create a new one if necessary. - current_order(true) + current_order(true) if spree_current_user end def current_currency diff --git a/spec/controllers/base_controller_spec.rb b/spec/controllers/base_controller_spec.rb index fb1d399335..e68e10e0c2 100644 --- a/spec/controllers/base_controller_spec.rb +++ b/spec/controllers/base_controller_spec.rb @@ -28,17 +28,6 @@ describe BaseController, type: :controller do expect(user.orders.count).to eq 1 end - it "uses the last incomplete order" do - last_cart = create(:order, user: user, created_by: user, state: "cart", completed_at: nil) - allow(controller).to receive(:spree_current_user).and_return(user) - - expect { - get :index - }.to_not change { Spree::Order.count } - - expect(session[:order_id]).to eq last_cart.id - end - it "ignores the last incomplete order" do # Spree used to merge the last order with the current one. # And we used to override that logic to delete old incomplete orders. From 72f5b1b251c62df5b2a2ab71860204d3380dbed4 Mon Sep 17 00:00:00 2001 From: Luis Ramos Date: Wed, 19 Aug 2020 17:36:36 +0100 Subject: [PATCH 10/10] Revert "Remove unreachable order recovery code" This reverts commit 355c5f5c55ceffce9ffb958ea72be8f33eb89e97. This code is necessary to preserver cart contents across logins on different browser sessions. --- lib/spree/core/controller_helpers/order.rb | 13 +++++++++++-- spec/controllers/base_controller_spec.rb | 11 +++++++++++ 2 files changed, 22 insertions(+), 2 deletions(-) diff --git a/lib/spree/core/controller_helpers/order.rb b/lib/spree/core/controller_helpers/order.rb index fe99381c7f..e95499bef2 100644 --- a/lib/spree/core/controller_helpers/order.rb +++ b/lib/spree/core/controller_helpers/order.rb @@ -68,9 +68,18 @@ module Spree session[:guest_token] = nil end - # Load current order and create a new one if necessary. + # Recover incomplete orders from other sessions after logging in. def set_current_order - current_order(true) if spree_current_user + return unless (user = spree_current_user) + + last_incomplete_order = user.last_incomplete_spree_order + + if session[:order_id].nil? && last_incomplete_order + session[:order_id] = last_incomplete_order.id + end + + # Load current order and create a new one if necessary. + current_order(true) end def current_currency diff --git a/spec/controllers/base_controller_spec.rb b/spec/controllers/base_controller_spec.rb index e68e10e0c2..fb1d399335 100644 --- a/spec/controllers/base_controller_spec.rb +++ b/spec/controllers/base_controller_spec.rb @@ -28,6 +28,17 @@ describe BaseController, type: :controller do expect(user.orders.count).to eq 1 end + it "uses the last incomplete order" do + last_cart = create(:order, user: user, created_by: user, state: "cart", completed_at: nil) + allow(controller).to receive(:spree_current_user).and_return(user) + + expect { + get :index + }.to_not change { Spree::Order.count } + + expect(session[:order_id]).to eq last_cart.id + end + it "ignores the last incomplete order" do # Spree used to merge the last order with the current one. # And we used to override that logic to delete old incomplete orders.