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

[MBL-853] Create Feature Flag To Hide Creator Dashboard #1827

Merged
merged 5 commits into from Jun 15, 2023
Merged

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Jun 13, 2023

📲 What

Create a new feature flag that will allow KS to hide creator tools on iOS in a more controlled way.
This feature flag will be defaulted to true because the feature being implemented here is the removal of our creator dashboard.

🤔 Why

Creator tools will eventually be removed from the app altogether and this flag will allow us to slowly roll this out.

🛠 How

  1. Add a new parameter, creator_dashboard, in our Firebase Remote Config Dashboard
    • You’ll want to add one to the Production project and Beta project (for testing locally)
  2. In RemoteConfigFeature use the same parameter string to add a new enum case
  3. Add a new helper in RemoteConfigFeature+Helpers
  4. Add unit tests around this new helper in RemoteConfigFeatureHelpersTests
  5. Update RemoteConfigFeatureFlagToolsViewControllerTests and its corresponding snapshot.

👀 See

Trello, screenshots, external resources?

Before 🐛 After 🦋
Screenshot 2023-06-13 at 11 27 57 AM Screenshot 2023-06-14 at 10 24 25 AM

✅ Acceptance criteria

You’ll need to run the app locally using the AppCenter Beta scheme.
Remember that Firebase is only configured #if RELEASE || APPCENTER

  • A new parameter with the string creator_dashboard has been added to the firebase dashboard to the production and beta projects and is defaulted to true
  • The new feature is showing up in the Remote Config Feature Flags list in the app's debug menu.
  • The flags toggle value in the debug menu updates accordingly when the parameter value changes in the Firebase Dashboard. Re-open the app for the flag values to re activate
  • Changing the flag value from the debug menu persists. This is for local development. Once changed locally the app will prioritize the value stored in user defaults.

@scottkicks scottkicks self-assigned this Jun 13, 2023
@scottkicks scottkicks added this to the release-5.9.0 milestone Jun 13, 2023
@codecov
Copy link

codecov bot commented Jun 13, 2023

Codecov Report

Merging #1827 (8c1f7c9) into main (7015f3b) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #1827      +/-   ##
==========================================
- Coverage   84.35%   84.35%   -0.01%     
==========================================
  Files        1276     1276              
  Lines      115436   115457      +21     
  Branches    30738    30751      +13     
==========================================
+ Hits        97381    97395      +14     
- Misses      16988    16994       +6     
- Partials     1067     1068       +1     
Impacted Files Coverage Δ
Library/RemoteConfig/RemoteConfigFeature.swift 0.00% <0.00%> (ø)
...Models/RemoteConfigFeatureFlagToolsViewModel.swift 76.92% <37.50%> (-5.54%) ⬇️
...ary/RemoteConfig/RemoteConfigFeature+Helpers.swift 80.00% <80.00%> (ø)
...oteConfigFeatureFlagToolsViewControllerTests.swift 100.00% <100.00%> (ø)
...emoteConfig/RemoteConfigFeature+HelpersTests.swift 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@scottkicks scottkicks marked this pull request as ready for review June 13, 2023 19:23
@scottkicks scottkicks requested a review from msadoon June 13, 2023 19:23
Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

All looks good, functionality working from dashboard to beta flag tools with caching taking precedence.
One thing - and maybe this is just my sense of how I'm reading the flag - can we remove "Hidden" from the naming? It's just the same as the other flags, "Creator Dashboard" - comment below.

Library/RemoteConfig/RemoteConfigFeature.swift Outdated Show resolved Hide resolved
Copy link
Contributor

@msadoon msadoon left a comment

Choose a reason for hiding this comment

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

One minor miss here, easily corrected, but could be caught the first time around.

@@ -40,4 +40,13 @@ final class RemoteConfigFeatureHelpersTests: TestCase {
XCTAssertFalse(featureFacebookLoginInterstitialEnabled())
}
}

func testCreatorDashboardHidden_RemoteConfig_FeatureFlag_False() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Always remember to update the test names to reflect the correct values being tested. In this case we changed creatorDashboardHiddenEnabled to creatorDashboardEnabled so the test name should change as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for minor changes like this it could be helpful to comment it as a suggestion so that we can easily commit directly from the PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, that's normally how I'd do it, but I want us to get into the habit of committing all changes once correctly.
So example - You assign a PR to me, it's reviewed, all changes requested in review, you make as many commits/ask as many questions as you want, but once it's re-assigned back to me, it should pretty much be perfect.

The intention is to reduce the amount of back and forth, just as learning tool. I'm going to explicitly request changes for minor corrections from now on so we get into the habit.

@scottkicks scottkicks merged commit 2c5ef89 into main Jun 15, 2023
7 checks passed
@scottkicks scottkicks deleted the scott/mbl-853 branch June 15, 2023 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants