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 playlists to sync code, DB & API and add playlist direct links #1159

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

LukasKalbertodt
Copy link
Member

@LukasKalbertodt LukasKalbertodt commented Apr 10, 2024

This is the first PR about playlists for Tobira. It requires opencast/opencast#5734 to be useful.

This does mainly two things:

  • Tobira now understands the playlist data that Opencast is sending via Harvest API and stores it in Tobira's DB.
  • Adds Playlists to Tobira's GraphQL API (without any mutations yet)
  • Add the two direct link routes so that playlists can be viewed given their ID. This required making the core "series block" code more generic to also work with playlists.

Two main features are still missing:

  • Playlist blocks (that can be placed on pages)
  • "My Playlists": management area to create, delete and edit playlists

For these two, I will create separate issues. Therefore:

Closes #937

Screenshots

Unfortunately, there are no playlists on the test deployment yet. So here are some screenshots:

image

image

image

As you can see:

  • Currently this looks very very similar to how series are displayed.
  • For playlists, there is a new order mode "Playlist order" which shows the videos in the order that the playlist specifies. This is the default of course. But users can still sort by date and title, if they so wish.
  • In the "Playlist order" mode, video that are missing or cannot be accessed are shown with a placeholder.
  • In any other ordering mode, only a small note is shown at the bottom. (Since we have no information about the missing items, we cannot properly sort them. They would be sorted all at the very end anyway.)

Open questions

  • Should the "missing items" note also be shown for series? So far we don't: we simply do not show the videos that the user has no read access to.
  • What about the specific formulations for the note and the missing video placeholders? Specifically:
    • German:
      missing-video: Video nicht gefunden
      unauthorized: Fehlende Berechtigung
      hidden-items_one: 'Ein Video wurde nicht gefunden oder Sie haben keinen Zugriff darauf.'
      hidden-items_other: '{{count}} Videos wurden nicht gefunden oder Sie haben keinen Zugriff darauf.'
    • English:
      missing-video: Video not found
      unauthorized: Missing permissions
      hidden-items_one: 'One video is missing or requires additional permissions to view.'
      hidden-items_other: '{{count}} videos are missing or require additional permissions to view.'
  • How should we call the "order as specified by the playlist" ordering mode? Currently I called it "Playlist order" (en) / "Wie Playlist" (de). Better ideas appreciated.

Can be reviewed commit by commit.

@LukasKalbertodt LukasKalbertodt force-pushed the add-playlists branch 2 times, most recently from a7b1948 to 8f64bb9 Compare April 10, 2024 15:09
@github-actions github-actions bot temporarily deployed to test-deployment-pr1159 April 10, 2024 15:12 Destroyed
@LukasKalbertodt LukasKalbertodt added the changelog:user User facing changes label Apr 10, 2024
@github-actions github-actions bot temporarily deployed to test-deployment-pr1159 April 11, 2024 07:20 Destroyed
@dagraf
Copy link
Collaborator

dagraf commented Apr 12, 2024

  • Should the "missing items" note also be shown for series? So far we haven't: we simply did not show the videos that the user has no write access to.

I guess you mean "... that the user has no read access to.", right?

For us (University of Bern): No. But since having the merge mode activated, all events of the same series have the same ACLs we might not be the right institution to answer this question.

How should we call the "order as specified by the playlist" ordering mode? Currently I called it "Playlist order" (en) / "Wie Playlist" (de). Better ideas appreciated.

What about: "Manually (Playlist order)" (en) / "Manuell (wie Playlist)" (de) oder nur "Manually/Manuell"?

@oas777
Copy link
Collaborator

oas777 commented Apr 16, 2024

Thanks, Lukas. I was surprised to see this:

  • But users can still sort by date and title, if they so wish.

Two things make me think we should not let them do this (at the moment):

  • to me, a playlist is a collection of videos someone has compiled, including a certain order; it's not for the end user to change that.
  • Currently, we don't even have "'My Playlists': management area to create, delete and edit playlists". Let's build that first, then allow users to mess with playlist orders.

As to your questions:

  • Missing item note for series: Yes, even though neither Bern nor Zürich need this (currently).
  • Wording: "Playlist. You have no right to change this." Or simply "Playlist" (DE/EN).

@LukasKalbertodt
Copy link
Member Author

I guess you mean "... that the user has no read access to.", right?

Yes, you are correct! Fixed.

Two things make me think we should not let them do this (at the moment):

  • to me, a playlist is a collection of videos someone has compiled, including a certain order; it's not for the end user to change that.
  • Currently, we don't even have "'My Playlists': management area to create, delete and edit playlists". Let's build that first, then allow users to mess with playlist orders.

I think there are two reasons I already implemented this:

  • There are two use cases for the feature we call "playlists": a literal playlist with definitive order and a "video collection", where order is not important. From the specification:

    Videos inside a playlist have a definitive order; it’s not just an unordered set of videos. Some use cases require ordering (e.g. teaching videos that depend on one another), that would not be adressed with an unordered set. Use cases which don't require ordering can just ignore it.

    While I agree that changing the order in the first case makes no sense, it is useful in the "collection" use case. And users can simply not change the order in the first use case. I don't really see how having this option can hurt users.

  • It was very easy to implement. In fact, it would have been almost more difficult to now let users change order in playlists, due to code structure reasons.


