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

Custom roles in ACL policy #729

Merged
merged 4 commits into from
Jul 12, 2024
Merged

Custom roles in ACL policy #729

merged 4 commits into from
Jul 12, 2024

Conversation

dennis531
Copy link

@dennis531 dennis531 commented Jun 21, 2024

Changes

  • Add create option function to dropdown component
  • Add button "new custom role" to create a policy with a new custom role

Alternative

We could show only one button "new policy" and enable the creation in the dropdown

Screenshot

grafik

Close #658

@dennis531 dennis531 added the type:enhancement New feature or request label Jun 21, 2024
Copy link
Contributor

github-actions bot commented Jun 21, 2024

This pull request is deployed at test.admin-interface.opencast.org/729/2024-07-11_12-28-21/ .
It might take a few minutes for it to become available.

Copy link
Contributor

Use docker or podman to test this pull request locally.

Run test server using develop.opencast.org as backend:

podman run --rm -it -p 127.0.0.1:3000:3000 ghcr.io/opencast/opencast-admin-interface:pr-729

Specify a different backend like stable.opencast.org:

podman run --rm -it -p 127.0.0.1:3000:3000 -e PROXY_TARGET=https://stable.opencast.org ghcr.io/opencast/opencast-admin-interface:pr-729

It may take a few seconds for the interface to spin up.
It will then be available at http://127.0.0.1:3000.
For more options you can pass on to the proxy, take a look at the README.md.

Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Code works and looks reasonable.

@Arnei
Copy link
Member

Arnei commented Jun 24, 2024

As for the alternative you suggested:

  • The benefit I see in the additional "New custom role" button is that it serves a a sort of confirmation. Meaning by clicking this button, the user knowingly accepts that their adding roles unknown to the system.
  • However, not having the button also has its upsides. It means we don't have two dropdowns that look like they should work identically, but don't. Furthermore, dropdowns for custom roles are not "creatable" anymore after closing and opening the event details, meaning if you want to change a custom role later on because of a typo, you cannot change the role directly but have to remove it and create a new one, which can be a bit cumbersome.

Therefore I'm slightly in favor of the no-button alternative, but it's not hard enough of an opinion that I would block this PR.

@dennis531
Copy link
Author

I prefer to have only one button, as the new create dropdown can also be used to select existing ACLs. In my opinion, this separation would rather confuse users and lead to lower usability.

We could also use the create dropdown for editing roles to avoid the workaround of removing policies.

Copy link
Contributor

github-actions bot commented Jul 5, 2024

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Dennis Benz added 3 commits July 8, 2024 10:39
Changes:
- Dropdowns for new policies are always creatable
- Allow creating new roles in existing policies
@dennis531
Copy link
Author

I prefer to have only one button, as the new create dropdown can also be used to select existing ACLs. In my opinion, this separation would rather confuse users and lead to lower usability.

We could also use the create dropdown for editing roles to avoid the workaround of removing policies.

Done as described.

Copy link
Member

@Arnei Arnei left a comment

Choose a reason for hiding this comment

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

Works and looks good to me. Really like this over having an extra button. One minor nitpick below.

src/components/shared/DropDown.tsx Outdated Show resolved Hide resolved
@dennis531 dennis531 requested a review from Arnei July 11, 2024 12:30
@Arnei Arnei merged commit a0ab426 into opencast:main Jul 12, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom roles pattern not respected in ACL tab
2 participants