Previously when creating a new thread the memberships.last_message was set to 1 (or 0 if it was supposed to be unread). We should set it to correct value, because after D8492 we depends on this value t always make sense. One problem is that if a user doesn't have a visiblity into a subthread, they don't see the subthread creation message so we need to handle this. Another problem is that we can't just query the db, because some of the membership rows are yet to be created, so we need to handle this case too.
Details
Details
- Register a new user, check if their genesis last message is set to the last message in genesis
- Create a secret subthread, add a user, check that their last message is set to the message directly before
- Create an open subthread, add a user, check that their last message is set to the subthread-creation-message
(in the last two cases the last_message is then overriden by joined-chat message)
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- michal/no-messages
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
thread-permission-updaters.js is one of the most complex parts of the keyserver codebase. Some quick notes:
- I should be reviewing any changes to that file, as I'm really the only one on the team with full context on how it works.
- Whenever possible, we should avoid making changes to that file, and keep the changes outside. Have you considered modifying the membershipRows after they are returned and before they are passed to commitMembershipChangeset, rather than modifying updateRole directly?
- The functions in this file have serious performance issues. We should avoid slowing down any workflows that we don't need to slow down. If you must keep your changes inside thread-permission-updaters.js, is there a way you can limit your changes so they only slow things down when necessary? As an example, see how D8564 uses forcePermissionRecalculation.
- I'm worried about the performance of your getLatestMessages SQL query. Can you modify your Test Plan to explain what've you've done to look at the performance of this query? Some things you can do are query analyzing, and asking me to test an example query in production.
| keyserver/src/updaters/thread-permission-updaters.js | ||
|---|---|---|
| 1260 | This function is really complicated. I decided to skip reviewing it in detail in part because of the complexity, and in part because I suspect the approach might need to be changed. Does it really need to be so complicated? If so, can you explain what the alternatives were that you tried, and why you opted for this approach? | |
| 1270 | Move this to the top-level scope to avoid redeclaring it on every invocation | |
| 1277–1282 | There are numerous formatting issues here, including strange indentation and lines that are longer than 80 chars. Quite frankly, it's not a good use of my time to be formatting queries for you. Please make sure you get this right in the future before submitting a diff. If you're not sure what the expectation is, try reviewing some existing queries. Here is how this query should be formatted: const latestMessageQuery = SQL` SELECT mm.user, m.thread, MAX(m.id) AS latest_message FROM messages m LEFT JOIN memberships mm ON mm.thread = m.content AND m.type = ${messageTypes.CREATE_SUB_THREAD} WHERE mm.user IS NULL OR JSON_EXTRACT(mm.permissions, ${visibleExtractString}) IS TRUE `; | |
| 1297–1300 | Fix formatting. Once again, tons of issues here... inconsistent capitalization, weird indentation, and the open/close of the template literal don't match conventions | |