Page MenuHomePhabricator

[keyserver] introduce sendRescindNotifs
ClosedPublic

Authored by ginsu on Feb 21 2023, 11:00 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Oct 22, 12:31 PM
Unknown Object (File)
Tue, Oct 22, 12:31 PM
Unknown Object (File)
Tue, Oct 22, 12:31 PM
Unknown Object (File)
Tue, Oct 22, 12:31 PM
Unknown Object (File)
Tue, Oct 22, 12:31 PM
Unknown Object (File)
Tue, Oct 22, 3:10 AM
Unknown Object (File)
Mon, Oct 21, 3:59 PM
Unknown Object (File)
Mon, Oct 21, 3:38 PM
Subscribers

Details

Summary

Introduce sendRescindNotifs. sendRescindNotifs gathers all the necessary info needed to build the rescindCondition and inputCountCondition from the rescind push info. The rescindCondition and inputCountCondition get passed to rescindPushNotifs, which handles rescinding push notifs in the notificaitons table of the db


Depends on D6829
Linear Task: ENG-2644

Test Plan

I logged out fetchResult in rescind.js to see if I was fetching the correct notification to rescind based on the condition I built and provided in sendRescindNotifs. I also checked out deliveryResults in rescind.js to see if the rescind notifcation was successly being sent

Screenshot 2023-02-23 at 11.57.17 AM.png (460×3 px, 379 KB)

I also checked the notifications table in the db gui to see that a notification was sent from the message reaction and then rescinded by the subsequent unreaction

Screenshot 2023-02-23 at 11.57.29 AM.png (1×3 px, 1 MB)

Diff Detail

Repository
rCOMM Comm
Branch
eng-2644 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ginsu edited the test plan for this revision. (Show Details)
ginsu added reviewers: ashoat, atul, tomek.
Harbormaster returned this revision to the author for changes because remote builds failed.Feb 21 2023, 11:13 PM
Harbormaster failed remote builds in B16753: Diff 22913!
ginsu edited the test plan for this revision. (Show Details)

fix prettier issue

ashoat requested changes to this revision.Feb 22 2023, 11:08 AM
ashoat added inline comments.
keyserver/src/creators/message-creator.js
498–499 ↗(On Diff #22915)

Why are you awaiting these promises in sequence? Is there a reason you decided to wait on sendPushNotifs to complete before calling sendRescindNotifs?

Are you worried about somebody calling createMessages with an array containing both a react and an un-react by the same user on the same message? I don't think that's a scenario we need to consider

keyserver/src/push/send.js
253 ↗(On Diff #22915)

What is Promise.all doing here?

254 ↗(On Diff #22915)

You are calling fetchInfos, but only looking at usersToCollapsableNotifInfo. Why aren't you calling fetchCollapsableNotifs instead? This is a lot of wasted work...

260 ↗(On Diff #22915)

What does this condition do?

272 ↗(On Diff #22915)

It appears like you're assuming that the thread is read now, but I'm not sure why you're making that assumption.

At a baseline we know from ENG-3066 that this is not true in terms of what MySQL thinks.

Even if that task was addressed, we still have no guaranteed that this thread should be considered "read" after this notif is rescinded. Eg. if somebody likes a message, then somebody else sends a message, and finally the original liker unlikes... in that scenario, why are we concluding that this thread is now read?

This revision now requires changes to proceed.Feb 22 2023, 11:08 AM
keyserver/src/push/send.js
260 ↗(On Diff #22915)

I was trying to keep consistent with the sendPushNotifs function above, and sendPushNotifs had a similar condition. However, after taking second look it is unnecessary here since we will never enter the loop below if notifInfo.existingMessageInfos.length === 0.

272 ↗(On Diff #22915)

I intially thought this would be something that we would preemptively need for ENG-3066, but after reading the example you provided above, and rethinking this, my initial assumption is incorrect

This revision is now accepted and ready to land.Feb 23 2023, 12:27 PM
This revision was automatically updated to reflect the committed changes.