Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[$500] LHN - Expense details report title does not show the amount in LHN after relogin #32382

Closed
6 tasks done
lanitochka17 opened this issue Dec 1, 2023 · 28 comments
Closed
6 tasks done
Assignees
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors

Comments

@lanitochka17
Copy link

lanitochka17 commented Dec 1, 2023

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.7-0
Reproducible in staging?: Y
Reproducible in production?: N
If this was caught during regression testing, add the test name, ID and link from TestRail:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:

Issue found when executing PR #31302

Action Performed:

  1. Go to staging.new.expensify.com
  2. Go to workspace chat > + > Request money
  3. Create a manual expense
  4. Open manual expense details page and send a message
  5. Open another chat and send a message (if on web and desktop app)
  6. Log out
  7. Log in

Expected Result:

The title for expense details report in LHN shows the correct amount

Actual Result:

The title for expense details report in LHN does not show the amount at all after relogin. It only shows the amount after opening the report

Workaround:

Unknown

Platforms:

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

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6297756_1701457553231.Screen_Recording_20231202_003805_Chrome.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~012c05b0ef5839e23d
  • Upwork Job ID: 1731684125096988672
  • Last Price Increase: 2023-12-18
@lanitochka17 lanitochka17 added the DeployBlockerCash This issue or pull request should block deployment label Dec 1, 2023
Copy link
Contributor

github-actions bot commented Dec 1, 2023

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

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

Copy link

melvin-bot bot commented Dec 1, 2023

