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

Toggle customer balance to entire instance #6801

Conversation

sauloperez
Copy link
Contributor

@sauloperez sauloperez commented Feb 3, 2021

What? Why?

Closes #6804

This refactors the feature toggles implementation to make it flexible enough to fit more complex toggling strategies such as enabling a feature for an entire OFN instance.

For reviewers, going commit by commit might be easier to understand.

What should we test?

spec/initializers/feature_toggles_spec.rb covers this (yes, it's the first time I test an initializer but it's proven enough) but I'd like you @filipefurtad0 to give it a try so you get acquainted with it and can give me feedback. I guess this can ease testing toggled features.

Release notes

Enabled toggling a feature to an entire instance.
Changelog Category: Technical changes

@sauloperez sauloperez self-assigned this Feb 3, 2021
@sauloperez sauloperez force-pushed the toggle-customer-balance-to-entire-instance branch from fdabd5b to 089bfc1 Compare February 3, 2021 10:56
@sauloperez
Copy link
Contributor Author

I feel like we're getting closer and closer to fully reinventing the wheel and having to switch to https://github.com/jnunemaker/flipper. Let's see if can get going a bit more.

This enables toggling features as best fits us in each case. With this
new approach we can then toggle :customer_balance to an entire instance,
which is what we want in France.
@sauloperez sauloperez force-pushed the toggle-customer-balance-to-entire-instance branch from 089bfc1 to d6350c3 Compare February 4, 2021 09:21
Kernel#require prevented this from happening thus, the file was only
loaded once so tests were passing so far by pure luck.
This is a bit more thorough.
@sauloperez
Copy link
Contributor Author

sauloperez commented Feb 4, 2021

@jibees thanks for the thorough review. This revealed a deeper issue that I addressed in 88e22a7. Now I'm fully confident we're covered for future refactorings of the feature flags.

@Matt-Yorkley
Copy link
Contributor

Those flaky specs are different in each build, and they're flaky elsewhere, so I think we can merge 👌

@sauloperez
Copy link
Contributor Author

I tested it locally and it's working as expected. BETA_TESTERS=all also toggles it on.

@sauloperez sauloperez merged commit b7af92a into openfoodfoundation:master Feb 10, 2021
@filipefurtad0
Copy link
Contributor

Ok, thank for testing this @sauloperez . I'll get familiar with this pull request after merge 👍

@sauloperez sauloperez deleted the toggle-customer-balance-to-entire-instance branch February 10, 2021 17:32
@filipefurtad0
Copy link
Contributor

One question about this @sauloperez : BETA_TESTERS=all toggles a feature for all users; but I've found it not to be toggled for guest users. Is this intended? Is there a way to toggle a feature which is not based on email addresses?

@sauloperez
Copy link
Contributor Author

sauloperez commented Feb 24, 2021

but I've found it not to be toggled for guest users.

what do you mean by guest users here? To me, guest user => no user record in our DB for the user that's checking the browser.

Is there a way to toggle a feature which is not based on email addresses?

yes. We enabled :unit_price based on the environment is running on (staging and development), for instance.

@filipefurtad0
Copy link
Contributor

I mean it as an un-logged user. For example, unit prices in #6905 - only appear in the shopfront after a user logs in. If the user logs out the feature is gone as well.

Maybe I'm missing how to activate this on the environment level.

@sauloperez
Copy link
Contributor Author

sauloperez commented Feb 24, 2021

Now I see. Toggles are based on the logged-in user; we choose what criteria should they meet in order for the feature to be enabled for them. Without a user (aka. contexts where you don't need to log in) that cannot happen.

The environment is just one of the myriad of criteria we can choose, but again it's all about which environment the logged user is checking.

@RachL
Copy link
Contributor

RachL commented Feb 24, 2021

Nice convo! I think this is interesting to remember for features we want to toggl for an entire hub (it would by default exclude guest shoppers), am I understanding correctly?

@sauloperez
Copy link
Contributor Author

so far, yes. I think we all see that a toggle's criteria can escalate pretty quickly and become too hard to reason about, right? 😂 something to keep in mind.

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.

Activate new customer balance for all FR users
5 participants