-
Notifications
You must be signed in to change notification settings - Fork 0
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
Role management endpoints #145
Conversation
Check the documentation preview: https://642d8b5ae9a57b4c25c78bd5--niaefeup-backend-docs.netlify.app |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #145 +/- ##
=============================================
+ Coverage 88.49% 90.82% +2.33%
- Complexity 345 393 +48
=============================================
Files 61 65 +4
Lines 791 916 +125
Branches 65 75 +10
=============================================
+ Hits 700 832 +132
+ Misses 57 40 -17
- Partials 34 44 +10
☔ View full report in Codecov by Sentry. |
Check the documentation preview: https://642d8bdd7b8e354bb1f7061d--niaefeup-backend-docs.netlify.app |
Is it ready for review? |
Check the documentation preview: https://64494031ea2fff069ea42bf3--niaefeup-backend-docs.netlify.app |
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 job so far! I know I wrote a lot of comments but the functionalities are there and seem to work as expected! A lot of the comments are about small things like documentation and code suggestions
src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt
Outdated
Show resolved
Hide resolved
src/test/kotlin/pt/up/fe/ni/website/backend/controller/RoleControllerTest.kt
Outdated
Show resolved
Hide resolved
Co-authored-by: BrunoRosendo <[email protected]>
Co-authored-by: BrunoRosendo <[email protected]>
src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt
Show resolved
Hide resolved
Check the documentation preview: https://64a5762f1e76480aacf54ba7--niaefeup-backend-docs.netlify.app |
b1ee70c
to
b01bb25
Compare
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.
Almost there! I just marked some comments that haven't been properly dealt with and one other suggestions!
src/main/kotlin/pt/up/fe/ni/website/backend/controller/RoleController.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
Please change this in the generation service too: website-niaefeup-backend/src/main/kotlin/pt/up/fe/ni/website/backend/service/GenerationService.kt Line 92 in 0c670a9
I commented the lines where I defined both sides of the relationship |
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 just noticed we don't have a way of updating some role fields, such as name and isSection. We can discuss that in the next meeting
Tag.ROLES, | ||
mutableListOf( | ||
DocumentedJSONField("name", "Name of the role", JsonFieldType.STRING), | ||
DocumentedJSONField("permissions", "Permissions in the role, as an integer", JsonFieldType.ARRAY), |
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.
You may want to open an issue for that and CC him
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
role.generation = generation | ||
generation.roles.add(role) |
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.
Only the generation side is necessary
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.
without the generation side it doesn't work for some reason :/
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
4c164c6
to
62907e4
Compare
Check the documentation preview: https://64c134edc956871187f2c84d--niaefeup-backend-docs.netlify.app |
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 🚀
@LuisDuarte1 I only want to discuss this in a meeting, I forgot last time |
Check the documentation preview: https://64dcb75697ed715c4dab4f17--niaefeup-backend-docs.netlify.app |
Done |
src/main/kotlin/pt/up/fe/ni/website/backend/service/RoleService.kt
Outdated
Show resolved
Hide resolved
Check the documentation preview: https://64dceaaf583dbb275aaa12e6--niaefeup-backend-docs.netlify.app |
Check the documentation preview: |
Closes #79
Adds various endpoints for role management, such as: