[HOLD for payment 2023-10-09] HIGH: LHN logic is broken for paid reports

[JmillsExpensify]

Correct logic as follows:

policyExpenseChat (member view)

• First line: %workspaceName% (note: name depends on who’s looking at it, this is the member case)
• Second line: latest message in the workspace chat. We a report has just paid paid, that's %workspaceName% paid %amount%

Report (member & admin)

• First line: report name (e.g. %workspaceName% paid %amount%)
• Second line: latest message, (e.g. %userDisplayName% paid %amount%

At the moment the policyExpenseChat is incorrectly showing the second line from the report (e.g. Niki paid...) instead of what should should actually be the second line for the policyExpenseChat.

Example

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0159fb834042f1a338
• Upwork Job ID: 1698698179177246720
• Last Price Increase: 2023-09-04

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0159fb834042f1a338

[melvin-bot]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Bug0 Triage Checklist (Main S/O)

• This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
• This bug is not a duplicate report (check E/App issues and #expensify-bugs)
  • If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
• This bug is reproducible using the reproduction steps in the OP. S/O
  • If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
  • If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
• This issue is filled out as thoroughly and clearly as possible
  • Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
• I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

[melvin-bot]

Triggered auto assignment to Contributor Plus for review of internal employee PR - @mananjadhav (Internal)

[JmillsExpensify]

Taking this as part of waves

[melvin-bot]

@JmillsExpensify, @mananjadhav 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

[JmillsExpensify]

Adding this to wave4 so that we ensure it gets picked up.

[mountiny]

Closed #26542 in favour of this one, posting my findings from there here as well:

---

I have tried to get to the bottom of this but its getting quite convoluted with all the perspectives and if user is owed or its settled. Its doable just the logic is getting too tricky and its prone to cause some regressions in future. Might have to clean this up a bit.

I will move this to weekly and will look into it later.

Maybe @koko57 or @BeeMargarida would be interested in taking this on.

Currently the getReportPreviewMessage for both Money report and the workspace or IOU chat last message in LHN. But its misleading and I think we should separate the logic of the report preview action and the paid report action message into two functions

Then I think we should simplify the translations that in case of expense report, the paid message will always include the Policy name same as in case of Workspace owes £40 we would have Workspace paid £40. Then we need to update the logic that adds the last actor to not add it for this kind of message in LHN

[mountiny]

This does not have to be internal, but he logic is quite complicated at this point and we are reusing the same getReportPreviewMessage method to get the message for report preview and also for the money report, which was fine until we have added more than an admin role and I think we need to separate this.

I think it would be a good issue for some agency

[koko57]

@mountiny I'm currently working on another ticket but as I'm waiting for a PR review I can have a look 🙂

[mountiny]

🎉 lets do this!

Let us know if things are not clear from the issue body?

I think we will have to split the getReportPreviewMessage into two methods to make this cleaner even though they share lots of similar logic

[koko57]

@mountiny @JmillsExpensify
So just to make sure:
in this case

the second line should be: "TEST paid 3.00 elsewhere"?

[mountiny]

cc @JmillsExpensify

Correct, In this case there will be the workspace name twice

[koko57]

@mountiny I've found the culprit here:
the name displayed for the member was created by this line of code (because we have lastActorAccountID and fulfill other conditions)

App/src/libs/SidebarUtils.js

Lines 291 to 292 in 95c9e47

	let lastMessageText =
	hasMultipleParticipants && lastActorDetails && lastActorDetails.accountID && Number(lastActorDetails.accountID) !== currentUserAccountID ? `${lastActorDetails.displayName}: ` : '';

and then added to the lastMessageTextFromReport here

App/src/libs/SidebarUtils.js

Line 293 in 95c9e47

lastMessageText += report ? lastMessageTextFromReport : '';

(btw do we need this condition with report? the function returns when there is no report)

but as we don't add the payer to the string like here

App/src/libs/ReportUtils.js

Line 1467 in 95c9e47

return Localize.translateLocal('iou.payerOwesAmount', {payer: payerName, amount: formattedAmount});

but we pass only amount

App/src/libs/ReportUtils.js

Line 1459 in 95c9e47

return Localize.translateLocal(translatePhraseKey, {amount: formattedAmount});

we end up with incomplete string for the workspace admin

and the string with added name for the member

Starting working on my proposal, I guess the solution can be easy to implement and won't require splitting the logic of getReportPreviewMessage for the report and policyExpenseChat, but I need to check if it won't cause any regressions.

PS. I also wondered why it works properly for 'owes' action -> we pass payer's name to the string and the condition for adding the name with colon is false - the lastActorAccountId is 0. Is it correct behavior?

[koko57]

and one more question: we don't need the ":" after the workspace/payer's name?

[JmillsExpensify]

Still waiting on the checklist and then we can close this out.

[mananjadhav]

I am not sure if we should treat this as a regression as I couldn't find a PR where we had this messaging requirement. @mountiny do you have that? Hence, I don't have a specific offending PR.

But I do feel we should add a regression test. QA Steps from the PR look good to me.

[mountiny]

@mananjadhav I agree finding the regression for this can be tricky due to complexity of the LHN code

Can you comment the steps here so its easier for the bugZero assignee to handle? Thanks!

[trjExpensify]

👋 Did the work for this issue break the iou/expenseReport preview logic?

It should be:

[%displayName% || %workspaceName%] owes £21.96
%firstName%: requested £5.00 for [%merchant% || %comment%]

I.e

IOU Report

Jeff Banks owes £21.96
Tom: requested £5.00 for Thursday Lunch

Expense Report

Duraflame owes £21.96
Tom: requested £5.00 for Home Depot

[JmillsExpensify]

Great reason that we need a regression test for this case. @mananjadhav do you mind suggesting one please?

[trjExpensify]

Hm, and another one. Why isn't this reportAction coming from the reimburser who clicked Pay? We're now using the workspace name, so that means there's no audit history of who actually paid an expenseReport on behalf of the company. That's not great for compliance, we need to fix that as well please!

[mananjadhav]

Regression Test proposal:

1. Login to the app in two devices - one as a workspace member (assume userA) and the other as the owner (ownerB).

2. As the member, request money (assume $50) from the workspace (example controlRoom)

3. As the owner pay for the request.

4. As the member you should see the policy expense chat in LHS as well as preview as:
   <workspace-name> paid <amount> <payment-method>

   With our example it would:
   controlRoom paid $50 elsehwere

@mountiny @JmillsExpensify ^

Also should we create a new issue for @trjExpensify's comment ?

[mountiny]

I agree with Manan that this PR did not touch the logic of who the action is coming from.

And it feels like the subscript in of the paid expense report, that could be mixture of the backend changes to return both participants in the expense report and this change.

Either way we made this logic exteremely brittle depending of various things and these never ending regressions are consequence of it. I am not sure how to tackle this.

@koko57 we could try to abstract away some of the logic to determine to LHN preview messages and then create unit tests for that method based on various scenarios of the reports, would that be feasible?

[koko57]

@trjExpensify on your comment: #26669 (comment) could you please explain what is wrong here? My PR is only adding "[LAST_ACTOR_NAME]:" where necessary (it was originally also adding the workspace name for the workspace requests #27700 (comment)).

@mountiny @trjExpensify So we want to display a different message in the chat (with the payer's name) and the workspace name in the sidebar?

[mountiny]

@koko57 I dont think this comes from your PR as it tested well there, there must have been some other change. However, if you have time to investigate that would be helpful

And yeah you are correct, in the LHN we should show the Workspace paid, but in the expense report itself for auditing purposes we should show which person actually paid cc @trjExpensify

[trjExpensify]

And yeah you are correct, in the LHN we should show the Workspace paid,

You're talking about the title of the expenseReport in the LHN, yeah?

[mountiny]

Both of these

[trjExpensify]

Ah, so no.. the preview is of the report action added to the report and should come from whichever user took the action.

When someone pays an expenseReport:

1. The reportAction added to the report comes from the reimburser who paid
2. The LHN title updates to "%workspaceName% paid %reportTotal%"
3. The LHN preview shows (1).

I.e

Report Action

Tom Rhys Jones 11:58am
paid $50.00 elsewhere

LHN

Duraflame paid $50.00
Tom: paid $50.00 elsewhere

[mountiny]

Ah I missed it with the workspace chat preview

Then there its correctly saying the %workspaceName% paid this

[trjExpensify]

Yup, the workspace chat preview is correct because it's a preview of the report which title updated in (2) above.

[JmillsExpensify]

Shall we create a fresh issue then? For what it's worth, I like this idea from @mountiny:

@koko57 we could try to abstract away some of the logic to determine to LHN preview messages and then create unit tests for that method based on various scenarios of the reports, would that be feasible?

[JmillsExpensify]

P.S. Regression test has been added.

[JmillsExpensify]

$500 payment approved for @mananjadhav based on this summary.

[koko57]

@JmillsExpensify @mountiny should I start working on this one? 🙂 Will a new ticket be created?

[mananjadhav]

New issue would be helpful and I can help with the review.

[mountiny]

Yes @trjExpensify would you be able to create a new one for this? You will be the best in defining exactly what needs to be done

[trjExpensify]

@koko57 can you write a message on the new issue so I can assign you? Thanks! #30042

[mountiny]

Thanks this can be closed then ❤️