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

Feature flags base #4626

Merged
merged 8 commits into from
Dec 3, 2021
Merged

Feature flags base #4626

merged 8 commits into from
Dec 3, 2021

Conversation

ouchadam
Copy link
Contributor

@ouchadam ouchadam commented Dec 3, 2021

  • Separates the SSO redirection from the LoginActivity + LoginActivity2 by using a dedicated invisible SSORedirectRouterActivity, this allows both activities to be enabled at the same time and not exported

  • Introduces a VectorFeatures abstraction to start grouping our feature flags, this will also enable overriding at runtime and provides an entry point for forks to extend to help avoid conflicts (debug overrides to come in the next PR)

  • Ports the LoginVersion and NotificationSettingsVersion to the VectorFeatures

BEFORE SSO REDIRECT AFTER SSO REDIRECT
before-sso after-sso

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Unit Test Results

  66 files  ±0    66 suites  ±0   50s ⏱️ -5s
135 tests ±0  135 ✔️ ±0  0 💤 ±0  0 ±0 
418 runs  ±0  418 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit db40670. ± Comparison against base commit 1aa5321.

♻️ This comment has been updated with latest results.

Copy link
Member

@bmarty bmarty left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

} else {
clickOn(R.string.settings_notification_advanced)
pressBack()
when (BuildConfig.NOTIFICATION_SETTINGS_VERSION!!) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not using a DefaultVectorFeatures instance here? It would prevent the usage of !! (I think)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! I wasn't sure how the injections would work in the SanityTests, I would invest more time but this is removed in #4627

@@ -145,6 +147,7 @@ class VectorSettingsNotificationPreferenceFragment @Inject constructor(
refreshBackgroundSyncPrefs()

handleSystemPreference()
handleVersionedSettings()
Copy link
Member

Choose a reason for hiding this comment

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

Can we observe some blink with this new way to hide preference? Maybe hide both pref by default in the XML

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there's a blink if the visibility is updated in onResume but onBindPref is instant (as long as we stay on the main thread)

the programmatic visibility is also removed in #4627 🎉

@@ -309,6 +312,15 @@ class VectorSettingsNotificationPreferenceFragment @Inject constructor(
}
}

private fun handleVersionedSettings() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this change is temporary as V2 is live, V1 is removed in #4627

@bmarty bmarty merged commit 1f58913 into develop Dec 3, 2021
@bmarty bmarty deleted the feature/adm/feature-flags branch December 3, 2021 16:38
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