[$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]

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

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

[nkdengineer]

@ntdiary The PR is ready for review. cc @Beamanator

[ntdiary]

I reviewed the code first, and then encountered a strange issue during testing:

I only created two expenses, but ReportPreview duplicated, I can reproduce it on staging as well, so it might not be related to this issue. BTW, I couldn't find any ticket for this problem. (Update: it might be front-end related, as I logged in again only shows one ReportPreview)
Additionally, I'm in UTC+8 timezone, plan to complete tests and fill out the check list tomorrow, but if it's urgent, feel free to reassign it. :)

[ntdiary]

@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 😬

@Beamanator, does this mean that category/tag rules should also work in the S&A flow? If so, I think the scenario of automatic delayed submission might not be correct, the correct approver here should be no.3, right?

My test data:
no.1 is approver for cat_1
no.2 is approver for cat_2
no.3 is approver for tag_3
no.4 is approver for tag_4
no.25 is policy owner

basic-tag-test.mp4

[Beamanator]

does this mean that category/tag rules should also work in the S&A flow?

Yes, only if the policy is a Control policy, NOT on Collect 👍

And ok hmm, looking at your video, if it was automatically submitted yesterday, it may not have picked up the most recent backend changes I made... Can you test again today? Also if this is a new bug in "automatic delayed submission / harvesting" then I will create a new issue for that to fix

[ntdiary]

@Beamanator, The approval has proceeded as expected. BTW, did we push the NextStep onyx data to previous node? I noticed the that data hasn't been updated. (the account in the screenshot is no.1) 🤔

[Beamanator]

Ooohhh goood question - I see that we should return updated Next Steps for the approver, but we may not live update that for all clients when we forward / approve 🤔 Thanks for reporting that! I will make sure to look into that

[Beamanator]

Ok I just discussed internally & we found the expected category vs tag approver sort order for NewDot here, and here's what we came up with:

Category vs Tag approver order (for now):

1. Category approvers come before tag approvers
2. If there's multiple category approvers for the report (for example, but same logic for tag approvers)
   1. First choose approvers from expenses with earliest (farthest in the past) transaction created date (the selected date on the expense)
   2. If there's multiple expenses on that day w/ different category approvers, sort by earliest transaction inserted date
3. If there's multiple tag approvers for any 1 single expense...
   1. Sort approvers alphabetically

[nkdengineer]

@Beamanator Currently, the transaction data in the frontend only have the created (the selected date on the expense), we don't have inserted date. It makes it a bit hard to sort the transaction list if multiple transactions have the same created. Because in this case, we need to compare the iou created action.

[Beamanator]

Ooh good shout @nkdengineer - I will fix that in a PR today which should get merged tomorrow

[Beamanator]

PR was merged, should get deployed later today 🙏

[nkdengineer]

Good, I will complete the order after the PR is deployed.

[Beamanator]

I was OOO yesterday :D but the PR was deployed 👍

[nkdengineer]

Updated the PR.

[ntdiary]

The PR disappeared due to some strange reason. 🤔
a related slack conv

[nkdengineer]

I'm back.

[Beamanator]

welcome!! 🤣