[$250] [Simple AA in NewDot] Update the approval chain / next receiver logic to handle correct optimistic next approver in Advanced Approval cases

[Beamanator]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: Current Reproducible in staging?: Y Reproducible in production?: Y Expensify/Expensify Issue URL: https://github.com/Expensify/Expensify/issues/430318

[1] Action Performed:

1. Create a workspace in NewDot
2. Enable delay submission
3. Invite submitter, Approver A, Approver B
4. Set to Advanced Approval in OldDot via workspace owner (workspace should be type Control)
5. Set category approvals for Category A going to Approver A + Category B as Approver B
6. As submitter in NewDot, submit 2 expenses to the WS chat, one with Category A, one with Category B
7. Submit the report
8. Confirm first approver is Approver A
9. As Approver A, approve the report

[1] Expected Result:

10. Confirm the second approver is Approver B (both offline & online)
11. As Approver B, approve the report
12. Confirm the third approver is workspace owner (both offline & online)
13. As WS owner, approve and confirm that the report is final approved.

[2] Action Performed:

1. Create a workspace in NewDot
2. Enable delay submission
3. Invite submitter, Approver A, Approver B, Approver C
4. Set to Advanced Approval in OldDot via workspace owner (workspace should be type Control)
5. Set up approver workflow like this:
   • Submitter submitsTo Approver A
   • Approver A forwardsTo Approver B
   • Approver C forwardsTo workspace owner
6. As submitter, create an expense in the workspace chat & submit it -> this should get submitted to Approver A
7. As workspace owner, update approver workflow so that Submitter has submitsTo of Approver C
8. As Approver A, approve the report (in NewDot)

[2] Expected Result:

9. Verify that Approver C is the next approver (both offline & online)
10. As Approver C, approve the report
11. Verify the next approver is the workspace owner (both offline & online)
12. As the workspace owner, approve and confirm the report is final approved

Implementation details

ReportUtils.getApprovalChain

This function should ONLY return the submitter's approval chain based on the current policy.employeeList data.

We need to start including category & tag approvers in this function. Category approvers should come first, then tag approvers, then approvers in employeeList.

Here's what we have in C++ which should give a good starting point:

    vector<string> approvalChain;
    string nextApprover = Policy::getSubmitsToEmail(db, policyID, submitterEmail, employeeList);

    // If there's no submitsTo, just use the policy owner so we have someone to submit to
    if (nextApprover.empty()) {
        const string& policyOwner = Account::getEmail(db, Policy::getOwner(db, policyID));
        approvalChain.push_back(policyOwner);
    }

    // We don't want to add someone who's already in the approval chain, or it'd become an infinite loop
    const int64_t reportTotal = abs(getTransactionTotal(db, reportID, getCurrency(db, reportID)));
    while (!nextApprover.empty() && find(approvalChain.begin(), approvalChain.end(), nextApprover) == approvalChain.end()) {
        approvalChain.push_back(nextApprover);

        // Get the forwardsTo or overLimitForwardsTo (if over the approval limit)
        nextApprover = Policy::getForwardsTo(employeeList, policyID, nextApprover, reportTotal);
    }

    // The full approval chain with rule approvers before submitsTo/forwardsTo approvers and no repeats
    // We use an unordered_set to ensure unique elements while inserting and preserving the order of the original vectors
    vector<string> fullApprovalChain;
    unordered_set<string> uniqueApprovers;
    for (const auto& approver : ruleApprovers) {
        // Don't force the submitter to approve as a rule approver, because it could get confusing for the user
        if (ruleApprovers.contains(submitterEmail)) {
            SINFO("Submitter is also rule approver, skipping adding to approval chain");
            continue;
        }

        // If insertion was successful (i.e., not already in the set), then add to the full approval chain
        if (uniqueApprovers.insert(approver).second) {
            fullApprovalChain.push_back(approver);
        }
    }
    for (const auto& approver : approvalChain) {
        if (uniqueApprovers.insert(approver).second) {
            fullApprovalChain.push_back(approver);
        }
    }

    // If the submitter of the report is also the last person that needs to approve and the policy doesn't allow self-approval, then skip that person.
    if (!fullApprovalChain.empty() && fullApprovalChain.back() == submitterEmail && Policy::isPreventSelfApprovalEnabled(db, policyID)) {
        fullApprovalChain.pop_back();
    }

    return fullApprovalChain;

IOU.getNextApproverAccountID

This function should figure out who, in the policy's current approvalChain (built by buildApproverChain above) is next in line to approve the report.

This function will also check IF the report has been previously approved by someone in the approval chain - if so, they will be skipped.

