[HOLD for payment 2023-09-07] [$1000] Chat comment linking redirects to 'Hmm... it's not here' instead of the intended chat

[perunt]

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:

beta flag: commentLinking

1. Open the chat.
2. Copy link to the message
3. Put this link in the chat
4. Click on this link.

Expected Result:

The application should redirect to the referenced chat.

Actual Result:

Instead of navigating to the intended chat, the application returns a 'Hmm... it's not here' page.

Workaround:

do not need as the comment linking feature is in development

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.46-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
Expensify/Expensify Issue URL:
Issue reported by: @perunt
Slack conversation:

Screen.Recording.2023-07-27.at.10.16.17.mov

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01aef64f5f78bfffbd
• Upwork Job ID: 1685030150680666112
• Last Price Increase: 2023-08-04

[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

[c3024]

Not reproducible. Could you show which report the link refers to clearly in the video? It appears there is no report with that id exists.

[perunt]

Use beta flag 'commentLinking' to enable the possibility of 'copy link to message'

Screen.Recording.2023-07-27.at.11.00.17.mov

[dukenv0307]

Proposal

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

Chat comment linking redirects to 'Hmm... it's not here' instead of the intended chat

What is the root cause of that problem?

The link with format :r/:reportID/:reportActionID doesn't exist in linking config of the APP. That makes page not found appear

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Line 230 in a4687ab

Clipboard.setString(`${environmentURL}/r/${reportID}/${reportActionID}`);

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

We should pass reportActionID as a param in the query string of the URL when we copy the link of the report action here like And then if reportActionID param is exist in the URL we will scroll to the action in ReportScreen

In here:

App/src/pages/home/report/ContextMenu/ContextMenuActions.js

Line 230 in a4687ab

Clipboard.setString(`${environmentURL}/r/${reportID}/${reportActionID}`);

we should update like this:

Clipboard.setString(`${environmentURL}/r/${reportID}?reprotActionID=${reportActionID}`);

What alternative solutions did you explore? (Optional)

If we want the URL similar to Slack, we can update the route of report screen to :r/:reportID/:reportActionID?

App/src/ROUTES.js

Line 44 in 65ffc10

REPORT_WITH_ID: 'r/:reportID',

Update prop type accordingly and we also should scroll to the message if reportActionID exists in the URL to the same as Slack

[perunt]

Proposal
Please re-state the problem that we are trying to solve in this issue.
For further implementation of the comment linking feature, we need this functionality of navigating to chat works. Now we have error page instead of redirection to the proper report room

What is the root cause of that problem?
Navigator link formatting

What changes do you think we should make in order to solve the problem?
we need update this part of the code
and put

REPORT_WITH_ID: 'r/:reportID/:reportActionID?',

also expand propTypes here as well

/** Navigation route context info provided by react navigation */
route: PropTypes.shape({
    /** Route specific parameters used on this screen */
    params: PropTypes.shape({
        /** The ID of the report this screen should display */
        reportID: PropTypes.string,

        /** The reportActionID to scroll to */
        reportActionID: PropTypes.string,
    }).isRequired,
}).isRequired,

[roryabraham]

@alexpensify FWIW I asked @perunt to create this issue

[perunt]

@dukenv0307, it seems my solution found its way into your edited comment 🤣

Anyway, in the scope of this task, I would also fix the format of the link created, as now it needs to be revised. Can I do it in this scope, or should I create a new issue?

[dukenv0307]

@perunt This format URL is basic and slimilar with Slack and in my solution there is something different from your solution. C+ will choose you as the first contributor posts the solution if your solution is good .

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01aef64f5f78bfffbd

[melvin-bot]

Triggered auto assignment to @stephanieelliott (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[alexpensify]

@roryabraham thanks for the update.

I've assigned external. @mananjadhav when you get a chance please review the proposals. Thanks!

[mananjadhav]

@roryabraham do we have any preference on the URL? Path params vs query params. I am inclined to use @perunt's proposal as it is inline with rest of our URLs, but do we need any kind of identifier between the reportId and reportActionId?

[dukenv0307]

@mananjadhav If the URL contains reportActionID we should scroll to this message to make sense and similar with Slack.

[perunt]

@mananjadhav it's preparing to this task.

[alexpensify]

@roryabraham when you get a chance, can you review the comments starting here: #23711 (comment)

[melvin-bot]

📣 @mananjadhav Please request via NewDot manual requests for the Reviewer role ($1000)

[roryabraham]

Hey, sorry for any confusion here but @perunt was hired separately to work on the comment linking project and wrote a design doc which was reviewed by our team, so this really wasn't meant to be open to external proposals.

Regarding @mananjadhav's question, we're using path params, not query params. The format should be: /r/:reportID/:reportActionID?

[alexpensify]

Thank you for the update, Rory!

[melvin-bot]

@mananjadhav @alexpensify @roryabraham @perunt this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

[perunt]

@roryabraham, I don't want to distract you, but I also want to create an issue regarding the formatting of the copy link. I recall a conversation about it in the comment linking document, but the issue persists. I'm going to fix it before your PRs are ready.

[alexpensify]

@roryabraham and @mananjadhav - since there are various parts to this one, should we put on Hold?

[alexpensify]

No update

[alexpensify]

Weekly Update: On Hold

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.3.59-5 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Comment linking redirects #24411

If no regressions arise, payment will be issued on 2023-09-07. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @mananjadhav does not require payment (Eligible for Manual Requests)
• @perunt does not require payment (Contractor)
• @perunt does not require payment (Contractor)

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[melvin-bot]

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@mananjadhav] The PR that introduced the bug has been identified. Link to the PR:
• [@mananjadhav] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
• [@mananjadhav] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
• [@mananjadhav] Determine if we should create a regression test for this bug.
• [@mananjadhav] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
• [@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

[alexpensify]

@mananjadhav to prepare for the payment day, can you please complete the checklist? Thanks!

[alexpensify]

@mananjadhav - payment day is tomorrow, can yo please complete the checklist? Thanks!

[mananjadhav]

@alexpensify thanks for following up. I would treat this as a part of feature request, where we land on the specific action after clicking the link. Here we're only ensuring that it lands to a specific chat instead of Hmm... it's not here.

Hence I think this doesn't need a regression test too.

@alexpensify Can you post a payout summary?

[alexpensify]

Here is the payment summary:

• External issue reporter @perunt payment via Margelo
• Contributor that fixed the issue @perunt payment via Margelo
• Contributor+ that helped on the issue and/or PR @mananjadhav $1000

Upwork Job: https://www.upwork.com/jobs/~01aef64f5f78bfffbd

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

Extra Notes regarding payment: There is no bonus and no penalty, this GH required multiple steps and reviews.

[alexpensify]

@mananjadhav - let me know if there are questions or concerns, I will close tomorrow. Thanks!

[JmillsExpensify]

$1,000 payment approved via NewDot based on BZ summary.