[Simplified Collect][CLEAN UP][LOW] Standardize on Workspace wrapper#40598
Conversation
melvin-bot
bot
requested review from
ahmedGaber93
and removed request for
a team
|
@ahmedGaber93 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] |
Reviewer Checklist
Screenshots/VideosAndroid: Nativea1.mova2.mova3.movAndroid: mWeb Chromeaw1.movaw2.moviOS: Nativei1.movi2.movi3.moviOS: mWeb Safariiw1.moviw2.movMacOS: Chrome / Safariw4.movw1.movw3.movw2.movMacOS: Desktopd1.movd2.movd3.mov |
ahmedGaber93
left a comment
ahmedGaber93
left a comment
There was a problem hiding this comment.
The code works well, Just small suggestions to make the code more readable and simple.
| const POLICY_ACCESS_VARIANTS = { | ||
| PAID: (policy: OnyxEntry<OnyxTypes.Policy>) => !PolicyUtils.isPaidGroupPolicy(policy) || !policy?.isPolicyExpenseChatEnabled, | ||
| ADMIN: (policy: OnyxEntry<OnyxTypes.Policy>) => !PolicyUtils.isPolicyAdmin(policy), | ||
| } as const satisfies Record<string, (policy: OnyxTypes.Policy) => boolean>; |
There was a problem hiding this comment.
The code here is working well, but I think we need to make it more clean, because while the const name is ACCESS, the return method return true when INACCESS not when ACCESS, so I suggest the following change.
// revert the condition to return true when access
PAID: (policy: OnyxEntry<OnyxTypes.Policy>) => PolicyUtils.isPaidGroupPolicy(policy) && !!policy?.isPolicyExpenseChatEnabled,
ADMIN: (policy: OnyxEntry<OnyxTypes.Policy>) => PolicyUtils.isPolicyAdmin(policy),
// and change its usage here.
// const pageUnaccessible = accessVariants.reduce((acc, variant) => {
// const accessFunction = POLICY_ACCESS_VARIANTS[variant];
// return acc || accessFunction(policy);
// }, false);
const isPageAccessible = accessVariants.reduce((acc, variant) => {
const accessFunction = POLICY_ACCESS_VARIANTS[variant];
return acc && accessFunction(policy);
}, true);
// const shouldShowNotFoundPage = ... && pageUnaccessible
const shouldShowNotFoundPage = ... && !isPageAccessibleThere was a problem hiding this comment.
Also, I think using accessVariant name as a const (CONST.POLICY.ACCESS_VARIANTS.PAID) will be more safe than string ('PAID')
const POLICY_ACCESS_VARIANTS = {
[CONST.POLICY.ACCESS_VARIANTS.PAID]: ....,
[CONST.POLICY.ACCESS_VARIANTS.ADMIN]: ....
}
<AccessOrNotFoundWrapper accessVariants={[CONST.POLICY.ACCESS_VARIANTS.PAID]}There was a problem hiding this comment.
- Sure, I'll apply this, it seems more readable
- the idea is that this const is being used only in the context of AccessOrNotFoundWrapper, so splitting its keys to a separate file might be unintuitive, especially since
POLICY_ACCESS_VARIANTSis being used as the type source which would be exported to the other places. I'd suggest keeping it this way until we find a case for usingPOLICY_ACCESS_VARIANTSoutside of this component and then move it as a whole
There was a problem hiding this comment.
the idea is that this const is being used only in the context of AccessOrNotFoundWrapper.
@BrtqKr But it used in many places where AccessOrNotFoundWrapper is used as a prop of it, so I think we need to update it.
Also, this point is a Reviewer Checklist item
[ ] I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
| featureName?: PolicyFeatureName; | ||
| }; | ||
|
|
||
| type PageNotFoundFallackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {showFullScreenFallback: boolean}; |
There was a problem hiding this comment.
| type PageNotFoundFallackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {showFullScreenFallback: boolean}; | |
| type PageNotFoundFallbackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {shouldShowFullScreenFallback: boolean}; |
| function PageNotFoundFallback({policyID, showFullScreenFallback}: PageNotFoundFallackProps) { | ||
| return showFullScreenFallback ? ( | ||
| <FullPageNotFoundView | ||
| shouldShow | ||
| onBackButtonPress={() => Navigation.goBack(ROUTES.SETTINGS_WORKSPACES)} | ||
| shouldForceFullScreen | ||
| /> | ||
| ) : ( | ||
| <NotFoundPage onBackButtonPress={() => Navigation.goBack(ROUTES.WORKSPACE_PROFILE.getRoute(policyID))} /> | ||
| ); | ||
| } |
There was a problem hiding this comment.
I think no need for using both FullPageNotFoundView and NotFoundPage because NotFoundPage is the same as FullPageNotFoundView else if we pass prop shouldForceFullScreen = true.
What do you think about using only FullPageNotFoundView like this?
<FullPageNotFoundView
shouldShow
onBackButtonPress={() => Navigation.goBack(!isFeatureEnabled ? ROUTES.SETTINGS_WORKSPACES : ROUTES.WORKSPACE_PROFILE.getRoute(policyID))}
shouldForceFullScreen={!isFeatureEnabled}
/>There was a problem hiding this comment.
We can also use it inline and remove PageNotFoundFallback component.
There was a problem hiding this comment.
NotFoundPage is inside of a ScreenWrapper, which has a SafeAreaView included. If we replace it with FullPageNotFoundView, it will break the mobile version of the app as shown below. It's been done this way to preserve the logic present in the previous version.
I think we should take the wider approach and unify them both in a separate ticket.
|
Bug: NoFoundPage appear briefly then loading, then back to appear again when trying to open not found workspace, for example go to Screen.Recording.2024-04-23.at.8.59.10.AM.mov |
|
We need to edit tests steps to be more clear. For admin case:
For paid case:
For feature flag case:
also, please add some Screenshots/Videos, and fill Offline tests QA Steps, if it the same as Tests add it `Same as "Tests"` |
Yeah, it's also present on the main branch, so let's create a ticket for that |
|
Reviewing now. |
| featureName?: PolicyFeatureName; | ||
| }; | ||
|
|
||
| type PageNotFoundFallackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {shouldShowFullScreenFallback: boolean}; |
There was a problem hiding this comment.
| type PageNotFoundFallackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {shouldShowFullScreenFallback: boolean}; | |
| type PageNotFoundFallbackProps = Pick<AccessOrNotFoundWrapperProps, 'policyID'> & {shouldShowFullScreenFallback: boolean}; |
|
Tested well! And record videos for all cases. |
|
We have 2 issues from the previous code before this Standardize, we need to open new issues for it. issue 1Bug: NoFoundPage appear briefly then loading, then back to appear again when trying to open not found workspace, for example go to Screen.Recording.2024-04-23.at.8.59.10.AM.movissue 2Native - back button overlapping with stausbar when trying to access page without feature enable, and the not found page is displayed. issue1.mov |
luacmartins
left a comment
luacmartins
left a comment
There was a problem hiding this comment.
Nice! Thanks for cleaning this up. Just noting that we're deprecating all Free policies soon, so we'll need to update the access type to remove the Paid type.
|
@BrtqKr conflicts |
|
@BrtqKr friendly bump to fix conflicts :) |
Done 🙏 , but I had to adjust the wrapper to handle further customization. Let's get this reviewed and merged before we get more conflicts since this PR is modifying many parts of the app |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 1.4.67-7 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
@BrtqKr @ahmedGaber93 @luacmartins Could you help us how to validate in native apps and mweb?
|
|
🚀 Deployed to staging by https://github.com/luacmartins in version: 1.4.68-0 🚀
|
|
🚀 Cherry-picked to staging by https://github.com/francoisl in version: 1.4.67-7 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
|
@kavimuru Sorry for unclear that. mWeb/Native apps.We can test is page access or not by links in mWeb and deeplinks in Native apps. For admin case: try to open admin feature (categories) for any workspace you don't have admin role for it.Precondition: user is Admin in workspace A and member in workspace B.
For paid case: try to open paid feature (categories) for any free workspace.Precondition: user is Admin in paid and free Workspaces.
For feature flag case: disable any feature (categories) in a workspace, try to open this feature by URL.Precondition: user is Admin in paid workspace.
@BrtqKr can you link this comment in tests step, or add it manually for mWeb/Native apps. |
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.4.68-3 🚀
|
| testID={WorkspaceCreateTaxPage.displayName} | ||
| includeSafeAreaPaddingBottom={false} | ||
| style={[styles.defaultModalContainer]} | ||
| <AccessOrNotFoundWrapper |
| return acc && accessFunction(policy); | ||
| }, true); | ||
|
|
||
| const shouldShowNotFoundPage = isEmptyObject(policy) || (Object.keys(policy).length === 1 && !isEmptyObject(policy.errors)) || !policy?.id || !isPageAccessible || !isFeatureEnabled; |
There was a problem hiding this comment.
Coming from #44206:
Please be careful when fix conflict.
Recent code in FeatureEnabledAccessOrWrapper was not applied to this file so the fix of #39443 was partially reverted.
More details: #44206 (comment)
|
Coming from #54582 |
| <SelectionList | ||
| sections={sections} | ||
| textInputLabel={translate('optionsSelector.findMember')} | ||
| textInputValue={searchTerm} | ||
| onChangeText={setSearchTerm} | ||
| headerMessage={headerMessage} | ||
| ListItem={UserListItem} | ||
| onSelectRow={setPolicyAuthorizedPayer} | ||
| showScrollIndicator | ||
| /> |
There was a problem hiding this comment.
this caused
we should set initial focus key.
10 participants
Details
Fixed Issues
$ #37898
PROPOSAL:
Tests
That's a very broad cleanup, so to verify that nothing is broken would require:
For admin case:
For paid case:
For feature flag case:
Offline tests
Same as tests
QA Steps
Same as tests
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.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
Screen.Recording.2024-04-25.at.15.05.25.mov
Android: mWeb Chrome
Screen.Recording.2024-04-25.at.15.12.00.mov
iOS: Native
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-25.at.12.59.44.mp4
iOS: mWeb Safari
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-04-25.at.13.31.50.mp4
MacOS: Chrome / Safari
Screen.Recording.2024-04-24.at.14.04.05.mov
Screen.Recording.2024-04-24.at.14.10.12.mov
MacOS: Desktop
Screen.Recording.2024-04-24.at.15.14.17.mov