Add Custom Sort Option to To-Read List (#71) - #322
Conversation
added backend methods and changes changed toread ui and added sortablejs removed personal use comments linting and test of method
|
Ok thank you for this contribution. |
|
Hi, The code looks good, my biggest concern is that you are loading the entire DB in memory to perform the custom sort. Also, do you have a mechanism to prevent collisions in sort order ? An user could modify the order in the UI while the order is also modified in DB using the API for example. What would happen with two books having the same order number ? |
| true, ownedAsBool.value, null, | ||
| pageAsNumber.value - 1, perPage.value, sortQuery.value) | ||
| true, ownedAsBool.value, null, | ||
| 0, pageSize, sortParam) |
There was a problem hiding this comment.
are you sure that the page number should be harcoded to 0 ? I think it should be 0 only when in custom sort mode. You need the same logic as above with pageSize and sortParam
| // method to update custom to-read list order | ||
| @Transactional | ||
| fun updateCustomToRead(sortOrders: Map<UUID, Int>) { | ||
| sortOrders.forEach { (userBookId, order) -> |
There was a problem hiding this comment.
we are looping over a potential big number of changes in a Transactional block.
Is there a way to persist the changes in DB in one SQL/one ORM call instead ?
2 participants
This closes issue #71
Changes Made
(Hello, this was done as part of a software engineering course, and part of it contributing to an OSS project of our choice. I don't expect it to be accepted but i would appreciate feedback if you have time and you want to. Thanks!)