-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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-04-15] [$500] Room - WS is not shown empty on setting collect WS as default. #37656
Comments
Triggered auto assignment to @MitchExpensify ( |
@MitchExpensify 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 |
ProposalPlease re-state the problem that we are trying to solve in this issue.WS is not shown empty on setting collect WS as default What is the root cause of that problem?In App/src/pages/workspace/WorkspaceNewRoomPage.tsx Lines 128 to 140 in 22cb01c
And this further sets the default value over here: App/src/pages/workspace/WorkspaceNewRoomPage.tsx Lines 287 to 294 in 22cb01c
What changes do you think we should make in order to solve the problem?In Essentially we would set const [policyID, setPolicyID] = useState<string>(() => {
if (!!activePolicyID && workspaceOptions.some((option) => (option.value === activePolicyID && option.type !== CONST.POLICY.TYPE.TEAM))) {
return activePolicyID;
}
return '';
});
useEffect(() => {
.....
if (!!activePolicyID && workspaceOptions.some((opt) => (opt.value === activePolicyID && opt.type !== CONST.POLICY.TYPE.TEAM))) {
setPolicyID(activePolicyID);
}
The type property is inherited from App/src/pages/workspace/WorkspaceNewRoomPage.tsx Lines 73 to 83 in 22cb01c
Result:simplescreenrecorder-2024-03-03_05.58.41.mp4 |
I'm not sure why we'd want the Workspace field to show be blank here on the room creation screen. I'd expect us to show the Collect default workspace in the workspace field which seems to be what is happening - Rigth @lanitochka17 ? |
ProposalPlease re-state the problem that we are trying to solve in this issue.Room - WS is not shown empty on setting collect WS as default. What is the root cause of that problem?I believe the current behavior is partially correct. When a user has set a default workspace, it should be selected as the default in the room creation flow. However, the issue arises when the user switches to a different workspace in NewDot from the workspace switcher. When the user creates a room, the default workspace remains the default from the OldDot. In my opinion, for better user experience, we should select the newly selected workspace from NewDot. Bug Videodefault_workspace_bug.mp4What changes do you think we should make in order to solve the problem?Instead of checking for We will replace Resultdefault_ws.mp4AlternativeIf we want to always show empty default WS we can remove code blocks below: App/src/pages/workspace/WorkspaceNewRoomPage.tsx Lines 85 to 87 in 4231ac9
App/src/pages/workspace/WorkspaceNewRoomPage.tsx Lines 135 to 137 in 4231ac9
|
@MitchExpensify, can you please provide some feedback on this? Thanks
|
Yeah this is an interesting question @Krishna2323 - Should the workspace you are filtering by in NewDot be shown as the default workspace when creating a room? I think the answer is "yes" and I'll double-check internally that others agree! |
@MitchExpensify, any updates? |
@MitchExpensify I think collect workspaces are still not show by default in ND, we need to run a script when we create new collect policy in OD for it to show in ND, so I think not relying on ND workspace is a safe option (not 100% sure about this, but just thought this out loud :) )
We are updating ND to create collect workspaces by default, with this change, we give the user 7 day free trail, when it is over then we don't want the user to create a room with the expired workspace, and hence i think the expected behavior is mentioned in the issue |
Double checking what the team thinks! |
@MitchExpensify, do we have an update here? |
@MitchExpensify Whoops! This issue is 2 days overdue. Let's get this updated quick! |
Yes! The expected behavior is to show the Workspace you are currently filtering by as the suggested Workspace when creating a room. Updated the expected behavior: Expected Result:The Workspace you are currently filtering by should be shown as the top suggested option when creating a room. If filtering by all, your default workspace should be shown as the top suggested option when creating a room. |
Job added to Upwork: https://www.upwork.com/jobs/~014b390ee1a3dc9cad |
Triggered auto assignment to Contributor-plus team member for initial proposal review - @ishpaul777 ( |
@MitchExpensify @ishpaul777 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks! |
Thanks @MitchExpensify I will review proposals today |
Thanks for proposals, everyone. I like @Krishna2323's proposal here. He coordinated with Mitch to come up with the right expectations. He also provided a correct RCA and solution. 🎀👀🎀 C+ reviewed |
Triggered auto assignment to @AndrewGable, see https://stackoverflow.com/c/expensify/questions/7972 for more details. |
@hoangzinh, PR ready for review. |
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.60-13 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-04-15. 🎊 For reference, here are some details about the assignees on this issue:
|
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:
|
Payment summary:
|
Friendly bump on your regression step suggestion @hoangzinh 🙇 Offers sent for payment on this Job https://www.upwork.com/jobs/~014b390ee1a3dc9cad |
@MitchExpensify thanks, accepted 😁 |
Payment Summary
BugZero Checklist (@MitchExpensify)
|
paid, thanks @Krishna2323 ! |
Regression Test ProposalPre-condition: User must have a default workspace and some other workspaces in OldDot
Do we agree 👍 or 👎 |
BugZero Checklist:
|
@MitchExpensify could you process payment for me in upwork? Thanks |
Done! |
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.46
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4359738
Email or phone of affected tester (no customers):
Logs: https://stackoverflow.com/c/expensify/questions/4856
Expensify/Expensify Issue URL:
Issue reported by: Applause - Internal Team
Slack conversation:
Action Performed:
Pre-condition: User A in old dot must have a collect workspace
Expected Result:
The Workspace you are currently filtering by should be shown as the top suggested option when creating a room. If filtering by all, your default workspace should be shown as the top suggested option when creating a room.
Actual Result:
Workspace is not shown empty on setting personal Workspace as default
Workaround:
Unknown
Platforms:
Which of our officially supported platforms is this issue occurring on?
Screenshots/Videos
Add any screenshot/video evidence
Bug6399190_1709557892248.changee.mp4
View all open jobs on GitHub
The text was updated successfully, but these errors were encountered: