fix(settings): Do not use `null` on `string` parameter for sharing disclaimer

Signed-off-by: Ferdinand Thiessen <opensource@fthiessen.de>

apps/settings/lib/Settings/Admin/Sharing.php

@@ -62,7 +62,7 @@ class Sharing implements IDelegatedSettings {
 			'enforceExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_enforce_expire_date'),
 			'excludeGroups' => $this->config->getAppValue('core', 'shareapi_exclude_groups', 'no'),
 			'excludeGroupsList' => json_decode($excludedGroups, true) ?? [],
-			'publicShareDisclaimerText' => $this->config->getAppValue('core', 'shareapi_public_link_disclaimertext', null),
+			'publicShareDisclaimerText' => $this->config->getAppValue('core', 'shareapi_public_link_disclaimertext'),
 			'enableLinkPasswordByDefault' => $this->getHumanBooleanConfig('core', 'shareapi_enable_link_password_by_default'),
 			'defaultPermissions' => (int)$this->config->getAppValue('core', 'shareapi_default_permissions', (string)Constants::PERMISSION_ALL),
 			'defaultInternalExpireDate' => $this->getHumanBooleanConfig('core', 'shareapi_default_internal_expire_date'),

apps/settings/src/components/AdminSettingsSharingForm.vue

@@ -170,7 +170,7 @@
 			<NcCheckboxRadioSwitch type="switch" :checked.sync="publicShareDisclaimerEnabled">
 				{{ t('settings', 'Show disclaimer text on the public link upload page (only shown when the file list is hidden)') }}
 			</NcCheckboxRadioSwitch>
-			<div v-if="typeof settings.publicShareDisclaimerText === 'string'"
+			<div v-if="publicShareDisclaimerEnabled"
 				aria-describedby="settings-sharing-privary-related-disclaimer-hint"
 				class="sharing__sub-section">
 				<NcTextArea class="sharing__input"
