Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,15 @@
import lombok.RequiredArgsConstructor;
import lombok.extern.log4j.Log4j2;
import org.springdoc.core.converters.models.PageableAsQueryParam;
import org.springframework.data.domain.PageRequest;
import org.springframework.data.domain.Pageable;
import org.springframework.data.domain.Sort;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;

import java.util.Map;

import java.util.UUID;

@Log4j2
Expand Down Expand Up @@ -120,13 +124,22 @@ public ResponseEntity<OutstandingExportCountDTO> getRemainingExportsCount(@PathV
@PageableAsQueryParam
public ResponseEntity<ExportedSubmissionsListDto> getSubmissions(@PathVariable final UUID exportId,
@RequestParam(name = "grabOnlyFailed", required = false,
defaultValue = "false") final boolean grabOnlyFailed, final Pageable pagination) {
defaultValue = "false") final boolean grabOnlyFailed,
@RequestParam(name = "sortField", required = false, defaultValue = "submittedDate") final String sortField,
@RequestParam(name = "sortDir", required = false, defaultValue = "asc") final String sortDir,
final Pageable pagination) {
log.info("Getting submissions for exportId: {}, grabOnlyFailed : {}", exportId, grabOnlyFailed);

final Map<String, String> allowedSortFields = Map.of("gapId", "s.gapId", "submittedDate", "s.submittedDate");
final String jpqlSortField = allowedSortFields.getOrDefault(sortField, "s.submittedDate");
final Sort.Direction direction = Sort.Direction.fromOptionalString(sortDir).orElse(Sort.Direction.ASC);
final Pageable sortedPagination = PageRequest.of(pagination.getPageNumber(), pagination.getPageSize(),
Sort.by(direction, jpqlSortField));

final GrantExportStatus status = grabOnlyFailed ? GrantExportStatus.FAILED : GrantExportStatus.COMPLETE;
final String superZipLocation = grantExportBatchService.getSuperZipLocation(exportId);
final ExportedSubmissionsListDto exportedSubmissionsListDto = exportService
.generateExportedSubmissionsListDto(exportId, status, pagination, superZipLocation);
.generateExportedSubmissionsListDto(exportId, status, sortedPagination, superZipLocation);

return ResponseEntity.ok()
.header("cache-control", "private, no-cache, max-age=0, must-revalidate")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
@Builder
public class ExportedSubmissionsDto {

private String gapId;
private String name;
private String zipFileLocation;
private UUID submissionId;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ public ExportedSubmissionsDto grantExportEntityToExportedSubmissions(GrantExport
exportedSubmissionsDto.name(mapExportedSubmissionName(grantExportEntity));
exportedSubmissionsDto.submittedDate(mapExportedSubmissionSubmittedDate(grantExportEntity));
exportedSubmissionsDto.submissionName(mapExportedSubmissionSubmissionName(grantExportEntity));
exportedSubmissionsDto.gapId(mapExportedSubmissionGapId(grantExportEntity));

return exportedSubmissionsDto.build();
}
Expand Down Expand Up @@ -111,6 +112,19 @@ public String mapExportedSubmissionSubmissionName(GrantExportEntity grantExportE
return submission.get().getSubmissionName();
}

@Override
public String mapExportedSubmissionGapId(GrantExportEntity grantExportEntity) {
log.info("Getting gap ID from grant export {} and submission {}", grantExportEntity.getId(),
grantExportEntity.getId().getSubmissionId());
final UUID submissionId = grantExportEntity.getId().getSubmissionId();
final Optional<Submission> submission = submissionRepository.findById(submissionId);
if (submission.isEmpty()) {
log.error("Submission not found for id: {}", submissionId);
return null;
}
return submission.get().getGapId();
}

private UUID grantExportEntityIdSubmissionId(GrantExportEntity grantExportEntity) {
if (grantExportEntity == null) {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ public interface GrantExportMapper {
@Mapping(target ="status", source = "status")
@Mapping(target = "submittedDate", expression = "java(mapExportedSubmissionSubmittedDate(grantExportEntity))")
@Mapping(target = "submissionName", expression = "java(mapExportedSubmissionSubmissionName(grantExportEntity))")
@Mapping(target = "gapId", expression = "java(mapExportedSubmissionGapId(grantExportEntity))")
ExportedSubmissionsDto grantExportEntityToExportedSubmissions(GrantExportEntity grantExportEntity);

default String mapExportedSubmissionName(GrantExportEntity grantExportEntity) {
Expand All @@ -36,4 +37,8 @@ default String mapExportedSubmissionSubmissionName(GrantExportEntity grantExport
return "";
}

default String mapExportedSubmissionGapId(GrantExportEntity grantExportEntity) {
return "";
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -49,4 +49,9 @@ void updateExportRecordLocation(@Param("submissionId") UUID submissionId,
List<GrantExportEntity> findByCreatedByAndId_ExportBatchIdAndStatus(Integer createdBy, UUID exportBatchId,
GrantExportStatus status, Pageable pageable);

@Query("SELECT ge FROM GrantExportEntity ge JOIN Submission s ON s.id = ge.id.submissionId "
+ "WHERE ge.createdBy = :createdBy AND ge.id.exportBatchId = :exportBatchId AND ge.status = :status")
List<GrantExportEntity> findByCreatedByAndExportBatchIdAndStatusSorted(@Param("createdBy") Integer createdBy,
Comment on lines +52 to +54

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findByCreatedByAndExportBatchIdAndStatusSorted name implies sorting but the JPQL query lacks explicit ordering and duplicates an existing method; clarify purpose/rename or document expected sorting behavior.

Suggested change
@Query("SELECT ge FROM GrantExportEntity ge JOIN Submission s ON s.id = ge.id.submissionId "
+ "WHERE ge.createdBy = :createdBy AND ge.id.exportBatchId = :exportBatchId AND ge.status = :status")
List<GrantExportEntity> findByCreatedByAndExportBatchIdAndStatusSorted(@Param("createdBy") Integer createdBy,
@Query("SELECT ge FROM GrantExportEntity ge JOIN Submission s ON s.id = ge.id.submissionId "
List<GrantExportEntity> findByCreatedByAndExportBatchIdAndStatusJoinedWithSubmission(@Param("createdBy") Integer createdBy,
Details

✨ AI Reasoning
​A new repository method was added whose name implies it provides a sorted result, but the JPQL query does not include an ORDER BY and relies on a Pageable for sorting. There is already a similarly named derived query method. The combination makes the method's distinct purpose unclear to readers.

Reply @AikidoSec feedback: [FEEDBACK] to get better review comments in the future.
Reply @AikidoSec ignore: [REASON] to ignore this issue.
More info

@Param("exportBatchId") UUID exportBatchId, @Param("status") GrantExportStatus status, Pageable pageable);

}
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package gov.cabinetoffice.gap.adminbackend.services;

import gov.cabinetoffice.gap.adminbackend.dtos.grantExport.ExportedSubmissionsDto;
import gov.cabinetoffice.gap.adminbackend.dtos.grantExport.ExportedSubmissionsListDto;
import gov.cabinetoffice.gap.adminbackend.dtos.grantExport.GrantExportDTO;
import gov.cabinetoffice.gap.adminbackend.dtos.grantExport.GrantExportListDTO;
Expand All @@ -20,7 +19,6 @@
import java.util.List;
import java.util.UUID;

import static java.util.Comparator.comparing;

@Service
@RequiredArgsConstructor
Expand Down Expand Up @@ -62,7 +60,7 @@ public ExportedSubmissionsListDto generateExportedSubmissionsListDto(UUID export
final AdminSession adminSession = HelperUtils.getAdminSessionForAuthenticatedUser();
log.info("Grabbing list of grant-export with id {} and status {}", exportId,status.toString());

final List<GrantExportEntity> grantExports = exportRepository.findByCreatedByAndId_ExportBatchIdAndStatus(adminSession.getGrantAdminId(),
final List<GrantExportEntity> grantExports = exportRepository.findByCreatedByAndExportBatchIdAndStatusSorted(adminSession.getGrantAdminId(),
exportId, status, pagination);
log.info("Found {} grant-exports", grantExports.size());

Expand All @@ -74,7 +72,6 @@ public ExportedSubmissionsListDto generateExportedSubmissionsListDto(UUID export
.superZipFileLocation(superZipLocation)
.exportedSubmissions(grantExports.stream()
.map(customGrantExportMapper::grantExportEntityToExportedSubmissions)
.sorted(comparing(ExportedSubmissionsDto::getSubmittedDate))
.toList())
.build();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,7 +227,7 @@ void generateExportedSubmissionsListDto() {
.submissionName("Application Name 1")
.build();

when(exportRepository.findByCreatedByAndId_ExportBatchIdAndStatus(SEC_CONTEXT_ADMIN_ID,mockExportId, GrantExportStatus.COMPLETE, pagination))
when(exportRepository.findByCreatedByAndExportBatchIdAndStatusSorted(SEC_CONTEXT_ADMIN_ID,mockExportId, GrantExportStatus.COMPLETE, pagination))
.thenReturn(List.of(grantExport, grantExport2));
when(exportRepository.countByIdExportBatchIdAndStatus(mockExportId, GrantExportStatus.COMPLETE))
.thenReturn(2L);
Expand All @@ -242,12 +242,12 @@ void generateExportedSubmissionsListDto() {
final ExportedSubmissionsListDto response = grantExportService
.generateExportedSubmissionsListDto(mockExportId, GrantExportStatus.COMPLETE, pagination, "superZip");

verify(exportRepository).findByCreatedByAndId_ExportBatchIdAndStatus(SEC_CONTEXT_ADMIN_ID,mockExportId, GrantExportStatus.COMPLETE, pagination);
verify(exportRepository).findByCreatedByAndExportBatchIdAndStatusSorted(SEC_CONTEXT_ADMIN_ID,mockExportId, GrantExportStatus.COMPLETE, pagination);
assertThat(response.getGrantExportId()).isEqualTo(mockExportId);
assertThat(response.getExportedSubmissions().get(0))
.isEqualTo(exportedSubmissionsDto2);
.isEqualTo(exportedSubmissionsDto);
assertThat(response.getExportedSubmissions().get(1))
.isEqualTo(exportedSubmissionsDto);
.isEqualTo(exportedSubmissionsDto2);
assertThat(response.getSuperZipFileLocation()).isEqualTo("superZip");
assertThat(response.getSuccessCount()).isEqualTo(2);
assertThat(response.getFailedCount()).isEqualTo(0);
Expand Down
Loading