Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

APS-1752 - Add get person info in batches function #2765

Merged

Conversation

davidatkinsuk
Copy link
Contributor

@davidatkinsuk davidatkinsuk commented Jan 6, 2025

Add a function to get offender info in batches and update Cas3ReportService to use this function for batching requests.

This is being added as batching functionality will be required for some new CAS1 seed jobs

@davidatkinsuk davidatkinsuk force-pushed the feature/aps-1752-add-common-bulk-get-offenders-function branch 3 times, most recently from a4971ff to a1a1083 Compare January 6, 2025 15:59
@davidatkinsuk davidatkinsuk changed the title Add common get bulk offenders function APS-1752 - Add common get bulk offenders function Jan 6, 2025
@davidatkinsuk davidatkinsuk marked this pull request as ready for review January 6, 2025 16:00
@davidatkinsuk davidatkinsuk changed the title APS-1752 - Add common get bulk offenders function APS-1752 - Add get person info in batches function Jan 6, 2025
@davidatkinsuk davidatkinsuk force-pushed the feature/aps-1752-add-common-bulk-get-offenders-function branch from a1a1083 to 2768e0f Compare January 6, 2025 16:13
Copy link
Contributor

@muhammad-elabdulla muhammad-elabdulla left a comment

Choose a reason for hiding this comment

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

LGTM except the comment in the test

@@ -156,42 +156,6 @@ class Cas3ReportServiceTest {
verify(exactly = 0) { mockOffenderService.getPersonSummaryInfoResults(any<Set<String>>(), any()) }
}

@Test
fun `createCas3ApplicationReferralsReport successfully generate report and call offender service 2 times when crn search limit exceed`() {
val startDate = LocalDate.of(2024, 1, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should remain as it make sure that if the numbers of Crns exceed the bath size is called multiple times

Copy link
Contributor Author

@davidatkinsuk davidatkinsuk Jan 7, 2025

Choose a reason for hiding this comment

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

i've updated the code so that batching is dealt with in the OffenderService, so i think this test is now redundant? unless i'm missing another call that's made multiple times?

Copy link
Contributor

@muhammad-elabdulla muhammad-elabdulla Jan 7, 2025

Choose a reason for hiding this comment

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

In case there are 2000 crns the new code should split them to batches of 500. which means should call the function multiple times

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is what we're now doing in OffenderService.getPersonSummaryInfoResultsInBatches and i've added unit tests to ensure this happens (albeit with a batch size of 400). Given this, there is no more batching/partitioning happening in the Cas3ReportService, so we don't need to test this specific behaviour in Cas3ReportServiceTest, hence the removal of this test function. Given me a shout on slack if you think something needs changing here, i may be misunderstanding.

Note that the check in the new code relates to the requested batch size, not the number of crns requested

    if (batchSize > MAX_OFFENDER_REQUEST_COUNT) {
      throw InternalServerErrorProblem("Cannot request more than $MAX_OFFENDER_REQUEST_COUNT CRNs. A batch size of $batchSize has been requested.")
    }

@davidatkinsuk davidatkinsuk force-pushed the feature/aps-1752-add-common-bulk-get-offenders-function branch from 2768e0f to ba3e4b4 Compare January 7, 2025 09:05
This commit updates the `Cas3ReportService` to use the new `OffenderService.getPersonSummaryInfoResultsInBatches`, removing the local batching implementations.
@davidatkinsuk davidatkinsuk force-pushed the feature/aps-1752-add-common-bulk-get-offenders-function branch from ba3e4b4 to 81eb267 Compare January 7, 2025 14:06
@davidatkinsuk davidatkinsuk merged commit 70d664c into main Jan 7, 2025
7 checks passed
@davidatkinsuk davidatkinsuk deleted the feature/aps-1752-add-common-bulk-get-offenders-function branch January 7, 2025 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants