[HOLD for payment 2023-10-24] [$500] Workspace - Admin Checkbox in Members Section Ticked When Changing Contact Method #28539

kbecciv · 2023-09-30T15:56:56Z

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

Action Performed:

Go to Settings > Profile > Contact Method > Add New Contact Method
Go to Settings > Create Workspace > Members > Invite Members
Go to Settings > Profile > Contact Method > Change Contact Method
Go to Settings > Workspace > Members > Tick on the Select All checkboxes and observe that the Admin checkbox is also selected

Expected Result:

Changing the contact method should not cause the Admin checkbox to be ticked in the Members section.

Actual Result:

Changing the contact method causes the Admin checkbox to be ticked in the Members section.

Workaround:

Unknown

Platforms:

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

Version Number: 1.3.73.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:
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Notes/Photos/Videos: Any additional supporting documentation

Recorder_30092023_183937.mp4

Recorder_30092023_161410.mp4

Recorder_30092023_085849.mp4

Screen.Recording.2023-09-29.at.10.38.26.PM.mov

Recording.1631.mp4

screen-capture.-.2023-09-22T140546.004.webm

screen-capture.5.mp4

Expensify/Expensify Issue URL:
Issue reported by: @tewodrosGirmaA
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1695415622144909

View all open jobs on GitHub

esh-g · 2023-09-30T15:56:57Z

Proposal

Please re-state the issue

Able to select the policy owner after changing the contact method.

What is the root cause of this issue?

While checking whether an option should be disabled, we are setting the isDisabled property to true here:

App/src/pages/workspace/WorkspaceMembersPage.js

Line 317 in 4ea4a55

    
           isDisabled: accountID === props.session.accountID || details.login === props.policy.owner || policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,

But the accountID from props.policyMembers is actually a string while the props.session.accountID is actually a Number.

What changes do you think should be made to fix this problem?

We should convert the accountID from props.policyMembers to Number before comparing like we do in many other places for accountID.

isDisabled: Number(accountID) === props.session.accountID || details.login === props.policy.owner || policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,

What other alternative options did you explore? (Optional)

melvin-bot · 2023-09-30T15:57:01Z

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

melvin-bot · 2023-09-30T15:57:02Z

Triggered auto assignment to @alexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

melvin-bot · 2023-09-30T15:57:04Z

Bug0 Triage Checklist (Main S/O)

This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

melvin-bot · 2023-09-30T15:57:06Z

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

dukenv0307 · 2023-09-30T16:35:01Z

Proposal

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

Workspace - Admin Checkbox in Members Section Ticked When Changing Contact Method

What is the root cause of that problem?

When we change the contact method, policy.onwer is not updated until the openApp API is called again so details.login === props.policy.owner is false. Additional, props.session.accountID is string and accountID is number that makes accountID === props.session.accountID is always false

App/src/pages/workspace/WorkspaceMembersPage.js

Line 317 in 52f0e3c

    
           isDisabled: accountID === props.session.accountID || details.login === props.policy.owner || policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,

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

Instead of compare accountID === props.session.accountID we can re-use isAdmin or use props.session.email === details.login

App/src/pages/workspace/WorkspaceMembersPage.js

Line 311 in 52f0e3c

    
           const isAdmin = props.session.email === details.login || policyMember.role === CONST.POLICY.ROLE.ADMIN;

App/src/pages/workspace/WorkspaceMembersPage.js

Line 317 in 52f0e3c

    
           isDisabled: accountID === props.session.accountID || details.login === props.policy.owner || policyMember.pendingAction === CONST.RED_BRICK_ROAD_PENDING_ACTION.DELETE,

What alternative solutions did you explore? (Optional)

In setContactMethodAsDefault function we can update policy.onwer of all policies of current user to new contact method in optimisticData and reset it in failureData

alexpensify · 2023-10-03T23:34:42Z

@parasharrajat - when you get a chance can you review if one of these proposals will resolve this issue? Thanks!

alexpensify · 2023-10-06T00:50:57Z

@parasharrajat any update here?

parasharrajat · 2023-10-06T04:40:10Z

Started reviewing...

parasharrajat · 2023-10-08T13:38:48Z

Nice Catch. @esh-g proposal looks good to me. We migrated to accountId so that should be used. But this feels more like a backend change. We should use one type for account IDs everywhere.

🎀 👀 🎀 C+ reviewed

melvin-bot · 2023-10-08T13:38:55Z

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

alexpensify · 2023-10-10T20:39:28Z

Marc is OOO but will review soon. If Marc is unable to help, I'll find help to keep it moving forward.

