[Payment due @thelullabyy] Allow setting and displaying approval workflows with the new structure - #94825
[Payment due @thelullabyy] Allow setting and displaying approval workflows with the new structure#94825Gonals wants to merge 55 commits into
Conversation
Codecov Report✅ Changes either increased or maintained existing code coverage, great job!
|
|
@Gonals Changes lgtm, please let me know when the backend PR is available on staging so that I can test and do final round of review |
You should be good to test |
|
@Gonals Could you please check if we need enable any BE flag in order to test? It doesn't save the data for me in case I apply the workflow with limit Screen.Recording.2026-08-05.at.06.21.46.mov |
|
BUG 1:
Expected: We should be able to see created workflows Screen.Recording.2026-08-05.at.06.45.47.mov |
|
@Gonals sometimes, I cannot delete workflows Screen.Recording.2026-08-05.at.07.12.16.mov |
|
@Gonals I think below is the patch to fix this bug RCA: diff --git a/src/libs/actions/Workflow.ts b/src/libs/actions/Workflow.ts
index e2996eb040f..ceb87911cee 100644
--- a/src/libs/actions/Workflow.ts
+++ b/src/libs/actions/Workflow.ts
@@ -415,16 +415,21 @@ function updateApprovalWorkflowRules({approvalWorkflow, initialApprovalWorkflow,
}
/** Delete an approval workflow using the rules-based backend structure. */
-function removeApprovalWorkflowRules(approvalWorkflow: ApprovalWorkflow, policy: OnyxEntry<Policy>, rules: OnyxCollection<Rule>) {
+function removeApprovalWorkflowRules(approvalWorkflow: ApprovalWorkflow, policy: OnyxEntry<Policy>, rules: OnyxCollection<Rule>): boolean {
if (!policy) {
- return;
+ return false;
}
const existingRules = getApprovalWorkflowRulesForPolicy(rules, policy.id);
const memberEmails = getWorkflowMemberEmails(approvalWorkflow.members);
const rulesDiff = reconcileApprovalWorkflowRulesForRemove(memberEmails, {existingRules});
+ if (isEmptyObject(rulesDiff)) {
+ return false;
+ }
+
setApprovalWorkflowRules({policyID: policy.id, rulesDiff, previousRules: existingRules});
+ return true;
}
/** Set the members of the approval workflow that is currently edited */diff --git a/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx b/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx
index 97c564cf8a4..3ca83ba9893 100644
--- a/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx
+++ b/src/pages/workspace/workflows/approvals/WorkspaceWorkflowsApprovalsEditPage.tsx
@@ -111,12 +111,10 @@ function WorkspaceWorkflowsApprovalsEditPage({policy, isLoadingReportData = true
const useRulesBackend = isBetaEnabled(CONST.BETAS.MULTIPLE_APPROVERS);
Navigation.dismissModal({
afterTransition: () => {
- // Remove the approval workflow using the initial data as it could be already edited
- if (useRulesBackend) {
- removeApprovalWorkflowRules(initialApprovalWorkflow, policy, rulesCollection);
- } else {
- removeApprovalWorkflow(initialApprovalWorkflow, policy);
+ if (useRulesBackend && removeApprovalWorkflowRules(initialApprovalWorkflow, policy, rulesCollection)) {
+ return;
}
+ removeApprovalWorkflow(initialApprovalWorkflow, policy);
},
});
}; |
|
This issue was false alarm, it works fine now after I got the beta flag enabled in BE Screen.Recording.2026-08-05.at.16.04.50.mov |
|
We can ignore this issue as it is very edge case which normal user won't reach |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b7f1a9262
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@Gonals Could you please check the AI bot comments? |
Done |
|
@Gonals I found another issue is that when I try to update an existing Approvals by adding additional (second) approver, the optimistic data is showing correctly, but after getting response from BE, it the newly added approver is removed Screen.Recording.2026-08-05.at.22.41.21.mov |
I haven't been able to reproduce this one, but found a related bug and fixed it. Can you give this one a try again? Maybe that fixed it too. |
melvin-bot
Bot
changed the title
|
🎯 @thelullabyy, thanks for reviewing and testing this PR! 🎉 A payment issue will be created for your review once this PR is deployed to production. If payment is not needed (e.g., regression PR review fix etc), react with 👎 to this comment to prevent the payment issue from being created. |
|
@mountiny, @luacmartins, @tgolen, @JS00001, all yours! Let's push this! |
| ? { | ||
| onyxMethod: Onyx.METHOD.SET, | ||
| key: ruleKey, | ||
| value: {...previousRule, scope: CONST.RULES.SCOPE.POLICY, scopeID: policyID, pendingAction: null, errors: genericError}, |
There was a problem hiding this comment.
Why do we need to add scope/scopeID here? Shouldn't the previous rule already have that?
| // A submitter can only belong to one workflow, so first drop these members from any OTHER | ||
| // workflow's rules, then add them to the new workflow. | ||
| const removeDiff = reconcileApprovalWorkflowRulesForRemove(memberEmails, {existingRules}); | ||
| const rulesAfterRemoval = applyApprovalWorkflowRulesDiff(existingRules, removeDiff); |
There was a problem hiding this comment.
Do we show any warning to the user that we're doing this? I'd think that the admin wouldn't be able to select a user that is currently in another workflow to begin with.
| (account.accountData?.state === CONST.BANK_ACCOUNT.STATE.OPEN || (shouldIncludePartiallySetup && isBankAccountPartiallySetup(account.accountData?.state))) && | ||
| account.accountData?.type === CONST.BANK_ACCOUNT.TYPE.BUSINESS && | ||
| account.methodID !== excludeBankAccountID | ||
| (excludeBankAccountID === undefined || account.methodID !== excludeBankAccountID) |
There was a problem hiding this comment.
Why do we need this change?
| return [...workflowMembers, ...additionalMembers]; | ||
| } | ||
|
|
||
| type ApprovalWorkflowRulesDiff = Record<string, ApprovalWorkflowRule | null>; |
There was a problem hiding this comment.
NAB but this file seems over-engineered. We only have fixed shapes for these rules as of now, so it seems like we could simplify this quite a bit.
| POLICY_HAS_CONNECTIONS_DATA_BEEN_FETCHED: 'policyHasConnectionsDataBeenFetched_', | ||
| POLICY_CONNECTION_SYNC_PROGRESS: 'policyConnectionSyncProgress_', | ||
| POLICY_MERGE_HR_INITIAL_SYNC_MODAL_SHOWN: 'policyMergeHRInitialSyncModalShown_', | ||
| // AST rules keyed by ruleID |
There was a problem hiding this comment.
| // AST rules keyed by ruleID |
| "../../tests/unit/VideoRendererTest.tsx" "@typescript-eslint/no-unsafe-type-assertion" 1 | ||
| "../../tests/unit/WhisperContentMentionContextTest.tsx" "@typescript-eslint/no-unsafe-type-assertion" 3 | ||
| "../../tests/unit/WorkflowUtilsTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 7 | ||
| "../../tests/unit/WorkflowUtilsTest.ts" "@typescript-eslint/no-unsafe-type-assertion" 8 |
There was a problem hiding this comment.
should we be adding to the unsafe assertion count?
| function buildComparison( | ||
| operator: ValueOf<typeof CONST.SEARCH.SYNTAX_OPERATORS>, | ||
| left: ApprovalWorkflowFilterComparison['left'], | ||
| right: ApprovalWorkflowFilterComparison['right'], | ||
| ): ApprovalWorkflowFilterComparison { | ||
| return {operator, left, right}; | ||
| } |
There was a problem hiding this comment.
NAB can we make this take params as left, operator, right so it reads like an equation?
| // Different approvers may share an `overLimitForwardsTo`, which would emit identical terminal rules. | ||
| const seen = new Set<string>(); | ||
| return rules.filter((rule) => { | ||
| const fingerprint = JSON.stringify(rule); |
There was a problem hiding this comment.
do we need to sort this? E.g {a: '', b: ''} wont equal {b: '', a: ''}
| } | ||
|
|
||
| /** Extract the union of email values across every `from` leaf in a rule. */ | ||
| function extractSubmitterEmails(rule: ApprovalWorkflowRule): string[] { |
There was a problem hiding this comment.
I think it'd be helpful to use common AST methods for this (e.g getFilter(rule.filters) rather than parsing the ast in a one off method)
| function mergeEmails(existingEmails: string[], emailsToAdd: string[]): string[] { | ||
| const seen = new Set(existingEmails); | ||
| const result = [...existingEmails]; | ||
| for (const email of emailsToAdd) { | ||
| if (seen.has(email)) { | ||
| continue; | ||
| } | ||
| seen.add(email); | ||
| result.push(email); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
| function mergeEmails(existingEmails: string[], emailsToAdd: string[]): string[] { | |
| const seen = new Set(existingEmails); | |
| const result = [...existingEmails]; | |
| for (const email of emailsToAdd) { | |
| if (seen.has(email)) { | |
| continue; | |
| } | |
| seen.add(email); | |
| result.push(email); | |
| } | |
| return result; | |
| } | |
| function mergeEmails(existingEmails: string[], emailsToAdd: string[]): string[] { | |
| const seen = new Set(existingEmails); | |
| return [ | |
| ...existingEmails, | |
| ...emailsToAdd.filter(email => !seen.has(email) && !!seen.add(email)), | |
| ]; | |
| } |
| } | ||
|
|
||
| /** True when any of `emails` is in `set` — i.e. a rule's submitters overlap a workflow's members. */ | ||
| function containsAnyEmail(emails: string[], set: Set<string>): boolean { |
There was a problem hiding this comment.
This seems like it'd be a bit overengineered, will we reuse this method? Its one line either way, right?
containsAnyEmails(emails, set)
vs
emails.some(email => set.has(email))
| Navigation.dismissModal({ | ||
| afterTransition: () => { | ||
| // Remove the approval workflow using the initial data as it could be already edited | ||
| if (useRulesBackend && removeApprovalWorkflowRules(initialApprovalWorkflow, policy, rulesCollection)) { |
There was a problem hiding this comment.
This confused me, are we calling the API in the if statement?
| */ | ||
| type ApprovalWorkflowFilterComparison = { | ||
| /** The comparison operator. */ | ||
| operator: ValueOf<typeof CONST.SEARCH.SYNTAX_OPERATORS>; |
There was a problem hiding this comment.
I wonder if we should standardize the operators to no longer live in 'Search'. Maybe thats something we can handle separately
4 participants
Explanation of Change
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/651010
PROPOSAL:
Tests
This is where all the testing happens. Get ready because it is fun!
select * from rules where scopeID='<POLICYID>';. Confirm the rules are in the right shapeOffline tests
Same as tests
QA Steps
Same as tests, but no DB checks
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)Avatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari