Cas2v2/cba87 add application origin to the cas2v2 endpoint #2798
Conversation
| @@ -35,6 +35,7 @@ CREATE TABLE cas_2_v2_applications | |||
| hdc_eligibility_date date, | |||
| conditional_release_date date, | |||
| telephone_number TEXT, | |||
| application_origin TEXT, | |||
There was a problem hiding this comment.
we generally shouldn't modify existing migrations. This change should be made in a new migration
| @@ -35,6 +35,7 @@ CREATE TABLE cas_2_v2_applications | |||
| hdc_eligibility_date date, | |||
| conditional_release_date date, | |||
| telephone_number TEXT, | |||
| application_origin TEXT, | |||
There was a problem hiding this comment.
i think this should be not null?
| @@ -45,6 +46,7 @@ CREATE TABLE cas_2_v2_assessments | |||
| created_at TIMESTAMPTZ, | |||
| nacro_referral_id TEXT, | |||
| assessor_name TEXT, | |||
| applicationOrigin TEXT, | |||
There was a problem hiding this comment.
given this can be derived from the linked application, is this de-normalisation required
| @@ -45,6 +46,7 @@ CREATE TABLE cas_2_v2_assessments | |||
| created_at TIMESTAMPTZ, | |||
| nacro_referral_id TEXT, | |||
| assessor_name TEXT, | |||
| applicationOrigin TEXT, | |||
There was a problem hiding this comment.
convention is to underscores in database column names
There was a problem hiding this comment.
It's now removed - see above comment.
| @@ -16,4 +16,5 @@ data class DomainEvent<T>( | |||
| val data: T, | |||
| val metadata: Map<MetaDataName, String?> = emptyMap(), | |||
| val schemaVersion: Int? = null, | |||
| val applicationOrigin: String? = null, | |||
There was a problem hiding this comment.
Is this absolutely required given it can be derived from the linked application? If possible we should minimise CAS specific properties on shared types
| } | ||
|
|
||
| // hdc application | ||
| val hdcApplicationEntity = cas2v2ApplicationEntityFactory.produceAndPersist { |
There was a problem hiding this comment.
i don't think the three comments above are required
| objectMapper.readValue(rawResponseBody, object : TypeReference<List<Cas2v2ApplicationSummary>>() {}) | ||
|
|
||
| // check application origin is persisted and returned correctly | ||
| Assertions.assertThat(responseBody).anyMatch { |
There was a problem hiding this comment.
i don't think the comment is required
| @@ -402,6 +403,80 @@ class Cas2v2ApplicationTest : Cas2v2IntegrationTestBase() { | |||
| } | |||
| } | |||
|
|
|||
| @Test | |||
| fun `Get all applications returns 200 with correct body including applicationOrigin`() { | |||
There was a problem hiding this comment.
this test is only checking application origin, maybe worth renaming to reflect that (e.g. remove 'correct body including')
| @@ -100,6 +100,7 @@ class Cas2v2ApplicationsTransformerTest { | |||
| "telephoneNumber", | |||
| "assessment", | |||
| "timelineEvents", | |||
| "applicationOrigin", | |||
There was a problem hiding this comment.
i know you're just following what's already here but this isn't really testing the transform (it should really look at the value passed in is correctly returned). Same comment applies to other transformer test
There was a problem hiding this comment.
Good point. We have added an extra assert that verifies the applicationOrigin is transformed.
| @@ -2193,6 +2200,8 @@ components: | |||
| offenceId: | |||
| type: string | |||
| example: "M1502750438" | |||
| applicationOrigin: | |||
| $ref: "#/components/schemas/ApplicationOrigin" | |||
There was a problem hiding this comment.
I guess this should be mandatory for CAS2, but because this is a shared type that will break CAS1 and CAS3. I think this is good opportunity to introduce a CAS2 specific NewApplication type (e.g. Cas2NewApplication) and put it in the cas2 schemas yml, including this as a mandatory field. You may find that you can remove the other fields if they aren't required for CAS2. This type should then be used on your new cas2v2 endpoint. If you do this i don't think the PR should be updating _shared.yml at all
…-the-Cas2v2-endpoint
…int' of github.com:ministryofjustice/hmpps-approved-premises-api into cas2v2/cba87-Add-application-origin-to-the-Cas2v2-endpoint
No description provided.