Page MenuHomePhabricator

Send event to JavaScript when notification is tapped so that we navigate to the thread
ClosedPublic

Authored by marcin on Jan 18 2023, 6:54 AM.
Tags
None
Referenced Files
F3251011: D6296.diff
Fri, Nov 15, 5:19 PM
Unknown Object (File)
Thu, Nov 14, 10:28 AM
Unknown Object (File)
Mon, Nov 11, 11:53 PM
Unknown Object (File)
Mon, Nov 11, 11:53 PM
Unknown Object (File)
Mon, Nov 11, 11:53 PM
Unknown Object (File)
Mon, Nov 11, 11:53 PM
Unknown Object (File)
Mon, Nov 11, 11:53 PM
Unknown Object (File)
Fri, Nov 8, 10:11 AM
Subscribers

Details

Summary

This differential brings back the second half of functionality we expect from tapping notification. When notification is tapped we start the app from intent stored in notification. CommAndroidNotificationEventEmitter listents for intents. If they come from notification, it sends parsed
version of this notification to JavaScript as an event. JavaScript uses this event to navigate to thread.

Test Plan

Build the app. Kill it. Send notification. Tap it. Ensure app opens to a thread. Repeat but without killing it - just put it into background.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 18 2023, 6:57 AM
Harbormaster failed remote builds in B15429: Diff 21038!
tomek requested changes to this revision.Jan 18 2023, 10:12 AM

The test plan should also include testing this functionality while the app is running / backgrounded.

native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java
29 ↗(On Diff #21038)

Can we be consistent with our approach by introducing a new class instead of implementing the interface in CommAndroidNotificationsEventEmitter?

native/push/push-handler.react.js
6 ↗(On Diff #21038)

Are we still using the library?

This revision now requires changes to proceed.Jan 18 2023, 10:12 AM
native/push/push-handler.react.js
6 ↗(On Diff #21038)

As of this differential we are still using this library. We import getFirebase from firebase.js which uses this library. Future diffs will remove this dependency.

Be consistent about using private classes instead of implementing many interfaces

tomek requested changes to this revision.Jan 26 2023, 2:01 AM
tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java
52–63 ↗(On Diff #21249)

It would be great if we could reuse the logic instead of copying the code. One option might be to save CommAndroidNotificationsActivityEventListener instance and call its onNewIntent method. We can also modify CommAndroidNotificationsActivityEventListener by introducing a new method that accepts only the RemoteMessage.

This revision now requires changes to proceed.Jan 26 2023, 2:01 AM
native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java
52–63 ↗(On Diff #21249)

I am not sure whether it is safe to call onNewIntent directly once we register it in reactContext since there may be several additional layers of abstraction between registering and calling onNewIntent that provide additional logics. But extraction to a separate private method is always a good option, so I will proceed this way.

Refactor repeated code into separate method

tomek added inline comments.
native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java
69

I don't think the function's name is correct now. We take a message from any intent, so it isn't necessarily initial

This revision is now accepted and ready to land.Jan 26 2023, 9:28 AM