Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
3.12k stars 2.62k forks source link

[$250] Android - Workspace- WS invite message shows html message briefly #42668

Open lanitochka17 opened 1 month ago

lanitochka17 commented 1 month 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: 1.4.76 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4578418 Issue reported by: Applause - Internal Team

Action Performed:

  1. Launch app
  2. Tap profile -- workspaces -- workspace
  3. Tap members -- invite member
  4. Select a user and tap next
  5. Tap app's back button
  6. Tap next

Expected Result:

Workspace invite message must not show html message briefly

Actual Result:

Workspace invite message shows html message briefly

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/9110fcb7-7bdd-465b-addf-009d9147113a

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01b2c0cb41c6778335
  • Upwork Job ID: 1796503664094650368
  • Last Price Increase: 2024-06-28
  • Automatic offers:
    • mkhutornyi | Reviewer | 102917575
    • bernhardoj | Contributor | 102917576
Issue OwnerCurrent Issue Owner: @mkhutornyi
melvin-bot[bot] commented 1 month ago

Triggered auto assignment to @sonialiap (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.

lanitochka17 commented 1 month ago

@sonialiap FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

lanitochka17 commented 1 month ago

We think that this bug might be related to #vip-vsp

neonbhai commented 1 month ago

Proposal

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

WS invite message shows html message briefly

What is the root cause of that problem?

We don't parse to Markdown when setting default value here: https://github.com/Expensify/App/blob/563f8c704bca0c271ad8b2b9454e07462039b416/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L202

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

We should call the parser when setting default value:

defaultValue={parser.htmlToMarkdown(getDefaultWelcomeNote())}
allgandalf commented 1 month ago

Proposal

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

We see html input for a second for invite messages.

What is the root cause of that problem?

When the RHP renders for the first time, we get the default message from getDefaultWelcomeNote: https://github.com/Expensify/App/blob/563f8c704bca0c271ad8b2b9454e07462039b416/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L82

This will set the welcome message to: https://github.com/Expensify/App/blob/563f8c704bca0c271ad8b2b9454e07462039b416/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L89-L93

Screenshot 2024-05-28 at 3 49 11 AM

Here we get the content with HTML tags.

Now, we also have a useEffect in place, which will again set the state of welcomeNote using setWelcomeNote:

https://github.com/Expensify/App/blob/563f8c704bca0c271ad8b2b9454e07462039b416/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L95-L102

Here we have used htmlToMarkdown, this will render the HTML to markdown.

Hence we only see html message for a split second and then we see markdown content, this is the root cause.

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

We should update getDefaultWelcomeNote to use parser.htmlToMarkdown instead of parser.replace:

--- a/src/pages/workspace/WorkspaceInviteMessagePage.tsx
+++ b/src/pages/workspace/WorkspaceInviteMessagePage.tsx
@@ -86,12 +86,17 @@ function WorkspaceInviteMessagePage({
         // policy?.description can be an empty string
         // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
         policy?.description ||
-        parser.replace(
+        parser.htmlToMarkdown(
             translate('workspace.common.welcomeNote', {
                 workspaceName: policy?.name ?? '',
             }),
         );

This way we parse the default welcome message from the first time itself.

What alternative solutions did you explore? (Optional)

If parser.replace is required here, then we can instead first do parser.replace and then wrap the result in parser.htmlToMarkdown:

--- a/src/pages/workspace/WorkspaceInviteMessagePage.tsx
+++ b/src/pages/workspace/WorkspaceInviteMessagePage.tsx
@@ -86,12 +86,11 @@ function WorkspaceInviteMessagePage({
         // policy?.description can be an empty string
         // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
         policy?.description ||
-        parser.replace(
+        parser.htmlToMarkdown(parser.replace(
             translate('workspace.common.welcomeNote', {
                 workspaceName: policy?.name ?? '',
             }),
-        );
bernhardoj commented 1 month ago

Proposal

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

WS invite message page shows a message with HTML briefly.

What is the root cause of that problem?

The initial value of welcomeNote is empty, so it will use the defaultValue. https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L202-L203

The default value will take the workspace.common.welcomeNote translation and convert it to HTML (replace). https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L82-L93 https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/languages/en.ts#L1924-L1925

Then, this useEffect that is triggered on mount will update the welcomeNote state by taking the default value HTML and convert it to markdown.

https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L95-L97

That's why we see the HTML form briefly before the markdown.

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

The only markdown in the default welcome note is the link and possibly the workspace name (if the workspace name contains a markdown), https://github.com/Expensify/App/blob/2514f2988eb26858d30fa501c0040b1f8fc2e239/src/languages/en.ts#L1924-L1925

and I don't think it's necessary to convert it to HTML and then back to markdown just to make the link shown as [use.expensify.com/download](https://use.expensify.com/download).

The ExpensiMark already has an auto link rule that will convert use.expensify.com/download to a link.

Also, let's say the workspace name is b, if we call replace and htmlToMarkdown, it will just become b again, so it does nothing.

So, I would prefer to remove the replace and htmlToMarkdown logic to keep it simple.

sonialiap commented 1 month ago

Sounds buggy to me. Adding help wanted

allgandalf commented 1 month ago

@sonialiap I think we should add external label for C+ assignment, help wanted didn’t trigger auto assignment

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

sonialiap commented 1 month ago

Thanks for the catch! Adding External, hopefully automation runs this time

melvin-bot[bot] commented 1 month ago

@sonialiap, @mkhutornyi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mkhutornyi commented 1 month ago

reviewing proposals

mkhutornyi commented 1 month ago

@bernhardoj please make sure that your solution doesn't cause regression by checking email and sms (for phone user) and verifying that link is correctly shown.

mkhutornyi commented 1 month ago

It would be good to add offending PR in root cause. This should be recent regression.

allgandalf commented 1 month ago

@mkhutornyi do you have feedback on my proposal? or does it not solve the issue at core ? 🙃

mkhutornyi commented 1 month ago

@allgandalf thanks for the proposal. Can you please add the reason why this happens only on native in root cause? And same feedback above.

bernhardoj commented 1 month ago

@mkhutornyi the welcome note passed to the BE is the same before and after my proposal.

<h1>bernhard josephus 2 invited you to Bernhard.josephus+9999&#x27;s Workspace 12</h1><br />You have been invited to Bernhard.josephus+9999&#x27;s Workspace 12! Download the Expensify mobile app at <a href="https://use.expensify.com/download" target="_blank" rel="noreferrer noopener">use.expensify.com/download</a> to start tracking your expenses.

Both [use.expensify.com/download](https://use.expensify.com/download) and use.expensify.com/download is converted to <a href="https://use.expensify.com/download" target="_blank" rel="noreferrer noopener">use.expensify.com/download</a> HTML, so nothing is changed.

I haven't received the invitation email, but because the data sent to the BE is identical, I assume the email body text will be identical too

mkhutornyi commented 1 month ago

I haven't received the invitation email

Same to me so not able to confirm email body.

@bernhardoj why do you think this happens only on native?

bernhardoj commented 1 month ago

I believe that's simply because the web perf is faster. You can put a delay here and notice the issue on web too. https://github.com/Expensify/App/blob/0ae7febfa7f72d5c1563a23d3b8b05e576e12d68/src/pages/workspace/WorkspaceInviteMessagePage.tsx#L96-L99

mkhutornyi commented 1 month ago

There should also be offending PR since I think this is recent regression.

melvin-bot[bot] commented 1 month ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

allgandalf commented 1 month ago

There should also be offending PR since I think this is recent regression.

here’s the offending PR @mkhutornyi https://github.com/Expensify/App/pull/38487/files

melvin-bot[bot] commented 4 weeks ago

@sonialiap @mkhutornyi 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!

melvin-bot[bot] commented 4 weeks ago

@sonialiap, @mkhutornyi Whoops! This issue is 2 days overdue. Let's get this updated quick!

allgandalf commented 4 weeks ago

friendly bump @mkhutornyi here's the comment for offending PR

mkhutornyi commented 3 weeks ago

checking

mkhutornyi commented 3 weeks ago

@bernhardoj so your solution is to revert https://github.com/Expensify/App/pull/38487/commits/11916b569b4648c8ea1013d94dbaee36de4f3e44, right? There must be a reason why parser was introduced. @narefyev91 appreciate if you can chime in here.

narefyev91 commented 3 weeks ago

@bernhardoj so your solution is to revert 11916b5, right? There must be a reason why parser was introduced. @narefyev91 appreciate if you can chime in here.

The problem was in link - at that time either ExpensiMark was not able to automatically convert link or response from BE changed. Hard to say - but generally speaking - root problem to add parser was to show a link for download correctly. Hope that helps!

melvin-bot[bot] commented 3 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 3 weeks ago

@sonialiap, @mkhutornyi Whoops! This issue is 2 days overdue. Let's get this updated quick!

mkhutornyi commented 3 weeks ago

The problem was in link - at that time either ExpensiMark was not able to automatically convert link or response from BE changed. Hard to say - but generally speaking - root problem to add parser was to show a link for download correctly. Hope that helps!

Thanks for sharing the context

mkhutornyi commented 2 weeks ago

@bernhardoj's proposal looks good to me. Not found any issues after removing parse logic. Also, make sure to cover test case of https://github.com/Expensify/App/issues/44002. 🎀👀🎀 C+ reviewed

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 2 weeks ago

@sonialiap @roryabraham @mkhutornyi this issue is now 4 weeks old, please consider:

Thanks!

melvin-bot[bot] commented 2 weeks ago

@sonialiap, @roryabraham, @mkhutornyi Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

mkhutornyi commented 1 week ago

Not overdue from my side

melvin-bot[bot] commented 1 week ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 week ago

📣 @mkhutornyi 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 1 week ago

📣 @bernhardoj 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

bernhardoj commented 1 week ago

PR is ready

cc: @mkhutornyi