Conversation
# Conflicts: # src/libs/actions/Policy/Policy.ts # src/pages/workspace/WorkspacesListPage.tsx # src/types/onyx/index.ts
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
melvin-bot
bot
requested review from
abdulrahuman5196
and removed request for
a team
|
@abdulrahuman5196 Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Looks good overall. This is pretty minor, though I think we should put |
|
@DylanDylann are we good here? |
Reviewer Checklist
Screenshots/VideosAndroid: HybridAppAndroid: mWeb ChromeScreen.Recording.2025-08-15.at.11.56.29.moviOS: HybridAppScreen.Recording.2025-08-15.at.12.01.38.moviOS: mWeb SafariScreen.Recording.2025-08-15.at.11.57.58.movMacOS: Chrome / SafariScreen.Recording.2025-08-15.at.11.54.09.movMacOS: DesktopScreen.Recording.2025-08-15.at.11.58.46.mov |
|
@stitesExpensify The additional change looks good to me. I reviewed the regressions and most of them seem to be BE bugs or expectation issues. Since this PR also includes the feature flag and is quite large, I’d prefer to merge it first and then address each regression separately. |
|
@mountiny The avatar is not updated in the duplicated workspace (BE bug) Screen.Recording.2025-09-01.at.19.40.35.mov |
mountiny
left a comment
mountiny
left a comment
There was a problem hiding this comment.
Alright, I think we will have 2 things to tackle after this change:
- use only one read api call to get whatever data you might need to duplicate the policies, we cannot call N read commands, I dont know if you have raised this as an issue in the previous conversations, but remember this for future as it should have been raised
- Handle the backTo routes, we should not be adding new backTo routes now
| WORKSPACE_DUPLICATE: { | ||
| route: 'workspace/:policyID/duplicate', | ||
| getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`workspace/${policyID}/duplicate`, backTo), | ||
| }, | ||
| WORKSPACE_DUPLICATE_SELECT_FEATURES: { | ||
| route: 'workspace/:policyID/duplicate/select-features', | ||
| getRoute: (policyID: string, backTo?: string) => getUrlWithBackToParam(`workspace/${policyID}/duplicate/select-features`, backTo), | ||
| }, |
There was a problem hiding this comment.
@narefyev91, we should not add new routes with backTo. Can you please refactor these flows so that they do not need the backTo param?
I dont want to block this on it, but let's follow up soon after this PR change
| const fetchWorkspaceRelatedData = useCallback(() => { | ||
| if (!policyID) { | ||
| return; | ||
| } | ||
| openWorkspaceMembersPage(policyID, Object.keys(allIds ?? {})); | ||
| openPolicyCategoriesPage(policyID); | ||
| openPolicyDistanceRatesPage(policyID); | ||
| openPolicyPerDiemPage(policyID); | ||
| openPolicyReportFieldsPage(policyID); | ||
| openPolicyTagsPage(policyID); | ||
| openPolicyTaxesPage(policyID); | ||
| openPolicyWorkflowsPage(policyID); | ||
| }, [policyID, allIds]); |
There was a problem hiding this comment.
We cannot do this @narefyev91 We need to call only one API per action.
What data is missing from your testing that you need to call all of the APIs - what about the OpenPolicyProfilePage does that not return what you need?
There was a problem hiding this comment.
@mountiny here are the differences: - left (all apis), right (just OpenPolicyProfilePage)
We missing Tags and Categories
There was a problem hiding this comment.
Ok so it would be enough to call just the tags and categories, right? could you define it on the policy object data what exactly you are missing compared to OpenPolicyProfilePage to ensure the new API loads all you need
There was a problem hiding this comment.
@mountiny yeah - it will be good if we just add categories and tags - but i think we should not add those objects to policy? We should adjust a bit OpenPolicyProfilePage and include in it policyCategories_${policyID}, and policyTags_${policyID}
This what we have now for OpenPolicyProfilePage
This is what we get for Categories API:
This is what we get for Tags API:
There was a problem hiding this comment.
I think we just create a new API command like OpenPolicyDuplicatePage
|
🚀 Deployed to staging by https://github.com/mountiny in version: 9.2.1-0 🚀
|
|
@narefyev91 can you confirm if this PR caused this regression here: #69845 |
yes |
|
🚀 Deployed to production by https://github.com/arosiclair in version: 9.2.1-20 🚀
|
| const [lastPaymentMethod] = useOnyx(ONYXKEYS.NVP_LAST_PAYMENT_METHOD, {canBeMissing: true}); | ||
| const shouldShowLoadingIndicator = isLoadingApp && !isOffline; | ||
| const route = useRoute<PlatformStackRouteProp<AuthScreensParamList, typeof SCREENS.WORKSPACES_LIST>>(); | ||
| const [duplicateWorkspace] = useOnyx(ONYXKEYS.DUPLICATE_WORKSPACE, {canBeMissing: false}); |
There was a problem hiding this comment.
Seeing a console error on homepage: useOnyx returned no data for key with canBeMissing set to false for key duplicateWorkspace.
| ...announceRoomChat.onyxSuccessData, | ||
| ]; | ||
|
|
||
| const failureData: OnyxUpdate[] = [ |
There was a problem hiding this comment.
We haven't handled error state correctly which caused #70290.
| key: `${ONYXKEYS.COLLECTION.POLICY}${targetPolicyID}`, | ||
| value: { | ||
| ...policy, | ||
| areCategoriesEnabled: isCategoriesOptionSelected, |
There was a problem hiding this comment.
We should enable categories when duplicating a workspace, even if the user unchecks them, to match OD behavior
#72608
| : undefined, | ||
| } | ||
| : undefined, | ||
| ratesCount > 0 |
There was a problem hiding this comment.
This condition was not enough. We should also check areDistanceRatesEnabled.
Issue: #74469
|
@narefyev91 What's the point of this 
(Coming from #74650) |
|
@narefyev91 could you plz help with the comment |
It was part of design doc: |
12 participants
Explanation of Change
New feature from OldDot - Duplicate workspace
Fixed Issues
$ #67289
$ #69042
$ #69049
$ #69054
$ #69059
$ #69061
$ #69062
$ #69060
$ #69057
$ #69056
PROPOSAL:
Tests
Preconditions: Be sure that beta for duplicate workspace is enabled
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
android-web.mov
iOS: Native
ios.mov
iOS: mWeb Safari
ios-web.mov
MacOS: Chrome / Safari
web.mov
MacOS: Desktop
desktop.mov