[Hold for payment 05-29-2024][$250] Group chat - "Make member" icon is the same as "Make admin"

[lanitochka17]

No description provided.

[melvin-bot]

Triggered auto assignment to @lschurr (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]

@lschurr 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]

We think that this bug might be related to #wave-collect - Release 1

[cretadn22]

Proposal

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

What is the root cause of that problem?

App/src/pages/ReportParticipantsPage.tsx

Lines 233 to 240 in d03b4b9

	if (isAtleastOneAdminSelected) {
	options.push({
	text: translate('workspace.people.makeMember'),
	value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_MEMBER,
	icon: Expensicons.MakeAdmin,
	onSelected: () => changeUserRole(CONST.REPORT.ROLE.MEMBER),
	});
	}

We used wrong icon in ReportParticipantsPage

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

Update to icon: Expensicons.User,

What alternative solutions did you explore? (Optional)

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text. Do not create PRs unless you have been hired for this job.

[ShridharGoel]

Proposal

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

Group chat - "Make member" icon is the same as "Make admin".

What is the root cause of that problem?

A wrong icon is used:

App/src/pages/ReportParticipantsPage.tsx

Lines 234 to 239 in d03b4b9

	options.push({
	text: translate('workspace.people.makeMember'),
	value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_MEMBER,
	icon: Expensicons.MakeAdmin,
	onSelected: () => changeUserRole(CONST.REPORT.ROLE.MEMBER),
	});

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

Use Expensicons.User when isAtleastOneAdminSelected is true.

if (isAtleastOneAdminSelected) {
    options.push({
        text: translate('workspace.people.makeMember'),
        value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_MEMBER,
        icon: Expensicons.User,
        onSelected: () => changeUserRole(CONST.REPORT.ROLE.MEMBER),
    });
}

Also, this logic is duplicated in ReportParticipantsPage and WorkspaceMembersPage. We should move it to a common utils method.

const getRoleChangeOption = (option, changeUserRole) => {
    if (option === CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_MEMBER) {
        return {
            text: translate('workspace.people.makeMember'),
            value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_MEMBER,
            icon: Expensicons.User,
            onSelected: () => changeUserRole(CONST.REPORT.ROLE.MEMBER),
        }
    } else if (option === CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_ADMIN) {
        return {
            text: translate('workspace.people.makeAdmin'),
            value: CONST.POLICY.MEMBERS_BULK_ACTION_TYPES.MAKE_ADMIN,
            icon: Expensicons.MakeAdmin,
            onSelected: () => changeUserRole(CONST.REPORT.ROLE.ADMIN),
        }
    }
}

Then, use this at both the places.

We can also add common logic for remove members option, isAtleastOneAdminSelected and isAtleastOneMemberSelected.

[melvin-bot]

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

[melvin-bot]

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

[ishpaul777]

Thanks for proposals @ShridharGoel @cretadn22!

@ShridharGoel the only thing differentiate your proposal from @cretadn22 is a minor refactor suggestion, unrelated to the bug which i dont find enough to discard the first proposal.

So Proposal from @cretadn22 LGTM!

🎀 👀 🎀 C+ reviewed

[melvin-bot]

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

[ShridharGoel]

@ishpaul777 Don't you think the issue wouldn't have happened in the first place if a common method was being used? This refactor can help prevent such issues when more changes happen later.

[ShridharGoel]

The almost same code was using correct icon at one place and incorrect at another.

[ishpaul777]

@ishpaul777 Don't you think the issue wouldn't have happened in the first place if a common method was being used? This refactor can help prevent such issues when more changes happen later.

This will be very niche use for a util i dont think it really worth it to have a utility for such a small usecase having it in same component improves code readability IMO, that being said i'll let assigned engineer make final call on assignment.

[cretadn22]

@ishpaul777 Is this waiting for me to create PR? Or what pending here?

[dangrous]

Yeah I think let's go with the simpler (first) solution for now. I totally understand where you're coming from, @ShridharGoel, but I agree that for readability it makes sense to stick with the format we already have. Maybe if we have more roles in the future we can rethink, but for the two we have now (and this bug) I'll assign @cretadn22!

[cretadn22]

@ishpaul777 @dangrous I created the PR #41776

[trjExpensify]

Group chat is in #vip-split, moving.

[dangrous]

Looks like the automation didn't fire but this hit prod 5/22 - so should be paid 5/29

[lschurr]

Payment summary:

• Contributor: @cretadn22 $250 (offer sent in Upwork)
• Contributor+: @ishpaul777 $250 (offer sent in Upwork)

[lschurr]

Great! Offer sent in Upwork.

[cretadn22]

@lschurr Accepted your offer. Thanks 🙏

[lschurr]

Payments have been sent in Upwork. All set to close this one out.