[HOLD for payment 2025-01-02] [$500] [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-12-30
• Automatic offers:
• nkdengineer | Contributor | 104876723

Issue Owner

Current Issue Owner: @puneetlath

[melvin-bot]

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

[melvin-bot]

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

[melvin-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 #51001 recently

[nkdengineer]

@Beamanator I'd like to work on this.

[melvin-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

[ntdiary]

BugZero Checklist:

• [Contributor] Classify the bug:

Bug classification

Source of bug:

• 1a. Result of the original design (eg. a case wasn't considered)
• 1b. Mistake during implementation
• 1c. Backend bug
• 1z. Other: new feature

Where bug was reported:

• 2a. Reported on production (eg. bug slipped through the normal regression and PR testing process on staging)
• 2b. Reported on staging (eg. found during regression or PR testing)
• 2d. Reported on a PR
• 2z. Other: new feature

Who reported the bug:

• 3a. Expensify user
• 3b. Expensify employee
• 3c. Contributor
• 3d. QA
• 3z. Other:

• [Contributor] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake.

  Link to comment: N/A (new feature)

• [Contributor] If the regression was CRITICAL (e.g. interrupts a core flow) A discussion in #expensify-open-source has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner.

  Link to discussion: N/A

• [Contributor] If it was decided to create a regression test for the bug, please propose the regression test steps using the template below to ensure the same bug will not reach production again.
  No need, as we will try to add some automated unit tests later.

Regression Test Proposal Template

• [BugZero Assignee] Create a GH issue for creating/updating the regression test once above steps have been agreed upon.

  Link to issue:

Regression Test Proposal

Precondition:

Test:

Do we agree 👍 or 👎

[ntdiary]

@ntdiary can you please complete the checklist?

Honestly, I still have a small question here, based on the comment above, we could actually create multiple test cases. I’m still hesitating whether to try adding some automated unit tests, or just create a few regression tests. Personally, I’m leaning a bit toward the former—perhaps preparing some test data, then calling methods like SubmitReport/approveMoneyRequest, and gradually verifying the expected approvers (this is just a preliminary idea, not sure if it might get too complicated). Do you have different thoughts on this? :)

[puneetlath]

@Beamanator what do you think?

[Beamanator]

I def like the idea of automated unit tests 👍👍👍👍👍

Though I'm not quite sure how the current process works for that - should I make a new issue & assign one of you to write all the tests? I don't think we would need both a contributor & a C+ for that right? 🤷

But I definitely think it's a good idea to write automated tests for these changes since they're pretty complex and super easy to break

[nkdengineer]

@Beamanator I think it's a good idea.

1. We should create a unit test for getSubmitToAccountID to cover the correct submitsTo.
2. We should create a unit test for getApprovalChain to cover the correct list order approval chain.

We can talk about all possible scenarios related to this issue. I can help to work on this.

[Beamanator]

Sounds like a great idea to me! 👍

I thinkkk I'll create a new issue for this tomorrow, and I'll assign you @nkdengineer 🚀

[melvin-bot]

Issue is ready for payment but no BZ is assigned. @laurenreidexpensify you are the lucky winner! Please verify the payment summary looks correct and complete the checklist. Thanks!

[Beamanator]

Ummmm pretty sure @puneetlath is BZ here? 😅

[laurenreidexpensify]

@puneetlath isn't on the team anymore :)

[Beamanator]

ohhhhhh

[laurenreidexpensify]

That said, I'm locked out of my Upwork account right now so @puneetlath can you take this one to the finish line 🙏 thanks!

[ntdiary]

FYI, have completed the checklist. :)

[puneetlath]

Payment summary:

• C - $500 - @nkdengineer - paid via Upwork
• C+ - $500 - @ntdiary - to be paid via NewDot

Thanks everyone!

@Beamanator I'll leave the issue open for you to close once you've created the follow-up issue.

[melvin-bot]

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

[JmillsExpensify]

$500 approved for @ntdiary

[melvin-bot]

@puneetlath, @Beamanator, @ntdiary, @nkdengineer Eep! 4 days overdue now. Issues have feelings too...

[nkdengineer]

@Beamanator Is the new issue ready?

[Beamanator]

Created follow-up: #55003

@nkdengineer can you please comment there so i can assign you? Closing this one now 👍