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
Details
Details
- Reviewers
ashoat atul tomek - Commits
- rCOMM9518dca6bbf6: [keyserver] introduce sendRescindNotifs
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
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
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- eng-2644 (branched from master)
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| 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? |
| 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 |