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.34k stars 2.77k forks source link

[$250] Pay with business BA - System message is "paid x", while thread header is "paid x elsewhere" #48560

Open IuliiaHerets opened 2 weeks ago

IuliiaHerets commented 2 weeks 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.29-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: https://expensify.testrail.io/index.php?/tests/view/4921007 Email or phone of affected tester (no customers): applausetester+kh010901@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

Precondition:

  1. Go to staging.new.expensify.com
  2. [Member] Go to workspace chat and submit an expense.
  3. [Admin] Approve the expense.
  4. [Admin] Go offline (because there is an error when paying with business bank account).
  5. [Admin] Click Pay with business bank account.
  6. [Admin] Right click on the paid system message > Reply in thread.

Expected Result:

The thread header for paid system message should be the same as the system message.

Actual Result:

The system message is "paid x", while the thread header shows "paid x elsewhere".

Workaround:

Unknown

Platforms:

Screenshots/Videos

https://github.com/user-attachments/assets/ac922658-e485-498e-a10a-9e38468ee019

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021831454888955451181
  • Upwork Job ID: 1831454888955451181
  • Last Price Increase: 2024-09-18
Issue OwnerCurrent Issue Owner: @
melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

Triggered auto assignment to @AndrewGable (DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.

github-actions[bot] commented 2 weeks ago

:wave: 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.
AndrewGable commented 2 weeks ago

Trying to figure out which PR introduced this since it doesn't happen on prod

AndrewGable commented 2 weeks ago

@bondydaa / @MariaHCD - I am curious if you think this revert revert PR could possibly be related? https://github.com/Expensify/App/pull/48421

AndrewGable commented 2 weeks ago

After discussion in Slack we think this is probably a regression from the linked PR, we are going to work with @bondydaa @brandonhenry @allroundexperts to fix this regression!

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

Current assignee @allroundexperts is eligible for the External assigner, not assigning anyone new.

bondydaa commented 2 weeks ago

also throwing the external label on here, ideally @brandonhenry can fix but if not no need to block for that either.

jaydamani commented 2 weeks ago

Proposal

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

Thread title and orignal message are incosistent for payment action message

What is the root cause of that problem?

Both strings come from different functions that are incosistent.

The thread title comes from message field in reportAction which is added as part of optimistic data for payMoneyRequest. The optimistic data gets this message from getIOUReportActionMessage. The getIOUReportActionMessage function returns Paid x elsewhere by default.

The message content comes from getIOUReportActionDisplayMessage. The getIOUReportActionDisplayMessage returns Paid x by default.

This causes inconsistency between thread title and the message content.

getIOUReportActionMessage: https://github.com/Expensify/App/blob/8a35256bff4d66d0f0958fd9100e191a8fda404d/src/libs/ReportUtils.ts#L4467-L4475

getIOUReportActionDisplayMessage: https://github.com/Expensify/App/blob/8a35256bff4d66d0f0958fd9100e191a8fda404d/src/libs/ReportUtils.ts#L6933-L6944

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

Paid x feels more right so, we can update getIOUReportActionMessage to return Paid x by default. This function also has some strings hardcoded in english so maybe we should also localize this.

What alternative solutions did you explore? (Optional)

Option 1

Update getIOUReportActionDisplayMessage to return Paid x elsewhere.

Option 2

getIOUReportActionMessage is only used for building optmistic reports and for showing last message so maybe we can replace it with getIOUReportActionDisplayMessage.

bondydaa commented 1 week ago

i'll be OOO next week so if we get proposals ready please ask for another engineer to review

isabelastisser commented 1 week ago

@allroundexperts, can you please review the proposal above? Thanks!

allroundexperts commented 1 week ago

Thanks for the proposal @jaydamani. I think with your proposal, we'll show Paid x even on the report messages as well, which seems incorrect to me. @bondydaa @isabelastisser Should we replace this message with Paid x everywhere?

jaydamani commented 1 week ago

Thanks for the proposal @jaydamani. I think with your proposal, we'll show Paid x even on the report messages as well, which seems incorrect to me. @bondydaa @isabelastisser Should we replace this message with Paid x everywhere?

Paid x felt more right since it was paid through expensify but if Paid x elsewhere is correct then we can apply option 1 in alternate solution.

isabelastisser commented 1 week ago

Reassigning since Bondy is OOO.

melvin-bot[bot] commented 1 week ago

Auto-assigning issues to engineers is no longer supported. If you think this issue should receive engineering attention, please raise it in #whatsnext.

isabelastisser commented 1 week ago

@AndrewGable @MariaHCD, can you please weigh in since you were previously part of this discussion? Thanks!

melvin-bot[bot] commented 1 week ago

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

isabelastisser commented 1 week ago

Alright, I'm inclined to say that we should replace this message withPaid x everywhere, but I think I should ask @shawnborton what he thinks since this falls into a design change.

isabelastisser commented 5 days ago

Bump @shawnborton, what do you think?

shawnborton commented 5 days ago

This is a weird one. But my thoughts are that the chat header for the thread view should match the exact message that was threaded. So it's weird that the system message just says "Paid $XX.XX" but the thread header says "Paid $XX.XX with Expensify"

I'm not sure if the bug is that the system message actually needs to say "Paid $XX.XX with Expensify" or if the thread header needs to be updated to match the system message. cc @trjExpensify @Expensify/design @dangrous in case you have any thoughts here.

But actually @bondydaa since this was a regression, can you explain what happened and what the expected behavior is?

dubielzyk-expensify commented 3 days ago

Yeah I can agree with that.

Alternative I'd say that the system message has more room so could actually contain more details of where it's paid, where the header has less space to use so we could stick with simply Paid $XX.XX and leave the details to the system message.

dannymcclain commented 2 days ago

But my thoughts are that the chat header for the thread view should match the exact message that was threaded.

I think I fall in this camp.

isabelastisser commented 2 days ago

Thanks for discussing this, team!

trjExpensify commented 2 days ago

But my thoughts are that the chat header for the thread view should match the exact message that was threaded.

Agree with this, they should match.

I'm not sure if the bug is that the system message actually needs to say "Paid $XX.XX with Expensify" or if the thread header needs to be updated to match the system message.

I thought the former we historically expected for the system message to read:Paid $x.xx with Expensify. I could find the PR where we updated it from "using Expensify" to "with Expensify" to match the button terminology in that regard, but can't see where/if we made a decision to drop "with Expensify" from the sys message for in-app payments along the way.

I can see an argument for both really, I'm probably leaning towards only highlighting an alt payment method when it's outside of the app for brevity. I think the expectation is to be paid via the software you're using, so only highlighting when it's some place else seems okay. Especially if we start thinking about some additional variations that are coming into play like auto-reimbursement, which would otherwise make it:

automatically paid $15.00 with Expensify via workspace rules

Instead of the below:

automatically paid $20.00 via workspace rules

shawnborton commented 2 days ago

I think the expectation is to be paid via the software you're using, so only highlighting when it's some place else seems okay

Yeah, I totally agree with that.

trjExpensify commented 1 day ago

Cool, cool! Let's make the thread header match the system message that's on prod then 👍

melvin-bot[bot] commented 18 hours ago

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

melvin-bot[bot] commented 16 hours ago

@allroundexperts @isabelastisser this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!