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

Fix swallowed text between 2 mentions #772

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

akirk
Copy link

@akirk akirk commented Dec 1, 2023

When there are two mentions in a single Slack message, the text between the messages is swallowed.

I've extracted some code to make it easier to see the fix:

const USER_ID_REGEX = /<@(\w+)\|?\w*?>/g;
let text = 'Hello <@U123> ! :wave: I\'m pinging <@U456> as he would be the best folk to answer your question :slightly_smiling_face:';

let match = null;
while ((match = USER_ID_REGEX.exec(text)) !== null) {
    // foreach userId, pull out the ID
    // (if this is an emote msg, the format is <@ID|nick>, but in normal msgs it's just <@ID>
    const id = match[1];
    let displayName = "";
    let userId = '';
    if ( id === 'U123') userId = '@randy:server';
    if ( id === 'U456') userId = '@akirk:server';

    const users = {};
    if ( id === 'U123') users.displayName = 'Randy';
    if ( id === 'U456') users.displayName = 'Alex Kirk';

    if (!userId) {
        log.warn("Mentioned user not in store. Looking up display name from slack.");
        // if the user is not in the store then we look up the displayname
        displayName = users.displayName;
        // If the user is not in the room, we can't pills them, we have to just plain text mention them.
        text = text.slice(0, match.index) + displayName + text.slice(match.index + match[0].length);
    } else {
        displayName = users.displayName || userId;
        text = text.slice(0, match.index) + `<https://matrix.to/#/${userId}|${displayName}>` + text.slice(match.index + match[0].length);
    }
}

// TODO: This is fixing plaintext mentions, but should be refactored.
// https://github.com/matrix-org/matrix-appservice-slack/issues/110
console.log( text.replace(/<https:\/\/matrix\.to\/#\/@.+:.+\|(.+)>/g, "$1") );
console.log( text.replace(/<https:\/\/matrix\.to\/#\/@[^:]+:[^|]+\|([^>]+)>/g, "$1") );

outputs

Hello Alex Kirk as he would be the best folk to answer your question :slightly_smiling_face:
Hello Randy ! :wave: I'm pinging Alex Kirk as he would be the best folk to answer your question :slightly_smiling_face:

The first line is using the current regex which swallows text, the second one is the improved one.

Signed-off-by: Alex Kirk [email protected]

@akirk akirk requested a review from a team as a code owner December 1, 2023 09:56
@akirk akirk force-pushed the fix-swallowed-text-between-2-mentions branch from f1eff50 to 68109b3 Compare December 1, 2023 09:58
@akirk akirk force-pushed the fix-swallowed-text-between-2-mentions branch from 68109b3 to 33d2976 Compare December 1, 2023 10:04
Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix looks sensible to me. Thanks! Can you complete the contributing checklist (signoff and changelog https://github.com/matrix-org/matrix-appservice-bridge/blob/develop/CONTRIBUTING.md), and then this should be good to go.

@akirk
Copy link
Author

akirk commented Dec 1, 2023

Thanks! I added the signoff to the PR description and added a changelog entry.

@@ -0,0 +1 @@
Fix swallowed text between 2 mentions
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@akirk This line needs to end with a '.' or '!' as per tests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, added !

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

Successfully merging this pull request may close these issues.

3 participants