[$500] Web - When we send link with multiple ending parenthesis the link ends in the middle

[kbecciv]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Go to any report
2. Send a link with multiple umatched closing parenthesis that are not all at the end, for example: google.com/(toto))titi)

Expected Result:

The link should end after google.com/(toto) because the first unmatched parenthesis is probably not part of the link, for example it could be the parenthesis closing the markdown: google.com/(toto)

Actual Result:

The link ends after the unmatched parenthesis at a weird place

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android / native
• Android / Chrome
• iOS / native
• iOS / Safari
• MacOS / Chrome / Safari
• MacOS / Desktop

Version Number: 1.3.63.0
Reproducible in staging?: y
Reproducible in production?: y
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

2023-08-31.06-49-03.mp4

Recording.4277.mp4

Expensify/Expensify Issue URL:
Issue reported by: @ShogunFire
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693485959926909

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~010ba033b476889ccc
• Upwork Job ID: 1700291645262581760
• Last Price Increase: 2023-09-08
• Automatic offers:
• ShogunFire | Contributor | 26746827
• ShogunFire | Reporter | 26746828

Issue Owner

Current Issue Owner: @robertKozik

[melvin-bot]

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[kbecciv]

Proposal by: @ShogunFire
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1693485959926909

Proposal

Please re-state the problem that we are trying to solve in this issue.

The link look wrong when we send a link with multiple closing parenthesis.

What is the root cause of that problem?

We remove extra closing parenthesis from links here: https://github.com/Expensify/expensify-common/blob/7735de14112a968fd6a4f5af710d2fbaefc8809d/lib/ExpensiMark.js#L456-L471
But the way we do it is wrong because we just count the number of extra parenthesis x and then remove x characters from the link without checking if the closing parenthesis are at the end or not.

What changes do you think we should make in order to solve the problem?

We should remove every characters after an unmatched closing parenthesis
This is the code I propose:

// remove everything characters after an unmatched ) parenthese (including the parenthese)
let brace = 0;
for (let i = 0; i < match[2].length; i++) {
    if (match[2][i] === '(') {
        brace++;
    }
    if (match[2][i] === ')') {
        if(brace <= 0){ // unmatched closing parenthesis
            const numberOFCharsToRemove = match[2].length - i;
            match[0] = match[0].substr(0, match[0].length - numberOFCharsToRemove);
            match[2] = match[2].substr(0, match[2].length - numberOFCharsToRemove);
            break;
        }
        brace--;
    }
}

What alternative solutions did you explore? (Optional)

[alexpensify]

When I was reviewing for dupes, I found this GH: #25532 (comment)

@ShogunFire are you addressing this issue in that GH listed above? I see you included similar links.

cc: @MonilBhavsar and @0xmiroslav

[ShogunFire]

@alexpensify I found this issue while resolving the other one but I believe this is a different issue. The other one is about removing special characters from the url that we are not currently removing.
This one is about a problem with how we remove the end parenthesis currently.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~010ba033b476889ccc

[melvin-bot]

Current assignee @alexpensify is eligible for the External assigner, not assigning anyone new.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @robertKozik (External)

[alexpensify]

Thanks for the feedback @ShogunFire!

@robertKozik could you please review the proposal and identify if it will resolve this issue? Thanks!

[ShogunFire]

I couldn't edit my proposal but another approach that I put in the slack thread is to remove the unmatched end parenthesis only if they are at the end but I don't recomment it.

[khanumar03]

hi is issue still open ?

[melvin-bot]

📣 @khanumar03! 📣
Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork.
Please follow these steps:

1. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
2. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
3. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.
   
   Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

[khanumar03]

@alexpensify @ShogunFire! so we want to normalize parenthesis ?

[MonilBhavsar]

@alexpensify I prefer having two issues separate. We're discussing in the other issue.

[robertKozik]

Hi all! Thanks for proposal @ShogunFire, I've checked it and it solves the issue. If we are ok with reducing the link to the first unmatched parenthesis, proposal is good to go

🎀 👀 🎀 C+ reviewed

[melvin-bot]

Triggered auto assignment to @cead22, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[alexpensify]

@cead22 - any update on the proposal review? Thanks!

[cead22]

Review submitted. Requested a couple of small changes

[alexpensify]

@ShogunFire have you been able to review the changes that Carlos suggested?

[ShogunFire]

@alexpensify I just made the changes

[alexpensify]

Thank you for the update!

[melvin-bot]

@cead22 @alexpensify @ShogunFire @robertKozik this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

[alexpensify]

Easy melvin, there is a PR in review here. I've added the Reviewing label to clarify the state of this one.

[alexpensify]

Looks like the PR was merged so waiting for the automation here.

[alexpensify]

Looks like the PR was merged last week, I need to figure out if automation failed here and identify the new payment date. 🤔

[alexpensify]

@cead22 and @robertKozik -- can you share an update on the status of this GH? I'm trying to figure out if the automation broke here or if we are waiting on another PR. Thanks!

[cead22]

This is done, but the PR was on expensify-common, not App, which is probably why the automation failed. There should be an App PR to bump the version of expensify-common, but I bet we've already updated it to a version that includes the fix made in expensify-common for this

[alexpensify]

@ShogunFire and @robertKozik - was there an App PR here? I don't see it and trying to make sure we pay everyone since there has been no regression. Thanks!

[ShogunFire]

@alexpensify No, I haven't done an App PR, I am not sure what I am supposed to do. @cead22 seems to say it might already be done

[cead22]

Yes this is done. The expensify-common PR was merged in Sep and the expensify-common commit that App is pointing to is from Oct

[robertKozik]

The last bump of the expensify-common is from last week, where PR from this issue was merged about a month.

edit: ⬆️ confirming

[alexpensify]

@robertKozik any update on what action is needed here?

[robertKozik]

As @cead22 comment, the PR is already live on staging/production. The PR for expensify-common has been merged in Sep. We didn't create the PR for updating the expenfisy-common version, but It has been done by somebody else in the meantime (lates update is from the last week, so it for sure contains our fix as well). As there is no trace of any regression I think we can proceed with this issue like the normal one which didn't cause any problems

[cead22]

@alexpensify do you know what comes next or how to trigger the automation for the payments?

[alexpensify]

I'm OOO today. On Monday I will review the GH for the payment summary (ex. is there is an urgency bonus) and start the process in Upwork manually.

[alexpensify]

Here is the payment summary:

• External issue reporter @ShogunFire $50
• Contributor that fixed the issue @ShogunFire $500
• Contributor+ that helped on the issue and/or PR - @robertKozik payment outside of Upwork

Upwork Job:

*If applicable, the bonuses will be applied on the final payment

Extra Notes regarding payment: There is no urgency bonus but no penalties either.

@ShogunFire and @robertKozik -- please 👍🏼 if you agree and I'll work on the next steps. Thanks!

[alexpensify]

@ShogunFire - I sent over two Upwork contracts and need your approval to continue the process. Thanks!

[alexpensify]

Closing - I've completed the payment process in Upwork.