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-696] Cleanup Optimizely Experiments #1812

Merged
merged 32 commits into from Apr 19, 2023
Merged

Conversation

scottkicks
Copy link
Contributor

@scottkicks scottkicks commented Apr 12, 2023

📲 What

Remove all old optimizely experiments from the source code.

🤔 Why

Removing Optimizely saves us an expensive subscription that can be replaced for free by more useful tools like Firebase Remote Config.

🛠 How

Removed each experiment commit by commit.
Each commit shows the removal of the experiment as well as as updates to corresponding tests and associated code. For example, removing the nativeOnboarding experiment renders the goToLanding flow/page unused. That code has been removed as well as the experiment (see the last commit for these changes).

This is the list of the current experiments living in the code, everything is unit tested.

  • native_onboarding_series_new_backers
  • onboarding_category_personalization_flow
  • native_project_cards
  • native_risk_messaging
  • Finally I was able to remove OptimizelyExperiment.swift as well as getVariationKey() from OptimizelyClientType

I identified the control variant and integrated it properly outside the experiment.

The following landing page classes have been deleted.
If we want to add a landing page flow back in we can rebuild with SwiftUI/Combine

  • LandingPageViewController.swift
  • LandingPageViewControllerTests.swift
  • LandingPageStatsView.swift
  • LandingPageCardType.swift
  • LandingPageStatsViewModel.swift
  • LandingPageStatsViewModelTests.swift
  • LandingPageViewModel.swift
  • LandingPageViewModelTests.swift

I did not address the native_risk_messaging associated code because I'd like more context on that feature's expected behavior first.

✅ Acceptance criteria

I suggest reviewing this commit by commit to better understand the impact of each experiment.

  • Ensure existing features behave exactly as they do in the production app version and that all tests pass.

@scottkicks scottkicks self-assigned this Apr 12, 2023
@nativeksr
Copy link
Collaborator

1 Warning
⚠️ Big PR

Generated by 🚫 Danger

@scottkicks scottkicks marked this pull request as ready for review April 12, 2023 18:49
@scottkicks scottkicks requested a review from msadoon April 12, 2023 18:49
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.

Wow! That was a lot! Good work this is a lot of good unused code removal.

A few things:

  1. Go through each comment and write a commit for it specifically, that'll make the changes easier to review.
  2. Mostly on me, but if a PR ever becomes big like this earlier on in the coding just use best judgement and split it up. I think we both enjoy reading smaller more focused PRs.

Awesome. Launch the app and ensure the features are all working as expected. I suspect they should be fine. I'll do the same on my end after the PR is ready to merge.

Kickstarter-iOS/AppDelegateViewModel.swift Show resolved Hide resolved
Kickstarter-iOS/AppDelegateViewModel.swift Show resolved Hide resolved
Kickstarter-iOS/AppDelegateViewModel.swift Outdated Show resolved Hide resolved
Kickstarter-iOS/AppDelegateViewModel.swift Outdated Show resolved Hide resolved
Kickstarter-iOS/AppDelegateViewModelTests.swift Outdated Show resolved Hide resolved
Library/ViewModels/DiscoveryPageViewModelTests.swift Outdated Show resolved Hide resolved
Library/ViewModels/ThanksViewModelTests.swift Show resolved Hide resolved
Library/ViewModels/PledgeViewModelTests.swift Show resolved Hide resolved
Library/ViewModels/PledgeViewModelTests.swift Show resolved Hide resolved
@msadoon msadoon added this to the release-5.8.0 milestone Apr 17, 2023
@scottkicks scottkicks requested a review from msadoon April 17, 2023 17:27
@msadoon msadoon modified the milestones: release-5.8.0, release 5.8.0 Apr 17, 2023
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.

Ok a few more things to cover. I guess the biggest thing is to remove the screenshots from any view controller we delete. Those image files are big and lots of them exist for each test and there are many per test. Good opportunity for clean up!

The unresolved comments are the only things left...most of them should be straightforward, ping me if there's anything confusing.

Library/MockOptimizelyClient.swift Outdated Show resolved Hide resolved
Library/OptimizelyClientType.swift Show resolved Hide resolved
Library/ViewModels/LandingViewModel.swift Outdated Show resolved Hide resolved
Library/ViewModels/LandingViewModelTests.swift Outdated Show resolved Hide resolved
Kickstarter-iOS/AppDelegateViewModelTests.swift Outdated Show resolved Hide resolved
Library/ViewModels/ThanksViewModelTests.swift Show resolved Hide resolved
@scottkicks
Copy link
Contributor Author

@msadoon ok sorry about the missed comments. each should be addressed now. I just had one question about the removal of DiscoveryProjectCardCell. I see it being used in ThanksProjectDataSource so just want to make sure its ok to be removed.

@scottkicks scottkicks requested a review from msadoon April 18, 2023 16:24
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.

Ok, great work - this is a lot of unused code removed and excellent clean up.
I tested the feature flags - campaign, Stripe payment sheets - they are all still on. Maybe do a quick smoke test yourself of sign up flow.

@scottkicks scottkicks merged commit 6e20524 into main Apr 19, 2023
5 checks passed
@scottkicks scottkicks deleted the scott/clean-up-experiments branch April 19, 2023 15:40
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

3 participants