Enable customer_balance feature toggle to all users #7363
Conversation
This however makes the tests still pass, which rely on the off-branch logic of the toggle. We'll work on them next.
Codecov Report
@@ Coverage Diff @@
## master #7363 +/- ##
=======================================
Coverage 93.08% 93.08%
=======================================
Files 633 633
Lines 18142 18134 -8
=======================================
- Hits 16887 16880 -7
+ Misses 1255 1254 -1
Continue to review full report at Codecov.
|
| @@ -3,11 +3,7 @@ | |||
| beta_testers = ENV['BETA_TESTERS']&.split(/[\s,]+/) || [] | |||
There was a problem hiding this comment.
You keep this line just for the record?
There was a problem hiding this comment.
I would remove it in the same commit. So if we needed to revert this, it's done in one step.
These no longer make sense since we're enabling customer_balance unconditionally.
| else | ||
| beta_testers.include?(user.email) | ||
| end | ||
| !Rails.env.test? |
There was a problem hiding this comment.
This makes me a bit uncomfortable...
There was a problem hiding this comment.
Me too. It would have been good modify the specs in a way that they still pass. The simplest way would be to stub this method in related tests. That would make sure that all the other specs, which we think are not affected by this, are still passing and there is no surprising side effect.
|
Hey @sauloperez , |
| @@ -3,11 +3,7 @@ | |||
| beta_testers = ENV['BETA_TESTERS']&.split(/[\s,]+/) || [] | |||
There was a problem hiding this comment.
I would remove it in the same commit. So if we needed to revert this, it's done in one step.
| else | ||
| beta_testers.include?(user.email) | ||
| end | ||
| !Rails.env.test? |
There was a problem hiding this comment.
Me too. It would have been good modify the specs in a way that they still pass. The simplest way would be to stub this method in related tests. That would make sure that all the other specs, which we think are not affected by this, are still passing and there is no surprising side effect.
|
sorry guys, I took the shortcut to have it done quickly and get it into the release. No bugs were reported in FR and UK for the last few weeks (except for the one we fixed) and running the specs with the feature on a separate branch didn't reveal any unexpected issues either. |
This makes all tests exercise the new branch that `OrderBalance` abstracts. It follows up openfoodfoundation#7363 addressing code review comments.
|
I addressed all these comments in #7396 |
What? Why?
Closes #6699
It changes the toggle logic to have it enabled in all environments but testing. This makes the tests still pass, which rely on the off-branch logic of the toggle. We'll work on them next.
So, when we deploy this, if anything goes wrong we can simply revert this commit and we'll get back to the former state: balances enabled only for UK and FR and the individual users that we have configured in ofn-install for all the other instances.
If we wish to disable it for UK and FR too, we'll need to reprovision a new
BETA_TESTERSvar for them though at this point, I hardly doubt we'll encounter any outstanding issue.What should we test?
We should see the new balances, the ones that consider canceled orders, in /account but without having to enable the toggle manually.
Release notes
Release customer balances to all users
Changelog Category: User facing changes