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.

Sign up for GitHub

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

Jump to bottom

Add config to lock video ACLs to their series #1305

Open
wants to merge 3 commits into
base: next
Choose a base branch
from
Open

Add config to lock video ACLs to their series #1305

wants to merge 3 commits into from

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Dec 16, 2024

This will disable ACL editing for the uploader and the ACL UI when the video is part of a series.
Uploaded videos will then adopt the ACL of that series.

This needs to be configured with lock_acl_to_series, the default for this is false.

Closes #1006

@owi92 owi92 added the changelog:user User facing changes label Dec 16, 2024
@owi92 owi92 changed the base branch from main to next December 16, 2024 11:25
@github-actions GitHub Actions

This comment has been minimized.

Sign in to view
@github-actions github-actions bot temporarily deployed to test-deployment-pr1305 December 16, 2024 11:33 Destroyed
@github-actions github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Dec 16, 2024
@github-actions GitHub Actions

This comment was marked as resolved.

Sign in to view
@owi92 owi92 force-pushed the acl-upload-config branch from 3d53a9f to 119ad94 Compare December 16, 2024 13:46
@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Dec 16, 2024
@github-actions github-actions bot temporarily deployed to test-deployment-pr1305 December 16, 2024 13:51 Destroyed
@LukasKalbertodt LukasKalbertodt mentioned this pull request Dec 18, 2024
Closed
@owi92 owi92 modified the milestone: v3.0 Dec 18, 2024
LukasKalbertodt
LukasKalbertodt requested changes Jan 15, 2025
View reviewed changes
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

I tested it and it works very well, no problems found there!

I found a few minor code things, but unfortunately also a bigger one (due to a n+1 query problem). That will require a bit of frontend change as well, but shouldn't be too terrible.

backend/src/config/general.rs Outdated Show resolved Hide resolved
backend/src/api/model/search/series.rs Outdated Show resolved Hide resolved
frontend/src/routes/manage/Realm/RealmPermissions.tsx Outdated Show resolved Hide resolved
frontend/src/routes/Upload.tsx Outdated Show resolved Hide resolved
frontend/src/routes/Upload.tsx Outdated Show resolved Hide resolved
frontend/src/routes/Upload.tsx Outdated Show resolved Hide resolved
frontend/src/routes/manage/Video/Access.tsx Show resolved Hide resolved
@owi92 owi92 force-pushed the acl-upload-config branch from 119ad94 to b0490c8 Compare January 15, 2025 23:35
LukasKalbertodt
LukasKalbertodt requested changes Jan 16, 2025
View reviewed changes
frontend/src/routes/Upload.tsx
Comment on lines +673 to +679
const SeriesAclQuery = graphql`
query UploadSeriesAclQuery($seriesId: String!) {
series: seriesByOpencastId(id: $seriesId) {
acl { role actions info { label implies large } }
}
}
`;
Copy link
Member

Choose a reason for hiding this comment

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

Why by Opencast ID? 🤔 We do have the normal ID (maybe it has the wrong prefix, but thats easily fixed), no?

Copy link
Member Author

@owi92 owi92 Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Hm we need the opencast ID anyway for the ingest, so I didn't think it necessary to also pass the regular id around.

Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah I guess it doesn't really matter

frontend/src/routes/Upload.tsx
Comment on lines +704 to +708
const data = await fetchQuery<UploadSeriesAclQuery>(
environment,
SeriesAclQuery,
{ seriesId }
).toPromise();
Copy link
Member

Choose a reason for hiding this comment

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

I think useQueryLoader is more idiomatic here? :/ But mhhh not sure if it's worth it changing that. It would also change the UX a bit: the ACL UI would be completely hidden while loading instead of showing the old value. Not sure if thats better or worse. So yeah no hard opinion, we can probably just keep it...

frontend/src/routes/Upload.tsx Outdated

if (!data?.opencastId) {
setLockedAcl(null);
resetField("acl");
Copy link
Member

Choose a reason for hiding this comment

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

Do we really want to reset it in this situation? I would be fine if the ACL that were previously configured would re-eappear when the series is cleared. Not sure whats more useful...

Edit: oh especially when CONFIG.lockAclToSeries is not set, then this means that my configured ACLs (which I was able to edit with a series selected) get discarded when I clear the series, right?

@github-actions github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Jan 17, 2025
@github-actions GitHub Actions
Copy link

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

owi92 added 2 commits January 17, 2025 16:37
@owi92
Add config option to lock event ACL to their series
a53337b 
With this enabled, event ACLs can't be edited and will generally
be determined by their series.
@owi92
Add ACL information to series API
a4bff75
@owi92 owi92 force-pushed the acl-upload-config branch from b0490c8 to 34f9159 Compare January 17, 2025 15:45
@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Jan 17, 2025
@owi92
Show series ACL in uploader when configured
b3a10b3 
And then also disable ACL editing and attachment.
In a previous iteration, this was using a suboptimal
approach where each series returned by the API would
send an additional query to get the series ACL.
Now this only sends a single additional query whenever
a series is actually selected. While this requires
more code, a good amount of it is just state management
and error handling/reporting. The rest is related to
the data fetching.
@owi92 owi92 force-pushed the acl-upload-config branch from 34f9159 to b3a10b3 Compare January 17, 2025 15:45
@github-actions github-actions bot requested a deployment to test-deployment-pr1305 January 17, 2025 15:49 In progress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LukasKalbertodt LukasKalbertodt LukasKalbertodt requested changes

Assignees
No one assigned
Labels
changelog:user User facing changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The visibility selection to control ACL in the uploader should be configurable
2 participants
@owi92 @LukasKalbertodt