Includes handling the userNotMemberOfSubthreads logic that was previously handled in message-creator.js.
Depends on D6222
Details
Details
- Reviewers
ginsu - Commits
- rCOMMe709e8c851c5: [lib] Convert generatesNotifs to function
Flow, plus tested chat creation, message creation, liking and unliking a user's message, and liking and unliking somebody else's message
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
accepting with one clarifying question
| lib/shared/messages/message-spec.js | ||
|---|---|---|
| 111–112 ↗ | (On Diff #20767) | Didn't pick this up when we were working together, but is it okay that these arguments are not optional? |
| lib/shared/messages/message-spec.js | ||
|---|---|---|
| 111–112 ↗ | (On Diff #20767) | Yeah – it's only called from one place, and that place always specifies both params |
| keyserver/src/creators/message-creator.js | ||
|---|---|---|
| 382–383 | At this point it's correct, but might change in the future. For example I can imagine that reminders could be implemented as a new type of message that can send a notification when is created. Obviously, the fact that creator's activity doesn't generate notifs might remain true forever and designing a solution that doesn't assume that might be a waste of time. I have a slight preference to a solution where each message spec can decide if user's own activity generates notifs, but keeping this as is is also perfectly valid. A big downside of my solution is that it will create a lot of duplication and can cause some bugs when a developer forgets to add this check to a new spec. | |
| keyserver/src/creators/message-creator.js | ||
|---|---|---|
| 382–383 | Yeah, we opted for this to avoid duplication and to keep the message specs simple in most cases, but it would be really easy to change that later if we ever wanted to introduce something like the "reminders" feature you described | |