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 24ee0b91c..9bbd2ddae 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 @@ -73,7 +73,13 @@ public enum GameboardItemState { } public enum CompletionState { - ALL_CORRECT, IN_PROGRESS, NOT_ATTEMPTED + ALL_CORRECT, IN_PROGRESS, NOT_ATTEMPTED; + + private static final Set allStates = Set.of(CompletionState.values()); + + public static Set getAllStates() { + return allStates; + } } public enum QuestionPartState { 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 c3e60d045..24478b34f 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 @@ -388,28 +388,31 @@ public final Response getQuestionList(@Context final Request request, } try { - if (null == statuses || Objects.equals(statuses, "")) { - // If no statuses apply assume all statuses - filterByStatuses = Set.of(CompletionState.values()); + if (null == statuses) { + // If statuses isn't a URL param, we won't augment or filter! + filterByStatuses = null; + } else if (statuses.isEmpty()) { + // If statuses is blank, that shouldn't mean "please match nothing at all"; make it match everything. + filterByStatuses = CompletionState.getAllStates(); } else { filterByStatuses = Arrays.stream(statuses.split(",")) .map(CompletionState::valueOf) .collect(Collectors.toSet()); } } catch (IllegalArgumentException e) { - return new SegueErrorResponse( - Status.BAD_REQUEST, "Invalid question statuses to filter by provided.", e - ).toResponse(); + return new SegueErrorResponse(Status.BAD_REQUEST, "Invalid question status filter provided.").toResponse(); } String validatedSearchString = searchString.isBlank() ? null : searchString; - // Show content tagged as "nofilter" if the user is staff - boolean showNoFilterContent; + // Show content tagged as "nofilter" if the user is staff: + boolean showNoFilterContent = false; try { - showNoFilterContent = isUserStaff(userManager, httpServletRequest); + if (user instanceof RegisteredUserDTO) { + showNoFilterContent = isUserStaff(userManager, (RegisteredUserDTO) user); + } } catch (NoUserLoggedInException e) { - showNoFilterContent = false; + // This cannot happen! } List combinedResults = new ArrayList<>(); @@ -440,13 +443,15 @@ public final Response getQuestionList(@Context final Request request, List unfilteredSummarizedResults = new ArrayList<>(summarizedResults); - if (user instanceof RegisteredUserDTO) { - summarizedResults = userAttemptManager.augmentContentSummaryListWithAttemptInformation( - (RegisteredUserDTO) user, summarizedResults - ); - summarizedResults = summarizedResults.stream() - .filter(q -> filterByStatuses.contains(q.getState())) - .collect(Collectors.toList()); + if (null != filterByStatuses) { + // Only augment when filtering by statuses: + summarizedResults = userAttemptManager.augmentContentSummaryListWithAttemptInformation(user, summarizedResults); + // Optimise out unnecessary filtering: + if (!filterByStatuses.equals(CompletionState.getAllStates())) { + summarizedResults = summarizedResults.stream() + .filter(q -> filterByStatuses.contains(q.getState())) + .collect(Collectors.toList()); + } } if (limit < 0 || combinedResults.size() + summarizedResults.size() <= limit) { diff --git a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/UserAttemptManager.java b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/UserAttemptManager.java index 9e683c53e..a45a0afbc 100644 --- a/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/UserAttemptManager.java +++ b/src/main/java/uk/ac/cam/cl/dtg/isaac/api/managers/UserAttemptManager.java @@ -6,6 +6,7 @@ import uk.ac.cam.cl.dtg.isaac.dto.content.ContentBaseDTO; 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.AbstractSegueUserDTO; import uk.ac.cam.cl.dtg.isaac.dto.users.RegisteredUserDTO; import uk.ac.cam.cl.dtg.segue.api.managers.QuestionManager; import uk.ac.cam.cl.dtg.segue.dao.SegueDatabaseException; @@ -99,12 +100,20 @@ private void augmentContentSummaryWithAttemptInformation( * @return the augmented content summary list. * @throws SegueDatabaseException if there is an error retrieving question attempts. */ - public List augmentContentSummaryListWithAttemptInformation(RegisteredUserDTO user, List summarizedResults) throws SegueDatabaseException { - List questionPageIds = summarizedResults.stream().map(ContentSummaryDTO::getId).collect(Collectors.toList()); + public List augmentContentSummaryListWithAttemptInformation(AbstractSegueUserDTO user, List summarizedResults) throws SegueDatabaseException { - Map>> questionAttempts = - questionManager.getMatchingLightweightQuestionAttempts(Collections.singletonList(user), questionPageIds) - .getOrDefault(user.getId(), Collections.emptyMap()); + Map>> questionAttempts; + + if (user instanceof RegisteredUserDTO) { + // Load only relevant attempts: + RegisteredUserDTO registeredUser = (RegisteredUserDTO) user; + List questionPageIds = summarizedResults.stream().map(ContentSummaryDTO::getId).collect(Collectors.toList()); + questionAttempts = questionManager.getMatchingLightweightQuestionAttempts(Collections.singletonList(registeredUser), questionPageIds) + .getOrDefault(registeredUser.getId(), Collections.emptyMap()); + } else { + // For anon users, all attempts are in one place so just load all: + questionAttempts = questionManager.getQuestionAttemptsByUser(user); + } for (ContentSummaryDTO result : summarizedResults) { augmentContentSummaryWithAttemptInformation(result, questionAttempts);