-
Notifications
You must be signed in to change notification settings - Fork 22
Fix docs about "listed" behavior #1211
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
owi92
reviewed
Jul 12, 2024
b1079ab
to
70ddcbe
Compare
70ddcbe
to
114199a
Compare
114199a
to
e7e8964
Compare
owi92
reviewed
Jul 12, 2024
Our docs described a behavior that was implemented differently. This commit fixes this inconsistency. We discussed this in detail internally and believe the suggested behavior (which already happened to be implemented) is the most useful one for Tobira. An explanation of the three most discuss-worthy points: (1) Series do NOT become listed via a video-block of their videos: It is fine that only the event is findable, especially since "series title" is part of the event data in the search index, so you can find that event via the series title. And then the series is only one click away. So why not make the series findable directly as well? It's not necessary for UX, and would be a lot more complicated implementation-wise. (2) Events do NOT become listed via video-blocks of other videos in their series: For one, that might be unexpected to some users, especially because spooky action at a distance. But it also leads to user confusion elsewhere: if it were findable, then the "manage my events" page for that event should better also list the page responsible for that under "referencing pages", but then users might only see another event on that page, being confused. However, even if these events are not findable via search, they are "reachable", namely by finding another video in the series, and then clicking a couple of times. For that reason we adjusted the phrasing of our "unlisted" note (which is still hidden behind the experimental feature flag, so no user ever saw it yet). The new phrasing promises a bit less. (3) Events DO become listed via playlist-blocks of a playlist containing them: At first this seems like a bad idea as every user can add every event to all playlists they have write access to (for example: their own). So can every user make any event listed then? No: the playlist block still needs to be added to a non-user page, which requires a privilege that likely only few trusted users have. And if they have that privilege, they can just as easily add a video block with that video directly, so no one gets any additional powers for mischief. But wait: what if someone has write access to a playlist that is already mounted on a non-user page and they add a video there? Yes that is the other option on how to make the video listed, however: they also need access to such a playlist first. And note: even if we don't make the event findable via a playlist block: the event could still be reached by clicking around in the non-user pages! So it is already not hidden anymore. And finally: to add a video to a playlist, that person needs to have the ID (or a link) to that video in the first place. If they had malicious intends of making the video public, they might as well just e-mail the link around or something like that. So in summary: while it first sounds a bit dangerous, thinking it through shows that there are basically no way in which we give untrusted users more power to do unintended things. On the other hand, there are a few advantages of making events findable via playlist block. To users, a playlist block looks basically like a series block: the video with its thumbnail "is clearly on this page". Having different behavior for both could be confusing. Further, we would like to allow "video in context" pages, i.e. `/path/to/realm/v/<id>` for videos only mentioned via playlist in that realm. We also want to show a realm with a playlist block as a "referncing realm" in the "my videos" section, and we shouldn't show the "unlisted" note in case a playlist block exists for a video. With that, it's most consistent and easiest to explain if playlist blocks make events findable. --- Also, this is already the behavior in code! Only the docs were wrong about this. So this change isn't even breaking, it's just a doc fix.
e7e8964
to
feef9b3
Compare
👍 |
JulianKniephoff
added a commit
that referenced
this pull request
Aug 8, 2024
…er improvements (#1217) See commits for more info. The main motivation was to fix the last remaining spots disagreeing with #1211, as well as add trigger logic to automatically queue playlists and their events for reindex. The latter part is why this PR unblocks the next release. But since there were some minor bugs as well, and the logic was spread over many migrations, the easiest way to achieve this was to rewrite all triggering logic. Now everything is in one file and, I think, simpler and more future-proof. My hope is that with this PR, we won't have to touch the trigger logic or search views anytime soon. I added more fields to the search views and erred all triggers on the side of "queue more often than necessary". I also adjusted the unit tests checking this behavior and added new ones. Finally, I used this opportunity to get rid of the SQL query in `search::perform` by moving the event thumbnails into the series search index directly. Adding the SQL query was just a quick'n'dirty solution back then, since it would have required essentially this PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Text copied from the second commit:
Our docs described a behavior that was implemented differently. This
commit fixes this inconsistency. We discussed this in detail internally
and believe the suggested behavior (which already happened to be
implemented) is the most useful one for Tobira. An explanation of the
three most discuss-worthy points:
(1) Series do NOT become listed via a video-block of their videos:
It is fine that only the event is findable, especially since "series
title" is part of the event data in the search index, so you can find
that event via the series title. And then the series is only one click
away. So why not make the series findable directly as well? It's not
necessary for UX, and would be a lot more complicated
implementation-wise.
(2) Events do NOT become listed via video-blocks of other videos in
their series:
For one, that might be unexpected to some users, especially because
spooky action at a distance. But it also leads to user confusion
elsewhere: if it were findable, then the "manage my events" page for
that event should better also list the page responsible for that
under "referencing pages", but then users might only see another event
on that page, being confused. However, even if these events are not
findable via search, they are "reachable", namely by finding another
video in the series, and then clicking a couple of times. For that
reason we adjusted the phrasing of our "unlisted" note (which is still
hidden behind the experimental feature flag, so no user ever saw it
yet). The new phrasing promises a bit less.
(3) Events DO become listed via playlist-blocks of a playlist containing
them:
At first this seems like a bad idea as every user can add every event to
all playlists they have write access to (for example: their own). So
can every user make any event listed then? No: the playlist block still
needs to be added to a non-user page, which requires a privilege that
likely only few trusted users have. And if they have that privilege,
they can just as easily add a video block with that video directly, so
no one gets any additional powers for mischief. But wait: what if
someone has write access to a playlist that is already mounted on a
non-user page and they add a video there? Yes that is the other option
on how to make the video listed, however: they also need access to such
a playlist first. And note: even if we don't make the event findable
via a playlist block: the event could still be reached by clicking
around in the non-user pages! So it is already not hidden anymore. And
finally: to add a video to a playlist, that person needs to have the
ID (or a link) to that video in the first place. If they had malicious
intends of making the video public, they might as well just e-mail the
link around or something like that.
So in summary: while it first sounds a bit dangerous, thinking it
through shows that there are basically no way in which we give untrusted
users more power to do unintended things.
On the other hand, there are a few advantages of making events findable
via playlist block. To users, a playlist block looks basically like a
series block: the video with its thumbnail "is clearly on this page".
Having different behavior for both could be confusing. Further, we would
like to allow "video in context" pages, i.e.
/path/to/realm/v/<id>
forvideos only mentioned via playlist in that realm. We also want to show
a realm with a playlist block as a "referncing realm" in the "my videos"
section, and we shouldn't show the "unlisted" note in case a playlist
block exists for a video. With that, it's most consistent and easiest
to explain if playlist blocks make events findable.
Also, this is already the behavior in code! Only the docs were wrong
about this. So this change isn't even breaking, it's just a doc fix.