Skip to content

Changed <br> to <br /> as line break to solve the issue "Saving without editing the multiline comments edit box shows [Comment Deleted] in LHN"#433

Merged
Luke9389 merged 2 commits into
Expensify:mainfrom
sobitneupane:sn_bug-multiline-comment-deleted
Jan 31, 2022
Merged

Changed <br> to <br /> as line break to solve the issue "Saving without editing the multiline comments edit box shows [Comment Deleted] in LHN"#433
Luke9389 merged 2 commits into
Expensify:mainfrom
sobitneupane:sn_bug-multiline-comment-deleted

Conversation

@sobitneupane

@sobitneupane sobitneupane commented Jan 28, 2022

Copy link
Copy Markdown
Contributor

@puneetlath @rushatgabhane will you please review this?

This change is made to solve the issue "Saving without editing the multiline comments edit box shows [Comment Deleted] in LHN"

Reason:
In editReportComment in Report.js, we are using ExpensiMark as the parser and ExpensiMark uses <br> as the line break. But the originalReportAction.message[0].html has <br /> as the line break. So, two messages when compared appear different and it is considered to be edited. So, though the message is not edited, the message is indicated as an edited message. Problem begins here.

Fixed Issues

$ Expensify/App#7268

Tests

  1. Send multiline message.
  2. Click on edit to edit the message. Save without editing
  3. Nothing happens to lastMessageText in LHN. Previously, lastMessageText was changed to [Comment Deleted]

QA

  1. Send multiline message.
  2. Click on edit to edit the message. Save without editing
  3. Nothing happens to lastMessageText in LHN. Previously, lastMessageText was changed to [Comment Deleted]

Web

comment_deleted.mp4

mWeb

vidma_recorder_edited_28012022_232927.mp4

Android

vidma_recorder_edited_28012022_225407.mp4

@sobitneupane sobitneupane requested a review from a team as a code owner January 28, 2022 17:25
@github-actions

github-actions Bot commented Jan 28, 2022

Copy link
Copy Markdown

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

@MelvinBot MelvinBot requested review from Luke9389 and removed request for a team January 28, 2022 17:26
@sobitneupane

Copy link
Copy Markdown
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@puneetlath

Copy link
Copy Markdown
Contributor

@sobitneupane can you please test your change on all platforms and post screenshots from each? Thanks!

@sobitneupane

Copy link
Copy Markdown
Contributor Author

@sobitneupane can you please test your change on all platforms and post screenshots from each? Thanks!

@puneetlath I have attached screen captured for Web, mWeb and Android. I don't have IOS and MacOS.

@Luke9389

Copy link
Copy Markdown
Contributor

Hey @sobitneupane, a few things.

  1. Can you change the title of this PR to describe the change you've made? Upon reading the title, there's no way to know what fix/issue is being addressed here.
  2. Can you add detailed instructions for how someone else should test this change? Break it down step by step.

@rushatgabhane

Copy link
Copy Markdown
Member

@sobitneupane Looks like we need to change the tests as well.
image

@rushatgabhane

Copy link
Copy Markdown
Member

I don't have IOS and MacOS

I'll send those in a while

@sobitneupane sobitneupane changed the title Change made to solve the issue 7268 Changed <br> to <br /> as line break to solve the issue "Saving without editing the multiline comments edit box shows [Comment Deleted] in LHN" Jan 29, 2022
@sobitneupane

Copy link
Copy Markdown
Contributor Author

Hey @sobitneupane, a few things.

  1. Can you change the title of this PR to describe the change you've made? Upon reading the title, there's no way to know what fix/issue is being addressed here.
  2. Can you add detailed instructions for how someone else should test this change? Break it down step by step.

Updated.

@sobitneupane

Copy link
Copy Markdown
Contributor Author

@sobitneupane Looks like we need to change the tests as well. image

Changed.

@rushatgabhane rushatgabhane left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM 🎉 Tests well on all platforms.

cc: @Luke9389 ? 😂

@rushatgabhane

rushatgabhane commented Jan 31, 2022

Copy link
Copy Markdown
Member
  1. In /node_modules/expensify-common/lib/ExpensiMark.js, change <br> to <br />

@sobitneupane could you please remove this step from Tests and QA? Thanks

Comment thread lib/ExpensiMark.js
name: 'newline',
regex: /\n/g,
replacement: '<br>',
replacement: '<br />',

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Will this affect ordinary messages as well as edited ones? Have we tested what this does in the "normal" situation of ppl sending each other messages?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I ask bc the tests in the OP only mention editing an already-sent message

@rushatgabhane rushatgabhane Jan 31, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good question. I've looked into this here - Expensify/App#7268 (comment)

Basically, even if we pass Line1<br>Line2 to the API, the server returns Line1<br />Line2, and that's what is stored in Onyx.

I couldn't find any side effects that'll occur after implementing this change. I can't speak of any unrelated internal repos that use Expensimark.replace() though

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

awesome, thx @rushatgabhane

@Luke9389 Luke9389 merged commit c3bad5a into Expensify:main Jan 31, 2022
@puneetlath

Copy link
Copy Markdown
Contributor

I just want to check, did we do this? If not, I can create a PR to do so:

image

@rushatgabhane

This comment was marked as outdated.

@rushatgabhane

Copy link
Copy Markdown
Member

Forgive me!

Yes please, let's create a PR to deploy expensify-common change to Expensify/App.

@rushatgabhane

Copy link
Copy Markdown
Member

Created a PR ^

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.

4 participants