Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GA toggle #6657

Closed
wants to merge 2 commits into from
Closed

GA toggle #6657

wants to merge 2 commits into from

Conversation

sauloperez
Copy link
Contributor

What? Why?

I implemented this while working on #6643

Allows toggling a feature to all users. This comes in handy while in development and you don't want to check which user logged in. Simply run the app with BETA_TESTERS=all and solved.

It also gives us the flexibility to toggle it for all users in production without needing a deployment. Just change the ENV var and restart (reload won't be enough).

What should we test?

This shouldn't need manual testing. It's already thoroughly covered by automated tests.

Release notes

Allow toggling a feature to all users
Changelog Category: Technical changes

@sauloperez sauloperez self-assigned this Jan 13, 2021
@@ -1,3 +1,7 @@
require 'open_food_network/feature'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

@@ -1,3 +1,7 @@
require 'open_food_network/feature'
require 'open_food_network/null_feature'
require 'open_food_network/ga_feature'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to move all the feature toggles code to lib/feature_toggle/ but doing it now will likely mess up with the work I'm doing in #6643. This is good enough to keep on going IMO, and I'll finish later on.

This comes in handy while in development and you don't want to check
which user logged in. Simply run the app with `BETA_TESTERS=all` and
solved.

It also give us the flexibility to toggle it for all users in production
without needing a deploy. Just change the ENV var and restart (reload
won't be enough).
@sauloperez sauloperez changed the title Ga toggle GA toggle Jan 13, 2021
context 'and the feature is disabled for them' do
before { FeatureToggle.enable(:foo, []) }

it 'returns false' do
expect(FeatureToggle.enabled?(:foo, user)).to eq(false)
expect(FeatureToggle.enabled?(:foo, user)).to eq(true)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this WIP?

Copy link
Contributor Author

@sauloperez sauloperez Jan 14, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it wasn't until I found this 😓

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like you marked something that's not correct. I'll move this to in-dev until that's clarified.

context 'and the feature is disabled for them' do
before { FeatureToggle.enable(:foo, []) }

it 'returns false' do
expect(FeatureToggle.enabled?(:foo, user)).to eq(false)
expect(FeatureToggle.enabled?(:foo, user)).to eq(true)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this WIP?

@sauloperez
Copy link
Contributor Author

I'll revisit this once we start the customer balance's rollout. I have too much on my plate right now.

@sauloperez sauloperez closed this Jan 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants