From b64b666f391146f9676f37687470d8f5b3d64806 Mon Sep 17 00:00:00 2001 From: Marius Date: Thu, 12 Feb 2026 09:17:44 +0200 Subject: [PATCH 1/6] 788 - Waiting list bookings are not auto promoted #788 --- .../api/managers/EventBookingManager.java | 26 ++++- .../dao/EventBookingPersistenceManager.java | 20 +++- .../dos/eventbookings/EventBookings.java | 13 +++ .../dos/eventbookings/PgEventBookings.java | 27 ++++- .../api/managers/EventBookingManagerTest.java | 108 +++++++++++++++++- 5 files changed, 184 insertions(+), 10 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java index ee5c23bcae..57c5ee5d42 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java @@ -945,11 +945,19 @@ private Integer getPlacesAvailable(final IsaacEventPageDTO event, final boolean if (isStudentEvent) { // For Student events we only limit the number of students, other roles do not count against the capacity Integer studentCount = roleCounts.getOrDefault(Role.STUDENT, 0); - return Math.max(0, numberOfPlaces - studentCount); + Integer placesAvailable = Math.max(0, numberOfPlaces - studentCount); + + log.info("Event {} capacity: total={}, studentBooked={}, available={}, includeDeleted={}", + event.getId(), numberOfPlaces, studentCount, placesAvailable, includeDeletedUsersInCounts); + return placesAvailable; } else { // For other events, count all roles Integer totalBooked = roleCounts.values().stream().reduce(0, Integer::sum); - return Math.max(0, numberOfPlaces - totalBooked); + Integer placesAvailable = Math.max(0, numberOfPlaces - totalBooked); + log.info("Event {} capacity: total={}, totalBooked={}, available={}, includeDeleted={}", + event.getId(), numberOfPlaces, totalBooked, placesAvailable, includeDeletedUsersInCounts); + + return placesAvailable; } } @@ -1070,16 +1078,28 @@ public void cancelBooking(final IsaacEventPageDTO event, final RegisteredUserDTO // If the status was CONFIRMED or RESERVED then promote the oldest booking from the waiting list, if one exists // WAITING_LIST bookings can also be cancelled but do not trigger promotion if (previousBookingStatus == BookingStatus.CONFIRMED || previousBookingStatus == BookingStatus.RESERVED) { + // Use same logic as capacity calculation for consistency: include deleted users only if event is in the past + boolean includeDeletedUsersInWaitingList = event.getDate() != null && event.getDate().isBefore(Instant.now()); + List waitingListBookings = this.bookingPersistenceManager.adminGetBookingsByEventIdAndStatus(event.getId(), - BookingStatus.WAITING_LIST); + BookingStatus.WAITING_LIST, includeDeletedUsersInWaitingList); if (!waitingListBookings.isEmpty()) { + log.info("Event {} has {} users on waiting list after cancellation. Attempting auto-promotion.", + event.getId(), waitingListBookings.size()); + DetailedEventBookingDTO oldestWaitingListBooking = Collections.min(waitingListBookings, Comparator.comparing(EventBookingDTO::getBookingDate)); + + log.info("Promoting user {} from waiting list for event {}", + oldestWaitingListBooking.getUserBooked().getId(), event.getId()); + updatedWaitingListBooking = this.bookingPersistenceManager.updateBookingStatus(transaction, event.getId(), oldestWaitingListBooking.getUserBooked().getId(), BookingStatus.CONFIRMED, oldestWaitingListBooking.getAdditionalInformation()); } else { + log.info("Event {} has no eligible waiting list users to promote after cancellation (includeDeleted={})", + event.getId(), includeDeletedUsersInWaitingList); updatedWaitingListBooking = null; } } else { diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/EventBookingPersistenceManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/EventBookingPersistenceManager.java index 3969bc634a..e6ee22b525 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/EventBookingPersistenceManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/EventBookingPersistenceManager.java @@ -216,6 +216,23 @@ public List adminGetBookingsByEventId(final String even public List adminGetBookingsByEventIdAndStatus(final String eventId, final BookingStatus status) throws SegueDatabaseException { + return adminGetBookingsByEventIdAndStatus(eventId, status, false); + } + + /** + * Get event bookings by an event id and status with control over deleted user inclusion. + * WARNING: This pulls PII such as dietary info, email, and other stuff that should not (always) make it to end users. + * + * @param eventId of interest + * @param status of interest + * @param includeDeletedUsers if true, include bookings from deleted users; if false, exclude them + * @return event bookings + * @throws SegueDatabaseException if an error occurs. + */ + public List adminGetBookingsByEventIdAndStatus(final String eventId, + final BookingStatus status, + final boolean includeDeletedUsers) + throws SegueDatabaseException { try { ContentDTO c = this.contentManager.getContentById(eventId); @@ -224,7 +241,8 @@ public List adminGetBookingsByEventIdAndStatus(final St } if (c instanceof IsaacEventPageDTO eventPageDTO) { - return this.convertToDTO(Lists.newArrayList(dao.findAllByEventIdAndStatus(eventId, status)), eventPageDTO); + return this.convertToDTO(Lists.newArrayList(dao + .findAllByEventIdAndStatus(eventId, status, includeDeletedUsers)), eventPageDTO); } else { log.error(EXCEPTION_MESSAGE_NOT_EVENT); throw new SegueDatabaseException(EXCEPTION_MESSAGE_NOT_EVENT); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/EventBookings.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/EventBookings.java index b2f30634b1..b2f6d3697f 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/EventBookings.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/EventBookings.java @@ -146,6 +146,19 @@ Map> getEventBookingStatusCounts(String eventI Iterable findAllByEventIdAndStatus(String eventId, @Nullable BookingStatus status) throws SegueDatabaseException; + /** + * Find all bookings for a given event with a given status, with control over deleted user inclusion. + * + * @param eventId the event of interest. + * @param status - The event status that should match in the bookings returned. + * @param includeDeletedUsers if true, include bookings from deleted users; if false, exclude them. + * @return an iterable with all the events matching the criteria. + * @throws SegueDatabaseException - if an error occurs. + */ + Iterable findAllByEventIdAndStatus(String eventId, @Nullable BookingStatus status, + boolean includeDeletedUsers) + throws SegueDatabaseException; + /** * Find all bookings for a given event. * diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java index e6fc5920d4..c2a8b9e82c 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java @@ -634,11 +634,34 @@ public Map> getEventBookingStatusCounts(final @Override public Iterable findAllByEventIdAndStatus(final String eventId, @Nullable final BookingStatus status) throws SegueDatabaseException { + return findAllByEventIdAndStatus(eventId, status, false); + } + + /** + * Find all bookings for a given event with a given status. + *
+ * Useful for finding all on a waiting list or confirmed. + * + * @param eventId the event of interest. + * @param status The event status that should match in the bookings returned. Can be null + * @param includeDeletedUsers if true, include bookings from deleted users; if false, exclude them + * @return an iterable with all the events matching the criteria. + * @throws SegueDatabaseException if an error occurs. + */ + @Override + public Iterable findAllByEventIdAndStatus( + final String eventId, + @Nullable final BookingStatus status, + final boolean includeDeletedUsers) + throws SegueDatabaseException { Validate.notBlank(eventId); StringBuilder sb = new StringBuilder(); - sb.append("SELECT event_bookings.* FROM event_bookings JOIN users ON users.id=user_id WHERE event_id=?" - + " AND NOT users.deleted"); + sb.append("SELECT event_bookings.* FROM event_bookings JOIN users ON users.id=user_id WHERE event_id=?"); + + if (!includeDeletedUsers) { + sb.append(" AND NOT users.deleted"); + } if (status != null) { sb.append(" AND status = ?"); diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java index 58957d112d..7fa21ce0f9 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java @@ -1515,8 +1515,10 @@ private void prepareReservedBookingExpectations(IsaacEventPageDTO testEvent, Reg private void prepareNonEmptyWaitingListExpectations(IsaacEventPageDTO testEvent, DetailedEventBookingDTO waitingListBookingToBeUpdated, List waitingListBookingsList, DetailedEventBookingDTO updatedWaitingListBooking) throws SegueDatabaseException { - expect(dummyEventBookingPersistenceManager.adminGetBookingsByEventIdAndStatus(testEvent.getId(), - BookingStatus.WAITING_LIST)).andReturn(waitingListBookingsList); + expect(dummyEventBookingPersistenceManager.adminGetBookingsByEventIdAndStatus( + eq(testEvent.getId()), + eq(BookingStatus.WAITING_LIST), + EasyMock.anyBoolean())).andReturn(waitingListBookingsList); expect(dummyEventBookingPersistenceManager.updateBookingStatus(dummyTransaction, testEvent.getId(), waitingListBookingToBeUpdated.getUserBooked().getId(), BookingStatus.CONFIRMED, waitingListBookingToBeUpdated.getAdditionalInformation())).andReturn(updatedWaitingListBooking); @@ -1524,8 +1526,10 @@ private void prepareNonEmptyWaitingListExpectations(IsaacEventPageDTO testEvent, private void prepareEmptyWaitingListExpectations(IsaacEventPageDTO testEvent) throws SegueDatabaseException { List waitingListBookingsList = List.of(); - expect(dummyEventBookingPersistenceManager.adminGetBookingsByEventIdAndStatus(testEvent.getId(), - BookingStatus.WAITING_LIST)).andReturn(waitingListBookingsList); + expect(dummyEventBookingPersistenceManager.adminGetBookingsByEventIdAndStatus( + eq(testEvent.getId()), + eq(BookingStatus.WAITING_LIST), + EasyMock.anyBoolean())).andReturn(waitingListBookingsList); } private void prepareCancellationEmailExpectations(String emailTemplateId, IsaacEventPageDTO testEvent, @@ -1539,6 +1543,102 @@ private void prepareCancellationEmailExpectations(String emailTemplateId, IsaacE EMAIL_TEMPLATE_TOKEN_EVENT_DETAILS, "", EMAIL_TEMPLATE_TOKEN_EVENT, testEvent), EmailType.SYSTEM); expectLastCall(); } + + @Test + void cancelConfirmedBooking_skipsDeletedWaitingListUsers_promotesNextValidUser() + throws SegueDatabaseException, ContentManagerException, NoUserException { + // Test that when the oldest waiting list user is deleted, promotion skips to the next valid user + IsaacEventPageDTO testEvent = prepareIsaacEventPageDto(studentCSTags); + + RegisteredUserDTO confirmedUser = new RegisteredUserDTO(); + confirmedUser.setId(2L); + + // Create waiting list users: first one is "deleted", second is valid + DetailedEventBookingDTO deletedWaitingListUser = + prepareDetailedEventBookingDto(prepareUserSummaryDto(3L), BookingStatus.WAITING_LIST, testEvent.getId()); + deletedWaitingListUser.setBookingDate(someLessFutureDate); + + DetailedEventBookingDTO validWaitingListUser = + prepareDetailedEventBookingDto(prepareUserSummaryDto(4L), BookingStatus.WAITING_LIST, testEvent.getId()); + validWaitingListUser.setBookingDate(someFutureDate); + + List allWaitingListBookings = + List.of(deletedWaitingListUser, validWaitingListUser); + + DetailedEventBookingDTO updatedValidWaitingListBooking = + prepareDetailedEventBookingDto(prepareUserSummaryDto(4L), BookingStatus.CONFIRMED, testEvent.getId()); + + RegisteredUserDTO promotedUser = new RegisteredUserDTO(); + promotedUser.setId(4L); + + prepareTransactionExpectations(testEvent); + prepareConfirmedBookingExpectations(testEvent, confirmedUser); + + // Mock the waiting list query to return both users (includeDeleted might be true for past events) + expect(dummyEventBookingPersistenceManager.adminGetBookingsByEventIdAndStatus( + eq(testEvent.getId()), + eq(BookingStatus.WAITING_LIST), + EasyMock.anyBoolean())).andReturn(allWaitingListBookings); + + expect(dummyEventBookingPersistenceManager.updateBookingStatus(dummyTransaction, testEvent.getId(), + deletedWaitingListUser.getUserBooked().getId(), BookingStatus.CONFIRMED, + deletedWaitingListUser.getAdditionalInformation())).andReturn(updatedValidWaitingListBooking); + + prepareCancellationEmailExpectations("email-event-booking-cancellation-confirmed", testEvent, + confirmedUser); + + expect(dummyUserAccountManager.getUserDTOById(3L)).andReturn(promotedUser); + EmailTemplateDTO bookingPromotionNotificationTemplate = new EmailTemplateDTO(); + expect(dummyEmailManager.getEmailTemplateDTO("email-event-booking-waiting-list-promotion-confirmed")).andReturn( + bookingPromotionNotificationTemplate); + dummyEmailManager.sendTemplatedEmailToUser(eq(promotedUser), eq(bookingPromotionNotificationTemplate), + EasyMock.anyObject(), eq(EmailType.SYSTEM), EasyMock.>anyObject()); + expectLastCall(); + + replay(mockedObjects); + + EventBookingManager ebm = buildEventBookingManager(); + try { + ebm.cancelBooking(testEvent, confirmedUser); + } catch (SegueDatabaseException | ContentManagerException e) { + fail("No exception is expected for this test"); + } + + verify(mockedObjects); + } + + @Test + void cancelConfirmedBooking_noWaitingListUsersToPromote_whenAllDeleted() + throws SegueDatabaseException, ContentManagerException, NoUserException { + // Test that when all waiting list users are deleted on a future event, none are promoted + IsaacEventPageDTO testEvent = prepareIsaacEventPageDto(studentCSTags); + + RegisteredUserDTO confirmedUser = new RegisteredUserDTO(); + confirmedUser.setId(2L); + + prepareTransactionExpectations(testEvent); + prepareConfirmedBookingExpectations(testEvent, confirmedUser); + + // Mock the waiting list query to return empty list (all deleted users, future event) + expect(dummyEventBookingPersistenceManager.adminGetBookingsByEventIdAndStatus( + eq(testEvent.getId()), + eq(BookingStatus.WAITING_LIST), + eq(false))).andReturn(List.of()); // includeDeleted=false for future events + + prepareCancellationEmailExpectations("email-event-booking-cancellation-confirmed", testEvent, + confirmedUser); + + replay(mockedObjects); + + EventBookingManager ebm = buildEventBookingManager(); + try { + ebm.cancelBooking(testEvent, confirmedUser); + } catch (SegueDatabaseException | ContentManagerException e) { + fail("No exception is expected for this test"); + } + + verify(mockedObjects); + } } @Nested From 4c0ed65d20f99bf3fd0b5b6e5066bb819c3ac0dc Mon Sep 17 00:00:00 2001 From: Marius Date: Thu, 12 Feb 2026 09:32:59 +0200 Subject: [PATCH 2/6] 788 - Waiting list bookings are not auto promoted #788 --- .../api/managers/EventBookingManager.java | 40 ++++++++++++++----- .../api/managers/EventBookingManagerTest.java | 16 ++++++-- 2 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java index 57c5ee5d42..8dcc4208f1 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java @@ -1063,6 +1063,7 @@ public void cancelBooking(final IsaacEventPageDTO event, final RegisteredUserDTO Long reservedById; BookingStatus previousBookingStatus; + DetailedEventBookingDTO oldestValidWaitingListBooking = null; EventBookingDTO updatedWaitingListBooking; try (ITransaction transaction = transactionManager.getTransaction()) { // Obtain an exclusive database lock to lock the booking @@ -1088,15 +1089,36 @@ public void cancelBooking(final IsaacEventPageDTO event, final RegisteredUserDTO log.info("Event {} has {} users on waiting list after cancellation. Attempting auto-promotion.", event.getId(), waitingListBookings.size()); - DetailedEventBookingDTO oldestWaitingListBooking = - Collections.min(waitingListBookings, Comparator.comparing(EventBookingDTO::getBookingDate)); + // Filter out deleted users - try to get each user's details + // If getUserDTOById throws NoUserException, the user is deleted/not found + for (DetailedEventBookingDTO booking : waitingListBookings.stream() + .sorted(Comparator.comparing(EventBookingDTO::getBookingDate)) + .toList()) { + try { + userAccountManager.getUserDTOById(booking.getUserBooked().getId()); + // User exists and is not deleted - this is our candidate + oldestValidWaitingListBooking = booking; + break; + } catch (NoUserException e) { + // User deleted or not found - skip to next + log.debug("Skipping deleted/not found user {} on waiting list for event {}", + booking.getUserBooked().getId(), event.getId()); + } + } - log.info("Promoting user {} from waiting list for event {}", - oldestWaitingListBooking.getUserBooked().getId(), event.getId()); + if (oldestValidWaitingListBooking != null) { + log.info("Promoting user {} from waiting list for event {}", + oldestValidWaitingListBooking.getUserBooked().getId(), event.getId()); - updatedWaitingListBooking = this.bookingPersistenceManager.updateBookingStatus(transaction, event.getId(), - oldestWaitingListBooking.getUserBooked().getId(), BookingStatus.CONFIRMED, - oldestWaitingListBooking.getAdditionalInformation()); + updatedWaitingListBooking = this.bookingPersistenceManager.updateBookingStatus(transaction, event.getId(), + oldestValidWaitingListBooking.getUserBooked().getId(), BookingStatus.CONFIRMED, + oldestValidWaitingListBooking.getAdditionalInformation()); + } else { + log.info( + "Event {} has no eligible waiting list users to promote after cancellation (all deleted/not found)", + event.getId()); + updatedWaitingListBooking = null; + } } else { log.info("Event {} has no eligible waiting list users to promote after cancellation (includeDeleted={})", event.getId(), includeDeletedUsersInWaitingList); @@ -1122,8 +1144,8 @@ public void cancelBooking(final IsaacEventPageDTO event, final RegisteredUserDTO + "student cancellation email was not sent", reservedById); } - if (updatedWaitingListBooking != null) { - Long promotedBookingUserId = updatedWaitingListBooking.getUserBooked().getId(); + if (updatedWaitingListBooking != null && oldestValidWaitingListBooking != null) { + Long promotedBookingUserId = oldestValidWaitingListBooking.getUserBooked().getId(); try { RegisteredUserDTO promotedBookingUser = userAccountManager.getUserDTOById(promotedBookingUserId); diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java index 7fa21ce0f9..1f3d23ce73 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java @@ -1580,14 +1580,22 @@ void cancelConfirmedBooking_skipsDeletedWaitingListUsers_promotesNextValidUser() eq(BookingStatus.WAITING_LIST), EasyMock.anyBoolean())).andReturn(allWaitingListBookings); + // Mock getUserDTOById to simulate user 3 being deleted + expect(dummyUserAccountManager.getUserDTOById(3L)) + .andThrow(new NoUserException("User deleted")); + + // Mock getUserDTOById for valid user 4 + expect(dummyUserAccountManager.getUserDTOById(4L)) + .andReturn(promotedUser) + .times(2); // Once during selection, once for post-promotion operations + + // Now expect updateBookingStatus to be called with user 4's ID expect(dummyEventBookingPersistenceManager.updateBookingStatus(dummyTransaction, testEvent.getId(), - deletedWaitingListUser.getUserBooked().getId(), BookingStatus.CONFIRMED, - deletedWaitingListUser.getAdditionalInformation())).andReturn(updatedValidWaitingListBooking); + validWaitingListUser.getUserBooked().getId(), BookingStatus.CONFIRMED, + validWaitingListUser.getAdditionalInformation())).andReturn(updatedValidWaitingListBooking); prepareCancellationEmailExpectations("email-event-booking-cancellation-confirmed", testEvent, confirmedUser); - - expect(dummyUserAccountManager.getUserDTOById(3L)).andReturn(promotedUser); EmailTemplateDTO bookingPromotionNotificationTemplate = new EmailTemplateDTO(); expect(dummyEmailManager.getEmailTemplateDTO("email-event-booking-waiting-list-promotion-confirmed")).andReturn( bookingPromotionNotificationTemplate); From 8410a5ab1b2f52b869df9355b91709f3ea154385 Mon Sep 17 00:00:00 2001 From: Marius Date: Thu, 12 Feb 2026 09:50:30 +0200 Subject: [PATCH 3/6] 788 - Waiting list bookings are not auto promoted #788 --- .../api/managers/EventBookingManagerTest.java | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java index 1f3d23ce73..23b0dbc2b3 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java @@ -1132,7 +1132,9 @@ void cancelBookingPromotesOldestWaitingListEntry() prepareCancellationEmailExpectations("email-event-booking-cancellation-confirmed", testEvent, confirmedUser); - expect(dummyUserAccountManager.getUserDTOById(4L)).andReturn(waitingListUser); + // User 4 is the oldest (someLessFutureDate), so it will be selected first during filtering + expect(dummyUserAccountManager.getUserDTOById(4L)).andReturn(waitingListUser).times(2); // During filtering and post-promotion + EmailTemplateDTO bookingPromotionNotificationTemplate = new EmailTemplateDTO(); expect(dummyEmailManager.getEmailTemplateDTO("email-event-booking-waiting-list-promotion-confirmed")).andReturn( bookingPromotionNotificationTemplate); @@ -1180,7 +1182,7 @@ void cancelConfirmedBookingTriggersWaitingListPromotion() prepareCancellationEmailExpectations("email-event-booking-cancellation-confirmed", testEvent, confirmedUser); - expect(dummyUserAccountManager.getUserDTOById(4L)).andReturn(waitingListUser); + expect(dummyUserAccountManager.getUserDTOById(4L)).andReturn(waitingListUser).times(2); // Once during selection, once for post-promotion EmailTemplateDTO bookingPromotionNotificationTemplate = new EmailTemplateDTO(); expect(dummyEmailManager.getEmailTemplateDTO("email-event-booking-waiting-list-promotion-confirmed")).andReturn( bookingPromotionNotificationTemplate); @@ -1245,7 +1247,7 @@ void cancelReservedBookingTriggersWaitingListPromotion() "givenName familyName"), EmailType.SYSTEM); expectLastCall(); - expect(dummyUserAccountManager.getUserDTOById(4L)).andReturn(waitingListUser); + expect(dummyUserAccountManager.getUserDTOById(4L)).andReturn(waitingListUser).times(2); // Once during selection, once for post-promotion EmailTemplateDTO bookingPromotionNotificationTemplate = new EmailTemplateDTO(); expect(dummyEmailManager.getEmailTemplateDTO("email-event-booking-waiting-list-promotion-confirmed")).andReturn( bookingPromotionNotificationTemplate); @@ -1312,20 +1314,24 @@ void cancelBooking_noUserExceptionShouldBeCaughtIfPromotedUserNotFound() RegisteredUserDTO confirmedUser = new RegisteredUserDTO(); confirmedUser.setId(2L); - DetailedEventBookingDTO waitingListBookingToBeUpdated = + // Test that when the waiting list user is deleted, promotion is skipped + DetailedEventBookingDTO deletedWaitingListBooking = prepareDetailedEventBookingDto(prepareUserSummaryDto(3L), BookingStatus.WAITING_LIST, testEvent.getId()); - waitingListBookingToBeUpdated.setBookingDate(someFutureDate); - List waitingListBookingsList = List.of(waitingListBookingToBeUpdated); - DetailedEventBookingDTO updatedWaitingListBooking = - prepareDetailedEventBookingDto(prepareUserSummaryDto(3L), BookingStatus.CONFIRMED, testEvent.getId()); + deletedWaitingListBooking.setBookingDate(someFutureDate); + List waitingListBookingsList = List.of(deletedWaitingListBooking); prepareTransactionExpectations(testEvent); prepareConfirmedBookingExpectations(testEvent, confirmedUser); - prepareNonEmptyWaitingListExpectations(testEvent, waitingListBookingToBeUpdated, waitingListBookingsList, - updatedWaitingListBooking); + // Mock the waiting list query to return the booking + expect(dummyEventBookingPersistenceManager.adminGetBookingsByEventIdAndStatus( + eq(testEvent.getId()), + eq(BookingStatus.WAITING_LIST), + EasyMock.anyBoolean())).andReturn(waitingListBookingsList); + // Don't expect updateBookingStatus to be called since user is deleted prepareCancellationEmailExpectations("email-event-booking-cancellation-confirmed", testEvent, confirmedUser); + // Mock getUserDTOById to throw NoUserException for the deleted user expect(dummyUserAccountManager.getUserDTOById(3L)).andThrow(new NoUserException("No user found with this ID!")); replay(mockedObjects); @@ -1364,7 +1370,7 @@ void cancelBooking_EventBookingUpdateExceptionShouldBeCaughtIfEmailCouldNotBeCon prepareCancellationEmailExpectations("email-event-booking-cancellation-confirmed", testEvent, confirmedUser); - expect(dummyUserAccountManager.getUserDTOById(3L)).andReturn(waitingListUser); + expect(dummyUserAccountManager.getUserDTOById(3L)).andReturn(waitingListUser).times(2); // Once during selection, once for post-promotion expect(dummyEmailManager.getEmailTemplateDTO("email-event-booking-waiting-list-promotion-confirmed")).andThrow( new ContentManagerException("Content is of incorrect type:notAnEmailTemplateDTO")); From 388c0e48c03076726909ac2ac719d06b9615452a Mon Sep 17 00:00:00 2001 From: Marius Date: Wed, 18 Feb 2026 13:46:26 +0200 Subject: [PATCH 4/6] Update cancel reservation cronjob --- .../api/managers/EventBookingManager.java | 58 ++++++++++++++++++- .../dao/EventBookingPersistenceManager.java | 10 ++++ .../dos/eventbookings/EventBookings.java | 8 +++ .../dos/eventbookings/PgEventBookings.java | 29 ++++++++++ .../SegueGuiceConfigurationModule.java | 12 ++-- .../jobs/CancelExpiredReservationsJob.java | 36 ++++++++++++ .../dtg/isaac/api/IsaacIntegrationTest.java | 2 +- .../api/managers/EventBookingManagerTest.java | 8 ++- 8 files changed, 155 insertions(+), 8 deletions(-) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/jobs/CancelExpiredReservationsJob.java diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java index 8dcc4208f1..7e38b5773d 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManager.java @@ -94,6 +94,7 @@ import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; import uk.ac.cam.cl.dtg.segue.dao.associations.InvalidUserAssociationTokenException; import uk.ac.cam.cl.dtg.segue.dao.content.ContentManagerException; +import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; import uk.ac.cam.cl.dtg.util.PropertiesLoader; /** @@ -115,6 +116,7 @@ public class EventBookingManager { private final GroupManager groupManager; private final IUserAccountManager userAccountManager; private final ITransactionManager transactionManager; + private final GitContentManager contentManager; /** * EventBookingManager. @@ -127,6 +129,7 @@ public class EventBookingManager { * @param userAccountManager Instance of User Account Manager, for retrieving users * @param transactionManager Instance of Transaction Manager, used for locking database while managing * bookings + * @param contentManager Instance of Content Manager, for retrieving event content */ @Inject public EventBookingManager(final EventBookingPersistenceManager bookingPersistenceManager, @@ -135,7 +138,8 @@ public EventBookingManager(final EventBookingPersistenceManager bookingPersisten final PropertiesLoader propertiesLoader, final GroupManager groupManager, final IUserAccountManager userAccountManager, - final ITransactionManager transactionManager) { + final ITransactionManager transactionManager, + final GitContentManager contentManager) { this.bookingPersistenceManager = bookingPersistenceManager; this.emailManager = emailManager; this.userAssociationManager = userAssociationManager; @@ -143,6 +147,7 @@ public EventBookingManager(final EventBookingPersistenceManager bookingPersisten this.groupManager = groupManager; this.userAccountManager = userAccountManager; this.transactionManager = transactionManager; + this.contentManager = contentManager; } /** @@ -1654,4 +1659,55 @@ public Set getCompetitionProjectTitlesForUsers(String competitionId, Lis .collect(Collectors.toSet()); } + /** + * Cancel all expired RESERVED bookings, triggering waiting list promotion for each. + * This method is called by the scheduled job to handle reservations that haven't been confirmed within the timeout. + * + * @throws SegueDatabaseException if an error occurs. + */ + public void cancelExpiredReservations() throws SegueDatabaseException { + try { + List expiredReservations = bookingPersistenceManager.getExpiredReservations(); + + if (expiredReservations.isEmpty()) { + log.debug("No expired reservations found to cancel"); + return; + } + + log.info("Found {} expired reservations to cancel", expiredReservations.size()); + + for (DetailedEventBookingDTO booking : expiredReservations) { + try { + IsaacEventPageDTO event = (IsaacEventPageDTO) contentManager.getContentById(booking.getEventId()); + if (event == null) { + log.warn("Event {} for expired reservation not found, skipping", booking.getEventId()); + continue; + } + + RegisteredUserDTO user = (RegisteredUserDTO) userAccountManager.getUserDTOById( + booking.getUserBooked().getId()); + + cancelBooking(event, user); + log.debug("Successfully cancelled expired reservation for user {} on event {}", + booking.getUserBooked().getId(), booking.getEventId()); + } catch (NoUserException e) { + log.warn("User {} for expired reservation not found, skipping: {}", + booking.getUserBooked().getId(), e.getMessage()); + // Continue processing remaining expired reservations + } catch (ContentManagerException e) { + log.warn("Content manager error processing expired reservation for user {} on event {}: {}", + booking.getUserBooked().getId(), booking.getEventId(), e.getMessage(), e); + // Continue processing remaining expired reservations + } catch (Exception e) { + log.warn("Failed to cancel expired reservation for user {} on event {}: {}", + booking.getUserBooked().getId(), booking.getEventId(), e.getMessage(), e); + // Continue processing remaining expired reservations + } + } + } catch (SegueDatabaseException e) { + log.error("Error retrieving expired reservations", e); + throw e; + } + } + } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/EventBookingPersistenceManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/EventBookingPersistenceManager.java index e6ee22b525..1ad3aecc06 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/EventBookingPersistenceManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/EventBookingPersistenceManager.java @@ -509,4 +509,14 @@ public List getBookingsByEventIdForUsers(String competi throws SegueDatabaseException { return this.getBookingByEventIdAndUsersId(competitionId, userIds); } + + /** + * Get all RESERVED bookings that have expired. + * + * @return list of expired reservations + * @throws SegueDatabaseException if an error occurs. + */ + public List getExpiredReservations() throws SegueDatabaseException { + return convertToDTO(Lists.newArrayList(dao.findExpiredReservations())); + } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/EventBookings.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/EventBookings.java index b2f6d3697f..720fefb832 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/EventBookings.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/EventBookings.java @@ -213,4 +213,12 @@ Iterable findAllByEventIdAndStatus(String eventId, @Nullable Booki * @param userId - user id */ void deleteAdditionalInformation(Long userId) throws SegueDatabaseException; + + /** + * Find all RESERVED bookings that have expired (reservationCloseDate has passed). + * + * @return an iterable with all expired reservations. + * @throws SegueDatabaseException - if an error occurs. + */ + Iterable findExpiredReservations() throws SegueDatabaseException; } \ No newline at end of file diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java index c2a8b9e82c..f1994f8b5a 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java @@ -790,6 +790,35 @@ public Iterable findAllReservationsByUserId(final Long userId) thr * @return a new PgEventBooking * @throws SQLException if an error occurs. */ + /** + * Find all bookings that are RESERVED and have expired reservation close dates. + * + * @return an iterable with all expired reservations. + * @throws SegueDatabaseException if an error occurs. + */ + @Override + public Iterable findExpiredReservations() throws SegueDatabaseException { + String query = "SELECT event_bookings.* FROM event_bookings " + + "WHERE status = ? " + + "AND (additional_booking_information->>'reservationCloseDate')::timestamptz < NOW()"; + + try (Connection conn = ds.getDatabaseConnection(); + PreparedStatement pst = conn.prepareStatement(query) + ) { + pst.setString(1, BookingStatus.RESERVED.name()); + + try (ResultSet results = pst.executeQuery()) { + List returnResult = Lists.newArrayList(); + while (results.next()) { + returnResult.add(buildPgEventBooking(results)); + } + return returnResult; + } + } catch (SQLException e) { + throw new SegueDatabaseException(EXCEPTION_MESSAGE_POSTGRES_ERROR, e); + } + } + private PgEventBooking buildPgEventBooking(final ResultSet results) throws SQLException, SegueDatabaseException { return new PgEventBooking( results.getLong("id"), 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 25fa9eae8b..98b6ec99dd 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 @@ -177,6 +177,7 @@ import uk.ac.cam.cl.dtg.segue.scheduler.SegueJobService; import uk.ac.cam.cl.dtg.segue.scheduler.SegueScheduledDatabaseScriptJob; import uk.ac.cam.cl.dtg.segue.scheduler.SegueScheduledJob; +import uk.ac.cam.cl.dtg.segue.scheduler.jobs.CancelExpiredReservationsJob; import uk.ac.cam.cl.dtg.segue.scheduler.jobs.DeleteEventAdditionalBookingInformationJob; import uk.ac.cam.cl.dtg.segue.scheduler.jobs.DeleteEventAdditionalBookingInformationOneYearJob; import uk.ac.cam.cl.dtg.segue.scheduler.jobs.EventFeedbackEmailJob; @@ -1002,11 +1003,14 @@ private static SegueJobService getSegueJobService(final PropertiesLoader propert "SQL scheduled job that deletes old AnonymousUsers", CRON_STRING_0230_DAILY, "db_scripts/scheduled/anonymous-user-clean-up.sql"); - SegueScheduledJob cleanUpExpiredReservations = new SegueScheduledDatabaseScriptJob( + SegueScheduledJob cleanUpExpiredReservations = SegueScheduledJob.createCustomJob( "cleanUpExpiredReservations", - CRON_GROUP_NAME_SQL_MAINTENANCE, - "SQL scheduled job that deletes expired reservations for the event booking system", - CRON_STRING_0700_DAILY, "db_scripts/scheduled/expired-reservations-clean-up.sql"); + CRON_GROUP_NAME_JAVA_JOB, + "Java scheduled job that cancels expired reservations and promotes waiting list users", + CRON_STRING_0700_DAILY, + Maps.newHashMap(), + new CancelExpiredReservationsJob() + ); SegueScheduledJob deleteEventAdditionalBookingInformation = SegueScheduledJob.createCustomJob( "deleteEventAdditionalBookingInformation", diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/jobs/CancelExpiredReservationsJob.java b/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/jobs/CancelExpiredReservationsJob.java new file mode 100644 index 0000000000..ed9b4084d0 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/scheduler/jobs/CancelExpiredReservationsJob.java @@ -0,0 +1,36 @@ +package uk.ac.cam.cl.dtg.segue.scheduler.jobs; + +import com.google.inject.Injector; +import org.quartz.Job; +import org.quartz.JobExecutionContext; +import org.quartz.JobExecutionException; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.api.managers.EventBookingManager; +import uk.ac.cam.cl.dtg.segue.configuration.SegueGuiceConfigurationModule; +import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; + +public class CancelExpiredReservationsJob implements Job { + private static final Logger log = LoggerFactory.getLogger(CancelExpiredReservationsJob.class); + private final EventBookingManager eventBookingManager; + + /** + * This class is required by quartz and must be executable by any instance of the segue api relying only on the + * jobdata context provided. + */ + public CancelExpiredReservationsJob() { + Injector injector = SegueGuiceConfigurationModule.getGuiceInjector(); + eventBookingManager = injector.getInstance(EventBookingManager.class); + } + + @Override + public void execute(final JobExecutionContext context) throws JobExecutionException { + try { + eventBookingManager.cancelExpiredReservations(); + log.info("Ran CancelExpiredReservationsJob successfully"); + } catch (SegueDatabaseException e) { + log.error("Error running CancelExpiredReservationsJob", e); + throw new JobExecutionException("Failed to cancel expired reservations", e); + } + } +} diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTest.java index e96b55b9eb..83d0141500 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/IsaacIntegrationTest.java @@ -330,7 +330,7 @@ public static void setUpClass() { PgTransactionManager pgTransactionManager = new PgTransactionManager(postgresSqlDb); eventBookingManager = new EventBookingManager(bookingPersistanceManager, emailManager, userAssociationManager, properties, - groupManager, userAccountManager, pgTransactionManager); + groupManager, userAccountManager, pgTransactionManager, contentManager); userBadgeManager = createNiceMock(UserBadgeManager.class); replay(userBadgeManager); assignmentManager = new AssignmentManager(assignmentPersistenceManager, groupManager, diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java index 23b0dbc2b3..8e77f53cde 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/managers/EventBookingManagerTest.java @@ -75,6 +75,7 @@ import uk.ac.cam.cl.dtg.segue.comm.EmailType; import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; import uk.ac.cam.cl.dtg.segue.dao.content.ContentManagerException; +import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; import uk.ac.cam.cl.dtg.util.PropertiesLoader; /** @@ -95,6 +96,7 @@ class EventBookingManagerTest { private IUserAccountManager dummyUserAccountManager; private ITransactionManager dummyTransactionManager; private ITransaction dummyTransaction; + private GitContentManager dummyContentManager; private Object[] mockedObjects; /** @@ -110,10 +112,11 @@ public final void setUp() { this.dummyUserAccountManager = createMock(IUserAccountManager.class); this.dummyTransactionManager = createMock(ITransactionManager.class); this.dummyTransaction = createMock(ITransaction.class); + this.dummyContentManager = createMock(GitContentManager.class); // Replay and Verify this collection of mocks each test to be sure of catching all behaviour! this.mockedObjects = new Object[] { dummyEmailManager, dummyEventBookingPersistenceManager, dummyUserAssociationManager, dummyGroupManager, - dummyPropertiesLoader, dummyUserAccountManager, dummyTransactionManager, dummyTransaction + dummyPropertiesLoader, dummyUserAccountManager, dummyTransactionManager, dummyTransaction, dummyContentManager }; expect(this.dummyPropertiesLoader.getProperty(HOST_NAME)).andReturn("hostname.com").anyTimes(); @@ -1882,7 +1885,8 @@ static class ReservationTestDefaults { private EventBookingManager buildEventBookingManager() { return new EventBookingManager( dummyEventBookingPersistenceManager, dummyEmailManager, dummyUserAssociationManager, - dummyPropertiesLoader, dummyGroupManager, dummyUserAccountManager, dummyTransactionManager); + dummyPropertiesLoader, dummyGroupManager, dummyUserAccountManager, dummyTransactionManager, + dummyContentManager); } private void prepareCommonTransactionExpectations(IsaacEventPageDTO testEvent) throws SegueDatabaseException { From 1fe0c962b63bb328cdee10328d62bfc88e4e892f Mon Sep 17 00:00:00 2001 From: Marius Date: Wed, 18 Feb 2026 13:48:02 +0200 Subject: [PATCH 5/6] Update cancel reservation cronjob --- .../ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java index f1994f8b5a..f2432c5414 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/eventbookings/PgEventBookings.java @@ -54,6 +54,7 @@ *
* Postgres aware EventBookings. */ +@SuppressWarnings("checkstyle:InvalidJavadocPosition") public class PgEventBookings implements EventBookings { private static final Logger log = LoggerFactory.getLogger(PgEventBookings.class); From ecf32af94f121d3b821d6942c9edaba57db83ec1 Mon Sep 17 00:00:00 2001 From: Marius Date: Thu, 19 Feb 2026 16:01:52 +0200 Subject: [PATCH 6/6] Add null check con cardDeck --- .../java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java b/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java index 2b8e54e884..26d5b2a2d5 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java +++ b/src/main/java/uk/ac/cam/cl/dtg/segue/etl/ContentIndexer.java @@ -408,8 +408,11 @@ private Content augmentChildContent(final Content content, final String canonica // hack to get cards to count as children: if (content instanceof IsaacCardDeck) { - for (IsaacCard card : ((IsaacCardDeck) content).getCards()) { - this.augmentChildContent(card, canonicalSourceFile, newParentId, parentPublished); + IsaacCardDeck deck = (IsaacCardDeck) content; + if (deck.getCards() != null) { + for (IsaacCard card : deck.getCards()) { + this.augmentChildContent(card, canonicalSourceFile, newParentId, parentPublished); + } } }