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

Add series management pages #1313

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

Add series management pages #1313

wants to merge 13 commits into from

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Jan 15, 2025

Step 2 in adding proper series management.
Adds series details and acl pages, making sure all new and/or refactored components can also be used for playlists later on.

Based on #1311

Step 3 will then be more API related stuff, adding creation and content editing.

Edit: I added metadata editing here, and plan on also adding the delete function in this pull request. So, todos for this PR:

  • add delete function and UI (there are still some things to adjust)
    • investigate deserialization error (could not reproduce anymore)
    • fix translation/i18n
  • disable acl editing when series is not synced yet
  • list host realms
  • properly rebase this onto Add "My series" page #1311 again, but that is pending a review anyway so I'll do that last
    • This is currently broken Need to fix 'unwrapped an unloaded LazyLoad' error with series thumbnail on details page.*
    • * I fixed the above, but maybe we shoud make sure that each place that needs a series thumbnail uses the same event order for it.
  • Fix series deletion. I already did, but messed up and lost the fixed version during my rebase.

@Reviewer (is that a real user??): Once again, this is unfortunately probably better to review file-by-file, though it might still be useful to read the commit messages.

@owi92 owi92 changed the base branch from main to next January 15, 2025 10:38

This comment has been minimized.

@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 January 15, 2025 10:45 Destroyed
Copy link

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

@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 bot removed the status:conflicts This PR has conflicts that need to be resolved label Jan 21, 2025
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 January 21, 2025 22:12 Destroyed
@dagraf
Copy link
Collaborator

dagraf commented Jan 22, 2025

@owi92 I like seeing the progress here! 😍

@owi92 owi92 added the changelog:user User facing changes label Jan 22, 2025
Copy link

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

@github-actions github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Jan 27, 2025
@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Jan 27, 2025
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 January 27, 2025 19:40 Destroyed
@owi92 owi92 force-pushed the series-details branch 2 times, most recently from 0380207 to f86eb3f Compare January 27, 2025 21:34
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 January 27, 2025 21:37 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 January 28, 2025 17:05 Destroyed
@owi92 owi92 mentioned this pull request Jan 28, 2025
3 tasks
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 3, 2025 22:37 Destroyed
@github-actions github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Feb 4, 2025
Copy link

github-actions bot commented Feb 4, 2025

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

@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Feb 5, 2025
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 6, 2025 22:41 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 10, 2025 10:45 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 24, 2025 21:01 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 24, 2025 22:02 Destroyed
This generalizes the `Nav` and `Details` code that
was used for videos, and repurposes it for series
as well.

With these changes, it should also be fairly
easy to add this for playlists later on.
@owi92 owi92 marked this pull request as ready for review February 24, 2025 22:11
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 24, 2025 22:13 Destroyed
@owi92 owi92 marked this pull request as draft February 24, 2025 22:15
@owi92 owi92 marked this pull request as ready for review February 24, 2025 23:41
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 24, 2025 23:42 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 25, 2025 23:43 Destroyed
@owi92 owi92 force-pushed the series-details branch 2 times, most recently from 8b9ce95 to 30e56a9 Compare February 26, 2025 15:50
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 26, 2025 15:55 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1313 February 26, 2025 16:58 Destroyed
owi92 added 10 commits February 28, 2025 16:24
This required a bunch of changes:
- The `update_acl` endpoint which talks to Opencast
  was generalized to work for both events and series,
  as their acl `put` endpoints pretty much work the same.
- The access page of events was refactored and most code
  is now usable for both events and series. This tries to
  walk the thin line between modularity and overspecialization
  by attempting to balance out reusablility and complexity,
  limiting both duplicated code and prop drilling.
This adds a somewhat generic function that can be used
to send `update metadata` put requests to Opencast for
both events and series, as their respective endpoints
only differ in one string (i.e. `series` vs `events`).

For now, the function is only used to update title and
description, but it can be used for any kind of dublincore
metadata.
This refactors the metadata form to be more generic. Again,
it's a tradeoff between limiting duplication and making the
code more complicated.

I also changed the naming convention of the series/video
details and access pages to use more specific names, which
will hopefully be less confusing.
This repurposes the `delete` endpoint previously used
by events, and basically mirrors everything done when
an event is deleted.

This means the series table is renamed to `all_series`
and gets a field `tobiraDeletionTimestamp`, which is
used to mark a series as deleted. The original series
table is moved to a view.

When a series is deleted in Tobira, the timestamp is saved
and series marked as deleted, but still present in Opencast
get a special entry in the `My series` table. Every other
place uses the series view without deleted series.

When the series is actually deleted in Opencast and removed
from the DB with sync, the entry from the `My series` table
also disappears.
This, once again, is a bunch of generalization to leverage
the fact that we already have basically all that is needed
for this.
While that makes it sound relatively simple, this was still
a lot of work, mostly to not just copy everything but factor
out as much as possible and make it usable for both events and
series, and possibly also playlists in the future.
There are still some things that need ironing out:
- There is a deserialization error where postgres complains
about a null value
- The translation/i18n doesn't seem to be working correctly for all
cases
This page uses the `series_by_id` api, which doesn't
use the new `thumbnailStack` field. It could, but that would
need some additional mapping in backend and should then
probably also be used in all other places that need
a series thumbnail.
That would have the added benefit of these thumbnails always
having the same order everywhere. Right now, that's not given
since the only place the `thumbnailStack` is used as of now
orders them in backend by their creation date (ascending),
which we don't do for the other occurences that are mapped in
frontend.
So this is super annoying. The endpoint in opencast will
start `republish metadata` workflows for each event in the
series. This takes roughly 10 second **per video**.
Not in parallel but.. in.. series (pun intended? idk).

So long story short, it could take several minutes to return,
at which point it probably just responds with a `503`.
The series is still being deleted though, so I don't know how
to handle that case.

Anyway, this workaround will send the request, mark the series
as deleted and then just return without waiting. If it fails,
it will **eventually** become apparent, but until then it's just
showing as `deletion pending`, or sth. Actually, depending on
the configured interval, it will already show `deletion failed`
after a couple of minutes or sooner, when the "deletion process"
is still running in Opencast. So all in all a poor solution but
idk what else to do here other than an Opencast rewrite™.
This could also be used for other purposes, but is
currently only used for displaying a message on
the "My series" table when a series gets deleted.
The notification will clear when the user navigates away
from the table.

This commit also fixes the table layout on narrow screens.
Previously, the table would squish the "Title" column
and overlap it with the one right next to it. Now each
column uses a fixed width on smaller screens and adds
a scrollbar, it it gets too crammed.
All this did was provide a timeout option and some states
that weren't even used in all cases. That really wasn't
worth the added complexity.

The timeout was moved to the `SubmitButtonWithStatus` and
when the states are really needed, they can be added in
the parent component that uses them.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants