-
Notifications
You must be signed in to change notification settings - Fork 0
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
Adjust1-101 view tagged bail adjustment #150
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few fairly minor comments. Looks really good though.
server/@types/adjustments/index.d.ts
Outdated
@@ -215,7 +215,7 @@ export interface components { | |||
*/ | |||
prisonId?: string | |||
/** | |||
* @description The name name of the prison where the prisoner was located at the time the adjustment was created | |||
* @description The name of the prison where the prisoner was located at the time the adjustment was created |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files are generated by the scripts in /scripts
. The scripts generate these files from the backend API docs.
Not a big deal, just an FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've undone that comment change and have adjusted the comment in the Dto instead 👍
server/model/taggedBailViewModel.ts
Outdated
} | ||
|
||
private getCourtName(caseSequence: number): string { | ||
const sentenceAndOffence = this.sentencesAndOffences.find(it => it.caseSequence === caseSequence) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So there can be multiple sentences on the same court case (Will have the same case sequence).
Have a look at how we select the sentence by sentence date in the add journey.
https://github.com/ministryofjustice/hmpps-adjustments/blob/main/server/model/taggedBailSelectCaseModel.ts#L32
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved some logic out of the taggedBailSelectCaseModel.ts
into the utils.ts
so that they can be used by the taggedBailViewModel.ts
too. This way they're both using the same logic 👍
server/routes/taggedBailRoutes.ts
Outdated
const prisonerDetail = await this.prisonerService.getPrisonerDetail(nomsId, caseloads, token) | ||
const adjustments = await this.adjustmentsService.findByPerson(nomsId, token) | ||
const taggedBailAdjustments = adjustments.filter(it => it.adjustmentType === 'TAGGED_BAIL') | ||
if (!taggedBailAdjustments) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
!taggedBailAdjustments.length
If there are no tagged bail adjustments the filter will return an empty array which is truthy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've corrected this in the way you have suggested 👍
server/routes/taggedBailRoutes.ts
Outdated
return res.redirect(`/${nomsId}`) | ||
} | ||
|
||
const sentencesAndOffences = await this.prisonerService.getSentencesAndOffencesFilteredForRemand( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
use getSentencesAndOffences
over getSentencesAndOffencesFilteredForRemand
.
The function you use filters out sentences that aren't applicable for remand (unrelated to tagged bail.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've changed this method call in the way you have suggested 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
No description provided.