-
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
[Free trial] Restrict actions throughout the App of "past due" billing owner #44393
[Free trial] Restrict actions throughout the App of "past due" billing owner #44393
Conversation
…ut not the workspace owner
@fabioh8010 There are merge conflicts here to be resolved. I will continue to review though. |
@rojiphil Conflicts resolved |
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.
@fabioh8010 Thanks for the PR. Code looks good and tests well too.
But I also have left few comments. Please have a look.
@@ -39,7 +38,7 @@ function WorkspaceUserRestrictedAction({policyID}: WorkspaceUserRestrictedAction | |||
> | |||
<HeaderWithBackButton | |||
title={translate('workspace.restrictedAction.restricted')} | |||
onBackButtonPress={Navigation.goBack} | |||
onBackButtonPress={Navigation.resetToHome} |
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.
On returning from the Restricted Actions view for users, why do we need to reset the navigation history? Seems too harsh on the user for no fault of his.
Personally, I would not recommend to reset the navigation history even for Admins and Owner. Also, we reset the navigation history when we guide the user/admin/owner to the appropriate report.
Also, for the scenario of manual/split request, I think the user would expect to go back to the participant selection page instead of abruptly ending the request. no?
Is this all intentional/agreed upon somewhere? Maybe I am missing something here.
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.
If I keep the previous behavior this is what happens on mobile for example:
Screen.Recording.2024-06-28.at.14.22.20-compressed.mov
FAB -> Submit expense -> Restricted Action -> Workspace chat
If I go back on the workspace chat I will return to the restricted action, which i don't think it makes much sense. I'm not familiar with the navigation system so that's why I'm resetting like this, do you have any thoughts about the flow with goBack vs resetToHome?
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 the comment here may solve this problem.
src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.tsx
Outdated
Show resolved
Hide resolved
src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.tsx
Show resolved
Hide resolved
src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.tsx
Outdated
Show resolved
Hide resolved
src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.tsx
Outdated
Show resolved
Hide resolved
@chiragsalian I notice that we do not restrict 44393-submit-issue.mp4 |
Reviewer Checklist
Screenshots/VideosMacOS: Chrome / Safari44393-web-safari-all-002.mp4MacOS: Desktop44393-desktop-admin-001.mp4Android: Native44393-android-native-admin-001.mp444393-android-native-owner-001.mp444393-android-native-user-001.mp4Android: mWeb Chrome44390-mweb-chrome-001.mp4iOS: Native44393-ios-native-admin-002.mp444393-ios-native-owner-001.mp4iOS: mWeb Safari44393-mweb-safari-user-001.mp4 |
From the design doc, these are the actions that we'll have to restrict,
But i see fabio included these in his test steps. I see he tests for submitting a request. Did you test the same steps and they did not work for you or did you mean something else? I'm wondering if there was some edge case you might have noticed. In your video was that the workspace owner or admin? |
@chiragsalian Hmm... Looks like we use the same term to describe two actions which is causing confusion. We use the same term while, a) Initiating a |
@fabioh8010 I was unable to test the |
I will wait on this discussion as I don't really know if this case should be covered by the restriction.
@rojiphil I'm not sure if I understood, could you elaborate more? Did you try my test steps? |
Yeah i think we should add a restriction for submitting an expense via a report action as well. Basically submitting an expense anywhere should be restricted. @MitchExpensify can you double check and confirm as well. |
@@ -28,7 +27,7 @@ function WorkspaceUserRestrictedAction({policyID}: WorkspaceUserRestrictedAction | |||
|
|||
const openPolicyExpenseReport = useCallback(() => { | |||
const reportID = ReportUtils.findPolicyExpenseChatByPolicyID(policyID)?.reportID ?? '-1'; | |||
Report.openReport(reportID); | |||
// Navigation.resetToHome(); |
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.
Got it. I think the following may solve our problem. Can you also please test this out?
// Navigation.resetToHome(); | |
Navigation.closeRHPFlow(); |
44393-back-issue.mp4
src/pages/home/report/ReportActionCompose/AttachmentPickerWithMenuItems.tsx
Show resolved
Hide resolved
Yes, they should be restricted from submitting an expense report everywhere, so via a report action as well. 👍 |
@rojiphil Addressed the comments about the navigation and tested, looks better I think, please have a look! @rojiphil @chiragsalian @MitchExpensify @trjExpensify Also addressed the submit action and now we are restricting it too. |
This comment was marked as outdated.
This comment was marked as outdated.
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.
@fabioh8010 Thanks for the changes. Code looks good and tests well too. We are almost there for PR approval. And the checklist is also almost done.
- Pending work is to resolve the merge conflicts. Once resolved, will verify the outcome again.
- Please add the test steps for
submitting
expense via report action.
Specific to your test videos, I am referring to the workspace equivalent to And specific to the test steps, I am unable to test scenarios on a workspace i.e. |
@rojiphil I resolved the conflicts and updated test steps, please have another look. Regarding the Workspace Owner tests, please follow the instructions I put and try using this code instead. useEffect(() => {
Onyx.multiSet({
[ONYXKEYS.NVP_PRIVATE_OWNER_BILLING_GRACE_PERIOD_END]: 1719818912,
[ONYXKEYS.NVP_PRIVATE_AMOUNT_OWED]: 1000,
});
}, []); Also, please note that we've renamed |
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 @fabioh8010 . Tests well and completed checklist.
I still could not get the workspace chat
for workspace owner
though. But I think that’s fine since I have tested the workspace chat
for workspace admin
.
@chiragsalian. All yours.
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.
Code LGTM.
✋ 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/chiragsalian in version: 9.0.4-0 🚀
|
🚀 Cherry-picked to staging by https://github.com/tgolen in version: 9.0.4-5 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
@fabioh8010 Where do we run the scripts and how to run this step
|
@kavimuru You must add the function Expensify({
isCheckingPublicRoom = true,
session,
updateAvailable,
isSidebarLoaded = false,
screenShareRequest,
updateRequired = false,
focusModeNotification = false,
lastVisitedPath,
}: ExpensifyProps) {
...
// This is being done since we want to play sound even when iOS device is on silent mode, to align with other platforms.
useEffect(() => {
Audio.setAudioModeAsync({playsInSilentModeIOS: true});
}, []);
// Add the useEffect() here. <----------------------------------------
// Display a blank page until the onyx migration completes
if (!isOnyxMigrated) {
return null;
}
if (updateRequired) {
throw new Error(CONST.ERROR.UPDATE_REQUIRED);
}
return (
...
} |
@trjExpensify @MitchExpensify @fabioh8010 Do we run these scripts in the console? Could you help us? |
This comment was marked as off-topic.
This comment was marked as off-topic.
No. These changes are in the code. @fabioh8010 We have to provide Onyx updates for the QA to test this via Inspect Console. |
Oops yeah i missed this in test steps.
To reset Onyx data,
Workspace Owner, prerequisites,
|
@chiragsalian QA team is finding difficulty validating this PR. It would be great if this PR could be validated internally. Thanks! |
Sure, @fabioh8010 or @rojiphil would you guys be able to test this on staging? If not let me know and I'll test it out. |
Works well on staging. 44393-web-safari-staging-01.mp444393-mweb-chrome-staging-01.mp4 |
🚀 Deployed to production by https://github.com/Julesssss in version: 9.0.5-13 🚀
|
🚀 Deployed to production by https://github.com/thienlnam in version: 9.0.6-8 🚀
|
Details
This PR implements the restrict actions logic for the user, admin and owner against a "past due" billing workspace. (Design doc)
Fixed Issues
$ #43674
PROPOSAL:
Tests
Workspace User
Prerequisites
Expensify.tsx
in order to set the Onyx keys during initialisation, where<ownerAccountID>
is the account ID of the workspace's owner, and<timestamp>
is your current date in Unix timestamp minus some days to simulate that the owner is past due billing (Use https://www.unixtimestamp.com/ to calculate it).Test steps
Submitting an expense on a Workspace
Tracking an expense on a Workspace
Splitting an expense on a Workspace
Use the following code to reset Onyx data.
Workspace Admin
Prerequisites
Same as Workspace User prerequisites, except that now the user must be a admin of the workspace you created with another account.
Test steps
Submitting an expense on a Workspace
Tracking an expense on a Workspace
Splitting an expense on a Workspace
Use the following code to reset Onyx data.
Workspace Owner
Prerequisites
Manually
).Expensify.tsx
in order to set the Onyx keys during initialisation, where<timestamp>
is your current date in Unix timestamp minus some days to simulate that the owner is past due billing (Use https://www.unixtimestamp.com/ to calculate it).Test steps
Submitting an expense on a Workspace
Tracking an expense on a Workspace
Approving an expense on a Workspace
Paying an expense on a Workspace
Splitting an expense on a Workspace
Use the following code to reset Onyx data.
Offline tests
Same as above.
QA Steps
Same as above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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: mWeb Chrome
Screen.Recording.2024-06-26.at.19.08.24-compressed.mov
iOS: Native
Screen.Recording.2024-06-26.at.19.37.24-compressed.mov
iOS: mWeb Safari
Screen.Recording.2024-06-26.at.19.44.39-compressed.mov
MacOS: Chrome / Safari
Screen.Recording.2024-06-26.at.19.48.14-compressed.mov
Screen.Recording.2024-06-26.at.19.51.31-compressed.mov
MacOS: Desktop
Screen.Recording.2024-06-26.at.20.51.13-compressed.mov