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

Display money request in LHN, instead of user's email #3737

Closed
isagoico opened this issue Jun 23, 2021 · 37 comments
Closed

Display money request in LHN, instead of user's email #3737

isagoico opened this issue Jun 23, 2021 · 37 comments
Assignees
Labels
Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2

Comments

@isagoico
Copy link

isagoico commented Jun 23, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Log in to e.cash
  2. Navigate to a conversation
  3. Send an IOU
  4. Navigate to IOU

Expected Result:

The message preview in LHN should mirror what is in the message, instead of the email - i.e. "Requested $20.00 from X for Y"

Actual Result:

Message preview displays the user's email. This also happens when there's a completed payment.

Workaround:

None, visual issue.

Platform:

Where is this issue occurring?

Web ✔️
iOS✔️
Android✔️
Desktop App✔️
Mobile Web✔️

Version Number: 1.0.73-2

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos:

image

View all open jobs on Upwork


From @roryabraham https://expensify.slack.com/archives/C01GTK53T8Q/p1624479971030500

When the newest action on a report is an IOU payment, the LHN just display the user's email instead of a more user-friendly message such as "Amal paid you $x"

@MelvinBot
Copy link

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

@ctkochan22 ctkochan22 added the Improvement Item broken or needs improvement. label Jun 23, 2021
@ctkochan22
Copy link
Contributor

Pushing N5 and Cashback stuff out. But labels look good.

@ctkochan22 ctkochan22 added Weekly KSv2 and removed Daily KSv2 labels Jun 23, 2021
@ctkochan22 ctkochan22 removed their assignment Jun 23, 2021
@trjExpensify
Copy link
Contributor

👋 @ctkochan22 after triaging an issue, can you make sure to add the external label so that it continues on the journey to being posted to Upwork. If it can't be worked on externally, then the internal label can be added. Going to move this along.

@trjExpensify trjExpensify added the External Added to denote the issue can be worked on by a contributor label Jun 23, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jliexpensify (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Weekly KSv2 labels Jun 23, 2021
@jliexpensify jliexpensify changed the title LHN - Email of the user is displayed in message preview if the latest action was an IOU Display money request in LHN, instead of user's email Jun 24, 2021
@jliexpensify
Copy link
Contributor

POSTED: https://www.upwork.com/ab/applicants/1407860465960755200/job-details

@MelvinBot
Copy link

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

@dklymenk
Copy link
Contributor

As far as I can tell the message displayed on LHN right after you send the payment request is correct, however the message displayed after you reload the page is wrong. This means that the issue is in getSimplifiedReportObject function, specifically the part where it extracts html from the message, however it seems like BE response doesn't have message.html key/value pair for the IOU objects there.

As far as I can tell this string Requested <amount> from you for <reason> comes from BE in 2 cases: when fetching for report history (API.Report_GetHistory call) and when getting the IOU object from pusher event, but not from this request on line 301 of Report.js:

API.Get({
        returnValueList: 'reportStuff',
        reportIDList: chatList.join(','),
        shouldLoadOptionalKeys: true,
        includePinnedReports: true,
    })

So I think the easiest way for you would be to probably update that endpoint to return that html, since 2 endpoints already return it.

But if that's not possible I can look into ways to compose this sting on FE from what we already have, although I don't think it will look good. Please let me know, if you would be interested in that.

Thanks.

@Julesssss
Copy link
Contributor

Good spot @dklymenk, you're right that we should solve this on the backend, so we'll need to put this issue on hold until this is done.

@Julesssss
Copy link
Contributor

Internal backend issue for reference

@jliexpensify jliexpensify changed the title Display money request in LHN, instead of user's email [ON HOLD] Display money request in LHN, instead of user's email Jun 25, 2021
@jliexpensify
Copy link
Contributor

@Julesssss just checking - this issue's still on hold right?

@Julesssss
Copy link
Contributor

@Julesssss just checking - this issue's still on hold right?

Yep, I've been looking into the issue but have been OOO until today.

@AndrewGable
Copy link
Contributor

Still on hold from my understanding

@MelvinBot MelvinBot removed the Overdue label Aug 13, 2021
@Julesssss
Copy link
Contributor

Yeah, on hold while higher priority tasks are worked through.

@MelvinBot
Copy link

@AndrewGable, @jliexpensify it looks like no one is assigned to work on this job.
Please double the price or add a comment stating why the job isn't being doubled.

@Julesssss
Copy link
Contributor

Internal issue is on hold.

@AndrewGable
Copy link
Contributor

Still on hold @MelvinBot

@AndrewGable
Copy link
Contributor

Going to assign to @Julesssss as he is more up to date with the internal PR than I am

@Julesssss
Copy link
Contributor

Ah, my bad I thought I was assigned too. I'll keep this one updated.

@Julesssss
Copy link
Contributor

Julesssss commented Sep 6, 2021

Okay, the backend PR is once again in review.

@Julesssss Julesssss added the Reviewing Has a PR in review label Sep 6, 2021
@jliexpensify
Copy link
Contributor

Hi @Julesssss! Just wanted to confirm what was happening with this one? Is it still on hold? Thanks!

@Julesssss
Copy link
Contributor

Hey Jason, the PR is merged and we're awaiting deployment.

@jliexpensify
Copy link
Contributor

Awesome, should we remove the [ON HOLD] in the title?

@Julesssss Julesssss changed the title [ON HOLD] Display money request in LHN, instead of user's email Display money request in LHN, instead of user's email Sep 13, 2021
@Julesssss Julesssss removed External Added to denote the issue can be worked on by a contributor Planning Changes still in the thought process labels Sep 13, 2021
@Julesssss
Copy link
Contributor

Yep done. The problem is fully solved by the backend PR, so no external help is required here.

@jliexpensify
Copy link
Contributor

Sweet, chatted to Jules and we'll close this out as there's an internal issue!

@dklymenk
Copy link
Contributor

I would appreciate it if you update this issue once the change is deployed to staging. I have two client side issues blocked by this and would like to resubmit my PR as soon as it as merged.

Thanks in advance.

@Julesssss
Copy link
Contributor

@dklymenk I'm happy to report that it was deployed to staging last night

@dklymenk
Copy link
Contributor

We are talking about staging.new.expensify.com, right?

I can still reproduce the issue.
DeepinScreenshot_select-area_20210914124815

Send an IOU and refresh the page.

@Julesssss
Copy link
Contributor

Oh no, sorry. I was referring to the staging environment. Which confusingly is not the environment for staging.new.expensify.com 😅

I will let you know once this reaches prod.

@dklymenk
Copy link
Contributor

Sorry, my bad. I forgot that dev, staging and prod front-ends all use the same prod API. 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering Improvement Item broken or needs improvement. Reviewing Has a PR in review Weekly KSv2
Projects
None yet
Development

No branches or pull requests

9 participants