[MOB-2759] Onboarding card NTP Experiment #778

lucaschifino · 2024-09-09T14:37:50Z

Context

We want to experiment how enabling the users to have a self-paced onboarding affects retention.

Approach

Create an Experiment as usual, combined with a new Homepage cell and adjustments to the onboarding and it's flow as required by the acceptance criteria.

Before merging

Checklist

Merge core-side counter part https://github.com/ecosia/ios-core/pull/161
I performed some relevant testing on a real device and/or simulator
I updated only the english localization source files if needed
I added the // Ecosia: helper comments where needed
Double checked analytics

github-actions · 2024-09-09T14:38:51Z

PR Reviewer Guide 🔍

(Review updated until commit `18e7623`)

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review Concurrency Concern The use of `DispatchQueue.main.async` within a `Task` block at lines 43-51 might lead to race conditions or unexpected behavior due to the mix of concurrency paradigms (async/await with Grand Central Dispatch). Consider refactoring to fully leverage async/await for better readability and safety. Hardcoded Logic The method `start(with:)` contains hardcoded logic to check if the `OnboardingCardNTPExperiment` is enabled at lines 36-52. This could be abstracted into a separate method or class to handle feature flag checks, improving modularity and testability. Static Access The class `OnboardingCardNTPExperiment` heavily relies on static methods and properties, seen throughout the file. This design choice can make unit testing difficult and could be refactored to use instance methods with dependency injection for better testability and maintainability.

