[keyserver] Render chat mention in notification text
AbandonedPublic
Actions

Authored by • patryk on Aug 29 2023, 6:15 AM.

Details

Reviewers

tomek
inka
rohan
ashoat

Summary

Solution for ENG-4562.

This diff enables rendering raw chat mentions in notification text.

Depends on D9006.

Test Plan

Replace this piece of code:

if (userDevices.length === 0) {
  userDevices = [
    {
      platform: 'unknown',
      deviceToken: 'unknown',
      cookieID: 'unknown',
      codeVersion: null,
      stateVersion: null,
    },
  ];
}

Add console.log(notifTexts) here.
Make sure you have created at least two users.
Test if mentions are rendered correctly accordingly to "thread visibility":

Example thread store with members:

GENESIS
- group chat 1 (user1, user2)
  - - public subchannel (user1, user2)
      - public subsubchannel (user1)
  - secret subchannel (user1)
    - secret subsubchannel
Test community
- subchannel (user1, user2)
  - subsubchannel (user1)
- secret channel (user1)

Looking from user2 perspective, those chat mentions should be replaced with the current thread name (chat where we write message -> chat mention):

GENESIS:
- group chat 1 -> public subchannel | public subsubchannel
- public subchannel -> group chat 1

Test community:
- subchannel <-> subsubchannel

Those chat mentions should be replaced with default text:

GENESIS:
- group chat 1 -> secret subchannel | secret subsubchannel
- public subchannel -> secret subchannel | secret subsubchannel

Test community:
- subchannel -> secret channel

Diff Detail

Repository

rCOMM Comm

Branch

patryk/new-chat-mentioning

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

• patryk created this revision.Aug 29 2023, 6:15 AM

• patryk held this revision as a draft.

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 29 2023, 6:15 AM

Harbormaster completed remote builds in B22220: Diff 30496.Aug 29 2023, 6:31 AM

• patryk edited the summary of this revision. (Show Details)Aug 29 2023, 6:59 AM

• patryk edited the test plan for this revision. (Show Details)

• patryk added reviewers: tomek, inka, rohan.

• patryk added a parent revision: D9006: [keyserver] Introduce function which checks if thread is mentionable.

• patryk published this revision for review.Aug 29 2023, 7:30 AM

• patryk edited the test plan for this revision. (Show Details)

• patryk added a child revision: D9061: [lib][native][web] Move validChatNameRegex to validation-utils.js.Sep 1 2023, 5:43 AM

• patryk removed a child revision: D9061: [lib][native][web] Move validChatNameRegex to validation-utils.js.Sep 1 2023, 5:46 AM

What is the default text?

ashoat mentioned this in D9006: [keyserver] Introduce function which checks if thread is mentionable.Sep 11 2023, 1:42 PM

There are so many issues here. I don't think we can proceed with this approach, especially given @patryk's limited time left at Comm.

Instead, let's just use the text inside the mention. We can replace the mention with that text. This can be done by modifying notificationTexts for messageTypes.TEXT. It should be done without needing any new awaits.

keyserver/src/push/send.js
198	I made a big mistake in D6935 when I introduced this sequence `await` into this file. We absolutely should not block a loop with an `await`. This should be extracted into a collection and then a `Promise.all` or `promiseAll` should be used to `await` all of them in parallel, instead of in sequence. This diff introduces a second example of this issue on line 192, where you add an `await` to the inside of a `for` loop. This needs to be addressed before this diff can be landed.
501–502	Why are we fetching `ServerThreadInfo`s again? You did so much work (and added so much complexity) in order to fetch these in a batch in `fetchInfos`... that work seems to be wasted, since you refetch all of it here!
614	What's the point of this new collection when you merge the results at the end anyways? It complicates the code and reduces performance
672–675	Why are you sending two queries instead of one?
732	Why did you add code to `fetchInfos` to calculate this? This function is concerned with fetching data from the database. If you would like to calculate this `userThreadIDs`, it would be best to do it elsewhere to avoid cluttering this function, which is already complicated enough as it is...
lib/shared/messages/text-message-spec.js
195–197	Don't we still need to strip @-mentions of chats for this special case?