Skip to content

[CLEAN] Synthetic Benchmark PR #76153 - Revert "Merge pull request #73676 from mkzie2/mkzie2-issue/70286"#19

Open
tomerqodo wants to merge 1 commit into
base_pr_76153_20251204_3815from
clean_pr_76153_20251204_3815
Open

[CLEAN] Synthetic Benchmark PR #76153 - Revert "Merge pull request #73676 from mkzie2/mkzie2-issue/70286"#19
tomerqodo wants to merge 1 commit into
base_pr_76153_20251204_3815from
clean_pr_76153_20251204_3815

Conversation

@tomerqodo

@tomerqodo tomerqodo commented Dec 4, 2025 •
edited by qodo-code-review Bot
Loading

Copy link
Copy Markdown

User description

Benchmark PR Expensify#76153

Type: Clean (correct implementation)

Original PR Title: Revert "Merge pull request Expensify#73676 from mkzie2/mkzie2-issue/70286"
Original PR Description: ### Explanation of Change

Reverts Expensify#73676

Fixed Issues

$ Expensify#76086

Tests

  1. Create a control workspace
  2. Invite a member
  3. Enable workflows
  4. Set the member as the approver
  5. Remove the member
  6. Verify the default approval flow is still visible and the admin is set as the approver

Offline tests

None

QA Steps

Same as Tests

