From 96f64fcac2a21b404782bc1f1e3c39a836272c33 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Thu, 9 Apr 2026 12:22:47 +0100 Subject: [PATCH 01/36] Add bookmark endpoints --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 117 +++++++++++++++ .../IsaacApplicationRegister.java | 2 + .../ac/cam/cl/dtg/isaac/dos/IBookmarks.java | 21 +++ .../ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 138 ++++++++++++++++++ .../isaac/dto/content/ContentSummaryDTO.java | 21 +++ .../SegueGuiceConfigurationModule.java | 4 + .../cl/dtg/util/mappers/ContentMapper.java | 7 + 7 files changed, 310 insertions(+) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java new file mode 100644 index 0000000000..6bd0da5aa8 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -0,0 +1,117 @@ +package uk.ac.cam.cl.dtg.isaac.api; + +import com.google.inject.Inject; +import io.swagger.v3.oas.annotations.Operation; +import io.swagger.v3.oas.annotations.tags.Tag; +import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; +import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; +import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; +import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; +import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.ws.rs.DELETE; +import jakarta.ws.rs.GET; +import jakarta.ws.rs.POST; +import jakarta.ws.rs.Path; +import jakarta.ws.rs.Produces; +import jakarta.ws.rs.QueryParam; +import jakarta.ws.rs.core.Context; +import jakarta.ws.rs.core.MediaType; +import jakarta.ws.rs.core.Response; + +/** + * Bookmarks Facade + * + * Responsible for handling requests related to user bookmarks. + */ +@Path("/") +@Tag(name = "BookmarksFacade", description = "/bookmarks") +public class BookmarksFacade { + private final UserAccountManager userManager; + private final IBookmarks bookmarksManager; + + @Inject + public BookmarksFacade(final UserAccountManager userManager, final IBookmarks bookmarksManager) { + this.userManager = userManager; + this.bookmarksManager = bookmarksManager; + } + + /** + * Gets a list of content bookmarked by the current user. + * + * @param request + * - so we can find the current user. + * @param contentType + * - optional query parameter to filter bookmarks by content type. Valid values are "isaacQuestionPage" + * and "isaacConceptPage". If null, all bookmarks will be returned. + * @return the list of content that the user has bookmarked. + */ + @GET + @Path("bookmarks") + @Produces(MediaType.APPLICATION_JSON) + @Operation(summary = "Get bookmarks for the current user.") + public final Response getCurrentBookmarks(@Context final HttpServletRequest request, + @QueryParam("content_type") final String contentType) { + RegisteredUserDTO user; + try { + user = userManager.getCurrentRegisteredUser(request); + } catch (final NoUserLoggedInException e) { + return SegueErrorResponse.getNotLoggedInResponse(); + } + return Response.ok(bookmarksManager.getBookmarksForUser(user, contentType)).build(); + } + + /** + * Adds a bookmark for the current user. + * + * @param request + * - so we can find the current user. + * @param contentId + * - the id of the content to bookmark. + */ + @POST + @Path("bookmarks") + @Produces(MediaType.APPLICATION_JSON) + @Operation(summary = "Add a bookmark for the current user.") + public final Response addCurrentUserBookmark(@Context final HttpServletRequest request, + @QueryParam("content_id") final String contentId) { + RegisteredUserDTO user; + try { + user = userManager.getCurrentRegisteredUser(request); + } catch (final NoUserLoggedInException e) { + return SegueErrorResponse.getNotLoggedInResponse(); + } + if (bookmarksManager.getBookmarksForUser(user).size() >= 100) { + return new SegueErrorResponse(Response.Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.") + .toResponse(); + } else { + bookmarksManager.addBookmarkForUser(user, contentId); + } + return Response.noContent().build(); + } + + /** + * Removes a bookmark for the current user. + * + * @param request + * - so we can find the current user. + * @param contentId + * - the id of the content to be removed from the user's bookmarks. + */ + @DELETE + @Path("bookmarks") + @Produces(MediaType.APPLICATION_JSON) + @Operation(summary = "Remove a bookmark for the current user.") + public final Response removeCurrentUserBookmark(@Context final HttpServletRequest request, + @QueryParam("content_id") final String contentId) { + RegisteredUserDTO user; + try { + user = userManager.getCurrentRegisteredUser(request); + } catch (final NoUserLoggedInException e) { + return SegueErrorResponse.getNotLoggedInResponse(); + } + bookmarksManager.removeBookmarkForUser(user, contentId); + return Response.noContent().build(); + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java index d8f0faef54..a540be9203 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/configuration/IsaacApplicationRegister.java @@ -28,6 +28,7 @@ import io.swagger.v3.oas.models.info.Info; import io.swagger.v3.oas.models.servers.Server; import uk.ac.cam.cl.dtg.isaac.api.AssignmentFacade; +import uk.ac.cam.cl.dtg.isaac.api.BookmarksFacade; import uk.ac.cam.cl.dtg.isaac.api.EventsFacade; import uk.ac.cam.cl.dtg.isaac.api.GameboardsFacade; import uk.ac.cam.cl.dtg.isaac.api.IsaacController; @@ -121,6 +122,7 @@ public final Set getSingletons() { this.singletons.add(injector.getInstance(NotificationFacade.class)); this.singletons.add(injector.getInstance(EmailFacade.class)); this.singletons.add(injector.getInstance(QuizFacade.class)); + this.singletons.add(injector.getInstance(BookmarksFacade.class)); // initialise filters this.singletons.add(injector.getInstance(PerformanceMonitor.class)); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java new file mode 100644 index 0000000000..5c349ea34e --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java @@ -0,0 +1,21 @@ +package uk.ac.cam.cl.dtg.isaac.dos; + +import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; +import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; + +import java.util.List; + +/** + * IBookmarks. + * + * Interface for managing user bookmarks. + */ +public interface IBookmarks { + List getBookmarksForUser(RegisteredUserDTO user); + + List getBookmarksForUser(RegisteredUserDTO user, String contentType); + + void addBookmarkForUser(RegisteredUserDTO user, String contentId); + + void removeBookmarkForUser(RegisteredUserDTO user, String contentId); +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java new file mode 100644 index 0000000000..acb5cc44c9 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -0,0 +1,138 @@ +package uk.ac.cam.cl.dtg.isaac.dos; + +import com.google.inject.Inject; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; +import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; +import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; +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.segue.database.PostgresSqlDb; +import uk.ac.cam.cl.dtg.util.mappers.MainMapper; + +import java.sql.Connection; +import java.sql.Date; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.List; + +public class PgBookmarks implements IBookmarks { + private static final Logger log = LoggerFactory.getLogger(PgBookmarks.class); + + private final PostgresSqlDb database; + private final GitContentManager contentManager; + private final MainMapper mapper = MainMapper.INSTANCE; + + /** + * PgBookmarks. + * + * @param database client for postgres. + */ + @Inject + public PgBookmarks(final PostgresSqlDb database, final GitContentManager contentManager) { + this.database = database; + this.contentManager = contentManager; + } + + @Override + public List getBookmarksForUser(final RegisteredUserDTO user) { + return this.getBookmarksForUser(user, null); + } + + @Override + public List getBookmarksForUser(final RegisteredUserDTO user, final String contentType) { + + String query = "SELECT content_id FROM user_bookmarks WHERE user_id = ?"; + + boolean filterByContentType = false; + if (null != contentType) { + if (contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage")) { + query += " AND content_type = ?"; + filterByContentType = true; + } else { + log.warn("Invalid content type provided for bookmarks query: {}", contentType); + throw new IllegalArgumentException("Invalid content type for bookmarks query: " + contentType); + } + } + + List bookmarks = new ArrayList<>(); + + try (Connection conn = database.getDatabaseConnection(); + PreparedStatement pst = conn.prepareStatement(query); + ) { + pst.setLong(1, user.getId()); + + if (filterByContentType) { + pst.setString(2, contentType); + } + + try (ResultSet results = pst.executeQuery()) { + while (results.next()) { + String contentId = results.getString("content_id"); + ContentDTO content = this.contentManager.getContentById(contentId); + ContentSummaryDTO contentSummary = this.mapper.mapContentDTOtoContentSummaryDTO(content); + bookmarks.add(contentSummary); + } + } catch (final ContentManagerException e) { + throw new RuntimeException(e); + } + } catch (final SQLException e) { + e.printStackTrace(); + } + return bookmarks; + } + + @Override + public void addBookmarkForUser(final RegisteredUserDTO user, final String contentId) { + Date timestamp = new Date(System.currentTimeMillis()); + + String contentType = ""; + try { + ContentDTO content = this.contentManager.getContentById(contentId); + contentType = content.getType(); + } catch (final ContentManagerException e) { + throw new RuntimeException(e); + } + + if ((null == contentType) || !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { + log.warn("Failed to bookmark content with invalid content type: {}", contentType); + throw new IllegalArgumentException("Invalid content type for bookmark: " + contentType); + } + + String query = "INSERT INTO user_bookmarks (user_id, content_id, content_type, timestamp) VALUES (?, ?, ?, ?)"; + try (Connection conn = database.getDatabaseConnection(); + PreparedStatement pst = conn.prepareStatement(query); + ) { + pst.setLong(1, user.getId()); + pst.setString(2, contentId); + pst.setString(3, contentType); + pst.setDate(4, timestamp); + if (pst.executeUpdate() == 0) { + throw new SegueDatabaseException("Unable to save bookmark."); + } + + } catch (final SQLException | SegueDatabaseException e) { + e.printStackTrace(); + } + } + + @Override + public void removeBookmarkForUser(final RegisteredUserDTO user, final String contentId) { + String query = "DELETE FROM user_bookmarks WHERE user_id = ? AND content_id = ?"; + try (Connection conn = database.getDatabaseConnection(); + PreparedStatement pst = conn.prepareStatement(query); + ) { + pst.setLong(1, user.getId()); + pst.setString(2, contentId); + if (pst.executeUpdate() == 0) { + throw new SegueDatabaseException("Unable to remove bookmark."); + } + } catch (final SQLException | SegueDatabaseException e) { + e.printStackTrace(); + } + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java index b1d7c957ce..7d453b506a 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java @@ -20,6 +20,7 @@ import uk.ac.cam.cl.dtg.isaac.api.Constants.CompletionState; import uk.ac.cam.cl.dtg.isaac.dos.AudienceContext; +import java.util.Date; import java.util.List; /** @@ -37,6 +38,7 @@ public class ContentSummaryDTO { private List tags; private String url; private CompletionState state; + private Date bookmarked; private List questionPartIds; private String supersededBy; private Boolean deprecated; @@ -222,6 +224,25 @@ public void setState(final CompletionState state) { this.state = state; } + /** + * Gets the timestamp this content was bookmarked by the user. + * + * @return the timestamp this content was bookmarked, or null if not bookmarked + */ + public Date getBookmarked() { + return bookmarked; + } + + /** + * Sets the timestamp this content was bookmarked by the user. + * + * @param bookmarked + * the current timestamp for bookmarking operations, or null for unbookmarking operations + */ + public void setBookmarked(final Date bookmarked) { + this.bookmarked = bookmarked; + } + /** * Gets a list of the question part IDs. * 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 3555e0ff0a..1747eadfd7 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 @@ -55,10 +55,12 @@ import uk.ac.cam.cl.dtg.isaac.dao.PgQuizQuestionAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dos.AbstractUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.dos.IUserAlerts; +import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.IUserStreaksManager; import uk.ac.cam.cl.dtg.isaac.dos.LocationHistory; import uk.ac.cam.cl.dtg.isaac.dos.PgLocationHistory; import uk.ac.cam.cl.dtg.isaac.dos.PgUserAlerts; +import uk.ac.cam.cl.dtg.isaac.dos.PgBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.PgUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.dos.PgUserStreakManager; import uk.ac.cam.cl.dtg.isaac.quiz.IQuestionAttemptManager; @@ -440,6 +442,8 @@ private void configureApplicationManagers() { bind(IUserStreaksManager.class).to(PgUserStreakManager.class); + bind(IBookmarks.class).to(PgBookmarks.class); + bind(IStatisticsManager.class).to(StatisticsManager.class); bind(ITransactionManager.class).to(PgTransactionManager.class); diff --git a/src/main/java/uk/ac/cam/cl/dtg/util/mappers/ContentMapper.java b/src/main/java/uk/ac/cam/cl/dtg/util/mappers/ContentMapper.java index f904d22317..304fe42d78 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/util/mappers/ContentMapper.java +++ b/src/main/java/uk/ac/cam/cl/dtg/util/mappers/ContentMapper.java @@ -257,6 +257,7 @@ default T map(ContentDTO source, Class targetClass) { @Mapping(target = "url", ignore = true) @Mapping(target = "state", ignore = true) + @Mapping(target = "bookmarked", ignore = true) @Mapping(target = "questionPartIds", ignore = true) @Mapping(target = "difficulty", ignore = true) DetailedQuizSummaryDTO mapToDetailedQuizSummaryDTO(IsaacQuizDTO source); @@ -265,6 +266,7 @@ default T map(ContentDTO source, Class targetClass) { @Mapping(target = "supersededBy", ignore = true) @Mapping(target = "summary", ignore = true) @Mapping(target = "state", ignore = true) + @Mapping(target = "bookmarked", ignore = true) @Mapping(target = "questionPartIds", ignore = true) @Mapping(target = "difficulty", ignore = true) @Mapping(target = "deprecated", ignore = true) @@ -273,6 +275,7 @@ default T map(ContentDTO source, Class targetClass) { @Mapping(target = "url", ignore = true) @Mapping(target = "state", ignore = true) + @Mapping(target = "bookmarked", ignore = true) @Mapping(target = "questionPartIds", ignore = true) @Mapping(target = "difficulty", ignore = true) ContentSummaryDTO mapSeguePageDTOtoContentSummaryDTO(SeguePageDTO source); @@ -312,6 +315,7 @@ default T map(ContentDTO source, Class targetClass) { @Mapping(target = "supersededBy", ignore = true) @Mapping(target = "summary", ignore = true) @Mapping(target = "state", ignore = true) + @Mapping(target = "bookmarked", ignore = true) @Mapping(target = "questionPartIds", ignore = true) @Mapping(target = "hiddenFromRoles", ignore = true) @Mapping(target = "difficulty", ignore = true) @@ -321,6 +325,7 @@ default T map(ContentDTO source, Class targetClass) { @Mapping(target = "url", ignore = true) @Mapping(target = "state", ignore = true) + @Mapping(target = "bookmarked", ignore = true) @Mapping(target = "questionPartIds", ignore = true) @Mapping(target = "difficulty", ignore = true) @Named("mapQuizDTOtoQuizSummaryDTO") @@ -330,6 +335,7 @@ default T map(ContentDTO source, Class targetClass) { @Mapping(target = "supersededBy", ignore = true) @Mapping(target = "summary", ignore = true) @Mapping(target = "state", ignore = true) + @Mapping(target = "bookmarked", ignore = true) @Mapping(target = "rubric", ignore = true) @Mapping(target = "questionPartIds", ignore = true) @Mapping(target = "hiddenFromRoles", ignore = true) @@ -354,6 +360,7 @@ default T map(ContentDTO source, Class targetClass) { @Mapping(target = "summary", ignore = true) @Mapping(target = "subtitle", ignore = true) @Mapping(target = "state", ignore = true) + @Mapping(target = "bookmarked", ignore = true) @Mapping(target = "questionPartIds", ignore = true) @Mapping(target = "level", ignore = true) @Mapping(target = "difficulty", ignore = true) From 5d303fdff58d92438f41d1f0b4d61d108db08b76 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Thu, 9 Apr 2026 15:00:45 +0100 Subject: [PATCH 02/36] Augment question results with bookmarked status --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 14 ++++---- .../ac/cam/cl/dtg/isaac/api/PagesFacade.java | 11 +++++- .../isaac/api/managers/BookmarksManager.java | 36 +++++++++++++++++++ .../ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 6 +++- .../api/AbstractIsaacIntegrationTest.java | 5 +++ .../cam/cl/dtg/isaac/api/PagesFacadeIT.java | 2 +- 6 files changed, 64 insertions(+), 10 deletions(-) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 6bd0da5aa8..aff7d8a2f4 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -29,12 +29,12 @@ @Tag(name = "BookmarksFacade", description = "/bookmarks") public class BookmarksFacade { private final UserAccountManager userManager; - private final IBookmarks bookmarksManager; + private final IBookmarks bookmarksDbManager; @Inject - public BookmarksFacade(final UserAccountManager userManager, final IBookmarks bookmarksManager) { + public BookmarksFacade(final UserAccountManager userManager, final IBookmarks bookmarksDbManager) { this.userManager = userManager; - this.bookmarksManager = bookmarksManager; + this.bookmarksDbManager = bookmarksDbManager; } /** @@ -59,7 +59,7 @@ public final Response getCurrentBookmarks(@Context final HttpServletRequest requ } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } - return Response.ok(bookmarksManager.getBookmarksForUser(user, contentType)).build(); + return Response.ok(bookmarksDbManager.getBookmarksForUser(user, contentType)).build(); } /** @@ -82,11 +82,11 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } - if (bookmarksManager.getBookmarksForUser(user).size() >= 100) { + if (bookmarksDbManager.getBookmarksForUser(user).size() >= 100) { return new SegueErrorResponse(Response.Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.") .toResponse(); } else { - bookmarksManager.addBookmarkForUser(user, contentId); + bookmarksDbManager.addBookmarkForUser(user, contentId); } return Response.noContent().build(); } @@ -111,7 +111,7 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } - bookmarksManager.removeBookmarkForUser(user, contentId); + bookmarksDbManager.removeBookmarkForUser(user, contentId); return Response.noContent().build(); } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java index 40adebd1fe..e1afd63571 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java @@ -23,6 +23,7 @@ import org.jboss.resteasy.annotations.GZIP; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.api.managers.BookmarksManager; import uk.ac.cam.cl.dtg.isaac.api.managers.GameManager; import uk.ac.cam.cl.dtg.isaac.api.managers.URIManager; import uk.ac.cam.cl.dtg.isaac.api.managers.UserAttemptManager; @@ -109,6 +110,7 @@ public class PagesFacade extends AbstractIsaacFacade { private final GitContentManager contentManager; private final UserAttemptManager userAttemptManager; private final GameManager gameManager; + private final BookmarksManager bookmarksManager; /** * Creates an instance of the pages controller which provides the REST endpoints for accessing page content. @@ -131,13 +133,15 @@ public class PagesFacade extends AbstractIsaacFacade { * - So we can look up attempt information. * @param gameManager * - For looking up gameboard information. + * @param bookmarksManager + * - For looking up bookmark information. */ @Inject public PagesFacade(final ContentService api, final AbstractConfigLoader propertiesLoader, final ILogManager logManager, final MainMapper mapper, final GitContentManager contentManager, final UserAccountManager userManager, final URIManager uriManager, final QuestionManager questionManager, final GameManager gameManager, - final UserAttemptManager userAttemptManager) { + final UserAttemptManager userAttemptManager, final BookmarksManager bookmarksManager) { super(propertiesLoader, logManager); this.api = api; this.mapper = mapper; @@ -147,6 +151,7 @@ public PagesFacade(final ContentService api, final AbstractConfigLoader properti this.questionManager = questionManager; this.gameManager = gameManager; this.userAttemptManager = userAttemptManager; + this.bookmarksManager = bookmarksManager; } /** @@ -509,6 +514,10 @@ public final Response getQuestionList(@Context final HttpServletRequest httpServ } } + if (user instanceof RegisteredUserDTO registeredUser) { + summarizedResults = this.bookmarksManager.augmentContentSummaryListWithBookmarkInformation(registeredUser, summarizedResults); + } + if (limit < 0 || combinedResults.size() + summarizedResults.size() <= limit) { combinedResults.addAll(summarizedResults); nextSearchStartIndex += unfilteredSummarizedResults.size(); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java new file mode 100644 index 0000000000..44c19fab4a --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -0,0 +1,36 @@ +package uk.ac.cam.cl.dtg.isaac.api.managers; + +import com.google.inject.Inject; +import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; +import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; +import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; + +import java.util.Date; +import java.util.List; + +public class BookmarksManager { + + public IBookmarks bookmarksDbManager; + + @Inject + public BookmarksManager(final IBookmarks bookmarksDbManager) { + this.bookmarksDbManager = bookmarksDbManager; + } + + public List augmentContentSummaryListWithBookmarkInformation(final RegisteredUserDTO user, + final List contentSummaries) { + + List bookmarks = this.bookmarksDbManager.getBookmarksForUser(user); + + for (ContentSummaryDTO contentSummary : contentSummaries) { + for (ContentSummaryDTO bookmark : bookmarks) { + if (contentSummary.getId().equals(bookmark.getId())) { + Date timestamp = bookmark.getBookmarked(); + contentSummary.setBookmarked(timestamp); + break; + } + } + } + return contentSummaries; + } +} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index acb5cc44c9..70b54357f6 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -46,7 +46,7 @@ public List getBookmarksForUser(final RegisteredUserDTO user) @Override public List getBookmarksForUser(final RegisteredUserDTO user, final String contentType) { - String query = "SELECT content_id FROM user_bookmarks WHERE user_id = ?"; + String query = "SELECT content_id, timestamp FROM user_bookmarks WHERE user_id = ?"; boolean filterByContentType = false; if (null != contentType) { @@ -75,6 +75,10 @@ public List getBookmarksForUser(final RegisteredUserDTO user, String contentId = results.getString("content_id"); ContentDTO content = this.contentManager.getContentById(contentId); ContentSummaryDTO contentSummary = this.mapper.mapContentDTOtoContentSummaryDTO(content); + + Date timestamp = results.getDate("timestamp"); + contentSummary.setBookmarked(timestamp); + bookmarks.add(contentSummary); } } catch (final ContentManagerException e) { diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java index 3d4167e0ee..bdb62e3aeb 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java @@ -16,6 +16,7 @@ import org.testcontainers.utility.DockerImageName; import org.testcontainers.utility.MountableFile; import uk.ac.cam.cl.dtg.isaac.api.managers.AssignmentManager; +import uk.ac.cam.cl.dtg.isaac.api.managers.BookmarksManager; import uk.ac.cam.cl.dtg.isaac.api.managers.EventBookingManager; import uk.ac.cam.cl.dtg.isaac.api.managers.EventsManager; import uk.ac.cam.cl.dtg.isaac.api.managers.FastTrackManger; @@ -40,6 +41,7 @@ import uk.ac.cam.cl.dtg.isaac.dao.PgQuizAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizQuestionAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dos.AbstractUserPreferenceManager; +import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.PgUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.quiz.PgQuestionAttempts; import uk.ac.cam.cl.dtg.segue.api.Constants; @@ -148,6 +150,7 @@ public class AbstractIsaacIntegrationTest { protected static PgPasswordDataManager passwordDataManager; protected static UserAttemptManager userAttemptManager; protected static FastTrackManger fastTrackManger; + protected static BookmarksManager bookmarksManager; // Manager dependencies protected static IQuizAssignmentPersistenceManager quizAssignmentPersistenceManager; @@ -158,6 +161,7 @@ public class AbstractIsaacIntegrationTest { protected static QuizQuestionManager quizQuestionManager; protected static PgUsers pgUsers; protected static ContentSubclassMapper contentMapper; + protected static IBookmarks bookmarksDbManager; // Services protected static AssignmentService assignmentService; @@ -320,6 +324,7 @@ public static void setUpClass() throws Exception { quizQuestionManager = new QuizQuestionManager(questionManager, mainMapper, quizQuestionAttemptPersistenceManager, quizManager, quizAttemptManager); userAttemptManager = new UserAttemptManager(questionManager); fastTrackManger = new FastTrackManger(properties, contentManager, gameManager); + bookmarksManager = new BookmarksManager(bookmarksDbManager); misuseMonitor = new InMemoryMisuseMonitor(); misuseMonitor.registerHandler(GroupManagerLookupMisuseHandler.class.getSimpleName(), new GroupManagerLookupMisuseHandler(emailManager, properties)); diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacadeIT.java index 9ccd2d1096..7d6875f30b 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacadeIT.java @@ -43,7 +43,7 @@ public class PagesFacadeIT extends IsaacIntegrationTest{ public void setUp() { this.pagesFacade = new PagesFacade(new ContentService(contentManager), properties, logManager, mainMapper, contentManager, userAccountManager, new URIManager(properties), questionManager, - gameManager, userAttemptManager); + gameManager, userAttemptManager, bookmarksManager); } @Test From dbc2e45f8a93953dce8e1fc3229291fcc32fa600 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 10 Apr 2026 09:52:42 +0100 Subject: [PATCH 03/36] Store more precise timestamp information for bookmarks --- src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index 70b54357f6..e9f3e4f2f9 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -13,10 +13,10 @@ import uk.ac.cam.cl.dtg.util.mappers.MainMapper; import java.sql.Connection; -import java.sql.Date; import java.sql.PreparedStatement; import java.sql.ResultSet; import java.sql.SQLException; +import java.sql.Timestamp; import java.util.ArrayList; import java.util.List; @@ -76,7 +76,7 @@ public List getBookmarksForUser(final RegisteredUserDTO user, ContentDTO content = this.contentManager.getContentById(contentId); ContentSummaryDTO contentSummary = this.mapper.mapContentDTOtoContentSummaryDTO(content); - Date timestamp = results.getDate("timestamp"); + Timestamp timestamp = results.getTimestamp("timestamp"); contentSummary.setBookmarked(timestamp); bookmarks.add(contentSummary); @@ -92,7 +92,7 @@ public List getBookmarksForUser(final RegisteredUserDTO user, @Override public void addBookmarkForUser(final RegisteredUserDTO user, final String contentId) { - Date timestamp = new Date(System.currentTimeMillis()); + Timestamp timestamp = new Timestamp(System.currentTimeMillis()); String contentType = ""; try { @@ -114,7 +114,7 @@ public void addBookmarkForUser(final RegisteredUserDTO user, final String conten pst.setLong(1, user.getId()); pst.setString(2, contentId); pst.setString(3, contentType); - pst.setDate(4, timestamp); + pst.setTimestamp(4, timestamp); if (pst.executeUpdate() == 0) { throw new SegueDatabaseException("Unable to save bookmark."); } From 3be8bbd6140690f3b89934a80bbda9c8cc8ff35c Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 10 Apr 2026 14:21:56 +0100 Subject: [PATCH 04/36] Add bookmarks table migration --- .../2026-04-create-bookmarks-table.sql | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 src/main/resources/db_scripts/migrations/2026-04-create-bookmarks-table.sql diff --git a/src/main/resources/db_scripts/migrations/2026-04-create-bookmarks-table.sql b/src/main/resources/db_scripts/migrations/2026-04-create-bookmarks-table.sql new file mode 100644 index 0000000000..48b81b7c92 --- /dev/null +++ b/src/main/resources/db_scripts/migrations/2026-04-create-bookmarks-table.sql @@ -0,0 +1,17 @@ +-- Table: public.user_bookmarks + +-- DROP TABLE public.user_bookmarks; + +CREATE TABLE public.user_bookmarks( + user_id integer NOT NULL, + content_id text NOT NULL, + content_type text NOT NULL, + created timestamp with time zone NOT NULL DEFAULT now(), + CONSTRAINT user_bookmarks_pk PRIMARY KEY (user_id, content_id), + CONSTRAINT user_bookmarks_user_id_fk FOREIGN KEY (user_id) + REFERENCES public.users(id) ON UPDATE CASCADE ON DELETE CASCADE +); + +ALTER TABLE public.user_bookmarks + owner to rutherford; + From 4ed4a4664d65bc5b845c8b70407a5cc4487e9e83 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 10 Apr 2026 14:27:09 +0100 Subject: [PATCH 05/36] Rename method and update javadoc --- src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 2 +- .../uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index aff7d8a2f4..2cc6285f31 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -51,7 +51,7 @@ public BookmarksFacade(final UserAccountManager userManager, final IBookmarks bo @Path("bookmarks") @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Get bookmarks for the current user.") - public final Response getCurrentBookmarks(@Context final HttpServletRequest request, + public final Response getCurrentUserBookmarks(@Context final HttpServletRequest request, @QueryParam("content_type") final String contentType) { RegisteredUserDTO user; try { diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java index 7d453b506a..1b8a9956ae 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dto/content/ContentSummaryDTO.java @@ -237,7 +237,7 @@ public Date getBookmarked() { * Sets the timestamp this content was bookmarked by the user. * * @param bookmarked - * the current timestamp for bookmarking operations, or null for unbookmarking operations + * the timestamp this content was bookmarked */ public void setBookmarked(final Date bookmarked) { this.bookmarked = bookmarked; From 01e6379841148b492b4114388737a1e575103418 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 10 Apr 2026 15:13:08 +0100 Subject: [PATCH 06/36] Move bookmark content augmentation out of DB layer --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 11 ++- .../ac/cam/cl/dtg/isaac/api/PagesFacade.java | 2 +- .../isaac/api/managers/BookmarksManager.java | 71 +++++++++++++++++-- .../ac/cam/cl/dtg/isaac/dos/BookmarkDO.java | 8 +++ .../ac/cam/cl/dtg/isaac/dos/IBookmarks.java | 9 +-- .../ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 38 ++++------ 6 files changed, 100 insertions(+), 39 deletions(-) create mode 100644 src/main/java/uk/ac/cam/cl/dtg/isaac/dos/BookmarkDO.java diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 2cc6285f31..2bf7ece188 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -3,6 +3,8 @@ import com.google.inject.Inject; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.tags.Tag; +import uk.ac.cam.cl.dtg.isaac.api.managers.BookmarksManager; +import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; @@ -19,6 +21,7 @@ import jakarta.ws.rs.core.Context; import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; +import java.util.List; /** * Bookmarks Facade @@ -29,11 +32,14 @@ @Tag(name = "BookmarksFacade", description = "/bookmarks") public class BookmarksFacade { private final UserAccountManager userManager; + private final BookmarksManager bookmarksManager; private final IBookmarks bookmarksDbManager; @Inject - public BookmarksFacade(final UserAccountManager userManager, final IBookmarks bookmarksDbManager) { + public BookmarksFacade(final UserAccountManager userManager, final BookmarksManager bookmarksManager, + final IBookmarks bookmarksDbManager) { this.userManager = userManager; + this.bookmarksManager = bookmarksManager; this.bookmarksDbManager = bookmarksDbManager; } @@ -59,7 +65,8 @@ public final Response getCurrentUserBookmarks(@Context final HttpServletRequest } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } - return Response.ok(bookmarksDbManager.getBookmarksForUser(user, contentType)).build(); + List bookmarks = bookmarksDbManager.getBookmarksForUser(user, contentType); + return Response.ok(bookmarksManager.mapBookmarkListToContentSummaryList(bookmarks)).build(); } /** diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java index e1afd63571..13850e677e 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java @@ -515,7 +515,7 @@ public final Response getQuestionList(@Context final HttpServletRequest httpServ } if (user instanceof RegisteredUserDTO registeredUser) { - summarizedResults = this.bookmarksManager.augmentContentSummaryListWithBookmarkInformation(registeredUser, summarizedResults); + summarizedResults = bookmarksManager.augmentContentSummaryListWithBookmarkInformation(registeredUser, summarizedResults); } if (limit < 0 || combinedResults.size() + summarizedResults.size() <= limit) { diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java index 44c19fab4a..db1dda39d2 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -1,36 +1,93 @@ package uk.ac.cam.cl.dtg.isaac.api.managers; import com.google.inject.Inject; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; +import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; +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.mappers.MainMapper; +import java.util.ArrayList; import java.util.Date; import java.util.List; +/** + * A class to augment content with bookmark information, and bookmarks with content information. + */ public class BookmarksManager { + private static final Logger log = LoggerFactory.getLogger(BookmarksManager.class); - public IBookmarks bookmarksDbManager; + private final IBookmarks bookmarksDbManager; + private final GitContentManager contentManager; + private final MainMapper mapper; + /** + * Fully injected constructor. + * + * @param bookmarksDbManager the bookmarks database manager for retrieving bookmark information. + * @param contentManager the content manager for retrieving content information. + * @param mapper the mapper for mapping content to content summaries. + */ @Inject - public BookmarksManager(final IBookmarks bookmarksDbManager) { + public BookmarksManager(final IBookmarks bookmarksDbManager, final GitContentManager contentManager, final MainMapper mapper) { this.bookmarksDbManager = bookmarksDbManager; + this.contentManager = contentManager; + this.mapper = mapper; } + /** + * Augment a list of content summaries with bookmark information for a given user. + * + * @param user the user to augment the content summary list for. + * @param contentSummaries the content summary list to augment. + * @return the augmented content summary list. + */ public List augmentContentSummaryListWithBookmarkInformation(final RegisteredUserDTO user, final List contentSummaries) { - List bookmarks = this.bookmarksDbManager.getBookmarksForUser(user); + List bookmarks = this.bookmarksDbManager.getBookmarksForUser(user); for (ContentSummaryDTO contentSummary : contentSummaries) { - for (ContentSummaryDTO bookmark : bookmarks) { - if (contentSummary.getId().equals(bookmark.getId())) { - Date timestamp = bookmark.getBookmarked(); - contentSummary.setBookmarked(timestamp); + for (BookmarkDO bookmark : bookmarks) { + if (contentSummary.getId().equals(bookmark.contentId())) { + Date created = bookmark.created(); + contentSummary.setBookmarked(created); break; } } } return contentSummaries; } + + /** + * Map a list of bookmarks to a list of content summaries. + * + * @param bookmarks the list of bookmarks to map. + * @return the list of content summaries corresponding to the bookmarks. + */ + public List mapBookmarkListToContentSummaryList(final List bookmarks) { + + List contentSummaries = new ArrayList<>(); + + for (BookmarkDO bookmark : bookmarks) { + try { + ContentDTO content = this.contentManager.getContentById(bookmark.contentId()); + ContentSummaryDTO contentSummary = this.mapper.mapContentDTOtoContentSummaryDTO(content); + + Date created = bookmark.created(); + contentSummary.setBookmarked(created); + + contentSummaries.add(contentSummary); + } catch (final ContentManagerException e) { + log.warn("Error retrieving content for bookmark with content id {}: {}", bookmark.contentId(), e.getMessage()); + throw new RuntimeException(e); + } + } + return contentSummaries; + } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/BookmarkDO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/BookmarkDO.java new file mode 100644 index 0000000000..629c03f9e8 --- /dev/null +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/BookmarkDO.java @@ -0,0 +1,8 @@ +package uk.ac.cam.cl.dtg.isaac.dos; + +import java.util.Date; + +/** + * DO representing a bookmarked piece of content. + */ +public record BookmarkDO(String contentId, Date created) {} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java index 5c349ea34e..711b402aa5 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java @@ -1,19 +1,16 @@ package uk.ac.cam.cl.dtg.isaac.dos; -import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; import java.util.List; /** - * IBookmarks. - * - * Interface for managing user bookmarks. + * Interface for managing and persisting user bookmarks. */ public interface IBookmarks { - List getBookmarksForUser(RegisteredUserDTO user); + List getBookmarksForUser(RegisteredUserDTO user); - List getBookmarksForUser(RegisteredUserDTO user, String contentType); + List getBookmarksForUser(RegisteredUserDTO user, String contentType); void addBookmarkForUser(RegisteredUserDTO user, String contentId); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index e9f3e4f2f9..a01eb32d44 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -4,13 +4,11 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; -import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; 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.segue.database.PostgresSqlDb; -import uk.ac.cam.cl.dtg.util.mappers.MainMapper; import java.sql.Connection; import java.sql.PreparedStatement; @@ -20,12 +18,14 @@ import java.util.ArrayList; import java.util.List; +/** + * Postgres implementation for managing and persisting bookmarks data. + */ public class PgBookmarks implements IBookmarks { private static final Logger log = LoggerFactory.getLogger(PgBookmarks.class); private final PostgresSqlDb database; private final GitContentManager contentManager; - private final MainMapper mapper = MainMapper.INSTANCE; /** * PgBookmarks. @@ -39,14 +39,14 @@ public PgBookmarks(final PostgresSqlDb database, final GitContentManager content } @Override - public List getBookmarksForUser(final RegisteredUserDTO user) { + public List getBookmarksForUser(final RegisteredUserDTO user) { return this.getBookmarksForUser(user, null); } @Override - public List getBookmarksForUser(final RegisteredUserDTO user, final String contentType) { + public List getBookmarksForUser(final RegisteredUserDTO user, final String contentType) { - String query = "SELECT content_id, timestamp FROM user_bookmarks WHERE user_id = ?"; + String query = "SELECT content_id, created FROM user_bookmarks WHERE user_id = ?"; boolean filterByContentType = false; if (null != contentType) { @@ -59,7 +59,7 @@ public List getBookmarksForUser(final RegisteredUserDTO user, } } - List bookmarks = new ArrayList<>(); + List bookmarks = new ArrayList<>(); try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query); @@ -70,19 +70,11 @@ public List getBookmarksForUser(final RegisteredUserDTO user, pst.setString(2, contentType); } - try (ResultSet results = pst.executeQuery()) { - while (results.next()) { - String contentId = results.getString("content_id"); - ContentDTO content = this.contentManager.getContentById(contentId); - ContentSummaryDTO contentSummary = this.mapper.mapContentDTOtoContentSummaryDTO(content); - - Timestamp timestamp = results.getTimestamp("timestamp"); - contentSummary.setBookmarked(timestamp); - - bookmarks.add(contentSummary); - } - } catch (final ContentManagerException e) { - throw new RuntimeException(e); + ResultSet results = pst.executeQuery(); + while (results.next()) { + String contentId = results.getString("content_id"); + Timestamp created = results.getTimestamp("created"); + bookmarks.add(new BookmarkDO(contentId, created)); } } catch (final SQLException e) { e.printStackTrace(); @@ -92,7 +84,7 @@ public List getBookmarksForUser(final RegisteredUserDTO user, @Override public void addBookmarkForUser(final RegisteredUserDTO user, final String contentId) { - Timestamp timestamp = new Timestamp(System.currentTimeMillis()); + Timestamp created = new Timestamp(System.currentTimeMillis()); String contentType = ""; try { @@ -107,14 +99,14 @@ public void addBookmarkForUser(final RegisteredUserDTO user, final String conten throw new IllegalArgumentException("Invalid content type for bookmark: " + contentType); } - String query = "INSERT INTO user_bookmarks (user_id, content_id, content_type, timestamp) VALUES (?, ?, ?, ?)"; + String query = "INSERT INTO user_bookmarks (user_id, content_id, content_type, created) VALUES (?, ?, ?, ?)"; try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query); ) { pst.setLong(1, user.getId()); pst.setString(2, contentId); pst.setString(3, contentType); - pst.setTimestamp(4, timestamp); + pst.setTimestamp(4, created); if (pst.executeUpdate() == 0) { throw new SegueDatabaseException("Unable to save bookmark."); } From c87fb8ba0a9cf612afc3f5535cff920d8ee29c44 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 10 Apr 2026 15:26:40 +0100 Subject: [PATCH 07/36] Move bookmark content type check out of DB layer --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 3 ++- .../isaac/api/managers/BookmarksManager.java | 15 +++++++++++++ .../ac/cam/cl/dtg/isaac/dos/IBookmarks.java | 2 +- .../ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 22 ++----------------- 4 files changed, 20 insertions(+), 22 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 2bf7ece188..4ad308d00c 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -93,7 +93,8 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r return new SegueErrorResponse(Response.Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.") .toResponse(); } else { - bookmarksDbManager.addBookmarkForUser(user, contentId); + String contentType = bookmarksManager.getBookmarkContentType(contentId); + bookmarksDbManager.addBookmarkForUser(user, contentId, contentType); } return Response.noContent().build(); } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java index db1dda39d2..1d338f6408 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -90,4 +90,19 @@ public List mapBookmarkListToContentSummaryList(final List getBookmarksForUser(RegisteredUserDTO user, String contentType); - void addBookmarkForUser(RegisteredUserDTO user, String contentId); + void addBookmarkForUser(RegisteredUserDTO user, String contentId, String contentType); void removeBookmarkForUser(RegisteredUserDTO user, String contentId); } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index a01eb32d44..6b6645c83b 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -3,11 +3,8 @@ import com.google.inject.Inject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; 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.segue.database.PostgresSqlDb; import java.sql.Connection; @@ -25,7 +22,6 @@ public class PgBookmarks implements IBookmarks { private static final Logger log = LoggerFactory.getLogger(PgBookmarks.class); private final PostgresSqlDb database; - private final GitContentManager contentManager; /** * PgBookmarks. @@ -33,9 +29,8 @@ public class PgBookmarks implements IBookmarks { * @param database client for postgres. */ @Inject - public PgBookmarks(final PostgresSqlDb database, final GitContentManager contentManager) { + public PgBookmarks(final PostgresSqlDb database) { this.database = database; - this.contentManager = contentManager; } @Override @@ -83,22 +78,9 @@ public List getBookmarksForUser(final RegisteredUserDTO user, final } @Override - public void addBookmarkForUser(final RegisteredUserDTO user, final String contentId) { + public void addBookmarkForUser(final RegisteredUserDTO user, final String contentId, final String contentType) { Timestamp created = new Timestamp(System.currentTimeMillis()); - String contentType = ""; - try { - ContentDTO content = this.contentManager.getContentById(contentId); - contentType = content.getType(); - } catch (final ContentManagerException e) { - throw new RuntimeException(e); - } - - if ((null == contentType) || !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { - log.warn("Failed to bookmark content with invalid content type: {}", contentType); - throw new IllegalArgumentException("Invalid content type for bookmark: " + contentType); - } - String query = "INSERT INTO user_bookmarks (user_id, content_id, content_type, created) VALUES (?, ?, ?, ?)"; try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query); From fe4dc2055910602ce55a994e515ada20edaa9cdc Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 10 Apr 2026 15:32:22 +0100 Subject: [PATCH 08/36] Avoid passing entire user object to bookmarks DB manager --- .../ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 8 ++++---- .../uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java | 2 +- .../dtg/isaac/api/managers/BookmarksManager.java | 6 +++--- .../uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java | 8 ++++---- .../uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 16 ++++++++-------- 5 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 4ad308d00c..9cddaf6af1 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -65,7 +65,7 @@ public final Response getCurrentUserBookmarks(@Context final HttpServletRequest } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } - List bookmarks = bookmarksDbManager.getBookmarksForUser(user, contentType); + List bookmarks = bookmarksDbManager.getBookmarksForUser(user.getId(), contentType); return Response.ok(bookmarksManager.mapBookmarkListToContentSummaryList(bookmarks)).build(); } @@ -89,12 +89,12 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } - if (bookmarksDbManager.getBookmarksForUser(user).size() >= 100) { + if (bookmarksDbManager.getBookmarksForUser(user.getId()).size() >= 100) { return new SegueErrorResponse(Response.Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.") .toResponse(); } else { String contentType = bookmarksManager.getBookmarkContentType(contentId); - bookmarksDbManager.addBookmarkForUser(user, contentId, contentType); + bookmarksDbManager.addBookmarkForUser(user.getId(), contentId, contentType); } return Response.noContent().build(); } @@ -119,7 +119,7 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } - bookmarksDbManager.removeBookmarkForUser(user, contentId); + bookmarksDbManager.removeBookmarkForUser(user.getId(), contentId); return Response.noContent().build(); } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java index 13850e677e..b96c597958 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/PagesFacade.java @@ -515,7 +515,7 @@ public final Response getQuestionList(@Context final HttpServletRequest httpServ } if (user instanceof RegisteredUserDTO registeredUser) { - summarizedResults = bookmarksManager.augmentContentSummaryListWithBookmarkInformation(registeredUser, summarizedResults); + summarizedResults = bookmarksManager.augmentContentSummaryListWithBookmarkInformation(registeredUser.getId(), summarizedResults); } if (limit < 0 || combinedResults.size() + summarizedResults.size() <= limit) { diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java index 1d338f6408..0fd0fb368b 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -43,14 +43,14 @@ public BookmarksManager(final IBookmarks bookmarksDbManager, final GitContentMan /** * Augment a list of content summaries with bookmark information for a given user. * - * @param user the user to augment the content summary list for. + * @param userId the id of the user to augment the content summary list for. * @param contentSummaries the content summary list to augment. * @return the augmented content summary list. */ - public List augmentContentSummaryListWithBookmarkInformation(final RegisteredUserDTO user, + public List augmentContentSummaryListWithBookmarkInformation(final Long userId, final List contentSummaries) { - List bookmarks = this.bookmarksDbManager.getBookmarksForUser(user); + List bookmarks = this.bookmarksDbManager.getBookmarksForUser(userId); for (ContentSummaryDTO contentSummary : contentSummaries) { for (BookmarkDO bookmark : bookmarks) { diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java index 5f7a614c27..de018671e8 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java @@ -8,11 +8,11 @@ * Interface for managing and persisting user bookmarks. */ public interface IBookmarks { - List getBookmarksForUser(RegisteredUserDTO user); + List getBookmarksForUser(Long userId); - List getBookmarksForUser(RegisteredUserDTO user, String contentType); + List getBookmarksForUser(Long userId, String contentType); - void addBookmarkForUser(RegisteredUserDTO user, String contentId, String contentType); + void addBookmarkForUser(Long userId, String contentId, String contentType); - void removeBookmarkForUser(RegisteredUserDTO user, String contentId); + void removeBookmarkForUser(Long userId, String contentId); } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index 6b6645c83b..23075ffcf8 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -34,12 +34,12 @@ public PgBookmarks(final PostgresSqlDb database) { } @Override - public List getBookmarksForUser(final RegisteredUserDTO user) { - return this.getBookmarksForUser(user, null); + public List getBookmarksForUser(final Long userId) { + return this.getBookmarksForUser(userId, null); } @Override - public List getBookmarksForUser(final RegisteredUserDTO user, final String contentType) { + public List getBookmarksForUser(final Long userId, final String contentType) { String query = "SELECT content_id, created FROM user_bookmarks WHERE user_id = ?"; @@ -59,7 +59,7 @@ public List getBookmarksForUser(final RegisteredUserDTO user, final try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query); ) { - pst.setLong(1, user.getId()); + pst.setLong(1, userId); if (filterByContentType) { pst.setString(2, contentType); @@ -78,14 +78,14 @@ public List getBookmarksForUser(final RegisteredUserDTO user, final } @Override - public void addBookmarkForUser(final RegisteredUserDTO user, final String contentId, final String contentType) { + public void addBookmarkForUser(final Long userId, final String contentId, final String contentType) { Timestamp created = new Timestamp(System.currentTimeMillis()); String query = "INSERT INTO user_bookmarks (user_id, content_id, content_type, created) VALUES (?, ?, ?, ?)"; try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query); ) { - pst.setLong(1, user.getId()); + pst.setLong(1, userId); pst.setString(2, contentId); pst.setString(3, contentType); pst.setTimestamp(4, created); @@ -99,12 +99,12 @@ public void addBookmarkForUser(final RegisteredUserDTO user, final String conten } @Override - public void removeBookmarkForUser(final RegisteredUserDTO user, final String contentId) { + public void removeBookmarkForUser(final Long userId, final String contentId) { String query = "DELETE FROM user_bookmarks WHERE user_id = ? AND content_id = ?"; try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query); ) { - pst.setLong(1, user.getId()); + pst.setLong(1, userId); pst.setString(2, contentId); if (pst.executeUpdate() == 0) { throw new SegueDatabaseException("Unable to remove bookmark."); From 2563d3c4688bcac63f804516d18a93ef1f8a7b29 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 10 Apr 2026 16:24:53 +0100 Subject: [PATCH 09/36] Move bookmark content type checking to facade An invalid type constitutes a bad request, the response should indicate as much. --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 35 +++++++++++++++---- .../isaac/api/managers/BookmarksManager.java | 16 --------- 2 files changed, 29 insertions(+), 22 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 9cddaf6af1..dbec337b49 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -3,13 +3,18 @@ import com.google.inject.Inject; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.tags.Tag; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import uk.ac.cam.cl.dtg.isaac.api.managers.BookmarksManager; import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; +import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; +import uk.ac.cam.cl.dtg.segue.dao.content.ContentManagerException; +import uk.ac.cam.cl.dtg.segue.dao.content.GitContentManager; import jakarta.servlet.http.HttpServletRequest; import jakarta.ws.rs.DELETE; @@ -21,6 +26,7 @@ import jakarta.ws.rs.core.Context; import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; +import jakarta.ws.rs.core.Response.Status; import java.util.List; /** @@ -31,14 +37,18 @@ @Path("/") @Tag(name = "BookmarksFacade", description = "/bookmarks") public class BookmarksFacade { + private static final Logger log = LoggerFactory.getLogger(BookmarksFacade.class); + private final UserAccountManager userManager; + private final GitContentManager contentManager; private final BookmarksManager bookmarksManager; private final IBookmarks bookmarksDbManager; @Inject - public BookmarksFacade(final UserAccountManager userManager, final BookmarksManager bookmarksManager, - final IBookmarks bookmarksDbManager) { + public BookmarksFacade(final UserAccountManager userManager, final GitContentManager contentManager, + final BookmarksManager bookmarksManager, final IBookmarks bookmarksDbManager) { this.userManager = userManager; + this.contentManager = contentManager; this.bookmarksManager = bookmarksManager; this.bookmarksDbManager = bookmarksDbManager; } @@ -90,11 +100,24 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r return SegueErrorResponse.getNotLoggedInResponse(); } if (bookmarksDbManager.getBookmarksForUser(user.getId()).size() >= 100) { - return new SegueErrorResponse(Response.Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.") - .toResponse(); + return new SegueErrorResponse(Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.").toResponse(); } else { - String contentType = bookmarksManager.getBookmarkContentType(contentId); - bookmarksDbManager.addBookmarkForUser(user.getId(), contentId, contentType); + try { + ContentDTO content = this.contentManager.getContentById(contentId); + String contentType = content.getType(); + if ((null == contentType) || !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { + SegueErrorResponse error = new SegueErrorResponse(Status.BAD_REQUEST, + "Invalid content type for bookmark: " + contentType); + log.error(error.getErrorMessage()); + return error.toResponse(); + } + bookmarksDbManager.addBookmarkForUser(user.getId(), contentId, contentType); + } catch (final ContentManagerException | NullPointerException e) { + SegueErrorResponse error = new SegueErrorResponse(Status.NOT_FOUND, + "Failed to create bookmark, could not find content: " + contentId, e); + log.error(error.getErrorMessage(), e); + return error.toResponse(); + } } return Response.noContent().build(); } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java index 0fd0fb368b..298102e342 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -7,7 +7,6 @@ import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; -import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; 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.mappers.MainMapper; @@ -90,19 +89,4 @@ public List mapBookmarkListToContentSummaryList(final List Date: Mon, 20 Apr 2026 14:58:16 +0100 Subject: [PATCH 10/36] Add pg test data for user_bookmarks --- .../test-postgres-rutherford-data-dump.sql | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/test/resources/test-postgres-rutherford-data-dump.sql b/src/test/resources/test-postgres-rutherford-data-dump.sql index 71b579c217..36e0d29b3a 100644 --- a/src/test/resources/test-postgres-rutherford-data-dump.sql +++ b/src/test/resources/test-postgres-rutherford-data-dump.sql @@ -2,8 +2,8 @@ -- PostgreSQL database dump -- --- Dumped from database version 16.8 (Debian 16.8-1.pgdg120+1) --- Dumped by pg_dump version 16.8 (Debian 16.8-1.pgdg120+1) +-- Dumped from database version 16.3 (Debian 16.3-1.pgdg120+1) +-- Dumped by pg_dump version 16.3 (Debian 16.3-1.pgdg120+1) SET statement_timeout = 0; SET lock_timeout = 0; @@ -259,6 +259,19 @@ UAZFNZ 18 7 \. +-- +-- Data for Name: user_bookmarks; Type: TABLE DATA; Schema: public; Owner: rutherford +-- + +COPY public.user_bookmarks (user_id, content_id, content_type, created) FROM stdin; +7 cooling_excalibur isaacQuestionPage 2026-04-20 14:35:06.72+00 +7 man_vs_horse isaacQuestionPage 2025-03-11 14:35:42.634+00 +7 cp_newtoniii isaacConceptPage 2024-07-16 14:38:24.678+00 +8 man_vs_horse isaacQuestionPage 2025-08-13 14:38:35.17+00 +9 cp_abs_temperature isaacConceptPage 2025-10-27 14:38:43.858+00 +\. + + -- -- Data for Name: user_credentials; Type: TABLE DATA; Schema: public; Owner: rutherford -- From 638ade5b1f00d9a7f3dabc53dfe4ac641ab033fe Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Mon, 20 Apr 2026 15:21:21 +0100 Subject: [PATCH 11/36] Add user_bookmarks to DB creation script --- .../postgres-rutherford-create-script.sql | 36 ++++++++++++++++--- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql index 2e64f9fd1b..db99928ea9 100644 --- a/src/main/resources/db_scripts/postgres-rutherford-create-script.sql +++ b/src/main/resources/db_scripts/postgres-rutherford-create-script.sql @@ -2,8 +2,8 @@ -- PostgreSQL database dump -- --- Dumped from database version 16.2 (Debian 16.2-1.pgdg120+2) --- Dumped by pg_dump version 16.2 (Debian 16.2-1.pgdg120+2) +-- Dumped from database version 18.3 +-- Dumped by pg_dump version 18.3 SET statement_timeout = 0; SET lock_timeout = 0; @@ -50,7 +50,6 @@ CREATE TABLE public.archived_users ( ALTER TABLE public.archived_users OWNER TO rutherford; - -- -- Name: assignments; Type: TABLE; Schema: public; Owner: rutherford -- @@ -501,7 +500,6 @@ CREATE TABLE public.scheduled_emails ( ALTER TABLE public.scheduled_emails OWNER TO rutherford; - -- -- Name: temporary_user_store; Type: TABLE; Schema: public; Owner: rutherford -- @@ -587,6 +585,20 @@ CREATE TABLE public.user_associations_tokens ( ALTER TABLE public.user_associations_tokens OWNER TO rutherford; +-- +-- Name: user_bookmarks; Type: TABLE; Schema: public; Owner: rutherford +-- + +CREATE TABLE public.user_bookmarks ( + user_id integer NOT NULL, + content_id text NOT NULL, + content_type text NOT NULL, + created timestamp with time zone DEFAULT now() NOT NULL +); + + +ALTER TABLE public.user_bookmarks OWNER TO rutherford; + -- -- Name: user_credentials; Type: TABLE; Schema: public; Owner: rutherford -- @@ -1045,6 +1057,14 @@ ALTER TABLE ONLY public.user_associations ADD CONSTRAINT user_associations_composite_pkey PRIMARY KEY (user_id_granting_permission, user_id_receiving_permission); +-- +-- Name: user_bookmarks user_bookmarks_pk; Type: CONSTRAINT; Schema: public; Owner: rutherford +-- + +ALTER TABLE ONLY public.user_bookmarks + ADD CONSTRAINT user_bookmarks_pk PRIMARY KEY (user_id, content_id); + + -- -- Name: user_gameboards user_gameboard_composite_key; Type: CONSTRAINT; Schema: public; Owner: rutherford -- @@ -1435,6 +1455,14 @@ ALTER TABLE ONLY public.user_associations_tokens ADD CONSTRAINT token_owner_user_id FOREIGN KEY (owner_user_id) REFERENCES public.users(id) ON DELETE CASCADE; +-- +-- Name: user_bookmarks user_bookmarks_user_id_fk; Type: FK CONSTRAINT; Schema: public; Owner: rutherford +-- + +ALTER TABLE ONLY public.user_bookmarks + ADD CONSTRAINT user_bookmarks_user_id_fk FOREIGN KEY (user_id) REFERENCES public.users(id) ON UPDATE CASCADE ON DELETE CASCADE; + + -- -- Name: user_associations user_granting_permission_fkey; Type: FK CONSTRAINT; Schema: public; Owner: rutherford -- From afd9134f8ebd0128dd15cf388ad61b2a59075e66 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Mon, 20 Apr 2026 15:55:07 +0100 Subject: [PATCH 12/36] Update bookmarks PG test data to use IDs from ES test data --- .../resources/test-postgres-rutherford-data-dump.sql | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/resources/test-postgres-rutherford-data-dump.sql b/src/test/resources/test-postgres-rutherford-data-dump.sql index 36e0d29b3a..a2e23f0f68 100644 --- a/src/test/resources/test-postgres-rutherford-data-dump.sql +++ b/src/test/resources/test-postgres-rutherford-data-dump.sql @@ -264,11 +264,11 @@ UAZFNZ 18 7 -- COPY public.user_bookmarks (user_id, content_id, content_type, created) FROM stdin; -7 cooling_excalibur isaacQuestionPage 2026-04-20 14:35:06.72+00 -7 man_vs_horse isaacQuestionPage 2025-03-11 14:35:42.634+00 -7 cp_newtoniii isaacConceptPage 2024-07-16 14:38:24.678+00 -8 man_vs_horse isaacQuestionPage 2025-08-13 14:38:35.17+00 -9 cp_abs_temperature isaacConceptPage 2025-10-27 14:38:43.858+00 +7 _regression_test_ isaacQuestionPage 2026-04-20 14:35:06.72+00 +7 _assignment_test isaacQuestionPage 2025-03-11 14:35:42.634+00 +7 33935571-5a6c-4d42-a243-b5c01d4293e6 isaacConceptPage 2024-07-16 14:38:24.678+00 +9 33935571-5a6c-4d42-a243-b5c01d4293e6 isaacConceptPage 2025-10-27 14:38:43.858+00 +8 supersedes isaacQuestionPage 2025-08-13 14:38:35.17+00 \. From 7930013f29f79613684ca1f4345bbe0d62268434 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Mon, 20 Apr 2026 16:44:48 +0100 Subject: [PATCH 13/36] Correct response code for successful requests --- src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index dbec337b49..586b67f349 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -119,7 +119,7 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r return error.toResponse(); } } - return Response.noContent().build(); + return Response.ok().build(); } /** @@ -143,6 +143,6 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques return SegueErrorResponse.getNotLoggedInResponse(); } bookmarksDbManager.removeBookmarkForUser(user.getId(), contentId); - return Response.noContent().build(); + return Response.ok().build(); } } From 495d61a89ceb14ec876d4c0b1609af2d7c37d990 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Mon, 20 Apr 2026 16:46:10 +0100 Subject: [PATCH 14/36] Add integration tests for bookmarks facade --- .../cl/dtg/isaac/api/BookmarksFacadeIT.java | 153 ++++++++++++++++++ 1 file changed, 153 insertions(+) create mode 100644 src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java new file mode 100644 index 0000000000..d6b5c8593b --- /dev/null +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -0,0 +1,153 @@ +package uk.ac.cam.cl.dtg.isaac.api; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; + +import jakarta.servlet.http.Cookie; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.ws.rs.core.Response; +import java.sql.PreparedStatement; +import java.util.ArrayList; +import java.util.List; + +import static org.easymock.EasyMock.replay; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static uk.ac.cam.cl.dtg.isaac.api.Constants.*; + +public class BookmarksFacadeIT extends IsaacIntegrationTest { + + private BookmarksFacade bookmarksFacade; + + @BeforeEach + public void setUp() { + this.bookmarksFacade = new BookmarksFacade(userAccountManager, contentManager, bookmarksManager, bookmarksDbManager); + } + + @AfterEach + public void tearDown() throws Exception { + // Clean up bookmark added during tests + PreparedStatement deleteBookmarks = postgresSqlDb.getDatabaseConnection().prepareStatement("DELETE FROM user_bookmarks WHERE user_id = ? AND content_id = ?"); + deleteBookmarks.setLong(1, ITConstants.ALICE_STUDENT_ID); + deleteBookmarks.setString(2, ITConstants.FUZZY_MATCH_TEST_PAGE_ID); + deleteBookmarks.executeUpdate(); + } + + @Test + public void getCurrentUserBookmarks_searchWithoutContentType_returnsAllBookmarks() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest bookmarksRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(bookmarksRequest); + + // Act: make request + Response bookmarksResponse = bookmarksFacade.getCurrentUserBookmarks(bookmarksRequest, null); + + // Assert: check response is OK and contains expected bookmarks + assertEquals(Response.Status.OK.getStatusCode(), bookmarksResponse.getStatus()); + + ArrayList responseBody = (ArrayList) bookmarksResponse.getEntity(); + List actualBookmarkIds = responseBody.stream().map(ContentSummaryDTO::getId).toList(); + + List expectedBookmarkIds = new ArrayList<>(); + expectedBookmarkIds.add(ITConstants.REGRESSION_TEST_PAGE_ID); + expectedBookmarkIds.add(ITConstants.ASSIGNMENT_TEST_PAGE_ID); + expectedBookmarkIds.add(ITConstants.SEARCH_TEST_CONCEPT_ID); + + assertEquals(expectedBookmarkIds, actualBookmarkIds); + } + + @Test + public void getCurrentUserBookmarks_searchWithContentType_returnsFilteredBookmarks() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest bookmarksRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(bookmarksRequest); + + // Act: make request + Response bookmarksResponse = bookmarksFacade.getCurrentUserBookmarks(bookmarksRequest, QUESTION_TYPE); + + // Assert: check status code is OK and contains expected bookmarks + assertEquals(Response.Status.OK.getStatusCode(), bookmarksResponse.getStatus()); + + ArrayList responseBody = (ArrayList) bookmarksResponse.getEntity(); + List actualBookmarkIds = responseBody.stream().map(ContentSummaryDTO::getId).toList(); + + List expectedBookmarkIds = new ArrayList<>(); + expectedBookmarkIds.add(ITConstants.REGRESSION_TEST_PAGE_ID); + expectedBookmarkIds.add(ITConstants.ASSIGNMENT_TEST_PAGE_ID); + + assertEquals(expectedBookmarkIds, actualBookmarkIds); + } + + @Test + public void addCurrentUserBookmark_validContent_addsBookmark() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(addBookmarkRequest); + + // Act: make request + Response addBookmarkResponse = bookmarksFacade.addCurrentUserBookmark(addBookmarkRequest, ITConstants.FUZZY_MATCH_TEST_PAGE_ID); + + // Assert: check status code is OK + assertEquals(Response.Status.OK.getStatusCode(), addBookmarkResponse.getStatus()); + } + + @Test + public void addCurrentUserBookmark_notFoundContent_doesNotAddBookmark() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(addBookmarkRequest); + + // Act: make request + Response addBookmarkResponse = bookmarksFacade.addCurrentUserBookmark(addBookmarkRequest, "not_a_real_id"); + + // Assert: check status code is not found + assertEquals(Response.Status.NOT_FOUND.getStatusCode(), addBookmarkResponse.getStatus()); + } + + @Test + public void addCurrentUserBookmark_duplicateContent_doesNotAddBookmark() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(addBookmarkRequest); + + // Act: make request + Response addBookmarkResponse = bookmarksFacade.addCurrentUserBookmark(addBookmarkRequest, ITConstants.REGRESSION_TEST_PAGE_ID); + + // Assert: check status code is bad request + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), addBookmarkResponse.getStatus()); + } + + @Test + public void deleteCurrentUserBookmark_validContent_deletesBookmark() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(addBookmarkRequest); + + // Act: make request + Response deleteBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(addBookmarkRequest, ITConstants.SEARCH_TEST_CONCEPT_ID); + + // Assert: check status code is OK + assertEquals(Response.Status.OK.getStatusCode(), deleteBookmarkResponse.getStatus()); + } + + @Test + public void deleteCurrentUserBookmark_notFoundContent_doesNotDeleteBookmark() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(addBookmarkRequest); + + // Act: make request + Response addBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(addBookmarkRequest, "not_a_real_id"); + + // Assert: check status code is not found + assertEquals(Response.Status.NOT_FOUND.getStatusCode(), addBookmarkResponse.getStatus()); + } +} From 96a92c2463cb4dc18d2c44705db1547928f27da0 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Mon, 20 Apr 2026 17:12:59 +0100 Subject: [PATCH 15/36] Improve responses for invalid bookmark requests Return appropriate response if attempting to add a duplicate bookmark or delete a non-existent bookmark. --- .../uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 13 ++++++++++++- .../ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java | 12 ++++++------ 2 files changed, 18 insertions(+), 7 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 586b67f349..f5fadee7dd 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -99,8 +99,13 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } - if (bookmarksDbManager.getBookmarksForUser(user.getId()).size() >= 100) { + + List currentBookmarks = bookmarksDbManager.getBookmarksForUser(user.getId()); + + if (currentBookmarks.size() >= 100) { return new SegueErrorResponse(Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.").toResponse(); + } else if (currentBookmarks.stream().anyMatch(b -> b.contentId().equals(contentId))) { + return new SegueErrorResponse(Status.BAD_REQUEST, "You have already bookmarked this content.").toResponse(); } else { try { ContentDTO content = this.contentManager.getContentById(contentId); @@ -142,6 +147,12 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } + + List currentBookmarks = bookmarksDbManager.getBookmarksForUser(user.getId()); + if (currentBookmarks.stream().noneMatch(b -> b.contentId().equals(contentId))) { + return new SegueErrorResponse(Status.BAD_REQUEST, "You have not bookmarked this content.").toResponse(); + } + bookmarksDbManager.removeBookmarkForUser(user.getId(), contentId); return Response.ok().build(); } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java index d6b5c8593b..11779200a7 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -96,7 +96,7 @@ public void addCurrentUserBookmark_validContent_addsBookmark() throws Exception } @Test - public void addCurrentUserBookmark_notFoundContent_doesNotAddBookmark() throws Exception { + public void addCurrentUserBookmark_notFoundContent_returnsError() throws Exception { // Arrange: log in, create request LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); @@ -110,7 +110,7 @@ public void addCurrentUserBookmark_notFoundContent_doesNotAddBookmark() throws E } @Test - public void addCurrentUserBookmark_duplicateContent_doesNotAddBookmark() throws Exception { + public void addCurrentUserBookmark_duplicateContent_returnsError() throws Exception { // Arrange: log in, create request LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); @@ -138,16 +138,16 @@ public void deleteCurrentUserBookmark_validContent_deletesBookmark() throws Exce } @Test - public void deleteCurrentUserBookmark_notFoundContent_doesNotDeleteBookmark() throws Exception { + public void deleteCurrentUserBookmark_notBookmarkedContent_returnsError() throws Exception { // Arrange: log in, create request LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); replay(addBookmarkRequest); // Act: make request - Response addBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(addBookmarkRequest, "not_a_real_id"); + Response addBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(addBookmarkRequest, ITConstants.SEARCH_TEST_SUPERSEDED_BY_ID); - // Assert: check status code is not found - assertEquals(Response.Status.NOT_FOUND.getStatusCode(), addBookmarkResponse.getStatus()); + // Assert: check status code is bad request + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), addBookmarkResponse.getStatus()); } } From 76b8e5cf677af919214777b7de3a9346c86efa65 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Mon, 20 Apr 2026 17:13:49 +0100 Subject: [PATCH 16/36] Update AbstractIsaacIntegrationTest with bookmark objects --- .../ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java index bdb62e3aeb..c5fdad749f 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java @@ -42,6 +42,7 @@ import uk.ac.cam.cl.dtg.isaac.dao.PgQuizQuestionAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dos.AbstractUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; +import uk.ac.cam.cl.dtg.isaac.dos.PgBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.PgUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.quiz.PgQuestionAttempts; import uk.ac.cam.cl.dtg.segue.api.Constants; @@ -324,7 +325,8 @@ public static void setUpClass() throws Exception { quizQuestionManager = new QuizQuestionManager(questionManager, mainMapper, quizQuestionAttemptPersistenceManager, quizManager, quizAttemptManager); userAttemptManager = new UserAttemptManager(questionManager); fastTrackManger = new FastTrackManger(properties, contentManager, gameManager); - bookmarksManager = new BookmarksManager(bookmarksDbManager); + bookmarksDbManager = new PgBookmarks(postgresSqlDb); + bookmarksManager = new BookmarksManager(bookmarksDbManager, contentManager, mainMapper); misuseMonitor = new InMemoryMisuseMonitor(); misuseMonitor.registerHandler(GroupManagerLookupMisuseHandler.class.getSimpleName(), new GroupManagerLookupMisuseHandler(emailManager, properties)); From 2f6c3dd703e5b8d03a62edacaa0749c2a6dfffbf Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Tue, 21 Apr 2026 08:59:07 +0100 Subject: [PATCH 17/36] Extend bookmarks integration tests --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 7 ++ .../cl/dtg/isaac/api/BookmarksFacadeIT.java | 79 +++++++++++++++++-- 2 files changed, 80 insertions(+), 6 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index f5fadee7dd..2b3bfb880b 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -75,6 +75,13 @@ public final Response getCurrentUserBookmarks(@Context final HttpServletRequest } catch (final NoUserLoggedInException e) { return SegueErrorResponse.getNotLoggedInResponse(); } + + if (null != contentType && !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { + SegueErrorResponse error = new SegueErrorResponse(Status.BAD_REQUEST, "Invalid content type for bookmarks query: " + contentType); + log.warn(error.getErrorMessage()); + return error.toResponse(); + } + List bookmarks = bookmarksDbManager.getBookmarksForUser(user.getId(), contentType); return Response.ok(bookmarksManager.mapBookmarkListToContentSummaryList(bookmarks)).build(); } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java index 11779200a7..5fa5bc54a4 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -81,6 +81,33 @@ public void getCurrentUserBookmarks_searchWithContentType_returnsFilteredBookmar assertEquals(expectedBookmarkIds, actualBookmarkIds); } + @Test + public void getCurrentUserBookmarks_noLoggedInUser_returnsError() { + // Arrange: create request with no cookie + HttpServletRequest bookmarksRequest = createRequestWithCookies(new Cookie[]{}); + replay(bookmarksRequest); + + // Act: make request + Response bookmarksResponse = bookmarksFacade.getCurrentUserBookmarks(bookmarksRequest, QUESTION_TYPE); + + // Assert: check status code is unauthorised + assertEquals(Response.Status.UNAUTHORIZED.getStatusCode(), bookmarksResponse.getStatus()); + } + + @Test + public void getCurrentUserBookmarks_invalidContentType_returnsError() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest bookmarksRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(bookmarksRequest); + + // Act: make request + Response bookmarksResponse = bookmarksFacade.getCurrentUserBookmarks(bookmarksRequest, EVENT_TYPE); + + // Assert: check status code is bad request + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), bookmarksResponse.getStatus()); + } + @Test public void addCurrentUserBookmark_validContent_addsBookmark() throws Exception { // Arrange: log in, create request @@ -95,6 +122,19 @@ public void addCurrentUserBookmark_validContent_addsBookmark() throws Exception assertEquals(Response.Status.OK.getStatusCode(), addBookmarkResponse.getStatus()); } + @Test + public void addCurrentUserBookmark_noLoggedInUser_returnsError() { + // Arrange: create request with no cookie + HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{}); + replay(addBookmarkRequest); + + // Act: make request + Response addBookmarkResponse = bookmarksFacade.addCurrentUserBookmark(addBookmarkRequest, ITConstants.FUZZY_MATCH_TEST_PAGE_ID); + + // Assert: check status code is unauthorised + assertEquals(Response.Status.UNAUTHORIZED.getStatusCode(), addBookmarkResponse.getStatus()); + } + @Test public void addCurrentUserBookmark_notFoundContent_returnsError() throws Exception { // Arrange: log in, create request @@ -124,30 +164,57 @@ public void addCurrentUserBookmark_duplicateContent_returnsError() throws Except } @Test - public void deleteCurrentUserBookmark_validContent_deletesBookmark() throws Exception { + public void addCurrentUserBookmark_invalidContentType_returnsError() throws Exception { // Arrange: log in, create request LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); replay(addBookmarkRequest); // Act: make request - Response deleteBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(addBookmarkRequest, ITConstants.SEARCH_TEST_CONCEPT_ID); + Response addBookmarkResponse = bookmarksFacade.addCurrentUserBookmark(addBookmarkRequest, ITConstants.QUIZ_TEST_QUIZ_ID); + + // Assert: check status code is bad request + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), addBookmarkResponse.getStatus()); + } + + @Test + public void deleteCurrentUserBookmark_validContent_deletesBookmark() throws Exception { + // Arrange: log in, create request + LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); + HttpServletRequest deleteBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(deleteBookmarkRequest); + + // Act: make request + Response deleteBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(deleteBookmarkRequest, ITConstants.SEARCH_TEST_CONCEPT_ID); // Assert: check status code is OK assertEquals(Response.Status.OK.getStatusCode(), deleteBookmarkResponse.getStatus()); } + @Test + public void deleteCurrentUserBookmark_noLoggedInUser_returnsError() throws Exception { + // Arrange: create request with no cookie + HttpServletRequest deleteBookmarkRequest = createRequestWithCookies(new Cookie[]{}); + replay(deleteBookmarkRequest); + + // Act: make request + Response deleteBookmarksResponse = bookmarksFacade.removeCurrentUserBookmark(deleteBookmarkRequest, ITConstants.FUZZY_MATCH_TEST_PAGE_ID); + + // Assert: check status code is unauthorised + assertEquals(Response.Status.UNAUTHORIZED.getStatusCode(), deleteBookmarksResponse.getStatus()); + } + @Test public void deleteCurrentUserBookmark_notBookmarkedContent_returnsError() throws Exception { // Arrange: log in, create request LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); - HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); - replay(addBookmarkRequest); + HttpServletRequest deleteBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); + replay(deleteBookmarkRequest); // Act: make request - Response addBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(addBookmarkRequest, ITConstants.SEARCH_TEST_SUPERSEDED_BY_ID); + Response deleteBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(deleteBookmarkRequest, ITConstants.SEARCH_TEST_SUPERSEDED_BY_ID); // Assert: check status code is bad request - assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), addBookmarkResponse.getStatus()); + assertEquals(Response.Status.BAD_REQUEST.getStatusCode(), deleteBookmarkResponse.getStatus()); } } From 6215cd11b04bc3facc9b759d41f14ae42715ed61 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Tue, 21 Apr 2026 12:28:07 +0100 Subject: [PATCH 18/36] Clarify & improve various bookmarks functionality --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 54 +++++++++++-------- .../ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 3 +- .../cl/dtg/isaac/api/BookmarksFacadeIT.java | 20 +++---- 3 files changed, 43 insertions(+), 34 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 2b3bfb880b..2207eababe 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -34,7 +34,7 @@ * * Responsible for handling requests related to user bookmarks. */ -@Path("/") +@Path("/bookmarks") @Tag(name = "BookmarksFacade", description = "/bookmarks") public class BookmarksFacade { private static final Logger log = LoggerFactory.getLogger(BookmarksFacade.class); @@ -64,7 +64,7 @@ public BookmarksFacade(final UserAccountManager userManager, final GitContentMan * @return the list of content that the user has bookmarked. */ @GET - @Path("bookmarks") + @Path("/") @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Get bookmarks for the current user.") public final Response getCurrentUserBookmarks(@Context final HttpServletRequest request, @@ -76,7 +76,8 @@ public final Response getCurrentUserBookmarks(@Context final HttpServletRequest return SegueErrorResponse.getNotLoggedInResponse(); } - if (null != contentType && !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { + if (null != contentType && !contentType.isEmpty() + && !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { SegueErrorResponse error = new SegueErrorResponse(Status.BAD_REQUEST, "Invalid content type for bookmarks query: " + contentType); log.warn(error.getErrorMessage()); return error.toResponse(); @@ -95,7 +96,7 @@ public final Response getCurrentUserBookmarks(@Context final HttpServletRequest * - the id of the content to bookmark. */ @POST - @Path("bookmarks") + @Path("/") @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Add a bookmark for the current user.") public final Response addCurrentUserBookmark(@Context final HttpServletRequest request, @@ -107,30 +108,37 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r return SegueErrorResponse.getNotLoggedInResponse(); } + if (null == contentId || contentId.isEmpty()) { + return new SegueErrorResponse(Status.BAD_REQUEST, "Cannot create bookmark without content ID.").toResponse(); + } + List currentBookmarks = bookmarksDbManager.getBookmarksForUser(user.getId()); if (currentBookmarks.size() >= 100) { return new SegueErrorResponse(Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.").toResponse(); - } else if (currentBookmarks.stream().anyMatch(b -> b.contentId().equals(contentId))) { + } + + if (currentBookmarks.stream().anyMatch(b -> b.contentId().equals(contentId))) { return new SegueErrorResponse(Status.BAD_REQUEST, "You have already bookmarked this content.").toResponse(); - } else { - try { - ContentDTO content = this.contentManager.getContentById(contentId); - String contentType = content.getType(); - if ((null == contentType) || !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { - SegueErrorResponse error = new SegueErrorResponse(Status.BAD_REQUEST, - "Invalid content type for bookmark: " + contentType); - log.error(error.getErrorMessage()); - return error.toResponse(); - } - bookmarksDbManager.addBookmarkForUser(user.getId(), contentId, contentType); - } catch (final ContentManagerException | NullPointerException e) { - SegueErrorResponse error = new SegueErrorResponse(Status.NOT_FOUND, - "Failed to create bookmark, could not find content: " + contentId, e); - log.error(error.getErrorMessage(), e); + } + + try { + ContentDTO content = this.contentManager.getContentById(contentId); + String contentType = content.getType(); + if (null == contentType || !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { + SegueErrorResponse error = new SegueErrorResponse(Status.BAD_REQUEST, + "Invalid content type for bookmark: " + contentType); + log.error(error.getErrorMessage()); return error.toResponse(); } + bookmarksDbManager.addBookmarkForUser(user.getId(), contentId, contentType); + } catch (final ContentManagerException | NullPointerException e) { + SegueErrorResponse error = new SegueErrorResponse(Status.NOT_FOUND, + "Failed to create bookmark, could not find content with ID: " + contentId, e); + log.error(error.getErrorMessage(), e); + return error.toResponse(); } + return Response.ok().build(); } @@ -143,7 +151,7 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r * - the id of the content to be removed from the user's bookmarks. */ @DELETE - @Path("bookmarks") + @Path("/") @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Remove a bookmark for the current user.") public final Response removeCurrentUserBookmark(@Context final HttpServletRequest request, @@ -155,6 +163,10 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques return SegueErrorResponse.getNotLoggedInResponse(); } + if (null == contentId || contentId.isEmpty()) { + return new SegueErrorResponse(Status.BAD_REQUEST, "Cannot delete bookmark without content ID.").toResponse(); + } + List currentBookmarks = bookmarksDbManager.getBookmarksForUser(user.getId()); if (currentBookmarks.stream().noneMatch(b -> b.contentId().equals(contentId))) { return new SegueErrorResponse(Status.BAD_REQUEST, "You have not bookmarked this content.").toResponse(); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index 23075ffcf8..fde1daef54 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -44,11 +44,12 @@ public List getBookmarksForUser(final Long userId, final String cont String query = "SELECT content_id, created FROM user_bookmarks WHERE user_id = ?"; boolean filterByContentType = false; - if (null != contentType) { + if (null != contentType && !contentType.isEmpty()) { if (contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage")) { query += " AND content_type = ?"; filterByContentType = true; } else { + // Should have already been caught at facade level log.warn("Invalid content type provided for bookmarks query: {}", contentType); throw new IllegalArgumentException("Invalid content type for bookmarks query: " + contentType); } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java index 5fa5bc54a4..ef5cc04c4a 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -50,10 +50,8 @@ public void getCurrentUserBookmarks_searchWithoutContentType_returnsAllBookmarks ArrayList responseBody = (ArrayList) bookmarksResponse.getEntity(); List actualBookmarkIds = responseBody.stream().map(ContentSummaryDTO::getId).toList(); - List expectedBookmarkIds = new ArrayList<>(); - expectedBookmarkIds.add(ITConstants.REGRESSION_TEST_PAGE_ID); - expectedBookmarkIds.add(ITConstants.ASSIGNMENT_TEST_PAGE_ID); - expectedBookmarkIds.add(ITConstants.SEARCH_TEST_CONCEPT_ID); + List expectedBookmarkIds = List.of(ITConstants.REGRESSION_TEST_PAGE_ID, ITConstants.ASSIGNMENT_TEST_PAGE_ID, + ITConstants.SEARCH_TEST_CONCEPT_ID); assertEquals(expectedBookmarkIds, actualBookmarkIds); } @@ -74,16 +72,14 @@ public void getCurrentUserBookmarks_searchWithContentType_returnsFilteredBookmar ArrayList responseBody = (ArrayList) bookmarksResponse.getEntity(); List actualBookmarkIds = responseBody.stream().map(ContentSummaryDTO::getId).toList(); - List expectedBookmarkIds = new ArrayList<>(); - expectedBookmarkIds.add(ITConstants.REGRESSION_TEST_PAGE_ID); - expectedBookmarkIds.add(ITConstants.ASSIGNMENT_TEST_PAGE_ID); + List expectedBookmarkIds = List.of(ITConstants.REGRESSION_TEST_PAGE_ID, ITConstants.ASSIGNMENT_TEST_PAGE_ID); assertEquals(expectedBookmarkIds, actualBookmarkIds); } @Test public void getCurrentUserBookmarks_noLoggedInUser_returnsError() { - // Arrange: create request with no cookie + // Arrange: create request without valid cookie HttpServletRequest bookmarksRequest = createRequestWithCookies(new Cookie[]{}); replay(bookmarksRequest); @@ -109,7 +105,7 @@ public void getCurrentUserBookmarks_invalidContentType_returnsError() throws Exc } @Test - public void addCurrentUserBookmark_validContent_addsBookmark() throws Exception { + public void addCurrentUserBookmark_validContent_returnsOK() throws Exception { // Arrange: log in, create request LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); @@ -124,7 +120,7 @@ public void addCurrentUserBookmark_validContent_addsBookmark() throws Exception @Test public void addCurrentUserBookmark_noLoggedInUser_returnsError() { - // Arrange: create request with no cookie + // Arrange: create request without valid cookie HttpServletRequest addBookmarkRequest = createRequestWithCookies(new Cookie[]{}); replay(addBookmarkRequest); @@ -178,7 +174,7 @@ public void addCurrentUserBookmark_invalidContentType_returnsError() throws Exce } @Test - public void deleteCurrentUserBookmark_validContent_deletesBookmark() throws Exception { + public void deleteCurrentUserBookmark_bookmarkedContent_returnsOK() throws Exception { // Arrange: log in, create request LoginResult login = loginAs(httpSession, ITConstants.ALICE_STUDENT_EMAIL, ITConstants.ALICE_STUDENT_PASSWORD); HttpServletRequest deleteBookmarkRequest = createRequestWithCookies(new Cookie[]{login.cookie}); @@ -193,7 +189,7 @@ public void deleteCurrentUserBookmark_validContent_deletesBookmark() throws Exce @Test public void deleteCurrentUserBookmark_noLoggedInUser_returnsError() throws Exception { - // Arrange: create request with no cookie + // Arrange: create request without valid cookie HttpServletRequest deleteBookmarkRequest = createRequestWithCookies(new Cookie[]{}); replay(deleteBookmarkRequest); From d2b888a4a1edd8da588985e8559c97dd49f31471 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Tue, 21 Apr 2026 13:53:45 +0100 Subject: [PATCH 19/36] Fix test cleanup --- .../cl/dtg/isaac/api/BookmarksFacadeIT.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java index ef5cc04c4a..05906453cf 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -27,11 +27,19 @@ public void setUp() { @AfterEach public void tearDown() throws Exception { - // Clean up bookmark added during tests - PreparedStatement deleteBookmarks = postgresSqlDb.getDatabaseConnection().prepareStatement("DELETE FROM user_bookmarks WHERE user_id = ? AND content_id = ?"); - deleteBookmarks.setLong(1, ITConstants.ALICE_STUDENT_ID); - deleteBookmarks.setString(2, ITConstants.FUZZY_MATCH_TEST_PAGE_ID); - deleteBookmarks.executeUpdate(); + // Restore ALICE_STUDENT bookmarks to original state + PreparedStatement pstDelete = postgresSqlDb.getDatabaseConnection().prepareStatement( + "DELETE FROM user_bookmarks WHERE user_id = ? AND content_id = ?"); + pstDelete.setLong(1, ITConstants.ALICE_STUDENT_ID); + pstDelete.setString(2, ITConstants.FUZZY_MATCH_TEST_PAGE_ID); + pstDelete.executeUpdate(); + + PreparedStatement pstInsert = postgresSqlDb.getDatabaseConnection().prepareStatement( + "INSERT INTO user_bookmarks (user_id, content_id, content_type, created) VALUES (?, ?, ?, NOW()) ON CONFLICT DO NOTHING"); + pstInsert.setLong(1, ITConstants.ALICE_STUDENT_ID); + pstInsert.setString(2, ITConstants.SEARCH_TEST_CONCEPT_ID); + pstInsert.setString(3, CONCEPT_TYPE); + pstInsert.executeUpdate(); } @Test From c6df3aaaebbd9c8f16c8c17c092c1f0e20b2bc81 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Thu, 23 Apr 2026 14:56:36 +0100 Subject: [PATCH 20/36] Improve logging & error responses --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 17 ++++++----------- .../uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 2 +- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 2207eababe..3c15a2861c 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -78,9 +78,8 @@ public final Response getCurrentUserBookmarks(@Context final HttpServletRequest if (null != contentType && !contentType.isEmpty() && !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { - SegueErrorResponse error = new SegueErrorResponse(Status.BAD_REQUEST, "Invalid content type for bookmarks query: " + contentType); - log.warn(error.getErrorMessage()); - return error.toResponse(); + log.warn("Invalid content type provided for bookmarks query: {}", contentType); + return new SegueErrorResponse(Status.BAD_REQUEST, "Only question and concept pages can be bookmarked!").toResponse(); } List bookmarks = bookmarksDbManager.getBookmarksForUser(user.getId(), contentType); @@ -126,17 +125,13 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r ContentDTO content = this.contentManager.getContentById(contentId); String contentType = content.getType(); if (null == contentType || !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { - SegueErrorResponse error = new SegueErrorResponse(Status.BAD_REQUEST, - "Invalid content type for bookmark: " + contentType); - log.error(error.getErrorMessage()); - return error.toResponse(); + log.warn("Invalid content type provided for bookmarks query: {}", contentType); + return new SegueErrorResponse(Status.BAD_REQUEST, "Only question and concept pages can be bookmarked!").toResponse(); } bookmarksDbManager.addBookmarkForUser(user.getId(), contentId, contentType); } catch (final ContentManagerException | NullPointerException e) { - SegueErrorResponse error = new SegueErrorResponse(Status.NOT_FOUND, - "Failed to create bookmark, could not find content with ID: " + contentId, e); - log.error(error.getErrorMessage(), e); - return error.toResponse(); + log.warn("Failed to create bookmark, could not find content with ID: {}", contentId); + return new SegueErrorResponse(Status.NOT_FOUND, "Unable to find content to bookmark.").toResponse(); } return Response.ok().build(); diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index fde1daef54..12ba73df6c 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -73,7 +73,7 @@ public List getBookmarksForUser(final Long userId, final String cont bookmarks.add(new BookmarkDO(contentId, created)); } } catch (final SQLException e) { - e.printStackTrace(); + log.error("Database error saving bookmark!", e); } return bookmarks; } From b474152f5770184c649cfdb298dd7b548be42de1 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Thu, 23 Apr 2026 14:57:25 +0100 Subject: [PATCH 21/36] Remove redundant content type check --- src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index 12ba73df6c..5ad4dda6e0 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -48,10 +48,6 @@ public List getBookmarksForUser(final Long userId, final String cont if (contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage")) { query += " AND content_type = ?"; filterByContentType = true; - } else { - // Should have already been caught at facade level - log.warn("Invalid content type provided for bookmarks query: {}", contentType); - throw new IllegalArgumentException("Invalid content type for bookmarks query: " + contentType); } } From 5de26fb43689dc64fa08ccf70f11c0f556cc1ec2 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Thu, 23 Apr 2026 15:00:03 +0100 Subject: [PATCH 22/36] Use "no content" HTTP response for successful bookmarks deletions --- src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 3c15a2861c..0d0dd854ce 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -168,6 +168,6 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques } bookmarksDbManager.removeBookmarkForUser(user.getId(), contentId); - return Response.ok().build(); + return Response.noContent().build(); } } From d72fabca0c7f913a2024e7e53e18b07793fe5473 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Thu, 23 Apr 2026 15:12:32 +0100 Subject: [PATCH 23/36] Update status code in test --- .../java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java index 05906453cf..b3ea938aaf 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -191,8 +191,8 @@ public void deleteCurrentUserBookmark_bookmarkedContent_returnsOK() throws Excep // Act: make request Response deleteBookmarkResponse = bookmarksFacade.removeCurrentUserBookmark(deleteBookmarkRequest, ITConstants.SEARCH_TEST_CONCEPT_ID); - // Assert: check status code is OK - assertEquals(Response.Status.OK.getStatusCode(), deleteBookmarkResponse.getStatus()); + // Assert: check status code is no content + assertEquals(Response.Status.NO_CONTENT.getStatusCode(), deleteBookmarkResponse.getStatus()); } @Test From b492bffbdbe029c24bcc24f8b4d739c5924d2543 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Thu, 23 Apr 2026 15:23:02 +0100 Subject: [PATCH 24/36] Move content ID to path param --- .../java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 0d0dd854ce..d3241712c2 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -21,6 +21,7 @@ import jakarta.ws.rs.GET; import jakarta.ws.rs.POST; import jakarta.ws.rs.Path; +import jakarta.ws.rs.PathParam; import jakarta.ws.rs.Produces; import jakarta.ws.rs.QueryParam; import jakarta.ws.rs.core.Context; @@ -95,11 +96,11 @@ public final Response getCurrentUserBookmarks(@Context final HttpServletRequest * - the id of the content to bookmark. */ @POST - @Path("/") + @Path("/{content_id}") @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Add a bookmark for the current user.") public final Response addCurrentUserBookmark(@Context final HttpServletRequest request, - @QueryParam("content_id") final String contentId) { + @PathParam("content_id") final String contentId) { RegisteredUserDTO user; try { user = userManager.getCurrentRegisteredUser(request); @@ -146,11 +147,11 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r * - the id of the content to be removed from the user's bookmarks. */ @DELETE - @Path("/") + @Path("/{content_id}") @Produces(MediaType.APPLICATION_JSON) @Operation(summary = "Remove a bookmark for the current user.") public final Response removeCurrentUserBookmark(@Context final HttpServletRequest request, - @QueryParam("content_id") final String contentId) { + @PathParam("content_id") final String contentId) { RegisteredUserDTO user; try { user = userManager.getCurrentRegisteredUser(request); From e87e942f4cd844c0b270c7e309139e6c0bf7b61e Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Thu, 23 Apr 2026 15:44:30 +0100 Subject: [PATCH 25/36] Replace more stack traces with log messages --- src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index 5ad4dda6e0..93e66d5531 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -91,7 +91,7 @@ public void addBookmarkForUser(final Long userId, final String contentId, final } } catch (final SQLException | SegueDatabaseException e) { - e.printStackTrace(); + log.error("Database error saving bookmark!", e); } } @@ -107,7 +107,7 @@ public void removeBookmarkForUser(final Long userId, final String contentId) { throw new SegueDatabaseException("Unable to remove bookmark."); } } catch (final SQLException | SegueDatabaseException e) { - e.printStackTrace(); + log.error("Database error saving bookmark!", e); } } } From d97b489e9974efef38ce0736994ea545be0ee4c0 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 10:24:20 +0100 Subject: [PATCH 26/36] Augment bookmarks with content info more efficiently --- .../isaac/api/managers/BookmarksManager.java | 54 +++++++++++-------- 1 file changed, 32 insertions(+), 22 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java index 298102e342..d9369ef279 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -5,15 +5,17 @@ import org.slf4j.LoggerFactory; import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; +import uk.ac.cam.cl.dtg.isaac.dto.ResultsWrapper; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; 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.mappers.MainMapper; -import java.util.ArrayList; import java.util.Date; +import java.util.HashMap; import java.util.List; +import java.util.Map; /** * A class to augment content with bookmark information, and bookmarks with content information. @@ -48,16 +50,28 @@ public BookmarksManager(final IBookmarks bookmarksDbManager, final GitContentMan */ public List augmentContentSummaryListWithBookmarkInformation(final Long userId, final List contentSummaries) { - List bookmarks = this.bookmarksDbManager.getBookmarksForUser(userId); + return augmentContentSummaryListWithBookmarkInformation(bookmarks, contentSummaries); + } + + /** + * Augment a list of content summaries with bookmark information. + * + * @param bookmarks the bookmarks to augment the content summaries with. + * @param contentSummaries the content summary list to augment. + * @return the augmented content summary list. + */ + public List augmentContentSummaryListWithBookmarkInformation(final List bookmarks, + final List contentSummaries) { + Map bookmarkMap = new HashMap<>(); + for (BookmarkDO bookmark : bookmarks) { + bookmarkMap.put(bookmark.contentId(), bookmark.created()); + } for (ContentSummaryDTO contentSummary : contentSummaries) { - for (BookmarkDO bookmark : bookmarks) { - if (contentSummary.getId().equals(bookmark.contentId())) { - Date created = bookmark.created(); - contentSummary.setBookmarked(created); - break; - } + Date created = bookmarkMap.get(contentSummary.getId()); + if (created != null) { + contentSummary.setBookmarked(created); } } return contentSummaries; @@ -70,23 +84,19 @@ public List augmentContentSummaryListWithBookmarkInformation( * @return the list of content summaries corresponding to the bookmarks. */ public List mapBookmarkListToContentSummaryList(final List bookmarks) { + List bookmarkIDs = bookmarks.stream().map(BookmarkDO::contentId).toList(); + ResultsWrapper content; - List contentSummaries = new ArrayList<>(); - - for (BookmarkDO bookmark : bookmarks) { - try { - ContentDTO content = this.contentManager.getContentById(bookmark.contentId()); - ContentSummaryDTO contentSummary = this.mapper.mapContentDTOtoContentSummaryDTO(content); + try { + content = this.contentManager.getUnsafeCachedContentDTOsMatchingIds(bookmarkIDs, 0, bookmarkIDs.size()); + } catch (final ContentManagerException e) { + content = new ResultsWrapper<>(); + log.error("Unable to locate content for bookmark", e); + } - Date created = bookmark.created(); - contentSummary.setBookmarked(created); + List contentSummaries = content.getResults().stream().map(this.mapper::mapContentDTOtoContentSummaryDTO).toList(); + contentSummaries = augmentContentSummaryListWithBookmarkInformation(bookmarks, contentSummaries); - contentSummaries.add(contentSummary); - } catch (final ContentManagerException e) { - log.warn("Error retrieving content for bookmark with content id {}: {}", bookmark.contentId(), e.getMessage()); - throw new RuntimeException(e); - } - } return contentSummaries; } } From 26eb621d11a5beac137356b94017c47a4e952d36 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 10:47:46 +0100 Subject: [PATCH 27/36] Fix tests --- .../uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java index b3ea938aaf..87fae05171 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -9,9 +9,9 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.ws.rs.core.Response; import java.sql.PreparedStatement; -import java.util.ArrayList; import java.util.List; +import static org.assertj.core.api.Assertions.assertThat; import static org.easymock.EasyMock.replay; import static org.junit.jupiter.api.Assertions.assertEquals; import static uk.ac.cam.cl.dtg.isaac.api.Constants.*; @@ -55,13 +55,13 @@ public void getCurrentUserBookmarks_searchWithoutContentType_returnsAllBookmarks // Assert: check response is OK and contains expected bookmarks assertEquals(Response.Status.OK.getStatusCode(), bookmarksResponse.getStatus()); - ArrayList responseBody = (ArrayList) bookmarksResponse.getEntity(); + List responseBody = (List) bookmarksResponse.getEntity(); List actualBookmarkIds = responseBody.stream().map(ContentSummaryDTO::getId).toList(); List expectedBookmarkIds = List.of(ITConstants.REGRESSION_TEST_PAGE_ID, ITConstants.ASSIGNMENT_TEST_PAGE_ID, ITConstants.SEARCH_TEST_CONCEPT_ID); - assertEquals(expectedBookmarkIds, actualBookmarkIds); + assertThat(actualBookmarkIds).containsExactlyInAnyOrderElementsOf(expectedBookmarkIds); } @Test @@ -77,12 +77,12 @@ public void getCurrentUserBookmarks_searchWithContentType_returnsFilteredBookmar // Assert: check status code is OK and contains expected bookmarks assertEquals(Response.Status.OK.getStatusCode(), bookmarksResponse.getStatus()); - ArrayList responseBody = (ArrayList) bookmarksResponse.getEntity(); + List responseBody = (List) bookmarksResponse.getEntity(); List actualBookmarkIds = responseBody.stream().map(ContentSummaryDTO::getId).toList(); List expectedBookmarkIds = List.of(ITConstants.REGRESSION_TEST_PAGE_ID, ITConstants.ASSIGNMENT_TEST_PAGE_ID); - assertEquals(expectedBookmarkIds, actualBookmarkIds); + assertThat(actualBookmarkIds).containsExactlyInAnyOrderElementsOf(expectedBookmarkIds); } @Test From 87054040934fdc3ca904e119235103b06f08e0dd Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 14:26:45 +0100 Subject: [PATCH 28/36] Make BookmarksFacade extend AbstractIsaacFacade --- .../java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 9 +++++++-- .../uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java | 2 +- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index d3241712c2..b6ef71635c 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -13,8 +13,10 @@ import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; +import uk.ac.cam.cl.dtg.segue.dao.ILogManager; 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.AbstractConfigLoader; import jakarta.servlet.http.HttpServletRequest; import jakarta.ws.rs.DELETE; @@ -37,7 +39,7 @@ */ @Path("/bookmarks") @Tag(name = "BookmarksFacade", description = "/bookmarks") -public class BookmarksFacade { +public class BookmarksFacade extends AbstractIsaacFacade { private static final Logger log = LoggerFactory.getLogger(BookmarksFacade.class); private final UserAccountManager userManager; @@ -46,8 +48,11 @@ public class BookmarksFacade { private final IBookmarks bookmarksDbManager; @Inject - public BookmarksFacade(final UserAccountManager userManager, final GitContentManager contentManager, + public BookmarksFacade(final AbstractConfigLoader propertiesLoader, final ILogManager logManager, + final UserAccountManager userManager, final GitContentManager contentManager, final BookmarksManager bookmarksManager, final IBookmarks bookmarksDbManager) { + super(propertiesLoader, logManager); + this.userManager = userManager; this.contentManager = contentManager; this.bookmarksManager = bookmarksManager; diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java index 87fae05171..5f64c915b9 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -22,7 +22,7 @@ public class BookmarksFacadeIT extends IsaacIntegrationTest { @BeforeEach public void setUp() { - this.bookmarksFacade = new BookmarksFacade(userAccountManager, contentManager, bookmarksManager, bookmarksDbManager); + this.bookmarksFacade = new BookmarksFacade(properties, logManager, userAccountManager, contentManager, bookmarksManager, bookmarksDbManager); } @AfterEach From b166b3e1db302e48a978fb3d2369b22e2ed4f5f1 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 14:32:25 +0100 Subject: [PATCH 29/36] Move max number of bookmarks to constant --- .../java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 6 ++++-- src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java | 5 +++++ 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index b6ef71635c..93c9a0b709 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -32,6 +32,8 @@ import jakarta.ws.rs.core.Response.Status; import java.util.List; +import static uk.ac.cam.cl.dtg.isaac.api.Constants.*; + /** * Bookmarks Facade * @@ -119,8 +121,8 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r List currentBookmarks = bookmarksDbManager.getBookmarksForUser(user.getId()); - if (currentBookmarks.size() >= 100) { - return new SegueErrorResponse(Status.BAD_REQUEST, "You cannot have more than 100 bookmarks.").toResponse(); + if (currentBookmarks.size() >= MAXIMUM_BOOKMARKS) { + return new SegueErrorResponse(Status.BAD_REQUEST, "You already have the maximum number of bookmarks!.").toResponse(); } if (currentBookmarks.stream().anyMatch(b -> b.contentId().equals(contentId))) { diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java index aaec542028..0d0e916254 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java @@ -233,6 +233,11 @@ public enum IsaacUserPreferences { */ public static final long QUIZ_VIEW_STUDENT_ANSWERS_RELEASE_TIMESTAMP = Date.UTC(123, Calendar.JUNE, 12, 0, 0, 0); // 12/06/2023 + /** + * Bookmarks + */ + public static final int MAXIMUM_BOOKMARKS = 100; + /** * Feedback messages */ From 04f288c25730656f4dfc845342b344b1d848d68f Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 15:31:22 +0100 Subject: [PATCH 30/36] Store all bookmark properties in BookmarkDO --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 10 ++++++++-- .../ac/cam/cl/dtg/isaac/dos/BookmarkDO.java | 2 +- .../ac/cam/cl/dtg/isaac/dos/IBookmarks.java | 4 ++-- .../ac/cam/cl/dtg/isaac/dos/PgBookmarks.java | 20 +++++++++---------- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 93c9a0b709..40fbb3e047 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -30,6 +30,7 @@ import jakarta.ws.rs.core.MediaType; import jakarta.ws.rs.core.Response; import jakarta.ws.rs.core.Response.Status; +import java.util.Date; import java.util.List; import static uk.ac.cam.cl.dtg.isaac.api.Constants.*; @@ -136,7 +137,10 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r log.warn("Invalid content type provided for bookmarks query: {}", contentType); return new SegueErrorResponse(Status.BAD_REQUEST, "Only question and concept pages can be bookmarked!").toResponse(); } - bookmarksDbManager.addBookmarkForUser(user.getId(), contentId, contentType); + + BookmarkDO bookmarkToAdd = new BookmarkDO(user.getId(), contentId, contentType, new Date()); + bookmarksDbManager.addBookmarkForUser(bookmarkToAdd); + } catch (final ContentManagerException | NullPointerException e) { log.warn("Failed to create bookmark, could not find content with ID: {}", contentId); return new SegueErrorResponse(Status.NOT_FOUND, "Unable to find content to bookmark.").toResponse(); @@ -175,7 +179,9 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques return new SegueErrorResponse(Status.BAD_REQUEST, "You have not bookmarked this content.").toResponse(); } - bookmarksDbManager.removeBookmarkForUser(user.getId(), contentId); + BookmarkDO bookmarkToRemove = new BookmarkDO(user.getId(), contentId, null, null); + bookmarksDbManager.removeBookmarkForUser(bookmarkToRemove); + return Response.noContent().build(); } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/BookmarkDO.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/BookmarkDO.java index 629c03f9e8..435ceefa76 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/BookmarkDO.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/BookmarkDO.java @@ -5,4 +5,4 @@ /** * DO representing a bookmarked piece of content. */ -public record BookmarkDO(String contentId, Date created) {} +public record BookmarkDO(Long userId, String contentId, String contentType, Date created) {} diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java index de018671e8..d1740b6b76 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java @@ -12,7 +12,7 @@ public interface IBookmarks { List getBookmarksForUser(Long userId, String contentType); - void addBookmarkForUser(Long userId, String contentId, String contentType); + void addBookmarkForUser(BookmarkDO bookmark); - void removeBookmarkForUser(Long userId, String contentId); + void removeBookmarkForUser(BookmarkDO bookmark); } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java index 93e66d5531..9355c70bb4 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java @@ -66,7 +66,7 @@ public List getBookmarksForUser(final Long userId, final String cont while (results.next()) { String contentId = results.getString("content_id"); Timestamp created = results.getTimestamp("created"); - bookmarks.add(new BookmarkDO(contentId, created)); + bookmarks.add(new BookmarkDO(userId, contentId, contentType, created)); } } catch (final SQLException e) { log.error("Database error saving bookmark!", e); @@ -75,34 +75,32 @@ public List getBookmarksForUser(final Long userId, final String cont } @Override - public void addBookmarkForUser(final Long userId, final String contentId, final String contentType) { - Timestamp created = new Timestamp(System.currentTimeMillis()); + public void addBookmarkForUser(final BookmarkDO bookmark) { String query = "INSERT INTO user_bookmarks (user_id, content_id, content_type, created) VALUES (?, ?, ?, ?)"; try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query); ) { - pst.setLong(1, userId); - pst.setString(2, contentId); - pst.setString(3, contentType); - pst.setTimestamp(4, created); + pst.setLong(1, bookmark.userId()); + pst.setString(2, bookmark.contentId()); + pst.setString(3, bookmark.contentType()); + pst.setTimestamp(4, (Timestamp) bookmark.created()); if (pst.executeUpdate() == 0) { throw new SegueDatabaseException("Unable to save bookmark."); } - } catch (final SQLException | SegueDatabaseException e) { log.error("Database error saving bookmark!", e); } } @Override - public void removeBookmarkForUser(final Long userId, final String contentId) { + public void removeBookmarkForUser(final BookmarkDO bookmark) { String query = "DELETE FROM user_bookmarks WHERE user_id = ? AND content_id = ?"; try (Connection conn = database.getDatabaseConnection(); PreparedStatement pst = conn.prepareStatement(query); ) { - pst.setLong(1, userId); - pst.setString(2, contentId); + pst.setLong(1, bookmark.userId()); + pst.setString(2, bookmark.contentId()); if (pst.executeUpdate() == 0) { throw new SegueDatabaseException("Unable to remove bookmark."); } From 19eb56f5d4cbd1986a944748fef0e042c286ffd8 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 15:47:28 +0100 Subject: [PATCH 31/36] Move bookmarks DB layer to DAO package --- src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 2 +- .../uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java | 2 +- .../java/uk/ac/cam/cl/dtg/isaac/{dos => dao}/IBookmarks.java | 4 ++-- .../java/uk/ac/cam/cl/dtg/isaac/{dos => dao}/PgBookmarks.java | 4 ++-- .../segue/configuration/SegueGuiceConfigurationModule.java | 4 ++-- .../ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java | 4 ++-- 6 files changed, 10 insertions(+), 10 deletions(-) rename src/main/java/uk/ac/cam/cl/dtg/isaac/{dos => dao}/IBookmarks.java (79%) rename src/main/java/uk/ac/cam/cl/dtg/isaac/{dos => dao}/PgBookmarks.java (97%) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index 40fbb3e047..d311029ba8 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -6,8 +6,8 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import uk.ac.cam.cl.dtg.isaac.api.managers.BookmarksManager; +import uk.ac.cam.cl.dtg.isaac.dao.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; -import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java index d9369ef279..133c40cc94 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -3,8 +3,8 @@ import com.google.inject.Inject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import uk.ac.cam.cl.dtg.isaac.dao.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; -import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dto.ResultsWrapper; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/IBookmarks.java similarity index 79% rename from src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java rename to src/main/java/uk/ac/cam/cl/dtg/isaac/dao/IBookmarks.java index d1740b6b76..93f216a65f 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/IBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/IBookmarks.java @@ -1,6 +1,6 @@ -package uk.ac.cam.cl.dtg.isaac.dos; +package uk.ac.cam.cl.dtg.isaac.dao; -import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; +import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; import java.util.List; diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java similarity index 97% rename from src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java rename to src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java index 9355c70bb4..ac80597ce9 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dos/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java @@ -1,9 +1,9 @@ -package uk.ac.cam.cl.dtg.isaac.dos; +package uk.ac.cam.cl.dtg.isaac.dao; import com.google.inject.Inject; import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; +import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; import uk.ac.cam.cl.dtg.segue.database.PostgresSqlDb; 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 a5127f4004..398b5fa9b6 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 @@ -46,21 +46,21 @@ import uk.ac.cam.cl.dtg.isaac.api.services.GroupChangedService; import uk.ac.cam.cl.dtg.isaac.dao.GameboardPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.IAssignmentPersistenceManager; +import uk.ac.cam.cl.dtg.isaac.dao.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dao.IQuizAssignmentPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.IQuizAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.IQuizQuestionAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgAssignmentPersistenceManager; +import uk.ac.cam.cl.dtg.isaac.dao.PgBookmarks; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizAssignmentPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizQuestionAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dos.AbstractUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.dos.IUserAlerts; -import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.IUserStreaksManager; import uk.ac.cam.cl.dtg.isaac.dos.LocationHistory; import uk.ac.cam.cl.dtg.isaac.dos.PgLocationHistory; import uk.ac.cam.cl.dtg.isaac.dos.PgUserAlerts; -import uk.ac.cam.cl.dtg.isaac.dos.PgBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.PgUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.dos.PgUserStreakManager; import uk.ac.cam.cl.dtg.isaac.quiz.IQuestionAttemptManager; diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java index 38cadc5dd2..273c8ef934 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/AbstractIsaacIntegrationTest.java @@ -33,16 +33,16 @@ import uk.ac.cam.cl.dtg.isaac.dao.EventBookingPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.GameboardPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.IAssignmentPersistenceManager; +import uk.ac.cam.cl.dtg.isaac.dao.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dao.IQuizAssignmentPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.IQuizAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.IQuizQuestionAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgAssignmentPersistenceManager; +import uk.ac.cam.cl.dtg.isaac.dao.PgBookmarks; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizAssignmentPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dao.PgQuizQuestionAttemptPersistenceManager; import uk.ac.cam.cl.dtg.isaac.dos.AbstractUserPreferenceManager; -import uk.ac.cam.cl.dtg.isaac.dos.IBookmarks; -import uk.ac.cam.cl.dtg.isaac.dos.PgBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.PgUserPreferenceManager; import uk.ac.cam.cl.dtg.isaac.quiz.PgQuestionAttempts; import uk.ac.cam.cl.dtg.segue.api.Constants; From ea2e252c5254b9ff5258fc91d88fbeb7c078e6a6 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 16:28:27 +0100 Subject: [PATCH 32/36] Avoid direct interaction with DB layer from bookmarks facade --- .../cam/cl/dtg/isaac/api/BookmarksFacade.java | 21 ++++------ .../isaac/api/managers/BookmarksManager.java | 41 +++++++++++++++++++ .../cl/dtg/isaac/api/BookmarksFacadeIT.java | 2 +- 3 files changed, 51 insertions(+), 13 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index d311029ba8..f6a7f2ee0a 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -6,10 +6,10 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import uk.ac.cam.cl.dtg.isaac.api.managers.BookmarksManager; -import uk.ac.cam.cl.dtg.isaac.dao.IBookmarks; import uk.ac.cam.cl.dtg.isaac.dos.BookmarkDO; import uk.ac.cam.cl.dtg.isaac.dto.SegueErrorResponse; import uk.ac.cam.cl.dtg.isaac.dto.content.ContentDTO; +import uk.ac.cam.cl.dtg.isaac.dto.content.ContentSummaryDTO; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; import uk.ac.cam.cl.dtg.segue.api.managers.UserAccountManager; import uk.ac.cam.cl.dtg.segue.auth.exceptions.NoUserLoggedInException; @@ -48,18 +48,16 @@ public class BookmarksFacade extends AbstractIsaacFacade { private final UserAccountManager userManager; private final GitContentManager contentManager; private final BookmarksManager bookmarksManager; - private final IBookmarks bookmarksDbManager; @Inject public BookmarksFacade(final AbstractConfigLoader propertiesLoader, final ILogManager logManager, final UserAccountManager userManager, final GitContentManager contentManager, - final BookmarksManager bookmarksManager, final IBookmarks bookmarksDbManager) { + final BookmarksManager bookmarksManager) { super(propertiesLoader, logManager); this.userManager = userManager; this.contentManager = contentManager; this.bookmarksManager = bookmarksManager; - this.bookmarksDbManager = bookmarksDbManager; } /** @@ -68,8 +66,7 @@ public BookmarksFacade(final AbstractConfigLoader propertiesLoader, final ILogMa * @param request * - so we can find the current user. * @param contentType - * - optional query parameter to filter bookmarks by content type. Valid values are "isaacQuestionPage" - * and "isaacConceptPage". If null, all bookmarks will be returned. + * - the type of content to filter bookmarks by, or null to return all bookmarks. * @return the list of content that the user has bookmarked. */ @GET @@ -91,8 +88,8 @@ public final Response getCurrentUserBookmarks(@Context final HttpServletRequest return new SegueErrorResponse(Status.BAD_REQUEST, "Only question and concept pages can be bookmarked!").toResponse(); } - List bookmarks = bookmarksDbManager.getBookmarksForUser(user.getId(), contentType); - return Response.ok(bookmarksManager.mapBookmarkListToContentSummaryList(bookmarks)).build(); + List bookmarks = bookmarksManager.getAugmentedBookmarksForUser(user.getId(), contentType); + return Response.ok(bookmarks).build(); } /** @@ -120,7 +117,7 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r return new SegueErrorResponse(Status.BAD_REQUEST, "Cannot create bookmark without content ID.").toResponse(); } - List currentBookmarks = bookmarksDbManager.getBookmarksForUser(user.getId()); + List currentBookmarks = bookmarksManager.getBookmarksForUser(user.getId(), null); if (currentBookmarks.size() >= MAXIMUM_BOOKMARKS) { return new SegueErrorResponse(Status.BAD_REQUEST, "You already have the maximum number of bookmarks!.").toResponse(); @@ -139,7 +136,7 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r } BookmarkDO bookmarkToAdd = new BookmarkDO(user.getId(), contentId, contentType, new Date()); - bookmarksDbManager.addBookmarkForUser(bookmarkToAdd); + bookmarksManager.addBookmarkForUser(bookmarkToAdd); } catch (final ContentManagerException | NullPointerException e) { log.warn("Failed to create bookmark, could not find content with ID: {}", contentId); @@ -174,13 +171,13 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques return new SegueErrorResponse(Status.BAD_REQUEST, "Cannot delete bookmark without content ID.").toResponse(); } - List currentBookmarks = bookmarksDbManager.getBookmarksForUser(user.getId()); + List currentBookmarks = bookmarksManager.getBookmarksForUser(user.getId(), null); if (currentBookmarks.stream().noneMatch(b -> b.contentId().equals(contentId))) { return new SegueErrorResponse(Status.BAD_REQUEST, "You have not bookmarked this content.").toResponse(); } BookmarkDO bookmarkToRemove = new BookmarkDO(user.getId(), contentId, null, null); - bookmarksDbManager.removeBookmarkForUser(bookmarkToRemove); + bookmarksManager.removeBookmarkForUser(bookmarkToRemove); return Response.noContent().build(); } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java index 133c40cc94..e715402757 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -99,4 +99,45 @@ public List mapBookmarkListToContentSummaryList(final List getBookmarksForUser(final Long userId, final String contentType) { + return this.bookmarksDbManager.getBookmarksForUser(userId, contentType); + } + + /** + * Get a list of bookmarks associated with a user, augmented with content information + * + * @param userId the ID of the user to return bookmarks for. + * @param contentType the type of content to filter bookmarks by, or null to return all bookmarks. + * @return the bookmarks associated with the user, augmented with content information + */ + public List getAugmentedBookmarksForUser(final Long userId, final String contentType) { + List bookmarks = this.bookmarksDbManager.getBookmarksForUser(userId, contentType); + return this.mapBookmarkListToContentSummaryList(bookmarks); + } + + /** + * Save a bookmark to a user's account + * + * @param bookmark the bookmark to save + */ + public void addBookmarkForUser(final BookmarkDO bookmark) { + this.bookmarksDbManager.addBookmarkForUser(bookmark); + } + + /** + * Remove a bookmark from a user's account + * + * @param bookmark the bookmark to remove + */ + public void removeBookmarkForUser(final BookmarkDO bookmark) { + this.bookmarksDbManager.removeBookmarkForUser(bookmark); + } } diff --git a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java index 5f64c915b9..b3f7c0eebf 100644 --- a/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java +++ b/src/test/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacadeIT.java @@ -22,7 +22,7 @@ public class BookmarksFacadeIT extends IsaacIntegrationTest { @BeforeEach public void setUp() { - this.bookmarksFacade = new BookmarksFacade(properties, logManager, userAccountManager, contentManager, bookmarksManager, bookmarksDbManager); + this.bookmarksFacade = new BookmarksFacade(properties, logManager, userAccountManager, contentManager, bookmarksManager); } @AfterEach From 6ee4c85b5058904a16104e0f931cf29ec6ffe283 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 16:29:26 +0100 Subject: [PATCH 33/36] Remove redundant content type check --- src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java index ac80597ce9..f18ddf1747 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java @@ -45,10 +45,8 @@ public List getBookmarksForUser(final Long userId, final String cont boolean filterByContentType = false; if (null != contentType && !contentType.isEmpty()) { - if (contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage")) { - query += " AND content_type = ?"; - filterByContentType = true; - } + query += " AND content_type = ?"; + filterByContentType = true; } List bookmarks = new ArrayList<>(); From df85625b8add55724bc2d1c535a2469226d525e6 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 16:57:31 +0100 Subject: [PATCH 34/36] Fix date conversion --- src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java index f18ddf1747..1963110e11 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/dao/PgBookmarks.java @@ -82,7 +82,7 @@ public void addBookmarkForUser(final BookmarkDO bookmark) { pst.setLong(1, bookmark.userId()); pst.setString(2, bookmark.contentId()); pst.setString(3, bookmark.contentType()); - pst.setTimestamp(4, (Timestamp) bookmark.created()); + pst.setTimestamp(4, new Timestamp(bookmark.created().getTime())); if (pst.executeUpdate() == 0) { throw new SegueDatabaseException("Unable to save bookmark."); } From e412da63ab90cc16668d4ae88efdb02d4c0656e9 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 24 Apr 2026 17:11:49 +0100 Subject: [PATCH 35/36] Add logged events for adding/removing bookmarks --- .../uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java | 13 ++++++++++++- .../java/uk/ac/cam/cl/dtg/isaac/api/Constants.java | 6 ++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java index f6a7f2ee0a..5508cba239 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/BookmarksFacade.java @@ -1,5 +1,6 @@ package uk.ac.cam.cl.dtg.isaac.api; +import com.google.common.collect.ImmutableMap; import com.google.inject.Inject; import io.swagger.v3.oas.annotations.Operation; import io.swagger.v3.oas.annotations.tags.Tag; @@ -127,9 +128,10 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r return new SegueErrorResponse(Status.BAD_REQUEST, "You have already bookmarked this content.").toResponse(); } + String contentType; try { ContentDTO content = this.contentManager.getContentById(contentId); - String contentType = content.getType(); + contentType = content.getType(); if (null == contentType || !(contentType.equals("isaacQuestionPage") || contentType.equals("isaacConceptPage"))) { log.warn("Invalid content type provided for bookmarks query: {}", contentType); return new SegueErrorResponse(Status.BAD_REQUEST, "Only question and concept pages can be bookmarked!").toResponse(); @@ -143,6 +145,11 @@ public final Response addCurrentUserBookmark(@Context final HttpServletRequest r return new SegueErrorResponse(Status.NOT_FOUND, "Unable to find content to bookmark.").toResponse(); } + this.getLogManager().logEvent(user, request, IsaacServerLogType.ADD_BOOKMARK, + ImmutableMap.of(BOOKMARK_USER_ID_LOG_FIELDNAME, user.getId(), + BOOKMARK_CONTENT_ID_LOG_FIELDNAME, contentId, + BOOKMARK_CONTENT_TYPE_LOG_FIELDNAME, contentType)); + return Response.ok().build(); } @@ -179,6 +186,10 @@ public final Response removeCurrentUserBookmark(@Context final HttpServletReques BookmarkDO bookmarkToRemove = new BookmarkDO(user.getId(), contentId, null, null); bookmarksManager.removeBookmarkForUser(bookmarkToRemove); + this.getLogManager().logEvent(user, request, IsaacServerLogType.DELETE_BOOKMARK, + ImmutableMap.of(BOOKMARK_USER_ID_LOG_FIELDNAME, user.getId(), + BOOKMARK_CONTENT_ID_LOG_FIELDNAME, contentId)); + return Response.noContent().build(); } } diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java index 0d0e916254..8135e7aaae 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/Constants.java @@ -133,6 +133,10 @@ public enum GameboardState { public static final String PAGE_ID_LOG_FIELDNAME = "pageId"; public static final String FRAGMENT_ID_LOG_FIELDNAME = "pageFragmentId"; public static final String DOCUMENT_PATH_LOG_FIELDNAME = "path"; + public static final String BOOKMARK_USER_ID_LOG_FIELDNAME = "userId"; + public static final String BOOKMARK_CONTENT_ID_LOG_FIELDNAME = "contentId"; + public static final String BOOKMARK_CONTENT_TYPE_LOG_FIELDNAME = "contentType"; + public static final Long DEFAULT_MISUSE_STATISTICS_LIMIT = 5L; @@ -146,8 +150,10 @@ public enum GameboardState { */ public enum IsaacServerLogType implements LogType { ADD_BOARD_TO_PROFILE, + ADD_BOOKMARK, CREATE_GAMEBOARD, DELETE_ASSIGNMENT, + DELETE_BOOKMARK, DELETE_QUIZ_ASSIGNMENT, DELETE_BOARD_FROM_PROFILE, DOWNLOAD_ASSIGNMENT_PROGRESS_CSV, From c8ddd788550f1f4c839939e486c1fb2e75b5b5e6 Mon Sep 17 00:00:00 2001 From: Alex Lewin Date: Fri, 1 May 2026 15:17:46 +0100 Subject: [PATCH 36/36] Skip content augmentation if bookmarks list is empty --- .../isaac/api/managers/BookmarksManager.java | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java index e715402757..2eefd8b5fd 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/BookmarksManager.java @@ -63,15 +63,17 @@ public List augmentContentSummaryListWithBookmarkInformation( */ public List augmentContentSummaryListWithBookmarkInformation(final List bookmarks, final List contentSummaries) { - Map bookmarkMap = new HashMap<>(); - for (BookmarkDO bookmark : bookmarks) { - bookmarkMap.put(bookmark.contentId(), bookmark.created()); - } + if (!bookmarks.isEmpty()) { + Map bookmarkMap = new HashMap<>(); + for (BookmarkDO bookmark : bookmarks) { + bookmarkMap.put(bookmark.contentId(), bookmark.created()); + } - for (ContentSummaryDTO contentSummary : contentSummaries) { - Date created = bookmarkMap.get(contentSummary.getId()); - if (created != null) { - contentSummary.setBookmarked(created); + for (ContentSummaryDTO contentSummary : contentSummaries) { + Date created = bookmarkMap.get(contentSummary.getId()); + if (created != null) { + contentSummary.setBookmarked(created); + } } } return contentSummaries;