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.52k stars 2.87k forks source link

[$250] Workspace - Approve button returns briefly for the admin after approving the expense #43783

Closed kavimuru closed 1 week ago

kavimuru commented 4 months 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: 1.4.83-1 Reproducible in staging?: y Reproducible in production?: y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4627303&group_by=cases:section_id&group_id=306206&group_order=asc 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:

Action Performed:

Precondition: Create a Collect workspace in ND with an expensifail account. Invite a Gmail account as a member. Enable Workflows and add a Plaid Regions bank account.

  1. Member: Navigate to https://staging.new.expensify.com/

  2. Member: Log in

  3. Admin: Log in

  4. Member: Submit a manual expense to the workspace chat

  5. Admin: Quickly open the IOU and approve it

Expected Result:

"Pay with Expensify" should be there until the expense is paid.

Actual Result:

"Pay with Expensify" button appears after approving, after a few seconds, it reverts to "Approve" and back to "Pay with Expensify".

Workaround:

unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/43996225/8f7906fb-c568-419d-9c6a-99300e943b83

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01ee0bafa7bb3dc0e3
  • Upwork Job ID: 1802955877876368442
  • Last Price Increase: 2024-10-22
  • Automatic offers:
    • ZhenjaHorbach | Reviewer | 102863571
Issue OwnerCurrent Issue Owner: @
Issue OwnerCurrent Issue Owner: @techievivek
melvin-bot[bot] commented 2 months ago

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

trjExpensify commented 2 months ago

I'm thinking of closing this dupe in favour of this issue as we've progressed further here. That said, can we confirm this will solve for the Submit button on delayed submission, referenced here. I can't see why not, seems to be the same bug.

@techievivek please also review the proposal so we can move it on. ;)

mountiny commented 2 months ago

@danieldoglas @chrispader (I believe Chris worked on part of this) would you be able to check out the proposal too, I am not 100% sure if this is safe to do

Change SaveResponseInOnyx method and even if we detected the gap between FE state and BE state apply a response for the current request, then put the queue on hold, and run getMissingOnyxUpdates request.

I do not know any specific problem with this but just think there is a reason why its built the way its now

techievivek commented 2 months ago

@techievivek please also review https://github.com/Expensify/App/issues/43783#issuecomment-2252404214 so we can move it on. ;)

I looked through the proposal, but both solutions need verification from experts. Vit has already tagged Daniel, so I will wait for them to review it and share their thoughts.

Also ccing @iwiznia and @tgolen since this change is related to Onyx updates. Thanks.

danieldoglas commented 2 months ago

@techievivek I think we need to investigate the method that is being called for approving the report. It's probably not returning all the onyxUpdates it should, which then generates the gap for the next request.

tgolen commented 2 months ago

I don't have any immediate thoughts, but I am intently following along!

Change SaveResponseInOnyx method and even if we detected the gap between FE state and BE state apply a response for the current request, then put the queue on hold, and run getMissingOnyxUpdates request.

I do not know any specific problem with this but just think there is a reason why its built the way its now

The reason this was done is so that everything happens to the client in the right order. If there is a gap of Onyx updates, that gap should be applied before the updates in the current response, or else the updates in the gap might overwrite the changes in the current response.

iwiznia commented 2 months ago

Tim is right and we should not change that behavior. It seems that in this flow we always go through the getMissingOnyxUpdates flow? If so what we need to do is figure out why that happens, as that should be rare and only happen when you missed an onyx update.

ZhenjaHorbach commented 2 months ago

Not overdue

trjExpensify commented 2 months ago

@techievivek have you been able to investigate this suggestion?

@techievivek I think we need to investigate the method that is being called for approving the report. It's probably not returning all the onyxUpdates it should, which then generates the gap for the next request.

techievivek commented 2 months ago

No, I haven't been able to triage this again, I will take a look into it by tomorrow at max.

techievivek commented 2 months ago

I looked through the request, and it seems like some updates are missing in the response. As a result, we call GetMissingOnyxMessages to fetch those updates.

Screenshot 2024-08-20 at 7 02 29 PM Screenshot 2024-08-20 at 7 06 39 PM
iwiznia commented 2 months ago

Yeah that sounds wrong, can you debut it further to figure out why that happens?

melvin-bot[bot] commented 2 months ago

@pasyukevich, @stephanieelliott, @techievivek, @ZhenjaHorbach Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

melvin-bot[bot] commented 2 months ago

@pasyukevich, @stephanieelliott, @techievivek, @ZhenjaHorbach 6 days overdue. This is scarier than being forced to listen to Vogon poetry!

stephanieelliott commented 2 months ago

Hey @techievivek have you had a chance to debug any further on this one?

techievivek commented 2 months ago

No updates from last week. I will debug this further this week.

trjExpensify commented 2 months ago

Thanks, that would be great because this is pretty janky in a mainline core flow. 👍

stephanieelliott commented 2 months ago

Just a note that this issue is linked to an external deadline of Sept 9th, if you can try and prioritize it soon @techievivek!

techievivek commented 1 month ago

Hey, I have a few other similar priority issue on my plate, can we look for a volunteer here, thanks. 🙇

trjExpensify commented 1 month ago

Ah, out of curiosity, which issues are those @techievivek? This is a long standing issue at this point in a core usage flow, I'd rather not go back to the drawing board with catching someone up with the history if priorities can align.

techievivek commented 1 month ago

I'm currently handling two Fast-API issues: #419963 and #408736. Additionally, there's a cleanup task related to these issues that needs to be prioritized to fully utilize the improved performance: #427142.

techievivek commented 1 month ago

