Add shelves column in shelf table #2562
Conversation
| <td data-title="{% trans "Shelves" %}"> | ||
| {% spaceless %} | ||
| {% for shelf in shelves %} | ||
| {% if shelf in book.shelves.all %} |
There was a problem hiding this comment.
I think book.shelves will produce all shelves for all users, rather than the shelf related to the current user
There was a problem hiding this comment.
I think that it only displays the shelves for the user (see screenshot below: same book on different shelves for different users). Is there another way to test what book.shelves produces?
There was a problem hiding this comment.
Oh, we're actually both sort of right -- book.shelves.all does produce all shelves for all users, but because it's checking if a Shelf object is present, and a Shelf is tied to a user, it still displays correctly, so this does work exactly how you intended.
My new qualm is that on a large instance, that's going to be a costly lookup, since there are a lot of shelves. It may be that assigning book.shelves.all to a variable using {% set ... %}, so that the query is only called once rather than once per shelf, will be enough mitigation?
|
@joachimesque Are you still there and have resources to take a look at the review? |
|
Sorry, I haven’t had the time (personal matters). Please don’t hesitate to deal with this PR as you see fit, I don’t know if its changes are still relevant :)
Le dim. 16 avr. 2023 à 17:56, Jascha Ezra Urbach ***@***.***> a écrit :
… ***@***.***(https://github.com/joachimesque) Are you still there and have resources to take a look at the review?
—
Reply to this email directly, [view it on GitHub](#2562 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AANS2TUYTWU6IPT5ARNLFK3XBQJETANCNFSM6AAAAAATOIY76M).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
@joachimesque All good, thanks for letting us know and all the best! |
|
@mouse-reeve @jaschaurbach The PR is open for another look :) |
|
Ah, I see what you mean, I hadn’t thought of it! I’ll try that out. What would be the best way to test the cost in terms of performance?
… Le 2 août 2023 à 04:15, Mouse Reeve ***@***.***> a écrit :
@mouse-reeve commented on this pull request.
---------------------------------------------------------------
In [bookwyrm/templates/shelf/shelf.html](#2562 (comment)):
> @@ -189,6 +192,19 @@ <h2 class="title is-3">
{% include 'snippets/stars.html' with rating=book.rating %}
</td>
{% endif %}
+ {% if shelves|length > 0 %}
+ <td data-title="{% trans "Shelves" %}">
+ {% spaceless %}
+ {% for shelf in shelves %}
+ {% if shelf in book.shelves.all %}
Oh, we're actually both sort of right -- book.shelves.all does produce all shelves for all users, but because it's checking if a Shelf object is present, and a Shelf is tied to a user, it still displays correctly, so this does work exactly how you intended.
My new qualm is that on a large instance, that's going to be a costly lookup, since there are a lot of shelves. It may be that assigning book.shelves.all to a variable using {% set ... %}, so that the query is only called once rather than once per shelf, will be enough mitigation?
—
Reply to this email directly, [view it on GitHub](#2562 (comment)), or [unsubscribe](https://github.com/notifications/unsubscribe-auth/AANS2TXIS32UYTW7G5SNGXTXTGZ4TANCNFSM6AAAAAATOIY76M).
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Closes #2484