[MBL-854] Gate Creator Dashboard Behind Feature Flag #1828

scottkicks · 2023-06-15T15:32:00Z

📲 What

Gates displaying creator tools (dashboard) behind the new feature flag.

🤔 Why

Since the creator tools (dashboard) is going to be depreciated, and this is a pretty big feature in the app currently, doing so behind a feature flag is safer and gives us more control over the deprecation.

🛠 How

There are a couple of entry points to the creator dashboard.

The Dashboard tab in the main tab bar that only shows when a creator is logged in.
Deep links handled in RootTabBarViewController & ProjectPageViewController

We can use the featureCreatorDashboardEnabled() helper method to gate the following methods responsible for displaying the dashboard tab.

RootViewModel.generatePersonalizedViewControllers(): which view controllers to add to the root tab view.
- Creates an array with .dashboard(isMember: Bool) and .profile(isLoggedIn: Bool)
- The setViewControllers signal output is then used in the RootTabBarViewController to determine which VC to instantiate for each tab.
RootViewModel.tabData(forUser: User): what tab data to use for each tab.
- Signal is called when there is a currentUser and viewDidLoad, or userLocalePreferencesChangedProperty (logged in/logged out), is emitted.
- Based on whether the user is a member (determined by whether the user.stats.memberProjectsCount > 0)

The deep link cases can be found:

access to switch to the dashboard: line 170 RootTabBarViewController
access to feature of the creator dashboard project activities: line 231 RootTabBarViewController
access to feature of the creator dashboard creator messages: line 217 RootTabBarViewController
creator dashboard from the project page: line 264 ProjectPageViewController

👀 See

Dashboard Enabled	Dashboard Disabled

✅ Acceptance criteria

You can log in as a creator using the therealnativesquad@gmail.com account in 1Password under Kickstarter Native Squad

When Creator Dashboard is not enabled

The creator dashboard tab should not be available regardless of whether you’re logged in as a backer or creator, or logged out.

When Creator Dashboard is enabled

The creator dashboard tab should be shown as normal to logged in creators only.

⏰ TODO

Discuss the fact that testing cases where RemoteConfig flags are enabled isn't possible right now because the RemoteConfig SDK's obj-c initializer isn't available.

…feature flag pre-planning

the cases where the flag is true is currently not testable due to RemoteConfig's OBJ-C initializer not being available. Still looking into this.

codecov · 2023-06-15T16:12:10Z

Codecov Report

Merging #1828 (18a1b49) into main (e629b80) will decrease coverage by 0.01%.
The diff coverage is 73.12%.

@@            Coverage Diff             @@
##             main    #1828      +/-   ##
==========================================
- Coverage   84.37%   84.37%   -0.01%     
==========================================
  Files        1276     1276              
  Lines      115679   115816     +137     
  Branches    30764    30793      +29     
==========================================
+ Hits        97604    97714     +110     
- Misses      17009    17036      +27     
  Partials     1066     1066

Impacted Files	Coverage Δ
...ectPage/Controller/ProjectPageViewController.swift	`50.23% <0.00%> (-0.17%)`	⬇️
...Features/RootTabBar/RootTabBarViewController.swift	`4.77% <0.00%> (-0.38%)`	⬇️
Library/ViewModels/RootViewModel.swift	`0.00% <0.00%> (ø)`
...emoteConfig/RemoteConfigFeature+HelpersTests.swift	`100.00% <100.00%> (ø)`
Library/ViewModels/RootViewModelTests.swift	`100.00% <100.00%> (ø)`

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

scottkicks · 2023-06-15T16:34:10Z

@msadoon re: our sync conversation around testing cases where our Remote Config flags are true...

See the FIXME comments I've left in RootViewModelTests.
You can step through func tabData(forUser...)on line: 498 in RootViewModel to see that even though the remoteConfigClient in AppEnvironment is being set to the Mock with the flag set to true via the tests, the featureCreatorDashboardEnabled() call in the view model is returning false.

My current theory is that its related to the commented out tests in RemoteConfigFeature+HelpersTests that deal with the RemoteConfigValue not initializing.

Lmk if you have any thoughts on what might be happening here. Thanks!

