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

Allow tracks in event asset upload #698

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Jun 17, 2024

Old logic dictates that media of type track cannot be in the asset upload tab of the event details or new event wizard. Since subtitles are now tracks, this logic finally breaks.

Instead of relying on checking for type track to differentiate between options for source upload and asset upload, this commit instead uses the key in the list provider that is there for this exact purpose.

To review this, you will need an Opencast with Asset Upload configured. Check the Opencast docs on how to do that https://docs.opencast.org/develop/admin/#configuration/admin-ui/asset-upload/#manual-asset-upload. This changes behaviour for both the event details and the create event dialog.

Fixes #694.

Edit: Furthermore, this also adds support for showForNewEvents and showForExistingEvents, two keys that specifically specify whether an asset upload option should show up for new events or existing events. Previously, an asset upload option would always show for new and existing events, but now this can be configured in detail.

Old logic dictates that media of type track
cannot be in the asset upload tab of the event
details or new event wizard. Since subtitles
are now tracks, this logic finally breaks.

Instead of relying on checking for type
track to differentiate between options
for source upload and asset upload, this
commit instead uses the key in the list
provider that is there for this exact purpose.
@Arnei Arnei added the type:bug Something isn't working label Jun 17, 2024
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-698

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

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
Contributor

github-actions bot commented Jun 17, 2024

This pull request is deployed at test.admin-interface.opencast.org/698/2024-08-09_12-23-28/ .
It might take a few minutes for it to become available.

Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Jul 1, 2024

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

@Arnei Arnei force-pushed the allow-tracks-to-show-up-in-asset-upload branch from 6200751 to 1c411b6 Compare July 1, 2024 07:34
Copy link
Contributor

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

Copy link
Contributor

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

Copy link
Contributor

github-actions bot commented Aug 9, 2024

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

Copy link
Contributor

github-actions bot commented Sep 5, 2024

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

@snoesberger
Copy link

I have tested this PR with our test environment. I now have two steps in the event creation dialogue where I can upload a subtitle file:
grafik
grafik

If I add different subtitle files in both steps, only the one in the second step is added to the event. It should only be possible to add a subtitle file at one point in the create event dialogue.

Adding a subtitle file to an existing event in the event modal works fine.

@mtneug
Copy link
Member

mtneug commented Oct 14, 2024

I still favour adding subtitles on the second page so I would like to have the option to configure Opencast in that way. But having the same file picker on two pages is obviously wrong.

@JulianKniephoff
Copy link
Member

Just nagging @Arnei here that CI is broken since your last conflict resolution. Should be an easy fix.

Copy link
Contributor

github-actions bot commented Nov 5, 2024

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

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

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
Contributor

github-actions bot commented Nov 5, 2024

This pull request is deployed at test.admin-interface.opencast.org/698/2024-11-05_10-22-07/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member Author

Arnei commented Nov 12, 2024

Excuse the late response to the reviews.

It should only be possible to add a subtitle file at one point in the create event dialogue.

I'm inclined to disagree. What is shown in the Source and Asset Upload step for new events is in the end based on etc/listproviders/events.upload.asset.options.properties in Opencast, meaning it is based on user configuration. If the user wants to have subtitle uploads in both steps, I set let them, lest they be confused why their configuration does not work.

I do agree though that it should be possible to configure subtitles in the asset upload for existing events, without also adding them to the asset upload step for creating new events. To that end it would make sense to me to extend the etc/listproviders/events.upload.asset.options.properties to allow for differentiating between new and existing events. Currently there is only EVENTS.EVENTS.NEW.UPLOAD_ASSET.OPTION.{name}, which is applied to both new and existing events. We could add EVENTS.EVENTS.EXISTING.UPLOAD_ASSET.OPTION.{name}. What do you think @snoesberger @mtneug ?

If I add different subtitle files in both steps, only the one in the second step is added to the event.

That I would consider a bug, I would expect both files to be added. Took a quick look, and this seems to be an issue in the backend for multiple files with the same flavor. Personally, I would consider this bug a seperate issue from this PR as this PR did not add the bug, merely brought it to light. But I'm open to persuasion :)

@mtneug
Copy link
Member

mtneug commented Nov 12, 2024

So you would have:

  • EVENTS.EVENTS.NEW.UPLOAD_ASSET.OPTION = new and existing events
  • EVENTS.EVENTS.EXISTING.UPLOAD_ASSET.OPTION = existing events
  • EVENTS.EVENTS.NEW.SOURCE.UPLOAD = new events, but on the first page

I find this confusing. What about having only one key prefix for everything and adding properties to the JSON that determine where this gets included?

{
  ...
  "group": "main", // main = first page, secondary = second page, but I'm not tight to this property
  "showForNewEvents": true,
  "showForExistingEvents": false,
  ...
}

@Arnei
Copy link
Member Author

Arnei commented Nov 12, 2024

My idea was that keys with NEW would only be for new events, and keys with EXISTING would only be for existing events. So EVENTS.EVENTS.NEW.UPLOAD_ASSET.OPTION would then only be for new events, not new and existing.

But I can still see why that would be confusing. Adding properties to the JSON also seems good.

@snoesberger
Copy link

Ok, I hadn't in mind being able to configure the behaviour of the asset upload.

I do agree though that it should be possible to configure subtitles in the asset upload for existing events, without also adding them to the asset upload step for creating new events.

I also agree with this and would prefer the solution with the properties in JSON format suggested by @mtneug as I think it is easier to understand.

That I would consider a bug, I would expect both files to be added. Took a quick look, and this seems to be an issue in the backend for multiple files with the same flavor. Personally, I would consider this bug a seperate issue from this PR as this PR did not add the bug, merely brought it to light. But I'm open to persuasion :)

I agree, we should create a separate issue for this.

Adds support for `showForNewEvents` and
`showForExistingEvents`, two keys that
specifically specify whether an asset upload
option should show up for new events or
existing events. Previously, an asset upload
option would always show for new and
existing events, but now this can be configured
in detail.
Copy link
Contributor

github-actions bot commented Dec 3, 2024

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

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

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
Contributor

github-actions bot commented Dec 3, 2024

This pull request is deployed at test.admin-interface.opencast.org/698/2024-12-03_09-17-53/ .
It might take a few minutes for it to become available.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upload subtitle for existing events via Asset Upload not possible
4 participants