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

ADJUST1-530 unused deductions banner make reason specific #259

Merged

Conversation

pavilionsahota
Copy link
Contributor

No description provided.

@pavilionsahota pavilionsahota requested a review from a team as a code owner March 26, 2024 15:50
@ldlharper
Copy link
Contributor

I don't think 'NONE' should be returned whenever it was false.

}
await delay(this.waitBetweenTries)
adjustments = await this.adjustmentsService.findByPersonOutsideSentenceEnvelope(nomsId, token)
// Try again
} else {
// Unable to calculate unused deductions.
return false
return 'NONE'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where the calculated unused deductions don't match whats in the database. There isn't really a message for this.

}
}
} catch {
// Error couldn't calculate unused deductions.
}
return false
return 'NONE'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is where there was an exception somewhere. Not sure it should be 'NONE'

@ldlharper
Copy link
Contributor

I think in the couple of examples where we shouldn't return NONE we should default to the old message.

try {
let adjustments = await this.adjustmentsService.findByPersonOutsideSentenceEnvelope(nomsId, token)

const deductions = adjustments.filter(it => it.adjustmentType === 'REMAND' || it.adjustmentType === 'TAGGED_BAIL')
if (!deductions.length) {
// If there are no deductions then unused deductions doesn't need to be calculated
return true
return 'NONE'
}
if (this.anyDeductionFromNomis(deductions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was meant to move to after the validation check

try {
const deductions = adjustments.filter(it => it.adjustmentType === 'REMAND' || it.adjustmentType === 'TAGGED_BAIL')
if (!deductions.length) {
// If there are no deductions then unused deductions doesn't need to be calculated
return true
return 'NONE'
}
if (this.anyDeductionFromNomis(deductions)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This was meant to move to after the validation check

}
const calculatedUnusedDeducions = unusedDeductionsResponse.unusedDeductions

const deductions = adjustments.filter(it => it.adjustmentType === 'REMAND' || it.adjustmentType === 'TAGGED_BAIL')
if (!deductions.length) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This bit didn't need to move. Just the anyDeductionFromNomis condition. Otherwise looks good.

Copy link
Contributor

@ldlharper ldlharper left a comment

Choose a reason for hiding this comment

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

LGTM

@ldlharper ldlharper enabled auto-merge (squash) March 27, 2024 14:22
@ldlharper ldlharper merged commit 082dc77 into main Mar 27, 2024
3 of 4 checks passed
@ldlharper ldlharper deleted the ADJUST1-530-Unused-deductions-banner-Make-reason-specific branch March 27, 2024 14:25
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