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-897] Feature Flag Use of AI Project Tab #1841

Merged
merged 8 commits into from Aug 2, 2023

Conversation

msadoon
Copy link
Contributor

@msadoon msadoon commented Aug 1, 2023

πŸ“² What

Part of the new project page "Use of AI" tab.

πŸ€” Why

Feature flag is standard for launching new feature.

πŸ›  How

Added to RemoteConfigFeatures, RemoteConfigFeature+Helpers, RemoteConfigFeatureFlagToolsViewModel.

use_of_ai_project_tab also added to RemoteConfig on Firebase Remote Config. Productio, Beta and Alpha.

πŸ‘€ See

Trello, screenshots, external resources?

Before πŸ› After πŸ¦‹

βœ… Acceptance criteria

  • Feature flag is available for use via a helper method.

@msadoon msadoon added this to the release-5.10.0 milestone Aug 1, 2023
@msadoon msadoon self-assigned this Aug 1, 2023
@codecov
Copy link

codecov bot commented Aug 1, 2023

Codecov Report

Merging #1841 (3c05f52) into main (6266d87) will increase coverage by 0.00%.
The diff coverage is 89.39%.

@@           Coverage Diff           @@
##             main    #1841   +/-   ##
=======================================
  Coverage   84.53%   84.53%           
=======================================
  Files        1274     1274           
  Lines      115216   115279   +63     
  Branches    30666    30698   +32     
=======================================
+ Hits        97402    97456   +54     
- Misses      16746    16754    +8     
- Partials     1068     1069    +1     
Files Changed Coverage Ξ”
Library/RemoteConfig/RemoteConfigFeature.swift 0.00% <0.00%> (ΓΈ)
...Models/RemoteConfigFeatureFlagToolsViewModel.swift 72.60% <37.50%> (-4.33%) ⬇️
...ary/RemoteConfig/RemoteConfigFeature+Helpers.swift 80.00% <80.00%> (ΓΈ)
...emoteConfig/RemoteConfigFeature+HelpersTests.swift 100.00% <100.00%> (ΓΈ)
...s/RemoteConfigFeatureFlagToolsViewModelTests.swift 100.00% <100.00%> (ΓΈ)

... and 1 file with indirect coverage changes

πŸ“£ We’re building smart automated test selection to slash your CI/CD build times. Learn more

@msadoon msadoon marked this pull request as ready for review August 1, 2023 18:24
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

Why is GraphAPI being updated πŸ€”? This mutation should have been updated when I cleaned up this mutation in my TriggerThirdPartyEvent implementation.

We need this update, but it would be better to address this in its own PR and keep the context of this one to the feature flag

Comment on lines +42 to +49
func testCreatorDashboard_RemoteConfig_FeatureFlag_True() {
let mockRemoteConfigClient = MockRemoteConfigClient()
|> \.features .~ [RemoteConfigFeature.creatorDashboardEnabled.rawValue: true]

withEnvironment(remoteConfigClient: mockRemoteConfigClient) {
XCTAssertTrue(featureCreatorDashboardEnabled())
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nice add

Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

We also should update the feature flag view controller's snapshot tests

@msadoon
Copy link
Contributor Author

msadoon commented Aug 2, 2023

Why is GraphAPI being updated πŸ€”? This mutation should have been updated when I cleaned up this mutation in my TriggerThirdPartyEvent implementation.

We need this update, but it would be better to address this in its own PR and keep the context of this one to the feature flag

Good point, normally it would be better to have this as a separate change elsewhere, but it's a change that happens as part of the build process. I mentioned in this comment, every time the project gets built that GraphAPI file updates based on the GraphSchema.json downloaded off staging. It's the script in Build Phases that runs in KsAPI when its' built.

With a cursory glance, it looks like the backend folks cleaned up the TriggerCapiEventMutation. Since our app still builds and we no longer support that mutation, it's safe to keep in.

Another alternative is to remove the changes here for GraphAPI, create a separate PR and run through tests for sending those events again. We already did this in our QA party so let me know if you see a valid case for making those changes and I will remove them in this PR and the PX upgrade.

msadoon and others added 2 commits August 2, 2023 10:49
Co-authored-by: Scott Clampet <110618242+scottkicks@users.noreply.github.com>
Co-authored-by: Scott Clampet <110618242+scottkicks@users.noreply.github.com>
@msadoon
Copy link
Contributor Author

msadoon commented Aug 2, 2023

We also should update the feature flag view controller's snapshot tests

Yes! Missed this, good catch.

@msadoon msadoon requested a review from scottkicks August 2, 2023 15:54
Copy link
Contributor

@scottkicks scottkicks left a comment

Choose a reason for hiding this comment

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

πŸ‘
just need to update with main

@msadoon msadoon merged commit 0e82d13 into main Aug 2, 2023
7 checks passed
@msadoon msadoon deleted the mbl-897/ff-use-of-ai-project-tab branch August 2, 2023 17:19
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