-
Notifications
You must be signed in to change notification settings - Fork 5
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
feat(cb2-9882): Add ability to produce a rejection letter for TRLs on provisional tech records #1350
Conversation
const currentTechRecord = this.techRecord?.techRecord_statusCode === StatusCodes.CURRENT; | ||
|
||
return this.correctApprovalType && currentTechRecord && !this.isEditing; | ||
const currentTechRecord = this.techRecord?.techRecord_statusCode === StatusCodes.CURRENT |
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.
This shouldn't be called currentTechRecord
if it could be provisional too
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.
Yes, I will correct this line and give it a proper name.
src/app/forms/custom-sections/letters/letters.component.spec.ts
Outdated
Show resolved
Hide resolved
get checkForCurrentRecordInHistory(): boolean { | ||
let hasCurrent = false; | ||
this.techRecordService.techRecordHistory$.subscribe((historyArray: TechRecordSearchSchema[] | undefined) => { | ||
historyArray?.forEach((history: TechRecordSearchSchema) => { | ||
if (history.techRecord_statusCode === StatusCodes.CURRENT | ||
&& this.techRecord?.techRecord_statusCode === StatusCodes.PROVISIONAL) hasCurrent = true; | ||
}); | ||
}); | ||
return hasCurrent; | ||
} |
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.
All the code in this PR works functionally, and I'm happy with it, but this bit causes a memory leak (can show you how if needed). Not sure what standard practice in VTM is @shivangidas but this looks like it could be its own selector on the tech record history service that you:
a. Subscribe to on ngOnInit
b. Have a takeUntil destroyed pipe, so it stops listening after this component is destroyed (stopping the memory leak)
c. assign as a member of the class so it doesn't create a new subscription on every component re-render, as it currently does using the getter approach.
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.
Currently, there's a bit of everything, but would certainly like the solution recommended, think there already is a tech record history selector present selectTechRecordHistory
here
… provisional tech records (#1350) * feat(cb2-9882): provisional letter work * feat(cb2-9882): fix the unexpected display buy * feat(cb2-9882): refactor code to more readable * feat(cb2-9882): add unit tests * feat(cb2-9882): remove unnecessary variable * feat(cb2-9882): amend variable name * feat(cb2-9882): amend the logic of ineligibility * feat(cb2-9882): fix memory leak * feat(cb2-9882): update tests --------- Co-authored-by: pbardy2000 <[email protected]>
Ticket title
Add the ability to produce a rejection letter for TRLs on provisional tech records
CB2-9882
Checklist