Regarding "missing note": Bern says "no", ETH says "yes", but both actually don't care. So I will just keep it as is and not invest time there now :P

@github-actions github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Apr 23, 2024

This comment was marked as resolved.

@oas777
Copy link
Collaborator

oas777 commented May 2, 2024

Let me get this straight: If I create a playlist, will users be able to change the order, thus making it a collection (from his/her perspective)? Or is there an alternative to create a collection?

@LukasKalbertodt
Copy link
Member Author

If I create a playlist, will users be able to change the order, thus making it a collection (from his/her perspective)?

Yes, they can locally change the display order only for themselves.

@oas777
Copy link
Collaborator

oas777 commented May 8, 2024

And your reasoning is that playlists are only a suggestion to watch in a given order anyway?

@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label May 15, 2024
@github-actions github-actions bot temporarily deployed to test-deployment-pr1159 May 15, 2024 12:17 Destroyed
@LukasKalbertodt
Copy link
Member Author

Rebased, solving the merge conflicts. Remaining open points:

Wording playlist order

  • Currently: "Playlist order" (en) / "Wie Playlist" (de)
  • David:

    What about: "Manually (Playlist order)" (en) / "Manuell (wie Playlist)" (de) oder nur "Manually/Manuell"?

  • Olaf:

    simply "Playlist" (DE/EN).

I am not a big fan of David's long suggestion, simply because they are pretty long. I also don't really like "manually" as it's note immediately clear who did/can do something manually where. Just "Playlists" is fine, but I think the current version is slightly better as it suggests being about "ordering" in itself, even without opening the menu and seeing the "Order" heading. So I would just keep the current version for now, if no one objects? Please note though that my opinion is not strong at all here, and I gladly change it if someone with a stronger opinion speaks up.


And your reasoning is that playlists are only a suggestion to watch in a given order anyway?

We discussed this in person and concluded that indeed it doesn't hurt to let users change the order for themselves in this case.


Assuming no one objects to the points above, I think this is ready. I will now try to update our test Opencast to support playlists.

@github-actions github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label May 16, 2024

This comment was marked as resolved.

This means that Tobira will store all playlist info already when
syncing.

I thought about the DB "array vs. table" question for some time. Here
are my notes that made me decide for the array solution:


Should the entries be stored as an array in a column of the `playlists`
table? Or be a separate table that needs to be joined? The latter would
be the traditionally "better" version, as it normalizes the data.

A playlist entry contains these fields: 
- Opencast ID, 
- Type (e.g. video)
- Content ID (e.g. Opencast video ID). 

If we store it in a separate table we also need the following fields:
- Playlist: foreign key to playlist ID
- Index: ordering in a table is not well defined, so to get a definite
  order, we need another index field.
- Potentially another artificial ID?



- Normalization is what you're supposed to do, right?
- Allows to use LIMIT and OFFSET, i.e. only load part of all entries.
  (Kind of possible with arrays as well though).
- Allows to modify/delete individual entries without rewriting all of
  them

- Many situations treat the entries as one atomic value:
    - Harvest/sync: the API sends the full playlist object including
      entries. If using an extra table, we would have to delete all old
      entries there, then insert all new ones.
    - Playlist block: loads all entries. Ok but what about pagination:
      we don't have that yet. But we should at some point? 
    - Even in the modifying/moving/deleting a single entry case: 
        - Opencast's APIs require us to send all entries, so while we
          can do very localized updates in our DB, we have to load all
          entries anyway.
        - After changing something, the Harvest API will send an update
          which leads to overwriting all entries again.
- Extra table requires at least two additional fields to be stored
  (foreign key & index)
- Management of `index` adds complexity whenever somehow editing
  entries. Just writing all entries at once again is easier. Also:
  should there be DB checks that make sure the indices are always 0 to
  N like for blocks? Adds more complexity and we ran into problems with
  that once.
- Even if we add pagination: apart from offering a different UX,
  pagination helps with performance in four spots: rendering,
  backend<->frontend communication, DB<->backend communication, DB
  loading from disk. The first three benefits we can still have, even
  with arrays.
- Looking at how PostgreSQL stores rows, arrays shouldn't incur any
  meaningful overhead and might even be faster in the majority of
  situations.
We did not have types depending on other types before. Adding "cascade"
luckily fixes this.
I only moved stuff around and fixed imports, that's all. I did that so
that the reviewer can just skip this commit. It is preparation for
making a general `VideoListBlock`.
Again, preparation for using that for playlists as well
This was added a long time ago. By now, the ordering is done in the
frontend, so the parameter is completely unused. We can get rid of some
complexity by removing it.
This only adds querying capabilities, no mutations yet.
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.

Implement Playlists (once Opencast has them)
3 participants