-
Notifications
You must be signed in to change notification settings - Fork 16
Feature/roles #287
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
base: develop
Are you sure you want to change the base?
Feature/roles #287
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #287 +/- ##
===========================================
- Coverage 83.05% 78.55% -4.51%
===========================================
Files 71 79 +8
Lines 1216 1413 +197
Branches 60 74 +14
===========================================
+ Hits 1010 1110 +100
- Misses 186 283 +97
Partials 20 20
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| * Protect a query or mutation by specific roles. Also implemments GQLAuthGuard | ||
| * @param roles List of RoleType's to auth a query/mutation/etc | ||
| */ | ||
| export const Roles = (...roles: (keyof typeof RoleType)[]) => applyDecorators( |
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 standard seems to be that decorators are defined in their own file.
E.g.
- CurrentUser.decorator.ts
- https://docs.nestjs.com/guards
Can this be moved to a roles.decorator.ts file?
| */ | ||
| export const Roles = (...roles: (keyof typeof RoleType)[]) => applyDecorators( | ||
| SetMetadata('roles', roles), | ||
| UseGuards(GQLAuthGuard, RoleGuard) |
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.
NestJS documentation related to creating a roles decorator doesn't use UseGuard in their example.
Can you explain the need for UseGuards(GQLAuthGuard, RoleGuard) here?
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.
Testcheck failed and will need to be fixed before merge- No unit tests added for this new feature; what is your plan for adding unit tests?
|
Hey @RosoPenaranda how's it going with this one? |
|
I'm going to take over this one tonight, as it's over a month old |
IMPORTANT: Please do not create a Pull Request without creating an issue first.
Fixed issues (PRs without this will not be accepted)
Changes
PR Checklist
yarn lintoryarn lint:fixyarn build