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 popularity sorting options for books #5132

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

fuszti
Copy link

@fuszti fuszti commented Jul 19, 2024

Now the user can sort the /books by the view_count. It is the #1712 feature request.

Unit tests are added too.

Now the user can sort the /books by the view_count. It is the BookStackApp#1712
feature request.

Unit tests are added too.
@fuszti
Copy link
Author

fuszti commented Jul 22, 2024

@CodiumAI-Agent /review

@CodiumAI-Agent
Copy link

CodiumAI-Agent commented Jul 22, 2024

PR Reviewer Guide 🔍

(Review updated until commit 5d17e99)

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Key issues to review

Possible Bug
The method applySortOptions does not handle the case where the sort option is not recognized. This could lead to unexpected behavior or errors if an invalid sort option is provided.

Code Clarity
The method getListOptions and applySortOptions could benefit from additional inline comments explaining the logic, especially how sorting preferences are applied and handled.

@ssddanbrown
Copy link
Member

Thanks for offering this @fuszti and sorry for the late response. Personally I'm not too convinced of the value of adding this feature, since there's been little desire for this so far and we already list a few of the most popular books in this view. Plus if we add this here it would also be expected in the shelves view, adding a little complexity there too.

@CodiumAI-Agent
Copy link

Persistent review updated to latest commit 5d17e99

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

Successfully merging this pull request may close these issues.

3 participants