Skip to content
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 5373- update permissions #87

Merged
merged 44 commits into from
Jun 18, 2024
Merged

Mrc 5373- update permissions #87

merged 44 commits into from
Jun 18, 2024

Conversation

absternator
Copy link
Contributor

@absternator absternator commented Jun 7, 2024

This PR is for updating permissions on roles and updating specific permissions on users. Also had to add get endpoints for tags,packet groups and packets so can retrieve these on adding scoped permissions

Testing:
Try mixing and matching adding and removing permission. Here are some rules of code

  • no submit if nothing selected
  • No adding duplicate role permissions
    Add Permissions Form part
  • if user.manage cant select scope
  • if not user.manage and scope is not global , has to select a resource

image

Copy link

codecov bot commented Jun 7, 2024

Codecov Report

Attention: Patch coverage is 98.50746% with 2 lines in your changes missing coverage. Please review.

Project coverage is 95.95%. Comparing base (e9b13d5) to head (fb037cc).

Current head fb037cc differs from pull request most recent head ac8d381

Please upload reports for the commit ac8d381 to get more accurate results.

Files Patch % Lines
...cess/updatePermission/AddScopedPermissionInput.tsx 94.11% 1 Missing ⚠️
...eAccess/updatePermission/UpdatePermissionsForm.tsx 95.65% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #87      +/-   ##
==========================================
+ Coverage   95.61%   95.95%   +0.33%     
==========================================
  Files          95      105      +10     
  Lines         844      964     +120     
  Branches      218      248      +30     
==========================================
+ Hits          807      925     +118     
- Misses         36       38       +2     
  Partials        1        1              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Base automatically changed from mrc-5372-manage-users-update-roles to mrc-5370-manage-roles-update-users June 7, 2024 13:21
Base automatically changed from mrc-5370-manage-roles-update-users to main June 9, 2024 11:27
Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't gone through all the code, just got a couple of remarks on behaviour and on the API to start with. Will look at the rest of the code later!

User interface looks good! It's clean and makes sense. Works really nicely for roles. I get "Role not found" error when I try to save user-specific permissions.

I think it would be good to make the + button in the dialog a little more obvious. I intuitively found myself expecting the permission to get added as soon as I'd selected the scope from the drop down and was surprised when it wasn't. I can see why you haven't done that, because you need to accommodate global permissions. But I think maybe if it just had 'Add to list' or 'Add permission' as text it would be a bit more obvious.

Add button could also appear as disabled/non-clickable when a resource needs to be selected.

It would be nice if the scope drop-down was multi-select. And also the permissions to remove drop-down.

