Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Mentions in forwarded messages #10631

Closed
Antreesy opened this issue Oct 4, 2023 · 7 comments · Fixed by #10640
Closed

RFC: Mentions in forwarded messages #10631

Antreesy opened this issue Oct 4, 2023 · 7 comments · Fixed by #10640

Comments

@Antreesy
Copy link
Contributor

Antreesy commented Oct 4, 2023

ATM we handle mentions by names here:

async forwardMessage(context, { messageToBeForwarded }) {

Depending on correlation id/name it will be rendered in different conversation if:

  • name === id
  • name.split(' ').includes(id).

Notification won't be triggered for this person though (or something prevents reproducing it)
When developing, all mention options should be considered:

@all
@user1
@"user@2"
@"user 3"

See screenshot attached
image

Originally posted by @Antreesy in #10603 (comment)

@DorraJaouad
Copy link
Contributor

				if (key.includes('mention-call')) {
					message.message = message.message.replace(`{${key}}`, `@${mention.name}`)
				} else {
					message.message = message.message.replace(`{${key}}`, `@"${mention.id}"`)

				}

The double quotes " could solve mentions with id containing space and the other types remain rendered correctly.
For the at-all mention, we can just identify it by 'mention-call' and use name instead of rendering its token but it won't remain as a mention object in forward.

image

@nickvergessen
Copy link
Member

For the at-all mention, we can just identify it by 'mention-call' and use name instead of rendering its token but it won't remain as a mention object in forward.

yes, that is actually the plan and okay.

Can be an issue thou if e.g. the room name starts with a user ID or the word "All", because then @Old room display name will actually create a mention with @old if the user exists.

@Antreesy
Copy link
Contributor Author

Antreesy commented Oct 5, 2023

Or we can avoid using @, just posting the Conversation name, maybe with bold markdown wrapping?

@DorraJaouad
Copy link
Contributor

DorraJaouad commented Oct 5, 2023

with bold markdown wrapping?

That might not be understood as a mention in the context. maybe also wrap it with double quotes ?
e.g : @"conversation name"

@nickvergessen
Copy link
Member

That might not be read as a mention though.

Well the @all from the other room must not be a mention in the new room, so it would exactly match the desired behaviour.

maybe also wrap it with double quotes ?

That still renders mentions. We need to use it when the mention id conttains a space or slash. but for @all marking it bold sounds like a reasonable replacement.

@Antreesy
Copy link
Contributor Author

Antreesy commented Oct 6, 2023

Note: as it's a bug in 27 and below, do we consider backporting it? Requires manual backport, as logic was moved to store with feature PR

@nickvergessen
Copy link
Member

It was a bug since such a long time and no one cared, so no backporting :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants