Details

Reviewers

tomek
atul
ashoat

Commits

rCOMM945bf7ccad1e: Use native modules to clear notifications for a given thread

Summary

This differential calls native api to remove all active notifications for a given thread without using additional persistence layer from redux.

Test Plan

Add logging statement to native implementation of removeAllActiveNotificationsForThread. Build the app, put it in background. Send several notifications ensure they are present in notification center. Open the app, enter the thread notifications were sent for. Ensure they disappear and
the are logged.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

marcin created this revision.Jan 11 2023, 4:48 AM

Herald added a subscriber: ashoat. · View Herald TranscriptJan 11 2023, 4:48 AM

marcin added a parent revision: D6227: Implement native Android module that can remove all active notifications for a given thread.Jan 11 2023, 4:48 AM

Harbormaster completed remote builds in B15245: Diff 20785.Jan 11 2023, 5:01 AM

marcin requested review of this revision.Jan 11 2023, 5:01 AM

marcin added a child revision: D6229: Remove threadID to notifID mapping from redux and all related usages.Jan 11 2023, 5:25 AM

Use directly in push handler not in reducer

Bring back necessary if statement

Harbormaster completed remote builds in B15247: Diff 20787.Jan 11 2023, 5:44 AM

Harbormaster completed remote builds in B15249: Diff 20789.Jan 11 2023, 5:47 AM

ashoat added inline comments.Jan 11 2023, 8:49 AM

native/push/push-handler.react.js
300 ↗	(On Diff #20789)	Let's avoid directly accessing `NativeModules`, and instead introduce a file that handles the interaction with `NativeModules`, and call that file from here (same as iOS)

marcin added inline comments.Jan 13 2023, 12:46 AM

native/push/push-handler.react.js
300 ↗	(On Diff #20789)	Sure. I will probably use `android.js` file for that.

Move NativeModules access to separate file

Harbormaster failed remote builds in B15339: Diff 20919!Jan 13 2023, 1:47 AM

Harbormaster completed remote builds in B15339: Diff 20919.Jan 13 2023, 2:20 AM

ashoat added inline comments.Jan 13 2023, 6:30 AM

native/push/android.js
16	I'm surprised Flow is okay with this!! What does it report the type of `CommAndroidNotifications` is? We probably need to explicitly type it, eg. like we type `AndroidLifecycleModule` in native/lifecycle/lifecycle-module.js

Requesting changes for types, but let me know if there's some codegen magic already generating them or something

This revision now requires changes to proceed.Jan 13 2023, 6:30 AM

marcin added inline comments.Jan 13 2023, 6:57 AM

native/push/android.js
16	What does it report the type of `CommAndroidNotifications` is? It reports the type is any and the type of `NativeModules` is `{ [moduleName: string]: any }`

ashoat added inline comments.Jan 13 2023, 7:00 AM

native/push/android.js
16	Ah yeah, we need to type this (and the iOS one)

Type CommAndroidNotifications

Harbormaster completed remote builds in B15385: Diff 20977.Jan 16 2023, 7:03 AM

Please address comment before landing

native/push/android.js
17 ↗	(On Diff #20977)	Is there anything else on `NativeModules.CommAndroidNotifications`? I think yes... if so, we should type it so

This revision is now accepted and ready to land.Jan 16 2023, 7:28 PM

marcin added inline comments.Jan 17 2023, 6:49 AM

native/push/android.js
17 ↗	(On Diff #20977)	Currently no, but I will expand the scope of this type as I will be gradually adding differentials that replace subsequent `react-native-firebase` methods.

ashoat added inline comments.Jan 17 2023, 7:11 AM

native/push/android.js
17 ↗	(On Diff #20977)	I am skeptical that there are no default methods on the native module, eg. `getConstants`. Can you modify your Test Plan to include confirming that there is only one method? You can do that via JS debugger with a `debugger` breakpoint, or possibly with some kind of `console.log` (might need to log the prototype or something though).

Looks ok, but please make sure the types are correct.

native/push/push-handler.react.js
287–297 ↗	(On Diff #20977)	It seems like we can have similar api for both platforms. Basically, we could have a method `removeAllActiveNotificationsForThread` implemented in a file with platform specific extension.