-
Notifications
You must be signed in to change notification settings - Fork 17
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 selection to uploader (and start maintaining a series search index) #705
Merged
owi92
merged 20 commits into
elan-ev:master
from
LukasKalbertodt:series-selection-uploader
Feb 15, 2023
Merged
Add series selection to uploader (and start maintaining a series search index) #705
owi92
merged 20 commits into
elan-ev:master
from
LukasKalbertodt:series-selection-uploader
Feb 15, 2023
Conversation
This file contains 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
There is a new Meili index which contains series data. It is updated and rebuild as with the events index. The index version was bumped so that everything us automatically rebuilt when updating to a newer version. Technically, it's wasteful as the realm and event indexes don't need to be rebuild. But since rebuild is fast, we can just do that to make sure admins don't forget to rebuild the series index manually.
This is especially useful when there are very few items in total. And it doesn't hurt if there are many.
In some situations, the generic solution fails.
Now it works directly with `tokio_postgres` which helps with tests.
For each test, a new temporary database is created to ensure isolation between tests. The database is destroyed after the test.
This improves the previous commit, making the single test cases smaller and checking all interesting cases (as far as I'm aware). This uncovered a couple of bugs which are fixed in subsequent commits. (I.e. some tests fail with only this commit.)
These were found in the previous commit by writing exhaustive tests.
We said that a series and all its videos are one unit and are all either listed or all unlisted. Specifically, if one video is listed, the series is also listed. It felt wrong to include those realms (where a video of the series was mounted) in `host_realms`. So now `listed` is not simply `!host_realms.is_empty()` anymore for series.
owi92
reviewed
Feb 14, 2023
owi92
reviewed
Feb 14, 2023
owi92
requested changes
Feb 15, 2023
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 think this can be merged once you have updated the translation. The other thing isn't really an issue for me.
Before, the cursor was a "text selection" cursor when hovering over text.
owi92
approved these changes
Feb 15, 2023
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.
Fixes #675
This PR:
search_series
DB view (which is used to populate the search index).search_index_queue
when necessary.SearchableSelect
work with series.SearchableSelect
to add a series selection to the uploader.Additionally, I added tests to make sure all those triggers work as expected. Those tests revealed that, in fact, those triggers didn't work as expected. So another migration was added to fix those bugs. See commit messages for more information. Probably review commit by commit. (Though the last two commits change previous code, be aware.)
As series are now indexed, we could make the main search now also return series. I would wait with that though until Meili allows multi-index search.
I noticed that in our test data, there aren't many series that have non-empty
write_roles
. So even Sabine can upload in just 2 series right now. But this "requires write permission to series" is the only sane condition we can use, and it was specified like that in #675.