Abstract layer for Push Notifications and Deep Linking #263

rnr · 2024-01-30T14:03:20Z

Draft PR to review and discuss an abstract implementation of Push notifications and deep link processing

What was done

added PushNotificationsManager and DeepLinkManager to DI
added protocols for PushNotificationsProvider and PushNotificationsListener
added protocol for DeepLinkService
added DeepLink and PushLink classes, which are used to redirect processes from deep link and from push notifications (DeepLink does not currently contain all possible types)
added configurations for Braze and Branch services
added empty implementation for FCM and Braze providers and listeners
added empty implementation for Branch deep linking service

How `PushNotificationsManager` works

PushNotificationsManager creates arrays of providers and listeners given the configuration (which service(s) are enabled). When any AppDelegate method is called (e.g. registerWithDeviceToken, getRemoteNotification), PushNotificationsManager translates that call to providers or listeners. Each provider/listener has its own initialization and processing functions. A listener can also call the DeepLinkManager to process the received notification data and trigger a redirect to the appropriate view.

How `DeepLinkManager` works

DeepLinkManager contains one service that will handle deep links. The type of redirect will be determined by the DeepLinkType from the DeepLink or PushLink classes. The service processes the URL and, if it is a supported deep link, calls DeepLinkManager.processDeepLink() with parameters and the manager performs the redirect.

chore: added abstract layer for push notifications

saeedbashir · 2024-02-02T11:00:06Z

@rnr The suggested approach is very close to what we have on the production app, and the approach is modular and can adopt new providers very easily. We can go with this approach.

saeedbashir · 2024-02-07T14:58:01Z

OpenEdX/AppDelegate.swift

