From fa9f0742094d74b92edb4d7d9ce1455a69646a86 Mon Sep 17 00:00:00 2001 From: Dani Palou Date: Mon, 17 Oct 2022 09:03:18 +0200 Subject: [PATCH] MOBILE-4120 messages: Fix wrong messages displayed in split view --- src/addons/messages/messages-lazy.module.ts | 19 ++++++++---- .../pages/contacts-35/contacts.module.ts | 8 ++--- .../pages/contacts-35/contacts.page.ts | 11 ++----- .../pages/contacts/contacts.module.ts | 8 ++--- .../messages/pages/contacts/contacts.page.ts | 6 ++-- .../pages/discussion/discussion.page.ts | 30 +++++-------------- .../discussions-35/discussions.module.ts | 8 ++--- .../pages/discussions-35/discussions.page.ts | 7 ++--- .../group-conversations.module.ts | 8 ++--- .../group-conversations.page.ts | 12 ++------ .../messages/pages/search/search.module.ts | 8 ++--- .../messages/pages/search/search.page.ts | 21 ++++++------- .../services/handlers/discussion-link.ts | 6 ++-- .../messages/services/handlers/push-click.ts | 21 +++++-------- .../services/handlers/user-send-message.ts | 3 +- src/core/services/navigator.ts | 23 ++++++++++++++ src/core/services/tests/navigator.test.ts | 18 ++++++++++- 17 files changed, 107 insertions(+), 110 deletions(-) diff --git a/src/addons/messages/messages-lazy.module.ts b/src/addons/messages/messages-lazy.module.ts index 47a4d5d15..88b9d4d25 100644 --- a/src/addons/messages/messages-lazy.module.ts +++ b/src/addons/messages/messages-lazy.module.ts @@ -18,11 +18,18 @@ import { Route, RouterModule, ROUTES, Routes } from '@angular/router'; import { buildTabMainRoutes } from '@features/mainmenu/mainmenu-tab-routing.module'; import { AddonMessagesIndexGuard } from './guards'; -export const AddonMessagesDiscussionRoute: Route = { - path: 'discussion', - loadChildren: () => import('./pages/discussion/discussion.module') - .then(m => m.AddonMessagesDiscussionPageModule), -}; +export const DISCUSSION_ROUTES: Route[] = [ + { + path: 'discussion/user/:userId', + loadChildren: () => import('./pages/discussion/discussion.module') + .then(m => m.AddonMessagesDiscussionPageModule), + }, + { + path: 'discussion/:conversationId', + loadChildren: () => import('./pages/discussion/discussion.module') + .then(m => m.AddonMessagesDiscussionPageModule), + }, +]; function buildRoutes(injector: Injector): Routes { return [ @@ -40,7 +47,7 @@ function buildRoutes(injector: Injector): Routes { loadChildren: () => import('./pages/group-conversations/group-conversations.module') .then(m => m.AddonMessagesGroupConversationsPageModule), }, - AddonMessagesDiscussionRoute, + ...DISCUSSION_ROUTES, { path: 'search', loadChildren: () => import('./pages/search/search.module') diff --git a/src/addons/messages/pages/contacts-35/contacts.module.ts b/src/addons/messages/pages/contacts-35/contacts.module.ts index 3d9a91693..9a9752f5d 100644 --- a/src/addons/messages/pages/contacts-35/contacts.module.ts +++ b/src/addons/messages/pages/contacts-35/contacts.module.ts @@ -15,7 +15,7 @@ import { NgModule } from '@angular/core'; import { RouterModule, Routes } from '@angular/router'; import { conditionalRoutes } from '@/app/app-routing.module'; -import { AddonMessagesDiscussionRoute } from '@addons/messages/messages-lazy.module'; +import { DISCUSSION_ROUTES } from '@addons/messages/messages-lazy.module'; import { CoreScreen } from '@services/screen'; import { CoreSharedModule } from '@/core/shared.module'; @@ -28,16 +28,14 @@ const mobileRoutes: Routes = [ path: '', component: AddonMessagesContacts35Page, }, - AddonMessagesDiscussionRoute, + ...DISCUSSION_ROUTES, ]; const tabletRoutes: Routes = [ { path: '', component: AddonMessagesContacts35Page, - children: [ - AddonMessagesDiscussionRoute, - ], + children: DISCUSSION_ROUTES, }, ]; diff --git a/src/addons/messages/pages/contacts-35/contacts.page.ts b/src/addons/messages/pages/contacts-35/contacts.page.ts index 6ef6d90fe..34a379992 100644 --- a/src/addons/messages/pages/contacts-35/contacts.page.ts +++ b/src/addons/messages/pages/contacts-35/contacts.page.ts @@ -25,7 +25,7 @@ import { import { CoreDomUtils } from '@services/utils/dom'; import { CoreApp } from '@services/app'; import { CoreEventObserver, CoreEvents } from '@singletons/events'; -import { ActivatedRoute, Params } from '@angular/router'; +import { ActivatedRoute } from '@angular/router'; import { Translate } from '@singletons'; import { CoreScreen } from '@services/screen'; import { CoreNavigator } from '@services/navigator'; @@ -241,15 +241,10 @@ export class AddonMessagesContacts35Page implements OnInit, OnDestroy { gotoDiscussion(discussionUserId: number): void { this.discussionUserId = discussionUserId; - const params: Params = { - userId: discussionUserId, - }; - - const splitViewLoaded = CoreNavigator.isCurrentPathInTablet('**/messages/contacts-35/discussion'); - const path = (splitViewLoaded ? '../' : '') + 'discussion'; + const path = CoreNavigator.getRelativePathToParent('/messages/contacts-35') + `discussion/user/${discussionUserId}`; // @todo Check why this is failing on ngInit. - CoreNavigator.navigate(path, { params }); + CoreNavigator.navigate(path); } /** diff --git a/src/addons/messages/pages/contacts/contacts.module.ts b/src/addons/messages/pages/contacts/contacts.module.ts index c2688cdf9..b3325270f 100644 --- a/src/addons/messages/pages/contacts/contacts.module.ts +++ b/src/addons/messages/pages/contacts/contacts.module.ts @@ -15,7 +15,7 @@ import { NgModule } from '@angular/core'; import { RouterModule, Routes } from '@angular/router'; import { conditionalRoutes } from '@/app/app-routing.module'; -import { AddonMessagesDiscussionRoute } from '@addons/messages/messages-lazy.module'; +import { DISCUSSION_ROUTES } from '@addons/messages/messages-lazy.module'; import { CoreScreen } from '@services/screen'; import { CoreSharedModule } from '@/core/shared.module'; @@ -27,16 +27,14 @@ const mobileRoutes: Routes = [ path: '', component: AddonMessagesContactsPage, }, - AddonMessagesDiscussionRoute, + ...DISCUSSION_ROUTES, ]; const tabletRoutes: Routes = [ { path: '', component: AddonMessagesContactsPage, - children: [ - AddonMessagesDiscussionRoute, - ], + children: DISCUSSION_ROUTES, }, ]; diff --git a/src/addons/messages/pages/contacts/contacts.page.ts b/src/addons/messages/pages/contacts/contacts.page.ts index 4ec897a84..963d9d78d 100644 --- a/src/addons/messages/pages/contacts/contacts.page.ts +++ b/src/addons/messages/pages/contacts/contacts.page.ts @@ -291,10 +291,8 @@ export class AddonMessagesContactsPage implements OnInit, OnDestroy { this.selectedUserId = userId; - const splitViewLoaded = CoreNavigator.isCurrentPathInTablet('**/messages/contacts/discussion'); - const path = (splitViewLoaded ? '../' : '') + 'discussion'; - - CoreNavigator.navigate(path, { params : { userId } }); + const path = CoreNavigator.getRelativePathToParent('/messages/contacts') + `discussion/user/${userId}`; + CoreNavigator.navigate(path); } /** diff --git a/src/addons/messages/pages/discussion/discussion.page.ts b/src/addons/messages/pages/discussion/discussion.page.ts index 325c226bb..f92159fc3 100644 --- a/src/addons/messages/pages/discussion/discussion.page.ts +++ b/src/addons/messages/pages/discussion/discussion.page.ts @@ -154,28 +154,14 @@ export class AddonMessagesDiscussionPage implements OnInit, OnDestroy, AfterView * Setup code for the page. */ async ngOnInit(): Promise { + this.conversationId = CoreNavigator.getRouteNumberParam('conversationId'); + this.userId = CoreNavigator.getRouteNumberParam('userId'); + this.showInfo = !CoreNavigator.getRouteBooleanParam('hideInfo'); + this.showKeyboard = !!CoreNavigator.getRouteBooleanParam('showKeyboard'); - this.route.queryParams.subscribe(async (params) => { - const oldConversationId = this.conversationId; - const oldUserId = this.userId; - let forceScrollToBottom = false; - this.conversationId = CoreNavigator.getRouteNumberParam('conversationId', { params }) || undefined; - this.userId = CoreNavigator.getRouteNumberParam('userId', { params }) || undefined; - this.showInfo = !params.hideInfo; + await this.fetchData(); - if (oldConversationId != this.conversationId || oldUserId != this.userId) { - // Showing reload again can break animations. - this.loaded = false; - this.initialized = false; - forceScrollToBottom = true; - } - - this.showKeyboard = CoreNavigator.getRouteBooleanParam('showKeyboard', { params }) || false; - - await this.fetchData(); - - this.scrollToBottom(forceScrollToBottom); - }); + this.scrollToBottom(true); } /** @@ -1265,7 +1251,7 @@ export class AddonMessagesDiscussionPage implements OnInit, OnDestroy, AfterView }); if (userId !== undefined) { - const splitViewLoaded = CoreNavigator.isCurrentPathInTablet('**/messages/**/discussion'); + const splitViewLoaded = CoreNavigator.isCurrentPathInTablet('**/messages/**/discussion/**'); // Open user conversation. if (splitViewLoaded) { @@ -1277,7 +1263,7 @@ export class AddonMessagesDiscussionPage implements OnInit, OnDestroy, AfterView ); } else { // Open the discussion in a new view. - CoreNavigator.navigateToSitePath('/messages/discussion', { params: { userId } }); + CoreNavigator.navigateToSitePath(`/messages/discussion/user/${userId}`); } } } else { diff --git a/src/addons/messages/pages/discussions-35/discussions.module.ts b/src/addons/messages/pages/discussions-35/discussions.module.ts index f26263d62..a721b573b 100644 --- a/src/addons/messages/pages/discussions-35/discussions.module.ts +++ b/src/addons/messages/pages/discussions-35/discussions.module.ts @@ -16,7 +16,7 @@ import { NgModule } from '@angular/core'; import { RouterModule, Routes } from '@angular/router'; import { CoreScreen } from '@services/screen'; import { conditionalRoutes } from '@/app/app-routing.module'; -import { AddonMessagesDiscussionRoute } from '@addons/messages/messages-lazy.module'; +import { DISCUSSION_ROUTES } from '@addons/messages/messages-lazy.module'; import { CoreSharedModule } from '@/core/shared.module'; import { CoreSearchComponentsModule } from '@features/search/components/components.module'; @@ -33,7 +33,7 @@ const mobileRoutes: Routes = [ }, component: AddonMessagesDiscussions35Page, }, - AddonMessagesDiscussionRoute, + ...DISCUSSION_ROUTES, ]; const tabletRoutes: Routes = [ @@ -43,9 +43,7 @@ const tabletRoutes: Routes = [ mainMenuTabRoot: AddonMessagesMainMenuHandlerService.PAGE_NAME, }, component: AddonMessagesDiscussions35Page, - children: [ - AddonMessagesDiscussionRoute, - ], + children: DISCUSSION_ROUTES, }, ]; diff --git a/src/addons/messages/pages/discussions-35/discussions.page.ts b/src/addons/messages/pages/discussions-35/discussions.page.ts index a690647a0..a968beeab 100644 --- a/src/addons/messages/pages/discussions-35/discussions.page.ts +++ b/src/addons/messages/pages/discussions-35/discussions.page.ts @@ -257,16 +257,13 @@ export class AddonMessagesDiscussions35Page implements OnInit, OnDestroy { async gotoDiscussion(discussionUserId: number, messageId?: number): Promise { this.discussionUserId = discussionUserId; - const params: Params = { - userId: discussionUserId, - }; + const params: Params = {}; if (messageId) { params.message = messageId; } - const splitViewLoaded = CoreNavigator.isCurrentPathInTablet('**/messages/index/discussion'); - const path = (splitViewLoaded ? '../' : '') + 'discussion'; + const path = CoreNavigator.getRelativePathToParent('/messages/index') + `discussion/user/${discussionUserId}`; await CoreNavigator.navigate(path, { params }); } diff --git a/src/addons/messages/pages/group-conversations/group-conversations.module.ts b/src/addons/messages/pages/group-conversations/group-conversations.module.ts index 8fbcc9bae..88cd32835 100644 --- a/src/addons/messages/pages/group-conversations/group-conversations.module.ts +++ b/src/addons/messages/pages/group-conversations/group-conversations.module.ts @@ -15,7 +15,7 @@ import { NgModule } from '@angular/core'; import { RouterModule, Routes } from '@angular/router'; import { conditionalRoutes } from '@/app/app-routing.module'; -import { AddonMessagesDiscussionRoute } from '@addons/messages/messages-lazy.module'; +import { DISCUSSION_ROUTES } from '@addons/messages/messages-lazy.module'; import { CoreScreen } from '@services/screen'; import { CoreSharedModule } from '@/core/shared.module'; @@ -32,7 +32,7 @@ const mobileRoutes: Routes = [ }, component: AddonMessagesGroupConversationsPage, }, - AddonMessagesDiscussionRoute, + ...DISCUSSION_ROUTES, ]; const tabletRoutes: Routes = [ @@ -42,9 +42,7 @@ const tabletRoutes: Routes = [ mainMenuTabRoot: AddonMessagesMainMenuHandlerService.PAGE_NAME, }, component: AddonMessagesGroupConversationsPage, - children: [ - AddonMessagesDiscussionRoute, - ], + children: DISCUSSION_ROUTES, }, ]; diff --git a/src/addons/messages/pages/group-conversations/group-conversations.page.ts b/src/addons/messages/pages/group-conversations/group-conversations.page.ts index e5175dfa7..a423d6412 100644 --- a/src/addons/messages/pages/group-conversations/group-conversations.page.ts +++ b/src/addons/messages/pages/group-conversations/group-conversations.page.ts @@ -519,18 +519,12 @@ export class AddonMessagesGroupConversationsPage implements OnInit, OnDestroy { this.selectedUserId = userId; const params: Params = {}; - if (conversationId) { - params.conversationId = conversationId; - } - if (userId) { - params.userId = userId; - } if (messageId) { params.message = messageId; } - const splitViewLoaded = CoreNavigator.isCurrentPathInTablet('**/messages/group-conversations/discussion'); - const path = (splitViewLoaded ? '../' : '') + 'discussion'; + const path = CoreNavigator.getRelativePathToParent('/messages/group-conversations') + 'discussion/' + + (conversationId ? conversationId : `user/${userId}`); await CoreNavigator.navigate(path, { params }); } @@ -539,7 +533,7 @@ export class AddonMessagesGroupConversationsPage implements OnInit, OnDestroy { * Navigate to message settings. */ gotoSettings(): void { - CoreNavigator.navigateToSitePath('../message-settings'); + CoreNavigator.navigateToSitePath('message-settings'); } /** diff --git a/src/addons/messages/pages/search/search.module.ts b/src/addons/messages/pages/search/search.module.ts index eb3083df2..24337c262 100644 --- a/src/addons/messages/pages/search/search.module.ts +++ b/src/addons/messages/pages/search/search.module.ts @@ -16,7 +16,7 @@ import { NgModule } from '@angular/core'; import { RouterModule, Routes } from '@angular/router'; import { CoreScreen } from '@services/screen'; import { conditionalRoutes } from '@/app/app-routing.module'; -import { AddonMessagesDiscussionRoute } from '@addons/messages/messages-lazy.module'; +import { DISCUSSION_ROUTES } from '@addons/messages/messages-lazy.module'; import { CoreSharedModule } from '@/core/shared.module'; import { CoreSearchComponentsModule } from '@features/search/components/components.module'; @@ -28,16 +28,14 @@ const mobileRoutes: Routes = [ path: '', component: AddonMessagesSearchPage, }, - AddonMessagesDiscussionRoute, + ...DISCUSSION_ROUTES, ]; const tabletRoutes: Routes = [ { path: '', component: AddonMessagesSearchPage, - children: [ - AddonMessagesDiscussionRoute, - ], + children: DISCUSSION_ROUTES, }, ]; diff --git a/src/addons/messages/pages/search/search.page.ts b/src/addons/messages/pages/search/search.page.ts index e7e2e33e2..f9d1a02aa 100644 --- a/src/addons/messages/pages/search/search.page.ts +++ b/src/addons/messages/pages/search/search.page.ts @@ -24,7 +24,6 @@ import { import { CoreDomUtils } from '@services/utils/dom'; import { CoreApp } from '@services/app'; import { CoreNavigator } from '@services/navigator'; -import { Params } from '@angular/router'; import { CoreScreen } from '@services/screen'; /** @@ -107,9 +106,9 @@ export class AddonMessagesSearchPage implements OnDestroy { this.displayResults = false; // Empty details. - const splitViewLoaded = CoreNavigator.isCurrentPathInTablet('**/messages/search/discussion'); - if (splitViewLoaded) { - CoreNavigator.navigate('../'); + const path = CoreNavigator.getRelativePathToParent('/messages/search'); + if (path) { + CoreNavigator.navigate(path); } } @@ -250,16 +249,18 @@ export class AddonMessagesSearchPage implements OnDestroy { if (!onInit || CoreScreen.isTablet) { this.selectedResult = result; - const params: Params = {}; + let conversationId: number | undefined; + let userId: number | undefined; if ('conversationid' in result) { - params.conversationId = result.conversationid; + conversationId = result.conversationid; } else { - params.userId = result.id; + userId = result.id; } - const splitViewLoaded = CoreNavigator.isCurrentPathInTablet('**/messages/search/discussion'); - const path = (splitViewLoaded ? '../' : '') + 'discussion'; - CoreNavigator.navigate(path, { params }); + const path = CoreNavigator.getRelativePathToParent('/messages/search') + 'discussion/' + + (conversationId ? conversationId : `user/${userId}`); + + CoreNavigator.navigate(path); } } diff --git a/src/addons/messages/services/handlers/discussion-link.ts b/src/addons/messages/services/handlers/discussion-link.ts index 2eb03cdec..1ec3f28fc 100644 --- a/src/addons/messages/services/handlers/discussion-link.ts +++ b/src/addons/messages/services/handlers/discussion-link.ts @@ -46,10 +46,8 @@ export class AddonMessagesDiscussionLinkHandlerService extends CoreContentLinksH ): CoreContentLinksAction[] | Promise { return [{ action: (siteId): void => { - const stateParams = { - userId: parseInt(params.id || params.user2, 10), - }; - CoreNavigator.navigateToSitePath('/messages/discussion', { params: stateParams, siteId }); + const userId = parseInt(params.id || params.user2, 10); + CoreNavigator.navigateToSitePath(`/messages/discussion/user/${userId}`, { siteId }); }, }]; } diff --git a/src/addons/messages/services/handlers/push-click.ts b/src/addons/messages/services/handlers/push-click.ts index f90495ab4..6bb3da0eb 100644 --- a/src/addons/messages/services/handlers/push-click.ts +++ b/src/addons/messages/services/handlers/push-click.ts @@ -13,7 +13,6 @@ // limitations under the License. import { Injectable } from '@angular/core'; -import { Params } from '@angular/router'; import { CorePushNotificationsClickHandler } from '@features/pushnotifications/services/push-delegate'; import { CorePushNotificationsNotificationBasicData } from '@features/pushnotifications/services/pushnotifications'; import { CoreNavigator } from '@services/navigator'; @@ -63,28 +62,22 @@ export class AddonMessagesPushClickHandlerService implements CorePushNotificatio // Check if group messaging is enabled, to determine which page should be loaded. const enabled = await AddonMessages.isGroupMessagingEnabledInSite(notification.site); - let nextPageParams: Params | undefined; + let conversationId: number | undefined; + let userId: number | undefined; // Check if we have enough information to open the conversation. if (notification.convid && enabled) { - nextPageParams = { - conversationId: Number(notification.convid), - }; + conversationId = Number(notification.convid); } else if (notification.userfromid) { - nextPageParams = { - userId: Number(notification.userfromid), - }; + userId = Number(notification.userfromid); } await CoreNavigator.navigateToSitePath(AddonMessagesMainMenuHandlerService.PAGE_NAME, { siteId: notification.site, preferCurrentTab: false, - nextNavigation: nextPageParams ? - { - path: 'discussion', - options: { params: nextPageParams }, - } : - undefined, + nextNavigation: conversationId ? + { path: `discussion/${conversationId}` } : + (userId ? { path: `discussion/user/${userId}` } : undefined), }); } diff --git a/src/addons/messages/services/handlers/user-send-message.ts b/src/addons/messages/services/handlers/user-send-message.ts index 0e9d12f8d..aba6b12ed 100644 --- a/src/addons/messages/services/handlers/user-send-message.ts +++ b/src/addons/messages/services/handlers/user-send-message.ts @@ -71,10 +71,9 @@ export class AddonMessagesSendMessageUserHandlerService implements CoreUserProfi const pageParams: Params = { showKeyboard: true, - userId: user.id, hideInfo: true, }; - CoreNavigator.navigateToSitePath('/messages/discussion', { params: pageParams }); + CoreNavigator.navigateToSitePath(`/messages/discussion/user/${user.id}`, { params: pageParams }); }, }; } diff --git a/src/core/services/navigator.ts b/src/core/services/navigator.ts index a9ea1c921..130852f54 100644 --- a/src/core/services/navigator.ts +++ b/src/core/services/navigator.ts @@ -700,6 +700,29 @@ export class CoreNavigatorService { return promise; } + /** + * Get the relative path to a parent path. + * E.g. if parent path is '/foo' and current path is '/foo/bar/baz' it will return '../../'. + * + * @param parentPath Parent path. + * @return Relative path to the parent, empty if same path or parent path not found. + * @todo If messaging is refactored to use list managers, this function might not be needed anymore. + */ + getRelativePathToParent(parentPath: string): string { + // Add an ending slash to avoid collisions with other routes (e.g. /foo and /foobar). + parentPath = CoreTextUtils.addEndingSlash(parentPath); + + const path = this.getCurrentPath(); + const parentRouteIndex = path.indexOf(parentPath); + if (parentRouteIndex === -1) { + return ''; + } + + const depth = (path.substring(parentRouteIndex + parentPath.length - 1).match(/\//g) ?? []).length; + + return '../'.repeat(depth); + } + } export const CoreNavigator = makeSingleton(CoreNavigatorService); diff --git a/src/core/services/tests/navigator.test.ts b/src/core/services/tests/navigator.test.ts index 7b995a102..0433ecb0f 100644 --- a/src/core/services/tests/navigator.test.ts +++ b/src/core/services/tests/navigator.test.ts @@ -14,7 +14,7 @@ import { NavController as NavControllerService } from '@ionic/angular'; -import { mockSingleton } from '@/testing/utils'; +import { mock, mockSingleton } from '@/testing/utils'; import { CoreNavigatorService } from '@services/navigator'; import { NavController, Router } from '@singletons'; @@ -156,6 +156,22 @@ describe('CoreNavigator', () => { expect(navControllerMock.navigateRoot).toHaveBeenCalledWith(['/main/initialpage'], {}); }); + it('calculates relative paths to parent paths', () => { + navigator = mock(navigator, { + getCurrentPath: () => '/foo/bar/baz/xyz', + }); + + expect(navigator.getRelativePathToParent('/foo/bar/baz/xyz')).toEqual(''); + expect(navigator.getRelativePathToParent('/foo/bar/baz')).toEqual('../'); + expect(navigator.getRelativePathToParent('/foo/bar')).toEqual('../../'); + expect(navigator.getRelativePathToParent('/bar')).toEqual('../../'); + expect(navigator.getRelativePathToParent('/foo')).toEqual('../../../'); + expect(navigator.getRelativePathToParent('/foo/')).toEqual('../../../'); + expect(navigator.getRelativePathToParent('foo')).toEqual('../../../'); + expect(navigator.getRelativePathToParent('/invalid')).toEqual(''); + expect(navigator.getRelativePathToParent('/fo')).toEqual(''); + }); + it.todo('navigates to a different site'); it.todo('navigates to login credentials'); it.todo('navigates to NO_SITE_ID site');