-
Notifications
You must be signed in to change notification settings - Fork 1
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
Mrc 5295- Role create endpoint #62
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #62 +/- ##
=======================================
Coverage 93.98% 93.98%
=======================================
Files 67 67
Lines 532 532
Branches 131 131
=======================================
Hits 500 500
Misses 30 30
Partials 2 2 ☔ View full report in Codecov by Sentry. |
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'm assuming you didn't mean to close this PR??
Looks good, but I think there's been some confusion re the model classes, so let's chat about that on Tuesday. You've still got e.g. "Role" as the database entity, which I thought we were going to change so that "Role" was the dto and RoleEntity
the db class. We maybe got our wires crossed and I thought we had agreed on something that we hadn't.. I don't really think you need to put the dto classes in a sub-folder of model, since they're the primary objects, but don't mind as long as the db classes go into 'entitity'.
api/app/build.gradle.kts
Outdated
@@ -54,6 +54,7 @@ dependencies { | |||
implementation("com.auth0:java-jwt:4.4.0") | |||
implementation("com.fasterxml.jackson.module:jackson-module-kotlin") | |||
implementation("org.flywaydb:flyway-core") | |||
testImplementation("org.testng:testng:7.1.0") |
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.
What was this for? I don't see any references to testng
anywhere else in the PR.
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.
oh may dead end when testing. have removed
{ | ||
roleService.createRole(createRole) | ||
|
||
return ResponseEntity.ok(mapOf("message" to "Role created")) |
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'm not sure how useful {"message": "Role created"}
is really. It might be nice to actually just return the new Role, since the front end is probably going to want to display it immediately. Or else just return an empty 200 and let the front end decide what message it wants to show.
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.
yup agree.. have created dtos needed for this in user endpoints so for now have created ticket https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5324/Create-endpoints-to-return-Dtos... this will get done after endpoints tickets
@@ -25,8 +25,7 @@ class Packet( | |||
inverseJoinColumns = [JoinColumn(name = "tag_id")] | |||
) | |||
var tags: MutableList<Tag> = mutableListOf(), | |||
|
|||
@OneToMany(mappedBy = "packet") | |||
@OneToMany(mappedBy = "packet", cascade = [CascadeType.ALL]) |
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'm not sure rolePermissions
actually belongs on Packet
anyway - it's not really of interest relating to the packet itself, just something we need to take account of when determing user access to packet, and for admin users managing access -but they'll be coming from the user/role side and won't be interested in the full packet data at that point.
But even so, I guess it's useful to include it here so that we get the cascade functionality on delete?
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.
yeah it needs to be there because there a FK from the role_permissions table to the packet table...
data class CreateRole( | ||
@field:NotNull | ||
val name: String, | ||
val permissions: List<String> = listOf() |
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.
Are these permission names or permission ids? Might be best to update this field to be clear i,e, permissionNames
?
Though we're just dealing with global permissions here of course, so this is going to evolve.
I think it would also be acceptable to make the create function just take a role name and create an empty role initially, since you're going to need to made an add permissions endpoint anyway. But this is fine too!
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.
have renamed to permissionNames... but yeah added this cos they may want to add global permissions at start.. but they don't have to provide permissionNames
@@ -0,0 +1,29 @@ | |||
package packit.service |
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 think there's an inconsistency in module naming creeping in here, as you've got PacketService
interface and BasePacketService
class declared in BasePacketService
module, but here you've named the module after the interface, PermissionsService
. I actually prefer this way, so would suggest renaming BasePacketService
module to PacketService
.
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.
oh yeah didnt even notice that for packet one must've been old... but yup il update this
private val permissionRepository: PermissionRepository | ||
) : PermissionService | ||
{ | ||
override fun checkMatchingPermissions(permissionsToCheck: List<String>): List<Permission> |
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.
So this method is literally just checking that the basic permissions themselves exist in the database? We're not checking that a user has sufficient permissions to do anything? So we can rely on the result count since we know that permission names are enforced as unique?
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.
yup yup exactly
{ | ||
if (roleRepository.existsByName(roleName)) | ||
{ | ||
throw PackitException("roleAlreadyExists") | ||
} | ||
val role = Role(name = roleName) | ||
role.rolePermissions = permissions.map { RolePermission(permission = it, role = role) } | ||
.toMutableList() |
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.
Could be toList()
, this doesn't need to be mutable. Oh, except it's defined as mutable on the entity type.. maybe it's doesn't need to be mutable there either, seems a bit odd..? Depends on how you're going to do updates I guess.
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.
yeah i wish but cant because its a mutable list.. needs to be mutable because we will be updating the list (adding/ removing)...
{ | ||
override fun checkMatchingPermissions(permissionsToCheck: List<String>): List<Permission> | ||
{ | ||
val matchedPermissions = permissionRepository.findByNameIn(permissionsToCheck) |
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.
Is this case sensitive?
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.
yup will be
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.
Also.. no controller unit tests?
Oh haha yeah weird not sure how it closed... But yup in the previous PR I made a ticket for this and will do all renaming for models and folder sorting in separate ticket as agreed 😄. Will do that ticket asap after these endpoint tickets have been merged in https://mrc-ide.myjetbrains.com/youtrack/issue/mrc-5310/Model-folder-structure-Naming |
yeah I didn't put any for all the PRs..... because they legit just call out to service and return whats returned... These are covered in integration tests (as I wrote integration tests for controllers) |
This PR adds an endpoint for role creation. I have also moved some models into the dto folder thus looks like I touched lots of files but these are only import statements... For testing try hit the endpoint with a user with admin privileges (use super admn script to create this, and get bearer token via login)..