marcaaron · 2023-10-10T22:12:01Z

the accountID from props.policyMembers is actually a string while the props.session.accountID is actually a Number.

Not sure why the accountID would ever be a string though. This sounds like more of a backend error to me.

cc @puneetlath I think this change was first introduced here in case you have some idea.

marcaaron · 2023-10-10T22:18:05Z

Rejected the proposal because:

We should convert the accountID from props.policyMembers to Number before comparing like we do in many other places for accountID.

It looks like we already do this here.

marcaaron · 2023-10-10T22:18:36Z

Passing the 🏀 back to you @parasharrajat 😄

marcaaron · 2023-10-10T22:19:33Z

Oh duh I see now that the isDisabled logic is set in the same object 🤦

marcaaron · 2023-10-10T22:24:09Z

I think ideally we can fix this at a lower level instead of cast to a Number everywhere. Let's find out where we are setting the ONYXKEYS.COLLECTION.POLICY_MEMBERS accountID to a string.

parasharrajat · 2023-10-11T06:23:11Z

@esh-g can you analyze this #28539 (comment)?

esh-g · 2023-10-12T13:38:29Z

@parasharrajat PR is ready: #29422 💫

marcaaron · 2023-10-12T19:19:15Z

Yep exactly

alexpensify · 2023-10-13T17:21:44Z

Awesome, looks like the PR is moving forward!

melvin-bot · 2023-10-17T23:06:04Z

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

melvin-bot · 2023-10-17T23:06:09Z

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

Workspace admin checkbox #29422

If no regressions arise, payment will be issued on 2023-10-24. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

External issue reporter
Contributor that fixed the issue
Contributor+ that helped on the issue and/or PR

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

@parasharrajat does not require payment (Eligible for Manual Requests)
@esh-g requires payment
@tewodrosGirmaA requires payment

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot · 2023-10-17T23:06:10Z

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:

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR:
[@parasharrajat] 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:
[@parasharrajat] 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:
[@parasharrajat] Determine if we should create a regression test for this bug.
[@parasharrajat] 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.
[@alexpensify] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

alexpensify · 2023-10-20T23:14:35Z

@parasharrajat - to prepare for the payment date, can you please complete the checklist? Thanks!

alexpensify · 2023-10-23T22:50:20Z

@parasharrajat - please complete the checklist. Thanks!

parasharrajat · 2023-10-24T07:21:21Z

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:

[@parasharrajat] The PR that introduced the bug has been identified. Link to the PR: refactor: SelectionList multiple selection #22622
[@parasharrajat] 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/22622/files#r1369721574
[@parasharrajat] 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 required. looks like a simple mistake. Could have been caught in testing.
[@parasharrajat] Determine if we should create a regression test for this bug. Yes
[@parasharrajat] 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

Create a new workspace.
Go to workspace members page.
Make sure we are not able to select the workspace admin (current user)
Go to contact methods.
Add a new contact method and set it as your default.
Go to workspace members page.
Make sure we are not able to select the workspace admin (current user)

Do you agree 👍 or 👎 ?

alexpensify · 2023-10-24T23:45:53Z

@marcaaron - can we confirm if you agree with the regression test above?

marcaaron · 2023-10-24T23:51:29Z

LGTM

alexpensify · 2023-10-24T23:53:04Z

Here is the payment summary:

External issue reporter @tewodrosGirmaA $50
Contributor that fixed the issue @esh-g $750
Contributor+ that helped on the issue and/or PR @parasharrajat $750

Upwork Job: https://www.upwork.com/jobs/~01ccfbef1a9c9e1edd

*If applicable, the bonuses will be applied on the final payment

Extra Notes regarding payment: There is an urgency bonus and hit the 3-day business day mark.

alexpensify · 2023-10-24T23:54:19Z

@tewodrosGirmaA and @esh-g - can you please apply to the link above and I can complete the next steps in Upwork. Not sure what happened but the automation didn't kick in here. Thanks!

esh-g · 2023-10-25T02:12:48Z

Applied @alexpensify 🚀

tewodrosGirmaA · 2023-10-25T03:44:05Z

Applied @alexpensify

alexpensify · 2023-10-25T17:17:34Z

Thanks! I need your approval in Upwork, and then can complete the process.

esh-g · 2023-10-25T17:41:18Z

Accepted the offer @alexpensify

alexpensify · 2023-10-25T22:44:08Z

Everyone has been paid via Upwork.

@parasharrajat please submit your request too. Thanks!

alexpensify · 2023-10-25T22:55:12Z

