[MOB-2959] Braze IAM for Newsletter NTP card

[lucaschifino]

MOB-2959

Context

After spiking Braze IAM, we're setting up an experiment that uses it.

Approach

• Refactor Braze integration and move it to Client
• Add Braze IAM
• Add Newsletter NTP card connected to an experiment
• Link card and Braze IAM via a custom event
• Track both the card and the IAM

Other

Before merging

• Merge https://github.com/ecosia/ios-core/pull/169 and update this PR

Checklist

• I performed some relevant testing on a real device and/or simulator
• I updated only the english localization source files if needed -> [MOB-2959] Newsletter card english strings to be translated #813
• I added the // Ecosia: helper comments where needed
• I made sure that any change to the Analytics events included in PR won't alter current analytics (e.g. new users, upgrading users)

[github-actions]

PR Reviewer Guide 🔍

(Review updated until commit f84532a)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Error Handling The method initialize in BrazeService class catches errors but only prints them. Consider handling these errors more robustly or propagating them to allow for better error management and recovery. Error Handling The method requestAPNConsentIfNeeded catches errors but only logs them. It might be beneficial to handle these errors more explicitly to avoid silent failures and improve reliability. Code Duplication The methods brazeIAM and newsletterCardExperiment in Analytics.swift are very similar and could potentially be refactored to reduce code duplication.

[github-actions]

PR Code Suggestions ✨

Latest suggestions up to f84532a
Explore these optional code suggestions:

