Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HOLD for payment 2024-05-20] [$250] Workspace - Invite error is not dismissed via X button in the workspace list #40800

Closed
6 tasks done
lanitochka17 opened this issue Apr 23, 2024 · 37 comments
Assignees
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor

Comments

@lanitochka17
Copy link

lanitochka17 commented Apr 23, 2024

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Version Number: 1.4.64-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: N/A
Issue reported by: Applause - Internal Team

Action Performed:

  1. Go to Account settings > Workspaces > any workspace
  2. Go to Members
  3. Invite +252 3 234211 which will cause an error
  4. Return to workspace list
  5. Click on the X button to dismiss the error

Expected Result:

The error is dimissed

Actual Result:

The error is not dismissed when clicking on X button in the workspace list

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

  • Android: Native
  • Android: mWeb Chrome
  • iOS: Native
  • iOS: mWeb Safari
  • MacOS: Chrome / Safari
  • MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6458809_1713877820690.20240423_210639.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0186e1563827cf6717
  • Upwork Job ID: 1782782840343851008
  • Last Price Increase: 2024-04-23
  • Automatic offers:
    • akinwale | Contributor | 0
    • bernhardoj | Contributor | 0
Issue OwnerCurrent Issue Owner: @joekaufmanexpensify
@lanitochka17 lanitochka17 added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Apr 23, 2024
Copy link

melvin-bot bot commented Apr 23, 2024

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

@lanitochka17
Copy link
Author

@joekaufmanexpensify FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

@lanitochka17
Copy link
Author

We think that this bug might be related to #vip-vsp

@dragnoir
Copy link
Contributor

Proposal

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

Invite error is not dismissed via X button in the workspace list

What is the root cause of that problem?

This is not a simple issue:

1- the error message is turned from backend as part of policy.errors
It's in english and can't be translated to spanish

2- The area here
image

displayes all policy.errors and the close button will delete the workspace

function dismissError(policyID: string) {
PolicyUtils.goBackFromInvalidPolicy();
Policy.removeWorkspace(policyID);
}

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

1- The backend should turn an error code/string so we can translate

image

2- The error should be displayed below the members list and not on the left with the menu items

image

3- Closing the error should only dismiss the message and not remove the workspace

onClose={() => clearMembersErrors();

@joekaufmanexpensify joekaufmanexpensify added the External Added to denote the issue can be worked on by a contributor label Apr 23, 2024
Copy link

melvin-bot bot commented Apr 23, 2024

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

@melvin-bot melvin-bot bot changed the title Workspace - Invite error is not dismissed via X button in the workspace list [$250] Workspace - Invite error is not dismissed via X button in the workspace list Apr 23, 2024
@melvin-bot melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Apr 23, 2024
@joekaufmanexpensify
Copy link
Contributor

this def seems like a bug.

Copy link

melvin-bot bot commented Apr 23, 2024

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

@ZhenjaHorbach
Copy link
Contributor

ZhenjaHorbach commented Apr 23, 2024

Proposal

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

Workspace - Invite error is not dismissed via X button in the workspace list

What is the root cause of that problem?

The main problem with issue is that for disable error from Workspace list we use pendingAction from policy
But in case of adding an incorrect user, we store this pendingAction inside the list for each user

As result when we try to close error from WorkspacesListPage nothing happened

dismissError: () => {
if (!policy.pendingAction) {
return;
}
dismissWorkspaceError(policy.id, policy.pendingAction);
},

And since we receive an error due to an incorrect user, we need to update the error closure inside the workspace and in the list to just close this error without removing workspace

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

I made implementation only for WorkspacesListPage
But we cane make similar and for WorkspaceInitialPage

We can update dismissError like

                    dismissError: () => {
                        const employeeWithError = Object.keys(policy.employeeList)?.find(
                            (employee) => policy.employeeList[employee]?.errors && !isEmptyObject(policy.employeeList[employee]?.errors),
                        );
                        if (!policy.pendingAction && !employeeWithError) {
                            return;
                        }
                        dismissWorkspaceError(policy.id, policy.pendingAction, policy.employeeList?.[employeeWithError]?.pendingAction);
                    },

dismissError: () => {
if (!policy.pendingAction) {
return;
}
dismissWorkspaceError(policy.id, policy.pendingAction);
},

And then update dismissWorkspaceError

function dismissWorkspaceError(policyID: string, pendingAction: OnyxCommon.PendingAction, employeePendingAction: OnyxCommon.PendingAction) {
    if (pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
        Policy.clearDeleteWorkspaceError(policyID);
        return;
    }

    if (pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
        Policy.removeWorkspace(policyID);
        return;
    }

    if (employeePendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
        Policy.clearErrors(policyID);
        return;
    }
    throw new Error('Not implemented');
}

Plus we need update setWorkspaceErrors for removing current errors and error from employee list

App/src/libs/actions/Policy.ts

Lines 1789 to 1796 in d296ef8

function setWorkspaceErrors(policyID: string, errors: Errors) {
if (!allPolicies?.[policyID]) {
return;
}
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {errors: null});
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {errors});
}

