#5190 - Appeals and forms analysis work - Student Notes#5862
Conversation
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
You can also share your feedback on Copilot code review. Take the survey.
| * @param note note to be transformed. | ||
| * @returns an object item for the notes returned by the API. | ||
| */ | ||
| export function noteToApiReturn(note: Note) { |
There was a problem hiding this comment.
not your code but can we call it transformNoteToApiReturn?
| return ApiClient.NoteApi.getStudentNotes(studentId); | ||
| } | ||
| const convertedNotes = STUDENT_NOTE_TO_NOTES_TYPE_MAP.get(noteType) ?? [ | ||
| noteType as unknown as NoteType, |
There was a problem hiding this comment.
Clever but confusing conversion, it seems there's no other way to "extend" enums but you could have a export type directly mapping to the enum ->
export type InstitutionNoteType =
| NoteType.General
| NoteType.Restriction
| NoteType.System
| NoteType.Program
| NoteType.Designation;
not sure of the side effects of this tho!
There was a problem hiding this comment.
Yeah, I wasn't too happy about it. I tried different approaches, but I tried to avoid losing the iterable part over the InstitutionNoteType that is used in one part of the UI.
I can give a second thought 😉
There was a problem hiding this comment.
Didn`t find any great alternative. I ended up just renaming it a bit and changing the cast as below.
studentNote as string as NoteType
tiago-graf
left a comment
There was a problem hiding this comment.
It looks great just a small comment
| */ | ||
| export class NoteAPIQueryStringAPIInDTO { | ||
| @IsOptional() | ||
| @ArrayMaxSize(Object.keys(NoteType).length) |
| @Roles(Role.StudentCreateNote) | ||
| @ApiNotFoundResponse({ description: "Student not found." }) | ||
| @ApiForbiddenResponse({ | ||
| description: |
There was a problem hiding this comment.
Asking the question if not raised already, for the scope of adding institution notes, there must be consideration of NoteTypes permissible and not permissible to be added.
There was a problem hiding this comment.
I am not sure if I am following. What would be the case to have institution notes permissions considerations in the context of this feature?
There was a problem hiding this comment.
Currently, the DTO is shared between the institution and students. We need to create a specific DTO for each client, do the same on the Web, and create an E2E for each. I do not see it as part of this PR.
dheepak-aot
left a comment
There was a problem hiding this comment.
Nice work. Thanks for making the changes. Looks good 👍
|




PR Goal
UI displaying "Student appeal" and "Student form"
Ministry
Institution
UI Fixes
Technical Changes
Formsand filter byStudent appealandStudent formsnotes.NoteServicemethods on Web are still accepting oneStudentNoteTypeorInstitutionNoteType. Please note that the Web deals with the institution vs students' notes using different enums, which are subsets of the backend notes.enum NoteTypewas added to the Web.NoteAPIOutDTOis now in sync between Web and API.E2E Tests