github-actions · 2024-09-09T14:38:53Z

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Use `[weak self]` in closures to prevent retain cycles and memory leaks To avoid potential retain cycles and memory leaks, consider using `[weak self]` in the closure where `self` is used inside the `Task` block. This ensures that `self` is not strongly captured by the closure, allowing it to be deallocated properly if needed. Client/Coordinators/Launch/LaunchCoordinator.swift [42-52] -Task { +Task { [weak self] in await FeatureManagement.fetchConfiguration() - DispatchQueue.main.async { [weak self] in + DispatchQueue.main.async { guard let self = self else { return } guard !OnboardingCardNTPExperiment.isEnabled else { self.parentCoordinator?.didFinishLaunch(from: self) return } self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) } } Suggestion importance[1-10]: 10 Why: Using `[weak self]` in closures is a best practice to prevent retain cycles and potential memory leaks. This suggestion is crucial for maintaining the application's memory efficiency and stability.	10
Best practice	Ensure UI updates are explicitly dispatched to the main thread to clarify intent and improve readability To ensure that the UI updates are performed on the main thread, consider using `DispatchQueue.main.async` directly in the `else` block where the UI update is required, rather than wrapping the entire block. This makes it clear that only the UI update needs to be dispatched to the main thread, not the entire logic. Client/Coordinators/Launch/LaunchCoordinator.swift [44-52] -DispatchQueue.main.async { [weak self] in - guard let self = self else { return } - guard !OnboardingCardNTPExperiment.isEnabled else { +guard let self = self else { return } +guard !OnboardingCardNTPExperiment.isEnabled else { + DispatchQueue.main.async { self.parentCoordinator?.didFinishLaunch(from: self) - return } + return +} +DispatchQueue.main.async { self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) } Suggestion importance[1-10]: 8 Why: This suggestion improves code clarity by explicitly dispatching only UI updates to the main thread, which is a best practice in Swift. It enhances readability and ensures correct thread usage.	8
Possible issue	Add error handling for asynchronous operations to improve robustness Consider handling potential exceptions or errors that might occur during the asynchronous operation within the `Task` block. This ensures that the application can gracefully handle situations where the `FeatureManagement.fetchConfiguration()` might fail due to network issues or other unexpected errors. Client/Coordinators/Launch/LaunchCoordinator.swift [42-52] Task { - await FeatureManagement.fetchConfiguration() - DispatchQueue.main.async { [weak self] in - guard let self = self else { return } - guard !OnboardingCardNTPExperiment.isEnabled else { - self.parentCoordinator?.didFinishLaunch(from: self) - return + do { + try await FeatureManagement.fetchConfiguration() + DispatchQueue.main.async { [weak self] in + guard let self = self else { return } + guard !OnboardingCardNTPExperiment.isEnabled else { + self.parentCoordinator?.didFinishLaunch(from: self) + return + } + self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) } - self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) + } catch { + print("Error fetching configuration: \(error)") } } Suggestion importance[1-10]: 9 Why: Adding error handling for the asynchronous operation is crucial for robustness, especially in network-related tasks where failures are common. This suggestion correctly addresses a potential issue and improves the code's reliability.	9
Maintainability	Refactor nested guard statements into a single guard statement to enhance code clarity To improve code readability and maintainability, consider refactoring the nested guard statements into a single guard statement with multiple conditions combined using logical operators. Client/Coordinators/Launch/LaunchCoordinator.swift [45-49] -guard let self = self else { return } -guard !OnboardingCardNTPExperiment.isEnabled else { - self.parentCoordinator?.didFinishLaunch(from: self) +guard let self = self, !OnboardingCardNTPExperiment.isEnabled else { + if OnboardingCardNTPExperiment.isEnabled { + self.parentCoordinator?.didFinishLaunch(from: self) + } return } Suggestion importance[1-10]: 7 Why: The refactoring of nested guard statements into a single statement improves code readability and maintainability. However, it is not a critical change, hence a moderate score.	7

lucaschifino · 2024-09-09T15:19:49Z

Client/Coordinators/Launch/LaunchCoordinator.swift

+            // TODO: Refactor `FeatureManagement.fetchConfiguration()` pre-condition - maybe a notification from FeatureManagement?
+            Task {
+                await FeatureManagement.fetchConfiguration()
+                DispatchQueue.main.async { [weak self] in
+                    guard let self = self else { return }
+                    guard !OnboardingCardNTPExperiment.isEnabled else {
+                        self.parentCoordinator?.didFinishLaunch(from: self)
+                        return
+                    }
+                    self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen)
+                }
+            }


For now this is working, but I don't think it looks great as it refreshes Unleash one extra time and mixes two concurrency patterns.

Maybe this could be some sort of Notification sent by FeatureManagement whenever it is finished so that places in the code that depend on it can use to make sure it is initialised.

We'll need to think about it thoroughly. For the time being, given the experiment context, I thought about leaving it as is. As a next step, I made a DRAFT RFC in our Confluence page that will be document with a proposed solution for us to comment on and eventually apply.

lucaschifino · 2024-09-09T15:21:14Z

Client/Ecosia/UI/NTP/BookmarkNudge/NTPBookmarkNudgeCellViewModel.swift

@@ -58,7 +58,7 @@ extension NTPBookmarkNudgeCellViewModel: HomepageViewModelProtocol {
    }

    var isEnabled: Bool {
-        User.shared.showsBookmarksNTPNudgeCard()
+        User.shared.showsBookmarksNTPNudgeCard() // TODO: Check why this logic is broken (`firstTime` false the first time it's executed)


I believe this might be related with the change asynchronous code to present onboarding, but bookmarks nudge card logic is broken since when it gets here the first time firstTime is already false for the user.

The issue here is not encountered, especially after the latest changes performed.

lucaschifino · 2024-09-09T15:21:58Z

Client/Ecosia/UI/NTP/OnboardingCard/NTPOnboardingCardCellViewModel.swift

+        let item = NSCollectionLayoutItem(layoutSize: itemSize)
+
+        let groupSize = NSCollectionLayoutSize(widthDimension: .fractionalWidth(1.0),
+                                               heightDimension: .estimated(128)) // TODO: Make cell automatically size it's height


Struggled a bit to make it resize it's height automatically, even setting up all constraints. Must be missing something but did not have the proper time to debug.

Solved ✅ . Adding this comment to track it 👍 . The issue was the right .estimated()dimension to assign to both the item and the group.

lucaschifino · 2024-09-09T15:27:01Z

Client/Ecosia/UI/NTP/OnboardingCard/NTPOnboardingCardCellViewModel.swift

+//        OnboardingCardNTPExperiment.shouldShowCard
+        true // TODO: Why is there a `UICollectionViewCompositionalLayout` whenever this is false?


Getting a strange error whenever this is false which seems to be related to the UICollectionViewCompositionalLayout. Cannot reproduce this on other similar cells like NTPBookmarkNudgeCellViewModel and couldn't find differences in setup 🤔

Solved ✅ . The root cause was essentially the order by which the viewModels in the HomepageViewController where defined. Added a supporting comment as well.

Assuming all cards will be shown at the top

Better handling of show/hide as well as minor cosmetic fixes

d4r1091 · 2024-09-24T13:22:22Z

Final review with configurable components

Image	Image

In-depth understanding

Reusable Component:

NTPConfigurableNudgeCardCell:
- A new UICollectionViewCell subclass that is reusable, and configurable via a view model.
- Configurable components include:
  - Title and description labels.
  - Optional image support.
  - Action button with customizable text.
  - Close button with the ability to toggle visibility.
- Implements basic accessibility support, setting labels and traits dynamically based on the view model data.
NTPConfigurableNudgeCardCellViewModel:
- A ViewModel responsible for providing the necessary data to configure the NTPConfigurableNudgeCardCell.
- Fields include:
  - Title, description, button text, image (optional), and visibility of the close button.
  - Unique card identifier and associated card type (HomepageSectionType).
Delegate Protocol (NTPConfigurableNudgeCardCellDelegate):
- Defines methods to handle user interactions, such as dismissing the card or performing an action triggered by the button.

Example with the Onboarding Card use case

Introduced NTPOnboardingCardCell which inherits from NTPConfigurableNudgeCardCell.
NTPOnboardingCardViewModel extends NTPConfigurableNudgeCardCellViewModel and adds logic specific to the onboarding experience, such as toggling whether the onboarding card should be shown. Every new ViewModel will need that. Decided to keep conformity to the Firefox architecture (a bit convoluted, from my perspective)
Uses a feature flag (OnboardingCardNTPExperiment.shouldShowCard) to determine whether to enable the onboarding card.

Priority mechanism

The priority mechanism is handled purely by cardsPrioritySectionTypes.
All you need to do is add another card type there, which will be displayed last. The FIFO approach handles it. The first one in the array is the one with highest priority.

github-actions · 2024-09-24T13:23:56Z

Persistent review updated to latest commit 18e7623

github-actions · 2024-09-24T13:24:00Z

PR Code Suggestions ✨

Category	Suggestion	Score
Best practice	Use `[weak self]` in closures to prevent retain cycles and memory leaks To avoid potential retain cycles and memory leaks, consider using `[weak self]` in the closure where `self` is used inside the `Task` block. This ensures that `self` is not strongly captured by the closure, allowing it to be deallocated properly if needed. Client/Coordinators/Launch/LaunchCoordinator.swift [42-52] -Task { +Task { [weak self] in await FeatureManagement.fetchConfiguration() - DispatchQueue.main.async { [weak self] in + DispatchQueue.main.async { guard let self = self else { return } guard !OnboardingCardNTPExperiment.isEnabled else { self.parentCoordinator?.didFinishLaunch(from: self) return } self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) } } Suggestion importance[1-10]: 10 Why: Using `[weak self]` in closures is a best practice to prevent retain cycles and potential memory leaks. This suggestion is crucial for maintaining the application's memory efficiency and stability.	10
Best practice	Ensure UI updates are explicitly dispatched to the main thread to clarify intent and improve readability To ensure that the UI updates are performed on the main thread, consider using `DispatchQueue.main.async` directly in the `else` block where the UI update is required, rather than wrapping the entire block. This makes it clear that only the UI update needs to be dispatched to the main thread, not the entire logic. Client/Coordinators/Launch/LaunchCoordinator.swift [44-52] -DispatchQueue.main.async { [weak self] in - guard let self = self else { return } - guard !OnboardingCardNTPExperiment.isEnabled else { +guard let self = self else { return } +guard !OnboardingCardNTPExperiment.isEnabled else { + DispatchQueue.main.async { self.parentCoordinator?.didFinishLaunch(from: self) - return } + return +} +DispatchQueue.main.async { self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) } Suggestion importance[1-10]: 8 Why: This suggestion improves code clarity by explicitly dispatching only UI updates to the main thread, which is a best practice in Swift. It enhances readability and ensures correct thread usage.	8
Possible issue	Add error handling for asynchronous operations to improve robustness Consider handling potential exceptions or errors that might occur during the asynchronous operation within the `Task` block. This ensures that the application can gracefully handle situations where the `FeatureManagement.fetchConfiguration()` might fail due to network issues or other unexpected errors. Client/Coordinators/Launch/LaunchCoordinator.swift [42-52] Task { - await FeatureManagement.fetchConfiguration() - DispatchQueue.main.async { [weak self] in - guard let self = self else { return } - guard !OnboardingCardNTPExperiment.isEnabled else { - self.parentCoordinator?.didFinishLaunch(from: self) - return + do { + try await FeatureManagement.fetchConfiguration() + DispatchQueue.main.async { [weak self] in + guard let self = self else { return } + guard !OnboardingCardNTPExperiment.isEnabled else { + self.parentCoordinator?.didFinishLaunch(from: self) + return + } + self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) } - self.presentIntroOnboarding(with: manager, isFullScreen: isFullScreen) + } catch { + print("Error fetching configuration: \(error)") } } Suggestion importance[1-10]: 9 Why: Adding error handling for the asynchronous operation is crucial for robustness, especially in network-related tasks where failures are common. This suggestion correctly addresses a potential issue and improves the code's reliability.	9
Maintainability	Refactor nested guard statements into a single guard statement to enhance code clarity To improve code readability and maintainability, consider refactoring the nested guard statements into a single guard statement with multiple conditions combined using logical operators. Client/Coordinators/Launch/LaunchCoordinator.swift [45-49] -guard let self = self else { return } -guard !OnboardingCardNTPExperiment.isEnabled else { - self.parentCoordinator?.didFinishLaunch(from: self) +guard let self = self, !OnboardingCardNTPExperiment.isEnabled else { + if OnboardingCardNTPExperiment.isEnabled { + self.parentCoordinator?.didFinishLaunch(from: self) + } return } Suggestion importance[1-10]: 7 Why: The refactoring of nested guard statements into a single statement improves code readability and maintainability. However, it is not a critical change, hence a moderate score.	7

ecotopian

Great work! ✨

No red flags, but tbh I am not entirely happy about the nested async call to Unleash. Maybe you find a clever way on bundling this with an overall start-up routine, because I assume this functionality is there to stay (e.g. controlling what's shown on first start via a flag)

I also added a few minor comments for wording, no blockers

ecotopian · 2024-09-26T12:57:37Z

Client/Coordinators/Browser/BrowserCoordinator.swift

@@ -279,9 +279,10 @@ class BrowserCoordinator: BaseCoordinator,
        browserViewController.presentIntroViewController()
    }

-    private func showIntroOnboarding() {
+    // Ecosia: Add `forceSkipExperiment` - used for `OnboardingCardNTPExperiment`
+    private func showIntroOnboarding(forceSkipExperiment: Bool = false) {


just a minor comment: skipExperiment already says what the method should do, no real reason to use force, or is there? ⚔️

The real reason behind it. I guess it was added mostly to advocate the imperative instruction. Can update 👍

ecotopian · 2024-09-26T13:03:01Z

Client/Coordinators/Launch/LaunchCoordinator.swift

+            }
+            // TODO: Refactor `FeatureManagement.fetchConfiguration()` pre-condition - maybe a notification from FeatureManagement?
+            Task {
+                await FeatureManagement.fetchConfiguration()


I am wondering, what's seen on screen while this is being executed? Slightly relates to Luca's comment from line 52, but I would expect that at this point we have fetched the Unleash configuration already, so that we can make a synchronous decision.

Don't we still have a LoadingScreen that's used when we get an invite code? We could bundle all async start up tasks there.

I agree with all the above and YES, it does require a refactor as we are all far from being happy about it 😅 .
When I was shown that, it already sparked some thoughts about a possible refactor immediately but then, eventually, I decided to leave it as is to speed up the data gathering.
I replied to Luca's handover comment about that, deciding eventually to leave it as is.

I tackled next steps already, creating a DRAFT RFC where I'm collecting info regarding the improvement.
If you are OK with that too, I would be more than happy to present some of the improvements I thought about when will publish the RFC.

ecotopian · 2024-09-26T13:06:48Z

Client/Ecosia/Analytics/Analytics.swift

@@ -306,7 +313,7 @@ final class Analytics: AnalyticsProtocol {
        guard let page else {
            return
        }
-        let event = Structured(category: Category.intro.rawValue,
+        let event = Structured(category: OnboardingCardNTPExperiment.analyticsIntroCategory ?? Category.intro.rawValue,


Matter of taste, but I am not really fond of this hidden fallback logic which relies on explicit knowledge in an allegedly agnostic function. Maybe we can have category a function parameter, but with .intro as default value. Then the caller has to decide explicitly which category to use

Agree with that. I believe that was added for the simplicity of the experiment.

d4r1091 · 2024-09-26T15:22:08Z

Will go ahead and merge it as approved already. My comments were supporting comments, alongside a single one tackled 👍

lucaschifino marked this pull request as draft September 9, 2024 14:38

lucaschifino commented Sep 9, 2024

View reviewed changes

d4r1091 force-pushed the ls-mob-2759-ntp-onboarding-card branch 2 times, most recently from 3fb9d4d to c566aa7 Compare September 18, 2024 14:41

lucaschifino added 21 commits September 23, 2024 16:00

Create OnboardingCardNTPExperiment

f603f12

Create NTPOnboardingCardCell

bd7565c

Adjust onboarding card UI

46241de

Adjust onboarding card constraints

2ae78df

Using correct translations

dd11dea

Temporarily point to ios-core feature branch

5efc188

Use actual Unleash flag

a66a32b

Not present onboarding if on experiment

cec5666

Remove NTP tooltip if on experiment

3745974

Add TODO

bd958f8

Add card dismissed user defaults

9028cdc

Set card dismissed

7984bcb

Show onboarding on card click

491f63b

Add TODO

dee519f

Change onboarding analytics category when on experiment

027e439

Hide skip button when on experiment

83adb87

Add Analytics tracking

8b2d65a

Add abb test context to resume and launch.

9c00e0b

Make MockBrowserCoordinator conform to updated protocol

7a046e7

Fix LaunchCoordinator intro tests

1ffd26c

Fix Ecosia ViewModel and Tooltip tests

841c19f

d4r1091 added 12 commits September 23, 2024 16:20

UI updates to Onboarding Card Cell. Fixed autoresizing

5541802

Fix order of ViewModels

e34f10b

Update SPM

0db6958

Fix button color in onboarding card

1ae7800

Remove custom bottom spacing for better UI look

50f8f32

SPM Update

dec02b4

Add card prioritization mechanism.

f57e4af

Assuming all cards will be shown at the top

Revert tests

ce08512

Update after rebase

a0031a3

Review NTPOnboardingCardCell to add the image

808e637

Made Nudge Cards configurable

57b0f5b

Add accessibility to the configurable nudge card action button

ac9ab67

d4r1091 force-pushed the ls-mob-2759-ntp-onboarding-card branch from ce631bb to ac9ab67 Compare September 24, 2024 12:14

d4r1091 added 3 commits September 24, 2024 14:32

Update on image handling

4c1e311

SPM Update

fe78731

Review structure readding a dedicated view model

18e7623

Better handling of show/hide as well as minor cosmetic fixes

d4r1091 marked this pull request as ready for review September 24, 2024 13:22

ecotopian previously approved these changes Sep 26, 2024

View reviewed changes

Update forceSkipExperiment in skipExperiment

c501aa5

d4r1091 requested a review from ecotopian September 26, 2024 14:51

d4r1091 dismissed ecotopian’s stale review via c501aa5 September 26, 2024 14:52

d4r1091 merged commit 3476fd7 into main Sep 26, 2024
2 checks passed

d4r1091 deleted the ls-mob-2759-ntp-onboarding-card branch September 26, 2024 15:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MOB-2759] Onboarding card NTP Experiment #778

[MOB-2759] Onboarding card NTP Experiment #778

lucaschifino commented Sep 9, 2024 •

edited by d4r1091

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

lucaschifino Sep 9, 2024

d4r1091 Sep 18, 2024

lucaschifino Sep 9, 2024

d4r1091 Sep 18, 2024

lucaschifino Sep 9, 2024

d4r1091 Sep 16, 2024

lucaschifino Sep 9, 2024

d4r1091 Sep 17, 2024

d4r1091 commented Sep 24, 2024 •

edited

Loading

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024 •

edited

Loading

ecotopian left a comment

ecotopian Sep 26, 2024

d4r1091 Sep 26, 2024

ecotopian Sep 26, 2024

d4r1091 Sep 26, 2024

ecotopian Sep 26, 2024

d4r1091 Sep 26, 2024

d4r1091 commented Sep 26, 2024

		// OnboardingCardNTPExperiment.shouldShowCard
		true // TODO: Why is there a `UICollectionViewCompositionalLayout` whenever this is false?

[MOB-2759] Onboarding card NTP Experiment #778

[MOB-2759] Onboarding card NTP Experiment #778

Conversation

lucaschifino commented Sep 9, 2024 • edited by d4r1091 Loading

Context

Approach

Before merging

Checklist

github-actions bot commented Sep 9, 2024 • edited Loading

PR Reviewer Guide 🔍

(Review updated until commit 18e7623)

github-actions bot commented Sep 9, 2024 • edited Loading

PR Code Suggestions ✨

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d4r1091 commented Sep 24, 2024 • edited Loading

Final review with configurable components

In-depth understanding

Reusable Component:

Example with the Onboarding Card use case

Priority mechanism

github-actions bot commented Sep 24, 2024

github-actions bot commented Sep 24, 2024 • edited Loading

PR Code Suggestions ✨

ecotopian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d4r1091 commented Sep 26, 2024

lucaschifino commented Sep 9, 2024 •

edited by d4r1091

Loading

github-actions bot commented Sep 9, 2024 •

edited

Loading

(Review updated until commit `18e7623`)

github-actions bot commented Sep 9, 2024 •

edited

Loading

d4r1091 commented Sep 24, 2024 •

edited

Loading

github-actions bot commented Sep 24, 2024 •

edited

Loading