[HOLD for payment 2024-06-05] Search - Expense preview with multiline description shows two spacings between each line

lanitochka17 commented 6 months 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.70-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: N/A Issue reported by: Applause - Internal Team

Action Performed:

Go to staging.new.expensify.com
Submit an expense to a user with no unsettled expense, and include a multiline description in the expense
Note the expense preview in LHN shows one spacing between each line
Click on the search icon
Note that expense preview in search list

Expected Result:

The expense preview in search list will also show one spacing between each line

Actual Result:

The expense preview in search list alsos two spacings between each line

Workaround:

Unknown

Platforms:

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

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/Expensify/App/assets/78819774/01f4bd19-4390-4b95-a4f0-ad19993ef37e

View all open jobs on GitHub

Issue Owner

Current Issue Owner: @mallenexpensify

melvin-bot[bot] commented 6 months ago

Triggered auto assignment to @mallenexpensify (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 6 months ago

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

lanitochka17 commented 6 months ago

@mallenexpensify 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

melvin-bot[bot] commented 6 months ago

Triggered auto assignment to @dannymcclain (Design), see these Stack Overflow questions for more details.

mallenexpensify commented 6 months ago

@dannymcclain , do you happen to know if this is expected behaviour?

dannymcclain commented 6 months ago

@mallenexpensify I don't think it's expected—I would imagine the LHN and search rows would look the same. At the same time, I would say this is extremely minor.

bernhardoj commented 6 months ago

Proposal

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

The multi-line description of an expense request shows extra spacing between lines on the search page, but not in LHN.

What is the root cause of that problem?

This issue happens after my PR here. In the PR, we are fixing an issue where the LHN subtitle is shown as multiline when we set the expense description to multiline by replacing all new line characters with a whitespace. https://github.com/Expensify/App/blob/2f5173c9c0f041967acfbdf9581ce52327623fa6/src/libs/ReportUtils.ts#L1630

The LINE_BREAK regex is then updated to consider a case where the new line character is \r\n as found here. https://github.com/Expensify/App/blob/2f5173c9c0f041967acfbdf9581ce52327623fa6/src/CONST.ts#L1879

But the new regex (\r|\n) matches each \r and \n and replaces it with whitespace, so if we have \r\n, we will end up with 2 whitespaces. This isn't noticeable in LHN because the text style set the white-space to nowrap which collapses the whitespace into a single whitespace.

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

Update the LINE_BREAK regex replace both \r\n into a single whitespace.

// stricter, if \n\r is given, then we will have 2 whitespaces, which I believe is expected
LINE_BREAK: /\r\n|\r|\n/g,

OR

// shorter, but will match \n\r too
LINE_BREAK: /[\r\n]+/g,

we can optionally set the white-space back to pre for the LHN that gets removed in this PR

dannymcclain commented 6 months ago

Not overdue Melvin.

mallenexpensify commented 6 months ago

@dannymcclain If we updated it so that Hello Hello

Didn't show as Hello Hello in LHN, it'd just show Hello. In another example. If someone sent. Hi, What's the ETA on the project If we fixed this to recognize the line break, we'd only show Hi, in LHN. I don't think that's better than showing ``Hi, What's the ETA on the project'

OOOOOOOF... check this weirdness, it shows all the text in LHN then the second line gets removed for some reason

https://github.com/Expensify/App/assets/22508304/f11f9f3e-e86d-45e8-b780-d0db06312fa1

dannymcclain commented 6 months ago

Whoaaaa, that is weird haha. I totally agree that "fixing" it to just show Hi, is not better than Hi, What's the ETA on the project

In the video posted on the issue description, the LHN looks fine to me.

The expense description is

Hello
Hello

And the LHN displays Hello Hello.

But the search row displays Hello Hello with an extra space between the lines.

MelvinBot commented 6 months ago

This has been labelled "Needs Reproduction". Follow the steps here: https://stackoverflowteams.com/c/expensify/questions/16989

mallenexpensify commented 6 months ago

Bumping to weekly and adding Needs Reproduction because this isn't a top priority and I want to suss things out next week once the site's doing better, then add it to a project. We should def align on what we think is best for how we should multi-line posts in LHN from comments on reports as well as expenses, tasks, ???

dannymcclain commented 6 months ago

We should def align on what we think is best for how we should multi-line posts in LHN from comments on reports as well as expenses, tasks, ???

cc @Expensify/design in case y'all have any strong feelings about this. (My feelings are all pretty mild right now.)

dubielzyk-expensify commented 6 months ago

Yeah agree, but very mild here too. I think showing it the preview on 1 line is desirable, but without double spaces. That being said it's an extremely minor issue

dannymcclain commented 6 months ago

I think showing it the preview on 1 line is desirable, but without double spaces. That being said it's an extremely minor issue

Totally agree.

shawnborton commented 6 months ago

Yup, similar feelings here as well.

Interesting that we even allow multi-line descriptions though and render them that way on the preview card... just flagging @trjExpensify for vis

trjExpensify commented 6 months ago

Interesting that we even allow multi-line descriptions though and render them that way on the preview card... just flagging @trjExpensify for vis

I think for this one we have a cap at something like 40 characters or something like that, I can't recall exactly what it is but I remember it from before.

trjExpensify commented 6 months ago

As for the bug in the OP, I agree it's really minor. @bernhardoj has identified his PR that caused it though, so can't we fix it as a follow-up to remove the second white space in Search? @parasharrajat for the C+ review as well to keep both on it.

parasharrajat commented 6 months ago

I think updating the regex is a better solution. I think @bernhardoj's proposal works great.

:ribbon: :eyes: :ribbon: C+ reviewed

melvin-bot[bot] commented 6 months ago

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

carlosmiceli commented 6 months ago

@trjExpensify @mallenexpensify @shawnborton sorry to be a bit confused here, but what would be the course of action for me at this point? I see that there's a proposal that's widely approved by everyone here, so how do I help this move forward?

mallenexpensify commented 6 months ago

@carlosmiceli , this one is a lil tricky. I assigned to @parasharrajat and @bernhardoj cuz of the below.

@bernhardoj has identified his PR that caused it though, so can't we fix it as a follow-up to remove the second white space in Search? @parasharrajat for the C+ review as well to keep both on it.

shawnborton commented 6 months ago

I guess we want to make sure the preview in the LHN only shows one space between the two lines.

But for me... I am wondering, do we want to accept multi-line descriptions in the preview card as well? Or should we force the line breaks to just become spaces?

Basically we don't allow for line breaks when you set a merchant, but we do when you set a description. And it creates an inconsistency when the preview card has a merchant vs when it has a description with a line break.

I don't feel too strongly here though, just noting that it's different. I think I would prefer to remove the line breaks and just use one line of text and stay consistent with truncating everything after two lines, both for merchants and descriptions

Also note, this is what it looks like when your description has three lines 🤦 we should fix this as well to remove the trailing ellipsis: CleanShot 2024-05-13 at 08 04 13@2x

dannymcclain commented 6 months ago

I think I would prefer to remove the line breaks and just use one line of text and stay consistent with truncating everything after two lines, both for merchants and descriptions

Also note, this is what it looks like when your description has three lines 🤦 we should fix this as well to remove the trailing ellipsis:

I'm on board for that update Shawn, and agree we should do our best to fix that little ellipsis dangler.

parasharrajat commented 6 months ago

@bernhardoj Could you please add changes for that into your proposal?

dubielzyk-expensify commented 6 months ago

I think I would prefer to remove the line breaks and just use one line of text and stay consistent with truncating everything after two lines, both for merchants and descriptions

This makes sense to me too. 1 line work better especially when it's a preview and the full info is a tap away anyway.

parasharrajat commented 6 months ago

@bernhardoj We can start working on the PR. Looks like this issue is missing exported label which creates the Up job.

bernhardoj commented 6 months ago

The PR is here

cc: @parasharrajat

carlosmiceli commented 6 months ago

@parasharrajat Just added the external label because I noticed it was missing, is that what you meant?

carlosmiceli commented 5 months ago

What's the status of this? @dannymcclain @mallenexpensify

parasharrajat commented 5 months ago

PR is merged.

bernhardoj commented 5 months ago

I think we need a payment here?

cc: @parasharrajat

parasharrajat commented 5 months ago

@carlosmiceli yes payment is pending.

melvin-bot[bot] commented 5 months ago

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

melvin-bot[bot] commented 5 months ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.4.76-7 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/42243

If no regressions arise, payment will be issued on 2024-06-05. :confetti_ball:

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

@parasharrajat requires payment through NewDot Manual Requests
@bernhardoj requires payment (Needs manual offer from BZ)

melvin-bot[bot] commented 5 months 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:

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

mallenexpensify commented 5 months ago

@carlosmiceli , when contributors are assigned to the issue (or, in the rare instance where they're not but they reviewed the associated/linked PR) we want to keep the issue open for payment. The BZ will manage the payment as well as ensuring a TestRail case is created, when needed.

carlosmiceli commented 5 months ago

My bad everyone, still learning some of the contributors' systems, thanks for the lesson @mallenexpensify 🙏

melvin-bot[bot] commented 5 months ago

Payment Summary

[Upwork Job]()

Reviewer: @parasharrajat owed $250 via NewDot
ROLE: @bernhardoj paid $(AMOUNT) via Upwork (LINK)

BugZero Checklist (@mallenexpensify)

[ ] 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

parasharrajat commented 5 months 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:

[x] [@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/39525
[x] [@parasharrajat] 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: https://github.com/Expensify/App/pull/39525/files#r1628296997
[x] [@parasharrajat] 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: Not needed
[x] [@parasharrajat] Determine if we should create a regression test for this bug. Yes
[x] [@parasharrajat] 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.

Regression Test Steps

Go to 1:1 DM with user that has no unsettled IOU.
Create a money request with multiline description.
Verify the LHN subtitle is shown as a single line.
Open the search page and make sure the chat subtitle is shown as single line.

Do you agree 👍 or 👎 ?

mallenexpensify commented 5 months ago

@bernhardoj can you please accept the job and reply here once you have? https://www.upwork.com/jobs/~01aab5a64c2284bdc2

TR GH created, thanks @parasharrajat https://github.com/Expensify/Expensify/issues/402725

bernhardoj commented 5 months ago

@mallenexpensify accepted!

mallenexpensify commented 5 months ago

Contributor: @bernhardoj paid $250 via Upwork Contributor+: @parasharrajat owed $250 via NewDot

Thanks!

parasharrajat commented 4 months ago

Payment requested as per https://github.com/Expensify/App/issues/41587#issuecomment-2159190056

JmillsExpensify commented 4 months ago

$250 approved for @parasharrajat

Expensify / App