[HOLD for payment 2024-10-22] Expense - "auto paid with Expensify" system message has <a href="link"> and </a> when pasted

IuliiaHerets commented 4 weeks ago

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

Version Number: 9.0.43-0 Reproducible in staging?: Y Reproducible in production?: N Email or phone of affected tester (no customers): applausetester+pso@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition:

Workspace has set up bank account.
Employee does not have personal bank account.

Go to staging.new.expensify.com
[Employee] Submit an expense in the workspace chat.
[Admin] Pay the expense via Expensify.
[Employee] Click on the expense preview.
[Employee] Click Add bank account button.
[Employee] Add a bank account.
[Employee] Right click on the system message "automatically paid $123.00 with Expensify via workspace rules" > Copy to clipboard.
[Employee] Paste the content in the composer.

Expected Result:

The content will be pasted correctly.

Actual Result:

The pasted content has <a href="link"> and </a>. The system message also shows <a href="link"> and </a> when the report is opened for the first time (after relogin).

Workaround:

Unknown

Platforms:

[x] Android: Native
[x] Android: mWeb Chrome
[x] iOS: Native
[x] iOS: mWeb Safari
[x] MacOS: Chrome / Safari
[x] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/751df924-d0ae-4af5-97d4-dc9d8a0a0662

https://github.com/user-attachments/assets/3514897f-6044-4fed-a71b-29af3f1a762d

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @sakluger

melvin-bot[bot] commented 4 weeks ago

