Repository navigation
[C+ Checklist Needs Completion] [$250] Send invoice - Workspaces in sender list are not arranged in alphabetical order #41340
Description
Activity
Triggered auto assignment to @AndrewGable (
DeployBlockerCash), see https://stackoverflowteams.com/c/expensify/questions/9980/ for more details.👋 Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open `StagingDeployCash` deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:
- Identify the pull request that introduced this issue and revert it.
- Find someone who can quickly fix the issue.
- Fix the issue yourself.
@AndrewGable 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
#vip-billpay related. Definitely not a blocker, not even sure if this is a bug, cc @danielrvidal
- added and removed
on Apr 30, 2024 Proposal
Please re-state the problem that we are trying to solve in this issue.
Workspaces are not sorted alphabetically in send invoice flow
What is the root cause of that problem?
We do not sort the list of workspace we directly return it:
App/src/pages/iou/request/step/IOURequestStepSendFrom.tsx
Lines 44 to 56 in 35772b2
What changes do you think we should make in order to solve the problem?
We need to sort the workspace while returning it using
.sort((a, b) => localeCompare(a.title, b.title));--- a/src/pages/iou/request/step/IOURequestStepSendFrom.tsx +++ b/src/pages/iou/request/step/IOURequestStepSendFrom.tsx @@ -54,7 +54,7 @@ function IOURequestStepSendFrom({route, transaction, allPolicies}: IOURequestSte }, ], isSelected: selectedWorkspace?.policyID === policy.id, - })); + })).sort((a, b) => a.text.localeCompare(b.text)); // Sort the workspaceOptions array by workspace name }, [allPolicies, selectedWorkspace]);
What alternative solutions did you explore? (Optional)
Proposal
Please re-state the problem that we are trying to solve in this issue.
Send invoice - Workspaces in sender list are not arranged in alphabetical order
What is the root cause of that problem?
We are not sorting the
activePoliciesbefore returning it fromgetActiveAdminWorkspaces.
Lines 382 to 386 in 35772b2
What changes do you think we should make in order to solve the problem?
We should sort the policies in
getActiveAdminWorkspaces, this will make sure that we always get sorted workspaces whenever we usegetActiveAdminWorkspaces.function getActiveAdminWorkspaces(policies: OnyxCollection<Policy>): Policy[] { const activePolicies = getActivePolicies(policies); return activePolicies.filter((policy) => shouldShowPolicy(policy, NetworkStore.isOffline()) && isPolicyAdmin(policy)).sort((a, b) => a.name.localeCompare(b.name)); }
What alternative solutions did you explore? (Optional)
Proposal
Please re-state the problem that we are trying to solve in this issue.
Workspaces in sender list are not arranged in alphabetical order.
What is the root cause of that problem?
We don't have any sorting logic in
workspaceOptions:App/src/pages/iou/request/step/IOURequestStepSendFrom.tsx
Lines 42 to 58 in 806a9f2
What changes do you think we should make in order to solve the problem?
Selected workspace should be shown at the top and others should be in alphabetical order after that.
Updated logic in
workspaceOptions:const workspaceOptions: WorkspaceListItem[] = useMemo(() => { const activeAdminWorkspaces = PolicyUtils.getActiveAdminWorkspaces(allPolicies); return activeAdminWorkspaces .sort((policy1, policy2) => sortWorkspacesBySelected(policy1, policy2, selectedWorkspace.policyID)) .map((policy) => ({ text: policy.name, value: policy.id, keyForList: policy.id, icons: [ { source: policy?.avatar ? policy.avatar : ReportUtils.getDefaultWorkspaceAvatar(policy.name), fallbackIcon: Expensicons.FallbackWorkspaceAvatar, name: policy.name, type: CONST.ICON_TYPE_WORKSPACE, }, ], isSelected: selectedWorkspace?.policyID === policy.id, })); }, [allPolicies, selectedWorkspace]);sortWorkspacesBySelectedlogic is also used in workspace switcher, we can move it to a common place and use the same method at both places (might need some modification in the signature becauseWorkspaceSwitcherPageusesWorkspaceListItem).const sortWorkspacesBySelected = (workspace1, workspace2, selectedWorkspaceID: string | undefined): number => { if (workspace1.id === selectedWorkspaceID) { return -1; } if (workspace2.id === selectedWorkspaceID) { return 1; } return (workspace1.name?.toLowerCase().localeCompare(workspace2.name?.toLowerCase() ?? '')) ?? 0; };What alternative options did you explore?
We can also apply this sorting logic in
getActiveAdminWorkspaces, we'll need to pass the selected workspace ID to that method as an optional parameter then.function getActiveAdminWorkspaces(policies: OnyxCollection<Policy>, selectedWorkspaceID?: string): Policy[] { const activePolicies = getActivePolicies(policies); return activePolicies .filter((policy) => shouldShowPolicy(policy, NetworkStore.isOffline()) && isPolicyAdmin(policy)) .sort((policy1, policy2) => sortWorkspacesBySelected(policy1, policy2, selectedWorkspaceID)); }36 remaining items
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.79-11 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-06-13. 🎊
For reference, here are some details about the assignees on this issue:
- @akinwale requires payment automatic offer (Reviewer)
- @ShridharGoel requires payment automatic offer (Contributor)
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:
- [@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:
- [@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.
- [@greg-schroeder] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:
Processing
Payment summary:
Contributor: @ShridharGoel - $250 - paid via Upwork
C+: @akinwale - $250 - paid via Upwork@akinwale can you take care of the checklist so we can close this out? Thanks!
- changed the title
[-][HOLD for payment 2024-06-13] [$250] Send invoice - Workspaces in sender list are not arranged in alphabetical order[/-][+][C+ Checklist Needs Completion] [$250] Send invoice - Workspaces in sender list are not arranged in alphabetical order[/+]on Jun 13, 2024 - [@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 exactly a regression. This is more of a feature which wasn't specifically implemented.
- [@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
- Launch the Expensify app.
- Click on the global create FAB and select Send Invoice.
- Specify an amount and select a participant.
- Click on the workspace on the confirmation page.
- Verify that the currently selected workspace is at the top of the list.
- Verify that the other workspaces below the selected workspace are arranged in alphabetical order.
Do we agree 👍 or 👎?
@akinwale can you take care of the checklist so we can close this out? Thanks!
@greg-schroeder Done!
Thanks! filing regression test and closing
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.68-0
Reproducible in staging?: Y
Reproducible in production?: N
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:
Precondition:
Applause's Workspace -> C
Applause's Workspace 1 -> A
Applause's Workspace 2 -> B
Expected Result:
The workspaces will be arranged in alphabetical order
Actual Result:
The workspaces will be arranged in alphabetical order
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6466686_1714487611680.bandicam_2024-04-30_22-29-55-385.mp4
View all open jobs on GitHub
Upwork Automation - Do Not Edit
Issue Owner
Current Issue Owner: @greg-schroeder