Fix deleting properties of user settings when not given explicitly

The controller can receive an optional subset of the properties of the
user settings; values not given are set to "null" by default. However,
those null values overwrote the previously existing values, so in
practice any value not given was deleted from the user settings. Now
only non null values overwrite the previous values.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>

2 files changed, 159 insertions, 13 deletions

diff --git a/apps/settings/lib/Controller/UsersController.php b/apps/settings/lib/Controller/UsersController.php
index 451dabde4d1..a568b350883 100644
--- a/apps/settings/lib/Controller/UsersController.php
+++ b/apps/settings/lib/Controller/UsersController.php
@@ -395,21 +395,47 @@ class UsersController extends Controller {
 
 		$data = $this->accountManager->getUser($user);
 		$beforeData = $data;
-		$data[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope;
+		if (!is_null($avatarScope)) {
+			$data[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope;
+		}
 		if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) {
-			$data[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $displayname;
-			$data[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $displaynameScope;
-			$data[IAccountManager::PROPERTY_EMAIL]['value'] = $email;
-			$data[IAccountManager::PROPERTY_EMAIL]['scope'] = $emailScope;
+			if (!is_null($displayname)) {
+				$data[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $displayname;
+			}
+			if (!is_null($displaynameScope)) {
+				$data[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $displaynameScope;
+			}
+			if (!is_null($email)) {
+				$data[IAccountManager::PROPERTY_EMAIL]['value'] = $email;
+			}
+			if (!is_null($emailScope)) {
+				$data[IAccountManager::PROPERTY_EMAIL]['scope'] = $emailScope;
+			}
+		}
+		if (!is_null($website)) {
+			$data[IAccountManager::PROPERTY_WEBSITE]['value'] = $website;
+		}
+		if (!is_null($websiteScope)) {
+			$data[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope;
+		}
+		if (!is_null($address)) {
+			$data[IAccountManager::PROPERTY_ADDRESS]['value'] = $address;
+		}
+		if (!is_null($addressScope)) {
+			$data[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope;
+		}
+		if (!is_null($phone)) {
+			$data[IAccountManager::PROPERTY_PHONE]['value'] = $phone;
+		}
+		if (!is_null($phoneScope)) {
+			$data[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope;
+		}
+		if (!is_null($twitter)) {
+			$data[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter;
+		}
+		if (!is_null($twitterScope)) {
+			$data[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope;
 		}
-		$data[IAccountManager::PROPERTY_WEBSITE]['value'] = $website;
-		$data[IAccountManager::PROPERTY_WEBSITE]['scope'] = $websiteScope;
-		$data[IAccountManager::PROPERTY_ADDRESS]['value'] = $address;
-		$data[IAccountManager::PROPERTY_ADDRESS]['scope'] = $addressScope;
-		$data[IAccountManager::PROPERTY_PHONE]['value'] = $phone;
-		$data[IAccountManager::PROPERTY_PHONE]['scope'] = $phoneScope;
-		$data[IAccountManager::PROPERTY_TWITTER]['value'] = $twitter;
-		$data[IAccountManager::PROPERTY_TWITTER]['scope'] = $twitterScope;
 
 		try {
 			$data = $this->saveUserSettings($user, $data);
diff --git a/apps/settings/tests/Controller/UsersControllerTest.php b/apps/settings/tests/Controller/UsersControllerTest.php
index 48c6fba1894..c0dadd4c470 100644
--- a/apps/settings/tests/Controller/UsersControllerTest.php
+++ b/apps/settings/tests/Controller/UsersControllerTest.php
@@ -438,6 +438,126 @@ class UsersControllerTest extends \Test\TestCase {
 	}
 
 	/**
+	 * @dataProvider dataTestSetUserSettingsSubset
+	 *
+	 * @param string $property
+	 * @param string $propertyValue
+	 */
+	public function testSetUserSettingsSubset($property, $propertyValue) {
+		$controller = $this->getController(false, ['saveUserSettings']);
+		$user = $this->createMock(IUser::class);
+		$user->method('getUID')->willReturn('johndoe');
+
+		$this->userSession->method('getUser')->willReturn($user);
+
+		$defaultProperties = $this->getDefaultAccountManagerUserData();
+
+		$this->accountManager->expects($this->once())
+			->method('getUser')
+			->with($user)
+			->willReturn($defaultProperties);
+
+		$avatarScope = ($property === 'avatarScope') ? $propertyValue : null;
+		$displayName = ($property === 'displayName') ? $propertyValue : null;
+		$displayNameScope = ($property === 'displayNameScope') ? $propertyValue : null;
+		$phone = ($property === 'phone') ? $propertyValue : null;
+		$phoneScope = ($property === 'phoneScope') ? $propertyValue : null;
+		$email = ($property === 'email') ? $propertyValue : null;
+		$emailScope = ($property === 'emailScope') ? $propertyValue : null;
+		$website = ($property === 'website') ? $propertyValue : null;
+		$websiteScope = ($property === 'websiteScope') ? $propertyValue : null;
+		$address = ($property === 'address') ? $propertyValue : null;
+		$addressScope = ($property === 'addressScope') ? $propertyValue : null;
+		$twitter = ($property === 'twitter') ? $propertyValue : null;
+		$twitterScope = ($property === 'twitterScope') ? $propertyValue : null;
+
+		$expectedProperties = $defaultProperties;
+		if ($property === 'avatarScope') {
+			$expectedProperties[IAccountManager::PROPERTY_AVATAR]['scope'] = $propertyValue;
+		}
+		if ($property === 'displayName') {
+			$expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['value'] = $propertyValue;
+		}
+		if ($property === 'displayNameScope') {
+			$expectedProperties[IAccountManager::PROPERTY_DISPLAYNAME]['scope'] = $propertyValue;
+		}
+		if ($property === 'phone') {
+			$expectedProperties[IAccountManager::PROPERTY_PHONE]['value'] = $propertyValue;
+		}
+		if ($property === 'phoneScope') {
+			$expectedProperties[IAccountManager::PROPERTY_PHONE]['scope'] = $propertyValue;
+		}
+		if ($property === 'email') {
+			$expectedProperties[IAccountManager::PROPERTY_EMAIL]['value'] = $propertyValue;
+		}
+		if ($property === 'emailScope') {
+			$expectedProperties[IAccountManager::PROPERTY_EMAIL]['scope'] = $propertyValue;
+		}
+		if ($property === 'website') {
+			$expectedProperties[IAccountManager::PROPERTY_WEBSITE]['value'] = $propertyValue;
+		}
+		if ($property === 'websiteScope') {
+			$expectedProperties[IAccountManager::PROPERTY_WEBSITE]['scope'] = $propertyValue;
+		}
+		if ($property === 'address') {
+			$expectedProperties[IAccountManager::PROPERTY_ADDRESS]['value'] = $propertyValue;
+		}
+		if ($property === 'addressScope') {
+			$expectedProperties[IAccountManager::PROPERTY_ADDRESS]['scope'] = $propertyValue;
+		}
+		if ($property === 'twitter') {
+			$expectedProperties[IAccountManager::PROPERTY_TWITTER]['value'] = $propertyValue;
+		}
+		if ($property === 'twitterScope') {
+			$expectedProperties[IAccountManager::PROPERTY_TWITTER]['scope'] = $propertyValue;
+		}
+
+		if (!empty($email)) {
+			$this->mailer->expects($this->once())->method('validateMailAddress')
+			->willReturn(true);
+		}
+
+		$controller->expects($this->once())
+			->method('saveUserSettings')
+			->with($user, $expectedProperties)
+			->willReturnArgument(1);
+
+		$result = $controller->setUserSettings(
+			$avatarScope,
+			$displayName,
+			$displayNameScope,
+			$phone,
+			$phoneScope,
+			$email,
+			$emailScope,
+			$website,
+			$websiteScope,
+			$address,
+			$addressScope,
+			$twitter,
+			$twitterScope
+		);
+	}
+
+	public function dataTestSetUserSettingsSubset() {
+		return [
+			['avatarScope', IAccountManager::SCOPE_PUBLISHED],
+			['displayName', 'Display name'],
+			['displayNameScope', IAccountManager::SCOPE_PUBLISHED],
+			['phone', '47658468'],
+			['phoneScope', IAccountManager::SCOPE_PUBLISHED],
+			['email', 'john@example.com'],
+			['emailScope', IAccountManager::SCOPE_PUBLISHED],
+			['website', 'nextcloud.com'],
+			['websiteScope', IAccountManager::SCOPE_PUBLISHED],
+			['address', 'street and city'],
+			['addressScope', IAccountManager::SCOPE_PUBLISHED],
+			['twitter', '@nextclouders'],
+			['twitterScope', IAccountManager::SCOPE_PUBLISHED],
+		];
+	}
+
+	/**
 	 * @dataProvider dataTestSaveUserSettings
 	 *
 	 * @param array $data