[HOLD for payment 2024-10-17] [$250] Plural Remove Members on the workspace members page even when we only have one member selected #49961

m-natarajan · 2024-09-30T22:38:48Z

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: 9.0.41-6
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
Expensify/Expensify Issue URL:
Issue reported by: @allgandalf
Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1727729303045069

Action Performed:

Go to staging.new.expensify.com
Select any workspace from settings which have members
Click Members > select any added member
Click the dropdown in the 1 selected pill

Expected Result:

Remove member displayed as only one member selected

Actual Result:

Remove members displayed even if only one member selected

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

Add any screenshot/video evidence

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~021841413637223772836
Upwork Job ID: 1841413637223772836
Last Price Increase: 2024-10-02
Automatic offers:

allgandalf | Reviewer | 104321898
nkdengineer | Contributor | 104321900

Issue Owner

Current Issue Owner: @

Issue Owner

Current Issue Owner: @trjExpensify

The text was updated successfully, but these errors were encountered:

melvin-bot · 2024-09-30T22:38:57Z

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

github-actions · 2024-09-30T23:02:19Z

adrian-brc Your proposal will be dismissed because you did not follow the proposal template.

Krishna2323 · 2024-09-30T23:09:30Z

Edited by proposal-police: This proposal was edited at 2024-09-30 23:16:29 UTC.

Proposal

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

Plural Remove Members on the workspace members page even when we only have one member selected

What is the root cause of that problem?

We are using always using workspace.people.removeMembersTitle.

App/src/pages/workspace/WorkspaceMembersPage.tsx

Line 480 in 98ac9ac

text: translate('workspace.people.removeMembersTitle'),

App/src/pages/workspace/WorkspaceMembersPage.tsx

Line 640 in 98ac9ac

title={translate('workspace.people.removeMembersTitle')}

App/src/pages/RoomMembersPage.tsx

Line 276 in 8f5f609

text: translate('workspace.people.removeMembersTitle'),

App/src/pages/RoomMembersPage.tsx

Line 371 in 8f5f609

title={translate('workspace.people.removeMembersTitle')}

App/src/pages/ReportParticipantsPage.tsx

Line 269 in 8f5f609

text: translate('workspace.people.removeMembersTitle'),

App/src/pages/ReportParticipantsPage.tsx

Line 393 in 8f5f609

title={translate('workspace.people.removeMembersTitle')}

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

We should check selectedEmployees.length and use workspace.people.removeMemberTitle if it is 1 otherwise workspace.people.removeMembersTitle.
The same need to be done in ReportParticipantsPage & RoomMembersPage. In these components we need to use selectedMembers.length

We also need to update confirmModalPrompt.

App/src/pages/workspace/WorkspaceMembersPage.tsx

Lines 102 to 111 in 8f5f609

    
           const confirmModalPrompt = useMemo(() => { 
        
               const approverAccountID = selectedEmployees.find((selectedEmployee) => Member.isApprover(policy, selectedEmployee)); 
        
               if (!approverAccountID) { 
        
                   return translate('workspace.people.removeMembersPrompt'); 
        
               } 
        
               return translate('workspace.people.removeMembersWarningPrompt', { 
        
                   memberName: getDisplayNameForParticipant(approverAccountID), 
        
                   ownerName: getDisplayNameForParticipant(policy?.ownerAccountID), 
        
               }); 
        
           }, [selectedEmployees, policy, translate]);

TO:

    const confirmModalPrompt = useMemo(() => {
        const approverAccountID = selectedEmployees.find((selectedEmployee) => Member.isApprover(policy, selectedEmployee));
        if (!approverAccountID) {
            return selectedEmployees.length === 1
                ? translate('workspace.people.removeMemberPrompt', {memberName: getDisplayNameForParticipant(selectedEmployees[0])})
                : translate('workspace.people.removeMembersPrompt');
        }
        return translate('workspace.people.removeMembersWarningPrompt', {
            memberName: getDisplayNameForParticipant(approverAccountID),
            ownerName: getDisplayNameForParticipant(policy?.ownerAccountID),
        });
    }, [selectedEmployees, policy, translate]);

