search

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.5k stars 2.85k forks source link

[Simple AA in NewDot] Make sure optimistic Next Steps & report action are set for Control mid-level "Approvals" in NewDot #43567

Closed Beamanator closed 1 month ago

Beamanator commented 4 months ago

Tracking issue: https://github.com/Expensify/Expensify/issues/393839

Design doc section: Make sure optimistic Next Steps & report action are set correctly, for “Approve” (old term “Forward”) in NewDot

Goals:

  1. Set optimistic next steps for approvals, when policy is Control and on Advanced Approval mode
    • Specifically new for mid-level approvers
  2. Need to build basic approval chain from policy employeeList in order to be able to figure out who should optimistically be the next approval
  3. Also needs to optimistically add Approved <total> report action to workspace chat

Background:

We currently generate optimistic Next Steps when approving a report in NewDot here. The current possible next steps are:

  1. “Finished! No further action required”
  2. “Next steps: Waiting for you to pay the expenses”

Problem:

There are now more possible cases when a report is getting approved!

When a Control policy has "Advanced Approvals" enabled, there can be multiple "mid-level" approvers. When these members approve the policy, we actually call this "forwarding" - because we "forward" the report to the next person in line. The final approver actually does the "approving".

Solution:

  1. Build the report's approval chain any time someone approves a report on a Control workspace with advanced approvals enabled
    • Note: For v1 we will only consider submitsTo, forwardsTo, and overLimitForwardsTo. Later, we'll add in rule approvers (tag & category)
  2. Optimistically add a report action saying approved <total> even when forwarding the report

Approval Chain Logic:

When building out the approval chain logic, logic should look like this:

  1. Get policy's employeeList
  2. Get report submitter, get their policy data with employeeList[submitterEmail]
  3. The first member of the approval chain is the employee's submitsTo - like submitsToEmail = employeeList[submitterEmail]?.submitsTo ?? policyOwnerEmail
  4. Next, get the submitsTo person's policy data with submitsToEmployeeData = employeeList[submitsToEmail]
  5. From here on, we get the next approver by looking at forwardsTo, overLimitForwardsTo, and approvalLimit
    1. If submitsToEmployeeData has an approvalLimit and overLimitForwardsTo, do this:
      1. Get the report total
      2. If the report total is > approvalLimit, then the next approver in line is the email address in submitsToEmployeeData?.overLimitForwardsTo
      3. If the report is <= approvalLimit, then the next approver in line is submitsToEmployeeData?.forwardsTo
    2. If submitsToEmployeeData doesn't have either approvalLimit or doesn't have overLimitForwardsTo, the next approver in line is submitsToEmployeeData?.forwardsTo
    3. Note: If we ever run into a missing approver here (a.k.a. if submitsToEmployeeData doesn't have a forwardsTo), we're done!
  6. Now we get the forwardsTo or overLimitForwardsTo's policy data with forwardsEmployeeData = employeeList[forwardsToEmail]
  7. Repeat steps 5 & 6 until you either:
    1. End up with an email address you already added to the approval chain
    2. Run into some employee's data that doesn't have a forwardsTo

This looks something like this:

Screenshot 2024-07-01 at 3 35 41 PM

Where $policy->getForwardsTo looks like this:

Screenshot 2024-07-01 at 3 36 52 PM

Examples:

Say, for example, we have this Control employeeList policy setup (with advanced approvals enabled):

Member Email policy role submitsTo forwardsTo overLimitForwardsTo approvalLimit
boss_owner@gmail.com admin mini_owner@gmail.com
mini_owner@gmail.com admin boss_owner@gmail.com
manager@gmail.com employee mini_owner@gmail.com mini_owner@gmail.com boss_owner@gmail.com 10000
employee1@gmail.com employee manager@gmail.com
employee2@gmail.com employee manager@gmail.com

Each member's approval chain looks like this:

Policy Member Approval Chain (if report total <= approvalLimit) Approval Chain (if report total > approvalLimit)
boss_owner@gmail.com mini_owner@gmail.com same
mini_owner@gmail.com boss_owner@gmail.com same
manager@gmail.com mini_owner@gmail.com same
employee1@gmail.com manager@gmail.com, mini_owner@gmail.com manager@gmail.com, boss_owner@gmail.com
employee2@gmail.com manager@gmail.com, mini_owner@gmail.com manager@gmail.com, boss_owner@gmail.com
Beamanator commented 4 months ago

Keeping internal for now, will make this external soon once all details are added here

Might want to hold on https://github.com/Expensify/Expensify/issues/404203 b/c that's where we'll start sharing the workspace chat w/ new people added to approval chains

melvin-bot[bot] commented 4 months ago

@Beamanator Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 4 months ago

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

melvin-bot[bot] commented 4 months ago

@Beamanator 8 days overdue is a lot. Should this be a Weekly issue? If so, feel free to change it!

Beamanator commented 4 months ago

Gotta get the held issue done, then will open this up!

melvin-bot[bot] commented 4 months ago

@Beamanator, @youssef-lr Whoops! This issue is 2 days overdue. Let's get this updated quick!

garrettmknight commented 4 months ago

I think we're off hold! cc @Beamanator yeah?

Beamanator commented 3 months ago

Hmm yes this is off hold, @youssef-lr I don't think we need you on this one since it can be worked externally (see my messages above) - I'm planning to finish writing up the necessary details & assigning this one out today or tomorrow!

Note: there's still a few things I'm working on to make workspace chat sharing / unsharing perfect (here) but this issue shouldn't hold on those changes! You just mayyy have weird experiences testing if you:

  1. Invite a new member to the workspace in OldDot -> This approval chain isn't sent everywhere (NewDot) immediately, so you can just modify the new member's approval chain a few times in OldDot & everything should get updated
  2. If you created expense reports in the workspace and THEN modified the approval chain, new invitees will not be able to access previously created expense reports :D Again, I'm planning to fix that in this issue
Beamanator commented 3 months ago

Asked @rushatgabhane for a review of the requirements to make sure it makes sense 🙏

melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @trjExpensify (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details. Please add this Feature request to a GH project, as outlined in the SO.

melvin-bot[bot] commented 3 months ago

@Beamanator, @trjExpensify, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

rushatgabhane commented 3 months ago

Okay so I was able to build the approval chain and it tests well for employee, manager, and both owners -

image
rushatgabhane commented 3 months ago

@Beamanator it is okay to always make the reportTotal a positive number, right?

rushatgabhane commented 3 months ago

The expense submitters owed money, so the report total was -X

rushatgabhane commented 3 months ago

@Beamanator I finished building the approval chain and optimistic steps in the PR, but I'm facing a backend issue.

When a manger approves the report, im getting a 400 error for ApproveMoneyRequest.

image

https://github.com/Expensify/App/assets/29673073/ff581cb8-93f8-4e05-9387-ec29767c6323

Beamanator commented 3 months ago

@rushatgabhane it looks like that is thrown when our backend thinks you're trying to "Forward" the report, but you SHOULD be trying to "Approve"

This is actually expected for right now, since @marcochavezf is working on adding changes so that mid-level approvers can always "Approve" OR "Forward" a report from NewDot. So you probably can't actually test out the full "Submitter -> submitsTo -> forwardsTo" approval flow YET (with the backend working) but you can test that it optimistically works probably?

rushatgabhane commented 3 months ago

@Beamanator yeah optimistic is no problem :)

It works offline

Beamanator commented 3 months ago

Groovy - but ya probably sadly we can't get your PR merged till the backend stuff is done. sorry for that, thanks for getting this done quick!

rushatgabhane commented 3 months ago

working on adding changes so that mid-level approvers can always "Approve" OR "Forward" a report from NewDot

makes sensee, thank you for digging into it : )

