feat: [FC-0047] FCM #461

volodymyr-chekyrta · 2024-06-12T08:59:39Z

This pull request introduces Firebase Cloud Messaging (FCM) to the project, enabling the functionality for receiving push notifications from Firebase.

Testing:
• Verified push notifications are received when the app is in the foreground, background, and terminated states.
• Confirmed the app correctly handles notification taps and displays relevant content.
• Ensured token generation and handling work as expected, including handling token refreshes.

Please use the Firebase console for testing; here is a set of all the required fields for navigation:

Notifications:

User has been enrolled
notification_type: enroll
course_id: Your course ID (example: course-v1:future+f1+2024)
User has been unenrolled
notification_type: unenroll
User has been added as a beta tester
notification_type: add_beta_tester
course_id: Your course ID (example: course-v1:future+f1+2024)
User has been removed from beta testers
notification_type: remove_beta_tester
course_id: Your course ID (example: course-v1:future+f1+2024)
Someone left a response in the user discussion/question
notification_type: forum_response
topic_id: Your Topic ID (example: course)
course_id: Your course ID (example: course-v1:future+f1+2024)
thread_id: Your thread ID (example: 66584e96f2195c001ad219c7)
comment_id: Your comment ID (example: 66585c9df2195c001ad219ce)
Someone left a comment in the user discussion/question
notification_type: forum_comment
topic_id: Your Topic ID (example: course)
course_id: Your course ID (example: course-v1:future+f1+2024)
thread_id: Your thread ID (example: 66583d13f2195c001a56a182)
parent_id: Your response ID (example: 66586579f2195c001ad219d8)
comment_id: Your comment ID (example: 6659ac92f2195c001bf37f77)

openedx-webhooks · 2024-06-12T08:59:44Z

Thanks for the pull request, @volodymyr-chekyrta! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

rnr · 2024-06-13T12:11:36Z

OpenEdX/AppDelegate.swift

+                    let pushManager = Container.shared.resolve(PushNotificationsManager.self)
+                    Messaging.messaging().delegate = pushManager
+                    UNUserNotificationCenter.current().delegate = pushManager
+                    UIApplication.shared.registerForRemoteNotifications()


@volodymyr-chekyrta This way didRegisterForRemoteNotificationsWithDeviceToken delegate method will fired only if cloudMessagingEnabled==true. If FCM is disabled then all other providers will unable to register device token.

Makes sense 👍
I've updated it for any provider ✅

let pushManager = Container.shared.resolve(PushNotificationsManager.self) if config.firebase.enabled { FirebaseApp.configure() if config.firebase.cloudMessagingEnabled { Messaging.messaging().delegate = pushManager UNUserNotificationCenter.current().delegate = pushManager } } if pushManager?.hasProviders == true { UIApplication.shared.registerForRemoteNotifications() }

rnr

LGTM

rnr · 2024-06-13T15:17:54Z

OpenEdX/Managers/PushNotificationsManager/PushNotificationsManager.swift

    private var providers: [PushNotificationsProvider] = []
    private var listeners: [PushNotificationsListener] = []