msadoon · 2023-06-15T18:25:35Z

@msadoon re: our sync conversation around testing cases where our Remote Config flags are true...

See the FIXME comments I've left in RootViewModelTests. You can step through func tabData(forUser...)on line: 498 in RootViewModel to see that even though the remoteConfigClient in AppEnvironment is being set to the Mock with the flag set to true via the tests, the featureCreatorDashboardEnabled() call in the view model is returning false.

My current theory is that its related to the commented out tests in RemoteConfigFeature+HelpersTests that deal with the RemoteConfigValue not initializing.

Lmk if you have any thoughts on what might be happening here. Thanks!

yep thanks for flagging this in a comment, I will take a look once the pr is ready to review.

msadoon

Overall good job hiding the DashboardViewController on first and subsequent launches.

I found that there are scenarios where the app just expects DashboardViewController is already shown to the user. Deeplinks for example are routes we support to open the dashboard view controller that can be used even if we've hidden it. So with that in mind let's also gate any events that change tabs when the DashboardViewController isn't shown.

The cases found were:

access to switch to the dashboard
- line 170 RootTabBarViewController
access to feature of the creator dashboard project activities
- line 231 RootTabBarViewController
access to feature of the creator dashboard creator messages
- line 217 RootTabBarViewController
creator dashboard from the project page
- line 264 ProjectPageViewController

So my suggestion is to gate these with the featureCreatorDashboardEnabled function. Make that guard check the first thing each function does before letting control flow through. Some tests might need corrections, no need to write both true and false cases, just make the exist ones to pass using MockRemoteConfigClient.

Which leads to the correction for MockRemoteConfigClient

Here is the correction, basically it overrides the value we check boolValue from RemoteConfig with a value we can change bool. We have to do this because RemoteConfigValue has no exposed initializer in the public API from Firebase.

private class MockRemoteConfigValue: RemoteConfigValue {
  var bool = false
  
  override var boolValue: Bool { bool }
}

All your tests should pass then. I ran them myself and they did. Good job on the tests.

I'll do actual in-app testing on the next review if there's no more code changes. I didn't do it on this one because I'd like to test all the final code changes in app. Let's aim to make the next review about testing the functionality and approved.

Library/ViewModels/RootViewModel.swift

Library/ViewModels/RootViewModelTests.swift

RemoteConfigValue has no exposed initializer in the public API from Firebase so this is how can change boolValue for testing.

scottkicks · 2023-06-20T20:48:51Z

@msadoon each of your suggestions have been addressed. Thanks for catching those other cases where the dashboard is shown. I think that should be everything. If anyone manages to get to the dashboard in an unforeseen case, the banner i'll be adding next will at least let them know it'll be going away soon.

also...

it looks like gating the deep link handlers doesn't impact any tests. All existing view controller tests still pass after the changes. And i've already covered the switchToDashboard flow, that these view controller methods end up calling, with unit tests in my previous commits. I think we should be good to go.

msadoon

Ok, code changes look good and in-app functionality works as expected for the most part, but I found an inconsistency for the tab items:

RPReplay_Final1687362266.MP4

Here's a recording and you'll notice that being logged in as a creator, if I turn off the beta flag, and switch to the profile tab, the user is shown two profile tabs, but the second profile tab is the Dashboard (it should be hidden). Relaunches fixes the issue.

If logged in as a backer then turning on the beta flag causes my profile tab to change to Dashboard but I don't see an additional tab - for Dashboard. Relaunch fixes the issue.

This bug occurs on production as well, without using the Beta Tools at all. Switching the dashboard values on/off there is a transition period before the tab bar fixes itself (takes subsequent relaunches).

So a small UI bug, but it does effect the production build and will end up confusing users, so we should probably fix it.

Can we investigate this issue and fix it? I don't know exactly what might be the cause, but if you want to pair on it, let me know.

msadoon

Ok I uploaded fix for the creator dashboard - but the tests are failing because the GraphSchema needs to be updated - will take a look at that in my Add New Card Removal PR.

msadoon · 2023-06-22T22:17:58Z

Library/ViewModels/RootViewModel.swift