Similar implementation we can make and for error inside workspace initial page

function dismissError(policyID: string) {
PolicyUtils.goBackFromInvalidPolicy();
Policy.removeWorkspace(policyID);
}

What alternative solutions did you explore? (Optional)

NA

@bernhardoj
Copy link
Contributor

bernhardoj commented Apr 23, 2024

Proposal

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

There are 3 issues here, first, the failed member and its error message don't show like before. Second, when we close the invite error message, the policy is removed and we are navigated back to the workspace list. Last, we can't clear the error from workspace list page/

What is the root cause of that problem?

For the first problem, we get the member data from getMemberAccountIDsForWorkspace

const policyMemberEmailsToAccountIDs = useMemo(() => PolicyUtils.getMemberAccountIDsForWorkspace(policy?.employeeList), [policy?.employeeList]);

However, getMemberAccountIDsForWorkspace ignores member that has an error.

App/src/libs/PolicyUtils.ts

Lines 157 to 159 in d296ef8

if (Object.keys(member?.errors ?? {})?.length > 0) {
return;
}

For the second problem, when closing the error message, we call dismissError which will navigate the user back to the workspace list page and remove the workspace.

function dismissError(policyID: string) {
PolicyUtils.goBackFromInvalidPolicy();
Policy.removeWorkspace(policyID);
}

For the 3rd problem, if there is no pending action, which is true for our case, we just do nothing.

dismissError: () => {
if (!policy.pendingAction) {
return;
}
dismissWorkspaceError(policy.id, policy.pendingAction);
},

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

For the first problem, we can add a new param to getMemberAccountIDsForWorkspace to include members with errors. This will show the member that has an error, however, the member won't show optimistically when we add it and only shows when there is an error. This is because we assign the optimistic member with a login from invitedEmailsToAccountIDs and the login from invitedEmailsToAccountIDs for phone number doesn't have @expensify.sms suffix, so when we search for the personal detail, we won't find anything and the member won't show.

App/src/libs/actions/Policy.ts

Lines 1403 to 1409 in b8d1908

Object.keys(invitedEmailsToAccountIDs).forEach((email) => {
optimisticMembersState[email] = {pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD, role: CONST.POLICY.ROLE.USER};
successMembersState[email] = {pendingAction: null};
failureMembersState[email] = {
errors: ErrorUtils.getMicroSecondOnyxError('workspace.people.error.genericAdd'),
};
});

To fix it, we should iterate over logins instead.

const logins = Object.keys(invitedEmailsToAccountIDs).map((memberLogin) => PhoneNumber.addSMSDomainIfPhoneNumber(memberLogin));

logins.forEach((email) => { ...

we also need to add personalDetails to this memo deps, otherwise it won't recalculate when personalDetails is updated.

const policyMemberEmailsToAccountIDs = useMemo(() => PolicyUtils.getMemberAccountIDsForWorkspace(policy?.employeeList), [policy?.employeeList]);

In 3rd problem, we already handle the case for add/delete pending action.

function dismissWorkspaceError(policyID: string, pendingAction: OnyxCommon.PendingAction) {
if (pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE) {
Policy.clearDeleteWorkspaceError(policyID);
return;
}
if (pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD) {
Policy.removeWorkspace(policyID);
return;
}
throw new Error('Not implemented');
}

To solve the 3rd problem, we will remove the pending action check from the code below.

if (!policy.pendingAction) {
return;
}
dismissWorkspaceError(policy.id, policy.pendingAction);

In dismissWorkspaceError, if the pending action is empty, then we just want to clear the error. We have Policy.clearErrors,

App/src/libs/actions/Policy.ts

Lines 2503 to 2506 in a852ea7

function clearErrors(policyID: string) {
setWorkspaceErrors(policyID, {});
hideWorkspaceAlertMessage(policyID);
}

but it doesn't work because we accessed the key wrongly, it should be

if (!allPolicies?.[`${ONYXKEYS.COLLECTION.POLICY}${policyID}`])

but even if we fix this one, looks like the double merge with a null and empty object doesn't work at all.

App/src/libs/actions/Policy.ts

Lines 1789 to 1796 in a852ea7

function setWorkspaceErrors(policyID: string, errors: Errors) {
if (!allPolicies?.[policyID]) {
return;
}
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {errors: null});
Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {errors});
}

To simplify it, we can just update Policy.clearErrors to replace setWorkspaceErrors with

Onyx.merge(`${ONYXKEYS.COLLECTION.POLICY}${policyID}`, {errors: null});

