-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
feat: Provide education/confirmation before creating workspaces in New Workspace flows #53845
feat: Provide education/confirmation before creating workspaces in New Workspace flows #53845
Conversation
…space flows. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
…space flows. Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@allgandalf, the regression issues are fixed but I'm facing a new error, can you please take a look? Monosnap.screencast.2024-12-11.13-55-01.mp4 |
Can you merge main and retry ? is this constantly reproducible ? |
I'm still able to reproduce this issue even after merging main. I'm investigating the RCA of the issue. @allgandalf please let me know if you know anything about this, this is probably related to how we are updating the onyx value. It only occurs when creating a second workspace with an avatar. I'm unsure if this is a backend or frontend bug. |
Can you put out reproduction steps please |
Reproduction Steps:
|
I can also reproduce the issue. You can see it here: |
src/libs/actions/Policy/Policy.ts
Outdated
@@ -1891,6 +1910,8 @@ function buildPolicyData(policyOwnerEmail = '', makeMeAdmin = false, policyName | |||
customUnitID, | |||
customUnitRateID, | |||
engagementChoice, | |||
currency: outputCurrency, | |||
file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file, | |
file: {...file}, |
Please soft clone it to remove non-indexable errors.
src/libs/actions/Policy/Policy.ts
Outdated
@@ -2007,6 +2038,8 @@ function createDraftWorkspace(policyOwnerEmail = '', makeMeAdmin = false, policy | |||
expenseCreatedReportActionID, | |||
customUnitID, | |||
customUnitRateID, | |||
currency: outputCurrency, | |||
file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file, | |
file: {...file}, |
Please soft clone it to remove non-indexable errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @shubham1206agra 🙇🏻, checking...
@Krishna2323 Can you ready this PR with fixes? I need this PR merged today. So, I can work on another feature which requires this screen. |
@@ -3723,7 +3723,7 @@ const translations = { | |||
}, | |||
emptyWorkspace: { | |||
title: 'Create a workspace', | |||
subtitle: 'Create a workspace to track receipts, reimburse expenses, send invoices, and more -- all at the speed of chat.', | |||
subtitle: 'Create a workspace to track receipts, reimburse expenses, send invoices, and more — all at the speed of chat.', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
subtitle: 'Create a workspace to track receipts, reimburse expenses, send invoices, and more — all at the speed of chat.', | |
subtitle: 'Create a workspace to track receipts, reimburse expenses, manage travel, send invoices, and more — all at the speed of chat.', |
Update the copy as per #53753 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this update can be made in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shubham1206agra can you make sure you include this in the above PR
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@allgandalf, you can review the code, I will add the screenshot after few hours. |
all good, please take a look at the failing test as well |
Not sure why the BE is not returning the correct avatar. Will check again. |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@allgandalf, are you still able to reproduce this issue? I can’t seem to reproduce it anymore. Monosnap.screencast.2025-01-15.20-50-57.mp4 |
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
Yes, can reproduce this constantly Screen.Recording.2025-01-16.at.8.11.34.PM.mov |
@Krishna2323 did you investigate what Francois suggested here?
|
Signed-off-by: krishna2323 <belivethatkg@gmail.com>
@allgandalf, have you pulled the latest on this branch? Its seems fixed for me: Monosnap.screencast.2025-01-17.05-16-19.mp4
I did and it was the file which had arrayBuffer(). |
Sorry , but can you explain what exactly is wrong technically? some code reference can also help
Not fixed, this is on your latest branch: Screen.Recording.2025-01-17.at.12.12.39.PM.mov |
@allgandalf, in |
@allgandalf, I'm not sure what to do here since the bug isn't related to our PR, and I have already invested a lot of time in this. I don't have the bandwidth to investigate further. cc: @grgia |
I believe Georgia is OOO today. @cristipaval are you able to help out here? |
chatted 1:1 with @shubham1206agra and we think to fix this holistically as follow up part of #54358 and this issue #47150 Just a note to whoever reviews this PR: that we will observe the same bug on staging/prodcution, so leaving it upto the internal engineer about merging this PR as is , happy to discuss more about this thing on slack |
I just tested this locally, and it doesn't reproduce for me either. I also think the bug is not related to this PR, and given that it is already being investigated, I don't think we should duplicate efforts here. |
Thanks @allgandalf for the rigorous testing and @Krishna2323 for investigating 🙏 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see @grgia is still ooo and we really need this one to get the travel issue unblocked as we're approaching the external commit. I'll merge it to get it in today's deploy.
Awesome, thanks @cristipaval - super excited to finally get this one into the product! |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/cristipaval in version: 9.0.88-0 🚀
|
🚀 Deployed to production by https://github.com/puneetlath in version: 9.0.88-7 🚀
|
Explanation of Change
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] Workspace - Unable to save new workspace name #53765
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] Create workspace - Selected currecy does not appear selected with a green checkmark #53766
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] Create workspace - Edit avatar modal is blocking the avatar #53768
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] Create workspace - Error message does not have left and right padding #53771
$ [HOLD for payment 2024-12-19] [HOLD for payment 2024-12-17] FAB - Page crashes after tracking an expense from QAB #53793
Fixed Issues
$ #51504
PROPOSAL: #51504 (comment)
Tests
Create workspace
Create workspace
> Enter any custom nameNew workspace
> Verify RHP is opened with avatar selector, name input and currency selector.New Workspace
buttonOffline tests
Create workspace
Create workspace
> Enter any custom nameNew workspace
> Verify RHP is opened with avatar selector, name input and currency selector.New Workspace
buttonQA Steps
Create workspace
Create workspace
> Enter any custom nameNew workspace
> Verify RHP is opened with avatar selector, name input and currency selector.New Workspace
buttonPR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
android_native.mp4
Android: mWeb Chrome
android_chrome.mp4
iOS: Native
ios_native.mp4
iOS: mWeb Safari
ios_safari.mp4
MacOS: Chrome / Safari
web_chrome.mp4
MacOS: Desktop
desktop_app.mp4