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

Convert FORM_LINK_WORKFLOW FF to a Standard plan privilege #32431

Merged
merged 7 commits into from
Jan 31, 2023

Conversation

sravfeyn
Copy link
Member

Product Description

This moves the FORM_LINK_WORKFLOW toggle to a Standard plan privilege within the existing End of Form Navigation feature.
https://dimagi-dev.atlassian.net/browse/SC-2410

Technical Summary

Feature Flag

Removed FORM_LINK_WORKFLOW toggle

Safety Assurance

Safety story

I have tested this locally for apps built with toggle enabled previously before removing the toggle and I have tested that the feature works without the toggle for fresh apps.

Automated test coverage

Tests exist

QA Plan

NA, I have done UI tests myself as they are minimal

Migrations

  • The migrations in this code can be safely applied first independently of the code

Rollback instructions

If the toggle deletion needs to be reverted we need to identify domains that start using the feature without the toggle and then enable the toggle for those domains after reverting

  • This PR can be reverted after deploy with no further considerations

Labels & Review

  • Risk label is set correctly
  • The set of people pinged as reviewers is appropriate for the level of risk of the change

@sravfeyn sravfeyn added the product/feature-flag Change will only affect users who have a specific feature flag enabled label Dec 20, 2022
@dimagimon dimagimon added the reindex/migration Reindex or migration will be required during or before deploy label Dec 20, 2022
@sravfeyn
Copy link
Member Author

This needs a bit more work on the strategy for dealing with domains that are below standard plan and have enabled the flag. Currently I have captured the subscription data for all the domains that have the flag enabled.

@sravfeyn
Copy link
Member Author

sravfeyn commented Dec 21, 2022

@snopoke @biyeun @dannyroberts

Just checking for others inputs on how to deal with domains that are under a plan that we want to upgrade the feature flag to. This is likely going to be an issue with other flags that will be GAed soon, so arriving at a good solution will make it easy for others to follow. I am happy to implement those, but want to get input from the wider team.

@gherceg suggested taking the approach of creating a custom plan using a method he followed earlier. This is likely the best approach we have so far, but I suspect if this will create too many accounting data models when we do this for large number of feature flags.

@snopoke
Copy link
Contributor

snopoke commented Dec 22, 2022

@snopoke @biyeun @dannyroberts

Just checking for others inputs on how to deal with domains that are under a plan that we want to upgrade the feature flag to. This is likely going to be an issue with other flags that will be GAed soon, so arriving at a good solution will make it easy for others to follow. I am happy to implement those, but want to get input from the wider team.

@gherceg suggested taking the approach of creating a custom plan using a method he followed earlier. This is likely the best approach we have so far, but I suspect if this will create too many accounting data models when we do this for large number of feature flags.

@sravfeyn I'm not sure what the best approach here is. This seems like something you should discuss with the SaaS ops team?

@sravfeyn
Copy link
Member Author

I have added a CEP #32482 that I am planning to implement to deal with the problem of enabling for domains that may not have paid plan.

]

operations = [
migrations.RunPython(_grandfather_form_link_workflow_privs),
Copy link
Contributor

@SmittieC SmittieC Jan 19, 2023

Choose a reason for hiding this comment

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

If I'm not mistaken, this migration is not revertable if a "down" method is not specified

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't usually add a down method for these sort of migrations,

@@ -537,7 +537,7 @@ def all_toggles_by_name_in_scope(scope_dict, toggle_class=StaticToggle):
result = {}
for toggle_name, toggle in scope_dict.items():
if not toggle_name.startswith('__'):
if isinstance(toggle, toggle_class):
if type(toggle) == toggle_class:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: add comment to indicate why isinstance can't be used

Copy link
Member Author

Choose a reason for hiding this comment

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

Added

@sravfeyn
Copy link
Member Author

Build is passing now. Bumping to see if there is any further feedback on this PR.

@sravfeyn
Copy link
Member Author

Hey @snopoke does this look good to you now?

@sravfeyn
Copy link
Member Author

@snopoke

@sravfeyn sravfeyn merged commit 36979ed into master Jan 31, 2023
@sravfeyn sravfeyn mentioned this pull request Feb 1, 2023
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
product/feature-flag Change will only affect users who have a specific feature flag enabled reindex/migration Reindex or migration will be required during or before deploy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants