-
Notifications
You must be signed in to change notification settings - Fork 14
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
#1910 - Student profile returns SIN #2354
Conversation
sources/packages/backend/apps/api/src/route-controllers/student/student.aest.controller.ts
Outdated
Show resolved
Hide resolved
@@ -181,10 +181,13 @@ export class StudentProfileAPIOutDTO { | |||
contact: ContactInformationAPIOutDTO; | |||
validSin: boolean; | |||
disabilityStatus: DisabilityStatus; | |||
} | |||
|
|||
export class SensitiveDataStudentProfileAPIOutDTO extends StudentProfileAPIOutDTO { |
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.
We probably have web dtos counterparts for these DTOs. Do they require any change also?
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.
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 see ministry student profile has SIN to it. But we can use a seperate service in Vue with different DTO for ministry and student.
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.
then shouldn't we make sin
as optional in web?
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 believe that the below one can reflect both possible DTOs received from the API.
SIMS/sources/packages/web/src/services/http/StudentApi.ts
Lines 46 to 52 in 43ca26b
async getStudentProfile( | |
studentId?: number, | |
): Promise<StudentProfileAPIOutDTO> { | |
return this.getCall<StudentProfileAPIOutDTO>( | |
this.addClientRoot(`student/${studentId ?? ""}`), | |
); | |
} |
The service is already returning a StudentProfile
that can have optional properties like SIN, as @ann-aot mentioned.
SIMS/sources/packages/web/src/services/StudentService.ts
Lines 52 to 62 in 43ca26b
async getStudentProfile(studentId?: number): Promise<StudentProfile> { | |
const { dateOnlyLongString } = useFormatters(); | |
const studentProfile = await ApiClient.Students.getStudentProfile( | |
studentId, | |
); | |
const studentInfoAll: StudentProfile = { | |
...studentProfile, | |
birthDateFormatted: dateOnlyLongString(studentProfile.dateOfBirth), | |
}; | |
return studentInfoAll; | |
} |
The StudentProfile
would be something like below.
export interface StudentProfile extends StudentProfileAPIOutDTO {
birthDateFormatted: string;
sin?: string;
hasRestriction?: boolean;
}
Does it make sense?
sources/packages/backend/apps/api/src/route-controllers/student/student.controller.service.ts
Outdated
Show resolved
Hide resolved
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, nice work @andrepestana-aot
sources/packages/backend/apps/api/src/route-controllers/student/student.controller.service.ts
Show resolved
Hide resolved
@@ -242,13 +242,18 @@ export class StudentAESTController extends BaseController { | |||
@Param("studentId", ParseIntPipe) studentId: number, | |||
): Promise<AESTStudentProfileAPIOutDTO> { | |||
const [student, studentRestrictions] = await Promise.all([ | |||
this.studentControllerService.getStudentProfile(studentId), | |||
this.studentControllerService.getStudentProfile(studentId, { | |||
withSensitiveData: true, |
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.
👍
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.
Nice work. 👍
Kudos, SonarCloud Quality Gate passed!
|
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.
Thanks for making the changes and for the discussions. Looks good 👍
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.
Thanks @andrepestana-aot for doing the changes and for the info 👍
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.
Thanks for doing the changes 👍
Student view
Institution view
Ministry view