Alright, the regression test has been created so I'm going to close out the issue. Great work here!

parasharrajat · 2023-12-05T06:56:34Z

Payment requested as per #28539 (comment)

JmillsExpensify · 2023-12-11T01:28:42Z

$750 payment approved for @parasharrajat based on this comment.

kbecciv added External Added to denote the issue can be worked on by a contributor Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 30, 2023

melvin-bot bot changed the title ~~Workspace - Admin Checkbox in Members Section Ticked When Changing Contact Method~~ [$500] Workspace - Admin Checkbox in Members Section Ticked When Changing Contact Method Sep 30, 2023

melvin-bot bot assigned alexpensify Sep 30, 2023

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Sep 30, 2023

melvin-bot bot assigned parasharrajat Sep 30, 2023

melvin-bot bot added the Overdue label Oct 3, 2023

melvin-bot bot removed the Overdue label Oct 3, 2023

melvin-bot bot assigned marcaaron Oct 8, 2023

melvin-bot bot added the Overdue label Oct 10, 2023

melvin-bot bot removed the Overdue label Oct 10, 2023

melvin-bot bot added Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Weekly KSv2 labels Oct 17, 2023

melvin-bot bot changed the title ~~[$500] Workspace - Admin Checkbox in Members Section Ticked When Changing Contact Method~~ [HOLD for payment 2023-10-24] [$500] Workspace - Admin Checkbox in Members Section Ticked When Changing Contact Method Oct 17, 2023

melvin-bot bot removed the Reviewing Has a PR in review label Oct 17, 2023

parasharrajat mentioned this issue Oct 24, 2023

refactor: SelectionList multiple selection #22622

Merged

62 tasks

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 24, 2023

alexpensify closed this as completed Oct 25, 2023

[HOLD for payment 2023-10-24] [$500] Workspace - Admin Checkbox in Members Section Ticked When Changing Contact Method #28539

[HOLD for payment 2023-10-24] [$500] Workspace - Admin Checkbox in Members Section Ticked When Changing Contact Method #28539

Comments

kbecciv commented Sep 30, 2023 • edited Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

esh-g commented Sep 30, 2023

Proposal

Please re-state the issue

What is the root cause of this issue?

What changes do you think should be made to fix this problem?

What other alternative options did you explore? (Optional)

melvin-bot bot commented Sep 30, 2023

melvin-bot bot commented Sep 30, 2023

melvin-bot bot commented Sep 30, 2023

Bug0 Triage Checklist (Main S/O)

melvin-bot bot commented Sep 30, 2023

dukenv0307 commented Sep 30, 2023

Proposal

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

What is the root cause of that problem?

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

What alternative solutions did you explore? (Optional)

alexpensify commented Oct 3, 2023

alexpensify commented Oct 6, 2023

parasharrajat commented Oct 6, 2023

parasharrajat commented Oct 8, 2023 • edited Loading

melvin-bot bot commented Oct 8, 2023

alexpensify commented Oct 10, 2023

marcaaron commented Oct 10, 2023

marcaaron commented Oct 10, 2023

marcaaron commented Oct 10, 2023

marcaaron commented Oct 10, 2023

marcaaron commented Oct 10, 2023 • edited Loading

parasharrajat commented Oct 11, 2023

esh-g commented Oct 12, 2023

marcaaron commented Oct 12, 2023

alexpensify commented Oct 13, 2023

melvin-bot bot commented Oct 17, 2023

melvin-bot bot commented Oct 17, 2023

melvin-bot bot commented Oct 17, 2023

alexpensify commented Oct 20, 2023

alexpensify commented Oct 23, 2023

parasharrajat commented Oct 24, 2023

Regression Test Steps

alexpensify commented Oct 24, 2023

marcaaron commented Oct 24, 2023

alexpensify commented Oct 24, 2023

alexpensify commented Oct 24, 2023

esh-g commented Oct 25, 2023

tewodrosGirmaA commented Oct 25, 2023 • edited Loading

alexpensify commented Oct 25, 2023 • edited Loading

esh-g commented Oct 25, 2023

alexpensify commented Oct 25, 2023

alexpensify commented Oct 25, 2023

parasharrajat commented Dec 5, 2023

JmillsExpensify commented Dec 11, 2023

kbecciv commented Sep 30, 2023 •

edited

Loading

parasharrajat commented Oct 8, 2023 •

edited

Loading

marcaaron commented Oct 10, 2023 •

edited

Loading

tewodrosGirmaA commented Oct 25, 2023 •

edited

Loading

alexpensify commented Oct 25, 2023 •

edited

Loading