-
Notifications
You must be signed in to change notification settings - Fork 25
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
All metadata should be in the metadata step of the add event wizard #691
base: main
Are you sure you want to change the base?
Conversation
Use Run test server using develop.opencast.org as backend:
Specify a different backend like stable.opencast.org:
It may take a few seconds for the interface to spin up. |
fcf6484
to
da10033
Compare
This pull request is deployed at test.admin-interface.opencast.org/691/2024-07-23_23-06-17/ . |
da10033
to
9e75fb8
Compare
Two other fields missing are "location" and "duration" (used for calculating the end date in the Admin UI). I set this for file uploads if I know these files are from a recording (e.g. CA did the recording, but editing was done locally). Is this in the scope of this PR, or should I create another issue? |
You mean, you want these two additional fields in the recording metadata section? I think that's unrelated enough to not wait for that when merging this pull request. Nevertheless, I'll try taking a peek at the issue since I now kind of know the code and I hope it's not too hard to do. |
Fair. I created issue #742 to track this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works. But I'm not totally happy with it ^^'.
Does the start date field require special attention? If not, I would prefer if start date was grouped together with the other metadata. Having it in its own special box might set wrong expectations, i.e. "Is 'recording metadata' a different metadata catalog from 'metadata'?".
src/components/events/partials/ModalTabsAndPages/NewMetadataPage.tsx
Outdated
Show resolved
Hide resolved
src/components/events/partials/ModalTabsAndPages/NewSourcePage.tsx
Outdated
Show resolved
Hide resolved
/** | ||
* This component renders the metadata page for new events and series in the wizards. | ||
*/ | ||
const NewSeriesMetadataPage = <T,>({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This component is a lot of code duplication just because we want to add start date to the event metadata tab. I would prefer if we could avoid this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've tried that first, but there was more special handling between both since not only the metadata is different, but also the control elements. At the ed it seemed to complicated to me and I gave up. So… yes, I at least thought about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sooo we merge this as is and leave the consolidation of the two components to someone else? :D
6823a7e
to
39bab94
Compare
Fixed the minor issues and merged the different metadata sections. Now they shouldn't seem like different catalogs any longer. |
39bab94
to
d3aefbe
Compare
Can confirm, looks good to me now. I was going to complain that we should not need extra code for rendering start_date, as it is part of the standard metadata catalog and therefore should be able to be rendered just like any other metadata field. But it turns the reason we cannot do that is ultimately the backends fault, as the relevant endpoint (admin-ng/event/new/metadata) explicitly excludes start_date (among other fields). Whether or not this is reasonable likely requires additional consideration, so I'm ok with leaving this as it is for now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you schedule a new event and have asset upload configured, the "Previous" button on the metadata tab does nothing. It should bring me back to the "Source" tab.
d3aefbe
to
2e7c812
Compare
Rebased to resolve merge conflicts and fixed the next/previous problem. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can confirm, it is fixed.
Did find another small issue while testing though, sorry :)
The validation for the main metadata is not happening anymore. Effectively this means you could create an event without a title.
The “source” step of the add event wizard also contains metadata. This doesn't really make sense since there is a whole step dedicated to metadata. We should move “Start date” in there. - The problem: This field should only be shown for a file upload. Not for a scheduled event. - The solution: Make “Source” step 1 and metadata step 2. We then know if we need a start date once we are in the metadata tab. This fixes opencast#487
This patch removes the special recording metadata section and merges it into the default metadata to avoid the appearance that these are separate metadata catalogs.
2e7c812
to
4a4ec31
Compare
Rebased to fix resolve conflicts.
Fixed. |
Can confirm, validation is happening again. But the "Previous" button on the metadata tab does nothing again. Seems to only occur if I want to schedule an event. (Edit: Since my mouse is missing from the recording: The one time I jump back to source, I used the circle buttons instead of the "Previous" button) |
This pull request has conflicts ☹ |
The “source” step of the add event wizard also contains metadata. This doesn't really make sense since there is a whole step dedicated to metadata. We should move “Start date” in there.
This fixes #487