-
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
#3313 - Content: Ministry: Accounts #4269
Conversation
sources/packages/web/src/views/aest/student/StudentAccountApplications.vue
Show resolved
Hide resolved
return this.repo | ||
.createQueryBuilder("studentAccountApplication") | ||
.select("studentAccountApplication.id", "id") | ||
.addSelect("studentAccountApplication.submittedDate", "submittedDate") | ||
.addSelect("user.firstName", "givenNames") | ||
.addSelect("user.lastName", "lastName") | ||
.addSelect("student.birthDate", "birthDate") | ||
.innerJoin("studentAccountApplication.user", "user") | ||
.leftJoin(Student, "student", "student.user.id = user.id") | ||
.where("studentAccountApplication.assessedDate is NULL") | ||
.orderBy("studentAccountApplication.submittedDate", "ASC") | ||
.getRawMany(); |
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.
Unless there is a strong reason, the preferred way would be to use the object query. Was there a reason to switch the query format?
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.
The reason is that we need to get birth date for a student record from the student table, and the origin query doesn't support the relation from user to student. The leftJoin(Student, ...)
in this object query allows to get the birth date.
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.
The student may not exist at this stage because it will be created after the Ministry's approval.
The birth date for this case should come from the submitted_data
->>dateOfBirth
.
...src/route-controllers/student-account-applications/models/student-account-application.dto.ts
Outdated
Show resolved
Hide resolved
...nd/apps/api/src/services/student-account-applications/student-account-applications.models.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.
Nice work, please take a look at the comments.
sources/packages/web/src/views/aest/student/StudentAccountApplications.vue
Outdated
Show resolved
Hide resolved
fullName: getUserFullName(accountApplication.user), | ||
submittedDate: accountApplication.submittedDate, | ||
})); | ||
return accountApplications; |
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.
The map
must happen for every DTO conversion to avoid changes at the service level to expose unwanted properties. Please add the map
back.
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, please take a look at the latest comments.
Quality Gate passedIssues Measures |
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, 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.
Looks good!
Screenshot of the updated page