PR Author Checklist

  • I linked the correct issue in the ### Fixed Issues section above
  • I wrote clear testing steps that cover the changes made in this PR
    • I added steps for local testing in the Tests section
    • I added steps for the expected offline behavior in the Offline steps section
    • I added steps for Staging and/or Production testing in the QA steps section
    • I added steps to cover failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
    • I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
    • I tested this PR with a High Traffic account against the staging or production API to ensure there are no regressions (e.g. long loading states that impact usability).
  • I included screenshots or videos for tests on all platforms
  • I ran the tests on all platforms & verified they passed on:
    • Android: Native
    • Android: mWeb Chrome
    • iOS: Native
    • iOS: mWeb Safari
    • MacOS: Chrome / Safari
  • I verified there are no console errors (if there's a console error not related to the PR, report it or open an issue for it to be fixed)
  • I verified there are no new alerts related to the canBeMissing param for useOnyx
  • I followed proper code patterns (see Reviewing the code)
    • I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick)
    • I verified that comments were added to code that is not self explanatory
    • I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
    • I verified any copy / text shown in the product is localized by adding it to src/languages/* files and using the translation method
      • If any non-english text was added/modified, I used JaimeGPT to get English > Spanish translation. I then posted it in #expensify-open-source and it was approved by an internal Expensify engineer. Link to Slack message:
    • I verified all numbers, amounts, dates and phone numbers shown in the product are using the localization methods
    • I verified any copy / text that was added to the app is grammatically correct in English. It adheres to proper capitalization guidelines (note: only the first word of header/labels should be capitalized), and is either coming verbatim from figma or has been approved by marketing (in order to get marketing approval, ask the Bug Zero team member to add the Waiting for copy label to the issue)
    • I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
    • I verified the JSDocs style guidelines (in STYLE.md) were followed
  • If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
  • I followed the guidelines as stated in the Review Guidelines
  • I tested other components that can be impacted by my changes (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar are working as expected)
  • I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
  • I verified any variables that can be defined as constants (ie. in CONST.ts or at the top of the file that uses the constant) are defined as such
  • I verified that if a function's arguments changed that all usages have also been updated correctly
  • If any new file was added I verified that:
    • The file has a description of what it does and/or why is needed at the top of the file if the code is not self explanatory
  • If a new CSS style is added I verified that:
    • A similar style doesn't already exist
    • The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))
  • If new assets were added or existing ones were modified, I verified that:
    • The assets are optimized and compressed (for SVG files, run npm run compress-svg)
    • The assets load correctly across all supported platforms.
  • If the PR modifies code that runs when editing or sending messages, I tested and verified there is no unexpected behavior for all supported markdown - URLs, single line code, code blocks, quotes, headings, bold, strikethrough, and italic.
  • If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
  • If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
  • If the PR modifies a component or page that can be accessed by a direct deeplink, I verified that the code functions as expected when the deeplink is used - from a logged in and logged out account.
  • If the PR modifies the UI (e.g. new buttons, new UI components, changing the padding/spacing/sizing, moving components, etc) or modifies the form input styles:
    • I verified that all the inputs inside a form are aligned with each other.
    • I added Design label and/or tagged @Expensify/design so the design team can review the changes.
  • If a new page is added, I verified it's using the ScrollView component to make it scrollable when more elements are added to the page.
  • I added unit tests for any new feature or bug fix in this PR to help automatically prevent regressions in this user flow.
  • If the main branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to the Test steps.

Screenshots/Videos

Screen.Recording.2025-11-26.at.11.04.50.AM.mov

Original PR URL: Expensify#76153

PR Type

Bug fix

Description

  • Revert approval workflow changes that incorrectly handled pending actions during member removal

  • Simplify pendingAction logic by removing conditional preservation of DELETE status

  • Move workflow update logic from Member.ts to UI layer for better separation of concerns

  • Remove unused helper functions and consolidate workflow management code

Diagram Walkthrough

flowchart LR
  A["Member Removal Logic"] -->|Simplified| B["Remove Conditional pendingAction"]
  A -->|Moved to UI| C["WorkspaceMembersPage & MemberDetailsPage"]
  D["Workflow Functions"] -->|Consolidated| E["Remove getUpdateApprovalWorkflowOnyxData"]
  D -->|Consolidated| F["Remove getRemoveApprovalWorkflowOnyxData"]
  G["WorkflowUtils"] -->|Simplified| H["Remove pendingAction Preservation Logic"]
Loading

File Walkthrough
Relevant files
Bug fix
WorkflowUtils.ts
Simplify pending action assignment logic                                 

src/libs/WorkflowUtils.ts

  • Removed conditional logic that preserved DELETE pending actions
  • Simplified pendingAction assignment to always use the provided value
  • Applied changes across four locations where approvers/members are
    updated
 +4/-8     
ExpenseReportRulesSection.tsx
Simplify prevent self approvals lock logic                             

src/pages/workspace/rules/ExpenseReportRulesSection.tsx

  • Removed calculation of membersCount and shouldLockPreventSelfApprovals
  • Simplified lock logic to only check workflowApprovalsUnavailable
  • Removed member count-based disabling of prevent self approvals toggle
 +4/-6     
Refactoring
Member.ts
Remove workflow handling from member removal                         

src/libs/actions/Policy/Member.ts

  • Removed workflow update logic from removeMembers function
  • Removed imports for workflow-related functions and types
  • Simplified function signature by removing approvalWorkflows and
    allPersonalDetails parameters
  • Removed logic that checked for approvers and updated workflows on
    member removal
 +29/-93 
Policy.ts
Consolidate prevent self approval logic                                   

src/libs/actions/Policy/Policy.ts

  • Consolidated getSetPolicyPreventSelfApprovalOnyxData logic into
    setPolicyPreventSelfApproval
  • Removed exported getSetPolicyPreventSelfApprovalOnyxData function
  • Simplified function to directly call API instead of returning Onyx
    data
 +7/-22   
Workflow.ts
Consolidate workflow update and remove functions                 

src/libs/actions/Workflow.ts

  • Consolidated getUpdateApprovalWorkflowOnyxData logic into
    updateApprovalWorkflow
  • Consolidated getRemoveApprovalWorkflowOnyxData logic into
    removeApprovalWorkflow
  • Removed exported helper functions that returned Onyx data
  • Functions now directly call API instead of returning data structures
 +6/-49   
Tests
PolicyMemberTest.ts
Update tests for simplified member removal                             

tests/actions/PolicyMemberTest.ts

  • Updated test calls to removeMembers with simplified parameters
  • Removed allPersonalDetails and empty approvalWorkflows array from
    function calls
  • Removed unused PersonalDetailsList type import
 +6/-14   
Enhancement
WorkspaceMembersPage.tsx
Move workflow handling to UI layer                                             

src/pages/workspace/WorkspaceMembersPage.tsx

  • Added workflow update logic to removeUsers function before calling
    removeMembers
  • Checks if removed members are approvers and updates workflows
    accordingly
  • Calls removeApprovalWorkflowAction or updateApprovalWorkflow based on
    workflow state
  • Simplified removeMembers call by removing workflow-related parameters
 +32/-3   
WorkspaceMemberDetailsPage.tsx
Add workflow and self-approval handling to member details

src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx

  • Added workflow update logic to removeUser callback before member
    removal
  • Checks if removed member is an approver and updates workflows
    accordingly
  • Added logic to disable prevent self approval when only one employee
    remains
  • Calls setPolicyPreventSelfApproval to handle self-approval setting
 +43/-4   

@qodo-code-review

qodo-code-review Bot commented Dec 4, 2025

Copy link
Copy Markdown

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
⚪
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

⚪
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit logs: Newly added logic updates approval workflows and removes members but adds no explicit
audit logging of these critical changes (who removed whom, which workflows changed).

Referred Code 
    Navigation.navigate(ROUTES.WORKSPACE_INVITE.getRoute(route.params.policyID, Navigation.getActiveRouteWithoutParams()));
}, [route.params.policyID, isAccountLocked, showLockedAccountModal]);

/**
 * Remove selected users from the workspace
 * Please see https://github.com/Expensify/App/blob/main/README.md#Security for more details
 */