@@ -500,13 +500,13 @@ private func tabData(forUser user: User?) -> TabBarItemsData {
    (user?.stats.memberProjectsCount ?? 0) > 0
  let items: [TabBarItem]

-  switch (featureCreatorDashboardEnabled(), isMember) {
-  case (false, _), (true, false):
+  switch isMember {


So the idea is that the tabs are based on the user. And they don't need gating because this tab data is only used for styling - the actual setting of the tabs in the tab bar is done in setViewControllers.

msadoon · 2023-06-22T22:22:33Z

Kickstarter-iOS/Features/RootTabBar/RootTabBarViewController.swift

    }
  }

+  fileprivate func isDashboardViewControllerDisplayable() -> Bool {


This function just checks if the DashboardTabViewController is available to display.

msadoon · 2023-06-22T22:22:52Z

Kickstarter-iOS/Features/RootTabBar/RootTabBarViewController.swift

+    }
+  }
+
+  fileprivate func setProfileImage(with data: TabBarItemsData, avatarUrl: URL?, index: Int) {


Move all the profile downloading to a separate function.

msadoon · 2023-06-22T22:41:39Z

Kickstarter-iOS/Features/RootTabBar/RootTabBarViewController.swift

+        let featureFlaggedTabBarItemStyle = self
+          .isDashboardViewControllerDisplayable() ? dashboardTabBarItemStyle :
+          profileTabBarItemStyle(isLoggedIn: data.isLoggedIn, isMember: data.isMember)
+        _ = tabBarItem(atIndex: index) ?|> featureFlaggedTabBarItemStyle


This function is called as a result of tabData(forUser and it checks if that tab is available before styling it. So if we sent "Explore, Activity, Search, Dashboard and Profile" to this function - the view controllers may only be "Explore, Activity, Search and Profile". Which means that Dashboard case updates its styling to reflect "Profile" and the Profile case doesn't update anything - the tab doesn't exist. self.tabBar.items is called under the hood for tabBarItem(atIndex: and reflects the view controllers - which are independently set by setViewControllers (whenever that happens).

The only trade-off is that we lose the Profile user avatar, if they were a creator logged in with the dashboard hidden. I think its' worth it because this only affects logged in creators and not backers and we're removing creator login functionality anyway.

…ture flag.

scottkicks · 2023-06-23T15:11:10Z

@msadoon this makes sense because directly gating tabBarItemsData from the ViewModel can easily cause the setting of the view controllers and the associated tab data to be out of sync. Plus updating from the dashboard can have some lag so this gets around that as well.

Why does the graphschema need to be updated? Since you'll be taking care of that in a different PR this one should wait to be merged in until that is resolved.

scottkicks · 2023-06-23T15:13:56Z

...res/RemoteConfigFeatureFlagTools/Controller/RemoteConfigFeatureFlagToolsViewController.swift

-    let (feature, _) = self.features[switchControl.tag]
-    if feature == RemoteConfigFeature.creatorDashboardEnabled {
-      NotificationCenter.default.post(
-        name: Notification.Name.ksr_userLocalePreferencesChanged,
-        object: nil,
-        userInfo: nil
-      )
-    }


This still might be worth having for testing. With your solution here, nothing will happen when toggling the feature from the local ff menu

msadoon

Good to go!

scottkicks added 2 commits June 15, 2023 09:11

Gate tab view controller instantiations and tab data creation behind …

4a246c4

…feature flag pre-planning

update RootViewModelTests

c11a758

the cases where the flag is true is currently not testable due to RemoteConfig's OBJ-C initializer not being available. Still looking into this.

scottkicks self-assigned this Jun 15, 2023

scottkicks added needs review WIP labels Jun 15, 2023

scottkicks added this to the release-5.9.0 milestone Jun 15, 2023

scottkicks removed the WIP label Jun 15, 2023

scottkicks marked this pull request as ready for review June 15, 2023 18:29

scottkicks requested a review from msadoon June 15, 2023 18:29

msadoon suggested changes Jun 15, 2023

View reviewed changes

Library/ViewModels/RootViewModel.swift Outdated Show resolved Hide resolved

Library/ViewModels/RootViewModelTests.swift Outdated Show resolved Hide resolved

Library/ViewModels/RootViewModelTests.swift Show resolved Hide resolved

scottkicks added 8 commits June 20, 2023 09:26

add override to MockRemoteConfigValue

b190213

RemoteConfigValue has no exposed initializer in the public API from Firebase so this is how can change boolValue for testing.

update tests now that we can effectively control the flag's boolValue

8cd67b1

gate access to switch to dashboard from RootTabBarViewController

6e91c2b

gate access to the creator dashboard project activities

a8fcf0c

gate access to creator dashboard creator messages + formatting

192da3f

gate access to creator dashboard from the project page

87d71dd

refactor if/else and add suffix to test

74d59fb

formatting

2b30a91

scottkicks requested a review from msadoon June 20, 2023 20:48

msadoon suggested changes Jun 21, 2023

View reviewed changes

scottkicks and others added 2 commits June 21, 2023 11:31

refresh the tabbar when toggling the creator dashboard feature flag

6a338af

corrected dashboard tabs/vcs inconsistency.

aa55b4e

msadoon reviewed Jun 22, 2023

View reviewed changes

removed unused test - because the tabs are no longer based on the fea…

f2760b5

…ture flag.

scottkicks added the blocked a PR that is blocked for external reasons label Jun 23, 2023

scottkicks commented Jun 23, 2023

View reviewed changes

Merge branch 'main' into scott/mbl-854

18a1b49

scottkicks removed the blocked a PR that is blocked for external reasons label Jun 29, 2023

scottkicks requested a review from msadoon June 29, 2023 19:49

msadoon approved these changes Jun 29, 2023

View reviewed changes

scottkicks merged commit ea00943 into main Jun 29, 2023

scottkicks deleted the scott/mbl-854 branch June 29, 2023 19:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MBL-854] Gate Creator Dashboard Behind Feature Flag #1828

[MBL-854] Gate Creator Dashboard Behind Feature Flag #1828

scottkicks commented Jun 15, 2023 •

edited by msadoon

Loading

codecov bot commented Jun 15, 2023 •

edited

Loading

scottkicks commented Jun 15, 2023

msadoon commented Jun 15, 2023

msadoon left a comment •

edited

Loading

scottkicks commented Jun 20, 2023

msadoon left a comment •

edited

Loading

msadoon left a comment

msadoon Jun 22, 2023

msadoon Jun 22, 2023

msadoon Jun 22, 2023

msadoon Jun 22, 2023 •

edited

Loading

msadoon Jun 22, 2023

scottkicks commented Jun 23, 2023

scottkicks Jun 23, 2023

msadoon left a comment

[MBL-854] Gate Creator Dashboard Behind Feature Flag #1828

[MBL-854] Gate Creator Dashboard Behind Feature Flag #1828

Conversation

scottkicks commented Jun 15, 2023 • edited by msadoon Loading

📲 What

🤔 Why

🛠 How

👀 See

✅ Acceptance criteria

When Creator Dashboard is not enabled

When Creator Dashboard is enabled

⏰ TODO

codecov bot commented Jun 15, 2023 • edited Loading

Codecov Report

scottkicks commented Jun 15, 2023

msadoon commented Jun 15, 2023

msadoon left a comment • edited Loading

Choose a reason for hiding this comment

scottkicks commented Jun 20, 2023

msadoon left a comment • edited Loading

Choose a reason for hiding this comment

msadoon left a comment

Choose a reason for hiding this comment

msadoon Jun 22, 2023

Choose a reason for hiding this comment

msadoon Jun 22, 2023

Choose a reason for hiding this comment

msadoon Jun 22, 2023

Choose a reason for hiding this comment

msadoon Jun 22, 2023 • edited Loading

Choose a reason for hiding this comment

msadoon Jun 22, 2023

Choose a reason for hiding this comment

scottkicks commented Jun 23, 2023

scottkicks Jun 23, 2023

Choose a reason for hiding this comment

msadoon left a comment

Choose a reason for hiding this comment

scottkicks commented Jun 15, 2023 •

edited by msadoon

Loading

codecov bot commented Jun 15, 2023 •

edited

Loading

msadoon left a comment •

edited

Loading

msadoon left a comment •

edited

Loading

msadoon Jun 22, 2023 •

edited

Loading