diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php index 73ef78c7a8e8c..d6c89990b0284 100644 --- a/apps/theming/lib/Controller/ThemingController.php +++ b/apps/theming/lib/Controller/ThemingController.php @@ -22,6 +22,7 @@ use OCP\AppFramework\Http\FileDisplayResponse; use OCP\AppFramework\Http\JSONResponse; use OCP\AppFramework\Http\NotFoundResponse; +use OCP\AppFramework\Services\IAppConfig; use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; use OCP\IConfig; @@ -40,34 +41,19 @@ class ThemingController extends Controller { public const VALID_UPLOAD_KEYS = ['header', 'logo', 'logoheader', 'background', 'favicon']; - private ThemingDefaults $themingDefaults; - private IL10N $l10n; - private IConfig $config; - private IURLGenerator $urlGenerator; - private IAppManager $appManager; - private ImageManager $imageManager; - private ThemesService $themesService; - public function __construct( $appName, IRequest $request, - IConfig $config, - ThemingDefaults $themingDefaults, - IL10N $l, - IURLGenerator $urlGenerator, - IAppManager $appManager, - ImageManager $imageManager, - ThemesService $themesService + private IConfig $config, + private IAppConfig $appConfig, + private ThemingDefaults $themingDefaults, + private IL10N $l10n, + private IURLGenerator $urlGenerator, + private IAppManager $appManager, + private ImageManager $imageManager, + private ThemesService $themesService, ) { parent::__construct($appName, $request); - - $this->themingDefaults = $themingDefaults; - $this->l10n = $l; - $this->config = $config; - $this->urlGenerator = $urlGenerator; - $this->appManager = $appManager; - $this->imageManager = $imageManager; - $this->themesService = $themesService; } /** @@ -80,6 +66,7 @@ public function __construct( public function updateStylesheet($setting, $value) { $value = trim($value); $error = null; + $saved = false; switch ($setting) { case 'name': if (strlen($value) > 250) { @@ -118,16 +105,25 @@ public function updateStylesheet($setting, $value) { case 'primary_color': if (!preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $value)) { $error = $this->l10n->t('The given color is invalid'); + } else { + $this->appConfig->setAppValueString('primary_color', $value); + $saved = true; } break; case 'background_color': if (!preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $value)) { $error = $this->l10n->t('The given color is invalid'); + } else { + $this->appConfig->setAppValueString('background_color', $value); + $saved = true; } break; case 'disable-user-theming': - if ($value !== 'yes' && $value !== 'no') { + if (!in_array($value, ['yes', 'true', 'no', 'false'])) { $error = $this->l10n->t('Disable-user-theming should be true or false'); + } else { + $this->appConfig->setAppValueBool('disable-user-theming', $value === 'yes' || $value === 'true'); + $saved = true; } break; } @@ -140,7 +136,9 @@ public function updateStylesheet($setting, $value) { ], Http::STATUS_BAD_REQUEST); } - $this->themingDefaults->set($setting, $value); + if (!$saved) { + $this->themingDefaults->set($setting, $value); + } return new DataResponse([ 'data' => [ diff --git a/apps/theming/lib/Migration/SeparatePrimaryColorAndBackground.php b/apps/theming/lib/Migration/SeparatePrimaryColorAndBackground.php index cfe475e01decb..4fe26d0ba89fc 100644 --- a/apps/theming/lib/Migration/SeparatePrimaryColorAndBackground.php +++ b/apps/theming/lib/Migration/SeparatePrimaryColorAndBackground.php @@ -10,14 +10,14 @@ namespace OCA\Theming\Migration; use OCA\Theming\AppInfo\Application; -use OCP\IConfig; +use OCP\IAppConfig; use OCP\IDBConnection; use OCP\Migration\IOutput; class SeparatePrimaryColorAndBackground implements \OCP\Migration\IRepairStep { public function __construct( - private IConfig $config, + private IAppConfig $appConfig, private IDBConnection $connection, ) { } @@ -27,25 +27,24 @@ public function getName() { } public function run(IOutput $output) { - $defaultColor = $this->config->getAppValue(Application::APP_ID, 'color', ''); + $defaultColor = $this->appConfig->getValueString(Application::APP_ID, 'color', ''); if ($defaultColor !== '') { // Restore legacy value into new field - $this->config->setAppValue(Application::APP_ID, 'background_color', $defaultColor); - $this->config->setAppValue(Application::APP_ID, 'primary_color', $defaultColor); + $this->appConfig->setValueString(Application::APP_ID, 'background_color', $defaultColor); + $this->appConfig->setValueString(Application::APP_ID, 'primary_color', $defaultColor); // Delete legacy field - $this->config->deleteAppValue(Application::APP_ID, 'color'); + $this->appConfig->deleteKey(Application::APP_ID, 'color'); // give some feedback $output->info('Global primary color restored'); } // This can only be executed once because `background_color` is again used with Nextcloud 30, // so this part only works when updating -> Nextcloud 29 -> 30 - $migrated = $this->config->getAppValue('theming', 'nextcloud_30_migration', 'false') === 'true'; - if ($migrated) { + if ($this->appConfig->getValueBool('theming', 'nextcloud_30_migration')) { return; } - $userThemingEnabled = $this->config->getAppValue('theming', 'disable-user-theming', 'no') !== 'yes'; + $userThemingEnabled = $this->appConfig->getValueBool('theming', 'disable-user-theming'); if ($userThemingEnabled) { $output->info('Restoring user primary color'); // For performance let the DB handle this @@ -59,6 +58,6 @@ public function run(IOutput $output) { $qb->executeStatement(); $output->info('Primary color of users restored'); } - $this->config->setAppValue('theming', 'nextcloud_30_migration', 'true'); + $this->appConfig->setValueBool('theming', 'nextcloud_30_migration', true); } } diff --git a/apps/theming/lib/ThemingDefaults.php b/apps/theming/lib/ThemingDefaults.php index f43e96a883051..88404c9da5874 100644 --- a/apps/theming/lib/ThemingDefaults.php +++ b/apps/theming/lib/ThemingDefaults.php @@ -11,6 +11,7 @@ use OCP\App\IAppManager; use OCP\Files\NotFoundException; use OCP\Files\SimpleFS\ISimpleFile; +use OCP\IAppConfig; use OCP\ICacheFactory; use OCP\IConfig; use OCP\IL10N; @@ -39,6 +40,7 @@ class ThemingDefaults extends \OC_Defaults { */ public function __construct( private IConfig $config, + private IAppConfig $appConfig, private IL10N $l, private IUserSession $userSession, private IURLGenerator $urlGenerator, @@ -221,7 +223,7 @@ public function getColorBackground(): string { */ public function getDefaultColorPrimary(): string { // try admin color - $defaultColor = $this->config->getAppValue(Application::APP_ID, 'primary_color', ''); + $defaultColor = $this->appConfig->getValueString(Application::APP_ID, 'primary_color', ''); if (preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $defaultColor)) { return $defaultColor; } @@ -234,7 +236,7 @@ public function getDefaultColorPrimary(): string { * Default background color only taking admin setting into account */ public function getDefaultColorBackground(): string { - $defaultColor = $this->config->getAppValue(Application::APP_ID, 'background_color', ''); + $defaultColor = $this->appConfig->getValueString(Application::APP_ID, 'background_color'); if (preg_match('/^\#([0-9a-f]{3}|[0-9a-f]{6})$/i', $defaultColor)) { return $defaultColor; } @@ -344,7 +346,7 @@ public function getScssVariables() { $variables['image-login-background'] = "url('".$this->imageManager->getImageUrl('background')."')"; $variables['image-login-plain'] = 'false'; - if ($this->config->getAppValue('theming', 'primary_color', '') !== '') { + if ($this->appConfig->getValueString(Application::APP_ID, 'primary_color', '') !== '') { $variables['color-primary'] = $this->getColorPrimary(); $variables['color-primary-text'] = $this->getTextColorPrimary(); $variables['color-primary-element'] = $this->util->elementColor($this->getColorPrimary()); @@ -520,6 +522,6 @@ public function getDefaultTextColorPrimary() { * Has the admin disabled user customization */ public function isUserThemingDisabled(): bool { - return $this->config->getAppValue('theming', 'disable-user-theming', 'no') === 'yes'; + return $this->appConfig->getValueBool(Application::APP_ID, 'disable-user-theming'); } } diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php index 86231e7daf7ec..280bb8df4d173 100644 --- a/apps/theming/tests/Controller/ThemingControllerTest.php +++ b/apps/theming/tests/Controller/ThemingControllerTest.php @@ -13,6 +13,7 @@ use OCP\App\IAppManager; use OCP\AppFramework\Http; use OCP\AppFramework\Http\DataResponse; +use OCP\AppFramework\Services\IAppConfig; use OCP\AppFramework\Utility\ITimeFactory; use OCP\Files\NotFoundException; use OCP\Files\SimpleFS\ISimpleFile; @@ -24,28 +25,23 @@ use Test\TestCase; class ThemingControllerTest extends TestCase { - /** @var IRequest|MockObject */ - private $request; - /** @var IConfig|MockObject */ - private $config; - /** @var ThemingDefaults|MockObject */ - private $themingDefaults; - /** @var IL10N|MockObject */ - private $l10n; - /** @var ThemingController */ - private $themingController; - /** @var IAppManager|MockObject */ - private $appManager; - /** @var ImageManager|MockObject */ - private $imageManager; - /** @var IURLGenerator|MockObject */ - private $urlGenerator; - /** @var ThemesService|MockObject */ - private $themesService; + + private IRequest&MockObject $request; + private IConfig&MockObject $config; + private IAppConfig&MockObject $appConfig; + private ThemingDefaults&MockObject $themingDefaults; + private IL10N&MockObject $l10n; + private IAppManager&MockObject $appManager; + private ImageManager&MockObject $imageManager; + private IURLGenerator&MockObject $urlGenerator; + private ThemesService&MockObject $themesService; + + private ThemingController $themingController; protected function setUp(): void { $this->request = $this->createMock(IRequest::class); $this->config = $this->createMock(IConfig::class); + $this->appConfig = $this->createMock(IAppConfig::class); $this->themingDefaults = $this->createMock(ThemingDefaults::class); $this->l10n = $this->createMock(L10N::class); $this->appManager = $this->createMock(IAppManager::class); @@ -64,6 +60,7 @@ protected function setUp(): void { 'theming', $this->request, $this->config, + $this->appConfig, $this->themingDefaults, $this->l10n, $this->urlGenerator, diff --git a/apps/theming/tests/ThemingDefaultsTest.php b/apps/theming/tests/ThemingDefaultsTest.php index 0dbf4336e8ae3..8edbcbc53c1b5 100644 --- a/apps/theming/tests/ThemingDefaultsTest.php +++ b/apps/theming/tests/ThemingDefaultsTest.php @@ -10,8 +10,8 @@ use OCA\Theming\ThemingDefaults; use OCA\Theming\Util; use OCP\App\IAppManager; -use OCP\Files\IAppData; use OCP\Files\NotFoundException; +use OCP\IAppConfig; use OCP\ICache; use OCP\ICacheFactory; use OCP\IConfig; @@ -20,21 +20,20 @@ use OCP\IURLGenerator; use OCP\IUser; use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; class ThemingDefaultsTest extends TestCase { - /** @var IConfig|\PHPUnit\Framework\MockObject\MockObject */ - private $config; + private IAppConfig&MockObject $appConfig; + private IConfig&MockObject $config; + private \OC_Defaults $defaults; + /** @var IL10N|\PHPUnit\Framework\MockObject\MockObject */ private $l10n; /** @var IUserSession|\PHPUnit\Framework\MockObject\MockObject */ private $userSession; /** @var IURLGenerator|\PHPUnit\Framework\MockObject\MockObject */ private $urlGenerator; - /** @var \OC_Defaults|\PHPUnit\Framework\MockObject\MockObject */ - private $defaults; - /** @var IAppData|\PHPUnit\Framework\MockObject\MockObject */ - private $appData; /** @var ICacheFactory|\PHPUnit\Framework\MockObject\MockObject */ private $cacheFactory; /** @var ThemingDefaults */ @@ -54,6 +53,7 @@ class ThemingDefaultsTest extends TestCase { protected function setUp(): void { parent::setUp(); + $this->appConfig = $this->createMock(IAppConfig::class); $this->config = $this->createMock(IConfig::class); $this->l10n = $this->createMock(IL10N::class); $this->userSession = $this->createMock(IUserSession::class); @@ -72,6 +72,7 @@ protected function setUp(): void { ->willReturn(''); $this->template = new ThemingDefaults( $this->config, + $this->appConfig, $this->l10n, $this->userSession, $this->urlGenerator, @@ -398,25 +399,31 @@ public function testGetShortFooterInvalidPrivacy($invalidPrivacyUrl) { } public function testGetColorPrimaryWithDefault() { - $this->config - ->expects($this->exactly(2)) - ->method('getAppValue') - ->willReturnMap([ - ['theming', 'disable-user-theming', 'no', 'no'], - ['theming', 'primary_color', '', $this->defaults->getColorPrimary()], - ]); + $this->appConfig + ->expects(self::once()) + ->method('getValueBool') + ->with('theming', 'disable-user-theming') + ->willReturn(false); + $this->appConfig + ->expects(self::once()) + ->method('getValueString') + ->with('theming', 'primary_color', '') + ->willReturn($this->defaults->getColorPrimary()); $this->assertEquals($this->defaults->getColorPrimary(), $this->template->getColorPrimary()); } public function testGetColorPrimaryWithCustom() { - $this->config - ->expects($this->exactly(2)) - ->method('getAppValue') - ->willReturnMap([ - ['theming', 'disable-user-theming', 'no', 'no'], - ['theming', 'primary_color', '', '#fff'], - ]); + $this->appConfig + ->expects(self::once()) + ->method('getValueBool') + ->with('theming', 'disable-user-theming') + ->willReturn(false); + $this->appConfig + ->expects(self::once()) + ->method('getValueString') + ->with('theming', 'primary_color', '') + ->willReturn('#fff'); $this->assertEquals('#fff', $this->template->getColorPrimary()); } @@ -424,37 +431,37 @@ public function testGetColorPrimaryWithCustom() { public function dataGetColorPrimary() { return [ 'with fallback default' => [ - 'disableTheming' => 'no', + 'disableTheming' => false, 'primaryColor' => '', 'userPrimaryColor' => '', 'expected' => BackgroundService::DEFAULT_COLOR, ], 'with custom admin primary' => [ - 'disableTheming' => 'no', + 'disableTheming' => false, 'primaryColor' => '#aaa', 'userPrimaryColor' => '', 'expected' => '#aaa', ], 'with custom invalid admin primary' => [ - 'disableTheming' => 'no', + 'disableTheming' => false, 'primaryColor' => 'invalid', 'userPrimaryColor' => '', 'expected' => BackgroundService::DEFAULT_COLOR, ], 'with custom invalid user primary' => [ - 'disableTheming' => 'no', + 'disableTheming' => false, 'primaryColor' => '', 'userPrimaryColor' => 'invalid-name', 'expected' => BackgroundService::DEFAULT_COLOR, ], 'with custom user primary' => [ - 'disableTheming' => 'no', + 'disableTheming' => false, 'primaryColor' => '', 'userPrimaryColor' => '#bbb', 'expected' => '#bbb', ], 'with disabled user theming primary' => [ - 'disableTheming' => 'yes', + 'disableTheming' => true, 'primaryColor' => '#aaa', 'userPrimaryColor' => '#bbb', 'expected' => '#aaa', @@ -465,7 +472,7 @@ public function dataGetColorPrimary() { /** * @dataProvider dataGetColorPrimary */ - public function testGetColorPrimary(string $disableTheming, string $primaryColor, string $userPrimaryColor, string $expected) { + public function testGetColorPrimary(bool $disableTheming, string $primaryColor, string $userPrimaryColor, string $expected) { $user = $this->createMock(IUser::class); $this->userSession->expects($this->any()) ->method('getUser') @@ -473,13 +480,16 @@ public function testGetColorPrimary(string $disableTheming, string $primaryColor $user->expects($this->any()) ->method('getUID') ->willReturn('user'); - $this->config - ->expects($this->any()) - ->method('getAppValue') - ->willReturnMap([ - ['theming', 'disable-user-theming', 'no', $disableTheming], - ['theming', 'primary_color', '', $primaryColor], - ]); + $this->appConfig + ->expects(self::any()) + ->method('getValueBool') + ->with('theming', 'disable-user-theming') + ->willReturn($disableTheming); + $this->appConfig + ->expects(self::any()) + ->method('getValueString') + ->with('theming', 'primary_color', '') + ->willReturn($primaryColor); $this->config ->expects($this->any()) ->method('getUserValue') @@ -699,8 +709,14 @@ public function testGetScssVariables() { ['theming', 'backgroundMime', '', 'jpeg'], ['theming', 'logoheaderMime', '', 'jpeg'], ['theming', 'faviconMime', '', 'jpeg'], - ['theming', 'primary_color', '', $this->defaults->getColorPrimary()], - ['theming', 'primary_color', $this->defaults->getColorPrimary(), $this->defaults->getColorPrimary()], + ]); + + $this->appConfig + ->expects(self::atLeastOnce()) + ->method('getValueString') + ->willReturnMap([ + ['theming', 'primary_color', '', false, $this->defaults->getColorPrimary()], + ['theming', 'primary_color', $this->defaults->getColorPrimary(), false, $this->defaults->getColorPrimary()], ]); $this->util->expects($this->any())->method('invertTextColor')->with($this->defaults->getColorPrimary())->willReturn(false); diff --git a/lib/private/Server.php b/lib/private/Server.php index 01d5bdac0b66f..b0ccb0f4b4d8c 100644 --- a/lib/private/Server.php +++ b/lib/private/Server.php @@ -1167,6 +1167,7 @@ public function __construct($webRoot, \OC\Config $config) { ); return new ThemingDefaults( $c->get(\OCP\IConfig::class), + $c->get(\OCP\IAppConfig::class), $c->getL10N('theming'), $c->get(IUserSession::class), $c->get(IURLGenerator::class),