@@ -231,7 +231,7 @@ interface IShareSettings {
 	enforceExpireDate: boolean
 	excludeGroups: string
 	excludeGroupsList: string[]
-	publicShareDisclaimerText?: string
+	publicShareDisclaimerText: string
 	enableLinkPasswordByDefault: boolean
 	defaultPermissions: number
 	defaultInternalExpireDate: boolean
@@ -252,8 +252,10 @@ export default defineComponent({
 		SelectSharingPermissions,
 	},
 	data() {
+		const settingsData = loadState<IShareSettings>('settings', 'sharingSettings')
 		return {
-			settingsData: loadState<IShareSettings>('settings', 'sharingSettings'),
+			settingsData,
+			publicShareDisclaimerEnabled: settingsData.publicShareDisclaimerText !== '',
 		}
 	},
 	computed: {
@@ -272,26 +274,24 @@ export default defineComponent({
 				},
 			})
 		},
-		publicShareDisclaimerEnabled: {
-			get() {
-				return typeof this.settingsData.publicShareDisclaimerText === 'string'
 	},
-			set(value) {
-				if (value) {
-					this.settingsData.publicShareDisclaimerText = ''
-				} else {
-					this.onUpdateDisclaimer()
+
+	watch: {
+		publicShareDisclaimerEnabled() {
+			// When disabled we just remove the disclaimer content
+			if (this.publicShareDisclaimerEnabled === false) {
+				this.onUpdateDisclaimer('')
 			}
 		},
 	},
-	},
+
 	methods: {
 		t,
 
-		onUpdateDisclaimer: debounce(function(value?: string) {
+		onUpdateDisclaimer: debounce(function(value: string) {
 			const options = {
 				success() {
-					if (value) {
+					if (value !== '') {
 						showSuccess(t('settings', 'Changed disclaimer text'))
 					} else {
 						showSuccess(t('settings', 'Deleted disclaimer text'))
@@ -301,7 +301,7 @@ export default defineComponent({
 					showError(t('settings', 'Could not set disclaimer text'))
 				},
 			}
-			if (!value) {
+			if (value === '') {
 				window.OCP.AppConfig.deleteKey('core', 'shareapi_public_link_disclaimertext', options)
 			} else {
 				window.OCP.AppConfig.setValue('core', 'shareapi_public_link_disclaimertext', value, options)

apps/settings/tests/Settings/Admin/SharingTest.php

@@ -20,9 +20,9 @@ use Test\TestCase;
 class SharingTest extends TestCase {
 	/** @var Sharing */
 	private $admin;
-	/** @var IConfig */
+	/** @var IConfig&MockObject */
 	private $config;
-	/** @var IL10N|MockObject */
+	/** @var IL10N&MockObject */
 	private $l10n;
 	/** @var IManager|MockObject */
 	private $shareManager;
@@ -35,9 +35,7 @@ class SharingTest extends TestCase {
 
 	protected function setUp(): void {
 		parent::setUp();
-		/** @var IConfig|MockObject */
 		$this->config = $this->getMockBuilder(IConfig::class)->getMock();
-		/** @var IL10N|MockObject */
 		$this->l10n = $this->getMockBuilder(IL10N::class)->getMock();
 
 		/** @var IManager|MockObject */
@@ -82,7 +80,7 @@ class SharingTest extends TestCase {
 				['core', 'shareapi_expire_after_n_days', '7', '7'],
 				['core', 'shareapi_enforce_expire_date', 'no', 'no'],
 				['core', 'shareapi_exclude_groups', 'no', 'no'],
-				['core', 'shareapi_public_link_disclaimertext', null, 'Lorem ipsum'],
+				['core', 'shareapi_public_link_disclaimertext', '', 'Lorem ipsum'],
 				['core', 'shareapi_enable_link_password_by_default', 'no', 'yes'],
 				['core', 'shareapi_default_permissions', (string)Constants::PERMISSION_ALL, Constants::PERMISSION_ALL],
 				['core', 'shareapi_default_internal_expire_date', 'no', 'no'],
@@ -98,15 +96,19 @@ class SharingTest extends TestCase {
 			->willReturn(false);
 
 		$this->appManager->method('isEnabledForUser')->with('files_sharing')->willReturn(false);
+
+		$initialStateCalls = [];
 		$this->initialState
 			->expects($this->exactly(3))
 			->method('provideInitialState')
-			->withConsecutive(
-				['sharingAppEnabled', false],
-				['sharingDocumentation', ''],
-				[
-					'sharingSettings',
-					[
+			->willReturnCallback(function (string $key) use (&$initialStateCalls) {
+				$initialStateCalls[$key] = func_get_args();
+			});
+		
+		$expectedInitialStateCalls = [
+			'sharingAppEnabled' => false,
+			'sharingDocumentation' => '',
+			'sharingSettings' => [
 				'allowGroupSharing' => true,
 				'allowLinks' => true,
 				'allowPublicUpload' => true,
@@ -140,8 +142,7 @@ class SharingTest extends TestCase {
 				'enforceLinksPasswordExcludedGroups' => [],
 				'enforceLinksPasswordExcludedGroupsEnabled' => false,
 			]
-				],
-			);
+		];
 
 		$expected = new TemplateResponse(
 			'settings',
@@ -151,6 +152,7 @@ class SharingTest extends TestCase {
 		);
 
 		$this->assertEquals($expected, $this->admin->getForm());
+		$this->assertEquals(sort($expectedInitialStateCalls), sort($initialStateCalls), 'Provided initial state does not match');
 	}
 
 	public function testGetFormWithExcludedGroups(): void {
@@ -175,7 +177,7 @@ class SharingTest extends TestCase {
 				['core', 'shareapi_expire_after_n_days', '7', '7'],
 				['core', 'shareapi_enforce_expire_date', 'no', 'no'],
 				['core', 'shareapi_exclude_groups', 'no', 'yes'],
-				['core', 'shareapi_public_link_disclaimertext', null, 'Lorem ipsum'],
+				['core', 'shareapi_public_link_disclaimertext', '', 'Lorem ipsum'],
 				['core', 'shareapi_enable_link_password_by_default', 'no', 'yes'],
 				['core', 'shareapi_default_permissions', (string)Constants::PERMISSION_ALL, Constants::PERMISSION_ALL],
 				['core', 'shareapi_default_internal_expire_date', 'no', 'no'],
@@ -191,15 +193,19 @@ class SharingTest extends TestCase {
 			->willReturn(false);
 
 		$this->appManager->method('isEnabledForUser')->with('files_sharing')->willReturn(true);
+
+		$initialStateCalls = [];
 		$this->initialState
 			->expects($this->exactly(3))
 			->method('provideInitialState')
-			->withConsecutive(
-				['sharingAppEnabled', true],
-				['sharingDocumentation', ''],
-				[
-					'sharingSettings',
-					[
+			->willReturnCallback(function (string $key) use (&$initialStateCalls) {
+				$initialStateCalls[$key] = func_get_args();
+			});
+
+		$expectedInitialStateCalls = [
+			'sharingAppEnabled' => true,
+			'sharingDocumentation' => '',
+			'sharingSettings' => [
 				'allowGroupSharing' => true,
 				'allowLinks' => true,
 				'allowPublicUpload' => true,
@@ -232,9 +238,8 @@ class SharingTest extends TestCase {
 				'onlyShareWithGroupMembersExcludeGroupList' => [],
 				'enforceLinksPasswordExcludedGroups' => [],
 				'enforceLinksPasswordExcludedGroupsEnabled' => false,
-					]
 			],
-			);
+		];
 
 		$expected = new TemplateResponse(
 			'settings',
@@ -244,6 +249,7 @@ class SharingTest extends TestCase {
 		);
 
 		$this->assertEquals($expected, $this->admin->getForm());
+		$this->assertEquals(sort($expectedInitialStateCalls), sort($initialStateCalls), 'Provided initial state does not match');
 	}
 
 	public function testGetSection(): void {

build/psalm-baseline.xml

@@ -1041,11 +1041,6 @@
       <code><![CDATA[isReady]]></code>
     </UndefinedInterfaceMethod>
   </file>
-  <file src="apps/settings/lib/Settings/Admin/Sharing.php">
-    <NullArgument>
-      <code><![CDATA[null]]></code>
-    </NullArgument>
-  </file>
   <file src="apps/sharebymail/lib/ShareByMailProvider.php">
     <InvalidArgument>
       <code><![CDATA[$share->getId()]]></code>