To solve the 2nd problem, we will update the onClose code of the error to be like this. We check !policyID for invalid policy ID error. I don't think we need to add for delete case like in above, but if we want to add it, we can just add it to follow the similar pattern in dismissWorkspaceError above.

onClose={() => {
    if (!policyID || policy?.pendingAction == 'add') {
        dismissError(policyID)
    } else {
        Policy.clearErrors(policyID);
    }
}}

@bernhardoj
Copy link
Contributor

Updated proposal to include more fix

@joekaufmanexpensify
Copy link
Contributor

Proposals pending review

@joekaufmanexpensify
Copy link
Contributor

@Santhosh-Sellavel mind reviewing?

@Santhosh-Sellavel
Copy link
Collaborator

Sorry this one, skipped my list will get to it later today!

@joekaufmanexpensify
Copy link
Contributor

TY!

@melvin-bot melvin-bot bot added the Overdue label Apr 29, 2024
@joekaufmanexpensify
Copy link
Contributor

Pending review from @Santhosh-Sellavel

@melvin-bot melvin-bot bot removed the Overdue label Apr 29, 2024
@Santhosh-Sellavel
Copy link
Collaborator

Sorry for the back and forth. I'm Just gonna unassign this as my bandwidth is too low. Asked here for volunteers

@Santhosh-Sellavel
Copy link
Collaborator

@arosiclair Will takeover this as C+!

@akinwale
Copy link
Contributor

Looks like an auto complete error above. Taking over here as C+.

Copy link

melvin-bot bot commented May 1, 2024

📣 @bernhardoj 🎉 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 📖

@melvin-bot melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 and removed Daily KSv2 labels May 2, 2024
@bernhardoj
Copy link
Contributor

PR is ready

cc: @akinwale

@melvin-bot melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels May 13, 2024
@melvin-bot melvin-bot bot changed the title [$250] Workspace - Invite error is not dismissed via X button in the workspace list [HOLD for payment 2024-05-20] [$250] Workspace - Invite error is not dismissed via X button in the workspace list May 13, 2024
@melvin-bot melvin-bot bot removed the Reviewing Has a PR in review label May 13, 2024
Copy link

melvin-bot bot commented May 13, 2024

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

Copy link

melvin-bot bot commented May 13, 2024

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

If no regressions arise, payment will be issued on 2024-05-20. 🎊

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

Copy link

melvin-bot bot commented May 13, 2024

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

  • [@akinwale] The PR that introduced the bug has been identified. Link to the PR: N/A
  • [@akinwale] 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
  • [@akinwale] A discussion in #expensify-bugs 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
  • [@akinwale] Determine if we should create a regression test for this bug.
  • [@akinwale] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
  • [@joekaufmanexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon: N/A

@joekaufmanexpensify
Copy link
Contributor

@akinwale could you please handle the BZ checklist this week so we can prep for payment?

@melvin-bot melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels May 19, 2024
@joekaufmanexpensify
Copy link
Contributor

bump @akinwale could you please handle BZ here so we can issue payment?

@akinwale
Copy link
Contributor

  • [@akinwale] The PR that introduced the bug has been identified. Link to the PR:
  • [@akinwale] 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:
  • [@akinwale] A discussion in #expensify-bugs 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:

Not a regression. This was not being specifically handled.

  • [@akinwale] Determine if we should create a regression test for this bug.
  • [@akinwale] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.

Regression Test Steps

  1. Launch Expensify.
  2. Open the workspace members as a workspace admin.
  3. Invite +252 3 234211.
  4. Verify that the invited member is displayed with an error message.
  5. Clear the displayed error message.
  6. On the workspace settings page, clear the error message.
  7. Verify that the error message is cleared.
  8. Repeat steps 2 through 4.
  9. Navigate back to the workspace list page.
  10. Clear the error message displayed below the list of workspace items.
  11. Verify that the error message is cleared.

Do we agree 👍 or 👎?

@akinwale
Copy link
Contributor

@joekaufmanexpensify Done!

@joekaufmanexpensify
Copy link
Contributor

Hmm, I feel like this is a pretty niche issue. I feel like it's fine to have no regression test for this.

@joekaufmanexpensify
Copy link
Contributor

Checklist is complete!

@joekaufmanexpensify
Copy link
Contributor

All set to issue payment. We need to pay:

@joekaufmanexpensify
Copy link
Contributor

@bernhardoj $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

@akinwale $250 sent and contract ended!

@joekaufmanexpensify
Copy link
Contributor

Upwork job closed.

@joekaufmanexpensify
Copy link
Contributor

All set, TY everyone!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Payment Auto-added when associated PR is deployed to production Bug Something is broken. Auto assigns a BugZero manager. Daily KSv2 External Added to denote the issue can be worked on by a contributor
Projects
No open projects
Archived in project
Development

No branches or pull requests

8 participants