diff --git a/myconext-server/src/main/java/myconext/cron/InactivityMail.java b/myconext-server/src/main/java/myconext/cron/InactivityMail.java index b646b822..030402e5 100644 --- a/myconext-server/src/main/java/myconext/cron/InactivityMail.java +++ b/myconext-server/src/main/java/myconext/cron/InactivityMail.java @@ -21,6 +21,8 @@ @Component public class InactivityMail { + public static final long ONE_DAY_IN_MILLIS = 24 * 60 * 60 * 1000L; + private static final Log LOG = LogFactory.getLog(InactivityMail.class); private final MailBox mailBox; @@ -50,7 +52,9 @@ public void mailInactiveUsers() { return; } try { - Stream.of(UserInactivity.values()).forEach(this::doMailInactiveUsers); + //We need to process the users in reverse order to prevent multiple emails to one user in the initial run + List userInactivities = Stream.of(values()).toList().reversed(); + userInactivities.forEach(this::doMailInactiveUsers); this.doDeleteInactiveUsers(); } catch (Exception e) { //swallow exception as the scheduling stops then @@ -60,15 +64,15 @@ public void mailInactiveUsers() { private void doMailInactiveUsers(UserInactivity userInactivity) { long nowInMillis = System.currentTimeMillis(); - long oneDayInMillis = 24 * 60 * 60 * 1000L; - long fiveYearsInMillis = 5 * 365 * oneDayInMillis; + long fiveYearsInMillis = 5 * 365 * ONE_DAY_IN_MILLIS; - long lastLoginBefore = nowInMillis - (oneDayInMillis * userInactivity.getInactivityDays()); - List users = userRepository.findByLastLoginBeforeAndUserInactivity(lastLoginBefore, userInactivity.getPreviousUserInactivity()); + long lastLoginBefore = nowInMillis - (ONE_DAY_IN_MILLIS * userInactivity.getInactivityDays()); + List users = userRepository.findByLastLoginBeforeAndUserInactivityIn(lastLoginBefore, + this.userInactivitiesWithNullElement(userInactivity.getPreviousUserInactivity())); Map localeVariables = new HashMap<>(); //This is the future date when the user will be deleted based on the inactivityDays of the userInactivity - Date date = new Date(nowInMillis + (fiveYearsInMillis - (oneDayInMillis * userInactivity.getInactivityDays()))); + Date date = new Date(nowInMillis + (fiveYearsInMillis - (ONE_DAY_IN_MILLIS * userInactivity.getInactivityDays()))); localeVariables.put("inactivity_period_en", userInactivity.getInactivityPeriodEn()); localeVariables.put("inactivity_period_nl", userInactivity.getInactivityPeriodNl()); localeVariables.put("deletion_period_en", userInactivity.getDeletionPeriodEn()); @@ -90,10 +94,18 @@ private void doDeleteInactiveUsers() { long oneDayInMillis = 24 * 60 * 60 * 1000L; long lastLoginBefore = nowInMillis - (oneDayInMillis * 5L * 365); - List users = userRepository.findByLastLoginBeforeAndUserInactivity(lastLoginBefore, WEEK_1_BEFORE_5_YEARS); + List users = userRepository.findByLastLoginBeforeAndUserInactivityIn(lastLoginBefore, List.of(WEEK_1_BEFORE_5_YEARS)); userRepository.deleteAll(users); LOG.info(String.format("Deleted %s users (%s) who has been inactive for 5 years in for %s ms", users.size(), users.stream().map(User::getEmail).collect(Collectors.joining(", ")), System.currentTimeMillis() - nowInMillis)); } + + private List userInactivitiesWithNullElement(UserInactivity userInactivity) { + //Can't use List.of as this does not permit null values + List userInactivities = new ArrayList<>(); + userInactivities.add(userInactivity); + userInactivities.add(null); + return userInactivities; + } } diff --git a/myconext-server/src/main/java/myconext/repository/UserRepository.java b/myconext-server/src/main/java/myconext/repository/UserRepository.java index 1d2afb18..5346efcb 100644 --- a/myconext-server/src/main/java/myconext/repository/UserRepository.java +++ b/myconext-server/src/main/java/myconext/repository/UserRepository.java @@ -54,7 +54,7 @@ public interface UserRepository extends MongoRepository { @Query("{'email' : {$regex : ?0, $options: 'i'}}") List findByEmailDomain(String regex); - List findByLastLoginBeforeAndUserInactivity(long lastLoginBefore, UserInactivity userInactivity); + List findByLastLoginBeforeAndUserInactivityIn(long lastLoginBefore, List userInactivities); @Query(""" { $and: [ { $or: [ diff --git a/myconext-server/src/test/java/myconext/cron/InactivityMailTest.java b/myconext-server/src/test/java/myconext/cron/InactivityMailTest.java index b3f19f35..8b299860 100644 --- a/myconext-server/src/test/java/myconext/cron/InactivityMailTest.java +++ b/myconext-server/src/test/java/myconext/cron/InactivityMailTest.java @@ -17,6 +17,7 @@ import java.util.stream.Collectors; import java.util.stream.Stream; +import static myconext.cron.InactivityMail.ONE_DAY_IN_MILLIS; import static org.junit.jupiter.api.Assertions.*; @SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT, @@ -64,6 +65,30 @@ public void mailInactivityMail() { assertThrows(ConditionTimeoutException.class, this::mailMessages); } + @SneakyThrows + @Test + public void mailInactivityFirstRun() { + //See https://github.com/OpenConext/OpenConext-myconext/issues/656 + User user = new User(); + long yesterday = System.currentTimeMillis() - ONE_DAY_IN_MILLIS; + user.setLastLogin(yesterday - (UserInactivity.WEEK_1_BEFORE_5_YEARS.getInactivityDays() * ONE_DAY_IN_MILLIS)); + user.setEmail(UserInactivity.WEEK_1_BEFORE_5_YEARS.name()); + //Explicit set userInactivity to null for self-explanation of the test code + user.setUserInactivity(null); + userRepository.save(user); + + inactivityMail.mailInactiveUsers(); + + List mimeMessages = mailMessages(); + assertEquals(1, mimeMessages.size()); + User userFromDB = userRepository.findOneUserByEmail(UserInactivity.WEEK_1_BEFORE_5_YEARS.name()); + assertEquals(UserInactivity.WEEK_1_BEFORE_5_YEARS, userFromDB.getUserInactivity()); + //Idempotency check + greenMail.purgeEmailFromAllMailboxes(); + inactivityMail.mailInactiveUsers(); + assertThrows(ConditionTimeoutException.class, this::mailMessages); + } + @SneakyThrows @Test public void mailInactivityMailDutch() { @@ -88,11 +113,10 @@ private String messageContent(MimeMessage mimeMessage) { } private void inactivityUserSeed(String preferredLanguage) { - long oneDayInMillis = 24 * 60 * 60 * 1000L; - long yesterday = System.currentTimeMillis() - oneDayInMillis; + long yesterday = System.currentTimeMillis() - ONE_DAY_IN_MILLIS; Stream.of(UserInactivity.values()).forEach(userInactivity -> { User user = new User(); - user.setLastLogin(yesterday - (userInactivity.getInactivityDays() * oneDayInMillis)); + user.setLastLogin(yesterday - (userInactivity.getInactivityDays() * ONE_DAY_IN_MILLIS)); user.setEmail(userInactivity.name()); user.setPreferredLanguage(preferredLanguage); user.setUserInactivity(userInactivity.getPreviousUserInactivity()); @@ -100,7 +124,7 @@ private void inactivityUserSeed(String preferredLanguage) { }); //And one extra User who is to be deleted User user = new User(); - user.setLastLogin(yesterday - (((5L * 365) + 5) * oneDayInMillis)); + user.setLastLogin(yesterday - (((5L * 365) + 5) * ONE_DAY_IN_MILLIS)); user.setEmail(DELETED_EMAIL); user.setUserInactivity(UserInactivity.WEEK_1_BEFORE_5_YEARS); userRepository.save(user);