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-1857 Copied cas1 timeline api types to cas1 schema. #2902

Merged
merged 1 commit into from
Feb 3, 2025

Conversation

vbala-moj
Copy link
Contributor

@vbala-moj vbala-moj commented Jan 31, 2025

@vbala-moj vbala-moj marked this pull request as ready for review January 31, 2025 09:28
@davidatkinsuk
Copy link
Contributor

Can you please add something into the commit message explaining what this commit is doing?

@vbala-moj
Copy link
Contributor Author

Can you please add something into the commit message explaining what this commit is doing?

It's there. may be some line break hides those.

Copy link
Contributor

@davidatkinsuk davidatkinsuk left a comment

Choose a reason for hiding this comment

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

This is looking good, i'll do a thorough review once my initial comments are addressed as i think they'll reduce the amount of code to review 👍

@davidatkinsuk
Copy link
Contributor

Can you please add something into the commit message explaining what this commit is doing?

It's there. may be some line break hides those.

Apologies, i'm being a dummy and misread the github UI :)

@davidatkinsuk
Copy link
Contributor

A minor point and don't worry about addressing on this PR. We tend to prefix our branch names with either feature/ or chore/. In this case it would be feature/APS-1857-copy-timeline-api-types-to-cas1

@vbala-moj vbala-moj force-pushed the APS-1857-copy-timeline-api-types-to-cas1 branch 2 times, most recently from 2f73f74 to d279d01 Compare January 31, 2025 11:35
),
APPROVED_PREMISES_BOOKING_MADE(
DomainEventCas.CAS1,
Cas1EventType.bookingMade.value,
"An Approved Premises booking has been made",
TimelineEventType.approvedPremisesBookingMade,
Cas1TimelineEventType.bookingNotMade,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this should be bookingMade?

),
APPROVED_PREMISES_PLACEMENT_APPLICATION_ALLOCATED(
DomainEventCas.CAS1,
Cas1EventType.placementApplicationAllocated.value,
"An Approved Premises Request for Placement has been allocated",
TimelineEventType.approvedPremisesPlacementApplicationAllocated,
Cas1TimelineEventType.assessmentAllocated,
Copy link
Contributor

Choose a reason for hiding this comment

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

i think this should be placementApplicationAllocated?

@@ -42,4 +43,22 @@ enum class ApprovedPremisesApplicationStatus(val apiValue: ApiApprovedPremisesAp
fun valueOf(apiValue: ApiApprovedPremisesApplicationStatus): ApprovedPremisesApplicationStatus =
ApprovedPremisesApplicationStatus.entries.first { it.apiValue == apiValue }
}

fun toCas1Status(): Cas1ApplicationStatus {
return when (this) {
Copy link
Contributor

Choose a reason for hiding this comment

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

i like using the when here, it makes sure we've covered all mappings 👍

createdBy:
$ref: '_shared.yml#/components/schemas/User'
payload:
$ref: '_shared.yml#/components/schemas/TimelineEventContentPayload'
Copy link
Contributor

@davidatkinsuk davidatkinsuk Jan 31, 2025

Choose a reason for hiding this comment

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

i guess this should be copied into the CAS1 schemas too?

to be honest we could probably remove it from the shared.yml (and shared API) completely because the UI won't be using it yet. Hopefully that makes it easier?

@@ -1004,3 +1004,129 @@ components:
- updatedReferenceNumber
- updatedReason
- updatedNotes
Cas1TimelineEvent:
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we can make a few fields mandatory (i appreciate they weren't before)

  • type
  • id
  • occurredAt

- booking
- assessment
- assessmentAppeal
- cas1SpaceBooking
Copy link
Contributor

Choose a reason for hiding this comment

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

this can just be spaceBooking in this context

Copy link
Contributor

@davidatkinsuk davidatkinsuk left a comment

Choose a reason for hiding this comment

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

This is looking really good, just a few comments from me

@vbala-moj vbala-moj force-pushed the APS-1857-copy-timeline-api-types-to-cas1 branch 2 times, most recently from ebd633b to bcc0aff Compare February 3, 2025 08:44
Refactored services and transformers to return cas1 api types.
@vbala-moj vbala-moj force-pushed the APS-1857-copy-timeline-api-types-to-cas1 branch from bcc0aff to 6e98f66 Compare February 3, 2025 13:51
@vbala-moj vbala-moj merged commit a37f4c1 into main Feb 3, 2025
8 checks passed
@vbala-moj vbala-moj deleted the APS-1857-copy-timeline-api-types-to-cas1 branch February 3, 2025 15:22
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