Conversation
Arnabsubedi233
requested review from
ChrisGarratt122,
iamfrankiemoran,
louisbriggs and
max-holland
iamfrankiemoran
left a comment
iamfrankiemoran
left a comment
There was a problem hiding this comment.
Some changes, but talk on Tuesday about it
| @Input() hasAccountMaintenancePermission: boolean = false; | ||
| @Input() hasEnterEnforcementPermission: boolean = false; | ||
| @Output() addEnforcementOverride = new EventEmitter<void>(); | ||
| @Output() changeEnforcementCourt = new EventEmitter<number | null>(); |
There was a problem hiding this comment.
Not sure if the null should be here
| public handleChangeEnforcementCourt(): void { | ||
| if (this.hasAccountMaintenancePermission) { | ||
| this.changeEnforcementCourt.emit(this.tabData.enforcement_overview.enforcement_court?.court_id ?? null); | ||
| } | ||
| } |
There was a problem hiding this comment.
Would the link not be wrapped around this permission to prevent users who didn't have this permission to click a link they can't actually click on. Also, I don't think the link should show if the court_id was null. We could put an if statement in the HTML which looks for the permission and the court_id and pass that into the handleChangeEnforcementCourt() method as a parameter
| summaryListId="enforcementOverviewDetails" | ||
| summaryListRowId="enforcement_court" | ||
| [actionEnabled]="hasAccountMaintenancePermission ? true : false" | ||
| (actionClick)="handleChangeEnforcementCourt()" |
There was a problem hiding this comment.
Could pass in tabData.enforcement_overview.enforcement_court.court_id here
| <app-fines-not-provided></app-fines-not-provided> | ||
| } | ||
| </ng-container> | ||
| @if (hasAccountMaintenancePermission) { |
There was a problem hiding this comment.
The change link is already wrapped around a permission so no need to add a further check in the typescript
| * | ||
| * @param currentEnforcementCourtId The current enforcement court id shown on the enforcement tab. | ||
| */ | ||
| public navigateToChangeEnforcementCourtPage(currentEnforcementCourtId: number | null): void { |
There was a problem hiding this comment.
I'm not sure about passing in the enforcement court id here. This would be cached data so we could just grab that on the component, this is being treated as throwaway data
There was a problem hiding this comment.
Could we not use the resolver here?
There was a problem hiding this comment.
This exports a type, but should export an interface also you are missing the following:
export interface IFinesAccEnfCourtChangeForm extends IAbstractFormBaseForm<IFinesAccEnfCourtChangeFormState> {
formData: IFinesAccEnfCourtChangeFormState;
}
| private readonly currentEnforcementCourtId = this.navigationRouter.currentNavigation()?.extras.state?.[ | ||
| 'currentEnforcementCourtId' | ||
| ] as number | null | undefined; |
There was a problem hiding this comment.
Not sure about this as we should be able to get it from cache
There was a problem hiding this comment.
This already exists within the Fines Mac journey. Probably need to bring it up a level
2 participants
Jira link
See https://tools.hmcts.net/jira/browse/PO-1849
Change description
Testing done
Security Vulnerability Assessment
CVE Suppression: Are there any CVEs present in the codebase (either newly introduced or pre-existing) that are being intentionally suppressed or ignored by this commit?
Checklist