Triggered auto assignment to @joelbettner (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@situchan
Copy link
Contributor

situchan commented Dec 2, 2023

I don't think this is blocker. Staging is even better than production which shows nothing in LHN title

Before opening reports:

Staging:

Screenshot 2023-12-03

Production:

Screenshot 2023-12-03

After reverting #31302:

Screenshot 2023-12-03

After opening reports:

Screenshot 2023-12-03

@melvin-bot melvin-bot bot added the Overdue label Dec 4, 2023
@joelbettner
Copy link
Contributor

@situchan I agree that this should not be a blocker, since it is at least an improvement over production.

@melvin-bot melvin-bot bot removed the Overdue label Dec 4, 2023
@joelbettner joelbettner added Overdue External Added to denote the issue can be worked on by a contributor and removed DeployBlockerCash This issue or pull request should block deployment Hourly KSv2 labels Dec 4, 2023
@melvin-bot melvin-bot bot changed the title LHN - Expense details report title does not show the amount in LHN after relogin [$500] LHN - Expense details report title does not show the amount in LHN after relogin Dec 4, 2023
Copy link

melvin-bot bot commented Dec 4, 2023

Job added to Upwork: https://www.upwork.com/jobs/~012c05b0ef5839e23d

@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Dec 4, 2023
Copy link

melvin-bot bot commented Dec 4, 2023

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

@melvin-bot melvin-bot bot added Daily KSv2 and removed Overdue labels Dec 4, 2023
@joelbettner
Copy link
Contributor

@situchan do you have any ideas for a solution?

@situchan
Copy link
Contributor

situchan commented Dec 4, 2023

The solution is limited. I don't think of any better solution in frontend just to show "Request" unless backend supports something for that.
cc: @puneetlath since you managed #30687

@tienifr
Copy link
Contributor

tienifr commented Dec 4, 2023

We decided to simply display Request since BE does not return transaction data on log in #31302 (comment).

@situchan
Copy link
Contributor

situchan commented Dec 4, 2023

Agree. We can close this @joelbettner

@dukenv0307
Copy link
Contributor

dukenv0307 commented Dec 4, 2023

Proposal

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

The title for expense details report in LHN does not show the amount at all after relogin. It only shows the amount after opening the report

What is the root cause of that problem?

When the report first loads, it doesn't have transaction data, so only Request shows.

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

We should rely on the fields of the money request report to show the title in LHN if transaction data is not available. We already do the same for the Expense report, that's why it still shows the amount correctly even after logged in again.

We can use fields like total for the amount and also the report.currency, but back-end will need to support this.

I think this is much better than showing only Request since the user can have multiple outstanding money requests and will not want to open each of them individually just to see the amount. This is also straight forward to implement the fix.

What alternative solutions did you explore? (Optional)

NA

@situchan
Copy link
Contributor

situchan commented Dec 4, 2023

@dukenv0307 if something should be fixed here, #30687 should have already covered this. So it should be fixed as follow-up of #30687 by same contributors. Otherwise just close.

@dukenv0307
Copy link
Contributor

dukenv0307 commented Dec 4, 2023

@situchan I think this is more like a feature request since back-end is not supporting this. So we can't expect it to be covered by an earlier bug fix. If we want to improve the UX here, we can do it in this issue.

Let's minimize discussions and see what @joelbettner thinks.

@melvin-bot melvin-bot bot added the Overdue label Dec 7, 2023
@shubham1206agra
Copy link
Contributor

@joelbettner Bump here

@melvin-bot melvin-bot bot removed the Overdue label Dec 8, 2023
@joelbettner
Copy link
Contributor

@situchan @dukenv0307 @shubham1206agra Can you please tell me what API call is used to gather the information that we use to display the info in the LHN for these money requests, and what information we'd need to make them look consistent with the information that is displayed after the report has been opened? I'll research what it would take to return that information.

@dukenv0307
Copy link
Contributor

@joelbettner When we open the App, all reports that are displayed in LHN are returned by OpenReport API. The title of transaction thread report is returned in getTransactionReportName function. Because the first time app loads, transaction data is empty so it returns Request here.

App/src/libs/ReportUtils.ts

Lines 1901 to 1902 in ab4449c

// Transaction data might be empty on app's first load, if so we fallback to Request
return Localize.translateLocal('iou.request');

So for transaction thread report, we should add some data like currency, amount, comment in report data which is returned by OpenApp API to display the correct title in LHN when transaction isn't loaded.

@melvin-bot melvin-bot bot added the Overdue label Dec 11, 2023
Copy link

melvin-bot bot commented Dec 11, 2023

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

@tsa321
Copy link
Contributor

tsa321 commented Dec 12, 2023

Proposal

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

In LHN, the title or name of chat thread of IOU request doesn't display full text after user logout then login

What is the root cause of that problem?

When user logut then login, the back end doesn't return all transactions data and when LHN is rendered the getTransactionReportName will fall in this section:

App/src/libs/ReportUtils.ts

Lines 1900 to 1902 in 86a2521

if (!isNotEmptyObject(transaction)) {
// Transaction data might be empty on app's first load, if so we fallback to Request
return Localize.translateLocal('iou.request');

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

We could use reportAction message data instead of displaying Request.
The code could be something like:
return reportAction.message[0].html;
in that line instead of return Localize.translateLocal('iou.request');
But if comment of IOU is edited the result will be different.

What alternative solutions did you explore? (Optional)

Copy link

melvin-bot bot commented Dec 12, 2023

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

Copy link

melvin-bot bot commented Dec 14, 2023

@joelbettner, @shubham1206agra Eep! 4 days overdue now. Issues have feelings too...

@shubham1206agra
Copy link
Contributor

@joelbettner bump

@melvin-bot melvin-bot bot removed the Overdue label Dec 14, 2023
@puneetlath
Copy link
Contributor

I think this behavior is expected, right @fedirjh @tienifr ?

@fedirjh
Copy link
Contributor

fedirjh commented Dec 14, 2023

@puneetlath Yes that’s correct , that's the expected behaviour : the Request title will be displayed until the request report is opened. If we want to change this behaviour then we should update the backend to return the transaction data inside the OpenApp command.

@joelbettner
Copy link
Contributor

Sorry...I'm just tied down with some other work before I can get to making those changes in OpenApp.

Copy link

melvin-bot bot commented Dec 18, 2023

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

@melvin-bot melvin-bot bot added the Overdue label Dec 18, 2023
Copy link

melvin-bot bot commented Dec 18, 2023

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

1 similar comment
Copy link

melvin-bot bot commented Dec 18, 2023

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

@joelbettner
Copy link
Contributor

I'm going to close this for now and this is the currently the intended bahavior. Perhaps the issue can be re-visited in the future if we change what our intended behavior is like.

@melvin-bot melvin-bot bot removed the Overdue label Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Daily KSv2 Engineering External Added to denote the issue can be worked on by a contributor Help Wanted Apply this label when an issue is open to proposals by contributors
Projects
None yet
Development

No branches or pull requests

9 participants