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
2.99k stars 2.5k forks source link

[$250] Approve - "Waiting for you to pay" message appears briefly for approver after approving report #41081

Open izarutskaya opened 3 weeks ago

izarutskaya commented 3 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: 1.4.66-2 Reproducible in staging?: Y Reproducible in production?: Y Found when validating PR: https://github.com/Expensify/App/pull/40532 Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

Precondition:

  1. Go to staging.new.expensify.com
  2. [Admin] Go to workspace chat.
  3. [Admin] Create an expense and submit the report.
  4. [Approver] Go to transaction thread.
  5. [Approver] Click Approve button.

Expected Result:

Approver will not be presented with this Next step message "Waiting for you to pay these expenses".

Actual Result:

"Waiting for you to pay these expenses" message appears briefly for approver after approving the report.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/Expensify/App/assets/115492554/d2aad5cf-cfb5-4538-8c2c-9dd65c6080e1

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013e61cf7fb38074ac
  • Upwork Job ID: 1785010427319132160
  • Last Price Increase: 2024-05-14
  • Automatic offers:
    • eh2077 | Reviewer | 0
    • GandalfGwaihir | Contributor | 0
melvin-bot[bot] commented 3 weeks ago

Triggered auto assignment to @muttmuure (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 3 weeks ago

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

GandalfGwaihir commented 3 weeks ago

Proposal

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

Wrong optimistic message is seen for the approver who is not the owner of the policy

What is the root cause of that problem?

Currently in NextStepsUtils, we do not cover the case where the approver is not the owner of the policy, this causes the current regression, as first we will show the optimistic message which will be set to waiting for you to pay...: https://github.com/Expensify/App/blob/3f379ec24cb5b7774fbc58003f3d015b77938442/src/libs/NextStepUtils.ts#L302-L308

here the managerDisplayName would be set to you as : https://github.com/Expensify/App/blob/3f379ec24cb5b7774fbc58003f3d015b77938442/src/libs/NextStepUtils.ts#L91

isSelfApproval will be true as thecurrent account id would be equal to theapprover/submitter account id: https://github.com/Expensify/App/blob/3f379ec24cb5b7774fbc58003f3d015b77938442/src/libs/NextStepUtils.ts#L89

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

We need to add new code which will check if the current user is the approver of the policy but isn't the owner of the policy, and then we will set the optimistic message for this case which will match with the BE returned value:

case CONST.REPORT.STATUS_NUM.APPROVED:
            if (isManager && !isOwner) {
                optimisticNextStep = {
                    type,
                    title: 'Finished!',
                    message: [
                        {
                            text: 'No further action required!',
                        },
                    ],
                };
            } else {
            ..... (The current existing code)

Note: instead of isManager to be more careful we can check submitToAccountID === currentUserAccountID, but that can be discussed during PR stage

Results

https://github.com/Expensify/App/assets/110545952/035541d9-ef49-4c8e-a420-96d2dcc12ad7

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

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

melvin-bot[bot] commented 2 weeks ago

@muttmuure Whoops! This issue is 2 days overdue. Let's get this updated quick!

eh2077 commented 2 weeks ago

Commented in the related PR to confirm if this is a legit regression

GandalfGwaihir commented 2 weeks ago

I guess we're good to proceed with the current issue based on this comment https://github.com/Expensify/App/pull/40532#issuecomment-2085453255 @eh2077

eh2077 commented 2 weeks ago

reviewing proposals

eh2077 commented 2 weeks ago

When owner and approver are the same person, the behaviour is same as described in this issue (owner and approver are different) after clicking the Approve button - Show Waiting for you to pay these expenses. and then No further action required!

https://github.com/Expensify/App/assets/117511920/0987915b-3a16-4ef2-bb4c-4373aa1872b8

@muttmuure Can you help to clarity the expected behaviours here?

Note: currently both cases show Waiting for you to pay these expenses. briefly and then show No further action required!

muttmuure commented 2 weeks ago

Will chat with Tom

muttmuure commented 2 weeks ago

The owner isn't a factor here, the owner is the person who owns billing

muttmuure commented 2 weeks ago

https://expensify.slack.com/archives/C036QM0SLJK/p1714555601945619

eh2077 commented 2 weeks ago

@muttmuure So in both cases the next step message will be No further action required! right?

muttmuure commented 2 weeks ago

Make or track payments is enabled, so there should be an action to pay, it just depends who it is - I will revert back soon

eh2077 commented 2 weeks ago

Make or track payments is enabled, so there should be an action to pay, it just depends who it is - I will revert back soon

@muttmuure Sorry, the expected results are still not clear to me. Looking forward to your update, thanks.

eh2077 commented 1 week ago

Waiting for update from @muttmuure

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? πŸ’Έ

muttmuure commented 1 week ago

If Make and track payments is enabled but no bank account is connected, the message Waiting for you to pay these expenses should be showing for all admins.

So the unexpected behavior here is that No further action required is showing at all. It should be Waiting for you to pay these expenses

eh2077 commented 1 week ago

@muttmuure Thank you for making that clear.

@GandalfGwaihir Can you update your proposal based on @muttmuure 's comment ^?

GandalfGwaihir commented 1 week ago

Ah missed this, will do by EOD πŸ‘

eh2077 commented 1 week ago

@GandalfGwaihir Friendly bump!

melvin-bot[bot] commented 1 week ago

@muttmuure @eh2077 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!

GandalfGwaihir commented 1 week ago

So the unexpected behavior here is that No further action required is showing at all. It should be Waiting for you to pay these expenses

I think you understood the issue wrong here @muttmuure , it indeed shows Waiting for you to pay these expenses for all admins.

The bug here is for approver (The person who is an approver but not the admin/owner of the workspace). In this case we optimistically set the message to Waiting for you to pay these expenses and then the BE returns no actions needed (which is right as the approver doesn't have the authority to pay the expense as he is neither the admin nor owner).

So I guess my proposal still is valid for the bug described in the OP.

What are your thoughts @muttmuure @eh2077 , sorry for the delay in reply

eh2077 commented 5 days ago

The bug here is for approver (The person who is an approver but not the admin/owner of the workspace). In this case we optimistically set the message to Waiting for you to pay these expenses and then the BE returns no actions needed (which is right as the approver doesn't have the authority to pay the expense as he is neither the admin nor owner).

This makes sense to me if we only want to fix this specific use case.

But I think it'll be better to fix https://github.com/Expensify/App/issues/41081#issuecomment-2098033539 together as they're relevant.

@GandalfGwaihir What do you think?

cc @muttmuure

melvin-bot[bot] commented 4 days ago

πŸ“£ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? πŸ’Έ

GandalfGwaihir commented 3 days ago

@GandalfGwaihir What do you think?

I am Okay with that, but for this to happen we also need a BE fix right, cause the message No further action required is set fro the BE, we can make the FE changes to set the message optimistically, but BE should also be async, but I think the discussion we had is sufficient for us to proceed with PR

eh2077 commented 3 days ago

@GandalfGwaihir Thanks for the update!

I think we can go with @GandalfGwaihir 's proposal to move this issue forward.

Apart from fixing the specific case as described by this issue, I think we can also fix similar cases together. Yes, these cases also need backend fix.

πŸŽ€πŸ‘€πŸŽ€ C+ reviewed

melvin-bot[bot] commented 3 days ago

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

melvin-bot[bot] commented 3 days ago

πŸ“£ @eh2077 πŸŽ‰ An offer has been automatically sent to your Upwork account for the Reviewer role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job

melvin-bot[bot] commented 3 days ago

πŸ“£ @GandalfGwaihir πŸŽ‰ An offer has been automatically sent to your Upwork account for the Contributor role πŸŽ‰ Thanks for contributing to the Expensify app!

Offer link Upwork job Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review πŸ§‘β€πŸ’» Keep in mind: Code of Conduct | Contributing πŸ“–

GandalfGwaihir commented 2 days ago

@eh2077 I will open the PR over the weekend/ as soon as the BE issues are resolved, sorry for delay, due to the current site reliability issues with ND it's really difficult for me to test the changes, I am getting error every time I submit an expense or even try to approve it 😒 , will post when the PR is up