diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java index b0af27a09f..7ac095e882 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManager.java @@ -58,111 +58,346 @@ public ExternalAccountManager(final MailJetApiClientWrapper mailjetApi, final IE */ @Override public synchronized void synchroniseChangedUsers() throws ExternalAccountSynchronisationException { + log.info("Starting Mailjet synchronization process"); + + List userRecordsToUpdate; try { - List userRecordsToUpdate = database.getRecentlyChangedRecords(); - - for (UserExternalAccountChanges userRecord : userRecordsToUpdate) { - - Long userId = userRecord.getUserId(); - log.debug(String.format("Processing user: %s", userId)); - try { - - String accountEmail = userRecord.getAccountEmail(); - boolean accountEmailDeliveryFailed = - EmailVerificationStatus.DELIVERY_FAILED.equals(userRecord.getEmailVerificationStatus()); - String mailjetId = userRecord.getProviderUserId(); - JSONObject mailjetDetails; - - if (null != mailjetId) { - // If there is a "mailjet_id", get the account from MailJet. - mailjetDetails = mailjetApi.getAccountByIdOrEmail(mailjetId); - if (userRecord.isDeleted()) { - // Case: deleted from Isaac but still on MailJet: - // Expect: "deleted" but non-null "mailjet_id" - // Action: GDPR deletion, null out MailJet ID?, update provider_last_updated - log.debug("Case: deletion."); - deleteUserFromMailJet(mailjetId, userRecord); - } else if (accountEmailDeliveryFailed) { - // Case: DELIVERY_FAILED but already on MailJet - // Expect: DELIVERY_FAILED, but non-null "mailjet_id" - // Action: same as deletion? Or just remove from lists for now? - log.debug("Case: delivery failed."); - mailjetApi.updateUserSubscriptions(mailjetId, MailJetSubscriptionAction.REMOVE, - MailJetSubscriptionAction.REMOVE); - } else if (!accountEmail.toLowerCase().equals(mailjetDetails.getString("Email"))) { - // Case: account email change: - // Expect: non-null "mailjet_id", email in MailJet != email in database - // Action: delete old email, add new user for new email - log.debug("Case: account email change."); - mailjetApi.permanentlyDeleteAccountById(mailjetId); - mailjetId = mailjetApi.addNewUserOrGetUserIfExists(accountEmail); - updateUserOnMailJet(mailjetId, userRecord); - } else { - // Case: changed details/preferences: - // Expect: not deleted, not DELIVERY_FAILED - // Action: update details, update subscriptions, update provider_last_updated - log.debug("Case: generic preferences update."); - updateUserOnMailJet(mailjetId, userRecord); - } - } else { - if (!accountEmailDeliveryFailed && !userRecord.isDeleted()) { - // Case: new to Isaac, not on MailJet: - // Expect: null "mailjet_id", not DELIVERY_FAILED, not deleted - // Action: create MailJet ID, update details, update subscriptions, update provider_last_updated - // This will upload even users who are not subscribed to emails. - log.debug("Case: new to Isaac/not yet on MailJet"); - mailjetId = mailjetApi.addNewUserOrGetUserIfExists(accountEmail); - updateUserOnMailJet(mailjetId, userRecord); - } else { - // Case: not on MailJet, do not upload to Mailjet: - // Expect: either invalid email, deleted, or not subscribed at all so don't upload. - log.debug("Case: invalid/incorrect/already-unsubscribed user to skip."); - database.updateExternalAccount(userId, null); - } - } - // Iff action done successfully, update the provider_last_updated time: - log.debug("Update provider_last_updated."); - database.updateProviderLastUpdated(userId); - - } catch (SegueDatabaseException e) { - log.error(String.format("Error storing record of MailJet update to user (%s)!", userId)); - } catch (MailjetClientCommunicationException e) { - log.error("Failed to talk to MailJet!"); - throw new ExternalAccountSynchronisationException("Failed to successfully connect to MailJet!"); - } catch (MailjetRateLimitException e) { - log.warn("MailJet rate limiting!"); - throw new ExternalAccountSynchronisationException("MailJet API rate limits exceeded!"); - } catch (MailjetException e) { - log.error(e.getMessage()); - throw new ExternalAccountSynchronisationException(e.getMessage()); - } - } + userRecordsToUpdate = database.getRecentlyChangedRecords(); + log.info("Found {} users to synchronize with Mailjet", userRecordsToUpdate.size()); } catch (SegueDatabaseException e) { - log.error("Database error whilst collecting users whose details have changed!", e); + throw new ExternalAccountSynchronisationException("Failed to retrieve users for synchronization" + + e.getMessage()); } + + if (userRecordsToUpdate.isEmpty()) { + log.info("No users to synchronize"); + return; + } + + SyncMetrics metrics = new SyncMetrics(); + + for (UserExternalAccountChanges userRecord : userRecordsToUpdate) { + Long userId = userRecord.getUserId(); + + try { + processUserSync(userRecord, metrics); + metrics.incrementSuccess(); + + } catch (SegueDatabaseException e) { + metrics.incrementDatabaseError(); + log.error("Database error storing Mailjet update for user ID: {}", userId, e); + } catch (MailjetClientCommunicationException e) { + metrics.incrementCommunicationError(); + throw new ExternalAccountSynchronisationException("Failed to connect to Mailjet: " + e.getMessage()); + } catch (MailjetRateLimitException e) { + metrics.incrementRateLimitError(); + log.warn("Mailjet rate limit exceeded while processing user ID: {}. Processed {} users before limit", + userId, metrics.getSuccessCount()); + throw new ExternalAccountSynchronisationException( + "Mailjet API rate limits exceeded after processing " + metrics.getSuccessCount() + " users" + e); + + } catch (MailjetException e) { + metrics.incrementMailjetError(); + log.error("Mailjet API error while processing user ID: {}. Continuing with next user", userId, e); + } catch (Exception e) { + metrics.incrementUnexpectedError(); + log.error("Unexpected error processing user ID: {}", userId, e); + } + } + + logSyncSummary(metrics, userRecordsToUpdate.size()); } + /** + * Process synchronization for a single user. + */ + private void processUserSync(UserExternalAccountChanges userRecord, SyncMetrics metrics) + throws SegueDatabaseException, MailjetException { + + Long userId = userRecord.getUserId(); + String accountEmail = userRecord.getAccountEmail(); + + if (accountEmail == null || accountEmail.trim().isEmpty()) { + log.warn("User ID {} has null or empty email address. Skipping", userId); + metrics.incrementSkipped(); + return; + } + + boolean accountEmailDeliveryFailed = + EmailVerificationStatus.DELIVERY_FAILED.equals(userRecord.getEmailVerificationStatus()); + String mailjetId = userRecord.getProviderUserId(); + + if (mailjetId != null && !mailjetId.trim().isEmpty()) { + handleExistingMailjetUser(mailjetId, userRecord, accountEmail, accountEmailDeliveryFailed, metrics); + } else { + handleNewMailjetUser(userRecord, accountEmail, accountEmailDeliveryFailed, metrics); + } + + database.updateProviderLastUpdated(userId); + } + + /** + * Handle synchronization for users that already exist in Mailjet. + */ + private void handleExistingMailjetUser(String mailjetId, UserExternalAccountChanges userRecord, + String accountEmail, boolean accountEmailDeliveryFailed, SyncMetrics metrics) + throws SegueDatabaseException, MailjetException { + + Long userId = userRecord.getUserId(); + JSONObject mailjetDetails = mailjetApi.getAccountByIdOrEmail(mailjetId); + + if (mailjetDetails == null) { + log.warn("User ID {} has Mailjet ID {} but account not found. Treating as new user", userId, mailjetId); + database.updateExternalAccount(userId, null); + handleNewMailjetUser(userRecord, accountEmail, accountEmailDeliveryFailed, metrics); + return; + } + + if (userRecord.isDeleted()) { + deleteUserFromMailJet(mailjetId, userRecord); + metrics.incrementDeleted(); + + } else if (accountEmailDeliveryFailed) { + log.info("User ID {} has delivery failed status. Unsubscribing from all lists", userId); + mailjetApi.updateUserSubscriptions(mailjetId, + MailJetSubscriptionAction.REMOVE, + MailJetSubscriptionAction.REMOVE); + metrics.incrementUnsubscribed(); + + } else if (!accountEmail.equalsIgnoreCase(mailjetDetails.getString("Email"))) { + log.info("User ID {} changed email. Recreating Mailjet account", userId); + mailjetApi.permanentlyDeleteAccountById(mailjetId); + String newMailjetId = mailjetApi.addNewUserOrGetUserIfExists(accountEmail); + + if (newMailjetId == null) { + throw new MailjetException("Failed to create new Mailjet account after email change for user: " + userId); + } + + updateUserOnMailJet(newMailjetId, userRecord); + metrics.incrementEmailChanged(); + + } else { + updateUserOnMailJet(mailjetId, userRecord); + metrics.incrementUpdated(); + } + } + + /** + * Handle synchronization for users that don't exist in Mailjet yet. + */ + private void handleNewMailjetUser(UserExternalAccountChanges userRecord, + String accountEmail, boolean accountEmailDeliveryFailed, SyncMetrics metrics) + throws SegueDatabaseException, MailjetException { + + Long userId = userRecord.getUserId(); + + if (!accountEmailDeliveryFailed && !userRecord.isDeleted()) { + log.info("Creating new Mailjet account for user ID {}", userId); + + String mailjetId = mailjetApi.addNewUserOrGetUserIfExists(accountEmail); + + if (mailjetId == null) { + throw new MailjetException("Mailjet returned null ID when creating account for user: " + userId); + } + + updateUserOnMailJet(mailjetId, userRecord); + metrics.incrementCreated(); + + } else { + log.debug("User ID {} not eligible for Mailjet (deleted={}, deliveryFailed={}). Skipping", + userId, userRecord.isDeleted(), accountEmailDeliveryFailed); + database.updateExternalAccount(userId, null); + metrics.incrementSkipped(); + } + } + + /** + * Update user details and subscriptions in Mailjet. + */ private void updateUserOnMailJet(final String mailjetId, final UserExternalAccountChanges userRecord) throws SegueDatabaseException, MailjetException { + + if (mailjetId == null || mailjetId.trim().isEmpty()) { + throw new IllegalArgumentException("Mailjet ID cannot be null or empty"); + } + Long userId = userRecord.getUserId(); - mailjetApi.updateUserProperties(mailjetId, userRecord.getGivenName(), userRecord.getRole().toString(), - userRecord.getEmailVerificationStatus().toString(), userRecord.getStage()); - - MailJetSubscriptionAction newsStatus = (userRecord.allowsNewsEmails() != null - && userRecord.allowsNewsEmails()) ? MailJetSubscriptionAction.FORCE_SUBSCRIBE : - MailJetSubscriptionAction.UNSUBSCRIBE; - MailJetSubscriptionAction eventsStatus = (userRecord.allowsEventsEmails() != null - && userRecord.allowsEventsEmails()) ? MailJetSubscriptionAction.FORCE_SUBSCRIBE : - MailJetSubscriptionAction.UNSUBSCRIBE; - mailjetApi.updateUserSubscriptions(mailjetId, newsStatus, eventsStatus); + String stage = userRecord.getStage() != null ? userRecord.getStage() : "unknown"; + + mailjetApi.updateUserProperties( + mailjetId, + userRecord.getGivenName(), + userRecord.getRole().toString(), + userRecord.getEmailVerificationStatus().toString(), + stage + ); + + MailJetSubscriptionAction newsStatus = Boolean.TRUE.equals(userRecord.allowsNewsEmails()) + ? MailJetSubscriptionAction.FORCE_SUBSCRIBE + : MailJetSubscriptionAction.UNSUBSCRIBE; + + MailJetSubscriptionAction eventsStatus = Boolean.TRUE.equals(userRecord.allowsEventsEmails()) + ? MailJetSubscriptionAction.FORCE_SUBSCRIBE + : MailJetSubscriptionAction.UNSUBSCRIBE; + mailjetApi.updateUserSubscriptions(mailjetId, newsStatus, eventsStatus); database.updateExternalAccount(userId, mailjetId); + + log.debug("Updated Mailjet account {} for user ID {} (news={}, events={})", + mailjetId, userId, newsStatus, eventsStatus); } + /** + * Delete user from Mailjet (GDPR compliance). + */ private void deleteUserFromMailJet(final String mailjetId, final UserExternalAccountChanges userRecord) throws SegueDatabaseException, MailjetException { + + if (mailjetId == null || mailjetId.trim().isEmpty()) { + log.warn("Attempted to delete user with null/empty Mailjet ID. User ID: {}", userRecord.getUserId()); + return; + } + Long userId = userRecord.getUserId(); mailjetApi.permanentlyDeleteAccountById(mailjetId); database.updateExternalAccount(userId, null); + + log.info("Deleted Mailjet account {} for user ID {} (GDPR deletion)", mailjetId, userId); + } + + /** + * Log summary of synchronization results. + */ + private void logSyncSummary(SyncMetrics metrics, int totalUsers) { + log.info("=== Mailjet Synchronization Complete ==="); + log.info("Total users to process: {}", totalUsers); + log.info("Successfully processed: {}", metrics.getSuccessCount()); + log.info(" - Created: {}", metrics.getCreatedCount()); + log.info(" - Updated: {}", metrics.getUpdatedCount()); + log.info(" - Deleted: {}", metrics.getDeletedCount()); + log.info(" - Email changed: {}", metrics.getEmailChangedCount()); + log.info(" - Unsubscribed: {}", metrics.getUnsubscribedCount()); + log.info(" - Skipped: {}", metrics.getSkippedCount()); + log.info("Errors:"); + log.info(" - Database errors: {}", metrics.getDatabaseErrorCount()); + log.info(" - Communication errors: {}", metrics.getCommunicationErrorCount()); + log.info(" - Rate limit errors: {}", metrics.getRateLimitErrorCount()); + log.info(" - Mailjet API errors: {}", metrics.getMailjetErrorCount()); + log.info(" - Unexpected errors: {}", metrics.getUnexpectedErrorCount()); + log.info("========================================"); + } + + /** + * Inner class to track synchronization metrics. + */ + private static class SyncMetrics { + private int successCount = 0; + private int createdCount = 0; + private int updatedCount = 0; + private int deletedCount = 0; + private int emailChangedCount = 0; + private int unsubscribedCount = 0; + private int skippedCount = 0; + private int databaseErrorCount = 0; + private int communicationErrorCount = 0; + private int rateLimitErrorCount = 0; + private int mailjetErrorCount = 0; + private int unexpectedErrorCount = 0; + + void incrementSuccess() { + successCount++; + } + + void incrementCreated() { + createdCount++; + } + + void incrementUpdated() { + updatedCount++; + } + + void incrementDeleted() { + deletedCount++; + } + + void incrementEmailChanged() { + emailChangedCount++; + } + + void incrementUnsubscribed() { + unsubscribedCount++; + } + + void incrementSkipped() { + skippedCount++; + } + + void incrementDatabaseError() { + databaseErrorCount++; + } + + void incrementCommunicationError() { + communicationErrorCount++; + } + + void incrementRateLimitError() { + rateLimitErrorCount++; + } + + void incrementMailjetError() { + mailjetErrorCount++; + } + + void incrementUnexpectedError() { + unexpectedErrorCount++; + } + + int getSuccessCount() { + return successCount; + } + + int getCreatedCount() { + return createdCount; + } + + int getUpdatedCount() { + return updatedCount; + } + + int getDeletedCount() { + return deletedCount; + } + + int getEmailChangedCount() { + return emailChangedCount; + } + + int getUnsubscribedCount() { + return unsubscribedCount; + } + + int getSkippedCount() { + return skippedCount; + } + + int getDatabaseErrorCount() { + return databaseErrorCount; + } + + int getCommunicationErrorCount() { + return communicationErrorCount; + } + + int getRateLimitErrorCount() { + return rateLimitErrorCount; + } + + int getMailjetErrorCount() { + return mailjetErrorCount; + } + + int getUnexpectedErrorCount() { + return unexpectedErrorCount; + } } -} +} \ No newline at end of file diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java b/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java index ac5cf51c3c..25fa9eae8b 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/configuration/SegueGuiceConfigurationModule.java @@ -230,7 +230,7 @@ public class SegueGuiceConfigurationModule extends AbstractModule implements Ser * previously been set. * * @param globalProperties PropertiesLoader object to be used for loading properties - * (if it has not previously been set). + * (if it has not previously been set). */ public static void setGlobalPropertiesIfNotSet(final PropertiesLoader globalProperties) { if (SegueGuiceConfigurationModule.globalProperties == null) { @@ -366,8 +366,6 @@ private void configureSegueSearch() { * Configure user security related classes. */ private void configureAuthenticationProviders() { - MapBinder mapBinder = MapBinder.newMapBinder(binder(), - AuthenticationProvider.class, IAuthenticator.class); this.bindConstantToProperty(Constants.HMAC_SALT, globalProperties); //Google reCAPTCHA @@ -378,6 +376,11 @@ private void configureAuthenticationProviders() { this.bindConstantToProperty(Constants.GOOGLE_CLIENT_SECRET_LOCATION, globalProperties); this.bindConstantToProperty(Constants.GOOGLE_CALLBACK_URI, globalProperties); this.bindConstantToProperty(Constants.GOOGLE_OAUTH_SCOPES, globalProperties); + + MapBinder mapBinder = MapBinder.newMapBinder(binder(), + AuthenticationProvider.class, IAuthenticator.class); + + mapBinder.addBinding(AuthenticationProvider.GOOGLE).to(GoogleAuthenticator.class); // Facebook diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java b/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java index 6fb0992236..cb4bbeae57 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManager.java @@ -1,6 +1,5 @@ package uk.ac.cam.cl.dtg.segue.dao.users; -import com.google.api.client.util.Lists; import com.google.inject.Inject; import java.sql.Connection; import java.sql.PreparedStatement; @@ -8,8 +7,10 @@ import java.sql.SQLException; import java.sql.Timestamp; import java.time.Instant; +import java.util.ArrayList; import java.util.List; import org.json.JSONArray; +import org.json.JSONException; import org.json.JSONObject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -25,6 +26,9 @@ public class PgExternalAccountPersistenceManager implements IExternalAccountDataManager { private static final Logger log = LoggerFactory.getLogger(PgExternalAccountPersistenceManager.class); + private static final String STAGE_UNKNOWN = "unknown"; + private static final String MAILJET = "MAILJET - "; + private final PostgresSqlDb database; /** @@ -39,95 +43,376 @@ public PgExternalAccountPersistenceManager(final PostgresSqlDb database) { @Override public List getRecentlyChangedRecords() throws SegueDatabaseException { - String query = "SELECT id, provider_user_identifier, email, role, given_name, deleted, email_verification_status, " - + " news_prefs.preference_value AS news_emails, events_prefs.preference_value AS events_emails " + // IMPORTANT: registered_contexts is JSONB[] (array of JSONB objects) in PostgreSQL + // We use array_to_json() to convert it to proper JSON that Java can parse + String query = "SELECT users.id, " + + " external_accounts.provider_user_identifier, " + + " users.email, " + + " users.role, " + + " users.given_name, " + + " users.deleted, " + + " users.email_verification_status, " + + " array_to_json(users.registered_contexts) AS registered_contexts, " // Convert JSONB[] to JSON + + " news_prefs.preference_value AS news_emails, " + + " events_prefs.preference_value AS events_emails, " + + " external_accounts.provider_last_updated " + "FROM users " - + " LEFT OUTER JOIN user_preferences AS news_prefs ON users.id = news_prefs.user_id " - + "AND news_prefs.preference_type='EMAIL_PREFERENCE' " - + "AND news_prefs.preference_name='NEWS_AND_UPDATES' " - + " LEFT OUTER JOIN user_preferences AS events_prefs ON users.id = events_prefs.user_id " - + "AND events_prefs.preference_type='EMAIL_PREFERENCE' " - + "AND events_prefs.preference_name='EVENTS' " - + " LEFT OUTER JOIN external_accounts ON users.id=external_accounts.user_id AND provider_name='MailJet' " - + "WHERE (users.last_updated >= provider_last_updated OR news_prefs.last_updated >= provider_last_updated " - + " OR events_prefs.last_updated >= provider_last_updated OR provider_last_updated IS NULL)"; + + " LEFT OUTER JOIN user_preferences AS news_prefs " + + " ON users.id = news_prefs.user_id " + + " AND news_prefs.preference_type = 'EMAIL_PREFERENCE' " + + " AND news_prefs.preference_name = 'NEWS_AND_UPDATES' " + + " LEFT OUTER JOIN user_preferences AS events_prefs " + + " ON users.id = events_prefs.user_id " + + " AND events_prefs.preference_type = 'EMAIL_PREFERENCE' " + + " AND events_prefs.preference_name = 'EVENTS' " + + " LEFT OUTER JOIN external_accounts " + + " ON users.id = external_accounts.user_id " + + " AND external_accounts.provider_name = 'MailJet' " + + "WHERE (users.last_updated >= external_accounts.provider_last_updated " + + " OR news_prefs.last_updated >= external_accounts.provider_last_updated " + + " OR events_prefs.last_updated >= external_accounts.provider_last_updated " + + " OR external_accounts.provider_last_updated IS NULL) " + + "ORDER BY users.id"; + + return executeQueryAndBuildUserRecords(query); + } + + /** + * Execute query and build user records list. + * Extracted to reduce nesting complexity. + */ + private List executeQueryAndBuildUserRecords(String query) + throws SegueDatabaseException { try (Connection conn = database.getDatabaseConnection(); - PreparedStatement pst = conn.prepareStatement(query); - ResultSet results = pst.executeQuery() + PreparedStatement pst = conn.prepareStatement(query) ) { - List listOfResults = Lists.newArrayList(); + log.debug("{} Executing query to fetch recently changed user records", MAILJET); - while (results.next()) { - listOfResults.add(buildUserExternalAccountChanges(results)); - } + try (ResultSet results = pst.executeQuery()) { + List listOfResults = new ArrayList<>(); - return listOfResults; + while (results.next()) { + extracted(results, listOfResults); + } + + log.debug("{} Retrieved {} user records requiring synchronization", MAILJET, listOfResults.size()); + return listOfResults; + } } catch (SQLException e) { - throw new SegueDatabaseException("Postgres exception", e); + String errorMsg = "Database error while fetching recently changed records"; + throw new SegueDatabaseException(errorMsg + ": " + e.getMessage(), e); + } + } + + private void extracted(ResultSet results, List listOfResults) throws SQLException { + try { + UserExternalAccountChanges userChange = buildUserExternalAccountChanges(results); + listOfResults.add(userChange); + } catch (SQLException | JSONException e) { + long userId = results.getLong("id"); + log.error("{} Error building UserExternalAccountChanges for user ID: {}. " + + "Error type: {}, Message: {}. Skipping this user and continuing with next.", + MAILJET, userId, e.getClass().getSimpleName(), e.getMessage(), e); } } @Override public void updateProviderLastUpdated(final Long userId) throws SegueDatabaseException { - String query = "UPDATE external_accounts SET provider_last_updated=? WHERE user_id=? AND provider_name='MailJet';"; + if (userId == null) { + throw new IllegalArgumentException("User ID cannot be null"); + } + + String query = "UPDATE external_accounts " + + "SET provider_last_updated = ? " + + "WHERE user_id = ? " + + "AND provider_name = 'MailJet'"; + try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query) ) { pst.setTimestamp(1, Timestamp.from(Instant.now())); pst.setLong(2, userId); - pst.executeUpdate(); + int rowsUpdated = pst.executeUpdate(); + + if (rowsUpdated == 0) { + log.warn("{} No rows updated when setting provider_last_updated for user ID: {}. " + + "User may not have an external_accounts record yet.", MAILJET, userId); + } else { + log.debug("{} Updated provider_last_updated for user ID: {}", MAILJET, userId); + } + } catch (SQLException e) { - throw new SegueDatabaseException("Postgres exception on update ", e); + String errorMsg = String.format("Database error updating provider_last_updated for user ID: %d", userId); + throw new SegueDatabaseException(errorMsg + ": " + e.getMessage(), e); } } @Override public void updateExternalAccount(final Long userId, final String providerUserIdentifier) throws SegueDatabaseException { - // Upsert the value in, using Postgres 9.5 syntax 'ON CONFLICT DO UPDATE ...' - String query = - "INSERT INTO external_accounts(user_id, provider_name, provider_user_identifier) VALUES (?, 'MailJet', ?)" - + " ON CONFLICT (user_id, provider_name) DO UPDATE SET" - + " provider_user_identifier=excluded.provider_user_identifier"; + + if (userId == null) { + throw new IllegalArgumentException("User ID cannot be null"); + } + + String query = "INSERT INTO external_accounts (user_id, provider_name, provider_user_identifier) " + + "VALUES (?, 'MailJet', ?) " + + "ON CONFLICT (user_id, provider_name) " + + "DO UPDATE SET provider_user_identifier = excluded.provider_user_identifier"; + try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query) ) { pst.setLong(1, userId); pst.setString(2, providerUserIdentifier); - pst.executeUpdate(); + int rowsAffected = pst.executeUpdate(); + + if (rowsAffected > 0) { + log.debug("{} Upserted external_account for user ID: {} with Mailjet ID: {}", + MAILJET, userId, providerUserIdentifier != null ? providerUserIdentifier : "[null]"); + } else { + log.warn("{} Upsert returned 0 rows for user ID: {}. This is unexpected.", MAILJET, userId); + } + } catch (SQLException e) { - throw new SegueDatabaseException("Postgres exception on upsert ", e); + String errorMsg = String.format("Database error upserting external_account for user ID: %d with Mailjet ID: %s", + userId, providerUserIdentifier); + throw new SegueDatabaseException(errorMsg + ": " + e.getMessage(), e); } } - private UserExternalAccountChanges buildUserExternalAccountChanges(final ResultSet results) throws SQLException { - String registeredContextsJson = results.getString("registered_contexts"); + /** + * Build UserExternalAccountChanges object from database result set. + * Extracts stage information from registered_contexts JSONB[] field. + * Parses boolean preference values with proper null handling. + */ + private UserExternalAccountChanges buildUserExternalAccountChanges(final ResultSet results) + throws SQLException { - // Parse the JSON string if it's not null - JSONObject registeredContexts = null; - if (registeredContextsJson != null && !registeredContextsJson.isEmpty()) { - registeredContexts = new JSONObject(registeredContextsJson); - } + Long userId = results.getLong("id"); - // Extract stage from the JSON object, or use a default value - String stage = (registeredContexts != null && registeredContexts.has("stage")) - ? registeredContexts.getString("stage") - : "unknown"; + String registeredContextsJson = results.getString("registered_contexts"); + String stage = extractStageFromRegisteredContexts(userId, registeredContextsJson); + + Boolean newsEmails = parseBooleanPreference(userId, "NEWS_AND_UPDATES", results, "news_emails"); + Boolean eventsEmails = parseBooleanPreference(userId, "EVENTS", results, "events_emails"); return new UserExternalAccountChanges( - results.getLong("id"), + userId, results.getString("provider_user_identifier"), results.getString("email"), Role.valueOf(results.getString("role")), results.getString("given_name"), results.getBoolean("deleted"), EmailVerificationStatus.valueOf(results.getString("email_verification_status")), - results.getBoolean("news_emails"), - results.getBoolean("events_emails"), - stage // Pass the extracted stage as a string + newsEmails, + eventsEmails, + stage ); } + + /** + * Parse boolean preference value from ResultSet with proper null handling. + * PostgreSQL boolean columns can be NULL, which JDBC returns as false by default. + * We need to check wasNull() to distinguish between false and NULL. + * + * @param userId User ID for logging + * @param preferenceName Name of preference for logging + * @param results ResultSet containing the data + * @param columnName Column name in ResultSet + * @return Boolean value (true/false/null) + */ + @javax.annotation.CheckForNull + private Boolean parseBooleanPreference(Long userId, String preferenceName, + ResultSet results, String columnName) throws SQLException { + boolean value = results.getBoolean(columnName); + boolean wasNull = results.wasNull(); + + if (wasNull) { + if (log.isDebugEnabled()) { + log.debug("{} User ID {} has NULL preference for {}. Treating as not subscribed.", + MAILJET, userId, preferenceName); + } + return null; + } + + if (log.isDebugEnabled()) { + log.debug("{} User ID {} has preference {} = {}", MAILJET, userId, preferenceName, value); + } + return value; + } + + /** + * Extract stage information from registered_contexts JSONB[] field. + * + *

+ * PostgreSQL JSONB[] is converted to JSON using array_to_json() in the query. + * This gives us clean JSON like: [{"stage": "gcse", "examBoard": "aqa"}] + * + * @param userId User ID for logging + * @param registeredContextsJson JSONB[] converted to JSON via array_to_json() + * @return stage string: "GCSE", "A Level", "GCSE and A Level", or "unknown" + */ + private String extractStageFromRegisteredContexts(Long userId, String registeredContextsJson) { + if (isNullOrEmpty(registeredContextsJson)) { + logDebugStage(userId, "NULL/empty registered_contexts"); + return STAGE_UNKNOWN; + } + + String trimmed = registeredContextsJson.trim(); + + if (isEmptyJsonArray(trimmed)) { + logDebugStage(userId, "empty/null registered_contexts"); + return STAGE_UNKNOWN; + } + + return parseJsonArrayForStage(userId, trimmed, registeredContextsJson); + } + + /** + * Check if string is null or empty. + */ + private boolean isNullOrEmpty(String str) { + return str == null || str.trim().isEmpty(); + } + + /** + * Check if trimmed string represents an empty JSON array. + */ + private boolean isEmptyJsonArray(String trimmed) { + return "[]".equals(trimmed) || "null".equals(trimmed); + } + + /** + * Log debug message for stage determination. + */ + private void logDebugStage(Long userId, String reason) { + if (log.isDebugEnabled()) { + log.debug("{} User ID {} has {}. Stage: {}", MAILJET, userId, reason, STAGE_UNKNOWN); + } + } + + /** + * Parse JSON array to extract stage information. + * Reduces cognitive complexity by extracting try-catch logic. + */ + private String parseJsonArrayForStage(Long userId, String trimmed, String originalJson) { + try { + JSONArray array = new JSONArray(trimmed); + + if (array.isEmpty()) { + logDebugStage(userId, "empty JSON array in registered_contexts"); + return STAGE_UNKNOWN; + } + + String stageFromArray = extractStageFromArray(userId, array); + if (stageFromArray != null) { + return stageFromArray; + } + + return handleMissingStage(userId, trimmed); + + } catch (JSONException e) { + log.warn("{} User ID {} has invalid JSON in registered_contexts: '{}'. " + + "Error type: {}, Message: {}. Stage: {}", + MAILJET, userId, truncateForLog(originalJson), e.getClass().getSimpleName(), + e.getMessage(), STAGE_UNKNOWN); + return STAGE_UNKNOWN; + } + } + + /** + * Extract stage from JSON array items. + */ + private String extractStageFromArray(Long userId, JSONArray array) { + for (int i = 0; i < array.length(); i++) { + Object item = array.get(i); + if (item instanceof JSONObject obj && obj.has("stage")) { + String stage = obj.getString("stage"); + String normalized = normalizeStage(stage); + if (log.isDebugEnabled()) { + log.debug("{} User ID {} has stage '{}' in registered_contexts[{}]. Normalized: {}", + MAILJET, userId, stage, i, normalized); + } + return normalized; + } + } + return null; + } + + /** + * Handle case where no stage key was found in JSON. + */ + private String handleMissingStage(Long userId, String trimmed) { + String fallbackStage = fallbackStageDetection(trimmed); + if (!STAGE_UNKNOWN.equals(fallbackStage)) { + if (log.isDebugEnabled()) { + log.debug("{} User ID {} stage detected via fallback pattern matching: {}", + MAILJET, userId, fallbackStage); + } + } else { + log.warn("{} User ID {} has registered_contexts but no 'stage' key found: {}. Stage: {}", + MAILJET, userId, trimmed, STAGE_UNKNOWN); + } + return fallbackStage; + } + + /** + * Fallback stage detection by pattern matching in the JSON string. + * Used when no explicit 'stage' key is found. + */ + private String fallbackStageDetection(String jsonString) { + String lower = jsonString.toLowerCase(); + boolean hasGcse = lower.contains("gcse"); + boolean hasLevel = lower.contains("a_level") || lower.contains("alevel") || lower.contains("a level"); + + if (hasGcse && hasLevel) { + return "GCSE and A Level"; + } else if (hasGcse) { + return "GCSE"; + } else if (hasLevel) { + return "A Level"; + } + + return STAGE_UNKNOWN; + } + + /** + * Normalize stage values to consistent format for Mailjet. + */ + private String normalizeStage(String stage) { + if (stage == null || stage.trim().isEmpty()) { + return STAGE_UNKNOWN; + } + + String normalized = stage.trim().toLowerCase(); + + return switch (normalized) { + case "gcse" -> "GCSE"; + case "a_level", "a level", "alevel", "a-level" -> "A Level"; + case "gcse_and_a_level", "gcse and a level", "both", "gcse,a_level", "gcse, a level" -> "GCSE and A Level"; + case "all" -> "ALL"; + default -> { + log.warn("{} Unexpected stage value '{}' encountered. Returning '{}'. " + + "Expected values: gcse, a_level, gcse_and_a_level, both", MAILJET, stage, STAGE_UNKNOWN); + yield STAGE_UNKNOWN; + } + }; + } + + /** + * Truncate long strings for logging to avoid cluttering logs. + */ + private String truncateForLog(String str) { + if (str == null) { + return "null"; + } + if (str.length() <= 100) { + return str; + } + return str.substring(0, 97) + "..."; + } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java b/src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java index b1e5b160ed..50daa7cf06 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java +++ b/src/main/java/uk/ac/cam/cl/dtg/util/email/MailJetApiClientWrapper.java @@ -16,13 +16,12 @@ package uk.ac.cam.cl.dtg.util.email; -import static java.util.Objects.requireNonNull; - import com.google.inject.Inject; import com.mailjet.client.ClientOptions; import com.mailjet.client.MailjetClient; import com.mailjet.client.MailjetRequest; import com.mailjet.client.MailjetResponse; +import com.mailjet.client.errors.MailjetClientCommunicationException; import com.mailjet.client.errors.MailjetClientRequestException; import com.mailjet.client.errors.MailjetException; import com.mailjet.client.resource.Contact; @@ -39,6 +38,9 @@ public class MailJetApiClientWrapper { private static final Logger log = LoggerFactory.getLogger(MailJetApiClientWrapper.class); + + private static final String PROPERTY_VALUE_KEY = "value"; + private final MailjetClient mailjetClient; private final String newsListId; private final String eventsListId; @@ -57,10 +59,12 @@ public class MailJetApiClientWrapper { public MailJetApiClientWrapper(final String mailjetApiKey, final String mailjetApiSecret, final String mailjetNewsListId, final String mailjetEventsListId, final String mailjetLegalListId) { - ClientOptions options = ClientOptions.builder() - .apiKey(mailjetApiKey) - .apiSecretKey(mailjetApiSecret) - .build(); + + if (mailjetApiKey == null || mailjetApiSecret == null) { + throw new IllegalArgumentException("Mailjet API credentials cannot be null"); + } + + ClientOptions options = ClientOptions.builder().apiKey(mailjetApiKey).apiSecretKey(mailjetApiSecret).build(); this.mailjetClient = new MailjetClient(options); this.newsListId = mailjetNewsListId; @@ -72,20 +76,47 @@ public MailJetApiClientWrapper(final String mailjetApiKey, final String mailjetA * Get user details for an existing MailJet account. * * @param mailjetIdOrEmail - email address or MailJet user ID - * @return JSONObject of the MailJet user + * @return JSONObject of the MailJet user, or null if not found * @throws MailjetException - if underlying MailjetClient throws an exception */ public JSONObject getAccountByIdOrEmail(final String mailjetIdOrEmail) throws MailjetException { - if (null == mailjetIdOrEmail) { + if (mailjetIdOrEmail == null || mailjetIdOrEmail.trim().isEmpty()) { + log.debug("Attempted to get account with null/empty identifier"); return null; } - MailjetRequest request = new MailjetRequest(Contact.resource, mailjetIdOrEmail); - MailjetResponse response = mailjetClient.get(request); - JSONArray responseData = response.getData(); - if (response.getTotal() == 1) { - return responseData.getJSONObject(0); + + try { + MailjetRequest request = new MailjetRequest(Contact.resource, mailjetIdOrEmail); + MailjetResponse response = mailjetClient.get(request); + + if (response.getStatus() == 404) { + return null; + } + + if (response.getStatus() != 200) { + log.warn("Unexpected Mailjet response status {} when fetching account", response.getStatus()); + throw new MailjetException("Unexpected response status: " + response.getStatus()); + } + + JSONArray responseData = response.getData(); + if (response.getTotal() == 1 && !responseData.isEmpty()) { + return responseData.getJSONObject(0); + } + + return null; + + } catch (MailjetException e) { + if (isNotFoundException(e)) { + return null; + } + + if (isCommunicationException(e)) { + String errorMsg = String.format("Communication error fetching Mailjet account: %s", mailjetIdOrEmail); + throw new MailjetClientCommunicationException(errorMsg, e); + } + + throw e; } - return null; } /** @@ -95,43 +126,134 @@ public JSONObject getAccountByIdOrEmail(final String mailjetIdOrEmail) throws Ma * @throws MailjetException - if underlying MailjetClient throws an exception */ public void permanentlyDeleteAccountById(final String mailjetId) throws MailjetException { - requireNonNull(mailjetId); - MailjetRequest request = new MailjetRequest(Contacts.resource, mailjetId); - mailjetClient.delete(request); + if (mailjetId == null || mailjetId.trim().isEmpty()) { + throw new IllegalArgumentException("Mailjet ID cannot be null or empty"); + } + + try { + MailjetRequest request = new MailjetRequest(Contacts.resource, mailjetId); + MailjetResponse response = mailjetClient.delete(request); + + if (response.getStatus() == 204 || response.getStatus() == 200) { + log.info("Successfully deleted Mailjet account: {}", mailjetId); + } else if (response.getStatus() == 404) { + log.debug("Attempted to delete non-existent Mailjet account: {}", mailjetId); + } else { + throw new MailjetException("Failed to delete account. Status: " + response.getStatus()); + } + + } catch (MailjetException e) { + if (isNotFoundException(e)) { + log.debug("Mailjet account already deleted or not found: {}", mailjetId); + return; + } + + if (isCommunicationException(e)) { + String errorMsg = String.format("Communication error deleting Mailjet account: %s", mailjetId); + throw new MailjetClientCommunicationException(errorMsg, e); + } + + throw e; + } } /** - * Add a new user to MailJet + * Add a new user to MailJet. *
- * If the user already exists, find by email as a fallback to ensure idempotence and better error recovery. + * If the user already exists, find by email as a fallback to ensure idempotence. * * @param email - email address - * @return the MailJet user ID + * @return the MailJet user ID, or null on failure * @throws MailjetException - if underlying MailjetClient throws an exception */ public String addNewUserOrGetUserIfExists(final String email) throws MailjetException { - if (null == email) { + if (email == null || email.trim().isEmpty()) { + log.warn("Attempted to create Mailjet account with null/empty email"); return null; } + + String normalizedEmail = email.trim().toLowerCase(); + try { - MailjetRequest request = new MailjetRequest(Contact.resource).property(Contact.EMAIL, email); - MailjetResponse response = mailjetClient.post(request); - // Get MailJet ID out: - JSONObject responseData = response.getData().getJSONObject(0); - return Integer.toString(responseData.getInt("ID")); + return createNewMailjetAccount(normalizedEmail); + } catch (MailjetClientRequestException e) { - if (e.getMessage().contains("already exists")) { - // FIXME - we need to test that this response always comes back with "already exists" in the message - log.warn(String.format("Attempted to create a user with email (%s) that already existed!", email)); - JSONObject existingMailJetAccount = getAccountByIdOrEmail(email); - return Integer.toString(existingMailJetAccount.getInt("ID")); - } else { - log.error(String.format("Failed to create user in MailJet with email: %s", email), e); + return handleUserAlreadyExists(e, normalizedEmail); + + } catch (MailjetException e) { + return handleMailjetException(e, normalizedEmail); + + } catch (JSONException e) { + String errorMsg = String.format("JSON parsing error when creating account for email: %s", normalizedEmail); + throw new MailjetException(errorMsg, e); + } + } + + /** + * Create a new Mailjet account for the given email. + * Extracted to reduce cognitive complexity. + */ + private String createNewMailjetAccount(String normalizedEmail) throws MailjetException, JSONException { + try { + MailjetRequest request = new MailjetRequest(Contact.resource).property(Contact.EMAIL, normalizedEmail); + MailjetResponse response = mailjetClient.post(request); + + if (response.getStatus() == 201 || response.getStatus() == 200) { + JSONObject responseData = response.getData().getJSONObject(0); + String mailjetId = String.valueOf(responseData.get("ID")); + log.info("Successfully created Mailjet account: {}", mailjetId); + return mailjetId; } + + throw new MailjetException("Failed to create account. Status: " + response.getStatus()); + } catch (JSONException e) { - log.error(String.format("Failed to create user in MailJet with email: %s", email), e); + String errorMsg = String.format("JSON parsing error when creating account for email: %s", normalizedEmail); + throw new MailjetException(errorMsg, e); } - return null; + } + + /** + * Handle the case where user already exists in Mailjet. + * Extracted to reduce cognitive complexity. + */ + private String handleUserAlreadyExists(MailjetClientRequestException e, String normalizedEmail) + throws MailjetException { + if (e.getMessage() != null && e.getMessage().toLowerCase().contains("already exists")) { + log.debug("User already exists in Mailjet, fetching existing account"); + + try { + JSONObject existingAccount = getAccountByIdOrEmail(normalizedEmail); + if (existingAccount != null) { + String mailjetId = String.valueOf(existingAccount.get("ID")); + log.info("Retrieved existing Mailjet account: {}", mailjetId); + return mailjetId; + } else { + String errorMsg = String.format("User reported as existing but couldn't fetch account: %s", normalizedEmail); + throw new MailjetException(errorMsg); + } + } catch (JSONException je) { + String errorMsg = String.format("JSON parsing error when retrieving existing account: %s", normalizedEmail); + throw new MailjetException(errorMsg, je); + } + } else { + String errorMsg = String.format("Failed to create Mailjet account for email: %s. Error: %s", + normalizedEmail, e.getMessage()); + throw new MailjetException(errorMsg, e); + } + } + + /** + * Handle general Mailjet exceptions. + * Extracted to reduce cognitive complexity. + */ + private String handleMailjetException(MailjetException e, String normalizedEmail) throws MailjetException { + if (isCommunicationException(e)) { + String errorMsg = String.format("Communication error creating Mailjet account for email: %s", normalizedEmail); + throw new MailjetClientCommunicationException(errorMsg, e); + } + + throw e; } /** @@ -141,22 +263,51 @@ public String addNewUserOrGetUserIfExists(final String email) throws MailjetExce * @param firstName - first name of user for contact details * @param role - role of user for contact details * @param emailVerificationStatus - verification status of user for contact details - * @param stage - stages of GCSE or ALevel + * @param stage - stages of GCSE or A Level * @throws MailjetException - if underlying MailjetClient throws an exception */ public void updateUserProperties(final String mailjetId, final String firstName, final String role, - final String emailVerificationStatus, String stage) throws MailjetException { - requireNonNull(mailjetId); - MailjetRequest request = new MailjetRequest(Contactdata.resource, mailjetId) - .property(Contactdata.DATA, new JSONArray() - .put(new JSONObject().put("Name", "firstname").put("value", firstName)) - .put(new JSONObject().put("Name", "role").put("value", role)) - .put(new JSONObject().put("Name", "verification_status").put("value", emailVerificationStatus)) - .put(new JSONObject().put("Name", "stage").put("value", stage)) - ); - MailjetResponse response = mailjetClient.put(request); - if (response.getTotal() != 1) { - throw new MailjetException("Failed to update user!" + response.getTotal()); + final String emailVerificationStatus, final String stage) throws MailjetException { + if (mailjetId == null || mailjetId.trim().isEmpty()) { + throw new IllegalArgumentException("Mailjet ID cannot be null or empty"); + } + + try { + MailjetRequest request = new MailjetRequest(Contactdata.resource, mailjetId).property(Contactdata.DATA, + new JSONArray().put( + new JSONObject().put("Name", "firstname").put(PROPERTY_VALUE_KEY, firstName != null ? firstName : "")) + .put(new JSONObject().put("Name", "role").put(PROPERTY_VALUE_KEY, role != null ? role : "")).put( + new JSONObject().put("Name", "verification_status") + .put(PROPERTY_VALUE_KEY, emailVerificationStatus != null ? emailVerificationStatus : "")) + .put(new JSONObject().put("Name", "stage").put(PROPERTY_VALUE_KEY, stage != null ? stage : "unknown"))); + + MailjetResponse response = mailjetClient.put(request); + + if (response.getStatus() == 200 && response.getTotal() == 1) { + log.debug("Successfully updated properties for Mailjet account: {}", mailjetId); + } else { + throw new MailjetException( + String.format("Failed to update user properties. Status: %d, Total: %d", response.getStatus(), + response.getTotal())); + } + + } catch (JSONException e) { + String errorMsg = String.format("JSON parsing error when updating properties for Mailjet account: %s", mailjetId); + throw new MailjetException(errorMsg, e); + + } catch (MailjetException e) { + if (isNotFoundException(e)) { + String errorMsg = String.format("Mailjet contact not found when updating properties: %s. " + + "Contact may have been deleted", mailjetId); + throw new MailjetException(errorMsg, e); + } + + if (isCommunicationException(e)) { + String errorMsg = String.format("Communication error updating properties for Mailjet account: %s", mailjetId); + throw new MailjetClientCommunicationException(errorMsg, e); + } + + throw e; } } @@ -170,23 +321,76 @@ public void updateUserProperties(final String mailjetId, final String firstName, */ public void updateUserSubscriptions(final String mailjetId, final MailJetSubscriptionAction newsEmails, final MailJetSubscriptionAction eventsEmails) throws MailjetException { - requireNonNull(mailjetId); - MailjetRequest request = new MailjetRequest(ContactManagecontactslists.resource, mailjetId) - .property(ContactManagecontactslists.CONTACTSLISTS, new JSONArray() - .put(new JSONObject() - .put(ContactslistImportList.LISTID, legalListId) - .put(ContactslistImportList.ACTION, MailJetSubscriptionAction.FORCE_SUBSCRIBE.getValue())) - .put(new JSONObject() - .put(ContactslistImportList.LISTID, newsListId) - .put(ContactslistImportList.ACTION, newsEmails.getValue())) - .put(new JSONObject() - .put(ContactslistImportList.LISTID, eventsListId) - .put(ContactslistImportList.ACTION, eventsEmails.getValue())) - ); - MailjetResponse response = mailjetClient.post(request); - if (response.getTotal() != 1) { - throw new MailjetException("Failed to update user subscriptions!" + response.getTotal()); + + if (mailjetId == null || mailjetId.trim().isEmpty()) { + throw new IllegalArgumentException("Mailjet ID cannot be null or empty"); + } + + if (newsEmails == null || eventsEmails == null) { + throw new IllegalArgumentException("Subscription actions cannot be null"); + } + + try { + MailjetRequest request = new MailjetRequest(ContactManagecontactslists.resource, mailjetId).property( + ContactManagecontactslists.CONTACTSLISTS, new JSONArray().put( + new JSONObject().put(ContactslistImportList.LISTID, legalListId) + .put(ContactslistImportList.ACTION, MailJetSubscriptionAction.FORCE_SUBSCRIBE.getValue())).put( + new JSONObject().put(ContactslistImportList.LISTID, newsListId) + .put(ContactslistImportList.ACTION, newsEmails.getValue())).put( + new JSONObject().put(ContactslistImportList.LISTID, eventsListId) + .put(ContactslistImportList.ACTION, eventsEmails.getValue()))); + + MailjetResponse response = mailjetClient.post(request); + + if (response.getStatus() == 201 && response.getTotal() == 1) { + log.debug("Successfully updated subscriptions for Mailjet account: {}", mailjetId); + } else { + throw new MailjetException( + String.format("Failed to update user subscriptions. Status: %d, Total: %d", response.getStatus(), + response.getTotal())); + } + + } catch (JSONException e) { + String errorMsg = + String.format("JSON parsing error when updating subscriptions for Mailjet account: %s", mailjetId); + throw new MailjetException(errorMsg, e); + + } catch (MailjetException e) { + if (isNotFoundException(e)) { + String errorMsg = String.format("Mailjet contact not found when updating subscriptions: %s. " + + "Contact may have been deleted", mailjetId); + throw new MailjetException(errorMsg, e); + } + + if (isCommunicationException(e)) { + String errorMsg = String.format("Communication error updating subscriptions for Mailjet account: %s", + mailjetId); + throw new MailjetClientCommunicationException(errorMsg, e); + } + + throw e; } } + /** + * Check if exception is a 404 not found error. + */ + private boolean isNotFoundException(MailjetException e) { + if (e.getMessage() == null) { + return false; + } + String msg = e.getMessage().toLowerCase(); + return msg.contains("404") || msg.contains("not found") || msg.contains("object not found"); + } + + /** + * Check if exception is a communication/timeout error. + */ + private boolean isCommunicationException(MailjetException e) { + if (e.getMessage() == null) { + return false; + } + String msg = e.getMessage().toLowerCase(); + return msg.contains("timeout") || msg.contains("connection"); + } } diff --git a/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java index c658ca2d98..d23ca4a64c 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/segue/api/managers/ExternalAccountManagerTest.java @@ -6,13 +6,20 @@ import static org.easymock.EasyMock.replay; import static org.easymock.EasyMock.verify; import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import com.mailjet.client.errors.MailjetClientCommunicationException; import com.mailjet.client.errors.MailjetException; +import com.mailjet.client.errors.MailjetRateLimitException; +import java.util.ArrayList; import java.util.List; import org.json.JSONObject; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; import uk.ac.cam.cl.dtg.isaac.dos.users.EmailVerificationStatus; import uk.ac.cam.cl.dtg.isaac.dos.users.Role; import uk.ac.cam.cl.dtg.isaac.dos.users.UserExternalAccountChanges; @@ -25,65 +32,67 @@ class ExternalAccountManagerTest { private ExternalAccountManager externalAccountManager; private IExternalAccountDataManager mockDatabase; - private MailJetApiClientWrapper mailjetApi; + private MailJetApiClientWrapper mockMailjetApi; @BeforeEach public void setUp() { mockDatabase = createMock(IExternalAccountDataManager.class); - mailjetApi = createMock(MailJetApiClientWrapper.class); - externalAccountManager = new ExternalAccountManager(mailjetApi, mockDatabase); + mockMailjetApi = createMock(MailJetApiClientWrapper.class); + externalAccountManager = new ExternalAccountManager(mockMailjetApi, mockDatabase); } @Nested - class SynchroniseChangedUsers { + class SynchroniseChangedUsersTests { + @Test - void synchroniseChangedUsers_newUser() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { - // New user case: - // - providerUserId (mailjetId) is null - // - We expect to call addNewUserOrGetUserIfExists to create a new Mailjet account + void synchroniseChangedUsers_WithNewUser_ShouldCreateAccount() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange UserExternalAccountChanges userChanges = new UserExternalAccountChanges( 1L, null, "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "gcse" + EmailVerificationStatus.VERIFIED, true, false, "GCSE" ); List changedUsers = List.of(userChanges); expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mailjetApi.addNewUserOrGetUserIfExists("test@example.com")).andReturn("mailjetId"); - mailjetApi.updateUserProperties("mailjetId", "John", "STUDENT", "VERIFIED", "gcse"); + expect(mockMailjetApi.addNewUserOrGetUserIfExists("test@example.com")).andReturn("mailjetId123"); + mockMailjetApi.updateUserProperties("mailjetId123", "John", "STUDENT", "VERIFIED", "GCSE"); expectLastCall(); - mailjetApi.updateUserSubscriptions("mailjetId", + mockMailjetApi.updateUserSubscriptions("mailjetId123", MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); expectLastCall(); - mockDatabase.updateExternalAccount(1L, "mailjetId"); + mockDatabase.updateExternalAccount(1L, "mailjetId123"); expectLastCall(); mockDatabase.updateProviderLastUpdated(1L); expectLastCall(); - replay(mockDatabase, mailjetApi); + replay(mockDatabase, mockMailjetApi); + // Act externalAccountManager.synchroniseChangedUsers(); - verify(mockDatabase, mailjetApi); + // Assert + verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_existingUser() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { - // Existing user case: - // - providerUserId (mailjetId) is not null - // - We expect to call getAccountByIdOrEmail to retrieve existing Mailjet account + void synchroniseChangedUsers_WithExistingUser_ShouldUpdateAccount() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange UserExternalAccountChanges userChanges = new UserExternalAccountChanges( 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, - EmailVerificationStatus.VERIFIED, true, false, "gcse" + EmailVerificationStatus.VERIFIED, true, false, "GCSE" ); List changedUsers = List.of(userChanges); - expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); JSONObject mailjetDetails = new JSONObject(); mailjetDetails.put("Email", "test@example.com"); - expect(mailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(mailjetDetails); - mailjetApi.updateUserProperties("existingMailjetId", "John", "STUDENT", "VERIFIED", "gcse"); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(mailjetDetails); + mockMailjetApi.updateUserProperties("existingMailjetId", "John", "STUDENT", "VERIFIED", "GCSE"); expectLastCall(); - mailjetApi.updateUserSubscriptions("existingMailjetId", + mockMailjetApi.updateUserSubscriptions("existingMailjetId", MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); expectLastCall(); mockDatabase.updateExternalAccount(1L, "existingMailjetId"); @@ -91,53 +100,419 @@ void synchroniseChangedUsers_existingUser() throws SegueDatabaseException, Mailj mockDatabase.updateProviderLastUpdated(1L); expectLastCall(); - replay(mockDatabase, mailjetApi); + replay(mockDatabase, mockMailjetApi); + // Act externalAccountManager.synchroniseChangedUsers(); - verify(mockDatabase, mailjetApi); + // Assert + verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_deletedUser() throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + void synchroniseChangedUsers_WithDeletedUser_ShouldDeleteAccount() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange UserExternalAccountChanges userChanges = new UserExternalAccountChanges( 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", true, - EmailVerificationStatus.VERIFIED, true, false, "gcse" + EmailVerificationStatus.VERIFIED, true, false, "GCSE" ); List changedUsers = List.of(userChanges); + JSONObject mailjetDetails = new JSONObject(); + mailjetDetails.put("Email", "test@example.com"); + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(new JSONObject()); - mailjetApi.permanentlyDeleteAccountById("existingMailjetId"); + expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(mailjetDetails); + mockMailjetApi.permanentlyDeleteAccountById("existingMailjetId"); expectLastCall(); mockDatabase.updateExternalAccount(1L, null); expectLastCall(); mockDatabase.updateProviderLastUpdated(1L); expectLastCall(); - replay(mockDatabase, mailjetApi); + replay(mockDatabase, mockMailjetApi); + // Act externalAccountManager.synchroniseChangedUsers(); - verify(mockDatabase, mailjetApi); + // Assert + verify(mockDatabase, mockMailjetApi); } @Test - void synchroniseChangedUsers_mailjetException() throws SegueDatabaseException, MailjetException { + void synchroniseChangedUsers_WithDeliveryFailed_ShouldUnsubscribeFromAll() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange UserExternalAccountChanges userChanges = new UserExternalAccountChanges( 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.DELIVERY_FAILED, true, false, "GCSE" + ); + List changedUsers = List.of(userChanges); + + JSONObject mailjetDetails = new JSONObject(); + mailjetDetails.put("Email", "test@example.com"); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(mailjetDetails); + mockMailjetApi.updateUserSubscriptions("existingMailjetId", + MailJetSubscriptionAction.REMOVE, MailJetSubscriptionAction.REMOVE); + expectLastCall(); + mockDatabase.updateProviderLastUpdated(1L); + expectLastCall(); + + replay(mockDatabase, mockMailjetApi); + + // Act + externalAccountManager.synchroniseChangedUsers(); + + // Assert + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithEmailChange_ShouldRecreateAccount() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange + UserExternalAccountChanges userChanges = new UserExternalAccountChanges( + 1L, "existingMailjetId", "newemail@example.com", Role.STUDENT, "John", false, EmailVerificationStatus.VERIFIED, true, false, "GCSE" ); List changedUsers = List.of(userChanges); + JSONObject oldMailjetDetails = new JSONObject(); + oldMailjetDetails.put("Email", "oldemail@example.com"); + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); - expect(mailjetApi.getAccountByIdOrEmail("existingMailjetId")).andThrow(new MailjetException("Mailjet error")); + expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")).andReturn(oldMailjetDetails); + mockMailjetApi.permanentlyDeleteAccountById("existingMailjetId"); + expectLastCall(); + expect(mockMailjetApi.addNewUserOrGetUserIfExists("newemail@example.com")).andReturn("newMailjetId"); + mockMailjetApi.updateUserProperties("newMailjetId", "John", "STUDENT", "VERIFIED", "GCSE"); + expectLastCall(); + mockMailjetApi.updateUserSubscriptions("newMailjetId", + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); + expectLastCall(); + mockDatabase.updateExternalAccount(1L, "newMailjetId"); + expectLastCall(); + mockDatabase.updateProviderLastUpdated(1L); + expectLastCall(); - replay(mockDatabase, mailjetApi); + replay(mockDatabase, mockMailjetApi); - assertThrows(ExternalAccountSynchronisationException.class, () -> externalAccountManager.synchroniseChangedUsers()); + // Act + externalAccountManager.synchroniseChangedUsers(); - verify(mockDatabase, mailjetApi); + // Assert + verify(mockDatabase, mockMailjetApi); } + + @Test + void synchroniseChangedUsers_WithMailjetIdButAccountNotFound_ShouldTreatAsNew() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange + UserExternalAccountChanges userChanges = new UserExternalAccountChanges( + 1L, "nonExistentMailjetId", "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ); + List changedUsers = List.of(userChanges); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.getAccountByIdOrEmail("nonExistentMailjetId")).andReturn(null); + mockDatabase.updateExternalAccount(1L, null); + expectLastCall(); + expect(mockMailjetApi.addNewUserOrGetUserIfExists("test@example.com")).andReturn("newMailjetId"); + mockMailjetApi.updateUserProperties("newMailjetId", "John", "STUDENT", "VERIFIED", "GCSE"); + expectLastCall(); + mockMailjetApi.updateUserSubscriptions("newMailjetId", + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); + expectLastCall(); + mockDatabase.updateExternalAccount(1L, "newMailjetId"); + expectLastCall(); + mockDatabase.updateProviderLastUpdated(1L); + expectLastCall(); + + replay(mockDatabase, mockMailjetApi); + + // Act + externalAccountManager.synchroniseChangedUsers(); + + // Assert + verify(mockDatabase, mockMailjetApi); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + void synchroniseChangedUsers_WithNullOrEmptyEmail_ShouldSkip(String email) + throws SegueDatabaseException, ExternalAccountSynchronisationException { + // Arrange + UserExternalAccountChanges userChanges = new UserExternalAccountChanges( + 1L, null, email, Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ); + List changedUsers = List.of(userChanges); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + + replay(mockDatabase, mockMailjetApi); + + // Act + externalAccountManager.synchroniseChangedUsers(); + + // Assert - No mailjet calls should be made + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithEmptyList_ShouldReturnEarly() + throws SegueDatabaseException, ExternalAccountSynchronisationException { + // Arrange + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(new ArrayList<>()); + + replay(mockDatabase, mockMailjetApi); + + // Act + externalAccountManager.synchroniseChangedUsers(); + + // Assert + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithDatabaseException_ShouldThrow() throws SegueDatabaseException { + // Arrange + expect(mockDatabase.getRecentlyChangedRecords()) + .andThrow(new SegueDatabaseException("Database error")); + + replay(mockDatabase); + + // Act & Assert + assertThrows(ExternalAccountSynchronisationException.class, + () -> externalAccountManager.synchroniseChangedUsers()); + + verify(mockDatabase); + } + + @Test + void synchroniseChangedUsers_WithMailjetException_ShouldLogAndContinue() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange + UserExternalAccountChanges userChanges = new UserExternalAccountChanges( + 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ); + List changedUsers = List.of(userChanges); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")) + .andThrow(new MailjetException("Mailjet error")); + + replay(mockDatabase, mockMailjetApi); + + // Act - Should NOT throw - regular MailjetException is caught and logged + externalAccountManager.synchroniseChangedUsers(); + + // Assert + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithCommunicationException_ShouldThrow() + throws SegueDatabaseException, MailjetException { + // Arrange + UserExternalAccountChanges userChanges = new UserExternalAccountChanges( + 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ); + List changedUsers = List.of(userChanges); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")) + .andThrow(new MailjetClientCommunicationException("Communication error")); + + replay(mockDatabase, mockMailjetApi); + + // Act & Assert + assertThrows(ExternalAccountSynchronisationException.class, + () -> externalAccountManager.synchroniseChangedUsers()); + + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithRateLimitException_ShouldThrow() + throws SegueDatabaseException, MailjetException { + // Arrange + UserExternalAccountChanges userChanges = new UserExternalAccountChanges( + 1L, "existingMailjetId", "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ); + List changedUsers = List.of(userChanges); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.getAccountByIdOrEmail("existingMailjetId")) + .andThrow(new MailjetRateLimitException("Rate limit exceeded")); + + replay(mockDatabase, mockMailjetApi); + + // Act & Assert + ExternalAccountSynchronisationException exception = assertThrows( + ExternalAccountSynchronisationException.class, + () -> externalAccountManager.synchroniseChangedUsers()); + + assertTrue(exception.getMessage().contains("rate limit")); + + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithDatabaseErrorDuringUpdate_ShouldLogAndContinue() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange + UserExternalAccountChanges user1 = new UserExternalAccountChanges( + 1L, "mailjetId1", "test1@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ); + UserExternalAccountChanges user2 = new UserExternalAccountChanges( + 2L, "mailjetId2", "test2@example.com", Role.TEACHER, "Jane", false, + EmailVerificationStatus.VERIFIED, false, true, "A Level" + ); + List changedUsers = List.of(user1, user2); + + JSONObject mailjet1 = new JSONObject(); + mailjet1.put("Email", "test1@example.com"); + JSONObject mailjet2 = new JSONObject(); + mailjet2.put("Email", "test2@example.com"); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + + // First user - database error during update + expect(mockMailjetApi.getAccountByIdOrEmail("mailjetId1")).andReturn(mailjet1); + mockMailjetApi.updateUserProperties("mailjetId1", "John", "STUDENT", "VERIFIED", "GCSE"); + expectLastCall(); + mockMailjetApi.updateUserSubscriptions("mailjetId1", + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); + expectLastCall(); + mockDatabase.updateExternalAccount(1L, "mailjetId1"); + expectLastCall().andThrow(new SegueDatabaseException("DB error")); + + // Second user - should still process + expect(mockMailjetApi.getAccountByIdOrEmail("mailjetId2")).andReturn(mailjet2); + mockMailjetApi.updateUserProperties("mailjetId2", "Jane", "TEACHER", "VERIFIED", "A Level"); + expectLastCall(); + mockMailjetApi.updateUserSubscriptions("mailjetId2", + MailJetSubscriptionAction.UNSUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE); + expectLastCall(); + mockDatabase.updateExternalAccount(2L, "mailjetId2"); + expectLastCall(); + mockDatabase.updateProviderLastUpdated(2L); + expectLastCall(); + + replay(mockDatabase, mockMailjetApi); + + // Act - Should not throw, continues processing + externalAccountManager.synchroniseChangedUsers(); + + // Assert + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithMultipleUsers_ShouldProcessAll() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange + UserExternalAccountChanges user1 = new UserExternalAccountChanges( + 1L, null, "new@example.com", Role.STUDENT, "Alice", false, + EmailVerificationStatus.VERIFIED, true, true, "GCSE" + ); + UserExternalAccountChanges user2 = new UserExternalAccountChanges( + 2L, "existingId", "existing@example.com", Role.TEACHER, "Bob", false, + EmailVerificationStatus.VERIFIED, false, false, "A Level" + ); + List changedUsers = List.of(user1, user2); + + JSONObject mailjet2 = new JSONObject(); + mailjet2.put("Email", "existing@example.com"); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + + // User 1 - new user + expect(mockMailjetApi.addNewUserOrGetUserIfExists("new@example.com")).andReturn("newId"); + mockMailjetApi.updateUserProperties("newId", "Alice", "STUDENT", "VERIFIED", "GCSE"); + expectLastCall(); + mockMailjetApi.updateUserSubscriptions("newId", + MailJetSubscriptionAction.FORCE_SUBSCRIBE, MailJetSubscriptionAction.FORCE_SUBSCRIBE); + expectLastCall(); + mockDatabase.updateExternalAccount(1L, "newId"); + expectLastCall(); + mockDatabase.updateProviderLastUpdated(1L); + expectLastCall(); + + // User 2 - existing user + expect(mockMailjetApi.getAccountByIdOrEmail("existingId")).andReturn(mailjet2); + mockMailjetApi.updateUserProperties("existingId", "Bob", "TEACHER", "VERIFIED", "A Level"); + expectLastCall(); + mockMailjetApi.updateUserSubscriptions("existingId", + MailJetSubscriptionAction.UNSUBSCRIBE, MailJetSubscriptionAction.UNSUBSCRIBE); + expectLastCall(); + mockDatabase.updateExternalAccount(2L, "existingId"); + expectLastCall(); + mockDatabase.updateProviderLastUpdated(2L); + expectLastCall(); + + replay(mockDatabase, mockMailjetApi); + + // Act + externalAccountManager.synchroniseChangedUsers(); + + // Assert + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithUnexpectedError_ShouldLogAndContinue() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange + UserExternalAccountChanges userChanges = new UserExternalAccountChanges( + 1L, "mailjetId", "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ); + List changedUsers = List.of(userChanges); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.getAccountByIdOrEmail("mailjetId")) + .andThrow(new RuntimeException("Unexpected error")); + + replay(mockDatabase, mockMailjetApi); + + // Act - Should not throw, logs error and continues + externalAccountManager.synchroniseChangedUsers(); + + // Assert + verify(mockDatabase, mockMailjetApi); + } + + @Test + void synchroniseChangedUsers_WithNewUserAndNullMailjetId() + throws SegueDatabaseException, MailjetException, ExternalAccountSynchronisationException { + // Arrange + UserExternalAccountChanges userChanges = new UserExternalAccountChanges( + 1L, null, "test@example.com", Role.STUDENT, "John", false, + EmailVerificationStatus.VERIFIED, true, false, "GCSE" + ); + List changedUsers = List.of(userChanges); + + expect(mockDatabase.getRecentlyChangedRecords()).andReturn(changedUsers); + expect(mockMailjetApi.addNewUserOrGetUserIfExists("test@example.com")).andReturn(null); + + replay(mockDatabase, mockMailjetApi); + + // Act & Assert + externalAccountManager.synchroniseChangedUsers(); + + verify(mockDatabase, mockMailjetApi); + } + } } diff --git a/src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java index 8d3c10c70c..16cd0726be 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/segue/dao/users/PgExternalAccountPersistenceManagerTest.java @@ -1,74 +1,148 @@ package uk.ac.cam.cl.dtg.segue.dao.users; +import static org.easymock.EasyMock.anyString; import static org.easymock.EasyMock.createMock; import static org.easymock.EasyMock.expect; +import static org.easymock.EasyMock.expectLastCall; import static org.easymock.EasyMock.replay; +import static org.easymock.EasyMock.reset; import static org.easymock.EasyMock.verify; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.sql.Connection; +import java.sql.PreparedStatement; import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.List; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; -import uk.ac.cam.cl.dtg.isaac.dos.users.EmailVerificationStatus; -import uk.ac.cam.cl.dtg.isaac.dos.users.Role; import uk.ac.cam.cl.dtg.isaac.dos.users.UserExternalAccountChanges; -import uk.ac.cam.cl.dtg.util.ReflectionUtils; +import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; +import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb; class PgExternalAccountPersistenceManagerTest { private PgExternalAccountPersistenceManager persistenceManager; + private PostgresSqlDb mockDatabase; + private Connection mockConnection; + private PreparedStatement mockPreparedStatement; private ResultSet mockResultSet; @BeforeEach void setUp() { - persistenceManager = new PgExternalAccountPersistenceManager(null); + mockDatabase = createMock(PostgresSqlDb.class); + mockConnection = createMock(Connection.class); + mockPreparedStatement = createMock(PreparedStatement.class); mockResultSet = createMock(ResultSet.class); + persistenceManager = new PgExternalAccountPersistenceManager(mockDatabase); } - @Test - void buildUserExternalAccountChanges_ShouldParseRegisteredContextsCorrectly() throws Exception { - // Arrange - String registeredContextsJson = "{\"stage\": \"gcse\", \"examBoard\": \"ocr\"}"; - - // Mock ResultSet behavior - expect(mockResultSet.getString("registered_contexts")).andReturn(registeredContextsJson); // Expect this call once - expect(mockResultSet.getLong("id")).andReturn(1L); - expect(mockResultSet.getString("provider_user_identifier")).andReturn("providerId"); - expect(mockResultSet.getString("email")).andReturn("test@example.com"); - expect(mockResultSet.getString("role")).andReturn("STUDENT"); - expect(mockResultSet.getString("given_name")).andReturn("John"); - expect(mockResultSet.getBoolean("deleted")).andReturn(false); - expect(mockResultSet.getString("email_verification_status")).andReturn("VERIFIED"); - expect(mockResultSet.getBoolean("news_emails")).andReturn(true); - expect(mockResultSet.getBoolean("events_emails")).andReturn(false); - - replay(mockResultSet); - - // Act - UserExternalAccountChanges result = ReflectionUtils.invokePrivateMethod( - persistenceManager, - "buildUserExternalAccountChanges", - new Class[]{ResultSet.class}, - new Object[]{mockResultSet} - ); - - // Assert - verify(mockResultSet); // Verify all expected calls were made - assertNotNull(result); - assertEquals(1L, result.getUserId()); - assertEquals("providerId", result.getProviderUserId()); - assertEquals("test@example.com", result.getAccountEmail()); - assertEquals(Role.STUDENT, result.getRole()); - assertEquals("John", result.getGivenName()); - assertFalse(result.isDeleted()); - assertEquals(EmailVerificationStatus.VERIFIED, result.getEmailVerificationStatus()); - assertTrue(result.allowsNewsEmails()); - assertFalse(result.allowsEventsEmails()); - - // Verify JSON parsing and stage extraction - assertEquals("gcse", result.getStage()); // Ensure "stage" is correctly extracted from JSON + @AfterEach + void tearDown() { + reset(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet); } -} \ No newline at end of file + + @Nested + class GetRecentlyChangedRecordsTests { + + @Test + void getRecentlyChangedRecords_WithValidData_ShouldReturnUserList() throws Exception { + // Arrange + String registeredContextsJson = "[{\"stage\": \"gcse\"}]"; + + expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection); + expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement); + expect(mockPreparedStatement.executeQuery()).andReturn(mockResultSet); + + expect(mockResultSet.next()).andReturn(true).once(); + expect(mockResultSet.next()).andReturn(false).once(); + + setupMockResultSetForUser(1L, "mailjetId123", "test@example.com", "STUDENT", + "John", false, "VERIFIED", registeredContextsJson, true, false, false, false); + + mockResultSet.close(); + expectLastCall(); + mockPreparedStatement.close(); + expectLastCall(); + mockConnection.close(); + expectLastCall(); + + replay(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet); + + // Act + List result = persistenceManager.getRecentlyChangedRecords(); + + // Assert + verify(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet); + assertEquals(1, result.size()); + UserExternalAccountChanges user = result.get(0); + assertEquals(1L, user.getUserId()); + assertEquals("mailjetId123", user.getProviderUserId()); + assertEquals("test@example.com", user.getAccountEmail()); + assertEquals("GCSE", user.getStage()); + } + + @Test + void getRecentlyChangedRecords_WithEmptyResults_ShouldReturnEmptyList() throws Exception { + // Arrange + expect(mockDatabase.getDatabaseConnection()).andReturn(mockConnection); + expect(mockConnection.prepareStatement(anyString())).andReturn(mockPreparedStatement); + expect(mockPreparedStatement.executeQuery()).andReturn(mockResultSet); + expect(mockResultSet.next()).andReturn(false); + + mockResultSet.close(); + expectLastCall(); + mockPreparedStatement.close(); + expectLastCall(); + mockConnection.close(); + expectLastCall(); + + replay(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet); + + // Act + List result = persistenceManager.getRecentlyChangedRecords(); + + // Assert + verify(mockDatabase, mockConnection, mockPreparedStatement, mockResultSet); + assertTrue(result.isEmpty()); + } + + @Test + void getRecentlyChangedRecords_WithDatabaseError_ShouldThrowException() throws Exception { + // Arrange + expect(mockDatabase.getDatabaseConnection()).andThrow(new SQLException("Connection failed")); + + replay(mockDatabase); + + // Act & Assert + assertThrows(SegueDatabaseException.class, + () -> persistenceManager.getRecentlyChangedRecords()); + + verify(mockDatabase); + } + } + + // Helper method to setup mock ResultSet with all expected calls + private void setupMockResultSetForUser(Long userId, String mailjetId, String email, String role, + String givenName, boolean deleted, String verificationStatus, + String registeredContexts, boolean newsEmails, boolean eventsEmails, + boolean newsWasNull, boolean eventsWasNull) throws SQLException { + expect(mockResultSet.getLong("id")).andReturn(userId); + expect(mockResultSet.getString("provider_user_identifier")).andReturn(mailjetId); + expect(mockResultSet.getString("email")).andReturn(email); + expect(mockResultSet.getString("role")).andReturn(role); + expect(mockResultSet.getString("given_name")).andReturn(givenName); + expect(mockResultSet.getBoolean("deleted")).andReturn(deleted); + expect(mockResultSet.getString("email_verification_status")).andReturn(verificationStatus); + expect(mockResultSet.getBoolean("news_emails")).andReturn(newsEmails); + expect(mockResultSet.wasNull()).andReturn(newsWasNull); + expect(mockResultSet.getBoolean("events_emails")).andReturn(eventsEmails); + expect(mockResultSet.wasNull()).andReturn(eventsWasNull); + expect(mockResultSet.getString("registered_contexts")).andReturn(registeredContexts); + } + +} diff --git a/src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java b/src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java index f94e25f30f..54870aa6af 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/segue/util/email/MailJetApiClientWrapperTest.java @@ -1,23 +1,24 @@ package uk.ac.cam.cl.dtg.segue.util.email; -import static org.easymock.EasyMock.anyObject; -import static org.easymock.EasyMock.createMock; -import static org.easymock.EasyMock.expect; -import static org.easymock.EasyMock.replay; -import static org.easymock.EasyMock.verify; -import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertNull; - import com.mailjet.client.MailjetClient; import com.mailjet.client.MailjetRequest; import com.mailjet.client.MailjetResponse; +import com.mailjet.client.errors.MailjetClientRequestException; import com.mailjet.client.errors.MailjetException; +import com.mailjet.client.errors.MailjetClientCommunicationException; import org.json.JSONArray; import org.json.JSONObject; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; import uk.ac.cam.cl.dtg.util.email.MailJetApiClientWrapper; +import uk.ac.cam.cl.dtg.util.email.MailJetSubscriptionAction; + +import static org.easymock.EasyMock.*; +import static org.junit.jupiter.api.Assertions.*; class MailJetApiClientWrapperTest { @@ -34,57 +35,640 @@ void setUp() { injectMockMailjetClient(mailJetApiClientWrapper, mockMailjetClient); } - @Test - void getAccountByIdOrEmail_WithValidInput_ShouldReturnAccount() throws MailjetException { - // Arrange - String mailjetId = "123"; - MailjetResponse mockResponse = createMock(MailjetResponse.class); - JSONArray mockData = new JSONArray(); - JSONObject mockAccount = new JSONObject(); - mockAccount.put("ID", 123); - mockData.put(mockAccount); - - expect(mockMailjetClient.get(anyObject(MailjetRequest.class))).andReturn(mockResponse); - expect(mockResponse.getTotal()).andReturn(1); - expect(mockResponse.getData()).andReturn(mockData); - - replay(mockMailjetClient, mockResponse); - - // Act - JSONObject result = mailJetApiClientWrapper.getAccountByIdOrEmail(mailjetId); - - // Assert - verify(mockMailjetClient, mockResponse); - assertNotNull(result); - assertEquals(123, result.getInt("ID")); + @Nested + class ConstructorTests { + + @Test + void constructor_WithNullApiKey_ShouldThrowException() { + assertThrows(IllegalArgumentException.class, + () -> new MailJetApiClientWrapper(null, "secret", "news", "events", "legal")); + } + + @Test + void constructor_WithNullApiSecret_ShouldThrowException() { + assertThrows(IllegalArgumentException.class, + () -> new MailJetApiClientWrapper("key", null, "news", "events", "legal")); + } + + @Test + void constructor_WithValidCredentials_ShouldInitialize() { + assertDoesNotThrow(() -> + new MailJetApiClientWrapper("key", "secret", "news", "events", "legal")); + } + } + + @Nested + class GetAccountByIdOrEmailTests { + + @Test + void getAccountByIdOrEmail_WithValidId_ShouldReturnAccount() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + JSONArray mockData = new JSONArray(); + JSONObject mockAccount = new JSONObject(); + mockAccount.put("ID", 123); + mockAccount.put("Email", "test@example.com"); + mockData.put(mockAccount); + + expect(mockMailjetClient.get(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(200).times(2); + expect(mockResponse.getTotal()).andReturn(1); + expect(mockResponse.getData()).andReturn(mockData); + + replay(mockMailjetClient, mockResponse); + + // Act + JSONObject result = mailJetApiClientWrapper.getAccountByIdOrEmail(mailjetId); + + // Assert + verify(mockMailjetClient, mockResponse); + assertNotNull(result); + assertEquals(123, result.getInt("ID")); + } + + @Test + void getAccountByIdOrEmail_WithNotFound_ShouldReturnNull() throws MailjetException { + // Arrange + String mailjetId = "999"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.get(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(404); + + replay(mockMailjetClient, mockResponse); + + // Act + JSONObject result = mailJetApiClientWrapper.getAccountByIdOrEmail(mailjetId); + + // Assert + verify(mockMailjetClient, mockResponse); + assertNull(result); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + void getAccountByIdOrEmail_WithNullOrEmptyId_ShouldReturnNull(String input) throws MailjetException { + // Act + JSONObject result = mailJetApiClientWrapper.getAccountByIdOrEmail(input); + + // Assert + assertNull(result); + } + + @Test + void getAccountByIdOrEmail_WithUnexpectedStatus_ShouldThrowException() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.get(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + + replay(mockMailjetClient, mockResponse); + + // Act & Assert + assertThrows(MailjetException.class, + () -> mailJetApiClientWrapper.getAccountByIdOrEmail(mailjetId)); + + verify(mockMailjetClient, mockResponse); + } + + @Test + void getAccountByIdOrEmail_WithCommunicationError_ShouldThrowCommunicationException() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetException timeoutException = new MailjetClientCommunicationException("Timeout occurred"); + + expect(mockMailjetClient.get(anyObject(MailjetRequest.class))).andThrow(timeoutException); + + replay(mockMailjetClient); + + // Act & Assert + assertThrows(MailjetClientCommunicationException.class, + () -> mailJetApiClientWrapper.getAccountByIdOrEmail(mailjetId)); + + verify(mockMailjetClient); + } + + @Test + void getAccountByIdOrEmail_WithEmptyResponse_ShouldReturnNull() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + JSONArray emptyData = new JSONArray(); + + expect(mockMailjetClient.get(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(200).times(2); + expect(mockResponse.getTotal()).andReturn(0); + expect(mockResponse.getData()).andReturn(emptyData); + + replay(mockMailjetClient, mockResponse); + + // Act + JSONObject result = mailJetApiClientWrapper.getAccountByIdOrEmail(mailjetId); + + // Assert + verify(mockMailjetClient, mockResponse); + assertNull(result); + } + } + + @Nested + class PermanentlyDeleteAccountTests { + + @Test + void permanentlyDeleteAccount_WithValidId_ShouldDelete() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.delete(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(204); + + replay(mockMailjetClient, mockResponse); + + // Act + mailJetApiClientWrapper.permanentlyDeleteAccountById(mailjetId); + + // Assert + verify(mockMailjetClient, mockResponse); + } + + @Test + void permanentlyDeleteAccount_WithStatus200_ShouldSucceed() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.delete(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(200); + expect(mockResponse.getStatus()).andReturn(200); + + replay(mockMailjetClient, mockResponse); + + // Act + mailJetApiClientWrapper.permanentlyDeleteAccountById(mailjetId); + + // Assert + verify(mockMailjetClient, mockResponse); + } + + @Test + void permanentlyDeleteAccount_WithNotFound_ShouldNotThrow() throws MailjetException { + // Arrange + String mailjetId = "999"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.delete(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(200); + expect(mockResponse.getStatus()).andReturn(200); + + replay(mockMailjetClient, mockResponse); + + // Act - Should not throw + mailJetApiClientWrapper.permanentlyDeleteAccountById(mailjetId); + + // Assert + verify(mockMailjetClient, mockResponse); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + void permanentlyDeleteAccount_WithNullOrEmptyId_ShouldThrowException(String input) { + // Act & Assert + assertThrows(IllegalArgumentException.class, + () -> mailJetApiClientWrapper.permanentlyDeleteAccountById(input)); + } + + @Test + void permanentlyDeleteAccount_WithUnexpectedStatus_ShouldThrowException() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.delete(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + + replay(mockMailjetClient, mockResponse); + + // Act & Assert + assertThrows(MailjetException.class, + () -> mailJetApiClientWrapper.permanentlyDeleteAccountById(mailjetId)); + + verify(mockMailjetClient, mockResponse); + } + + @Test + void permanentlyDeleteAccount_WithCommunicationError_ShouldThrowCommunicationException() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetException connectionException = new MailjetException("Connection refused"); + + expect(mockMailjetClient.delete(anyObject(MailjetRequest.class))).andThrow(connectionException); + + replay(mockMailjetClient); + + // Act & Assert + assertThrows(MailjetClientCommunicationException.class, + () -> mailJetApiClientWrapper.permanentlyDeleteAccountById(mailjetId)); + + verify(mockMailjetClient); + } + + @Test + void permanentlyDeleteAccount_WithNotFoundException_ShouldNotThrow() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetException notFoundException = new MailjetException("Object not found (404)"); + + expect(mockMailjetClient.delete(anyObject(MailjetRequest.class))).andThrow(notFoundException); + + replay(mockMailjetClient); + + // Act - Should not throw + assertDoesNotThrow(() -> mailJetApiClientWrapper.permanentlyDeleteAccountById(mailjetId)); + + verify(mockMailjetClient); + } + } + + @Nested + class AddNewUserOrGetUserIfExistsTests { + + @Test + void addNewUser_WithNewEmail_ShouldReturnNewId() throws MailjetException { + // Arrange + String email = "test@example.com"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + JSONArray mockData = new JSONArray(); + JSONObject mockUser = new JSONObject(); + mockUser.put("ID", 456); + mockData.put(mockUser); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(201); + expect(mockResponse.getData()).andReturn(mockData); + + replay(mockMailjetClient, mockResponse); + + // Act + String result = mailJetApiClientWrapper.addNewUserOrGetUserIfExists(email); + + // Assert + verify(mockMailjetClient, mockResponse); + assertEquals("456", result); + } + + @Test + void addNewUser_WithExistingEmail_ShouldFetchAndReturnId() throws MailjetException { + // Arrange + String email = "existing@example.com"; + MailjetResponse postResponse = createMock(MailjetResponse.class); + MailjetResponse getResponse = createMock(MailjetResponse.class); + + JSONArray getData = new JSONArray(); + JSONObject existingUser = new JSONObject(); + existingUser.put("ID", 789); + existingUser.put("Email", email); + getData.put(existingUser); + + // Simulate "already exists" error on POST + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))) + .andThrow(new MailjetClientRequestException("Email already exists", 1)); + + // Then successful GET to retrieve existing account + expect(mockMailjetClient.get(anyObject(MailjetRequest.class))).andReturn(getResponse); + expect(getResponse.getStatus()).andReturn(200).times(2); + expect(getResponse.getTotal()).andReturn(1); + expect(getResponse.getData()).andReturn(getData); + + replay(mockMailjetClient, postResponse, getResponse); + + // Act + String result = mailJetApiClientWrapper.addNewUserOrGetUserIfExists(email); + + // Assert + verify(mockMailjetClient, getResponse); + assertEquals("789", result); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + void addNewUser_WithNullOrEmptyEmail_ShouldReturnNull(String input) throws MailjetException { + // Act + String result = mailJetApiClientWrapper.addNewUserOrGetUserIfExists(input); + + // Assert + assertNull(result); + } + + @Test + void addNewUser_WithEmailNormalization_ShouldTrimAndLowercase() throws MailjetException { + // Arrange + String email = " Test@EXAMPLE.COM "; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + JSONArray mockData = new JSONArray(); + JSONObject mockUser = new JSONObject(); + mockUser.put("ID", 999); + mockData.put(mockUser); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(201); + expect(mockResponse.getData()).andReturn(mockData); + + replay(mockMailjetClient, mockResponse); + + // Act + String result = mailJetApiClientWrapper.addNewUserOrGetUserIfExists(email); + + // Assert + verify(mockMailjetClient, mockResponse); + assertEquals("999", result); + } + + @Test + void addNewUser_WithUnexpectedStatus_ShouldThrowException() throws MailjetException { + // Arrange + String email = "test@example.com"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + + replay(mockMailjetClient, mockResponse); + + // Act & Assert + assertThrows(MailjetException.class, + () -> mailJetApiClientWrapper.addNewUserOrGetUserIfExists(email)); + + verify(mockMailjetClient, mockResponse); + } + + @Test + void addNewUser_WithExistingButCannotFetch_ShouldThrowException() throws MailjetException { + // Arrange + String email = "existing@example.com"; + MailjetResponse getResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))) + .andThrow(new MailjetClientRequestException("Email already exists", 1)); + + expect(mockMailjetClient.get(anyObject(MailjetRequest.class))).andReturn(getResponse); + expect(getResponse.getStatus()).andReturn(404); + + replay(mockMailjetClient, getResponse); + + // Act & Assert + assertThrows(MailjetException.class, + () -> mailJetApiClientWrapper.addNewUserOrGetUserIfExists(email)); + + verify(mockMailjetClient, getResponse); + } + + @Test + void addNewUser_WithCommunicationError_ShouldThrowCommunicationException() throws MailjetException { + // Arrange + String email = "test@example.com"; + MailjetException timeoutException = new MailjetException("Connection timeout"); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andThrow(timeoutException); + + replay(mockMailjetClient); + + // Act & Assert + assertThrows(MailjetClientCommunicationException.class, + () -> mailJetApiClientWrapper.addNewUserOrGetUserIfExists(email)); + + verify(mockMailjetClient); + } } - @Test - void addNewUserOrGetUserIfExists_WithNewEmail_ShouldReturnNewId() throws MailjetException { - // Arrange - String email = "test@example.com"; - MailjetResponse mockResponse = createMock(MailjetResponse.class); - JSONArray mockData = new JSONArray(); - JSONObject mockUser = new JSONObject(); - mockUser.put("ID", 456); - mockData.put(mockUser); + @Nested + class UpdateUserPropertiesTests { + + @Test + void updateUserProperties_WithValidData_ShouldUpdate() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.put(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(200); + expect(mockResponse.getTotal()).andReturn(1); + + replay(mockMailjetClient, mockResponse); + + // Act + mailJetApiClientWrapper.updateUserProperties(mailjetId, "John", "STUDENT", "VERIFIED", "GCSE"); + + // Assert + verify(mockMailjetClient, mockResponse); + } + + @Test + void updateUserProperties_WithNullValues_ShouldUseEmptyStrings() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.put(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(200); + expect(mockResponse.getTotal()).andReturn(1); + + replay(mockMailjetClient, mockResponse); + + // Act + mailJetApiClientWrapper.updateUserProperties(mailjetId, null, null, null, null); + + // Assert + verify(mockMailjetClient, mockResponse); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + void updateUserProperties_WithNullOrEmptyId_ShouldThrowException(String input) { + // Act & Assert + assertThrows(IllegalArgumentException.class, + () -> mailJetApiClientWrapper.updateUserProperties(input, "John", "STUDENT", "VERIFIED", "GCSE")); + } + + @Test + void updateUserProperties_WithNotFound_ShouldThrowException() throws MailjetException { + // Arrange + String mailjetId = "999"; + MailjetException notFoundException = new MailjetException("Contact not found (404)"); + + expect(mockMailjetClient.put(anyObject(MailjetRequest.class))).andThrow(notFoundException); + + replay(mockMailjetClient); - expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andReturn(mockResponse); - expect(mockResponse.getData()).andReturn(mockData); + // Act & Assert + assertThrows(MailjetException.class, + () -> mailJetApiClientWrapper.updateUserProperties(mailjetId, "John", "STUDENT", "VERIFIED", "GCSE")); - replay(mockMailjetClient, mockResponse); + verify(mockMailjetClient); + } + + @Test + void updateUserProperties_WithUnexpectedStatus_ShouldThrowException() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.put(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getTotal()).andReturn(0); - // Act - String result = mailJetApiClientWrapper.addNewUserOrGetUserIfExists(email); + replay(mockMailjetClient, mockResponse); + + // Act & Assert + assertThrows(MailjetException.class, + () -> mailJetApiClientWrapper.updateUserProperties(mailjetId, "John", "STUDENT", "VERIFIED", "GCSE")); + + verify(mockMailjetClient, mockResponse); + } - // Assert - verify(mockMailjetClient, mockResponse); - assertEquals("456", result); + @Test + void updateUserProperties_WithCommunicationError_ShouldThrowCommunicationException() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetException connectionException = new MailjetException("Connection timeout"); + + expect(mockMailjetClient.put(anyObject(MailjetRequest.class))).andThrow(connectionException); + + replay(mockMailjetClient); + + // Act & Assert + assertThrows(MailjetClientCommunicationException.class, + () -> mailJetApiClientWrapper.updateUserProperties(mailjetId, "John", "STUDENT", "VERIFIED", "GCSE")); + + verify(mockMailjetClient); + } } - @Test - void addNewUserOrGetUserIfExists_WithNullEmail_ShouldReturnNull() throws MailjetException { - assertNull(mailJetApiClientWrapper.addNewUserOrGetUserIfExists(null)); + @Nested + class UpdateUserSubscriptionsTests { + + @Test + void updateUserSubscriptions_WithValidData_ShouldUpdate() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(201); + expect(mockResponse.getTotal()).andReturn(1); + + replay(mockMailjetClient, mockResponse); + + // Act + mailJetApiClientWrapper.updateUserSubscriptions(mailjetId, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, + MailJetSubscriptionAction.UNSUBSCRIBE); + + // Assert + verify(mockMailjetClient, mockResponse); + } + + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {" "}) + void updateUserSubscriptions_WithNullOrEmptyId_ShouldThrowException(String input) { + // Act & Assert + assertThrows(IllegalArgumentException.class, + () -> mailJetApiClientWrapper.updateUserSubscriptions(input, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, + MailJetSubscriptionAction.UNSUBSCRIBE)); + } + + @Test + void updateUserSubscriptions_WithNullNewsAction_ShouldThrowException() { + // Act & Assert + assertThrows(IllegalArgumentException.class, + () -> mailJetApiClientWrapper.updateUserSubscriptions("123", null, + MailJetSubscriptionAction.UNSUBSCRIBE)); + } + + @Test + void updateUserSubscriptions_WithNullEventsAction_ShouldThrowException() { + // Act & Assert + assertThrows(IllegalArgumentException.class, + () -> mailJetApiClientWrapper.updateUserSubscriptions("123", + MailJetSubscriptionAction.FORCE_SUBSCRIBE, null)); + } + + @Test + void updateUserSubscriptions_WithNotFound_ShouldThrowException() throws MailjetException { + // Arrange + String mailjetId = "999"; + MailjetException notFoundException = new MailjetException("Contact not found (404)"); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andThrow(notFoundException); + + replay(mockMailjetClient); + + // Act & Assert + assertThrows(MailjetException.class, + () -> mailJetApiClientWrapper.updateUserSubscriptions(mailjetId, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, + MailJetSubscriptionAction.UNSUBSCRIBE)); + + verify(mockMailjetClient); + } + + @Test + void updateUserSubscriptions_WithUnexpectedStatus_ShouldThrowException() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetResponse mockResponse = createMock(MailjetResponse.class); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andReturn(mockResponse); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getStatus()).andReturn(500); + expect(mockResponse.getTotal()).andReturn(500); + replay(mockMailjetClient, mockResponse); + + // Act & Assert + assertThrows(MailjetException.class, + () -> mailJetApiClientWrapper.updateUserSubscriptions(mailjetId, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, + MailJetSubscriptionAction.UNSUBSCRIBE)); + + verify(mockMailjetClient, mockResponse); + } + + @Test + void updateUserSubscriptions_WithCommunicationError_ShouldThrowCommunicationException() throws MailjetException { + // Arrange + String mailjetId = "123"; + MailjetException timeoutException = new MailjetException("Timeout occurred"); + + expect(mockMailjetClient.post(anyObject(MailjetRequest.class))).andThrow(timeoutException); + + replay(mockMailjetClient); + + // Act & Assert + assertThrows(MailjetClientCommunicationException.class, + () -> mailJetApiClientWrapper.updateUserSubscriptions(mailjetId, + MailJetSubscriptionAction.FORCE_SUBSCRIBE, + MailJetSubscriptionAction.UNSUBSCRIBE)); + + verify(mockMailjetClient); + } } private void injectMockMailjetClient(MailJetApiClientWrapper wrapper, MailjetClient client) {