const removeUsers = () => {
    // Check if any of the members are approvers
    const hasApprovers = selectedEmployees.some((email) => isApprover(policy, email));

    if (hasApprovers) {
        const ownerEmail = ownerDetails.login;
        for (const login of selectedEmployees) {
            const accountID = policyMemberEmailsToAccountIDs[login];
            const removedApprover = personalDetails?.[accountID];
            if (!removedApprover?.login || !ownerEmail) {
                continue;
            }
            const updatedWorkflows = updateWorkflowDataOnApproverRemoval({
                approvalWorkflows,


 ... (clipped 21 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status:
No error paths: The new removal flow updates workflows and policy settings but does not handle or surface
API or update failures in the added code paths, relying on underlying APIs without local
fallback or user-facing error handling.

Referred Code 
const removeUser = useCallback(() => {
    const ownerEmail = ownerDetails?.login;
    const removedApprover = personalDetails?.[accountID];

    // If the user is not an approver, proceed with member removal
    if (!isApproverUserAction(policy, memberLogin) || !removedApprover?.login || !ownerEmail) {
        removeMemberAndCloseModal();
        return;
    }

    // Update approval workflows after approver removal
    const updatedWorkflows = updateWorkflowDataOnApproverRemoval({
        approvalWorkflows,
        removedApprover,
        ownerDetails,
    });

    for (const workflow of updatedWorkflows) {
        if (workflow?.removeApprovalWorkflow) {
            const {removeApprovalWorkflow, ...updatedWorkflow} = workflow;



 ... (clipped 9 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status:
Missing validation: The added logic derives emails and account IDs from state and proceeds with
updates/removals without explicit validation or authorization checks in these new call
sites, assuming upstream correctness.

Referred Code 
const hasApprovers = selectedEmployees.some((email) => isApprover(policy, email));

if (hasApprovers) {
    const ownerEmail = ownerDetails.login;
    for (const login of selectedEmployees) {
        const accountID = policyMemberEmailsToAccountIDs[login];
        const removedApprover = personalDetails?.[accountID];
        if (!removedApprover?.login || !ownerEmail) {
            continue;
        }
        const updatedWorkflows = updateWorkflowDataOnApproverRemoval({
            approvalWorkflows,
            removedApprover,
            ownerDetails,
        });
        for (const workflow of updatedWorkflows) {
            if (workflow?.removeApprovalWorkflow) {
                const {removeApprovalWorkflow, ...updatedWorkflow} = workflow;
                removeApprovalWorkflowAction(updatedWorkflow, policy);
            } else {
                updateApprovalWorkflow(workflow, [], [], policy);


 ... (clipped 4 lines)

Learn more about managing compliance generic rules or creating your own custom rules
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review

qodo-code-review Bot commented Dec 4, 2025

Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Consolidate workflow update logic

The logic for updating approval workflows upon member removal is duplicated
across WorkspaceMembersPage.tsx and WorkspaceMemberDetailsPage.tsx. This should
be consolidated into a single new action to centralize logic and improve
maintainability.

Examples:

src/pages/workspace/WorkspaceMembersPage.tsx [207-234] 
    const removeUsers = () => {
        // Check if any of the members are approvers
        const hasApprovers = selectedEmployees.some((email) => isApprover(policy, email));

        if (hasApprovers) {
            const ownerEmail = ownerDetails.login;
            for (const login of selectedEmployees) {
                const accountID = policyMemberEmailsToAccountIDs[login];
                const removedApprover = personalDetails?.[accountID];
                if (!removedApprover?.login || !ownerEmail) {

 ... (clipped 18 lines)
src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx [250-279] 
    const removeUser = useCallback(() => {
        const ownerEmail = ownerDetails?.login;
        const removedApprover = personalDetails?.[accountID];

        // If the user is not an approver, proceed with member removal
        if (!isApproverUserAction(policy, memberLogin) || !removedApprover?.login || !ownerEmail) {
            removeMemberAndCloseModal();
            return;
        }


 ... (clipped 20 lines)

Solution Walkthrough:

Before:

// In WorkspaceMembersPage.tsx
function removeUsers() {
  // ... check if removed members are approvers
  if (hasApprovers) {
    // ... loop through removed members
    // ... call updateWorkflowDataOnApproverRemoval
    // ... loop through updated workflows
    // ... call removeApprovalWorkflowAction or updateApprovalWorkflow
  }
  removeMembers(...);
}

// In WorkspaceMemberDetailsPage.tsx
function removeUser() {
  // ... check if removed member is an approver
  if (isApprover) {
    // ... call updateWorkflowDataOnApproverRemoval
    // ... loop through updated workflows
    // ... call removeApprovalWorkflowAction or updateApprovalWorkflow
  }
  removeMemberAndCloseModal(); // which calls removeMembers()
}

After:

// In a new action in src/libs/actions/Policy/Member.ts
function removeMembersAndUpdateWorkflows(policyID, membersToRemove, ...) {
  // ... check if removed members are approvers
  if (hasApprovers) {
    // ... loop through removed members
    // ... call updateWorkflowDataOnApproverRemoval
    // ... loop through updated workflows
    // ... call removeApprovalWorkflowAction or updateApprovalWorkflow
  }

  // Handle "Prevent Self Approval" logic
  if (remainingEmployeeCount === 1 && policy.preventSelfApproval) {
    setPolicyPreventSelfApproval(policyID, false);
  }

  removeMembers(policyID, ...);
}

// In WorkspaceMembersPage.tsx and WorkspaceMemberDetailsPage.tsx
// Call the new consolidated action
removeMembersAndUpdateWorkflows(policyID, selectedEmployees, ...);
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies significant code duplication of business logic in two UI components, which is a major maintainability concern, and proposes a sound architectural improvement by centralizing it into a single action.

High
Possible issue
Correctly count active workspace members

Correct the calculation of previousEmployeesCount by filtering out employees
with a pendingAction of 'delete' to ensure an accurate count of active members.

src/pages/workspace/members/WorkspaceMemberDetailsPage.tsx [238-248]

 // Function to remove a member and close the modal
 const removeMemberAndCloseModal = useCallback(() => {
     removeMembers(policyID, [memberLogin], {[memberLogin]: accountID});
-    const previousEmployeesCount = Object.keys(policy?.employeeList ?? {}).length;
+    const previousEmployeesCount = Object.values(policy?.employeeList ?? {}).filter(
+        (employee) => employee.pendingAction !== CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,
+    ).length;
     const remainingEmployeeCount = previousEmployeesCount - 1;
     if (remainingEmployeeCount === 1 && policy?.preventSelfApproval) {
         // We can't let the "Prevent Self Approvals" enabled if there's only one workspace user
         setPolicyPreventSelfApproval(policyID, false);
     }
     setIsRemoveMemberConfirmModalVisible(false);
 }, [accountID, memberLogin, policy?.employeeList, policy?.preventSelfApproval, policyID]);
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a bug where the count of remaining employees includes those pending deletion, which could lead to incorrect logic for disabling the 'Prevent Self Approval' setting.

Medium
  • More

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Review effort 3/5

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@tomerqodo