Same should be done in ReportParticipantsPage & RoomMembersPage.

App/src/pages/ReportParticipantsPage.tsx

Line 397 in 8f5f609

prompt={translate('workspace.people.removeMembersPrompt')}

App/src/pages/RoomMembersPage.tsx

Line 375 in 8f5f609

prompt={translate('roomMembersPage.removeMembersPrompt')}

What alternative solutions did you explore? (Optional)

Krishna2323 · 2024-09-30T23:19:28Z

PROPOSAL UPDATED

Updated main solution to include the changes for the prompt.

allgandalf · 2024-10-01T05:23:38Z

@trjExpensify Can i be the C+ for this issue? I reported this bug on slack

trjExpensify · 2024-10-02T09:42:58Z

Sure!

melvin-bot · 2024-10-02T09:43:20Z

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

melvin-bot · 2024-10-02T09:43:24Z

Current assignee @allgandalf is eligible for the External assigner, not assigning anyone new.

nkdengineer · 2024-10-02T10:24:15Z

Edited by proposal-police: This proposal was edited at 2024-10-02 10:44:25 UTC.

Proposal

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

Remove members displayed even if only one member selected

What is the root cause of that problem?

We're always using workspace.people.removeMembersTitle without checking whether the selected employee is one or many

App/src/pages/workspace/WorkspaceMembersPage.tsx

Line 480 in 4878161

text: translate('workspace.people.removeMembersTitle'),

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

Following the same pattern of this translation we can update removeMembersTitle translation like this

removeMembersTitle: () => ({
    one: 'Remove member',
    other: 'Remove members',
}),
removeMembersTitle: () => ({
    one: 'Eliminar miembro',
    other: 'Eliminar miembros',
}),

App/src/languages/en.ts

Line 883 in 4878161

