From b5256181a49813063a8e6490316f7eed3a4341d5 Mon Sep 17 00:00:00 2001 From: Ferdinand Thiessen Date: Fri, 27 Sep 2024 21:40:30 +0200 Subject: [PATCH] fix(settings): Sort all settings - incl declarative settings - by priority Previously declarative settings were sorted by priority but behind the "native" settings, this is now fixed, meaning a declarative setting with higher priority than an `ISetting` will be correctly rendered before that `ISetting` in the settings list. Signed-off-by: Ferdinand Thiessen --- .../Controller/AdminSettingsController.php | 25 ++------ .../lib/Controller/CommonSettingsTrait.php | 61 ++++++++++++------- .../Controller/PersonalSettingsController.php | 15 ++--- .../AdminSettingsControllerTest.php | 50 +++++++-------- lib/public/Settings/IManager.php | 2 +- 5 files changed, 73 insertions(+), 80 deletions(-) diff --git a/apps/settings/lib/Controller/AdminSettingsController.php b/apps/settings/lib/Controller/AdminSettingsController.php index 25338f64326..2b731c5cdde 100644 --- a/apps/settings/lib/Controller/AdminSettingsController.php +++ b/apps/settings/lib/Controller/AdminSettingsController.php @@ -5,7 +5,6 @@ */ namespace OCA\Settings\Controller; -use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException; use OCP\AppFramework\Controller; use OCP\AppFramework\Http\Attribute\NoAdminRequired; use OCP\AppFramework\Http\Attribute\NoCSRFRequired; @@ -16,7 +15,6 @@ use OCP\Group\ISubAdmin; use OCP\IGroupManager; use OCP\INavigationManager; use OCP\IRequest; -use OCP\IUser; use OCP\IUserSession; use OCP\Settings\IDeclarativeManager; use OCP\Settings\IManager as ISettingsManager; @@ -49,27 +47,14 @@ class AdminSettingsController extends Controller { /** * @NoSubAdminRequired * We are checking the permissions in the getSettings method. If there is no allowed - * settings for the given section. The user will be gretted by an error message. + * settings for the given section. The user will be greeted by an error message. */ #[NoAdminRequired] #[NoCSRFRequired] public function index(string $section): TemplateResponse { - return $this->getIndexResponse('admin', $section); - } - - /** - * @param string $section - * @return array - */ - protected function getSettings($section) { - /** @var IUser $user */ - $user = $this->userSession->getUser(); - $settings = $this->settingsManager->getAllowedAdminSettings($section, $user); - $declarativeFormIDs = $this->declarativeSettingsManager->getFormIDs($user, 'admin', $section); - if (empty($settings) && empty($declarativeFormIDs)) { - throw new NotAdminException("Logged in user doesn't have permission to access these settings."); - } - $formatted = $this->formatSettings($settings); - return $formatted; + return $this->getIndexResponse( + 'admin', + $section, + ); } } diff --git a/apps/settings/lib/Controller/CommonSettingsTrait.php b/apps/settings/lib/Controller/CommonSettingsTrait.php index dd307b67fab..26cbadd76a1 100644 --- a/apps/settings/lib/Controller/CommonSettingsTrait.php +++ b/apps/settings/lib/Controller/CommonSettingsTrait.php @@ -6,6 +6,8 @@ namespace OCA\Settings\Controller; +use InvalidArgumentException; +use OC\AppFramework\Middleware\Security\Exceptions\NotAdminException; use OCA\Settings\AppInfo\Application; use OCP\AppFramework\Http\TemplateResponse; use OCP\AppFramework\Services\IInitialState; @@ -21,7 +23,8 @@ use OCP\Settings\ISettings; use OCP\Util; /** - * @psalm-import-type DeclarativeSettingsFormField from IDeclarativeSettingsForm + * @psalm-import-type DeclarativeSettingsFormSchemaWithValues from IDeclarativeSettingsForm + * @psalm-import-type DeclarativeSettingsFormSchemaWithoutValues from IDeclarativeSettingsForm */ trait CommonSettingsTrait { @@ -106,16 +109,26 @@ trait CommonSettingsTrait { } /** - * @param array> $settings + * @param list<\OCP\Settings\ISettings> $settings + * @param list $declarativeSettings * @return array{content: string} */ - private function formatSettings(array $settings): array { + private function formatSettings(array $settings, array $declarativeSettings): array { + $settings = array_merge($settings, $declarativeSettings); + + usort($settings, function ($first, $second) { + $priorityOne = $first instanceof ISettings ? $first->getPriority() : $first['priority']; + $priorityTwo = $second instanceof ISettings ? $second->getPriority() : $second['priority']; + return $priorityOne - $priorityTwo; + }); + $html = ''; - foreach ($settings as $prioritizedSettings) { - foreach ($prioritizedSettings as $setting) { - /** @var ISettings $setting */ + foreach ($settings as $setting) { + if ($setting instanceof ISettings) { $form = $setting->getForm(); $html .= $form->renderAs('')->render(); + } else { + $html .= '
'; } } return ['content' => $html]; @@ -125,34 +138,38 @@ trait CommonSettingsTrait { * @psalm-param 'admin'|'personal' $type */ private function getIndexResponse(string $type, string $section): TemplateResponse { + $user = $this->userSession->getUser(); + assert($user !== null, 'No user logged in for settings'); + + $this->declarativeSettingsManager->loadSchemas(); + $declarativeSettings = $this->declarativeSettingsManager->getFormsWithValues($user, $type, $section); + if ($type === 'personal') { + $settings = array_values($this->settingsManager->getPersonalSettings($section)); if ($section === 'theming') { $this->navigationManager->setActiveEntry('accessibility_settings'); } else { $this->navigationManager->setActiveEntry('settings'); } } elseif ($type === 'admin') { + $settings = array_values($this->settingsManager->getAllowedAdminSettings($section, $user)); + if (empty($settings) && empty($declarativeSettings)) { + throw new NotAdminException('Logged in user does not have permission to access these settings.'); + } $this->navigationManager->setActiveEntry('admin_settings'); + } else { + throw new InvalidArgumentException('$type must be either "admin" or "personal"'); } - $this->declarativeSettingsManager->loadSchemas(); - - $templateParams = []; - $templateParams = array_merge($templateParams, $this->getNavigationParameters($type, $section)); - $templateParams = array_merge($templateParams, $this->getSettings($section)); - - /** @psalm-suppress PossiblyNullArgument */ - $declarativeFormIDs = $this->declarativeSettingsManager->getFormIDs($this->userSession->getUser(), $type, $section); - if (!empty($declarativeFormIDs)) { - foreach ($declarativeFormIDs as $app => $ids) { - /** @psalm-suppress PossiblyUndefinedArrayOffset */ - $templateParams['content'] .= join(array_map(fn (string $id) => '
', $ids)); - } + if (!empty($declarativeSettings)) { Util::addScript(Application::APP_ID, 'declarative-settings-forms'); - /** @psalm-suppress PossiblyNullArgument */ - $this->initialState->provideInitialState('declarative-settings-forms', $this->declarativeSettingsManager->getFormsWithValues($this->userSession->getUser(), $type, $section)); + $this->initialState->provideInitialState('declarative-settings-forms', $declarativeSettings); } + $settings = array_merge(...$settings); + $templateParams = $this->formatSettings($settings, $declarativeSettings); + $templateParams = array_merge($templateParams, $this->getNavigationParameters($type, $section)); + $activeSection = $this->settingsManager->getSection($type, $section); if ($activeSection) { $templateParams['pageTitle'] = $activeSection->getName(); @@ -162,6 +179,4 @@ trait CommonSettingsTrait { return new TemplateResponse('settings', 'settings/frame', $templateParams); } - - abstract protected function getSettings($section); } diff --git a/apps/settings/lib/Controller/PersonalSettingsController.php b/apps/settings/lib/Controller/PersonalSettingsController.php index 1a31a20eb04..0a87181c7d7 100644 --- a/apps/settings/lib/Controller/PersonalSettingsController.php +++ b/apps/settings/lib/Controller/PersonalSettingsController.php @@ -50,16 +50,9 @@ class PersonalSettingsController extends Controller { #[NoAdminRequired] #[NoCSRFRequired] public function index(string $section): TemplateResponse { - return $this->getIndexResponse('personal', $section); - } - - /** - * @param string $section - * @return array - */ - protected function getSettings($section) { - $settings = $this->settingsManager->getPersonalSettings($section); - $formatted = $this->formatSettings($settings); - return $formatted; + return $this->getIndexResponse( + 'personal', + $section, + ); } } diff --git a/apps/settings/tests/Controller/AdminSettingsControllerTest.php b/apps/settings/tests/Controller/AdminSettingsControllerTest.php index 60ba5e0750e..c8ba8edb968 100644 --- a/apps/settings/tests/Controller/AdminSettingsControllerTest.php +++ b/apps/settings/tests/Controller/AdminSettingsControllerTest.php @@ -14,6 +14,7 @@ use OCP\IGroupManager; use OCP\INavigationManager; use OCP\IRequest; use OCP\IUser; +use OCP\IUserManager; use OCP\IUserSession; use OCP\Settings\IDeclarativeManager; use OCP\Settings\IManager; @@ -29,26 +30,17 @@ use Test\TestCase; */ class AdminSettingsControllerTest extends TestCase { - /** @var AdminSettingsController */ - private $adminSettingsController; - /** @var IRequest|MockObject */ - private $request; - /** @var INavigationManager|MockObject */ - private $navigationManager; - /** @var IManager|MockObject */ - private $settingsManager; - /** @var IUserSession|MockObject */ - private $userSession; - /** @var IGroupManager|MockObject */ - private $groupManager; - /** @var ISubAdmin|MockObject */ - private $subAdmin; - /** @var IDeclarativeManager|MockObject */ - private $declarativeSettingsManager; - /** @var IInitialState|MockObject */ - private $initialState; - /** @var string */ - private $adminUid = 'lololo'; + private IRequest&MockObject $request; + private INavigationManager&MockObject $navigationManager; + private IManager&MockObject $settingsManager; + private IUserSession&MockObject $userSession; + private IGroupManager&MockObject $groupManager; + private ISubAdmin&MockObject $subAdmin; + private IDeclarativeManager&MockObject $declarativeSettingsManager; + private IInitialState&MockObject $initialState; + + private string $adminUid = 'lololo'; + private AdminSettingsController $adminSettingsController; protected function setUp(): void { parent::setUp(); @@ -74,15 +66,17 @@ class AdminSettingsControllerTest extends TestCase { $this->initialState, ); - $user = \OC::$server->getUserManager()->createUser($this->adminUid, 'mylongrandompassword'); + $user = \OCP\Server::get(IUserManager::class)->createUser($this->adminUid, 'mylongrandompassword'); \OC_User::setUserId($user->getUID()); - \OC::$server->getGroupManager()->createGroup('admin')->addUser($user); + \OCP\Server::get(IGroupManager::class)->createGroup('admin')->addUser($user); } protected function tearDown(): void { - \OC::$server->getUserManager()->get($this->adminUid)->delete(); + \OCP\Server::get(IUserManager::class) + ->get($this->adminUid) + ->delete(); \OC_User::setUserId(null); - \OC::$server->getUserSession()->setUser(null); + \OCP\Server::get(IUserSession::class)->setUser(null); parent::tearDown(); } @@ -101,6 +95,12 @@ class AdminSettingsControllerTest extends TestCase { ->method('isSubAdmin') ->with($user) ->willReturn(false); + + $form = new TemplateResponse('settings', 'settings/empty'); + $setting = $this->createMock(ServerDevNotice::class); + $setting->expects(self::any()) + ->method('getForm') + ->willReturn($form); $this->settingsManager ->expects($this->once()) ->method('getAdminSections') @@ -113,7 +113,7 @@ class AdminSettingsControllerTest extends TestCase { ->expects($this->once()) ->method('getAllowedAdminSettings') ->with('test') - ->willReturn([5 => $this->createMock(ServerDevNotice::class)]); + ->willReturn([5 => [$setting]]); $this->declarativeSettingsManager ->expects($this->any()) ->method('getFormIDs') diff --git a/lib/public/Settings/IManager.php b/lib/public/Settings/IManager.php index d73e4055f31..0bb1a396671 100644 --- a/lib/public/Settings/IManager.php +++ b/lib/public/Settings/IManager.php @@ -89,7 +89,7 @@ interface IManager { /** * Returns a list of admin settings that the given user can use for the give section * - * @return array> The array of admin settings there admin delegation is allowed. + * @return array> List of admin-settings the user has access to, with priority as key. * @since 23.0.0 */ public function getAllowedAdminSettings(string $section, IUser $user): array; -- 2.39.5