[Due for payment 2025-03-31] [$250] Workspace - Incorrect members number when import spreadsheet

[IuliiaHerets]

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: v9.0.99-0
Reproducible in staging?: Yes
Reproducible in production?: Yes
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/5599527&group_by=cases:section_id&group_id=229065&group_order=asc
Email or phone of affected tester (no customers): [email protected]
Issue reported by: Applause Internal Team
Device used: Lenovo 80ES / Windows 10 Pro
App Component: Workspace Settings

Action Performed:

1. Create workspace
2. Invite 2 members
3. Download CSV from 3-dot menu
4. Delete 2 members
5. Import spreadsheet from step 3 and finish the flow

Expected Result:

Import successful modal should display the correct number of imported members

Actual Result:

Import successful modal does not display the correct number of imported members
When import 2 members - Import successful modal displays 3 members

Workaround:

Unknown

Platforms:

• Android: Standalone
• Android: HybridApp
• Android: mWeb Chrome
• iOS: Standalone
• iOS: HybridApp
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Bug6745694_1739797104103.Recording__5074.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021891920662582766128
• Upwork Job ID: 1891920662582766128
• Last Price Increase: 2025-02-18

Issue Owner

Current Issue Owner: @OfstadC

[melvin-bot]

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

[FitseTLT]

🚨 Edited by proposal-police: This proposal was edited at 2025-02-17 13:55:30 UTC.

Proposal

Please re-state the problem that we are trying to solve in this issue.

Workspace - Incorrect members number when import spreadsheet

What is the root cause of that problem?

When downloaded csv the owner was added and then we are re-importing all members/rows from the csv and notifying with that number without checking if some of the members are already member of the policy

App/src/libs/actions/Policy/Member.ts

Lines 760 to 767 in 84885ab

	const onyxData = updateImportSpreadsheetData(members.length);
	const parameters = {
	policyID,
	employees: JSON.stringify(members.map((member) => ({email: member.email, role: member.role}))),
	};
	API.write(WRITE_COMMANDS.IMPORT_MEMBERS_SPREADSHEET, parameters, onyxData);

What changes do you think we should make in order to solve the problem?

We should exclude emails that are already members of a policy by filtering out members emails that are already member of the policy here

App/src/pages/workspace/members/ImportedMembersPage.tsx

Line 77 in 84885ab

