Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix/28098: Clicking back button bring back the workspace #28573

Merged
merged 10 commits into from
Oct 30, 2023
Merged

Fix/28098: Clicking back button bring back the workspace #28573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
a27b365
fix clicking back button bring back the workspace
DylanDylann Oct 2, 2023
a803a71
fix refactor logic then
DylanDylann Oct 2, 2023
4fe4ff0
fix handle case create workspace without openning modal
DylanDylann Oct 2, 2023
ddb0f4c
Merge branch 'Expensify:main' into fix/28098-clicking-back-button-bri…
DylanDylann Oct 4, 2023
392e320
fix wait for browser history load url after navigate
DylanDylann Oct 6, 2023
fd3300d
Merge branch 'Expensify:main' into fix/28098-clicking-back-button-bri…
DylanDylann Oct 20, 2023
a88dabc
fix update the latest change in staging
DylanDylann Oct 23, 2023
54a01bb
fix on press create new workspace
DylanDylann Oct 23, 2023
cd3261a
fix remove createWorkspaceAndNavigateToIt
DylanDylann Oct 23, 2023
2d732e2
Merge branch 'main' into fix/28098-clicking-back-button-bring-back-th…
DylanDylann Oct 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/libs/Navigation/AppNavigator/AuthScreens.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,9 @@ function AuthScreens({isUsingMemoryOnlyKeys, lastUpdateIDAppliedToClient, sessio
} else {
App.reconnectApp(lastUpdateIDAppliedToClient);
}
App.setUpPoliciesAndNavigate(session, !isSmallScreenWidth);

App.setUpPoliciesAndNavigate(session);

App.redirectThirdPartyDesktopSignIn();

// Check if we should be running any demos immediately after signing in.
Expand Down
39 changes: 5 additions & 34 deletions src/libs/actions/App.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,44 +322,17 @@ function endSignOnTransition() {
return resolveSignOnTransitionToFinishPromise();
}

/**
* Create a new workspace and navigate to it
*
* @param {String} [policyOwnerEmail] Optional, the email of the account to make the owner of the policy
* @param {Boolean} [makeMeAdmin] Optional, leave the calling account as an admin on the policy
* @param {String} [policyName] Optional, custom policy name we will use for created workspace
* @param {Boolean} [transitionFromOldDot] Optional, if the user is transitioning from old dot
* @param {Boolean} [shouldNavigateToAdminChat] Optional, navigate to the #admin room after creation
*/
function createWorkspaceAndNavigateToIt(policyOwnerEmail = '', makeMeAdmin = false, policyName = '', transitionFromOldDot = false, shouldNavigateToAdminChat = true) {
const policyID = Policy.generatePolicyID();
const adminsChatReportID = Policy.createWorkspace(policyOwnerEmail, makeMeAdmin, policyName, policyID);
Navigation.isNavigationReady()
.then(() => {
if (transitionFromOldDot) {
// We must call goBack() to remove the /transition route from history
Navigation.goBack(ROUTES.HOME);
}

if (shouldNavigateToAdminChat) {
Navigation.navigate(ROUTES.REPORT_WITH_ID.getRoute(adminsChatReportID));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DylanDylann Why did we remove this line? on staging when we create a new workspace , it should be redirected to admins room.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The dismissModal should redirect to #admins room but that doesn’t seem to be the case, can you please check?

Copy link
Contributor Author

@DylanDylann DylanDylann Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I replace it by Navigation.dismissModal(adminsChatReportID);, it still redirects to the admins room, and removes the current RHN modal as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the proposal #28098 (comment):

we can make sure that the current RIGHT_MODAL_NAVIGATOR screen is REPLACE by report screen, rather than PUSH a new report screen to the stack by using Navigation.dismissModal function:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it still redirects to the admins room, and removes the current RHN modal as well

It doesn’t work for me.

Can we just dismiss the modal , navigate to admins then open the modal again?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the browsers history is just [/r/{reportID}, /settings, /settings/workspaces]`

That's the expected behavior, no? I mean what's the issue here?

Copy link
Contributor Author

@DylanDylann DylanDylann Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or sorry. My bad. I have updated this comment #28573 (comment)

the browser`s history is just [/r/{reportID}, /settings/workspaces]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedirjh do you have any suggestions for this one? The issue is that window.history does not sync with the react-navigation stacks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fedirjh based on the issue here #28573 (comment), I just updated the PR to fix the error mentioned here #28573 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result

#28573 (comment)

Screencast.from.06-10-2023.22.40.21.webm
Screencast.from.06-10-2023.22.39.00.webm

}

Navigation.navigate(ROUTES.WORKSPACE_INITIAL.getRoute(policyID));
})
.then(endSignOnTransition);
}

/**
* Create a new draft workspace and navigate to it
*
* @param {String} [policyOwnerEmail] Optional, the email of the account to make the owner of the policy
* @param {String} [policyName] Optional, custom policy name we will use for created workspace
* @param {Boolean} [transitionFromOldDot] Optional, if the user is transitioning from old dot
* @param {Boolean} [makeMeAdmin] Optional, leave the calling account as an admin on the policy
*/
function createWorkspaceWithPolicyDraftAndNavigateToIt(policyOwnerEmail = '', policyName = '', transitionFromOldDot = false) {
function createWorkspaceWithPolicyDraftAndNavigateToIt(policyOwnerEmail = '', policyName = '', transitionFromOldDot = false, makeMeAdmin = false) {
const policyID = Policy.generatePolicyID();
Policy.createDraftInitialWorkspace(policyOwnerEmail, policyName, policyID);
Policy.createDraftInitialWorkspace(policyOwnerEmail, policyName, policyID, makeMeAdmin);

Navigation.isNavigationReady()
.then(() => {
Expand Down Expand Up @@ -403,9 +376,8 @@ function savePolicyDraftByNewWorkspace(policyID, policyName, policyOwnerEmail =
* pass it in as a parameter. withOnyx guarantees that the value has been read
* from Onyx because it will not render the AuthScreens until that point.
* @param {Object} session
* @param {Boolean} shouldNavigateToAdminChat Should we navigate to admin chat after creating workspace
*/
function setUpPoliciesAndNavigate(session, shouldNavigateToAdminChat) {
function setUpPoliciesAndNavigate(session) {
const currentUrl = getCurrentUrl();
if (!session || !currentUrl || !currentUrl.includes('exitTo')) {
return;
Expand All @@ -426,7 +398,7 @@ function setUpPoliciesAndNavigate(session, shouldNavigateToAdminChat) {

const shouldCreateFreePolicy = !isLoggingInAsNewUser && isTransitioning && exitTo === ROUTES.WORKSPACE_NEW;
if (shouldCreateFreePolicy) {
createWorkspaceAndNavigateToIt(policyOwnerEmail, makeMeAdmin, policyName, true, shouldNavigateToAdminChat);
createWorkspaceWithPolicyDraftAndNavigateToIt(policyOwnerEmail, policyName, true, makeMeAdmin);
return;
}
if (!isLoggingInAsNewUser && exitTo) {
Expand Down Expand Up @@ -555,7 +527,6 @@ export {
handleRestrictedEvent,
beginDeepLinkRedirect,
beginDeepLinkRedirectAfterTransition,
createWorkspaceAndNavigateToIt,
getMissingOnyxUpdates,
finalReconnectAppAfterActivatingReliableUpdates,
savePolicyDraftByNewWorkspace,
Expand Down
4 changes: 3 additions & 1 deletion src/libs/actions/Policy.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -954,8 +954,9 @@ function buildOptimisticCustomUnits() {
* @param {String} [policyOwnerEmail] Optional, the email of the account to make the owner of the policy
* @param {String} [policyName] Optional, custom policy name we will use for created workspace
* @param {String} [policyID] Optional, custom policy id we will use for created workspace
* @param {Boolean} [makeMeAdmin] Optional, leave the calling account as an admin on the policy
*/
function createDraftInitialWorkspace(policyOwnerEmail = '', policyName = '', policyID = generatePolicyID()) {
function createDraftInitialWorkspace(policyOwnerEmail = '', policyName = '', policyID = generatePolicyID(), makeMeAdmin = false) {
const workspaceName = policyName || generateDefaultWorkspaceName(policyOwnerEmail);
const {customUnits} = buildOptimisticCustomUnits();

Expand All @@ -973,6 +974,7 @@ function createDraftInitialWorkspace(policyOwnerEmail = '', policyName = '', pol
outputCurrency: lodashGet(allPersonalDetails, [sessionAccountID, 'localCurrencyCode'], CONST.CURRENCY.USD),
pendingAction: CONST.RED_BRICK_ROAD_PENDING_ACTION.ADD,
customUnits,
makeMeAdmin,
},
},
{
Expand Down
2 changes: 1 addition & 1 deletion src/pages/home/sidebar/SidebarScreen/FloatingActionButtonAndPopover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,7 @@ function FloatingActionButtonAndPopover(props) {
iconHeight: 40,
text: props.translate('workspace.new.newWorkspace'),
description: props.translate('workspace.new.getTheExpensifyCardAndMore'),
onSelected: () => interceptAnonymousUser(() => App.createWorkspaceAndNavigateToIt('', false, '', false, !props.isSmallScreenWidth)),
onSelected: () => interceptAnonymousUser(() => App.createWorkspaceWithPolicyDraftAndNavigateToIt()),
},
]
: []),
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/WorkspaceInitialPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ function WorkspaceInitialPage(props) {
return;
}

App.savePolicyDraftByNewWorkspace(props.policyDraft.id, props.policyDraft.name, '', false);
App.savePolicyDraftByNewWorkspace(props.policyDraft.id, props.policyDraft.name, '', props.policyDraft.makeMeAdmin);
// We only care when the component renders the first time
// eslint-disable-next-line react-hooks/exhaustive-deps
}, []);
Expand Down
2 changes: 1 addition & 1 deletion src/pages/workspace/WorkspaceNewRoomPage.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ function WorkspaceNewRoomPage(props) {
shouldShow={!Permissions.canUsePolicyRooms(props.betas) || !workspaceOptions.length}
shouldShowBackButton={false}
linkKey="workspace.emptyWorkspace.title"
onLinkPress={() => App.createWorkspaceAndNavigateToIt('', false, '', false, false)}
onLinkPress={() => App.createWorkspaceWithPolicyDraftAndNavigateToIt()}
>
<ScreenWrapper
shouldEnableKeyboardAvoidingView={false}
Expand Down
Loading