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

MOBILE-4686 user-menu: Fix routes with canLeave cannot leave #4218

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

MOBILE-4686 user-menu: Fix routes with canLeave cannot leave #4218

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 2 deletions src/core/features/mainmenu/components/user-menu/user-menu.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ export class CoreMainMenuUserMenuComponent implements OnInit, OnDestroy {
* @param event Click event
*/
async logout(event: Event): Promise<void> {
if (CoreNavigator.currentRouteCanBlockLeave()) {
if (!CoreNavigator.currentRouteCanBlockLeave()) {
await CoreDomUtils.showAlert(undefined, Translate.instant('core.cannotlogoutpageblocks'));

return;
Expand Down Expand Up @@ -242,7 +242,7 @@ export class CoreMainMenuUserMenuComponent implements OnInit, OnDestroy {
* @param event Click event
*/
async switchAccounts(event: Event): Promise<void> {
if (CoreNavigator.currentRouteCanBlockLeave()) {
if (!CoreNavigator.currentRouteCanBlockLeave()) {
await CoreDomUtils.showAlert(undefined, Translate.instant('core.cannotlogoutpageblocks'));

return;
Expand Down
8 changes: 7 additions & 1 deletion src/core/services/navigator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -726,7 +726,13 @@ export class CoreNavigatorService {
* @returns Whether the current route page can block leaving the route.
*/
currentRouteCanBlockLeave(): boolean {
Copy link
Contributor

Choose a reason for hiding this comment

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

You're reversing the result of this function for no reason. Before, this function returned true if the route could block, now it returns false if it blocks, which doesn't make sense IMO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Your change is a breaking change that will break the usage in behat-blocking. IMO you should leave the previous function as it was and create a new one for the logout/switch account use case.

return !!this.getCurrentRoute().snapshot?.routeConfig?.canDeactivate?.length;
const canDeactivate = this.getCurrentRoute().snapshot?.routeConfig?.canDeactivate;

if (!canDeactivate?.length) {
return true;
}

return canDeactivate.every(method => typeof method === 'function' && method());
Copy link
Contributor

Choose a reason for hiding this comment

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

This code doesn't work. The canDeactivate function (in here, method) expects to receive an instance of the component of the current route, and you aren't passing anything. This throws an error in console:

ERROR TypeError: Cannot use 'in' operator to search for 'canLeave' in undefined
    at isCanLeave (can-leave.ts:25:37)

I checked the contents of getCurrentRoute() and I see we have access to the class of the component, but not the instance so I'm not sure if this is feasible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Even if we manage to fix this, I think this will cause a bad UX for the user. Your code is calling the canDeactivate function to check if the user can leave or not, which in most cases will display a confirm modal to the user, but you're not leaving the page.

After the user confirms that he wants to leave, the logout process will continue and this process performs a navigation to '/login/sites', which will trigger Angular's canDeactivate function again, displaying the confirm modal a second time. This is not what we want.

}

/**
Expand Down