Updated Notification behaviour #1626
Conversation
kiminkim724
requested review from
sashamaryl,
mertbagt,
nesanders,
mvictor55,
timblais,
alexjball and
Mephistic
as code owners
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Love the progress here! I had a few notes I'd like to check in on before we push this out - not sure if it's an issue with the code itself or just my understanding.
I also think we've hit the level of complexity here where a few basic unit and/or integration tests would actually speed up development by adding clarity - doesn't have to part of this PR, but we should earmark it for near future work.
functions/src/notifications/types.ts
Outdated
| @@ -7,15 +7,17 @@ export interface Notification { | |||
| billCourt: string | |||
| billId: string | |||
| billName: string | |||
| /*isBillMatch: boolean | |||
There was a problem hiding this comment.
Should these still be commented out?
There was a problem hiding this comment.
They're unnecessary, so they should be deleted now
| id: entity.billId, | ||
| subheader: subheader, | ||
| timestamp: entity.updateTime, | ||
| type: entity.type, | ||
| court, | ||
| position: position ?? "", | ||
| isBillMatch: entity.type === "bill" || entity.type === "testimony", |
There was a problem hiding this comment.
Just want to double-check the logic here
As I understand it, there are three options here:
- BillHistoryUpdateNotification
- TestimonySubmittedNotification (for testimony on a followed bill)
- TestimonySubmittedNotification (for testimony from a followed user/org)
It seems like isBillMatch would currently be true for the third condition (where the notification is for a user following another user who submitted testimony), but should be false. Does that seem right?
There was a problem hiding this comment.
Yep, correct. Should be fixed now.
| ) => { | ||
| const notificationFields = createNotificationFields(topic) | ||
|
|
||
| console.log(JSON.stringify(notificationFields)) | ||
|
|
||
| const topicNameSnapshot = await db | ||
| .collectionGroup("activeTopicSubscriptions") | ||
| .where("topicName", "==", notificationFields.topicName) | ||
| .where("topicName", "in", notificationFields.topicName) |
There was a problem hiding this comment.
Are we sure this has solved the duplication problem? It looks like if we match on two rules for the same user, it would return two snapshots here (e.g. one for the bill topic and one for the org/user topic) and we would create one notification for each snapshot.
It might be helpful to:
- Wait to
createNotificationFieldsuntil we know which topics we matched on - Group the returned
activeTopicSubscriptions byuid - Extract the matching follow types (billFollow vs userFollow) from the
activeTopicSubscriptions - For each
uid, runcreateNotificationFieldsonly now that we know which follows types triggered the notification and can setisBillMatch/isUserMatchfor each user's notificatiion accordingly.
There was a problem hiding this comment.
You are correct, it didn't solve the duplication problem. I thought it solved the duplication problem because the notifications didn't appear, but it's because the topicName that was being created was prefixed with org instead of testimony, so it never populated the notification. I've updated the code to reflect your suggestions.
…correctly set filter flags
Summary
Checklist
Screenshots
Known issues
If you've run against limitations or caveats, include them here. Include follow-up issues as well.
Steps to test/reproduce
For each feature or bug fix, create a step by step list for how a reviewer can test it out. E.g.: