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 <opensource@fthiessen.de>

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,
+		);
 	}
 }

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<int, list<\OCP\Settings\ISettings>> $settings
+	 * @param list<\OCP\Settings\ISettings> $settings
+	 * @param list<DeclarativeSettingsFormSchemaWithValues> $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 .= '<div id="' . $setting['app'] . '_' . $setting['id'] . '"></div>';
 			}
 		}
 		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) => '<div id="' . $app . '_' . $id . '"></div>', $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);
 }

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,
+		);
 	}
 }

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')

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<int, list<ISettings>> The array of admin settings there admin delegation is allowed.
+	 * @return array<int, list<ISettings>> 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;