Could we have a helper script to add tags, so we can test that part?
Did we decide that we should be able to add tags from packit? (I can't remember!)

I think "Permissions To Add" and "Permissions To Remove" headers could be a bit more prominent - I think that would make the panels with the list of selected permissions feel more a part of their respective sections on the dialog, That isn't completely clear visually at the moment. Wasn't there a mock-up or a suggestion of having "To Add" and "To Remove" side by side? Did you reject that idea?

A slight disrepancy: when you select a permission type, then change scope to non-global, it shows validation error as no resource is selected. However, if you add a scoped resource, then change scope type, no validation error is shown, even though it's not possible to add the permission at that point for the same reason.

"Permission already exists" error persists even after you've seleted a different permission - I think it should be cleared when you select a new permission or scope.

Also - you seem to not get the "Permission already exists" error when you add a permission which the role had before the dialog was opened. Instead you get a "Role permission already exists" when you try to Save. I think it should flag the duplicate immediately, otherwise it's going to be confusing to a user who may be adding multiple permissions which was the problematic one. But maybe have slightly different messages e.g. "Permission already exists on role" vs "You have already added this permission"

I think it would better to show "packet group" as a scope type to the user in the radio buttons (even though "packetGroup" is shown in the permission names which are generated and displayed, I think the radio button label would be better as human-English.

Check with RIch about this, but I'm not sure all the scopable permissions can be scoped to everything. So, I'm not sure packet run makes sense for an individual packet, because a packet is what gets generated by a packet run (of a packet group). Not sure about packet push either, but that might make a bit more sense.

I think this could have been split into multiple PRs. For example, could have split out the backend changes into a separate branch, and worked on two branches in parallel. Alternatively, supporting the different resource types could have gone into separate PRs. Could also have made user-specific perms a separate PR, though obviously there'a s lot of overlap so that would have been quite small. Perhaps we should start making the tickets finer-grained from the outset to encourage smaller branches.

@@ -70,4 +72,15 @@ class PacketController(private val packetService: PacketService)
.headers(response.second)
.body(response.first)
}

@GetMapping("/packetGroup")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From a REST API perspective, arguably it would be more consistent to put this at the same level at packets, not below it, and should also be pluralised to /packetGroups... because packetGroups are also a top level resource type, of a different type to packets themselves.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah initially was iffy because packetGroup is like a subset of packet (derived ) but you are right... il do as part of #87 because I've made changes to endpoint

})
})
.refine((data) => data.scope === "global" || !!data.scopeResource?.id, {
message: "Scoped name is required if not global scope ",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think "scoped name" is going to mean anything to the user in the context of the rest of the UI.
How about this?

Suggested change
message: "Scoped name is required if not global scope ",
message: "Select a resource for non-global scope",

Comment on lines 104 to 112
override fun getPacketGroups(pageablePayload: PageablePayload, filteredName: String): Page<PacketGroup>
{
val pageable = PageRequest.of(
pageablePayload.pageNumber,
pageablePayload.pageSize,
Sort.by("name")
)
return packetGroupRepository.findAllByNameContaining(filteredName, pageable)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should there maybe be a PacketGroupService that this method belongs to, since it's talking to the packetGroup repository - would be consistent with adding a PacketGroupController and top level route..

Copy link
Contributor Author

@absternator absternator Jun 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup will be done as part of #87

Comment on lines +23 to +25
val pageable = PageRequest.of(
pageablePayload.pageNumber,
pageablePayload.pageSize,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This little bit gets repeated a lot - would it be worthwhile having a util to create a PageRequest from a PageablePayload, plus sort key?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup done as part of #87


@Test
@WithAuthenticatedUser
fun `get ordered pageable packetGroups with filtered name`()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't remember if you've got an integration test elsewhere of getting a page other than the first one, but might be worth including that (though not necessarily for every endpoint, since they all use the same logic).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do as part of #87 as have changed paging logic

Tag(name = "test-tag-1"),
Tag(name = "test-tag-5"),
Tag(name = "test-tag-4"),
Tag(name = "test-tag-3"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe include some tags here which shouldn't be returned by the filter.

Comment on lines 23 to 30
export interface NewRolePermission {
permission: string;
packet?: BasicPacket | null;
tag?: Tag | null;
packetGroup?: BasicPacketGroup | null;
id?: number;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the only real distinction between this and RolePermission that RolePermission will have an id, and NewRolePermission doesn't. Feels like one ought to be able to extend the other, and just add id

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yuppp for sure haha feel stupid now done


if (error) return <CommandEmpty>Error Fetching data</CommandEmpty>;
return (
<CommandList>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This component is basically the drop down list for choosing the scope? Should it be called PermissionScopeCommandList rather than PermissionCommandList?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agreed

import { PageableBasicDto } from "../../../../../../types";
import { fetcher } from "../../../../../../lib/fetch";

export const useGetDtosForScopedPermissions = (scope: PermissionScope, filterName: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to mention Dtos: as far as the front end is concerned, the Dto is the model..

Suggested change
export const useGetDtosForScopedPermissions = (scope: PermissionScope, filterName: string) => {
export const useGetScopedPermissions = (scope: PermissionScope, filterName: string) => {

import { fetcher } from "../../../../../../lib/fetch";

export const useGetDtosForScopedPermissions = (scope: PermissionScope, filterName: string) => {
const scopePathVariable = scope === "tag" ? "tag" : `packets${scope === "packetGroup" ? "/packetGroup" : ""}`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Scope might be global here? So in that case, you'd generate "packet" for scopePathVariable.. and then not use it.. because in that case you don't do the fetch, because you pass null into useSWR?
This structure is quite confusing - I feel like just a conditional on "global" around the fetch, and a switch on the scope type when setting the path would be clearer and worth the extra few lines.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup will make more clear as part of #87. as I will be touching the endpoint anyways

Comment on lines 4 to 5
permission1: RolePermission | NewRolePermission,
permission2: RolePermission | NewRolePermission
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be able to just use the base permission type here if you make one extend the other,.

@absternator
Copy link
Contributor Author

User interface looks good! It's clean and makes sense. Works really nicely for roles. I get "Role not found" error when I try to save user-specific permissions.

Migtve got error because reside super user... i thought I made it so you cant delete/update role but forgot so have fixed now

I think it would be good to make the + button in the dialog a little more obvious. I intuitively found myself expecting the permission to get added as soon as I'd selected the scope from the drop down and was surprised when it wasn't. I can see why you haven't done that, because you need to accommodate global permissions. But I think maybe if it just had 'Add to list' or 'Add permission' as text it would be a bit more obvious.

Good idea done

Add button could also appear as disabled/non-clickable when a resource needs to be selected.

Good idea done

It would be nice if the scope drop-down was multi-select. And also the permissions to remove drop-down.

That was initial plan but limitations with multiselect and technical issues prevented me... maybe il make ticket and revisit later if needed

Could we have a helper script to add tags, so we can test that part? Did we decide that we should be able to add tags from packit? (I can't remember!)

Its just 2 column table might be easier to just add manually if needed.... but can add later when tags work.... Im not sure if can add tags from packit I forgot haha

I think "Permissions To Add" and "Permissions To Remove" headers could be a bit more prominent - I think that would make the panels with the list of selected permissions feel more a part of their respective sections on the dialog, That isn't completely clear visually at the moment. Wasn't there a mock-up or a suggestion of having "To Add" and "To Remove" side by side? Did you reject that idea?

I have made more bold also added seperator in between.. Yup originally but rejected after the dialogs didn't do side by side

A slight disrepancy: when you select a permission type, then change scope to non-global, it shows validation error as no resource is selected. However, if you add a scoped resource, then change scope type, no validation error is shown, even though it's not possible to add the permission at that point for the same reason.

The reasoning is that you must have submitted the form and failed in the first case. this triggers use-hook-form errors as the form is dirty!! but after submit its successful thus you don't get those errors...

"Permission already exists" error persists even after you've seleted a different permission - I think it should be cleared when you select a new permission or scope.

Similar to above comment it gets cleared after submit form successfully then

Also - you seem to not get the "Permission already exists" error when you add a permission which the role had before the dialog was opened. Instead you get a "Role permission already exists" when you try to Save. I think it should flag the duplicate immediately, otherwise it's going to be confusing to a user who may be adding multiple permissions which was the problematic one. But maybe have slightly different messages e.g. "Permission already exists on role" vs "You have already added this permission"

Good catch I thought I did it but a bug!!! thanks

I think it would better to show "packet group" as a scope type to the user in the radio buttons (even though "packetGroup" is shown in the permission names which are generated and displayed, I think the radio button label would be better as human-English.

Check with RIch about this, but I'm not sure all the scopable permissions can be scoped to everything. So, I'm not sure packet run makes sense for an individual packet, because a packet is what gets generated by a packet run (of a packet group). Not sure about packet push either, but that might make a bit more sense.

Oh yeah makes sense, il double check with him and make a PR accordingly

I think this could have been split into multiple PRs. For example, could have split out the backend changes into a separate branch, and worked on two branches in parallel. Alternatively, supporting the different resource types could have gone into separate PRs. Could also have made user-specific perms a separate PR, though obviously there'a s lot of overlap so that would have been quite small. Perhaps we should start making the tickets finer-grained from the outset to encourage smaller branches.

Yup for sure that is totally my bad I'm sorry... it was all just in a state of chaos during the ticket initially.. Il also make sure to make more fine grained tickets.

Copy link
Contributor

@EmmaLRussell EmmaLRussell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All good, assuming those changes made in upcoming PR - still got a couple of reservations about the form behaviour, but don't think that should hold up merging.

Migtve got error because reside super user... i thought I made it so you cant delete/update role but forgot so have fixed now

👍 Seems to working ok for non-supers.

It would be nice if the scope drop-down was multi-select. And also the permissions to remove drop-down.

That was initial plan but limitations with multiselect and technical issues prevented me... maybe il make ticket and revisit later if needed

I'm sure we'll get asked for this, so yeah let's make a ticket for it.

Could we have a helper script to add tags, so we can test that part?
Its just 2 column table might be easier to just add manually if needed....

It is needed for manual testing, and 'just adding manually' requires me to remember where and how that needs to be done! 😆

OK. here's what I used. Could you maybe include this, or something like it? It's going to be useful in other tickets where we need to play around with tags. Or alternatively, maybe we could just have a generic db-exec script, since as you say the SQL itself is very easy once you've dredged up what container and creds to execute it with...

docker exec packit-db psql -U packituser -d packit -c \
"INSERT INTO TAG (ID, NAME) VALUES(1, 'dev-tag-1')"

docker exec packit-db psql -U packituser -d packit -c \
"INSERT INTO TAG (ID, NAME) VALUES(2, 'dev-tag-2')"

Tags stuff works great btw!

The reasoning is that you must have submitted the form and failed in the first case. this triggers use-hook-form errors as the form is dirty!! but after submit its successful thus you don't get those errors...

OK, but it means that you immediately see new error messages after you add the first permission whenever you subsequently change to an invalid combination of inputs. But you wouldn't see them for same inputs before you add the first permission.

There's also a somewhat pathological case, if you:

  • add a scoped permission
  • click global
  • click packet - see "select a resource for non-global scope" validation error message
  • select user.manage
  • At this point, the scope automatically updates to global, but the error message about non-global scope remains. You can successfully 'Add permission' despite this validation error.

I think this is all potentially confusing. Doesn't need sorting out in this branch. but i think it should be tidied up at some point.

Similar to above comment it gets cleared after submit form successfully then

Again, this doesn't feel like intuitive behaviour - I don't think the user is going to be thinking about adding a permission from that area of the dialog as a 'form submission', since defining the whole set of permissions to add and remove is 'the form' of the overall dialog. I think they'd expect to either see immediately updating validation messages, or messages which only update on "Add" button, but not a combination of the two which is the current behaviour.

Again, this doesn't necessarily need changing in this branch, but it feels a bit inconsistent.

As discussed earlier, I'm assuming we'll postpone more refined scoping for the different permissions to a future branch.

<Button variant="outline" size="icon" aria-label="delete-user" disabled>
<Trash2 className="h-4 w-4 " />
</Button>
<Button variant="outline" size="icon" aria-label="edit-user" disabled>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it would be ok for an admin to grant themselves additional permissions (e.g. packet push) but we'd want to stop them from removing permissions. We can probably assume admins gets properly initialised for now though...

Copy link
Contributor Author

@absternator absternator Jun 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeap admin has all the permissions so nothing for them to add

@@ -1,14 +1,14 @@
/* eslint-disable max-len */
import {
NewRolePermission,
BaseRolePermission,
RolePermission
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused import warning for RolePermission here. Couple of similar gha linting warnings on UpdateRoleDropDownMenu.test.tsx

@absternator absternator merged commit 1bb5519 into main Jun 18, 2024
3 checks passed
@absternator absternator deleted the mrc-5373-update-permissions branch June 18, 2024 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants