-
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
#1830 - Institution Student Authorization Part 2 #1915
Conversation
...ges/backend/apps/api/src/route-controllers/student/_tests_/e2e/student.institutions.utils.ts
Outdated
Show resolved
Hide resolved
...s/packages/backend/apps/api/src/route-controllers/student/student.institutions.controller.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/apps/api/src/services/institution/institution.service.ts
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.
Good work @dheepak-aot . added some minor comments. I have qn, so, do we have a ticket to add cache to other queries too?
...ontrollers/student/_tests_/e2e/student.institutions.controller.getStudentProfile.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...student/_tests_/e2e/student.institutions.controller.getStudentApplicationSummary.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...student/_tests_/e2e/student.institutions.controller.getStudentApplicationSummary.e2e-spec.ts
Outdated
Show resolved
Hide resolved
We do not have a ticket for that so far. Probably we can discuss during our upcoming dev sync up and create as we have other things to discuss about the cache to other queries. |
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...e-controllers/student/_tests_/e2e/student.institutions.controller.searchStudents.e2e-spec.ts
Outdated
Show resolved
Hide resolved
...ges/backend/apps/api/src/route-controllers/student/_tests_/e2e/student.institutions.utils.ts
Outdated
Show resolved
Hide resolved
sources/packages/backend/libs/utilities/src/system-configurations-constants.ts
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.
Great work enabling the ORM cache. Please take a look at the comments.
sources/packages/backend/apps/api/src/auth/guards/institution/institution-bc-public.guard.ts
Outdated
Show resolved
Hide resolved
...ckages/backend/apps/api/src/auth/guards/institution/institution-student-data-access.guard.ts
Outdated
Show resolved
Hide resolved
...ges/backend/apps/api/src/route-controllers/student/_tests_/e2e/student.institutions.utils.ts
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed!
|
@@ -140,3 +140,9 @@ export async function authorizeUserTokenForLocation( | |||
InstitutionUserTypes.user, | |||
); | |||
} | |||
|
|||
export const INSTITUTION_BC_PUBLIC_ERROR_MESSAGE = |
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 am ok to keep those here. I was just having second thoughts if we should move or not to a constant file.
Either way, I would be good with 😉
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, great start enabling the cache. Let's ensure that we have a ticket created to enable the cache for other areas like, as we talked about, the DB methods on the jwt.strategy.ts
file. 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 for doing the changes 👍 LGTM
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!
Enable Typeorm Cache with Redis
Ref: https://github.com/typeorm/typeorm/blob/master/docs/caching.md