From 9d78935bf08e24583a60ce82f62416b483cd505b Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sun, 7 Jun 2026 10:05:11 +0200 Subject: [PATCH 1/2] Revert "[stable34] Revert "refactor(NavigationManager): move navigation definitions into apps"" Signed-off-by: Ferdinand Thiessen --- apps/appstore/appinfo/info.xml | 17 ++ .../lib/Controller/PageController.php | 4 - .../tests/Controller/PageControllerTest.php | 13 - apps/profile/lib/AppInfo/Application.php | 33 +++ apps/settings/appinfo/info.xml | 1 - apps/settings/appinfo/routes.php | 2 +- apps/settings/lib/AppInfo/Application.php | 92 +++++++ core/AppInfo/Application.php | 36 ++- cypress/e2e/systemtags/admin-settings.cy.ts | 6 +- .../e2e/theming/admin-settings_branding.cy.ts | 2 +- lib/base.php | 1 + lib/private/App/AppManager.php | 5 +- lib/private/NavigationManager.php | 120 +-------- tests/lib/NavigationManagerTest.php | 240 +++++------------- 14 files changed, 248 insertions(+), 324 deletions(-) diff --git a/apps/appstore/appinfo/info.xml b/apps/appstore/appinfo/info.xml index 65e4a8f2d6e85..a59246caa10a8 100644 --- a/apps/appstore/appinfo/info.xml +++ b/apps/appstore/appinfo/info.xml @@ -19,4 +19,21 @@ + + + + Appstore + appstore.page.viewApps + app.svg + 99 + + + + Apps + appstore.page.viewApps + app.svg + 5 + settings + + diff --git a/apps/appstore/lib/Controller/PageController.php b/apps/appstore/lib/Controller/PageController.php index f77d004626ca8..d218e612ac16a 100644 --- a/apps/appstore/lib/Controller/PageController.php +++ b/apps/appstore/lib/Controller/PageController.php @@ -23,7 +23,6 @@ use OCP\AppFramework\Services\IInitialState; use OCP\IConfig; use OCP\IL10N; -use OCP\INavigationManager; use OCP\IRequest; use OCP\IURLGenerator; use OCP\Server; @@ -41,7 +40,6 @@ public function __construct( private readonly IURLGenerator $urlGenerator, private readonly IInitialState $initialState, private readonly BundleFetcher $bundleFetcher, - private readonly INavigationManager $navigationManager, ) { parent::__construct(Application::APP_ID, $request); } @@ -51,8 +49,6 @@ public function __construct( #[FrontpageRoute(verb: 'GET', url: '/settings/apps/{category}', defaults: ['category' => ''], root: '')] #[FrontpageRoute(verb: 'GET', url: '/settings/apps/{category}/{id}', defaults: ['category' => '', 'id' => ''], root: '')] public function viewApps(): TemplateResponse { - $this->navigationManager->setActiveEntry('core_apps'); - $this->initialState->provideInitialState('appstoreEnabled', $this->config->getSystemValueBool('appstoreenabled', true)); $this->initialState->provideInitialState('appstoreBundles', $this->getBundles()); $this->initialState->provideInitialState('appstoreDeveloperDocs', $this->urlGenerator->linkToDocs('developer-manual')); diff --git a/apps/appstore/tests/Controller/PageControllerTest.php b/apps/appstore/tests/Controller/PageControllerTest.php index 1e5edfe061f86..b41683fa616cb 100644 --- a/apps/appstore/tests/Controller/PageControllerTest.php +++ b/apps/appstore/tests/Controller/PageControllerTest.php @@ -16,7 +16,6 @@ use OCP\AppFramework\Services\IInitialState; use OCP\IConfig; use OCP\IL10N; -use OCP\INavigationManager; use OCP\IRequest; use OCP\IURLGenerator; use PHPUnit\Framework\MockObject\MockObject; @@ -30,8 +29,6 @@ final class PageControllerTest extends TestCase { private IConfig&MockObject $config; - private INavigationManager&MockObject $navigationManager; - private IAppManager&MockObject $appManager; private BundleFetcher&MockObject $bundleFetcher; @@ -54,7 +51,6 @@ protected function setUp(): void { ->method('t') ->willReturnArgument(0); $this->config = $this->createMock(IConfig::class); - $this->navigationManager = $this->createMock(INavigationManager::class); $this->appManager = $this->createMock(IAppManager::class); $this->bundleFetcher = $this->createMock(BundleFetcher::class); $this->installer = $this->createMock(Installer::class); @@ -70,7 +66,6 @@ protected function setUp(): void { $this->urlGenerator, $this->initialState, $this->bundleFetcher, - $this->navigationManager, ); } @@ -84,10 +79,6 @@ public function testViewApps(): void { ->method('getSystemValueBool') ->with('appstoreenabled', true) ->willReturn(true); - $this->navigationManager - ->expects($this->once()) - ->method('setActiveEntry') - ->with('core_apps'); $this->initialState ->expects($this->exactly(4)) @@ -117,10 +108,6 @@ public function testViewAppsAppstoreNotEnabled(): void { ->method('getSystemValueBool') ->with('appstoreenabled', true) ->willReturn(false); - $this->navigationManager - ->expects($this->once()) - ->method('setActiveEntry') - ->with('core_apps'); $this->initialState ->expects($this->exactly(4)) diff --git a/apps/profile/lib/AppInfo/Application.php b/apps/profile/lib/AppInfo/Application.php index ff2d3f8601683..8eb075ff59e0e 100644 --- a/apps/profile/lib/AppInfo/Application.php +++ b/apps/profile/lib/AppInfo/Application.php @@ -16,6 +16,11 @@ use OCP\AppFramework\Bootstrap\IRegistrationContext; use OCP\Collaboration\Reference\RenderReferenceEvent; +use OCP\INavigationManager; +use OCP\IURLGenerator; +use OCP\IUserSession; +use OCP\L10N\IFactory; +use OCP\Server; class Application extends App implements IBootstrap { public const APP_ID = 'profile'; @@ -32,5 +37,33 @@ public function register(IRegistrationContext $context): void { #[\Override] public function boot(IBootContext $context): void { + $context->injectFn($this->registerNavigationEntry(...)); + } + + /** + * Registers the navigation entry for the profile app in the user settings. + * Needed as the href is dynamic and thus we cannot use the appinfo/info.xml + */ + public function registerNavigationEntry( + INavigationManager $navigationManager, + IUserSession $userSession, + IURLGenerator $urlGenerator, + ): void { + if (!$userSession->isLoggedIn()) { + return; + } + + $l = Server::get(IFactory::class)->get('profile'); + // Profile + $navigationManager->add([ + 'type' => 'settings', + 'id' => 'profile', + 'order' => 1, + 'href' => $urlGenerator->linkToRoute( + 'profile.ProfilePage.index', + ['targetUserId' => $userSession->getUser()->getUID()], + ), + 'name' => $l->t('View profile'), + ]); } } diff --git a/apps/settings/appinfo/info.xml b/apps/settings/appinfo/info.xml index 879859fe1c840..10b1e0be9fcda 100644 --- a/apps/settings/appinfo/info.xml +++ b/apps/settings/appinfo/info.xml @@ -74,6 +74,5 @@ OCA\Settings\Activity\Provider OCA\Settings\Activity\SecurityProvider - diff --git a/apps/settings/appinfo/routes.php b/apps/settings/appinfo/routes.php index abe000122cbf3..85604ab7585a9 100644 --- a/apps/settings/appinfo/routes.php +++ b/apps/settings/appinfo/routes.php @@ -33,7 +33,7 @@ ['name' => 'CheckSetup#getFailedIntegrityCheckFiles', 'url' => '/settings/integrity/failed', 'verb' => 'GET' , 'root' => ''], ['name' => 'CheckSetup#rescanFailedIntegrityCheck', 'url' => '/settings/integrity/rescan', 'verb' => 'GET' , 'root' => ''], ['name' => 'PersonalSettings#index', 'url' => '/settings/user/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'personal-info'] , 'root' => ''], - ['name' => 'AdminSettings#index', 'url' => '/settings/admin/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'server'] , 'root' => ''], + ['name' => 'AdminSettings#index', 'url' => '/settings/admin/{section}', 'verb' => 'GET', 'defaults' => ['section' => 'overview'] , 'root' => ''], ['name' => 'AdminSettings#form', 'url' => '/settings/admin/{section}', 'verb' => 'GET' , 'root' => ''], ['name' => 'ChangePassword#changePersonalPassword', 'url' => '/settings/personal/changepassword', 'verb' => 'POST' , 'root' => ''], ['name' => 'ChangePassword#changeUserPassword', 'url' => '/settings/users/changepassword', 'verb' => 'POST' , 'root' => ''], diff --git a/apps/settings/lib/AppInfo/Application.php b/apps/settings/lib/AppInfo/Application.php index ed746572ff485..ca005642a0f7b 100644 --- a/apps/settings/lib/AppInfo/Application.php +++ b/apps/settings/lib/AppInfo/Application.php @@ -90,8 +90,12 @@ use OCP\Group\Events\GroupDeletedEvent; use OCP\Group\Events\UserAddedEvent; use OCP\Group\Events\UserRemovedEvent; +use OCP\Group\ISubAdmin; use OCP\IConfig; +use OCP\IGroupManager; +use OCP\INavigationManager; use OCP\IURLGenerator; +use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Mail\IMailer; use OCP\Security\ICrypto; @@ -226,5 +230,93 @@ public function register(IRegistrationContext $context): void { #[\Override] public function boot(IBootContext $context): void { + $context->injectFn($this->registerNavigationEntries(...)); + } + + /** + * Registers the navigation entries for the user settings. + * Needed as some entries are dynamic and thus we cannot use the appinfo/info.xml + * + * Registers the following entries: + * - Appearance and accessibility + * - Personal settings (named "Settings" for non-admins) + * - Accounts (only for subadmins) + * - Help & privacy (conditionally enabled based on config) + */ + public function registerNavigationEntries( + INavigationManager $navigationManager, + IURLGenerator $urlGenerator, + IUserSession $userSession, + IConfig $config, + ): void { + if ($userSession->getUser() === null) { + return; + } + + $l = Server::get(IFactory::class) + ->get('settings'); + $groupManager = Server::get(IGroupManager::class); + $subAdmin = Server::get(ISubAdmin::class); + $isAdmin = $groupManager->isAdmin($userSession->getUser()->getUID()); + $isSubAdmin = $subAdmin->isSubAdmin($userSession->getUser()); + + // Accessibility settings - the URL is dynamic (route parameters) which is currently not supported by appinfo.xml + $navigationManager->add([ + 'type' => 'settings', + 'id' => 'accessibility_settings', + 'order' => 2, + 'href' => $urlGenerator->linkToRoute('settings.PersonalSettings.index', ['section' => 'theming']), + 'name' => $l->t('Appearance and accessibility'), + 'icon' => $urlGenerator->imagePath('theming', 'accessibility-dark.svg'), + ]); + + // Personal settings - this entry is dynamic so we cannot use appinfo + $navigationManager->add([ + 'type' => 'settings', + 'id' => 'settings_personal', + 'order' => 3, + 'href' => $urlGenerator->linkToRoute('settings.PersonalSettings.index'), + 'name' => $isAdmin + ? $l->t('Personal settings') + : $l->t('Settings'), + 'icon' => $isAdmin + ? $urlGenerator->imagePath('settings', 'personal.svg') + : $urlGenerator->imagePath('settings', 'admin.svg'), + ]); + + if ($isAdmin) { + $navigationManager->add([ + 'type' => 'settings', + 'id' => 'settings_administration', + 'order' => 4, + 'href' => $urlGenerator->linkToRoute('settings.adminSettings.index'), + 'name' => $l->t('Administration settings'), + 'icon' => $urlGenerator->imagePath('settings', 'admin.svg'), + ]); + } + + // User management is conditionally enabled for subadmins, but appinfo currently only supports full admins + if ($isSubAdmin) { + $navigationManager->add([ + 'type' => 'settings', + 'id' => 'core_users', + 'order' => 6, + 'href' => $urlGenerator->linkToRoute('settings.Users.usersList'), + 'name' => $l->t('Accounts'), + 'icon' => $urlGenerator->imagePath('settings', 'users.svg'), + ]); + } + + // conditionally enabled navigation entry + if ($config->getSystemValueBool('knowledgebaseenabled', true)) { + $navigationManager->add([ + 'type' => 'settings', + 'id' => 'help', + 'order' => 99998, + 'href' => $urlGenerator->linkToRoute('settings.Help.help'), + 'name' => $l->t('Help & privacy'), + 'icon' => $urlGenerator->imagePath('settings', 'help.svg'), + ]); + } } } diff --git a/core/AppInfo/Application.php b/core/AppInfo/Application.php index 15cf42c4a5505..83edd25dd8cf9 100644 --- a/core/AppInfo/Application.php +++ b/core/AppInfo/Application.php @@ -32,6 +32,11 @@ use OCP\AppFramework\Http\Events\BeforeTemplateRenderedEvent; use OCP\DB\Events\AddMissingIndicesEvent; use OCP\DB\Events\AddMissingPrimaryKeyEvent; +use OCP\INavigationManager; +use OCP\IURLGenerator; +use OCP\IUserSession; +use OCP\L10N\IFactory; +use OCP\Server; use OCP\User\Events\BeforeUserDeletedEvent; use OCP\User\Events\PasswordUpdatedEvent; use OCP\User\Events\UserDeletedEvent; @@ -93,7 +98,36 @@ public function register(IRegistrationContext $context): void { #[\Override] public function boot(IBootContext $context): void { - // ... + $context->injectFn($this->registerNavigationEntries(...)); + } + + /** + * Registers the navigation entries for the core app: + * - The logout button in the settings menu + */ + public function registerNavigationEntries( + INavigationManager $navigationManager, + IUserSession $userSession, + IURLGenerator $urlGenerator, + ): void { + if (!$userSession->isLoggedIn()) { + return; + } + + $l = Server::get(IFactory::class)->get('core'); + + // Register the logout button in the user settings + $logoutUrl = \OC_User::getLogoutUrl($urlGenerator); + if ($logoutUrl !== '') { + $navigationManager->add([ + 'type' => 'settings', + 'id' => 'logout', + 'order' => 99999, + 'href' => $logoutUrl, + 'name' => $l->t('Log out'), + 'icon' => $urlGenerator->imagePath('core', 'actions/logout.svg'), + ]); + } } } diff --git a/cypress/e2e/systemtags/admin-settings.cy.ts b/cypress/e2e/systemtags/admin-settings.cy.ts index c2e6c89a5809b..c8b74335a5a15 100644 --- a/cypress/e2e/systemtags/admin-settings.cy.ts +++ b/cypress/e2e/systemtags/admin-settings.cy.ts @@ -21,7 +21,7 @@ describe('Create system tags', () => { // login as admin and go to admin settings cy.login(admin) - cy.visit('/settings/admin') + cy.visit('/settings/admin/server') }) it('Can create a tag', () => { @@ -48,7 +48,7 @@ describe('Create system tags', () => { describe('Update system tags', { testIsolation: false }, () => { before(() => { cy.login(admin) - cy.visit('/settings/admin') + cy.visit('/settings/admin/server') }) it('select the tag', () => { @@ -92,7 +92,7 @@ describe('Update system tags', { testIsolation: false }, () => { describe('Delete system tags', { testIsolation: false }, () => { before(() => { cy.login(admin) - cy.visit('/settings/admin') + cy.visit('/settings/admin/server') }) it('select the tag', () => { diff --git a/cypress/e2e/theming/admin-settings_branding.cy.ts b/cypress/e2e/theming/admin-settings_branding.cy.ts index 314a7e2224b6f..1f21045edf174 100644 --- a/cypress/e2e/theming/admin-settings_branding.cy.ts +++ b/cypress/e2e/theming/admin-settings_branding.cy.ts @@ -154,7 +154,7 @@ describe('Admin theming: Change the login fields then reset them', function() { it('See the admin theming section', function() { cy.visit('/settings/admin/theming') - cy.findByRole('heading', { name: /^Theming/ }) + cy.findByRole('heading', { name: /^Theming/, level: 2 }) .should('exist') .scrollIntoView() }) diff --git a/lib/base.php b/lib/base.php index b945ba3ee1d33..3277f0c42e0e0 100644 --- a/lib/base.php +++ b/lib/base.php @@ -850,6 +850,7 @@ public static function init(): void { // Make sure that the application class is not loaded before the database is setup if ($systemConfig->getValue('installed', false)) { + $appManager->loadApp('core'); $appManager->loadApp('settings'); } diff --git a/lib/private/App/AppManager.php b/lib/private/App/AppManager.php index 1f91825733713..d4521edabfc96 100644 --- a/lib/private/App/AppManager.php +++ b/lib/private/App/AppManager.php @@ -1187,8 +1187,11 @@ public function isAppCompatible(string $serverVersion, array $appInfo, bool $ign #[\Override] public function getAppNamespace(string $appId): string { - $topNamespace = 'OCA\\'; + if ($appId === 'core') { + return 'OC\\Core'; + } + $topNamespace = 'OCA\\'; // Hit the cache! if (isset($this->namespaceCache[$appId])) { return $topNamespace . $this->namespaceCache[$appId]; diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php index f1047d17a537e..f5a9d995c7b25 100644 --- a/lib/private/NavigationManager.php +++ b/lib/private/NavigationManager.php @@ -8,7 +8,6 @@ namespace OC; use InvalidArgumentException; -use OC\Group\Manager; use OCP\App\IAppManager; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; @@ -203,109 +202,6 @@ private function init(bool $resolveClosures = true): void { $this->init = true; $l = $this->l10nFac->get('lib'); - if ($this->config->getSystemValueBool('knowledgebaseenabled', true)) { - $this->add([ - 'type' => 'settings', - 'id' => 'help', - 'order' => 99998, - 'href' => $this->urlGenerator->linkToRoute('settings.Help.help'), - 'name' => $l->t('Help & privacy'), - 'icon' => $this->urlGenerator->imagePath('settings', 'help.svg'), - ]); - } - - if ($this->userSession->isLoggedIn()) { - // Profile - $this->add([ - 'type' => 'settings', - 'id' => 'profile', - 'order' => 1, - 'href' => $this->urlGenerator->linkToRoute( - 'profile.ProfilePage.index', - ['targetUserId' => $this->userSession->getUser()->getUID()], - ), - 'name' => $l->t('View profile'), - ]); - - // Accessibility settings - if ($this->appManager->isEnabledForUser('theming', $this->userSession->getUser())) { - $this->add([ - 'type' => 'settings', - 'id' => 'accessibility_settings', - 'order' => 2, - 'href' => $this->urlGenerator->linkToRoute('settings.PersonalSettings.index', ['section' => 'theming']), - 'name' => $l->t('Appearance and accessibility'), - 'icon' => $this->urlGenerator->imagePath('theming', 'accessibility-dark.svg'), - ]); - } - - if ($this->isAdmin()) { - // App management - $this->add([ - 'type' => 'settings', - 'id' => 'core_apps', - 'order' => 5, - 'href' => $this->urlGenerator->linkToRoute('appstore.Page.viewApps'), - 'icon' => $this->urlGenerator->imagePath('appstore', 'app.svg'), - 'name' => $l->t('Apps'), - ]); - - // Personal settings - $this->add([ - 'type' => 'settings', - 'id' => 'settings', - 'order' => 3, - 'href' => $this->urlGenerator->linkToRoute('settings.PersonalSettings.index'), - 'name' => $l->t('Personal settings'), - 'icon' => $this->urlGenerator->imagePath('settings', 'personal.svg'), - ]); - - // Admin settings - $this->add([ - 'type' => 'settings', - 'id' => 'admin_settings', - 'order' => 4, - 'href' => $this->urlGenerator->linkToRoute('settings.AdminSettings.index', ['section' => 'overview']), - 'name' => $l->t('Administration settings'), - 'icon' => $this->urlGenerator->imagePath('settings', 'admin.svg'), - ]); - } else { - // Personal settings - $this->add([ - 'type' => 'settings', - 'id' => 'settings', - 'order' => 3, - 'href' => $this->urlGenerator->linkToRoute('settings.PersonalSettings.index'), - 'name' => $l->t('Settings'), - 'icon' => $this->urlGenerator->imagePath('settings', 'admin.svg'), - ]); - } - - $logoutUrl = \OC_User::getLogoutUrl($this->urlGenerator); - if ($logoutUrl !== '') { - // Logout - $this->add([ - 'type' => 'settings', - 'id' => 'logout', - 'order' => 99999, - 'href' => $logoutUrl, - 'name' => $l->t('Log out'), - 'icon' => $this->urlGenerator->imagePath('core', 'actions/logout.svg'), - ]); - } - - if ($this->isSubadmin()) { - // User management - $this->add([ - 'type' => 'settings', - 'id' => 'core_users', - 'order' => 6, - 'href' => $this->urlGenerator->linkToRoute('settings.Users.usersList'), - 'name' => $l->t('Accounts'), - 'icon' => $this->urlGenerator->imagePath('settings', 'users.svg'), - ]); - } - } $this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent()); if ($this->userSession->isLoggedIn()) { @@ -316,10 +212,6 @@ private function init(bool $resolveClosures = true): void { } foreach ($apps as $app) { - if (!$this->userSession->isLoggedIn() && !$this->appManager->isEnabledForUser($app, $this->userSession->getUser())) { - continue; - } - // load plugins and collections from info.xml $info = $this->appManager->getAppInfo($app); if (!isset($info['navigations']['navigation'])) { @@ -365,14 +257,14 @@ private function init(bool $resolveClosures = true): void { 'order' => $order, // Target of the navigation entry 'href' => $route, - // The icon used for the naviation entry + // The icon used for the navigation entry 'icon' => $icon, // Type of the navigation entry ('link' vs 'settings') 'type' => $type, // Localized name of the navigation entry 'name' => $l->t($nav['name']), ], $type === 'link' ? [ - // App that registered this navigation entry (not necessarly the same as the id) + // App that registered this navigation entry (not necessarily the same as the id) 'app' => $app, ] : [] )); @@ -388,14 +280,6 @@ private function isAdmin(): bool { return false; } - private function isSubadmin(): bool { - $user = $this->userSession->getUser(); - if ($user !== null && $this->groupManager instanceof Manager) { - return $this->groupManager->getSubAdmin()->isSubAdmin($user); - } - return false; - } - #[Override] public function setUnreadCounter(string $id, int $unreadCounter): void { $this->unreadCounters[$id] = $unreadCounter; diff --git a/tests/lib/NavigationManagerTest.php b/tests/lib/NavigationManagerTest.php index 4322791e29ea3..1b2805f9c0860 100644 --- a/tests/lib/NavigationManagerTest.php +++ b/tests/lib/NavigationManagerTest.php @@ -11,8 +11,6 @@ use OC\App\AppManager; use OC\Group\Manager; use OC\NavigationManager; -use OC\Security\CSRF\CsrfTokenManager; -use OC\SubAdmin; use OCP\EventDispatcher\IEventDispatcher; use OCP\IConfig; use OCP\IGroupManager; @@ -22,7 +20,6 @@ use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Navigation\Events\LoadAdditionalEntriesEvent; -use OCP\Server; use PHPUnit\Framework\MockObject\MockObject; use Psr\Log\LoggerInterface; @@ -260,9 +257,6 @@ public function testWithAppManager($expected, $navigation, $isAdmin = false): vo ->with($user) ->willReturn(['test']); $this->groupManager->expects($this->any())->method('isAdmin')->willReturn($isAdmin); - $subadmin = $this->createMock(SubAdmin::class); - $subadmin->expects($this->any())->method('isSubAdmin')->with($user)->willReturn(false); - $this->groupManager->expects($this->any())->method('getSubAdmin')->willReturn($subadmin); $this->navigationManager->clear(); $this->dispatcher->expects($this->once()) @@ -275,112 +269,21 @@ public function testWithAppManager($expected, $navigation, $isAdmin = false): vo } public static function providesNavigationConfig(): array { - $apps = [ - 'core_apps' => [ - 'id' => 'core_apps', - 'order' => 5, - 'href' => '/apps/test/', - 'icon' => '/apps/appstore/img/app.svg', - 'name' => 'Apps', - 'active' => false, - 'type' => 'settings', - 'classes' => '', - 'unread' => 0 - ] - ]; - $defaults = [ - 'profile' => [ - 'type' => 'settings', - 'id' => 'profile', - 'order' => 1, - 'href' => '/apps/test/', - 'name' => 'View profile', - 'icon' => '', - 'active' => false, - 'classes' => '', - 'unread' => 0, - ], - 'accessibility_settings' => [ - 'type' => 'settings', - 'id' => 'accessibility_settings', - 'order' => 2, - 'href' => '/apps/test/', - 'name' => 'Appearance and accessibility', - 'icon' => '/apps/theming/img/accessibility-dark.svg', - 'active' => false, - 'classes' => '', - 'unread' => 0, - ], - 'settings' => [ - 'id' => 'settings', - 'order' => 3, - 'href' => '/apps/test/', - 'icon' => '/apps/settings/img/admin.svg', - 'name' => 'Settings', - 'active' => false, - 'type' => 'settings', - 'classes' => '', - 'unread' => 0 - ], - 'logout' => [ - 'id' => 'logout', - 'order' => 99999, - 'href' => 'https://example.com/logout?requesttoken=' . urlencode(Server::get(CsrfTokenManager::class)->getToken()->getEncryptedValue()), - 'icon' => '/apps/core/img/actions/logout.svg', - 'name' => 'Log out', - 'active' => false, - 'type' => 'settings', - 'classes' => '', - 'unread' => 0 - ] - ]; - $adminSettings = [ - 'accessibility_settings' => $defaults['accessibility_settings'], - 'settings' => [ - 'id' => 'settings', - 'order' => 3, - 'href' => '/apps/test/', - 'icon' => '/apps/settings/img/personal.svg', - 'name' => 'Personal settings', - 'active' => false, - 'type' => 'settings', - 'classes' => '', - 'unread' => 0 - ], - 'admin_settings' => [ - 'id' => 'admin_settings', - 'order' => 4, - 'href' => '/apps/test/', - 'icon' => '/apps/settings/img/admin.svg', - 'name' => 'Administration settings', - 'active' => false, - 'type' => 'settings', - 'classes' => '', - 'unread' => 0 - ] - ]; - return [ 'minimalistic' => [ - array_merge( - ['profile' => $defaults['profile']], - ['accessibility_settings' => $defaults['accessibility_settings']], - ['settings' => $defaults['settings']], - ['test' => [ - 'id' => 'test', - 'order' => 100, - 'href' => '/apps/test/', - 'icon' => '/apps/test/img/app.svg', - 'name' => 'Test', - 'active' => false, - 'type' => 'link', - 'classes' => '', - 'unread' => 0, - 'default' => true, - 'app' => 'test', - ]], - ['logout' => $defaults['logout']] - ), + ['test' => [ + 'id' => 'test', + 'order' => 100, + 'href' => '/apps/test/', + 'icon' => '/apps/test/img/app.svg', + 'name' => 'Test', + 'active' => false, + 'type' => 'link', + 'classes' => '', + 'unread' => 0, + 'default' => true, + 'app' => 'test', + ]], ['navigations' => [ 'navigation' => [ ['route' => 'test.page.index', 'name' => 'Test'] @@ -388,23 +291,17 @@ public static function providesNavigationConfig(): array { ]] ], 'minimalistic-settings' => [ - array_merge( - ['profile' => $defaults['profile']], - ['accessibility_settings' => $defaults['accessibility_settings']], - ['settings' => $defaults['settings']], - ['test' => [ - 'id' => 'test', - 'order' => 100, - 'href' => '/apps/test/', - 'icon' => '/apps/test/img/app.svg', - 'name' => 'Test', - 'active' => false, - 'type' => 'settings', - 'classes' => '', - 'unread' => 0, - ]], - ['logout' => $defaults['logout']] - ), + ['test' => [ + 'id' => 'test', + 'order' => 100, + 'href' => '/apps/test/', + 'icon' => '/apps/test/img/app.svg', + 'name' => 'Test', + 'active' => false, + 'type' => 'settings', + 'classes' => '', + 'unread' => 0, + ]], ['navigations' => [ 'navigation' => [ ['route' => 'test.page.index', 'name' => 'Test', 'type' => 'settings'] @@ -412,38 +309,32 @@ public static function providesNavigationConfig(): array { ]] ], 'with-multiple' => [ - array_merge( - ['profile' => $defaults['profile']], - ['accessibility_settings' => $defaults['accessibility_settings']], - ['settings' => $defaults['settings']], - ['test' => [ - 'id' => 'test', - 'order' => 100, + ['test' => [ + 'id' => 'test', + 'order' => 100, + 'href' => '/apps/test/', + 'icon' => '/apps/test/img/app.svg', + 'name' => 'Test', + 'active' => false, + 'type' => 'link', + 'classes' => '', + 'unread' => 0, + 'default' => false, + 'app' => 'test', + ], + 'test1' => [ + 'id' => 'test1', + 'order' => 50, 'href' => '/apps/test/', 'icon' => '/apps/test/img/app.svg', - 'name' => 'Test', + 'name' => 'Other test', 'active' => false, 'type' => 'link', 'classes' => '', 'unread' => 0, - 'default' => false, + 'default' => true, // because of order 'app' => 'test', - ], - 'test1' => [ - 'id' => 'test1', - 'order' => 50, - 'href' => '/apps/test/', - 'icon' => '/apps/test/img/app.svg', - 'name' => 'Other test', - 'active' => false, - 'type' => 'link', - 'classes' => '', - 'unread' => 0, - 'default' => true, // because of order - 'app' => 'test', - ]], - ['logout' => $defaults['logout']] - ), + ]], ['navigations' => [ 'navigation' => [ ['route' => 'test.page.index', 'name' => 'Test'], @@ -452,25 +343,19 @@ public static function providesNavigationConfig(): array { ]] ], 'admin' => [ - array_merge( - ['profile' => $defaults['profile']], - $adminSettings, - $apps, - ['test' => [ - 'id' => 'test', - 'order' => 100, - 'href' => '/apps/test/', - 'icon' => '/apps/test/img/app.svg', - 'name' => 'Test', - 'active' => false, - 'type' => 'link', - 'classes' => '', - 'unread' => 0, - 'default' => true, - 'app' => 'test', - ]], - ['logout' => $defaults['logout']] - ), + ['test' => [ + 'id' => 'test', + 'order' => 100, + 'href' => '/apps/test/', + 'icon' => '/apps/test/img/app.svg', + 'name' => 'Test', + 'active' => false, + 'type' => 'link', + 'classes' => '', + 'unread' => 0, + 'default' => true, + 'app' => 'test', + ]], ['navigations' => [ 'navigation' => [ ['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index', 'name' => 'Test'] @@ -479,12 +364,7 @@ public static function providesNavigationConfig(): array { true ], 'no name' => [ - array_merge( - ['profile' => $defaults['profile']], - $adminSettings, - $apps, - ['logout' => $defaults['logout']] - ), + [], // nothing because the entry is not added because it has no name ['navigations' => [ 'navigation' => [ ['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index'] @@ -493,12 +373,13 @@ public static function providesNavigationConfig(): array { true ], 'no admin' => [ - $defaults, + [], // nothing because user is not an admin ['navigations' => [ 'navigation' => [ ['@attributes' => ['role' => 'admin'], 'route' => 'test.page.index', 'name' => 'Test'] ], ]], + false, ] ]; } @@ -567,9 +448,6 @@ function (string $userId, string $appName, string $key, mixed $default = '') use ->with($user) ->willReturn(['test']); $this->groupManager->expects($this->any())->method('isAdmin')->willReturn(false); - $subadmin = $this->createMock(SubAdmin::class); - $subadmin->expects($this->any())->method('isSubAdmin')->with($user)->willReturn(false); - $this->groupManager->expects($this->any())->method('getSubAdmin')->willReturn($subadmin); $this->navigationManager->clear(); $this->dispatcher->expects($this->once()) From 848b521d3d8e15928b7c1054912ac175be17d211 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Sun, 7 Jun 2026 11:08:46 +0200 Subject: [PATCH 2/2] fix(NavigationManager): resolve entries only when needed The `init` method previously contained two different logics: 1. It set up the internal state of default apps and app order 2. It resolved the app navigation entries The 1. is needed before `add` can be called, so it was always called by the `add` method, but this also resolved all appinfo.xml entries on the first `add` call even if never used. The 2. is only needed when the navigations are actually fetched. This splits the logic into two functions: - `init` for the bare initialization - `resolveAppNavigationEntries` for resolving the entries when requesting to output them. This should give a small performance improvement for API calls and fixes a problem when navigations are added before all apps are registered. Signed-off-by: Ferdinand Thiessen --- cypress/e2e/core/header_app-menu.cy.ts | 47 +++++++++++++++++--- lib/private/NavigationManager.php | 60 ++++++++++++++++++-------- 2 files changed, 82 insertions(+), 25 deletions(-) diff --git a/cypress/e2e/core/header_app-menu.cy.ts b/cypress/e2e/core/header_app-menu.cy.ts index aad8a816678b3..2452e2a19486a 100644 --- a/cypress/e2e/core/header_app-menu.cy.ts +++ b/cypress/e2e/core/header_app-menu.cy.ts @@ -12,12 +12,10 @@ const getAppMenu = () => getNextcloudHeader().find('.app-menu') // the next-best stable selectors. const getWaffleTrigger = () => getAppMenu().find('.app-menu__waffle') -describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => { - beforeEach(() => { - clearState() - }) +before(clearState) - describe('Open and click', () => { +describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => { + describe('Normal user', () => { beforeEach(() => { cy.createRandomUser().then(($user) => { cy.login($user) @@ -25,7 +23,7 @@ describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => { }) }) - it('opens the popover and navigates when a tile is clicked', () => { + it('Open and click opens the popover and navigates when a tile is clicked', () => { getWaffleTrigger().click() cy.get('.app-menu__popover').should('be.visible') getWaffleTrigger().should('have.attr', 'aria-expanded', 'true') @@ -39,9 +37,16 @@ describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => { cy.location('pathname').should('include', '/apps/') }) }) + + it('has all correct app navigation items', () => { + waffleMenuShouldContainApps([ + { name: 'Files', href: '/apps/files' }, + { name: 'Dashboard', href: '/apps/dashboard' }, + ]) + }) }) - describe('Admin gating: "More apps" tile', () => { + describe('Admin', () => { const admin = new User('admin', 'admin') beforeEach(() => { @@ -54,5 +59,33 @@ describe('Header: App menu (waffle launcher)', { testIsolation: true }, () => { cy.get('.app-menu__popover').should('be.visible') cy.findByRole('menuitem', { name: 'More apps' }).should('be.visible') }) + + it('has all correct app navigation items', () => { + waffleMenuShouldContainApps([ + { name: 'Files', href: '/apps/files' }, + { name: 'Dashboard', href: '/apps/dashboard' }, + { name: 'Appstore', href: '/settings/apps' }, + ]) + }) }) }) + +/** + * Check that the waffle menu contains the given apps, by name and href. + * + * @param apps - The apps that should be present in the waffle menu, with their expected name and href. + */ +function waffleMenuShouldContainApps(apps: { name: string, href: string }[]) { + getWaffleTrigger().click() + getWaffleTrigger().should('have.attr', 'aria-expanded', 'true') + cy.findByRole('menu', { name: 'Apps' }).should('be.visible') + + cy.findAllByRole('menuitem') + .then((items) => { + apps.forEach((app) => { + const item = items.toArray().find((i) => i.textContent?.includes(app.name)) + expect(item, `App menu should contain ${app.name}`).to.exist + expect(item?.getAttribute('href')).to.match(new RegExp(`${app.href}(\\?.+|/?$)`)) + }) + }) +} diff --git a/lib/private/NavigationManager.php b/lib/private/NavigationManager.php index f5a9d995c7b25..1ef7d4471748b 100644 --- a/lib/private/NavigationManager.php +++ b/lib/private/NavigationManager.php @@ -36,6 +36,8 @@ class NavigationManager implements INavigationManager { protected bool $init = false; /** User defined app order (cached for the `add` function) */ private ?array $customAppOrder = null; + /** List of loaded app info */ + private array $loadedAppInfo = []; public function __construct( protected IAppManager $appManager, @@ -55,7 +57,7 @@ public function add(array|callable $entry): void { $this->closureEntries[] = $entry; return; } - $this->init(false); + $this->init(); $id = $entry['id']; @@ -98,7 +100,7 @@ private function updateDefaultEntries(): void { #[Override] public function getAll(string $type = 'link'): array { - $this->init(); + $this->resolveAppNavigationEntries(); $result = $this->entries; if ($type !== 'all') { @@ -180,7 +182,16 @@ public function getActiveEntry(): ?string { return $this->activeEntry; } - private function init(bool $resolveClosures = true): void { + /** + * Initialize the internal state. + * This loads the default app mapping and user mapping for app ordering. + */ + private function init(): void { + if ($this->init) { + return; + } + $this->init = true; + if ($this->customAppOrder === null) { if ($this->userSession->isLoggedIn()) { $user = $this->userSession->getUser(); @@ -189,21 +200,23 @@ private function init(bool $resolveClosures = true): void { $this->customAppOrder = []; } } + } - if ($resolveClosures) { - while ($c = array_pop($this->closureEntries)) { - $this->add($c()); - } + /** + * Resolve the app navigation entries from closures and info.xml files. + */ + private function resolveAppNavigationEntries(): void { + // Resolve app navigation closures + while ($c = array_pop($this->closureEntries)) { + $this->add($c()); } - if ($this->init) { - return; + // Resolve dynamically added navigation entries via event listeners + if ($this->loadedAppInfo === []) { + $this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent()); } - $this->init = true; - - $l = $this->l10nFac->get('lib'); - $this->eventDispatcher->dispatchTyped(new LoadAdditionalEntriesEvent()); + // Resolve classic info.xml based navigation entries if ($this->userSession->isLoggedIn()) { $user = $this->userSession->getUser(); $apps = $this->appManager->getEnabledAppsForUser($user); @@ -212,6 +225,11 @@ private function init(bool $resolveClosures = true): void { } foreach ($apps as $app) { + // skip already loaded apps + if (in_array($app, $this->loadedAppInfo)) { + continue; + } + // load plugins and collections from info.xml $info = $this->appManager->getAppInfo($app); if (!isset($info['navigations']['navigation'])) { @@ -230,7 +248,6 @@ private function init(bool $resolveClosures = true): void { if ($role === 'admin' && !$this->isAdmin()) { continue; } - $l = $this->l10nFac->get($app); $id = $nav['id'] ?? $app . ($key === 0 ? '' : $key); $order = $nav['order'] ?? 100; $type = $nav['type']; @@ -249,7 +266,14 @@ private function init(bool $resolveClosures = true): void { if ($icon === null) { $icon = $this->urlGenerator->imagePath('core', 'places/default-app-icon.svg'); } + if ($type === 'link' && $route === '') { + // This means either the route is invalid in the info.xml or the app was not year loaded by the router + $this->logger->debug('Missing or invalid navigation route for app ' . $app, ['entry' => $nav]); + continue; + } + $l = $this->l10nFac->get($app); + $this->loadedAppInfo[] = $app; $this->add(array_merge([ // Navigation id 'id' => $id, @@ -287,13 +311,13 @@ public function setUnreadCounter(string $id, int $unreadCounter): void { #[Override] public function get(string $id): ?array { - $this->init(); + $this->resolveAppNavigationEntries(); return $this->entries[$id]; } #[Override] public function getDefaultEntryIdForUser(?IUser $user = null, bool $withFallbacks = true): string { - $this->init(); + $this->resolveAppNavigationEntries(); // Disable fallbacks here, as we need to override them with the user defaults if none are configured. $defaultEntryIds = $this->getDefaultEntryIds(false); @@ -335,7 +359,7 @@ public function getDefaultEntryIdForUser(?IUser $user = null, bool $withFallback #[Override] public function getDefaultEntryIds(bool $withFallbacks = true): array { - $this->init(); + $this->resolveAppNavigationEntries(); $storedIds = explode(',', $this->config->getSystemValueString('defaultapp', $withFallbacks ? 'dashboard,files' : '')); $ids = []; $entryIds = array_keys($this->entries); @@ -349,7 +373,7 @@ public function getDefaultEntryIds(bool $withFallbacks = true): array { #[Override] public function setDefaultEntryIds(array $ids): void { - $this->init(); + $this->resolveAppNavigationEntries(); $entryIds = array_keys($this->entries); foreach ($ids as $id) {