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

Implement "Random" as a sorting option, add standalone "Random" page #109

Merged
merged 7 commits into from
Apr 29, 2024

Conversation

DarthNerdus
Copy link
Contributor

Inspired by the "Random" page segment in Calibre-Web, I really enjoy rediscovering items in my list randomly. To that end, this PR implements "Random" sort.

  • API additions
    • Implements a convenience querybuilder leveraging RANDOM() to append to queries
    • Implements a convenience function to determine whether the passed sort order is "random"
    • Using this check, it will bypass the default parseSort method and directly apply the sort order
    • It further limits the query to a single page size
  • Page Changes
    • Adds "Random" as a sort option to:
      • Book List
      • To Read list
      • Shelves (tag) pages
    • Adds a standalone Random page, constrained to only Random sort

@bayang
Copy link
Owner

bayang commented Apr 19, 2024

Thank you ! That looks cool.
Give me a few days to review it please.

@DarthNerdus
Copy link
Contributor Author

Awesome! I just pushed a small change to remove the pagination from the Random page--a single set is all that's needed as it randomly regenerates every reload.

@@ -90,6 +94,18 @@ fun formatLike(input: String): String {
return "%$input%"
}

// Implement function for SQL RANDOM()
object RandomOrder : Function<Long>(LongColumnType()) {
override fun toQueryBuilder(queryBuilder: QueryBuilder) = queryBuilder { append("RANDOM()") }
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry I suck at sql, where does the random() function comes from ?
Is it sqlite random() ? https://www.sqlite.org/lang_corefunc.html#random
Does it work well with the Exposed framework ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your question prompted me to revisit my research. The documentation/threads I found called out that RANDOM had to implemented via direct SQL command. Since then, there's actually a Random class available in Exposed. I've pushed changes to adopt this instead.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok that looks cool, I'll have a deeper look later. Thank you for looking into this.

@DarthNerdus
Copy link
Contributor Author

I further reverted the PageRequest changes as the paginator is suppressed on the Random page anyways. This cleans up the changes.

@bayang
Copy link
Owner

bayang commented Apr 25, 2024

We will need tests for the backend here as well.

@DarthNerdus
Copy link
Contributor Author

I'm curious -- how would you propose that? We'd need to populate the test harness DB with multiple books and can query them, but there's always the risk that two concurrent requests return the same result set/order (especially if there's a small number of results).

I'm happy to implement something, but testing a Random sortby will require a lot more standup and there is always that chance. I can't think of anything that wouldn't be mitigating a low chance that the test fails -- which will always be there.

@bayang
Copy link
Owner

bayang commented Apr 25, 2024

For this use case I would go for the sanity check rather than testing that we actually get random results.
We want to make sure we get a list of results even when we use the random function.
This is to make sure that if one day a breaking change happens in the Exposed framework for example we have a test that will fail.
That would be enough for me in this situation.
What do you think of that ?

@DarthNerdus
Copy link
Contributor Author

That's reasonable -- I'll look to mock that in tonight.

@DarthNerdus
Copy link
Contributor Author

Alright, tests are up!

@bayang bayang merged commit 186f8f0 into bayang:main Apr 29, 2024
3 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 29, 2024
## [0.56.0](v0.55.0...v0.56.0) (2024-04-29)

### Features

* add Random page and random sorting option ([#109](#109)) ([186f8f0](186f8f0))
@bayang
Copy link
Owner

bayang commented Apr 29, 2024

🎉 This PR is included in version 0.56.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants