Skip to content

Conversation

@ferishili
Copy link
Contributor

@ferishili ferishili commented May 19, 2025

This PR fixes #1290,

It introduces the optional offering of the button for the "overwrite of the events" when saving the series acl based on the config: admin.series.acl.event.update.mode as to being optional or empty to offer both buttons!
The button label and hint also got slightly more meaningful texts.

NOTE: At the time of creating this PR there is a commit/PR missing from develop (default) which makes the user and system perm separation, hence the permission dialogs are broken! So, please use stable.opencast.org in order to test!

@ferishili ferishili self-assigned this May 19, 2025
@ferishili ferishili added priority:critical Unless this is fixed, we will not use the new admin ui type:enhancement New feature or request type:usability Improves the UX labels May 19, 2025
@ferishili ferishili requested a review from Arnei May 19, 2025 11:59
@github-actions
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-1295

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-1295

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.

@github-actions
Copy link
Contributor

This pull request is deployed at test.admin-interface.opencast.org/1295/2025-05-19_11-59-08/ .
It might take a few minutes for it to become available.

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.

The configuration key admin.series.acl.event.update.mode seems to be missing in the backend. At least I could not find it. If it should exist, could you add it?


const orgProperties = useAppSelector(state => getOrgProperties(state));

const overrideEnabled = (orgProperties['admin.series.acl.event.update.mode'] || 'optional').toLowerCase() === 'optional';
Copy link
Member

Choose a reason for hiding this comment

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

Aren't there supposed to be three modes, always, never and optional? Can that really be represented with a boolean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right!
in this place to check and offer the button if it is optional is correct.
what is missing is to make sure that never & always are also in the play!

@ferishili
Copy link
Contributor Author

The configuration key admin.series.acl.event.update.mode seems to be missing in the backend. At least I could not find it. If it should exist, could you add it?

It is under etc/org.opencastproject.organization-mh_default_org.cfg but by default commented out and set to optional!

@Arnei
Copy link
Member

Arnei commented May 22, 2025

The configuration key admin.series.acl.event.update.mode seems to be missing in the backend. At least I could not find it. If it should exist, could you add it?

It is under etc/org.opencastproject.organization-mh_default_org.cfg but by default commented out and set to optional!

Sorry for troubling you, but could you point out the line in the file? I'm really not seeing it in https://github.com/opencast/opencast/blob/r/17.x/etc/org.opencastproject.organization-mh_default_org.cfg

@ferishili
Copy link
Contributor Author

ferishili commented May 22, 2025

Sorry for troubling you, but could you point out the line in the file? I'm really not seeing it in https://github.com/opencast/opencast/blob/r/17.x/etc/org.opencastproject.organization-mh_default_org.cfg

No worries, it seems that it is missing in 17.x but it is there in 16.x!

https://github.com/opencast/opencast/blob/0fea543a1a8ada9e9017758e66bcbdf2513363c4/etc/org.opencastproject.organization-mh_default_org.cfg#L341C1-L342C1

However, the documentation for 17.x says it is still effectively there: https://docs.opencast.org/r/17.x/admin/#configuration/acl/#updating-series-permissions

[UPDATE]: @Arnei, I found the patch that removed the settings, it appears to me that this setting is removed considering it is related to old admin-ui which shouldn't!
PS: fun fact! Both removing commit and requesting to add it back came from Lars 😆

@Arnei
Copy link
Member

Arnei commented May 22, 2025

No worries, it seems that it is missing in 17.x but it is there in 16.x!

https://github.com/opencast/opencast/blob/0fea543a1a8ada9e9017758e66bcbdf2513363c4/etc/org.opencastproject.organization-mh_default_org.cfg#L341C1-L342C1

However, the documentation for 17.x says it is still effectively there: https://docs.opencast.org/r/17.x/admin/#configuration/acl/#updating-series-permissions

[UPDATE]: @Arnei, I found the patch that removed the settings, it appears to me that this setting is removed considering it is related to old admin-ui which shouldn't! PS: fun fact! Both removing commit and requesting to add it back came from Lars 😆

:D Then would you do Lars the favor of adding it back to 17.x, just like it was in 16.x?

@github-actions
Copy link
Contributor

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

@github-actions
Copy link
Contributor

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

@snoesberger
Copy link
Contributor

Thanks for this PR @ferishili.

I did test this PR with our test environment where we have merge mode "actions" configured.

Setting prop.admin.series.acl.event.update.mode=never now hides the "Save series and overwrite event permissions" button and changing series permissions doesn't override the event ACLs.

If I configure prop.admin.series.acl.event.update.mode=optional and use the "Save series and overwrite event permissions" button, the existing event ACL is removed and replaced by the new series ACL. The "Save series permission" button only changes the series ACL. This works as expected

However, the behaviour of prop.admin.series.acl.event.update.mode=always does not appear to be as described in the documentation.

  • always: When modifying series permissions, automatically remove all permission rules specific to single episodes
    belonging to the series. This enforces every episode has the rules of the series in effect as soon as they
    are changed.

In our test environment, "always" and "never" behave exactly the same. As I understand it, when changing the series ACLs, the event ACLs should be removed and replaced with the new series ACLs. This is equivalent to the "Save series and overwrite event permissions" button with the prop.admin.series.acl.event.update.mode=optional configuration.

@Arnei Arnei changed the base branch from develop to r/17.x July 24, 2025 08:31
@Arnei
Copy link
Member

Arnei commented Jul 24, 2025

Rebased this onto r/17.x, as this is "fixing" a feature introduced with r/17.x (which was main at the time).

@gregorydlogan
Copy link
Member

I'm seeing wonky behaviour locally which I can't reproduce with legacy.opencast.org:

Screencast.from.2025-09-23.15-28-21.mp4

This happens every time you modify the permissions for an unsaved role. So at the end of the video, if I went back and modified ROLE_ADMIN's permissions everything would work fine. If, however, I added a different role then it would reset the new role's dropdown to "Select or create a role" every time I change the permission - either adding, or removing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

priority:critical Unless this is fixed, we will not use the new admin ui type:enhancement New feature or request type:usability Improves the UX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Button text for series ACL changes is unclear and 2nd save button can't be deactivated

4 participants