There is another wave-control issue on my plate where we are going back and forth with the reviews https://github.com/Expensify/Auth/pull/11996

trjExpensify commented 1 month ago

Thanks!

I'm currently handling two Fast-API issues: https://github.com/Expensify/Expensify/issues/419963 and https://github.com/Expensify/Expensify/issues/408736.

Both of these are LOW and HIGH in #fast-apis, which is below CRITICAL where this issue is classified in #wave-collect as we focus on bugs in core flows as the deliverable for the SuiteWorld release.

There is another wave-control issue on my plate where we are going back and forth with the reviews https://github.com/Expensify/Auth/pull/11996

That one looks like it's attached to the Violations in Auth project which is classified as HIGH in the OP tracker, and #wave-control list of projects in the channel here.

So I would suggest this issue takes priority 👍

zanyrenney commented 1 month ago

sorry did not mean to touch the label here. readding!

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

trjExpensify commented 1 month ago

@techievivek just wondering if you had any thoughts on my comment here: https://github.com/Expensify/App/issues/43783#issuecomment-2340272990

stephanieelliott commented 1 month ago

@techievivek another bump on this -- the two Fast-API issues you referenced were closed yesterday, can we bump the priority of this one now?

techievivek commented 1 month ago

Yes, it's now a top priority for me. Since I'm still getting familiar with the flow, it's taking a bit longer, I will update the GH today.

techievivek commented 1 month ago

Hmm, I can't reproduce this anymore. I'm going to ask QA if it's still reproducible. P.S. Asked here https://expensify.slack.com/archives/C9YU7BX5M/p1726501151009569

kavimuru commented 1 month ago

@techievivek The app has changed a bit, now tester only have the pay button without additional setup, but tester can still repro the issue with the pay button.

https://github.com/user-attachments/assets/2cb0c404-acad-4250-b35b-8af967dcdeac

https://github.com/user-attachments/assets/2bc79186-1a2e-451d-b643-a12b34e969af

techievivek commented 1 month ago

@kavimuru

The app has changed a bit, now tester only have the pay button without additional setup, but tester can still repro the issue with the pay button.

I think you will need to be on control policy and have the approval flow set so that when we submit an expense above a certain amount, admins need to manually approve it before paying.

techievivek commented 1 month ago

@trjExpensify Are you able to reproduce this? I have been trying to reproduce this since yesterday but have had no luck. Am I holding anything wrong here?

https://github.com/user-attachments/assets/2fb4409a-2205-46f2-bab2-adee4f4c1189

techievivek commented 1 month ago

CC @stephanieelliott can you also give it a look and see if you can reproduce it? Thanks.

trjExpensify commented 1 month ago

@techievivek The app has changed a bit, now tester only have the pay button without additional setup, but tester can still repro the issue with the pay button.

QA repro'd 18 hours ago in that video? Let's confirm their setup.

melvin-bot[bot] commented 1 month 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 1 month ago

@pasyukevich, @stephanieelliott, @techievivek, @ZhenjaHorbach Whoops! This issue is 2 days overdue. Let's get this updated quick!

trjExpensify commented 1 month ago

@techievivek did you confirm the setup in Slack?

melvin-bot[bot] commented 1 month 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 1 month ago

@pasyukevich, @stephanieelliott, @techievivek, @ZhenjaHorbach Still overdue 6 days?! Let's take care of this!

stephanieelliott commented 1 month ago

Hey @techievivek, what's the latest here?

techievivek commented 1 month ago

I have been ooo for the last four working days. I'm catching up, but updating here that I haven't yet confirmed the setup used by QA on Slack.

trjExpensify commented 1 month ago

Okay, well the first diff I see in your video here versus the prerequisite steps in the OP (copied below), and QA's video here, is that it doesn't look like you're testing with a VBBA connected to the workspace. I'd start there.

Precondition: Create a Collect workspace in ND with an expensifail account. Invite a Gmail account as a member. Enable Workflows and add a Plaid Regions bank account.

techievivek commented 1 month ago

I tested this again following Tom's comment above. While I couldn’t reproduce the original issue mentioned in the GH OP https://github.com/Expensify/App/issues/43783#issue-2353765649, I was able to replicate the problem Applause highlighted here: https://github.com/Expensify/App/issues/43783#issuecomment-2353526195. Specifically, I had to click 'Pay with Expensify' twice for the payment to go through, which is definitely a bug.

techievivek commented 1 month ago

https://github.com/user-attachments/assets/065b4bdc-b469-4da9-864c-da388d1358e9

melvin-bot[bot] commented 1 month ago

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

trjExpensify commented 1 month ago

Great! It would be ideal to replicate the exact conditions to rule those out for the approve button flash - so don't have dupes to cause Hold to be another variable in the mix, and per the OP, the invited member is a gmail account.

Either way, agree the Pay button shouldn't need to be clicked twice. Also, did you keep an eye on the LHN, see how that preview messages goes from approved > paid > approved > paid?

https://github.com/user-attachments/assets/c2688fd7-ef95-480a-a592-6116f99743f5

melvin-bot[bot] commented 1 month ago

@pasyukevich, @stephanieelliott, @techievivek, @ZhenjaHorbach Whoops! This issue is 2 days overdue. Let's get this updated quick!

techievivek commented 4 weeks ago

I noticed an inconsistency while testing locally. It seems we are creating two MANUALREIMBURSED actions when trying to process the expense request, which is why it requires clicking the pay button twice. Something seems off in my DEV environment, so I'll continue investigating once I get newdot running again.

melvin-bot[bot] commented 3 weeks ago

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