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.53k stars 2.88k forks source link

[$250] Workspace chat - Approve button appears briefly Instead of pay. #49281

Closed izarutskaya closed 1 week ago

izarutskaya commented 1 month 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.35-7 Reproducible in staging?: Y Reproducible in production?: Y Email or phone of affected tester (no customers): biruknew45+973@gmail.com Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Navigate to https://staging.new.expensify.com/
  2. Create a new workspace.
  3. Submit an expense in the workspace chat.
  4. Hold the submitted expense.
  5. Create another workspace and submit an expense in that workspace chat.
  6. Go to Settings > Troubleshoot > Clear Cache, then quickly return to the workspace chat from step 5.

Expected Result:

After clearing the cache, the skeleton UI should appear, followed by the Pay button.

Actual Result:

The "Approve" button briefly appears before quickly switching to the Pay button.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/6d744c01-a75f-48d5-b399-9703410db135

Bug6605484_1726493428904!1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~021836562014848142699
  • Upwork Job ID: 1836562014848142699
  • Last Price Increase: 2024-09-19
Issue OwnerCurrent Issue Owner: @danieldoglas
melvin-bot[bot] commented 1 month ago

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

izarutskaya commented 1 month ago

We think this issue might be related to the #collect project.

CyberAndrii commented 1 month ago

This is because the OpenApp request response is missing the approverMode property for the second workspace (unlike the first). This causes the isSubmitAndClose function to incorrectly return false.

https://github.com/Expensify/App/blob/2190f6279041ed8260752294d095bbdda76faebe/src/libs/actions/IOU.ts#L6918-L6921

After some delay approverMode will be set by the OpenReport request and the correct button will be displayed.

melvin-bot[bot] commented 1 month ago

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

melvin-bot[bot] commented 1 month ago

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

paultsimura commented 1 month ago

🎀👀🎀 C+ reviewed to assign an engineer – we need to talk 👀

melvin-bot[bot] commented 1 month ago

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

paultsimura commented 1 month ago

Hey @danieldoglas – what @CyberAndrii found here is correct. Could we find out why we do not fetch full Policy data on the OpenApp call?

Do we fetch it for the default policy only?

Note the OpenApp response: it contains mergeCollection for policy_ with basic data of all 3 policies, and then a single merge operation with the full data of the first one:

image
danieldoglas commented 1 month ago

Yes, that was by design. We only return all the details for the default policy and a smaller version of the other ones.

@strepanier03 Do you think this is a bug that requires fixing (the button changing after the report is loaded)? If yes, I can add approverMode to the other policies.

paultsimura commented 1 month ago

Thanks for the confirmation @danieldoglas. Potentially, we could also implement a FE fix to show some loading animation/skeleton on this button until the policy data is fully fetched.

danieldoglas commented 1 month ago

@paultsimura do we have a loading pattern for buttons anywhere else that you know of?

paultsimura commented 1 month ago

@paultsimura do we have a loading pattern for buttons anywhere else that you know of?

E.g. when calculating the distance:

https://github.com/user-attachments/assets/91911ba2-18f1-4e3e-b963-b6515a8706d1

danieldoglas commented 1 month ago

But that page is not accessible if we're offline, right?

paultsimura commented 1 month ago

It is accessible, it just allows passing along without the calculation. The distance will be calculated on the server in this case.

https://github.com/user-attachments/assets/0097a525-c447-406b-9396-47fc47f21664

danieldoglas commented 1 month ago

I see, thanks!

@strepanier03 do you think we need to discuss this internally if we add the loading button?

trjExpensify commented 1 month ago

Wouldn't that result in an infinite spinner offline in this case? I feel like adding approverMode is the better UX option, personally.

danieldoglas commented 1 month ago

@marcaaron we were discussing sending the policy list to be processed in Auth yesterday - is that something you're going to do soon? If yes, do you wanna assign yourself from this issue? We'll need to include a new property (approverMode) when returning policy list on OpenApp.

marcaaron commented 1 month ago

is that something you're going to do soon?

Ah not really, it was just a suggestion - something we'd probably do whenever it's more of a priority to work on.

I'd be open to looking into it as part of this ticket you'd rather pass it off. Checking the current implementation and it should be pretty straightforward to migrate this.

However, I don't think we have an approverMode property. Did we mean approvalMode?

Create another workspace and submit an expense in that workspace chat. Go to Settings > Troubleshoot > Clear Cache, then quickly return to the workspace chat from step 5.

These test steps are very sus to me. What are we trying to achieve exactly? Is there a real bug that affects users? If so, how does it happen organically?

We don't necessarily have to move the whole policySummaryList to Auth to complete this ticket seems enough to add the approvalMode here and here. And I guess also keeping in mind that the more stuff we add to the policy summary the larger it gets. But this is a very simple value so it's probably OK.

paultsimura commented 1 month ago

That should be approvalMode, correct.

As for the steps - I guess we can replicate this by having a non-default-policy expense as the only pinned chat, which will be opened right after login, so we'll see the button change while the data is loading.

danieldoglas commented 1 month ago

@marcaaron I agree with you about just adding the new property. My question was more like - if we're moving this to Auth, we can do it all together. If we're not planning on doing that right now, I can add them in auth/php. Thanks for looking into it!

melvin-bot[bot] commented 1 month ago

@danieldoglas, @strepanier03, @paultsimura Whoops! This issue is 2 days overdue. Let's get this updated quick!

danieldoglas commented 1 month ago

Removing help wanted and external since it's an internal change.

moving to weekly since it's not urgent.

trjExpensify commented 1 month ago

Moved this into my list of internal bugs in core flows (i.e approving, submitting, paying etc) as we approach wrapping up #wave-collect.

melvin-bot[bot] commented 1 month ago

@danieldoglas @strepanier03 @paultsimura this issue was created 2 weeks ago. Are we close to a solution? Let's make sure we're treating this as a top priority. Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

danieldoglas commented 1 month ago

moving to daily so I can address it this week

paultsimura commented 1 month ago

Not overdue, waiting for @danieldoglas

danieldoglas commented 1 month ago

Both PRs were created and waiting for merge

danieldoglas commented 1 month ago

Everything was deployed. Adding retest label

mvtglobally commented 3 weeks ago

Issue not reproducible during KI retests. (First week)

mvtglobally commented 1 week ago

Issue not reproducible during KI retests. (Second week)

paultsimura commented 1 week ago

@strepanier03 @danieldoglas can we close based on the retest results?

danieldoglas commented 1 week ago

Yep, let's close it!