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>

apps/settings/lib/Controller/UsersController.php

@@ -375,26 +375,52 @@ class UsersController extends Controller {
 		}
 		$user = $this->userSession->getUser();
 		$data = $this->accountManager->getUser($user);
+		if (!is_null($avatarScope)) {
 			$data[IAccountManager::PROPERTY_AVATAR]['scope'] = $avatarScope;
+		}
 		if ($this->config->getSystemValue('allow_user_to_change_display_name', true) !== false) {
+			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 ($this->appManager->isEnabledForUser('federatedfilesharing')) {
 			$shareProvider = \OC::$server->query(FederatedShareProvider::class);
 			if ($shareProvider->isLookupServerUploadEnabled()) {
+				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;
 				}
 			}
+		}
 		try {
 			$this->saveUserSettings($user, $data);
 			return new DataResponse(

apps/settings/tests/Controller/UsersControllerTest.php

@@ -415,6 +415,130 @@ 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);
+
+		$this->userSession->method('getUser')->willReturn($user);
+
+		$defaultProperties = $this->getDefaultAccountManagerUserData();
+
+		$this->accountManager->expects($this->once())
+			->method('getUser')
+			->with($user)
+			->willReturn($defaultProperties);
+
+		$this->appManager->expects($this->once())
+			->method('isEnabledForUser')
+			->with('federatedfilesharing')
+			->willReturn(true);
+
+		$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::VISIBILITY_PUBLIC],
+			['displayName', 'Display name'],
+			['displayNameScope', IAccountManager::VISIBILITY_PUBLIC],
+			['phone', '47658468'],
+			['phoneScope', IAccountManager::VISIBILITY_PUBLIC],
+			['email', 'john@example.com'],
+			['emailScope', IAccountManager::VISIBILITY_PUBLIC],
+			['website', 'nextcloud.com'],
+			['websiteScope', IAccountManager::VISIBILITY_PUBLIC],
+			['address', 'street and city'],
+			['addressScope', IAccountManager::VISIBILITY_PUBLIC],
+			['twitter', '@nextclouders'],
+			['twitterScope', IAccountManager::VISIBILITY_PUBLIC],
+		];
+	}
+
 	/**
 	 * @dataProvider dataTestSaveUserSettings
 	 *