BookStack icon indicating copy to clipboard operation
BookStack copied to clipboard

Add popularity sorting options for books

Open fuszti opened this issue 1 year ago • 4 comments
trafficstars

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

Unit tests are added too.

fuszti avatar Jul 19 '24 20:07 fuszti

@CodiumAI-Agent /review

fuszti avatar Jul 22 '24 08:07 fuszti

PR Reviewer Guide 🔍

(Review updated until commit https://github.com/BookStackApp/BookStack/commit/5d17e991b64a615b281395bcc165e01d23b339c7)

⏱️ 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.

CodiumAI-Agent avatar Jul 22 '24 08:07 CodiumAI-Agent

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.

ssddanbrown avatar Sep 14 '24 12:09 ssddanbrown

Persistent review updated to latest commit https://github.com/BookStackApp/BookStack/commit/5d17e991b64a615b281395bcc165e01d23b339c7

CodiumAI-Agent avatar Sep 14 '24 12:09 CodiumAI-Agent

I'm going to go ahead and close this off as per my comment above.

ssddanbrown avatar Dec 05 '24 14:12 ssddanbrown

Oh wow, I somehow missed your previous comment. Thank you for responding to my PR. I understand your point, no worries.

fuszti avatar Dec 05 '24 14:12 fuszti