Here's what we have in C++ which should give a good starting point:

string Report::getNextReceiver(SQLite& db, const int64_t reportID, const string& currentApprover, const vector<string>& approvalChain)
{
    if (!verifyState(db, reportID, STATE_OPEN) && !verifyState(db, reportID, STATE_SUBMITTED)) {
        return "";
    }

    const set<int64_t> previousReportApprovers = getAccountsThatHaveApprovedReport(db, reportID);

    // Loop through approval chain to find the next approver who has not yet approved the report
    const string& managerOnVacation = getValue(db, reportID, NVP_MANAGER_ON_VACATION);
    for (auto it = approvalChain.begin(); it != approvalChain.end(); ++it) {
        const string& approver = *it;
        string approverOrDelegate = approver;

        // Handle vacation delegate
        if (approver == managerOnVacation) {
            const string& vacationDelegate = Account::getVacationDelegate(db, managerOnVacation);
            approverOrDelegate = !vacationDelegate.empty() ? vacationDelegate : approver;
        }

        // Return early if the report is open
        if (verifyState(db, reportID, STATE_OPEN)) {
            return approverOrDelegate;
        }

        // Return the first approver in the approval chain that hasn't approved yet
        const bool reportHasBeenApprovedByApproverOrDelegate = previousReportApprovers.contains(Account::getID(db, approverOrDelegate));
        if (currentApprover != approverOrDelegate && !reportHasBeenApprovedByApproverOrDelegate) {
            return approverOrDelegate;
        }
    }

    return "";
}

set<int64_t> Report::getAccountsThatHaveApprovedReport(SQLite& db, const int64_t reportID)
{
    const set<string> RESET_APPROVAL_ACTIONS = {
        ACTION_REJECTED_TO_SUBMITTER,
        ACTION_RETRACTED,
        ReportAction::ACTION_SUBMITTED,
        ReportAction::ACTION_DELEGATE_SUBMIT,
        ACTION_REOPENED,
        ReportAction::ACTION_UNAPPROVED,
    };

    const set<string> APPROVAL_ACTIONS = {
        ReportAction::ACTION_APPROVED,
        ACTION_FORWARDED,
        ACTION_CLOSED,
    };

    set<string> allActionsOfInterest;
    allActionsOfInterest.insert(RESET_APPROVAL_ACTIONS.begin(), RESET_APPROVAL_ACTIONS.end());
    allActionsOfInterest.insert(APPROVAL_ACTIONS.begin(), APPROVAL_ACTIONS.end());
    allActionsOfInterest.insert(ACTION_REJECTED);

    SQResult result;
    DB::read(db, "SELECT action, accountID, created FROM reportActions WHERE reportID = " + SQ(reportID) + " AND action IN (" + SQList(allActionsOfInterest) + ") ORDER BY created DESC;", result);

    set<int64_t> recentApprovers;
    map<int64_t, int64_t> approversWithApprovalCount;
    for (const auto& row : result.rows) {
        const int64_t accountID = SToInt64(row["accountID"]);
        if (!SContains(approversWithApprovalCount, accountID)) {
            // Note: We don't care about approval order because we only care about the order of the "next approvers"
            // in the approval chain to figure out the next receiver, not the order in which previous approvers approved the report
            recentApprovers.insert(accountID);

            // Initialize this approver in the map
            approversWithApprovalCount[accountID] = 0;
        }
        if (APPROVAL_ACTIONS.contains(row["action"])) {
            approversWithApprovalCount[accountID] += 1;
        } else if (row["action"] == ACTION_REJECTED) {
            approversWithApprovalCount[accountID] -= 1;
        } else {
            // We reached an action that resets the "approval" state of a report, ignore rest
            break;
        }
    }

    // Take out all the approvers who had a net 0 amount of approvals (ex: they approved but then unapproved)
    for (const auto& [accountID, count] : approversWithApprovalCount) {
        if (count <= 0) {
            recentApprovers.erase(accountID);
        }
    }
    return recentApprovers;
}

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021856641526591668663
• Upwork Job ID: 1856641526591668663
• Last Price Increase: 2024-11-13
• Automatic offers:
• nkdengineer | Contributor | 104876723

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[Beamanator]

cc @nkdengineer in case you're interested to work on this, since you worked on https://github.com/Expensify/App/issues/51001 recently

[nkdengineer]

@Beamanator I'd like to work on this.

[melvin-bot[bot]]

📣 @nkdengineer 🎉 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 📖

[Beamanator]

Thank you @nkdengineer 👍

Please feel free to ask questions here before starting your PR IF you have any, either way I will try to do a very detailed review 🙏

