Add group scoped feature flag for branching#1729
Conversation
| def branching_enabled | ||
| @branching_enabled ||= Settings.features.branch_routing || current_form.group.branching_enabled? | ||
| end | ||
|
|
There was a problem hiding this comment.
This could be a concern to stop it cluttering the page controller.
I think for what will be a short lived feature flag, it's simpler to be explicit.
happy to hear other views though.
There was a problem hiding this comment.
Could we instead extend the FeatureService#enabled? method to optionally accept a group (or form even)?
There was a problem hiding this comment.
@lfdebrux I think we previously did something similar to that (though I can't actually remember for which feature 😅 ).
I think it made things tricky across environments, as the ids obviously don't match up. This meant that to test in dev you'd be using an id that might match an entirely different form in production, for example. I think @stephencdaly and I concluded it was nicer storing whether the feature was enabled on the model itself, as then you can freely test in different environments without causing issues.
There was a problem hiding this comment.
Sorry, I meant that passing a group to enabled? would check whether group.send("#{feature}_enabled") was true, or similar. Or if a form was passed it would look at form.group. I'm not suggesting where the feature flag is recorded, just hoping to keep a consistent interface for reading it.
There was a problem hiding this comment.
I've replaced this PR with another which tackles this issue
The branching_enabled boolean column defaults to false. It's a group specific feature flag. A group with branching_enabled set to true indicates the users of the group will be able to use the new branching and secondary skip features before it's widely released.
Add two tasks to enable and disable the branch feature for a specific group. Heavily inspired by #1645 Like the above PR, there are no tests.
To keep track of whether the advanced branching feature is enabled for the current form, or if it's enabled for the whole environment, a new method is added to the page controller. It's a shame add more methods to the controller. Doing so allows us to access value in all the places we need it.
This commit changes the way the summary_card_data_presenter checks if the branching feature flag is enabled. The flag can now be set on a group. The value is passed down from the controller into the views. The tests are updated where needed.
The routing page view now accesses the branching_enabled flag and the tests have been changed.
The feature flag for advanced branching features can now be set per group or environment wide. This commit changes the view to use the instance variable rather than the FeatureService directly.
The before action to check that the advanced branching features are enabled has been changed to use the Page#branching_enabled method, which checks the group and the environment variable.
thomasiles
force-pushed
the
add-group-scoped-feature-flag-for-branching
branch
from
ee70c1d to
6bf1654
Compare
|
|
Closed in favour of #1737 |
3 participants
Add new Group scoped feature flag for branching
Trello card: https://trello.com/c/GgbE96M1/2083-enable-branch-routing-feature-in-production-for-an-internal-test-group
To begin with, we want to release the advanced branching features to a small number of users.
To do this, we are enabling some Groups to have the feature flag enabled, instead of enabling it for the whole environment. We still want the flag to be enabled in dev for all users and groups.
This PR is heavily based off the changes @stephencdaly and feature team one did for file upload.
It's more complicated because the branching feature flag is used in more places, mainly views.
Things to consider when reviewing