[lib] Merge generating notifs data with processing operations
ClosedPublic
Actions

Authored by tomek on Oct 31 2024, 8:03 AM.

Details

Reviewers

kamil
angelika

Commits

rCOMM50bce357c2ac: [lib] Merge generating notifs data with processing operations

Summary

In the next diff we're modifying the notifications data so that it contains the threads necessary to generate notifs. For some specs, threads need to represent the state after processing the operation, which means that notificationsCreationData would need to call processDMOperation, use returned updates, and apply them to the threads. This has two problems: we would call the processDMOperation twice for each operation (once top-level, once from notificationsCreationData), and there would be a cycle between notificationsCreationData and processDMOperation, which calls notificationsCreationData. To solve these two issue we need to merge the logic into one method.

https://linear.app/comm/issue/ENG-9823/fix-sending-notifs-about-leaving-a-thick-thread

Depends on D13842

Test Plan

Flow. Did a sanity check - sent a text message to a thick thread and verified that a notif was delivered.

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

tomek created this revision.Oct 31 2024, 8:03 AM

Herald added a subscriber: ashoat. · View Herald TranscriptOct 31 2024, 8:03 AM

tomek added a child revision: D13845: [lib] Delete remove members spec.Oct 31 2024, 8:06 AM

Harbormaster returned this revision to the author for changes because remote builds failed.Oct 31 2024, 8:17 AM

Harbormaster failed remote builds in B32477: Diff 45510!

Fix Eslint

Harbormaster completed remote builds in B32487: Diff 45520.Oct 31 2024, 10:03 AM

tomek requested review of this revision.Oct 31 2024, 10:03 AM

I like this new approach 👌

lib/shared/dm-ops/change-thread-read-status-spec.js
15–23	nit: for me it's cleaner to inline this but this is okay too
lib/shared/dm-ops/dm-op-spec.js
14	why this is removed?
lib/shared/dm-ops/process-dm-ops.js
217	this is slightly different, we could have mocked empty object now - but see my comment below
lib/types/dm-ops.js
524	shouldn't this be nullable as previously? Not familiar with this that much but wondering if returning mock object: notificationsCreationData: { messageDatasWithMessageInfos: [], }, has some impact on the logic?

This revision is now accepted and ready to land.Nov 5 2024, 2:02 AM

tomek marked an inline comment as done.Nov 7 2024, 7:05 AM

tomek added inline comments.

lib/shared/dm-ops/change-thread-read-status-spec.js
15–23	I'm open to both - going to update the code.
lib/shared/dm-ops/dm-op-spec.js
14	This comment isn't useful and is outdated - we're using it in other contexts too
lib/types/dm-ops.js
524	It seems like this is safe, but we're skipping an option to early exit in `preparePushNotifs`. I'm going to change this.