diff --git a/orcid-persistence/src/main/java/org/orcid/persistence/dao/ProfileDao.java b/orcid-persistence/src/main/java/org/orcid/persistence/dao/ProfileDao.java index 09bf55902b6..9984f40bf9e 100644 --- a/orcid-persistence/src/main/java/org/orcid/persistence/dao/ProfileDao.java +++ b/orcid-persistence/src/main/java/org/orcid/persistence/dao/ProfileDao.java @@ -76,9 +76,7 @@ public interface ProfileDao extends GenericDao { void updateLastModifiedDateAndIndexingStatusWithoutResult(String orcid, Date lastModified, IndexingStatus indexingStatus); - public List> findEmailsUnverfiedDays(int daysUnverified, int maxResults); - - public List> findEmailsUnverifiedDaysByEventType(int daysUnverified, int tooOldNumberOfDays); + public List> findEmailsUnverfiedDays(int daysUnverified, EmailEventType eventSent); String retrieveOrcidType(String orcid); diff --git a/orcid-persistence/src/main/java/org/orcid/persistence/dao/impl/ProfileDaoImpl.java b/orcid-persistence/src/main/java/org/orcid/persistence/dao/impl/ProfileDaoImpl.java index d916513f83a..0eca19bc9ff 100644 --- a/orcid-persistence/src/main/java/org/orcid/persistence/dao/impl/ProfileDaoImpl.java +++ b/orcid-persistence/src/main/java/org/orcid/persistence/dao/impl/ProfileDaoImpl.java @@ -13,16 +13,16 @@ import org.apache.commons.lang3.tuple.Triple; import org.orcid.persistence.aop.UpdateProfileLastModifiedAndIndexingStatus; import org.orcid.persistence.dao.ProfileDao; +import org.orcid.persistence.jpa.entities.EmailEventType; import org.orcid.persistence.jpa.entities.IndexingStatus; import org.orcid.persistence.jpa.entities.OrcidGrantedAuthority; import org.orcid.persistence.jpa.entities.ProfileEntity; import org.orcid.persistence.jpa.entities.ProfileEventEntity; import org.orcid.persistence.jpa.entities.ProfileEventType; -import org.springframework.beans.factory.annotation.Value; -import org.springframework.transaction.annotation.Transactional; - import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.transaction.annotation.Transactional; public class ProfileDaoImpl extends GenericDaoImpl implements ProfileDao { @@ -30,6 +30,22 @@ public class ProfileDaoImpl extends GenericDaoImpl implem private static final Logger LOGGER = LoggerFactory.getLogger(ProfileDaoImpl.class); + private static final String FIND_EMAILS_TO_SEND_VERIFICATION_REMINTER = "select orcid, email, is_verified \n" + + "from email \n" + + "where email in \n" + + " (SELECT distinct(e.email) \n" + + " FROM email e, email_event ev, profile p \n" + + " WHERE e.is_verified = false \n" + + " AND p.orcid = e.orcid \n" + + " AND p.deprecated_date is null \n" + + " AND p.profile_deactivation_date is null \n" + + " AND p.account_expiry is null \n" + + " AND e.date_created BETWEEN (DATE_TRUNC('day', now()) - CAST('{RANGE_START}' AS INTERVAL DAY)) AND (DATE_TRUNC('day', now()) - CAST('{RANGE_END}' AS INTERVAL DAY)) \n" + + " AND NOT EXISTS \n" + + " (SELECT email FROM email_event x WHERE x.email = e.email AND email_event_type IN ('{EVENT_SENT}')\n" + + " )\n" + + ") order by last_modified;"; + @Value("${org.orcid.postgres.query.timeout:30000}") private Integer queryTimeout; @@ -136,47 +152,20 @@ public List findUnclaimedNotIndexedAfterWaitPeriod(int waitPeriodDays, i @SuppressWarnings("unchecked") @Override - public List> findEmailsUnverfiedDays(int daysUnverified, int maxResults) { - StringBuilder queryString = new StringBuilder("SELECT e.email, e.is_primary, e.date_created FROM email e "); - queryString.append("LEFT JOIN email_event ev ON e.email = ev.email "); - queryString.append("JOIN profile p on p.orcid = e.orcid and p.claimed = true "); - queryString.append("AND p.deprecated_date is null AND p.profile_deactivation_date is null AND p.account_expiry is null "); - queryString.append("where ev.email IS NULL " + "and e.is_verified = false "); - queryString.append("and e.date_created < (now() - CAST('").append(daysUnverified).append("' AS INTERVAL DAY)) "); - queryString.append("and (e.source_id = e.orcid OR e.source_id is null)"); - queryString.append(" ORDER BY e.last_modified"); - - Query query = entityManager.createNativeQuery(queryString.toString()); - query.setMaxResults(maxResults); - List dbInfo = query.getResultList(); - List> results = new ArrayList>(); - dbInfo.stream().forEach(element -> { - Triple pair = Triple.of((String) element[0], (Boolean) element[1], (Date) element[2]); - results.add(pair); - }); - return results; - } - - @SuppressWarnings("unchecked") - @Override - public List> findEmailsUnverifiedDaysByEventType(int daysUnverified, int tooOldNumberOfDays) { - StringBuilder queryString = new StringBuilder("SELECT e.email, e.is_primary, ev.email_event_type FROM email e "); - queryString.append("LEFT JOIN email_event ev ON e.email = ev.email "); - queryString.append("JOIN profile p on p.orcid = e.orcid and p.claimed = true "); - queryString.append("AND p.deprecated_date is null AND p.profile_deactivation_date is null AND p.account_expiry is null "); - queryString.append("where e.is_verified = false "); - queryString.append("and e.date_created between (now() - CAST('").append(tooOldNumberOfDays).append("' AS INTERVAL DAY)) and (now() - CAST('") - .append(daysUnverified).append("' AS INTERVAL DAY)) "); - queryString.append("and e.date_created < (now() - CAST('").append(daysUnverified).append("' AS INTERVAL DAY)) "); - queryString.append("and (e.source_id = e.orcid OR e.source_id is null)"); - queryString.append(" ORDER BY e.last_modified"); + public List> findEmailsUnverfiedDays(int daysUnverified, EmailEventType eventSent) { + int rangeStart = daysUnverified; + int rangeEnd = daysUnverified - 1; + String queryString = FIND_EMAILS_TO_SEND_VERIFICATION_REMINTER.replace("{RANGE_START}", String.valueOf(rangeStart)) + .replace("{RANGE_END}", String.valueOf(rangeEnd)).replace("{EVENT_SENT}", eventSent.name()); + LOGGER.debug("Query to search unverified emails"); + LOGGER.debug(queryString); Query query = entityManager.createNativeQuery(queryString.toString()); List dbInfo = query.getResultList(); - List> results = new ArrayList>(); + List> results = new ArrayList>(); dbInfo.stream().forEach(element -> { - Triple pair = Triple.of((String) element[0], (Boolean) element[1], (String) element[2]); - results.add(pair); + Triple q = Triple.of((String) element[0], (String) element[1], (Boolean) element[2]); + results.add(q); }); return results; } diff --git a/orcid-persistence/src/test/java/org/orcid/persistence/dao/ProfileDaoTest.java b/orcid-persistence/src/test/java/org/orcid/persistence/dao/ProfileDaoTest.java index 96e6551b8e9..0eaf5017474 100644 --- a/orcid-persistence/src/test/java/org/orcid/persistence/dao/ProfileDaoTest.java +++ b/orcid-persistence/src/test/java/org/orcid/persistence/dao/ProfileDaoTest.java @@ -5,7 +5,6 @@ import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertTrue; -import static org.junit.Assert.fail; import java.util.Arrays; import java.util.Calendar; @@ -17,7 +16,6 @@ import javax.persistence.EntityManager; import javax.persistence.Query; -import org.apache.commons.lang3.tuple.Triple; import org.dbunit.dataset.DataSetException; import org.joda.time.LocalDateTime; import org.junit.AfterClass; @@ -26,11 +24,11 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.orcid.persistence.jpa.entities.EmailEventEntity; -import org.orcid.persistence.jpa.entities.EmailEventType; import org.orcid.persistence.jpa.entities.IndexingStatus; import org.orcid.persistence.jpa.entities.ProfileEntity; import org.orcid.persistence.jpa.entities.ProfileEventEntity; import org.orcid.persistence.jpa.entities.ProfileEventType; +import org.orcid.persistence.util.Quadruple; import org.orcid.test.DBUnitTest; import org.orcid.test.OrcidJUnit4ClassRunner; import org.springframework.test.annotation.Rollback; @@ -393,51 +391,57 @@ public void findEmailsUnverfiedDaysTest() throws IllegalAccessException { // Created today assertEquals(1, insertEmailWithDateCreated("unverified_1@test.orcid.org", "bd22086b65b6259fe79f7844a6b6a369441733b9ef04eff762f3d640957b78f5", orcid, false, new Date())); - // Created a week ago - assertEquals(1, insertEmailWithDateCreated("unverified_2@test.orcid.org", "95770578974f683fb05c179a84f57c3fc7d4b260f8079fbc590080e51873bb67", orcid, false, LocalDateTime.now().minusDays(7).toDate())); + // Created 2 days ago + assertEquals(1, insertEmailWithDateCreated("unverified_2@test.orcid.org", "95770578974f683fb05c179a84f57c3fc7d4b260f8079fbc590080e51873bb67", orcid, false, LocalDateTime.now().minusDays(2).toDate())); + + // Created 7 days ago + assertEquals(1, insertEmailWithDateCreated("unverified_3@test.orcid.org", "3cbebfc1de2500494fc95553c956e757cb1998149d366afb71888cdeb1550719", orcid, false, LocalDateTime.now().minusDays(7).toDate())); // Created 15 days ago - assertEquals(1, insertEmailWithDateCreated("unverified_3@test.orcid.org", "3cbebfc1de2500494fc95553c956e757cb1998149d366afb71888cdeb1550719", orcid, false, LocalDateTime.now().minusDays(15).toDate())); + assertEquals(1, insertEmailWithDateCreated("unverified_4@test.orcid.org", "8fccc2e14b546b968033dee2efb1644623a31a7878b55ae8ad52951961ca3719", orcid, false, LocalDateTime.now().minusDays(15).toDate())); + + // Created 2 days ago and verified + assertEquals(1, insertEmailWithDateCreated("verified_1@test.orcid.org", "2f4812b9c675e9803a4bb616dd1bc241c8c9302ba5690a1ea9d48049a32e7c5f", orcid, true, LocalDateTime.now().minusDays(2).toDate())); // Created 7 days ago and verified - assertEquals(1, insertEmailWithDateCreated("verified_1@test.orcid.org", "2f4812b9c675e9803a4bb616dd1bc241c8c9302ba5690a1ea9d48049a32e7c5f", orcid, true, LocalDateTime.now().minusDays(7).toDate())); + assertEquals(1, insertEmailWithDateCreated("verified_2@test.orcid.org", "896dea808bbf69bde1b177f27800e84d17763860bffde1dfd8ef200e79ff9971", orcid, true, LocalDateTime.now().minusDays(7).toDate())); // Created 15 days ago and verified - assertEquals(1, insertEmailWithDateCreated("verified_2@test.orcid.org", "896dea808bbf69bde1b177f27800e84d17763860bffde1dfd8ef200e79ff9971", orcid, true, LocalDateTime.now().minusDays(15).toDate())); + assertEquals(1, insertEmailWithDateCreated("verified_3@test.orcid.org", "f98cce12446df199b852583ce677ecf9870ebe1b58df21bc4e7b01dea67daf01", orcid, true, LocalDateTime.now().minusDays(15).toDate())); - List> results = profileDao.findEmailsUnverfiedDays(7, 100); + List> results = profileDao.findEmailsUnverfiedDays(2); assertNotNull(results); - assertEquals(2, results.size()); + assertEquals(1, results.size()); + assertEquals(orcid, results.get(0).getFirst()); + assertEquals("unverified_2@test.orcid.org", results.get(0).getSecond()); - boolean found1 = false, found2 = false; + results = profileDao.findEmailsUnverfiedDays(7); + assertNotNull(results); + assertEquals(1, results.size()); + assertEquals(orcid, results.get(0).getFirst()); + assertEquals("unverified_3@test.orcid.org", results.get(0).getSecond()); - for(Triple element : results) { - assertNotNull(element.getRight()); - if(element.getLeft().equals("unverified_2@test.orcid.org")) { - found1 = true; - } else if(element.getLeft().equals("unverified_3@test.orcid.org")) { - found2 = true; - } else { - fail("Unexpected email id: " + element.getRight()); - } - } + results = profileDao.findEmailsUnverfiedDays(15); + assertNotNull(results); + assertEquals(1, results.size()); + assertEquals(orcid, results.get(0).getFirst()); + assertEquals("unverified_4@test.orcid.org", results.get(0).getSecond()); - assertTrue(found1); - assertTrue(found2); + results = profileDao.findEmailsUnverfiedDays(6); + assertNotNull(results); + assertTrue(results.isEmpty()); - // Put an email event on 'unverified_2@test.orcid.org' and verify there is only one result - emailEventDao.persist(new EmailEventEntity("unverified_2@test.orcid.org", EmailEventType.VERIFY_EMAIL_7_DAYS_SENT)); + results = profileDao.findEmailsUnverfiedDays(8); + assertNotNull(results); + assertTrue(results.isEmpty()); - results = profileDao.findEmailsUnverfiedDays(7, 100); + results = profileDao.findEmailsUnverfiedDays(14); assertNotNull(results); - assertEquals(1, results.size()); - assertEquals("unverified_3@test.orcid.org", results.get(0).getLeft()); + assertTrue(results.isEmpty()); - // Put an email event on 'unverified_3@test.orcid.org' and verify there is no result anymore - emailEventDao.persist(new EmailEventEntity("unverified_3@test.orcid.org", EmailEventType.VERIFY_EMAIL_TOO_OLD)); - results = profileDao.findEmailsUnverfiedDays(7, 100); + results = profileDao.findEmailsUnverfiedDays(16); assertNotNull(results); - assertTrue(results.isEmpty()); + assertTrue(results.isEmpty()); } private int insertEmailWithDateCreated(String email, String emailHash, String orcid, boolean isVerified, Date dateCreated) { diff --git a/orcid-scheduler-web/src/main/java/org/orcid/scheduler/email/cli/manager/EmailMessageSenderImpl.java b/orcid-scheduler-web/src/main/java/org/orcid/scheduler/email/cli/manager/EmailMessageSenderImpl.java index d69ee216fa3..d25758a8e45 100644 --- a/orcid-scheduler-web/src/main/java/org/orcid/scheduler/email/cli/manager/EmailMessageSenderImpl.java +++ b/orcid-scheduler-web/src/main/java/org/orcid/scheduler/email/cli/manager/EmailMessageSenderImpl.java @@ -25,7 +25,6 @@ import org.apache.commons.lang3.LocaleUtils; import org.apache.commons.lang3.time.DurationFormatUtils; import org.apache.commons.lang3.tuple.Triple; -import org.joda.time.LocalDateTime; import org.orcid.core.constants.EmailConstants; import org.orcid.core.locale.LocaleManager; import org.orcid.core.manager.EmailMessage; @@ -70,8 +69,6 @@ import org.springframework.transaction.support.TransactionCallbackWithoutResult; import org.springframework.transaction.support.TransactionTemplate; -import liquibase.repackaged.org.apache.commons.lang3.StringUtils; - /** * * @author Will Simpson @@ -572,84 +569,38 @@ private String getHtmlBody(NotificationAdministrative notificationAdministrative @Override synchronized public void processUnverifiedEmails2Days() { - LOGGER.info("About to process unverIfied emails for 2 days reminder"); - List> elements = Collections.> emptyList(); - do { - elements = profileDaoReadOnly.findEmailsUnverfiedDays(verifyReminderAfterTwoDays, 100); - LOGGER.info("Got batch of {} profiles with unverified emails for 2 days reminder", elements.size()); - LocalDateTime now = LocalDateTime.now(); - // togglz here - Date tooOld = now.minusDays(emailTooOldLegacy).toDate(); - if (Features.SEND_ALL_VERIFICATION_EMAILS.isActive()) { - tooOld = now.minusDays(emailTooOld).toDate(); - } - for (Triple element : elements) { - if (element.getRight() == null || element.getRight().after(tooOld)) { - processUnverifiedEmailsInTransaction(element.getLeft(), element.getMiddle(), EmailEventType.VERIFY_EMAIL_2_DAYS_SENT, - EmailEventType.VERIFY_EMAIL_2_DAYS_SENT_SKIPPED); - } else { - // Mark is as too old to send the verification email - markUnverifiedEmailAsTooOld(element.getLeft()); - } - } - } while (!elements.isEmpty()); + processUnverifiedEmails(verifyReminderAfterTwoDays, EmailEventType.VERIFY_EMAIL_2_DAYS_SENT, EmailEventType.VERIFY_EMAIL_2_DAYS_SENT_SKIPPED); } synchronized public void processUnverifiedEmails7Days() { - if (Features.SEND_ALL_VERIFICATION_EMAILS.isActive()) { - LOGGER.info("About to process unverIfied emails for 7 days reminder"); - List> elements = Collections.> emptyList(); - - elements = profileDaoReadOnly.findEmailsUnverifiedDaysByEventType(verifyReminderAfterSevenDays, emailTooOld); - LOGGER.info("Got {} profiles with email event and unverified emails for 7 days reminder", elements.size()); - - HashMap> hasEventTypesMap = hasEventTypes(elements, - List.of(EmailEventType.VERIFY_EMAIL_7_DAYS_SENT, EmailEventType.VERIFY_EMAIL_7_DAYS_SENT_SKIPPED, EmailEventType.VERIFY_EMAIL_TOO_OLD)); - for (Triple element : elements) { - if (!hasEventTypesMap.containsKey(element.getLeft())) - processUnverifiedEmailsInTransaction(element.getLeft(), element.getMiddle(), EmailEventType.VERIFY_EMAIL_7_DAYS_SENT, - EmailEventType.VERIFY_EMAIL_7_DAYS_SENT_SKIPPED); - hasEventTypesMap.put(element.getLeft(), element); - } - } - } - - // helper method to get the triple map for an event type list - private HashMap> hasEventTypes(List> elements, List eventTypes) { - HashMap> hasEventTypesMap = new HashMap>(); - for (Triple element : elements) { - if (eventTypes.contains(EmailEventType.valueOf(element.getRight()))) { - hasEventTypesMap.put(element.getLeft(), element); - } - } - return hasEventTypesMap; + processUnverifiedEmails(verifyReminderAfterSevenDays, EmailEventType.VERIFY_EMAIL_7_DAYS_SENT, EmailEventType.VERIFY_EMAIL_7_DAYS_SENT_SKIPPED); } synchronized public void processUnverifiedEmails28Days() { + processUnverifiedEmails(verifyReminderAfterTwentyEightDays, EmailEventType.VERIFY_EMAIL_28_DAYS_SENT, EmailEventType.VERIFY_EMAIL_28_DAYS_SENT_SKIPPED); + } + + private void processUnverifiedEmails(int unverifiedDays, EmailEventType sent, EmailEventType failed) { if (Features.SEND_ALL_VERIFICATION_EMAILS.isActive()) { - LOGGER.info("About to process unverIfied emails for 28 days reminder"); - List> elements = Collections.> emptyList(); - elements = profileDaoReadOnly.findEmailsUnverifiedDaysByEventType(verifyReminderAfterTwentyEightDays, emailTooOld); - LOGGER.info("Got {} profiles with email event and unverified emails for 28 days reminder", elements.size()); - - HashMap> hasEventTypesMap = hasEventTypes(elements, - List.of(EmailEventType.VERIFY_EMAIL_28_DAYS_SENT, EmailEventType.VERIFY_EMAIL_28_DAYS_SENT_SKIPPED, EmailEventType.VERIFY_EMAIL_TOO_OLD)); - for (Triple element : elements) { - if (!hasEventTypesMap.containsKey(element.getLeft())) - processUnverifiedEmailsInTransaction(element.getLeft(), element.getMiddle(), EmailEventType.VERIFY_EMAIL_28_DAYS_SENT, - EmailEventType.VERIFY_EMAIL_28_DAYS_SENT_SKIPPED); - hasEventTypesMap.put(element.getLeft(), element); + LOGGER.info("About to process unverIfied emails for {} days reminder", unverifiedDays); + List> elements = Collections.> emptyList(); + elements = profileDaoReadOnly.findEmailsUnverfiedDays(unverifiedDays, sent); + LOGGER.info("Got {} profiles with email event and unverified emails for {} days reminder", elements.size(), unverifiedDays); + + for (Triple element : elements) { + processUnverifiedEmailsInTransaction(element.getLeft(), element.getMiddle(), element.getRight(), sent, + failed); } } } - private void processUnverifiedEmailsInTransaction(final String email, final Boolean isPrimaryEmail, EmailEventType eventSent, EmailEventType eventSkipped) { + private void processUnverifiedEmailsInTransaction(final String userOrcid, final String email, final Boolean isPrimaryEmail, EmailEventType eventSent, EmailEventType eventSkipped) { transactionTemplate.execute(new TransactionCallbackWithoutResult() { @Override @Transactional protected void doInTransactionWithoutResult(TransactionStatus status) { try { - String userOrcid = emailManagerReadOnly.findOrcidIdByEmail(email); + LOGGER.debug("Sending reminder {} to email address {}, orcid {}", eventSent, email, userOrcid); sendVerificationReminderEmail(userOrcid, email, isPrimaryEmail); emailEventDao.persist(new EmailEventEntity(email, eventSent)); emailEventDao.flush(); @@ -662,17 +613,6 @@ protected void doInTransactionWithoutResult(TransactionStatus status) { }); } - private void markUnverifiedEmailAsTooOld(final String email) { - transactionTemplate.execute(new TransactionCallbackWithoutResult() { - @Override - @Transactional - protected void doInTransactionWithoutResult(TransactionStatus status) { - emailEventDao.persist(new EmailEventEntity(email, EmailEventType.VERIFY_EMAIL_TOO_OLD)); - emailEventDao.flush(); - } - }); - } - private void sendVerificationReminderEmail(String userOrcid, String email, Boolean isPrimaryEmail) { ProfileEntity profile = profileEntityCacheManager.retrieve(userOrcid); Locale locale = getUserLocaleFromProfileEntity(profile);