Category	Suggestion	Score
Possible bug	Add error handling for potential exceptions during Braze service initialization Consider handling the scenario where BrazeService.shared.initialize might throw an error that isn't caught, which could lead to unhandled exceptions during the app initialization process. Client/Ecosia/Braze/BrazeService.swift [195] -await BrazeService.shared.initialize(notificationCenterDelegate: self) +do { + try await BrazeService.shared.initialize(notificationCenterDelegate: self) +} catch { + // Handle initialization error appropriately +} Suggestion importance[1-10]: 8 Why: Adding error handling for the initialization of Braze service is crucial to prevent the app from crashing due to unhandled exceptions, enhancing the robustness and reliability of the app initialization process.	8
	Implement error handling for device token registration in Braze service Ensure that the BrazeService.shared.registerDeviceToken method handles potential failures or exceptions when registering the device token, as the current implementation does not account for these scenarios. Client/Ecosia/Braze/BrazeService.swift [405] -BrazeService.shared.registerDeviceToken(deviceToken) +do { + try BrazeService.shared.registerDeviceToken(deviceToken) +} catch { + // Handle registration error appropriately +} Suggestion importance[1-10]: 7 Why: Implementing error handling for device token registration is important to ensure that potential failures are managed properly, which can prevent issues related to push notification functionality.	7
	Enhance robustness of APN consent request handling in Braze service Refactor the BrazeService.shared.requestAPNConsent method to handle the potential throwing of exceptions more robustly, especially considering it's used in an async context which might lead to unhandled promise rejections. Client/Ecosia/Braze/BrazeService.swift [26] -let granted = try await BrazeService.shared.requestAPNConsent(notificationCenterDelegate: delegate) +let granted: Bool +do { + granted = try await BrazeService.shared.requestAPNConsent(notificationCenterDelegate: delegate) +} catch { + // Handle consent request error appropriately + granted = false +} Suggestion importance[1-10]: 7 Why: Enhancing error handling for the APN consent request is critical as it directly affects the app's ability to manage push notification permissions, which is a key functionality for user engagement.	7
Enhancement	Provide a default API key for Braze service to avoid potential issues with empty configuration Add a fallback or default value for BrazeService.apiKey in case it is not found in the environment variables, to prevent the app from using an empty or invalid API key. Client/Ecosia/Braze/BrazeService.swift [18] -private static var apiKey = EnvironmentFetcher.valueFromMainBundleOrProcessInfo(forKey: "BRAZE_API_KEY") ?? "" +private static var apiKey = EnvironmentFetcher.valueFromMainBundleOrProcessInfo(forKey: "BRAZE_API_KEY") ?? "default_api_key" Suggestion importance[1-10]: 5 Why: Providing a default API key as a fallback is a good practice to ensure that the Braze service has a valid API key to operate, preventing potential runtime errors due to missing configuration. However, this should be handled with caution to avoid using inappropriate keys in production.	5

---

Previous suggestions

Suggestions up to commit 8059c7a

Category	Suggestion	Score
Possible bug	Add nil-check for notificationCenterDelegate to prevent potential delegate assignment errors Consider handling the case where notificationCenterDelegate is nil before setting it as the delegate to avoid potential runtime errors. Client/Ecosia/Braze/BrazeService.swift [93] -center.delegate = notificationCenterDelegate +if let delegate = notificationCenterDelegate { + center.delegate = delegate +} Suggestion importance[1-10]: 7 Why: Adding a nil-check before setting the delegate can prevent runtime errors if notificationCenterDelegate is nil. This is a good defensive programming practice.	7
Enhancement	Implement error handling for asynchronous ID update in registerDeviceToken Add error handling for the await self?.updateID(self?.userId) call within the registerDeviceToken method to manage potential failures gracefully. Client/Ecosia/Braze/BrazeService.swift [42-44] Task.detached(priority: .medium) { [weak self] in - await self?.updateID(self?.userId) + do { + try await self?.updateID(self?.userId) + } catch { + // Handle error, e.g., logging or retrying + } } Suggestion importance[1-10]: 6 Why: Implementing error handling for the asynchronous ID update operation enhances robustness and reliability of the registerDeviceToken method.	6
	Strengthen the validation of apiKey in the configuration setup Validate the apiKey in getBrazeConfiguration method to ensure it meets expected criteria (e.g., length, format) beyond just being non-empty. Client/Ecosia/Braze/BrazeService.swift [136] -guard !apiKey.isEmpty else { throw Error.invalidConfiguration } +guard !apiKey.isEmpty, apiKey.count > 10 else { throw Error.invalidConfiguration } Suggestion importance[1-10]: 4 Why: Strengthening the validation of apiKey is beneficial for ensuring the configuration's integrity, but the impact is moderate as it primarily adds a simple check for length.	4
Possible issue	Ensure thread safety for notificationAuthorizationStatus updates Ensure that the notificationAuthorizationStatus is thread-safe if accessed from multiple threads, possibly by using synchronization mechanisms or dispatch queues. Client/Ecosia/Braze/BrazeService.swift [100] -notificationAuthorizationStatus = currentStatus +DispatchQueue.main.async { + self.notificationAuthorizationStatus = currentStatus +} Suggestion importance[1-10]: 5 Why: Ensuring thread safety for updating notificationAuthorizationStatus is important, but the provided solution assumes multi-thread access which might not be necessary if the code runs on a single thread.	5

[github-actions]

Persistent review updated to latest commit f84532a

[d4r1091]

Great re-implementation 👌 .
Added some clarification points but nothing that strictly prevents this overall work from being merged 💪

[d4r1091]

I see the point here 👌 - I guess having them with the fatalError was an easier update considering the state of the class.
Wondering if we'd invest time and make it as protocol.

[lucaschifino]

I think there might be complexities in adding one more protocol since the HomepageViewModelProtocol is already used. I like the idea that this is a superclass that can be extended and has the majority of the logic, just like you initially implemented. The main reason I changed to overriding methods is because those custom contents are then much more organised as part of the subclass file itself instead of somewhat hidden in the initialisation (which in turn happens on the already big HomepageViewModel).

What do you think? Do you see any specific advantages to using protocol here instead?

[d4r1091]

Agreed with @lucaschifino that given all the challenges we had with the Braze integration and to make this work, we'd consider a further abstraction of the Braze implementation into another set of work.
Let's go 🚀

[lucaschifino]

@d4r1091 Found some issues with APN after the refactor when doing some last tests, had to add the notification options and update the status in one more place. Can you please take another look?

[d4r1091]

Gotcha 👍
👌