OO-2241: remove optimized getter access to user properties, remove stupid unit...

OO-2241: remove optimized getter access to user properties, remove stupid unit test, change unit tests with the !keePEmail option

src/main/java/org/olat/admin/user/delete/service/UserDeletionManager.java

@@ -329,26 +329,29 @@ public class UserDeletionManager extends BasicManager {
 
 		identity = securityManager.loadIdentityByKey(identity.getKey());
 		//keep login-name only -> change email
-		if (!keepUserEmailAfterDeletion){
-			List<UserPropertyHandler> userPropertyHandlers = UserManager.getInstance().getUserPropertyHandlersFor("org.olat.admin.user.UsermanagerUserSearchForm", true);
-			User persistedUser = identity.getUser();
-			String actualProperty;
-			for (UserPropertyHandler userPropertyHandler : userPropertyHandlers) {
-				actualProperty = userPropertyHandler.getName(); 
-				if (actualProperty.equals(UserConstants.EMAIL)){
-					String oldEmail = userPropertyHandler.getUserProperty(persistedUser, null);
-					String newEmail = "";
-					if (StringHelper.containsNonWhitespace(oldEmail)){ 
-						newEmail = getBackupStringWithDate(oldEmail);
-					}
-					logInfo("Update user-property user=" + persistedUser);
-					userPropertyHandler.setUserProperty(persistedUser, newEmail);
+
+		User persistedUser = identity.getUser();
+		List<UserPropertyHandler> userPropertyHandlers = UserManager.getInstance().getAllUserPropertyHandlers();
+		for (UserPropertyHandler userPropertyHandler : userPropertyHandlers) {
+			String actualProperty = userPropertyHandler.getName();
+			if (userPropertyHandler.isDeletable()
+					&& !(keepUserEmailAfterDeletion && UserConstants.EMAIL.equals(actualProperty))) {
+				persistedUser.setProperty(actualProperty, null);
+			}
+			
+			if((!keepUserEmailAfterDeletion && UserConstants.EMAIL.equals(actualProperty))) {
+				String oldEmail = userPropertyHandler.getUserProperty(persistedUser, null);
+				String newEmail = "";
+				if (StringHelper.containsNonWhitespace(oldEmail)){ 
+					newEmail = getBackupStringWithDate(oldEmail);
 				}
+				logInfo("Update user-property user=" + persistedUser);
+				userPropertyHandler.setUserProperty(persistedUser, newEmail);
 			}
 		}
+		UserManager.getInstance().updateUserFromIdentity(identity);
 		
-		logInfo("deleteUserProperties user=" + identity.getUser());
-		UserManager.getInstance().deleteUserProperties(identity.getUser(), keepUserEmailAfterDeletion);
+		logInfo("deleteUserProperties user=" + persistedUser);
 		DBFactory.getInstance().commit();
 		identity = securityManager.loadIdentityByKey(identity.getKey());
 		//keep email only -> change login-name

src/main/java/org/olat/user/AbstractUserPropertyHandler.java

@@ -52,7 +52,6 @@ public abstract class AbstractUserPropertyHandler implements UserPropertyHandler
 	private String name; 
 	private String group;
 	private boolean deletable = true; // default
-	private Field getter;
 	private String databaseColumnName;
 
 	/**
@@ -144,15 +143,9 @@ public abstract class AbstractUserPropertyHandler implements UserPropertyHandler
 	 */
 	protected String getInternalValue(User user) {
 		if (user instanceof UserImpl) {
-			//String value = ((UserImpl)user).getUserProperty(name);
-			String value = null;
-			try {
-				value = (String)getter.get(user);
-				if("_".equals(value) && "oracle".equals(DBFactory.getInstance().getDbVendor())) {
-					value = null;
-				}
-			} catch (IllegalArgumentException | IllegalAccessException e) {
-				log.error("", e);
+			String value = ((UserImpl)user).getUserProperty(name);
+			if("_".equals(value) && "oracle".equals(DBFactory.getInstance().getDbVendor())) {
+				value = null;
 			}
 			return value;
 		} else if (user != null) {
@@ -165,17 +158,13 @@ public abstract class AbstractUserPropertyHandler implements UserPropertyHandler
 	 * @param value The raw value in a 18n independent form
 	 */
 	protected void setInternalValue(User user, String value) {
-		if (user instanceof UserImpl && getter != null) {
-			try {
-				// remove fields with null or empty value from o_userfield table (hibernate)
-				// sparse data storage
-				if (value == null || value.length() == 0) {
-					getter.set(user, null);
-				} else {
-					getter.set(user, value);
-				}
-			} catch (IllegalArgumentException | IllegalAccessException e) {
-				log.error("", e);
+		if (user instanceof UserImpl) {
+			// remove fields with null or empty value from o_userfield table (hibernate)
+			// sparse data storage
+			if (value == null || value.length() == 0) {
+				((UserImpl)user).setUserProperty(name, null);
+			} else {
+				((UserImpl)user).setUserProperty(name, value);
 			}
 		} else {
 			log.warn("Set read-only value: " + name,  null);
@@ -188,6 +177,7 @@ public abstract class AbstractUserPropertyHandler implements UserPropertyHandler
 	 * @return String internal user field info
 	 * @see java.lang.Object#toString()
 	 */
+	@Override
 	public String toString() {
 		String quickinfo = "AbstractUserPropertyHandler("+this.getClass().getName()+")["+getName()+"]" ;
 		return quickinfo + "," + super.toString();
@@ -197,6 +187,7 @@ public abstract class AbstractUserPropertyHandler implements UserPropertyHandler
 	 * Spring setter
 	 * @param group
 	 */
+	@Override
 	public void setGroup(String group) {
 		this.group = group;
 	}
@@ -220,7 +211,7 @@ public abstract class AbstractUserPropertyHandler implements UserPropertyHandler
 	
 	protected void setInternalGetterSetter(String name) {
 		try {
-			getter = UserImpl.class.getDeclaredField(name);
+			Field getter = UserImpl.class.getDeclaredField(name);
 			getter.setAccessible(true);
 			if(getter.isAnnotationPresent(Column.class)) {
 				Column col = getter.getAnnotation(Column.class);

src/main/java/org/olat/user/UserImpl.java

@@ -104,7 +104,7 @@ public class UserImpl implements Persistable, User {
 	@Transient
 	private boolean webdav;
 
-	@Column(name="u_firstName", nullable=true, insertable=true, updatable=true)
+	@Column(name="u_firstname", nullable=true, insertable=true, updatable=true)
 	private String firstName;
 	@Column(name="u_lastname", nullable=true, insertable=true, updatable=true)
 	private String lastName;

src/main/java/org/olat/user/UserManager.java

@@ -195,12 +195,6 @@ public abstract class UserManager extends BasicManager {
 		return userPropertiesConfig;
 	}
 
-	/**
-	 * Delete all user-properties which are deletable.
-	 * @param user
-	 */
-	public abstract User deleteUserProperties(User user, boolean keepUserEmail);
-
 	public List<UserPropertyHandler> getUserPropertyHandlersFor(String usageIdentifyer, boolean isAdministrativeUser) {
 		return userPropertiesConfig.getUserPropertyHandlersFor(usageIdentifyer, isAdministrativeUser);
 	}

src/main/java/org/olat/user/UserManagerImpl.java

@@ -55,7 +55,6 @@ import org.olat.core.util.i18n.I18nModule;
 import org.olat.core.util.mail.MailHelper;
 import org.olat.properties.Property;
 import org.olat.properties.PropertyManager;
-import org.olat.user.propertyhandlers.UserPropertyHandler;
 import org.springframework.beans.factory.annotation.Autowired;
 
 /**
@@ -426,30 +425,6 @@ public class UserManagerImpl extends UserManager {
 	   return charset;
 	}
 
-	/**
-	 * Delete all user-properties which are deletable.
-	 * @param user
-	 */
-	@Override
-	public User deleteUserProperties(User user, boolean keepUserEmail) {
-		// prevent stale objects, reload first
-		user = loadUserByKey(user.getKey());
-		// loop over user fields and remove them form the database if they are
-		// deletable
-		List<UserPropertyHandler> propertyHandlers = userPropertiesConfig.getAllUserPropertyHandlers();
-		for (UserPropertyHandler propertyHandler : propertyHandlers) {
-			String fieldName = propertyHandler.getName();
-			if (propertyHandler.isDeletable()
-					&& !(keepUserEmail && UserConstants.EMAIL.equals(propertyHandler.getName()))) {
-				user.setProperty(fieldName, null);
-			}		
-		}
-		// persist changes
-		User updatedUser = updateUser(user);
-		if(isLogDebugEnabled()) logDebug("Delete all user-attributtes for user=" + user);
-		return updatedUser;
-	}
-
 	@Override
 	public int warmUp() {
 		EntityManager em = dbInstance.getCurrentEntityManager();

src/test/java/org/olat/admin/user/delete/service/UserDeletionManagerTest.java

@@ -83,7 +83,8 @@ public class UserDeletionManagerTest extends OlatTestCase {
 	@Test
 	public void testDeleteIdentity() {
 		String username = "id-to-del-" + UUID.randomUUID();
-		User user = userManager.createUser("first" + username, "last" + username, username + "@frentix.com");
+		String email = username + "@frentix.com";
+		User user = userManager.createUser("first" + username, "last" + username, email);
 		user.setProperty(UserConstants.COUNTRY, "");
 		user.setProperty(UserConstants.CITY, "Basel");
 		user.setProperty(UserConstants.INSTITUTIONALNAME, "Del-23");
@@ -136,6 +137,9 @@ public class UserDeletionManagerTest extends OlatTestCase {
 		Assert.assertFalse(StringHelper.containsNonWhitespace(institutionalName));
 		String institutionalId = deletedUser.getProperty(UserConstants.INSTITUTIONALUSERIDENTIFIER, null);
 		Assert.assertFalse(StringHelper.containsNonWhitespace(institutionalId));
+		String deletedEmail = deletedUser.getProperty(UserConstants.EMAIL, null);
+		Assert.assertNotNull(deletedEmail);
+		Assert.assertFalse(email.equals(deletedEmail));
 	}
 
 	@Test

src/test/profile/mysql/olat.local.properties

@@ -24,7 +24,7 @@ is.translation.server=disabled
 deploy.course.exports=false
 
 # for UserTest
-keepUserEmailAfterDeletion=true
+keepUserEmailAfterDeletion=false
 keepUserLoginAfterDeletion=true
 
 # do not run upgrades and scheduled jobs and such

src/test/profile/oracle/olat.local.properties

@@ -19,7 +19,7 @@ instance.id=${test.env.instance.id:2}
 generate.index.at.startup=false
 
 # for UserTest
-keepUserEmailAfterDeletion=true
+keepUserEmailAfterDeletion=false
 keepUserLoginAfterDeletion=true
 
 # do not run upgrades and scheduled jobs and such

src/test/profile/postgresql/olat.local.properties

@@ -24,7 +24,7 @@ is.translation.server=disabled
 deploy.course.exports=false
 
 # for UserTest
-keepUserEmailAfterDeletion=true
+keepUserEmailAfterDeletion=false
 keepUserLoginAfterDeletion=true
 
 # do not run upgrades and scheduled jobs and such