Groovy - but ya probably sadly we can't get your PR merged till the backend stuff is done. sorry for that, thanks for getting this done quick!

No worries

trjExpensify commented 3 months ago

Can we state what issue/PR this is on hold for in the title, so that's clear?

trjExpensify commented 3 months ago

^^ bump on the above! :)

Beamanator commented 3 months ago

Sorry for the delay! We're basically holding on https://github.com/Expensify/Expensify/issues/404206, where @marcochavezf will be enable clicking Approve (no matter if it's approving or forwarding) in NewDot

melvin-bot[bot] commented 3 months ago

@Beamanator, @trjExpensify, @rushatgabhane Huh... This is 4 days overdue. Who can take care of this?

trjExpensify commented 3 months ago

Still held.

Beamanator commented 3 months ago

Still held, but getting close!

melvin-bot[bot] commented 3 months ago

@Beamanator, @trjExpensify, @rushatgabhane Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

JmillsExpensify commented 3 months ago

Can we remove the hold based on Marco's most recent merged PRs?

trjExpensify commented 2 months ago

https://expensify.slack.com/archives/C06ML6X0W9L/p1722354497413689?thread_ts=1722319732.404769&cid=C06ML6X0W9L - according to @Beamanator not quite, but Marco's Web PR in the linked issue is on staging, so I think we're pretty much there.

Beamanator commented 2 months ago

Ooh yes even the App PR is on staging! I think it's time to take this off hold!

Beamanator commented 2 months ago

@rushatgabhane can you pull main & start testing on staging? 🙏 there may be a few bugs which @marcochavezf and I can work out, but as long as this optimistic stuff can be tested now, that would be amazing 🙏

Beamanator commented 2 months ago

Boom this is completely off hold as the ApproveMoneyRequest stuff should be on prod!

Beamanator commented 2 months ago

PR was most likely deployed to prod yesterday

Beamanator commented 2 months ago

I thinkkkk payment is due here 🚀

trjExpensify commented 2 months ago

Okay, so this is the PR: https://github.com/Expensify/App/pull/44940

Beamanator commented 2 months ago

What happened here with the review, it was merged before @mananjadhav's C+ review, but completed after, hence no checkmark? 🤔

Yep exactly 👍

Was this considered a regression?

Hmm seems like ifffff it was, we didn't consider it a big deal since it hasn't been worked on yet, so i would say we don't need to consider that a payment slashing regression, what you think?

trjExpensify commented 1 month ago

Alright, so payment summary as follows:

Closing it out!

garrettmknight commented 1 month ago

$250 approved for @rushatgabhane

garrettmknight commented 1 month ago

$250 approved for @mananjadhav