search

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.3k stars 2.74k forks source link

[$250] Android - LHN - Sending a text+attachment displays text with ellipsis. #46190

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: 9.0.12 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:

  1. Launch app
  2. Tap on a report
  3. Send a text+attachment choosing a image from gallery
  4. Navigate to LHN

Expected Result:

Sending a text+attachment must display text+attachment in LHN

Actual Result:

Sending a text+attachment displays text with ellipsis

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

https://github.com/user-attachments/assets/8c10f84d-4d8e-45e0-b894-d125f1c792d6

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0143efae25694044fa
  • Upwork Job ID: 1819194218797436242
  • Last Price Increase: 2024-08-02
Issue OwnerCurrent Issue Owner: @mollfpr
melvin-bot[bot] commented 1 month ago

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

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

RachCHopkins commented 1 month ago

@lanitochka17 do we know what "Sending a text+attachment must display text+attachment in LHN" means? I can't decipher the meaning of this statement, what should show in the LHN?

lanitochka17 commented 1 month ago

That is the word Attachment should be displayed instead of ...

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

Job added to Upwork: https://www.upwork.com/jobs/~0143efae25694044fa

melvin-bot[bot] commented 1 month ago

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

RachCHopkins commented 1 month ago

Thanks @lanitochka17

daledah commented 1 month ago

Proposal

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

Sending a text+attachment displays text with ellipsis

What is the root cause of that problem?

In many places we use formatReportLastMessageText

The idea is that we'll format the last message text correctly by replacing line breaks with spaces, trim the value to a max length, ... https://github.com/Expensify/App/blob/5005f595661607d563d8ae3bd72e763971d9d873/src/libs/ReportUtils.ts#L1789

However we don't use it consistently for all places, so in this text + attachment case, formatReportLastMessageText is not applied and the last message text is displayed with line breaks like

24

[Attachment]

As the last message text has numberOfLines 1, it will show ellipsis.

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

We need to make sure to apply formatReportLastMessageText consistently for all cases of last message text. The best way is to update right before we display it, here to

{ReportUtils.formatReportLastMessageText(Parser.htmlToText(optionItem.alternateText))}

After that optionally we can remove some other usage of formatReportLastMessageText like the ones mentioned in RCA, because using it here should be adequate for the last message text to be displayed correctly.

What alternative solutions did you explore? (Optional)

Use ReportUtils.formatReportLastMessageText and Parser.htmlToText for individual cases when we set the result.alternateText:

Another thing we could contemplate is to trim unnecessary spaces in formatReportLastMessageText, so if there's any multiple spaces sequentially they could be replaced by 1 space. However that seems out of scope of the issue.

github-actions[bot] commented 1 month ago

true

daledah commented 1 month ago

Proposal modified to add a small detail

github-actions[bot] commented 1 month ago

true

melvin-bot[bot] commented 1 month ago

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

mollfpr commented 1 month ago

@daledah Could you explain why it's only happen in native?

daledah commented 1 month ago

@mollfpr Great question! That's because in web, the alternate text has white-space: no-wrap, so both text and attachment are shown on the same line due to no-wrap behavior. Meanwhile on native there's no such property for Text. whiteSpace is disabled specifically for native as can see here.

So the problem is mostly only visible in native, although in web the alternateText is also formatted incorrectly but the no-wrap behavior hides the bug.

mollfpr commented 1 month ago

@daledah That's make sense to me!

The proposal looks good, but I would prefer the alternative solution incase we will use getOptionData in somewhere else then the alternateText is well formatted.

🎀 👀 🎀 C+ reviewed!

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

📣 @daledah You have been assigned to this job! Please apply to the Upwork job and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Once you apply to this job, your Upwork ID will be stored and you will be automatically hired for future jobs! Keep in mind: Code of Conduct | Contributing 📖

greg-schroeder commented 1 week ago

fyi @RachCHopkins the PR for this issue led to a regression (https://github.com/Expensify/App/issues/47248). I'm not paying folks in that issue, but they should be subject to the regression penalty in this one (if you haven't paid them yet, which it seems like you haven't)

melvin-bot[bot] commented 1 week ago

This issue has not been updated in over 15 days. @mollfpr, @RachCHopkins, @techievivek, @daledah eroding to Monthly issue.

P.S. Is everyone reading this sure this is really a near-term priority? Be brave: if you disagree, go ahead and close it out. If someone disagrees, they'll reopen it, and if they don't: one less thing to do!

RachCHopkins commented 2 days ago

@techievivek @mollfpr I can't actually tell what's going on with this issue - is someone able to give me a quick summary of where it's at?

mollfpr commented 2 days ago

@RachCHopkins I think it's ready for the payment summary. The original PR and follow-up PR were in production last month.