@@ -70,6 +75,13 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
        open url: URL, options: [UIApplication.OpenURLOptionsKey: Any] = [:]
    ) -> Bool {
        if let config = Container.shared.resolve(ConfigProtocol.self) {


How about using guard let here for better indentation thoughts?

saeedbashir · 2024-02-07T15:02:09Z

OpenEdX/AppDelegate.swift

@@ -125,4 +137,32 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
        window?.rootViewController = RouteController()
    }

+    // Push Notifications
+    func application(_ application: UIApplication, didRegisterForRemoteNotificationsWithDeviceToken deviceToken: Data) {
+        if let pushManager = Container.shared.resolve(PushNotificationsManager.self) {


How about using guard let here?

saeedbashir · 2024-02-07T15:02:28Z

OpenEdX/AppDelegate.swift

+        }
+    }
+    func application(_ application: UIApplication, didFailToRegisterForRemoteNotificationsWithError error: Error) {
+        if let pushManager = Container.shared.resolve(PushNotificationsManager.self) {


How about using guard let here and same for the following push notifications methods?

saeedbashir · 2024-02-07T15:20:38Z

OpenEdX/Managers/DeepLinkManager/DeepLinkManager.swift

+
+    // Init manager
+    public init(config: ConfigProtocol) {
+        self.service = self.serviceFor(config: config)


The DeeplinkManager can register just one service Branch service. How about configuring it to register multiple services thoughts?

saeedbashir · 2024-02-07T15:21:22Z

OpenEdX/Managers/DeepLinkManager/DeepLinkManager.swift

+
+    // Configure services
+    func configureDeepLinkService(launchOptions: [UIApplication.LaunchOptionsKey: Any]?) {
+        if let service = service {


How about using guard let here?

saeedbashir · 2024-02-07T15:34:43Z

OpenEdX/Managers/PushNotificationsManager/PushNotificationsManager.swift

+    func didReceiveRemoteNotification(userInfo: [AnyHashable: Any]) {
+       guard let dictionary = userInfo as? [String: Any], shouldListenNotification(userinfo: userInfo) else { return }
+       let link = PushLink(dictionary: dictionary)
+       if let deepLinkManager = Container.shared.resolve(DeepLinkManager.self) {


How about moving it with guard let dictionary?

saeedbashir · 2024-02-07T15:35:43Z

OpenEdX/Managers/PushNotificationsManager/PushNotificationsManager.swift

+        self.providers = self.providersFor(config: config)
+        self.listeners = self.listenersFor(config: config)


self is redundant.

saeedbashir · 2024-02-07T15:36:48Z

OpenEdX/Managers/PushNotificationsManager/PushNotificationsManager.swift

+    }
+
+    private func providersFor(config: ConfigProtocol) -> [PushNotificationsProvider] {
+        var rProviders: [PushNotificationsProvider] = []


How about renaming it to providers, pushProviders or pProviders? as rProviders doesn't go with the feature.

saeedbashir · 2024-02-07T15:37:24Z

OpenEdX/Managers/PushNotificationsManager/PushNotificationsManager.swift

+    }
+
+    private func listenersFor(config: ConfigProtocol) -> [PushNotificationsListener] {
+        var rListeners: [PushNotificationsListener] = []


How about renaming it to listeners, pushlisteners, or pListeners? as rListeners doesn't go with the feature.

saeedbashir · 2024-02-07T15:40:02Z

OpenEdX/Managers/PushNotificationsManager/Listeners/BrazeListener.swift

+
+import Foundation
+
+class BrazeListener: PushNotificationsListener {


I guess it should also implement didReceiveRemoteNotification

#263 (comment)

saeedbashir · 2024-02-09T04:53:18Z

OpenEdX/AppDelegate.swift

+        if config.facebook.enabled {
+            ApplicationDelegate.shared.application(
+                app,
+                open: url,
+                sourceApplication: options[UIApplication.OpenURLOptionsKey.sourceApplication] as? String,
+                annotation: options[UIApplication.OpenURLOptionsKey.annotation]
+            )
+        }


Although it's not part of the underlying PR, but as we are, how about fixing it as well, it can return false, whereas the method should have been called for Google or Microsoft. It would be nice if we see if the URL is for Facebook only then it will be returning the status.

saeedbashir · 2024-02-09T04:54:25Z

OpenEdX/AppDelegate.swift

+        didReceiveRemoteNotification userInfo: [AnyHashable: Any],
+        fetchCompletionHandler completionHandler: @escaping (UIBackgroundFetchResult) -> Void
+    ) {
+        if let pushManager = Container.shared.resolve(PushNotificationsManager.self) {


It would be nice if guard let can be used here as well.

saeedbashir · 2024-02-09T04:54:28Z

OpenEdX/AppDelegate.swift

+
+    // Deep link
+    func configureDeepLinkServices(launchOptions: [UIApplication.LaunchOptionsKey: Any]?) {
+        if let deepLinkManager = Container.shared.resolve(DeepLinkManager.self) {


It would be nice if guard let can be used here as well.

saeedbashir · 2024-02-09T04:56:47Z

OpenEdX/Managers/DeepLinkManager/DeepLinkManager.swift

+
+    // Init manager
+    public init(config: ConfigProtocol) {
+        services = servicesFor(config: config)


How about renaming servicesFor to something like configureServices(_ with config) and call it. The method will populate services for all enabled services?

I don't agree with this name - this func doesn't configure services but just inits and returns services for current config

saeedbashir · 2024-02-09T04:59:04Z

OpenEdX/Managers/DeepLinkManager/DeepLinkManager.swift

+
+    // This method do redirect with link from push notification
+    func processLinkFromNotification(_ link: PushLink) {
+        if anyServiceEnabled {


I guess this check isn't valid because it's for deep link services not for the push notifications.

saeedbashir · 2024-02-09T14:22:03Z

OpenEdX/Managers/DeepLinkManager/DeepLinkManager.swift

+    }
+
+    private func servicesFor(config: ConfigProtocol) -> [DeepLinkService] {
+        var deepServices: [DeepLinkService] = []


We are not going with method name renaming then how about renaming it to services?

It's just a var to return from a function. Why is its name so important if you are asking me to rename it a second time?

I believe the names should be meaningful according to their usage. It doesn't matter whether they are local or instance variables.

It can be renamed to services or deeplinkServices as per the usage.

Do you see a big difference between deepServices and deeplinkServices? Is it not "meaningful" enough?
Here #263 (comment) you asked me to rename to 'pushProviders' - why not to 'pushnotificationProviders' to ' be meaningful according to their usage'? I'm just following your example.
Don't you think we're just wasting time here?

push is a term that is mostly used for push notifications. That's why it was suggested to use a shorter name, but deep isn't used for deep links. I don't think is time wasting, proper naming enhances the code readability.

I believe var deepServices of type [DeepLinkService] is enough 'readable' (especially since it's not used anywhere but these 5 lines of code)

hmm ok if you are reluctant to make this change, then lets go with it.

saeedbashir

LGTM 👍

rnr added 5 commits January 24, 2024 15:22

chore: added abstract layer for push notifications

a6fc40d

Merge pull request #2 from touchapp/push_notifications_abstract

077ae85

chore: added abstract layer for push notifications

chore: abstract layer for deep linking

c87f7cb

chore: renamed some func

3659d5c

chore: small refactor

931643a

rnr requested review from saeedbashir, volodymyr-chekyrta and forgotvas January 30, 2024 14:05

rnr changed the title ~~Abstract layer for push notifications~~ Abstract layer for push notifications and Deep Linking Jan 30, 2024

rnr changed the title ~~Abstract layer for push notifications and Deep Linking~~ Abstract layer for Push Notifications and Deep Linking Jan 30, 2024

Merge branch 'develop' into abstract_layer_for_push_notifications

6f90fc0

rnr added 2 commits February 2, 2024 12:30

Merge branch 'develop' into abstract_layer_for_push_notifications

92be947

chore: added RawStringExtractable to configs

a1c2a49

rnr marked this pull request as ready for review February 2, 2024 11:41

Merge branch 'develop' into abstract_layer_for_push_notifications

27c2553

forgotvas previously approved these changes Feb 5, 2024

View reviewed changes

saeedbashir requested changes Feb 7, 2024

View reviewed changes

chore: address review feedback

2b3f9a5

rnr dismissed forgotvas’s stale review via 2b3f9a5 February 8, 2024 13:20

rnr requested review from saeedbashir and forgotvas February 8, 2024 13:21

forgotvas previously approved these changes Feb 8, 2024

View reviewed changes

saeedbashir reviewed Feb 9, 2024

View reviewed changes

chore: address review feedback

3b93284

rnr dismissed forgotvas’s stale review via 3b93284 February 9, 2024 12:29

rnr requested a review from saeedbashir February 9, 2024 12:29

saeedbashir reviewed Feb 9, 2024

View reviewed changes

saeedbashir approved these changes Feb 9, 2024

View reviewed changes

forgotvas approved these changes Feb 9, 2024

View reviewed changes

saeedbashir approved these changes Feb 12, 2024

View reviewed changes

volodymyr-chekyrta approved these changes Feb 12, 2024

View reviewed changes

volodymyr-chekyrta merged commit 35e7cbe into openedx:develop Feb 12, 2024
3 checks passed

rnr deleted the abstract_layer_for_push_notifications branch February 29, 2024 09:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Abstract layer for Push Notifications and Deep Linking #263

Abstract layer for Push Notifications and Deep Linking #263

rnr commented Jan 30, 2024

saeedbashir commented Feb 2, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

saeedbashir Feb 7, 2024

rnr Feb 8, 2024

saeedbashir Feb 9, 2024

saeedbashir Feb 9, 2024

saeedbashir Feb 9, 2024

saeedbashir Feb 9, 2024

rnr Feb 9, 2024

saeedbashir Feb 9, 2024

saeedbashir Feb 9, 2024

saeedbashir Feb 9, 2024

rnr Feb 9, 2024

saeedbashir Feb 9, 2024

saeedbashir Feb 9, 2024

rnr Feb 9, 2024

saeedbashir Feb 9, 2024

rnr Feb 9, 2024

saeedbashir Feb 12, 2024

saeedbashir left a comment •

edited

Loading

		self.providers = self.providersFor(config: config)
		self.listeners = self.listenersFor(config: config)


		import Foundation

		class BrazeListener: PushNotificationsListener {

Abstract layer for Push Notifications and Deep Linking #263

Abstract layer for Push Notifications and Deep Linking #263

Conversation

rnr commented Jan 30, 2024

What was done

How PushNotificationsManager works

How DeepLinkManager works

saeedbashir commented Feb 2, 2024

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saeedbashir left a comment • edited Loading

Choose a reason for hiding this comment

How `PushNotificationsManager` works

How `DeepLinkManager` works

saeedbashir left a comment •

edited

Loading