Triggered auto assignment to @luacmartins (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

melvin-bot[bot] commented 4 weeks ago

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

github-actions[bot] commented 4 weeks ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

IuliiaHerets commented 4 weeks ago

We think that this bug might be related to #wave-collect - Release 1

gijoe0295 commented 4 weeks ago

Edited by proposal-police: This proposal was edited at 2024-10-02 18:27:11 UTC.

Proposal

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

The pasted content has html anchor tag
The system message also shows html anchor tag when the report is opened for the first time (after relogin).

What is the root cause of that problem?

We're directly using anchor tags in copy:

https://github.com/Expensify/App/blob/66d1025282196cbe175f257dfc6d25ee19a495f0/src/languages/en.ts#L928-L929

This is a BE bug where OpenApp returns the message's html as plain text:

causing the shouldRenderAsText here to be false (because html and text are the same in this case) and thus the iouMessage renders as plain text:

https://github.com/Expensify/App/blob/0de8f42ed5080ac1ef0acbc3a0d5cb5edc60dc6a/src/pages/home/report/comment/TextCommentFragment.tsx#L60

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

We should use markdown link instead of html anchor tag like we did here:

...with Expensify via [workspace rules](${CONST.CONFIGURE_REIMBURSEMENT_SETTINGS_HELP_URL})

Fix the BE

sakluger commented 4 weeks ago

I don't know if this needs to be a deploy blocker, it's not really blocking users from completing actions in Expensify. @luacmartins what do you think?

jasperhuangg commented 4 weeks ago

@sakluger I think we should still try to fix the issue rather than letting the problem go out to production. @luacmartins were you able to look into this yet?

luacmartins commented 4 weeks ago

Not yet, but as pointed out by @gijoe0295 this seems to be a backend issue, so maybe a DeployBlocker for web-e, not App.

Gonals commented 4 weeks ago

Do we know which PR caused this? At this point, we should revert it or we won't deploy today

melvin-bot[bot] commented 4 weeks ago

Current assignee @luacmartins is eligible for the DeployBlockerCash assigner, not assigning anyone new.

github-actions[bot] commented 4 weeks ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

Identify the pull request that introduced this issue and revert it.
Find someone who can quickly fix the issue.
Fix the issue yourself.

luacmartins commented 4 weeks ago

Actually, I think the main cause is https://github.com/Expensify/App/pull/49187. I pinged Alex in Slack

Beamanator commented 4 weeks ago

yep yep, lookingggg

Beamanator commented 4 weeks ago

Hmm honestly i wouldn't call this a blocker - the copy / paste including the href part - i did that on purpose... If we want to change that so it doesn't have the link, i'm fine with that but doesn't need to be blocker-priority

I don't understand the other point - The system message also shows and when the report is opened for the first time (after relogin). - I don't see that in the videos

luacmartins commented 4 weeks ago

Demoting to NAB given Alex's comment

Beamanator commented 4 weeks ago

Assigning so i can also get some thoughts in - this is related to https://github.com/Expensify/App/issues/35091 btw

@trjExpensify do you think the "workspace rules" link should be copied with the html when a user copies the text of that auto-reimbursed report action?

ishpaul777 commented 4 weeks ago

I was involved in a similar PR a while ago, Here is how we did it

https://github.com/Expensify/App/pull/44930/files#diff-b8041d6cb129cc7b8a6223f8ffa570e79c326d8ea4affbad35c33cf6c8ea660dR1461-R1465

Instead of using <a> in translation we created a src/components/ReportActionItem/ExportIntegration.tsx use TextLink

https://github.com/Expensify/App/pull/44930/files#diff-1d22c1b69dd8b5fce98b695d5c37af6e4cae4da8dec4800258a6be61f27a3e76

Beamanator commented 4 weeks ago

Hmm @ishpaul777 so it looks like you left the HTML link in the message when copied, right?

ishpaul777 commented 4 weeks ago

yeah! actualy link copying was handled with setClipboardMessage, sorry i didn't noticed earlier

https://github.com/Expensify/App/blob/2d5efb259da5207c4165f18a9ee4394435ab56c5/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L46-L55

https://github.com/Expensify/App/blob/2d5efb259da5207c4165f18a9ee4394435ab56c5/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L467

trjExpensify commented 4 weeks ago

yeah! actualy link copying was handled with setClipboardMessage

and does it convert it to markdown when copied then?

gijoe0295 commented 4 weeks ago

For the second issue, the message shows as plain text initially when you first open the report after logging in:

For the first issue, you can try copy-paste a message from Concierge with link, you'll see that it converts the links to markdown. And the fix is just simple as mentioned in my proposal https://github.com/Expensify/App/issues/50090#issuecomment-2389313586.

Beamanator commented 4 weeks ago

aah markdown when copied! I like that plan, that sounds better than the HTML 👍

ishpaul777 commented 4 weeks ago

we might also need to fix delayed submission system message, within the scope of this issue

https://github.com/Expensify/App/pull/48939/files#diff-2a9066512656fa29b1565e362127de5cc11ba95b28f143a9db26f76dc3750785R822

would love to take c+ role if this goes external

Beamanator commented 4 weeks ago

Thanks @ishpaul777 - before we got external I think we need to nail down all the things we need to get updated / fixed in this issue 👍 want to help with that? Then, yes I'm quite sure we can get this going external for the fix(es)

ishpaul777 commented 4 weeks ago

Agreed! looked through other places we have hardcoded <a> all relevant place that are used as ReportAction is copied using setClipboardMessage instead of Clipboard.setString

for example ReportActionsUtils.getExportIntegrationMessageHTML and ReportActionsUtils.getCardIssuedMessage

https://github.com/Expensify/App/blob/2d5efb259da5207c4165f18a9ee4394435ab56c5/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L466-L481

Beamanator commented 4 weeks ago

Nice, yep i def think that's a first thing to update 👍 👍

Do you have any ideas for fixing the "oh first loading a report w/ HTML in the report action, after logging in & loading the report, the HTML is displayed as text instead of rendered HTML" - does that make sense?

Beamanator commented 3 weeks ago

By the way I have a backend PR up to fix the "first load of the auto-paid message" - I found the bug & hopefully fixed it

Beamanator commented 3 weeks ago

Backend PR merged, once deployed the "first load" part should be fixed

sakluger commented 3 weeks ago

Thanks for the update @Beamanator. Is https://github.com/Expensify/App/pull/50181 "first load" PR?

Beamanator commented 3 weeks ago

Nahhh, #50181 is just about copying / pasting that system message, which is lower priority IMO so i haven't had time to prioritize yettt

sakluger commented 3 weeks ago

Got it. So where is the other PR? I don't see any others linked to this GH issue.

Beamanator commented 3 weeks ago

Aah the other one is https://github.com/Expensify/Auth/pull/12691! Which is on prod as of last night so hopefully we can see it's fixed now! 🙏

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

https://github.com/Expensify/App/pull/50181

If no regressions arise, payment will be issued on 2024-10-22. :confetti_ball:

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

@ishpaul777 requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 2 weeks ago

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:

[ ] [@ishpaul777] The PR that introduced the bug has been identified. Link to the PR:
[ ] [@ishpaul777] 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:
[ ] [@ishpaul777] 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:
[ ] [@ishpaul777] Determine if we should create a regression test for this bug.
[ ] [@ishpaul777] 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.
[ ] [@sakluger] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

sakluger commented 2 weeks ago

Upwork offer: https://www.upwork.com/nx/wm/offer/104438278

I sent it from [$250] Payment issue for E/App PR#50059

sakluger commented 1 week ago

@ishpaul777 do we need any regression new tests for this one?

melvin-bot[bot] commented 1 week ago

Payment Summary

[Upwork Job]()

ROLE: @ishpaul777 paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@sakluger)

[ ] I have verified the correct assignees and roles are listed above and updated the neccesary manual offers
[ ] I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants//hired)
[ ] I have paid out the Upwork contracts or cancelled the ones that are incorrect
[ ] I have verified the payment summary above is correct

sakluger commented 1 week ago

This has already been paid in https://github.com/Expensify/App/issues/50742.

@ishpaul777 let us know if we need any regression tests before we close the issue.

ishpaul777 commented 1 week ago

Sorry i missed ping, no i dont think we need regression becuase its found in normal QA process we might have tests covering this already

ishpaul777 commented 1 week ago

@sakluger Isn't https://github.com/Expensify/App/issues/50742 a payment issue for different PR ?

sakluger commented 1 week ago

Oh wow, you're right - I misread the issue number on that one. My mistake!

Here's an offer for this one: https://www.upwork.com/nx/wm/offer/104577466

sakluger commented 1 week ago

Paid! Thanks again for staying on top of this @ishpaul777.

melvin-bot[bot] commented 1 week ago

@Beamanator @sakluger @luacmartins Be sure to fill out the Contact List!

Expensify / App