From 7c562d321dcbb81c4c9a42af0a885363fe53720a Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 25 Oct 2022 16:13:35 +1100 Subject: [PATCH 1/5] Activate DfcProvider with feature toggle, not prod We want to test in dev and on staging, too. --- config/routes.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/routes.rb b/config/routes.rb index cf7e544f5a..9c449ed11d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -117,7 +117,7 @@ Openfoodnetwork::Application.routes.draw do get 'sitemap.xml', to: 'sitemap#index', defaults: { format: 'xml' } - unless Rails.env.production? + constraints FeatureToggleConstraint.new(:dfc_provider) do # Mount DFC API endpoints mount DfcProvider::Engine, at: '/' end From 6aefc23c355ba1615903915a090a47339fe9f590 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 25 Oct 2022 16:14:54 +1100 Subject: [PATCH 2/5] Allow devs to lazy-load app with right dependency --- app/constraints/feature_toggle_constraint.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/constraints/feature_toggle_constraint.rb b/app/constraints/feature_toggle_constraint.rb index cd7070eed8..1393b007fe 100644 --- a/app/constraints/feature_toggle_constraint.rb +++ b/app/constraints/feature_toggle_constraint.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "open_food_network/feature_toggle" + class FeatureToggleConstraint def initialize(feature_name) @feature = feature_name From 36c44a548799ccdaddd1f3848110cc240fb96f29 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 27 Oct 2022 15:07:04 +1100 Subject: [PATCH 3/5] Allow FeatureToggleConstraint to run w/o warden Routing specs and controller specs don't set warden on the request environment. While we could try to re-implement Warden/Devise logic here, it's much easier to not rely on the environment to be set. While I'm usually opposed to changing production code to make testing easier, it does avoid a lot of complication in this case. And maybe it would be handy in other circumstances to have a more defensive approach to checking the logged in user. --- app/constraints/feature_toggle_constraint.rb | 2 +- spec/routing/stripe_spec.rb | 4 ---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/app/constraints/feature_toggle_constraint.rb b/app/constraints/feature_toggle_constraint.rb index 1393b007fe..cae850574d 100644 --- a/app/constraints/feature_toggle_constraint.rb +++ b/app/constraints/feature_toggle_constraint.rb @@ -12,6 +12,6 @@ class FeatureToggleConstraint end def current_user(request) - request.env['warden'].user + request.env['warden']&.user end end diff --git a/spec/routing/stripe_spec.rb b/spec/routing/stripe_spec.rb index 54dd76bde2..9ce5ce8c6c 100644 --- a/spec/routing/stripe_spec.rb +++ b/spec/routing/stripe_spec.rb @@ -3,10 +3,6 @@ require "spec_helper" describe "routing for Stripe return URLS", type: :routing do - before do - allow_any_instance_of(FeatureToggleConstraint).to receive(:current_user) { build(:user) } - end - context "checkout return URLs" do it "routes /checkout to checkout#edit" do expect(get: "checkout"). From f9246f19170cf10f4dc5eeb37211920ef4eb59d8 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 27 Oct 2022 15:25:33 +1100 Subject: [PATCH 4/5] Remove unused/broken feature toggle helper --- spec/base_spec_helper.rb | 1 - spec/support/feature_toggle_helper.rb | 11 ----------- 2 files changed, 12 deletions(-) delete mode 100644 spec/support/feature_toggle_helper.rb diff --git a/spec/base_spec_helper.rb b/spec/base_spec_helper.rb index 376299828a..82e0da623c 100644 --- a/spec/base_spec_helper.rb +++ b/spec/base_spec_helper.rb @@ -125,7 +125,6 @@ RSpec.configure do |config| config.include Spree::UrlHelpers config.include Spree::MoneyHelper config.include PreferencesHelper - config.include OpenFoodNetwork::FeatureToggleHelper config.include OpenFoodNetwork::FiltersHelper config.include OpenFoodNetwork::EnterpriseGroupsHelper config.include OpenFoodNetwork::ProductsHelper diff --git a/spec/support/feature_toggle_helper.rb b/spec/support/feature_toggle_helper.rb deleted file mode 100644 index a817e25049..0000000000 --- a/spec/support/feature_toggle_helper.rb +++ /dev/null @@ -1,11 +0,0 @@ -# frozen_string_literal: true - -module OpenFoodNetwork - module FeatureToggleHelper - def set_feature_toggle(feature, status) - features = OpenFoodNetwork::FeatureToggle.features - features[feature] = status - allow(OpenFoodNetwork::FeatureToggle).to receive(:features) { features } - end - end -end From 56667f61427edded589a0279d94b6325b0e1fa3f Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 27 Oct 2022 15:32:05 +1100 Subject: [PATCH 5/5] Simplify test setup with enabled features If a feature is activated or not depends on the database which is reset after each test scenario. So enabling a feature doesn't leak into other scenarios. Just enabling the feature is less code and more realistic than mocking a method call. --- .../payment_gateways/stripe_controller_spec.rb | 5 +---- spec/controllers/split_checkout_controller_spec.rb | 5 +---- spec/system/consumer/shopping/checkout_auth_spec.rb | 3 +-- spec/system/consumer/split_checkout_spec.rb | 3 +-- spec/system/consumer/split_checkout_tax_incl_spec.rb | 10 ++-------- .../consumer/split_checkout_tax_not_incl_spec.rb | 10 ++-------- 6 files changed, 8 insertions(+), 28 deletions(-) diff --git a/spec/controllers/payment_gateways/stripe_controller_spec.rb b/spec/controllers/payment_gateways/stripe_controller_spec.rb index 60f95abd39..3644ccb70a 100644 --- a/spec/controllers/payment_gateways/stripe_controller_spec.rb +++ b/spec/controllers/payment_gateways/stripe_controller_spec.rb @@ -88,10 +88,7 @@ module PaymentGateways context "using split checkout" do before do - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout) { true } - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout, anything) { true } + Flipper.enable(:split_checkout) order.update_attribute :state, "confirmation" end diff --git a/spec/controllers/split_checkout_controller_spec.rb b/spec/controllers/split_checkout_controller_spec.rb index 1b3e71c73a..40c1f0b354 100644 --- a/spec/controllers/split_checkout_controller_spec.rb +++ b/spec/controllers/split_checkout_controller_spec.rb @@ -16,10 +16,7 @@ describe SplitCheckoutController, type: :controller do let(:shipping_method) { distributor.shipping_methods.first } before do - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout) { true } - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout, anything) { true } + Flipper.enable(:split_checkout) exchange.variants << order.line_items.first.variant allow(controller).to receive(:current_order) { order } diff --git a/spec/system/consumer/shopping/checkout_auth_spec.rb b/spec/system/consumer/shopping/checkout_auth_spec.rb index 389c5a4eaf..d9aa53581a 100644 --- a/spec/system/consumer/shopping/checkout_auth_spec.rb +++ b/spec/system/consumer/shopping/checkout_auth_spec.rb @@ -83,8 +83,7 @@ describe "As a consumer I want to check out my cart", js: true do context "split checkout" do before do - allow(OpenFoodNetwork::FeatureToggle).to receive(:enabled?).with(:split_checkout) { true } - allow(OpenFoodNetwork::FeatureToggle).to receive(:enabled?).with(:split_checkout, anything) { true } + Flipper.enable(:split_checkout) end include_examples "with different checkout types", "split_checkout" end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index b5e21b83ce..038a2667ca 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -65,8 +65,7 @@ describe "As a consumer, I want to checkout my order", js: true do } before do - allow(OpenFoodNetwork::FeatureToggle).to receive(:enabled?).with(:split_checkout).and_return(true) - allow(OpenFoodNetwork::FeatureToggle).to receive(:enabled?).with(:split_checkout, anything).and_return(true) + Flipper.enable(:split_checkout) add_enterprise_fee enterprise_fee set_order order diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb index 96199c4a2a..146104e824 100644 --- a/spec/system/consumer/split_checkout_tax_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb @@ -111,10 +111,7 @@ describe "As a consumer, I want to see adjustment breakdown" do context "on split-checkout" do before do - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout).and_return(true) - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout, anything).and_return(true) + Flipper.enable(:split_checkout) set_order order_within_zone login_as(user_within_zone) @@ -171,10 +168,7 @@ describe "As a consumer, I want to see adjustment breakdown" do context "on split-checkout" do before do - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout).and_return(true) - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout, anything).and_return(true) + Flipper.enable(:split_checkout) set_order order_outside_zone login_as(user_outside_zone) diff --git a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb index ac376980a7..340063c0fd 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -118,10 +118,7 @@ describe "As a consumer, I want to see adjustment breakdown" do context "on split-checkout" do before do - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout).and_return(true) - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout, anything).and_return(true) + Flipper.enable(:split_checkout) set_order order_within_zone login_as(user_within_zone) @@ -181,10 +178,7 @@ describe "As a consumer, I want to see adjustment breakdown" do context "on split-checkout" do before do - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout).and_return(true) - allow(OpenFoodNetwork::FeatureToggle). - to receive(:enabled?).with(:split_checkout, anything).and_return(true) + Flipper.enable(:split_checkout) set_order order_outside_zone login_as(user_outside_zone)