[nkdengineer]

Sure.

[nkdengineer]

Update:

• I'm looking into this with the provided logic, I'll finish and raise the PR tomorrow or Saturday.

[nkdengineer]

@Beamanator I created a draft here. Please help test it with your local backend. Now I can only test with two rule approvers because it's marked as approved after the first rule approver approves the expense report.

[Beamanator]

Some problems I've seen so far in my testing

• Note: I have the following setup:
  1. Control policy, on Submit & Approve, default submitter is policy owner
  2. 2 tag approvers & 2 category approver set up
  3. Submitter creates expenses in this order:
     1. Tag approver 1
     2. Category approver 1
     3. Category approver 2
• When Submitter first submits, I see that we submit to Category approver 2, though in it should be Category approver 1 as the first expense w/ a category approver, no? It shouldn't be "the latest category approver", it should be the first by created date.

Can you please try that and see if you reproduce the issue?

As for the approval order when there's multiple category approvers, i think i see the problem in the back end that i'll start working on a fix for

[nkdengineer]

It shouldn't be "the latest category approver", it should be the first by created date.

@Beamanator I tested this case and Category approver 1 is the first next approver

[Beamanator]

Ok I tested this a few times, and somehow my first test has the same bug I reported before, BUT I'm not 1000% sure if there was some bad setup... When I continued testing, everything worked out perfectly 😅 - multiple times

@nkdengineer my backend PR fix is up for review, hopefully can get merged today but probably won't get deployed till tomorrow at the earliest

[Beamanator]

FYI backend PR was merged earlier today, hasn't been deployed yet

[Beamanator]

@nkdengineer backend PR was deployed 🙏 please continue with this one & let me know if you hit any more bugs

[nkdengineer]

Sure I will retest soon.

[nkdengineer]

@Beamanator I tested with the staging server and we still don't archive the expected:

Precondition:

• A WS with a submitter, approver 1, approver 2, approver 3, approver4
• Enable rules and add a category's approver for approver 1 (category 1), another one for approver 2 (category 2)
• Add a tag's approver for approver 3 (tag 1), another one for approver 4 (tag 2)

1. Create 4 expenses with category 1, category 2, tag 1, tag 2
2. Submit the report
3. After the submit the first approver is approver 1 (Correct)
4. As approver 1 approves the report
5. Offline, the next approver is the approver 2 (Correct)
6. Online, the next step is waiting admin to pay the expense (Incorrect) (The expected should be the next approver is approver 2).

[Beamanator]

wtf... i see what you're seeing 🙃 @nkdengineer what approval mode have you been testing on? Me, Submit & Approve

https://github.com/user-attachments/assets/b4dfefb7-9fa5-4c5a-baf6-b95f44963f9d

[nkdengineer]

@Beamanator Advance approve mode.

[ntdiary]

Hi, @Beamanator, @nkdengineer, if the PR requires C+ review this week, please feel free to reassign it, as my bandwidth is limited this week. :)

[Beamanator]

Sounds good - you should be good @ntdiary as long as you'll be free early next week!

I am working on fixing another internal bug 😬 https://github.com/Expensify/Expensify/issues/446644

[Beamanator]

@nkdengineer FYI the bug I found is actually in Submit & Approve flows... Because we DO actually need to test that this all works in both Submit & Approve (S&A) and Advanced Approval (AA) flows 😬

So once again, hopefully my fix will get merged today, not sureeeee if it'll get deployed by the weekend or not, but if not we'll have to pick this back up Monday 🙃

In the meantime, I would appreciate if you can let me know your exact setup for the Advanced Approval flow where you found a bug - cuz i don't thinkkk my fix will cover that case, only Submit & Approve

[melvin-bot[bot]]

@puneetlath, @Beamanator, @ntdiary, @nkdengineer Whoops! This issue is 2 days overdue. Let's get this updated quick!

[nkdengineer]

@Beamanator Ah checked my setting approval mode and in my reported case it's BASIC.

[Beamanator]

Okkkk that makes much more sense - thanks @nkdengineer 🙏

can you please test again on Advance Approval, and if it's working we can try to get the PR ready for that case? Hopefully today / tomorrow I'll get the fix in for BASIC approvals

[nkdengineer]

Will re-test today.

[nkdengineer]

@Beamanator Tested and it works perfectly. Will complete the video and open the PR tomorrow.

[Beamanator]

Amazing, that's great to hear 👍 👍

[Beamanator]

FYI backend PR merged now, may not get deployed till tomorrow - we'll see

[Beamanator]

FYI backend PR merged now, may not get deployed till tomorrow - we'll see