deleteConfirmation: () => ({

Then for each place below, we only need to pass the count param as the length of the selected member to this translation to display the translation accordingly with the length of the selected member.

App/src/pages/ReportParticipantsPage.tsx

Line 269 in 4878161

text: translate('workspace.people.removeMembersTitle'),

App/src/pages/ReportParticipantsPage.tsx

Line 393 in 4878161

title={translate('workspace.people.removeMembersTitle')}

App/src/pages/workspace/WorkspaceMembersPage.tsx

Line 480 in 4878161

text: translate('workspace.people.removeMembersTitle'),

App/src/pages/workspace/WorkspaceMembersPage.tsx

Line 640 in 4878161

title={translate('workspace.people.removeMembersTitle')}

App/src/pages/ReportParticipantsPage.tsx

Line 269 in 4878161

text: translate('workspace.people.removeMembersTitle'),

App/src/pages/RoomMembersPage.tsx

Line 276 in 4878161

text: translate('workspace.people.removeMembersTitle'),

App/src/pages/RoomMembersPage.tsx

Line 371 in 4878161

title={translate('workspace.people.removeMembersTitle')}

When we implement this, we should wrap this logic and other place that we use the new translation in InteractionsManager.runAfterInteraction or use Modal.close to ensure the modal is displayed correctly when we confirm delete.

https://github.com/nkdengineer/App/blob/487816184680744a2ad53a03460cbbdec363c432/src/pages/workspace/WorkspaceMembersPage.tsx#L211

OPTIONAL: We can make the same approach for removeMembersPrompt translation. And update all places that we're using removeMembersPrompt without checking the length of the selected member

removeMembersPrompt: ({memberName}: {memberName: string}) => ({
    one: `Are you sure you want to remove ${memberName}?`,
    other: 'Are you sure you want to remove these members?',
}),

App/src/languages/en.ts

Line 3352 in 4878161

removeMembersPrompt: 'Are you sure you want to remove these members?',

With this approach we can remove removeMemberTitle and removeMemberPrompt translation and use removeMembersTitle/removeMembersPrompt with count param as 1

What alternative solutions did you explore? (Optional)

nkdengineer · 2024-10-02T10:44:39Z

Updated proposal

Fix another case.

allgandalf · 2024-10-05T06:05:29Z

I will review this on monday not overdue

trjExpensify · 2024-10-07T23:55:31Z

@allgandalf did you review?

allgandalf · 2024-10-08T07:24:08Z

Sorry I was occupied with a deploy blocker yesterday, Reviewing now ♻️

allgandalf · 2024-10-08T07:39:37Z

♻️ both @Krishna2323 's and @nkdengineer 's proposals solve this bug.

🟢 Both have the correct RCA which addresses why the bug occurs

💭 In particular I feel @nkdengineer 's proposal is a bit of over-engineering and @Krishna2323 's proposal is a simplified approach towards this bug. So I would leave it upto the internal engineer for this case which one to select.

I would prefer @Krishna2323 's proposal though.

🎀👀🎀 C+ reviewed

melvin-bot · 2024-10-08T07:39:50Z

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

nkdengineer · 2024-10-08T07:49:09Z

In particular I feel @nkdengineer 's #49961 (comment) is a bit of over-engineering

@allgandalf I don't think so.

We already have a translation like this here

App/src/languages/en.ts

Lines 883 to 885 in 4878161

    
           deleteConfirmation: () => ({ 
        
               one: 'Are you sure that you want to delete this expense?', 
        
               other: 'Are you sure that you want to delete these expenses?',

App/src/components/Search/SearchPageHeader.tsx

Line 284 in 4878161

    
           title={translate('iou.deleteExpense', {count: selectedTransactionsKeys.length})}

App/src/components/Search/SearchPageHeader.tsx

Lines 146 to 148 in 4878161

    
           InteractionManager.runAfterInteractions(() => { 
        
               clearSelectedTransactions(); 
        
           });

With this approach, we can get the translation cleaner everywhere we use this. We only need to pass to count param to this translation instead of has an if else in all places to get the title.

cc @rlinoz

allgandalf · 2024-10-08T07:50:38Z

Lets see what @rlinoz opinion is here 😄

Krishna2323 · 2024-10-08T09:34:22Z

We also have translation, as I suggested in my proposal, so I don't think that matters much. My proposal does solve the issue in a way that is commonly used in many places.

App/src/pages/workspace/reportFields/WorkspaceReportFieldsPage.tsx

Line 168 in 41c0b9d

    
           text: translate(selectedReportFields.length === 1 ? 'workspace.reportFields.delete' : 'workspace.reportFields.deleteFields'),

App/src/pages/workspace/reportFields/WorkspaceReportFieldsPage.tsx

Line 274 in 41c0b9d

    
           title={translate(selectedReportFields.length === 1 ? 'workspace.reportFields.delete' : 'workspace.reportFields.deleteFields')}

App/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx

Line 186 in 41c0b9d

    
           text: translate(selectedCategoriesArray.length === 1 ? 'workspace.categories.deleteCategory' : 'workspace.categories.deleteCategories'),

App/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx

Line 206 in 41c0b9d

    
           text: translate(enabledCategories.length === 1 ? 'workspace.categories.disableCategory' : 'workspace.categories.disableCategories'),

App/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx

Line 228 in 41c0b9d

    
           text: translate(disabledCategories.length === 1 ? 'workspace.categories.enableCategory' : 'workspace.categories.enableCategories'),

App/src/pages/workspace/categories/WorkspaceCategoriesPage.tsx

Lines 377 to 378 in 41c0b9d

    
           title={translate(selectedCategoriesArray.length === 1 ? 'workspace.categories.deleteCategory' : 'workspace.categories.deleteCategories')} 
        
           prompt={translate(selectedCategoriesArray.length === 1 ? 'workspace.categories.deleteCategoryPrompt' : 'workspace.categories.deleteCategoriesPrompt')}

App/src/pages/workspace/reportFields/ReportFieldsListValuesPage.tsx

Line 186 in 41c0b9d

    
           text: translate(selectedValuesArray.length === 1 ? 'workspace.reportFields.deleteValue' : 'workspace.reportFields.deleteValues'),

App/src/pages/workspace/reportFields/ReportFieldsListValuesPage.tsx

Line 208 in 41c0b9d

    
           text: translate(enabledValues.length === 1 ? 'workspace.reportFields.disableValue' : 'workspace.reportFields.disableValues'),

App/src/pages/workspace/reportFields/ReportFieldsListValuesPage.tsx

Line 240 in 41c0b9d

    
           text: translate(disabledValues.length === 1 ? 'workspace.reportFields.enableValue' : 'workspace.reportFields.enableValues'),

App/src/pages/workspace/reportFields/ReportFieldsListValuesPage.tsx

Lines 343 to 344 in 41c0b9d

    
           title={translate(selectedValuesArray.length === 1 ? 'workspace.reportFields.deleteValue' : 'workspace.reportFields.deleteValues')} 
        
           prompt={translate(selectedValuesArray.length === 1 ? 'workspace.reportFields.deleteValuePrompt' : 'workspace.reportFields.deleteValuesPrompt')}

cc: @rlinoz

rlinoz · 2024-10-08T14:24:44Z

From the readme of the project it looks like we prefer to implement plurals with the count option, also checked internally and that is the general feel as well, so let's go with @nkdengineer's proposal

melvin-bot · 2024-10-08T14:25:11Z

📣 @allgandalf 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

melvin-bot · 2024-10-08T14:25:15Z

📣 @nkdengineer 🎉 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 · 2024-10-10T23:04:18Z

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

melvin-bot · 2024-10-10T23:04:23Z

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

fix: Plural Remove Members on the workspace when we only have one #50488

If no regressions arise, payment will be issued on 2024-10-17. 🎊

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

@allgandalf requires payment automatic offer (Reviewer)
@nkdengineer requires payment automatic offer (Contributor)
@allgandalf requires payment automatic offer (Reviewer)

melvin-bot · 2024-10-10T23:04:24Z

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:

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

allgandalf · 2024-10-16T18:43:20Z

The PR that introduced the bug has been identified. Link to the PR: N/A (we never considered the case for singular/plural options
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
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
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 Proposal

Select any workspace from settings which have members
Click Members > select any added member
Click the dropdown in the 1 selected pill

Verify that: Remove member displayed as only one member selected

Click Remove member

Verify that: modal prompt displayed as only one member selected

Do we agree 👍 or 👎

trjExpensify · 2024-10-17T19:13:07Z

It's a pretty minor language thang this, so I think we can skip the regression test.

Payment summary as follows:

$250 to @allgandalf for the C+ reveiw
$250 to @nkdengineer for the fix

Paid, closing!

m-natarajan added Daily KSv2 Bug Something is broken. Auto assigns a BugZero manager. labels Sep 30, 2024

melvin-bot bot assigned trjExpensify Sep 30, 2024

trjExpensify assigned allgandalf Oct 2, 2024

trjExpensify added the External Added to denote the issue can be worked on by a contributor label Oct 2, 2024

melvin-bot bot changed the title ~~Plural Remove Members on the workspace members page even when we only have one member selected~~ [$250] Plural Remove Members on the workspace members page even when we only have one member selected Oct 2, 2024

melvin-bot bot added the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 2, 2024

melvin-bot bot added the Overdue label Oct 4, 2024

melvin-bot bot added Overdue and removed Overdue labels Oct 5, 2024

melvin-bot bot removed the Overdue label Oct 7, 2024

melvin-bot bot assigned rlinoz Oct 8, 2024

rlinoz assigned nkdengineer Oct 8, 2024

melvin-bot bot removed the Help Wanted Apply this label when an issue is open to proposals by contributors label Oct 8, 2024

nkdengineer mentioned this issue Oct 9, 2024

fix: Plural Remove Members on the workspace when we only have one #50488

Merged

50 tasks

melvin-bot bot added Reviewing Has a PR in review Weekly KSv2 Awaiting Payment Auto-added when associated PR is deployed to production and removed Daily KSv2 Weekly KSv2 labels Oct 9, 2024

melvin-bot bot changed the title ~~[$250] Plural Remove Members on the workspace members page even when we only have one member selected~~ [HOLD for payment 2024-10-17] [$250] Plural Remove Members on the workspace members page even when we only have one member selected Oct 10, 2024

melvin-bot bot removed the Reviewing Has a PR in review label Oct 10, 2024

melvin-bot bot added Daily KSv2 and removed Weekly KSv2 labels Oct 17, 2024

trjExpensify closed this as completed Oct 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[HOLD for payment 2024-10-17] [$250] Plural Remove Members on the workspace members page even when we only have one member selected #49961

[HOLD for payment 2024-10-17] [$250] Plural Remove Members on the workspace members page even when we only have one member selected #49961

m-natarajan commented Sep 30, 2024 •

edited by melvin-bot bot

Loading

melvin-bot bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

Krishna2323 commented Sep 30, 2024 •

edited

Loading

Krishna2323 commented Sep 30, 2024

allgandalf commented Oct 1, 2024

trjExpensify commented Oct 2, 2024

melvin-bot bot commented Oct 2, 2024

melvin-bot bot commented Oct 2, 2024

nkdengineer commented Oct 2, 2024 •

edited by github-actions bot

Loading

nkdengineer commented Oct 2, 2024

allgandalf commented Oct 5, 2024

trjExpensify commented Oct 7, 2024

allgandalf commented Oct 8, 2024

allgandalf commented Oct 8, 2024

melvin-bot bot commented Oct 8, 2024

nkdengineer commented Oct 8, 2024 •

edited

Loading

allgandalf commented Oct 8, 2024 •

edited

Loading

Krishna2323 commented Oct 8, 2024

rlinoz commented Oct 8, 2024

melvin-bot bot commented Oct 8, 2024

melvin-bot bot commented Oct 8, 2024

melvin-bot bot commented Oct 10, 2024

melvin-bot bot commented Oct 10, 2024

melvin-bot bot commented Oct 10, 2024

allgandalf commented Oct 16, 2024

trjExpensify commented Oct 17, 2024

[HOLD for payment 2024-10-17] [$250] Plural Remove Members on the workspace members page even when we only have one member selected #49961

[HOLD for payment 2024-10-17] [$250] Plural Remove Members on the workspace members page even when we only have one member selected #49961

Comments

m-natarajan commented Sep 30, 2024 • edited by melvin-bot bot Loading

Action Performed:

Expected Result:

Actual Result:

Workaround:

Platforms:

Screenshots/Videos

melvin-bot bot commented Sep 30, 2024

github-actions bot commented Sep 30, 2024

Krishna2323 commented Sep 30, 2024 • edited Loading

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)

Krishna2323 commented Sep 30, 2024

PROPOSAL UPDATED

allgandalf commented Oct 1, 2024

trjExpensify commented Oct 2, 2024

melvin-bot bot commented Oct 2, 2024

melvin-bot bot commented Oct 2, 2024

nkdengineer commented Oct 2, 2024 • edited by github-actions bot Loading

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)

nkdengineer commented Oct 2, 2024

allgandalf commented Oct 5, 2024

trjExpensify commented Oct 7, 2024

allgandalf commented Oct 8, 2024

allgandalf commented Oct 8, 2024

melvin-bot bot commented Oct 8, 2024

nkdengineer commented Oct 8, 2024 • edited Loading

allgandalf commented Oct 8, 2024 • edited Loading

Krishna2323 commented Oct 8, 2024

rlinoz commented Oct 8, 2024

melvin-bot bot commented Oct 8, 2024

melvin-bot bot commented Oct 8, 2024

melvin-bot bot commented Oct 10, 2024

melvin-bot bot commented Oct 10, 2024

melvin-bot bot commented Oct 10, 2024

allgandalf commented Oct 16, 2024

Regression Test Proposal

trjExpensify commented Oct 17, 2024

m-natarajan commented Sep 30, 2024 •

edited by melvin-bot bot

Loading

Krishna2323 commented Sep 30, 2024 •

edited

Loading

nkdengineer commented Oct 2, 2024 •

edited by github-actions bot

Loading

nkdengineer commented Oct 8, 2024 •

edited

Loading

allgandalf commented Oct 8, 2024 •

edited

Loading