+    public var hasProviders: Bool {
+        return providers.count > 0


'return' isn't necessary in this case.

Yes. Done ✅

volodymyr-chekyrta · 2024-06-13T15:35:09Z

@saeedbashir, Kindly review the changes made in this pull request.
Thank you 🙌

saeedbashir · 2024-06-13T15:49:47Z

Please use the Firebase console for testing; here is a set of all the required fields for navigation:

Will review it tomorrow.

saeedbashir · 2024-06-14T02:52:47Z

@volodymyr-chekyrta Do I need to test this PR against the sandbox because the API https://courses.edx.org/api/mobile/v4/notifications/create-token/ (404) is returning 404 on the edX backend?

saeedbashir · 2024-06-14T02:36:25Z

Authorization/Authorization/Presentation/Registration/SignUpViewModel.swift

            analytics.userLogin(method: authMethod)
            isShowProgress = false
            router.showMainOrWhatsNewScreen(sourceScreen: sourceScreen)
+            NotificationCenter.default.post(name: .userAuthorized, object: nil)


This piece of code (register success) is redendant, how about moving it to a function, thoughts?

I don't understand why it is redundant.
It looks reasonable for me to be placed here.

The following code is same in both the success methods loginOrRegister and registerUser. Anyhow we can live with it because it's not changed under the scope of this PR.

let user = try await interactor.login(externalToken: response.token, backend: backend) analytics.identify(id: "\(user.id)", username: user.username, email: user.email) analytics.userLogin(method: authMethod) isShowProgress = false router.showMainOrWhatsNewScreen(sourceScreen: sourceScreen) NotificationCenter.default.post(name: .userAuthorized, object: nil)

Sure, we can address this later in some refactoring

saeedbashir · 2024-06-14T03:00:59Z

Core/Core/Network/RequestInterceptor.swift

+                        NotificationCenter.default.post(
+                            name: .userLoggedOut,
+                            object: nil,
+                            userInfo: [Notification.UserInfoKey.isForced: false]


I guess this is the forced logout case, if so then the value of isForced should be true.

Right. Fixed ✅

saeedbashir · 2024-06-14T03:21:11Z

Discussion/Discussion/Data/Network/DiscussionRepository.swift

@@ -176,7 +172,25 @@ public class DiscussionRepository: DiscussionRepositoryProtocol {
        }
    }

-    public func renameUsers(data: Data) async throws -> DataLayer.CommentsResponse {
+    private func renameThreadUser(data: Data) async throws -> DataLayer.ThreadList {


Are we doing this to format data for parsing in mobile?

Yes, it enables easier parsing of user avatar.

saeedbashir · 2024-06-14T03:37:11Z

OpenEdX/AppDelegate.swift

@@ -120,21 +141,34 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
        )
    }

-    @objc private func forceLogoutUser() {
+    @objc private func didUserAuthorize() {
+        guard Container.shared.resolve(ConfigProtocol.self)?.firebase.cloudMessagingEnabled == true else { return }


Regardless of the provider, if any provider available the synchronizeToken should be called and then the provider will decide what to do with the call.

Makes sense 👍
✅

saeedbashir · 2024-06-14T03:39:59Z

OpenEdX/AppDelegate.swift

+        if Container.shared.resolve(ConfigProtocol.self)?.firebase.cloudMessagingEnabled == true {
+            UNUserNotificationCenter.current().removeAllDeliveredNotifications()


Regardless of the provider, if any provider available the refreshToken() should be called and then the provider will decide what to do with the call.

Makes sense 👍
✅

saeedbashir · 2024-06-14T03:54:54Z

OpenEdX/Managers/PushNotificationsManager/Providers/FCMProvider.swift

+    func refreshToken() {
+        Messaging.messaging().deleteToken { error in


Any reason to delete the old token on logout, and create a new one? Will the user be still able to receive pushes while not login?

User should not receive push notifications after logging out.

I'm not sure about the requirements but in this case user may miss important notifications. Like if somehow user is logout, and an assignment due notification is sent from the server, the user won't be receiving the notification. The other case is, moderator replied to the user query/response in the discussion threads, the user won't be receiving the response.

By the current flow, if user is logging out, they are no longer interested in receiving notifications.
Users can still rely on email or calendar notifications.
We can change this flow in the future if we add some vital push notifications.

saeedbashir · 2024-06-14T04:01:04Z

OpenEdX/Managers/PushNotificationsManager/PushNotificationsManager.swift

+        let userInfo = response.notification.request.content.userInfo
+
+        // With swizzling disabled you must let Messaging know about the message, for Analytics
+        Messaging.messaging().appDidReceiveMessage(userInfo)


I guess the right place for this is FCMListener.

It requires extra abstraction for PushNotificationsProvider; it can create overhead.
For me, it seems ok to leave just 1 line here.
wdyt?

It won't required any abstraction as the abstraction is already there. The FCMListener: PushNotificationsListener is implementing the PushNotificationsListener protocol, you just need to implement optional method didReceiveRemoteNotification and thats it.

@saeedbashir, but if we override didReceiveRemoteNotification in FCMListener, the extension with default implementation (navigation) will stop working 🤷‍♂️.

We need one more function in PushNotificationsListener for this or refactor didReceiveRemoteNotification and not use it for navigation.

Strange, I guess it got missed while writing the abstraction layer. Ideally, every PushNotificationsListener should implement this piece of code. So first, the listener will check whether the push is for that listener (Braze, FCM etc) or not, and then do the required work before starting the navigation.

The other reason for giving each listener its own implementation is that maybe data will be received in a different format (hierarchy), and a parsing may be required to generate a PushLink from the data.

Thoughts?

Sure, I'll update it now, I think it makes sense

@saeedbashir I've updated implementation of the PushNotificationsManager, please check.

saeedbashir · 2024-06-14T04:03:43Z

OpenEdX/Managers/PushNotificationsManager/PushNotificationsManager.swift

+        willPresent notification: UNNotification
+    ) async -> UNNotificationPresentationOptions {
+        // With swizzling disabled you must let Messaging know about the message, for Analytics
+        Messaging.messaging().appDidReceiveMessage(notification.request.content.userInfo)


I guess the right place for this is FCMListner.

It requires extra abstraction for PushNotificationsProvider; it can create overhead.
For me, it seems ok to leave just 1 line here.
wdyt?

#461 (comment)

saeedbashir · 2024-06-14T04:18:18Z

OpenEdX/Router.swift

@@ -380,6 +380,10 @@ public class Router: AuthorizationRouter,
            lastVisitedBlockID: lastVisitedBlockID
        )
        navigationController.pushViewController(controller, animated: true)
+
+        DispatchQueue.main.asyncAfter(deadline: .now() + 1) {
+            Container.shared.resolve(PushNotificationsManager.self)?.performRegistration()


The push notification registration is happening from the course home screen, is that intended behavior?

Yes, it's a UX improvement.
Users automatically reject a push notification request if it appears when the app is launched.

volodymyr-chekyrta · 2024-06-14T08:40:09Z

@volodymyr-chekyrta Do I need to test this PR against the sandbox because the API https://courses.edx.org/api/mobile/v4/notifications/create-token/ (404) is returning 404 on the edX backend?

You cannot test this endpoint; it has not been merged yet.
For now, please use the Firebase console.

volodymyr-chekyrta · 2024-06-14T08:44:37Z

Hi @saeedbashir, thank you for the prompt feedback!
Feedback has been addressed, and PR is ready for the next pass 🙌

saeedbashir · 2024-06-20T09:55:18Z

OpenEdX/Managers/PushNotificationsManager/Listeners/FCMListener.swift

+    private lazy var deepLinkManager: DeepLinkManager? = {
+        Container.shared.resolve(DeepLinkManager.self)
+    }()


How about passing this as a parameter, thoughts?

Sure, why not
Done ✅

saeedbashir · 2024-06-20T09:55:59Z

OpenEdX/Managers/PushNotificationsManager/Listeners/FCMListener.swift

+
+    func didReceiveRemoteNotification(userInfo: [AnyHashable: Any]) {
+        // With swizzling disabled you must let Messaging know about the message, for Analytics
+        Messaging.messaging().appDidReceiveMessage(userInfo)


I guess it should move below the guard statement as the notification can be from the braze.

volodymyr-chekyrta · 2024-06-20T12:25:40Z

@saeedbashir, PR is ready for the next pass 🙌

saeedbashir · 2024-06-21T02:23:17Z

OpenEdX/Managers/PushNotificationsManager/Listeners/BrazeListener.swift

@@ -6,11 +6,26 @@
 //

 import Foundation
+import Swinject


Please remove this unused import.

saeedbashir · 2024-06-21T02:23:50Z

OpenEdX/Managers/PushNotificationsManager/Listeners/FCMListener.swift

@@ -6,8 +6,30 @@
 //

 import Foundation
+import Swinject


Please remove this unused import and after that we are good to go.

saeedbashir · 2024-06-21T02:26:08Z

OpenEdX/Managers/PushNotificationsManager/PushNotificationsManager.swift

@@ -9,56 +9,58 @@ import Foundation
 import Core
 import UIKit
 import Swinject


I guess we are safe to remove this import

saeedbashir · 2024-06-21T02:28:09Z

OpenEdX/Managers/PushNotificationsManager/Providers/FCMProvider.swift

+    private var storage = Container.shared.resolve(CoreStorage.self)!
+    private let api = Container.shared.resolve(API.self)!


I'm fine with this, but as we changed the dependency injection in the FCMListner, do we need to change it as well?

Yes, it makes sense to provide them as dependencies on the init lever 👍

saeedbashir · 2024-06-21T02:29:12Z

@saeedbashir, PR is ready for the next pass 🙌

@volodymyr-chekyrta I've left few comments, please take a look. After that we are good to go with this PR.

volodymyr-chekyrta · 2024-06-21T08:25:17Z

@saeedbashir, PR is ready for the next pass 🙌

@volodymyr-chekyrta I've left few comments, please take a look. After that we are good to go with this PR.

@saeedbashir, thanks for the feedback; PR is ready for final pass 🚀

saeedbashir

Remove the unused import import Swinject in FCMProvider.swift and you are good to go 👍

volodymyr-chekyrta · 2024-06-21T10:13:32Z

Remove the unused import import Swinject in FCMProvider.swift and you are good to go 👍

Sure, done ✅

volodymyr-chekyrta · 2024-06-21T10:15:22Z

@rnr @saeedbashir approval was dismissed after the fix, could you please provide the approval and we'll merge it

openedx-webhooks · 2024-06-21T11:45:36Z

@volodymyr-chekyrta 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jun 12, 2024

volodymyr-chekyrta requested review from rnr and saeedbashir June 12, 2024 10:26

rnr requested changes Jun 13, 2024

View reviewed changes

volodymyr-chekyrta force-pushed the feature/fcm branch from 4dc2a95 to 768726b Compare June 13, 2024 15:11

rnr previously approved these changes Jun 13, 2024

View reviewed changes

volodymyr-chekyrta dismissed rnr’s stale review via ca1d729 June 13, 2024 15:21

volodymyr-chekyrta force-pushed the feature/fcm branch from 768726b to ca1d729 Compare June 13, 2024 15:21

rnr previously approved these changes Jun 13, 2024

View reviewed changes

saeedbashir reviewed Jun 14, 2024

View reviewed changes

volodymyr-chekyrta dismissed rnr’s stale review via 7480208 June 14, 2024 08:08

volodymyr-chekyrta force-pushed the feature/fcm branch 2 times, most recently from 7480208 to 06f8387 Compare June 14, 2024 08:14

feat: firebase cloud messaging

61ab13d

volodymyr-chekyrta force-pushed the feature/fcm branch from 06f8387 to 61ab13d Compare June 14, 2024 08:28

saeedbashir reviewed Jun 20, 2024

View reviewed changes

fix: address feedback

c77f727

volodymyr-chekyrta force-pushed the feature/fcm branch from f0866f5 to c77f727 Compare June 20, 2024 12:24

saeedbashir reviewed Jun 21, 2024

View reviewed changes

saeedbashir previously approved these changes Jun 21, 2024

View reviewed changes

rnr previously approved these changes Jun 21, 2024

View reviewed changes

fix: address feedback

6ccd026

volodymyr-chekyrta dismissed stale reviews from rnr and saeedbashir via 6ccd026 June 21, 2024 10:12

volodymyr-chekyrta force-pushed the feature/fcm branch from b9ebbae to 6ccd026 Compare June 21, 2024 10:12

rnr approved these changes Jun 21, 2024

View reviewed changes

volodymyr-chekyrta merged commit b2539a6 into openedx:develop Jun 21, 2024
3 checks passed

volodymyr-chekyrta deleted the feature/fcm branch June 21, 2024 11:45

GlugovGrGlib mentioned this pull request Jul 2, 2024

feat: [FC-0047] add functionality to send push notifications openedx/edx-ace#282

Merged

13 tasks

		if Container.shared.resolve(ConfigProtocol.self)?.firebase.cloudMessagingEnabled == true {
		UNUserNotificationCenter.current().removeAllDeliveredNotifications()

		func refreshToken() {
		Messaging.messaging().deleteToken { error in

		private var storage = Container.shared.resolve(CoreStorage.self)!
		private let api = Container.shared.resolve(API.self)!

feat: [FC-0047] FCM #461

feat: [FC-0047] FCM #461

Conversation

volodymyr-chekyrta commented Jun 12, 2024 • edited Loading

openedx-webhooks commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

volodymyr-chekyrta commented Jun 13, 2024

saeedbashir commented Jun 13, 2024

saeedbashir commented Jun 14, 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

volodymyr-chekyrta Jun 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saeedbashir Jun 20, 2024 • edited Loading

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

volodymyr-chekyrta commented Jun 14, 2024

volodymyr-chekyrta commented Jun 14, 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

volodymyr-chekyrta commented Jun 20, 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

saeedbashir commented Jun 21, 2024

volodymyr-chekyrta commented Jun 21, 2024

saeedbashir left a comment

Choose a reason for hiding this comment

volodymyr-chekyrta commented Jun 21, 2024

volodymyr-chekyrta commented Jun 21, 2024

openedx-webhooks commented Jun 21, 2024

volodymyr-chekyrta commented Jun 12, 2024 •

edited

Loading

volodymyr-chekyrta Jun 20, 2024 •

edited

Loading

saeedbashir Jun 20, 2024 •

edited

Loading