const members = membersEmails?.slice(containsHeader ? 1 : 0).map((email, index) => {

            .filter((email) => !(policy?.employeeList && Object.keys(policy.employeeList).includes(email)))

BTW there are some exceptions or edge cases to this case because even if there is a member with that email the data in the csv might have different fields like role sumbitsTo so we need to decide and accordingly either leave the current status of the user or apply the data from csv. But currently the BE updates fields from the csv for members that already exist so we might need to filter out rows from csv only if all the data matches the current existing status of the workspace member.

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

assert importPolicyMembers excludes already existing members of a workspace

What alternative solutions did you explore? (Optional)

[bernhardoj]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Incorrect numbers of members added when importing workspace members.

What is the root cause of that problem?

When we download the CSV of the workspace members, it includes all of the members. When we import it, we get the length of the member rows and use it to show how many members are added.

App/src/libs/actions/Policy/Member.ts

Lines 759 to 760 in b22da34

	function importPolicyMembers(policyID: string, members: PolicyMember[]) {
	const onyxData = updateImportSpreadsheetData(members.length);

App/src/libs/actions/Policy/Member.ts

Line 210 in b22da34

prompt: translateLocal('spreadsheet.importMembersSuccessfullDescription', {members: membersLength}),

It's not accurate because there can be an existing member already.

What changes do you think we should make in order to solve the problem?

We first need to filter out the imported member if the member already exists on the workspace members.

const onyxData = updateImportSpreadsheetData(members.filter((member) => !getPolicy(policyID)?.employeeList?.[member.email]).length);

However, if the filtered result is 0, the modal will still show "1 member has been added".

App/src/languages/en.ts

Line 823 in b22da34

importMembersSuccessfullDescription: ({members}: ImportMembersSuccessfullDescriptionParams) => (members > 1 ? `${members} members have been added.` : '1 member has been added.'),

Also, it's possible that we import an existing member, but with an updated role (we don't need to care for other properties for now because they are all ignored).

To make the message more useful, I think we should add a case where there is no member added and there is one or more members updated.

For example, when there is no member added/updated, we can say: "No member has been added/updated"
When there are members added and updated: "1 member has been added and 2 members have been updated"
When there is only member updated: "2 members have been updated"

To get how many member has been updated and added, we can use this logic.

const policy = getPolicy(policyID);
const {added, updated} = members.reduce((acc, curr) => {
    const employee = policy?.employeeList?.[curr.email]
    if (employee) {
        if (curr.role !== employee.role) {
            return {...acc, updated: acc.updated + 1};    
        }
    } else {
        return {...acc, added: acc.added + 1};
    }
    return acc;
}, {added: 0, updated: 0});

What specific scenarios should we cover in automated tests to prevent reintroducing this issue in the future?

We can test importPolicyMembers and assert the onyx data of the modal prompt.

[melvin-bot]

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

[melvin-bot]

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

[alitoshmatov]

Thank you @FitseTLT for proposal, RCA is correct. While your solution is correct I really think @bernhardoj 's proposal is more fit for our situation since it provides a better way to handle updated members while correctly ignoring the same members.

[alitoshmatov]

I think we can go with @bernhardoj 's proposal which suggests to better show how many imported members were added and how many were actually updated.

C+ reviewed 🎀 👀 🎀

P. S: The expected result does not provide clear explanation on how to handle existing members with different roles from spreadsheet. If we just want to show how many members were added not regarding if they were added or updated then we can go with first proposal. But still selected proposal makes more sense to me

[melvin-bot]

Triggered auto assignment to @jasperhuangg, see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[melvin-bot]

@OfstadC, @jasperhuangg, @alitoshmatov Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[bernhardoj]

Can someone from the team check/create the copy first? We don't have this yet on the App and this is the one that I propose in the proposal.

when there is no member added/updated, we can say: "No member has been added or updated"
When there are members added and updated: "1 member has been added and 2 members have been updated"
When there is only member updated: "2 members have been updated"

cc: @jasperhuangg @jjcoffee

[melvin-bot]

Triggered auto assignment to @LLPeckham (Waiting for copy), see https://stackoverflow.com/c/expensify/questions/7025/ for more details.

[LLPeckham]

Just looking at the pop up as a whole:

• When there have been no updates:

Import successful
No members have been added or updated
[Got it]

• When members are added and updated:

Import successful
X member(s) added, X member(s) updated
[Got it]

• When only a member is updated:

Import successful
X member(s) updated
[Got it]

Thoughts cc @jasperhuangg @bernhardoj @jjcoffee

[bernhardoj]

That looks good. Thanks!

[bernhardoj]

@LLPeckham Oh wait, the existing copy for member added is:

X member(s) have/has been added.

But the copy for member updated above is

X member(s) updated

Do you think we should add have/has been to the updated copy, too?

X member(s) have/has been updated

[LLPeckham]

Ah good catch - sure let's do the following:

• When there have been no updates:

Import successful
No members have been added or updated
[Got it]

• When members are added and updated:

Import successful
X member(s) added, X member(s) updated
[Got it]

• When only a member is updated:

Import successful
X member(s) has/have been updated
[Got it]

• What a member is added:
  Import successful
  X member(s) has/have been added
  [Got it]

[jasperhuangg]

Thanks @LLPeckham, @bernhardoj let me know if this new copy makes sense, let's get this done!

[bernhardoj]

Thanks! Preparing the PR...

[bernhardoj]

PR is ready

cc: @alitoshmatov

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 9.1.17-1 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• Improve import member success message #57726

If no regressions arise, payment will be issued on 2025-03-31. 🎊

For reference, here are some details about the assignees on this issue:

• @bernhardoj requires payment through NewDot Manual Requests
• @alitoshmatov requires payment through NewDot Manual Requests

[melvin-bot]

@alitoshmatov @OfstadC @alitoshmatov The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed. Please copy/paste the BugZero Checklist from here into a new comment on this GH and complete it. If you have the K2 extension, you can simply click: [this button]

[OfstadC]

@alitoshmatov Please complete BZ checklist prior to the payment date. Thanks! 😃

[alitoshmatov]

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:

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:

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: https://github.com/Expensify/App/pull/48876/files#r2020201439

• [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: No discussion

• [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.

• [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:

• Example files:
  member.add.csv
  member.update.and.add.csv
  member.update.csv

Test:

1. Open the workspace members page.
2. Import a spreadsheet with new members.
3. Verify success message: "X member(s) have been added."
4. Re-import the same spreadsheet.
5. Verify success message: "No members have been added or updated."
6. Import a spreadsheet with the same members but different roles.
7. Verify success message: "X member(s) have been updated."
8. Import a spreadsheet with updated roles for existing members and a new member.
9. Verify success message: "X member(s) added, Y member(s) updated."

Do we agree 👍 or 👎

[alitoshmatov]

Completed checklist and requested payment in ND

[bernhardoj]

Requested in ND.

[melvin-bot]

Payment Summary

Upwork Job

• Reviewer: @bernhardoj owed $250 via NewDot
• Reviewer: @alitoshmatov owed $250 via NewDot

BugZero Checklist (@OfstadC)

• I have verified the correct assignees and roles are listed above and updated the necessary manual offers
• I have verified that there are no duplicate or incorrect contracts on Upwork for this job (https://www.upwork.com/ab/applicants/1891920662582766128/hired)
• I have paid out the Upwork contracts or cancelled the ones that are incorrect
• I have verified the payment summary above is correct

[JmillsExpensify]

$250 approved for @alitoshmatov

[JmillsExpensify]

$250 approved for @bernhardoj