Example: We have a library with a biblio that has 39000ish issues in the history, this page cannot be loaded within the plack/apache timeouts, and users can DOS themselves trying to see it. This is admittedly a very extreme case, but even bibs with fewer issues in the history would probably benefit from this page having better performance.
Created attachment 126626 [details] [review] Bug 29275: Add $patron_to_html to render patron information in JS Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126627 [details] [review] Bug 29275: Use the API to fetch checkouts information This patch changes issuehistory.pl so it leverages on the API for fetching checkouts information in the template. Workflow changes: This development splits the 'current' checkouts and the 'past' checkouts in two different tables. Terminology could be revisited by native english speakers. To test: 1. Have records with and without current/past checkouts 2. Notice how the Checkouts history tab presents the information on each case. => SUGGESTION: Keep each of them open on separate tabs for comparing 3. Apply this patchset and its dependencies 4. Restart plack, run: $ koha-plack --restart kohadev 5. Follow the same steps you followed for those open tabs, in new ones => SUCCESS: What you see makes sense, information is displayed as it used to, with the different tables now. 6. Try searching on the datatables => SUCCESS: Search works correctly! 7. Revisit the new texts I added => SUGGESTION: If you have better wordings, submit a patch, or just comment on the bug so I do it inline. 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126628 [details] [review] Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126634 [details] [review] Bug 29275: Add $patron_to_html to render patron information in JS Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 126635 [details] [review] Bug 29275: Use the API to fetch checkouts information This patch changes issuehistory.pl so it leverages on the API for fetching checkouts information in the template. Workflow changes: This development splits the 'current' checkouts and the 'past' checkouts in two different tables. Terminology could be revisited by native english speakers. To test: 1. Have records with and without current/past checkouts 2. Notice how the Checkouts history tab presents the information on each case. => SUGGESTION: Keep each of them open on separate tabs for comparing 3. Apply this patchset and its dependencies 4. Restart plack, run: $ koha-plack --restart kohadev 5. Follow the same steps you followed for those open tabs, in new ones => SUCCESS: What you see makes sense, information is displayed as it used to, with the different tables now. 6. Try searching on the datatables => SUCCESS: Search works correctly! 7. Revisit the new texts I added => SUGGESTION: If you have better wordings, submit a patch, or just comment on the bug so I do it inline. 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 126636 [details] [review] Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
cases tested to get all the strings and behaviors I could thing of - never checked out and no current checkout - never checked out and a current checkout - a past checkout and no current checkout - more than 10 past checkout (for pagination) and a current checkout strings make sense
A diff between current_checkouts and old_checkouts functions show that they have a lot of duplication. Is there a way to deduplicate without making something messy? I wonder if there is a way to assign old_checkouts as a copy of current_checkouts and then re assign so it it's properties to add and change stuff. The only difference is the URL param and a render function. Also a call to encodeURIComponent() might be missing in current_checkouts
(In reply to Victor Grousset/tuxayo from comment #8) > A diff between current_checkouts and old_checkouts functions show that they > have a lot of duplication. > Is there a way to deduplicate without making something messy? > I wonder if there is a way to assign old_checkouts as a copy of > current_checkouts and then re assign so it it's properties to add and change > stuff. > The only difference is the URL param and a render function. > > Also a call to encodeURIComponent() might be missing in current_checkouts Thanks! Fixing!
(In reply to Victor Grousset/tuxayo from comment #8) > A diff between current_checkouts and old_checkouts functions show that they > have a lot of duplication. > Is there a way to deduplicate without making something messy? > I wonder if there is a way to assign old_checkouts as a copy of > current_checkouts and then re assign so it it's properties to add and change > stuff. > The only difference is the URL param and a render function. I tried, but things get a bit harder to understand and I didn't feel like the trade-off justified a generalization.
I didn't submit the follow-up because the RM mentioned bug 28859, which is expected to be pushed first.
And it has been pushed :D
Created attachment 127701 [details] [review] Bug 29275: Add $patron_to_html to render patron information in JS Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 127702 [details] [review] Bug 29275: Use the API to fetch checkouts information This patch changes issuehistory.pl so it leverages on the API for fetching checkouts information in the template. Workflow changes: This development splits the 'current' checkouts and the 'past' checkouts in two different tables. Terminology could be revisited by native english speakers. To test: 1. Have records with and without current/past checkouts 2. Notice how the Checkouts history tab presents the information on each case. => SUGGESTION: Keep each of them open on separate tabs for comparing 3. Apply this patchset and its dependencies 4. Restart plack, run: $ koha-plack --restart kohadev 5. Follow the same steps you followed for those open tabs, in new ones => SUCCESS: What you see makes sense, information is displayed as it used to, with the different tables now. 6. Try searching on the datatables => SUCCESS: Search works correctly! 7. Revisit the new texts I added => SUGGESTION: If you have better wordings, submit a patch, or just comment on the bug so I do it inline. 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 127703 [details] [review] Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
(In reply to Victor Grousset/tuxayo from comment #8) > A diff between current_checkouts and old_checkouts functions show that they > have a lot of duplication. > Is there a way to deduplicate without making something messy? > I wonder if there is a way to assign old_checkouts as a copy of > current_checkouts and then re assign so it it's properties to add and change > stuff. > The only difference is the URL param and a render function. Not just that. But I made a function to avoid duplication. > Also a call to encodeURIComponent() might be missing in current_checkouts Solved!
Created attachment 127704 [details] [review] Bug 29275: (follow-up) Split columns settings and handle upgrade This patch splits the table settings as we now have two tables. The atomic update takes care of setting the right values (if present) on the new tables entries. To test: 1. Do not apply this patch 2. Set some values you will remember for the checkoutshistory-table table 3. Run: $ koha-mysql kohadev > SELECT * FROM columns_settings WHERE tablename='checkoutshistory-table'; => SUCCESS: Koha currently works :-D 4. Apply this patch 5. Run: $ updatedatabase 6. Repeat 3 => SUCCESS: Things have been deleted 7. Run: $ koha-mysql kohadev > SELECT * FROM columns_settings WHERE tablename LIKE '%_checkouts-table'; => SUCCESS: old_checkouts-table and current_checkouts-table have the right setting 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Sorry dude.. I love this improvement.. but I think we have a problem. The new js equivalent to patron-name.inc is awesome and works great.. but it got me looking at what patron-name.inc does and threw me into the rabbit hole looking at how patrons get hidden from other branch staff in certain modes of operation. I can't see any handling, either in the js function or in the API response builder, that would filter out patrons that the logged-in user should not be able to see details for. I'm hopeful that I'm just missing something in the API layer as I think that's where it should sit personally.. we shouldn't expose the data at all if the user doesn't have permission to view it, rather than hide it at the view stage. If that functionality is there, any chance you could point me to the unit tests for it?
Created attachment 127894 [details] [review] Bug 29275: Add $patron_to_html to render patron information in JS Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 127895 [details] [review] Bug 29275: Use the API to fetch checkouts information This patch changes issuehistory.pl so it leverages on the API for fetching checkouts information in the template. Workflow changes: This development splits the 'current' checkouts and the 'past' checkouts in two different tables. Terminology could be revisited by native english speakers. To test: 1. Have records with and without current/past checkouts 2. Notice how the Checkouts history tab presents the information on each case. => SUGGESTION: Keep each of them open on separate tabs for comparing 3. Apply this patchset and its dependencies 4. Restart plack, run: $ koha-plack --restart kohadev 5. Follow the same steps you followed for those open tabs, in new ones => SUCCESS: What you see makes sense, information is displayed as it used to, with the different tables now. 6. Try searching on the datatables => SUCCESS: Search works correctly! 7. Revisit the new texts I added => SUGGESTION: If you have better wordings, submit a patch, or just comment on the bug so I do it inline. 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 127896 [details] [review] Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 127897 [details] [review] Bug 29275: (follow-up) Split columns settings and handle upgrade This patch splits the table settings as we now have two tables. The atomic update takes care of setting the right values (if present) on the new tables entries. To test: 1. Do not apply this patch 2. Set some values you will remember for the checkoutshistory-table table 3. Run: $ koha-mysql kohadev > SELECT * FROM columns_settings WHERE tablename='checkoutshistory-table'; => SUCCESS: Koha currently works :-D 4. Apply this patch 5. Run: $ updatedatabase 6. Repeat 3 => SUCCESS: Things have been deleted 7. Run: $ koha-mysql kohadev > SELECT * FROM columns_settings WHERE tablename LIKE '%_checkouts-table'; => SUCCESS: old_checkouts-table and current_checkouts-table have the right setting 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Martin Renvoize from comment #18) > Sorry dude.. I love this improvement.. but I think we have a problem. The > new js equivalent to patron-name.inc is awesome and works great.. but it got > me looking at what patron-name.inc does and threw me into the rabbit hole > looking at how patrons get hidden from other branch staff in certain modes > of operation. Good catch! And this is bigger than this dev! > I can't see any handling, either in the js function or in the API response > builder, that would filter out patrons that the logged-in user should not be > able to see details for. I'm hopeful that I'm just missing something in the > API layer as I think that's where it should sit personally.. we shouldn't > expose the data at all if the user doesn't have permission to view it, > rather than hide it at the view stage. If that functionality is there, any > chance you could point me to the unit tests for it? I proposed a generic solution for checking 'allowed-to-embed' objects on bug 28523. It looks pretty solid to me and I made this bug dependent on it. For the specific case of a 'null' patron on the response (which is now the case, correctly handled in the controller level, not just hidden) I decided to handle it locally (so not in js-patron-format.inc) like this: if ( row.patron != null ) { return $patron_to_html( row.patron, { display_cardnumber: true, url: true } ); } else { return _("A patron from library %s").format(escape_str(row.library.name)); } it felt like there was no need for a generic message, as each scenario could require different wordings. Please test!
(In reply to Victor Grousset/tuxayo from comment #12) > And it has been pushed :D Time to test :D
Created attachment 128001 [details] [review] Bug 29275: Use the API to fetch checkouts information This patch changes issuehistory.pl so it leverages on the API for fetching checkouts information in the template. Workflow changes: This development splits the 'current' checkouts and the 'past' checkouts in two different tables. Terminology could be revisited by native english speakers. To test: 1. Have records with and without current/past checkouts 2. Notice how the Checkouts history tab presents the information on each case. => SUGGESTION: Keep each of them open on separate tabs for comparing 3. Apply this patchset and its dependencies 4. Restart plack, run: $ koha-plack --restart kohadev 5. Follow the same steps you followed for those open tabs, in new ones => SUCCESS: What you see makes sense, information is displayed as it used to, with the different tables now. 6. Try searching on the datatables => SUCCESS: Search works correctly! 7. Revisit the new texts I added => SUGGESTION: If you have better wordings, submit a patch, or just comment on the bug so I do it inline. 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 128002 [details] [review] Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 128003 [details] [review] Bug 29275: (follow-up) Split columns settings and handle upgrade This patch splits the table settings as we now have two tables. The atomic update takes care of setting the right values (if present) on the new tables entries. To test: 1. Do not apply this patch 2. Set some values you will remember for the checkoutshistory-table table 3. Run: $ koha-mysql kohadev > SELECT * FROM columns_settings WHERE tablename='checkoutshistory-table'; => SUCCESS: Koha currently works :-D 4. Apply this patch 5. Run: $ updatedatabase 6. Repeat 3 => SUCCESS: Things have been deleted 7. Run: $ koha-mysql kohadev > SELECT * FROM columns_settings WHERE tablename LIKE '%_checkouts-table'; => SUCCESS: old_checkouts-table and current_checkouts-table have the right setting 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
OK, I split out the first patch and moved it to bug 29575 and set it as a dependency. This allows other bugs to utilise this great new function ;) I've added my SO to the patches here and am just finishing up QAing the embed dependency.. will switch to PQA along with that one once I'm done.
Good idea thanks! :D
Created attachment 131333 [details] [review] Bug 29275: Use the API to fetch checkouts information This patch changes issuehistory.pl so it leverages on the API for fetching checkouts information in the template. Workflow changes: This development splits the 'current' checkouts and the 'past' checkouts in two different tables. Terminology could be revisited by native english speakers. To test: 1. Have records with and without current/past checkouts 2. Notice how the Checkouts history tab presents the information on each case. => SUGGESTION: Keep each of them open on separate tabs for comparing 3. Apply this patchset and its dependencies 4. Restart plack, run: $ koha-plack --restart kohadev 5. Follow the same steps you followed for those open tabs, in new ones => SUCCESS: What you see makes sense, information is displayed as it used to, with the different tables now. 6. Try searching on the datatables => SUCCESS: Search works correctly! 7. Revisit the new texts I added => SUGGESTION: If you have better wordings, submit a patch, or just comment on the bug so I do it inline. 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131334 [details] [review] Bug 29275: (follow-up) Remove use of CountSubscriptionFromBiblionumber Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 131335 [details] [review] Bug 29275: (follow-up) Split columns settings and handle upgrade This patch splits the table settings as we now have two tables. The atomic update takes care of setting the right values (if present) on the new tables entries. To test: 1. Do not apply this patch 2. Set some values you will remember for the checkoutshistory-table table 3. Run: $ koha-mysql kohadev > SELECT * FROM columns_settings WHERE tablename='checkoutshistory-table'; => SUCCESS: Koha currently works :-D 4. Apply this patch 5. Run: $ updatedatabase 6. Repeat 3 => SUCCESS: Things have been deleted 7. Run: $ koha-mysql kohadev > SELECT * FROM columns_settings WHERE tablename LIKE '%_checkouts-table'; => SUCCESS: old_checkouts-table and current_checkouts-table have the right setting 8. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
A quick rebase, now moving on to testing.
OK, the rebase was trivial enough, but I think we need some more work in context of bug 29523.. and there's some weird things happening in the call to kohaTable.. for instance I'm not seeing the embeds get added to the query here in my testing.
OK, this needs another rebase and there's a bit more to it this time so I think I need to defer to you Tomas.
Eager to see this one move forward :)
+ if ( row.patron != null ) { + return $patron_to_html( row.patron, { display_cardnumber: true, url: true } ); + } + else { + return _("A patron from library %s").format(escape_str(row.library.name)); } This is not correct, is it? row.library.name should be row.patron.library.name, but we don't have it.
(In reply to Jonathan Druart from comment #37) > + if ( row.patron != null ) { > + return $patron_to_html( row.patron, { > display_cardnumber: true, url: true } ); > + } > + else { > + return _("A patron from library > %s").format(escape_str(row.library.name)); > } > > This is not correct, is it? > row.library.name should be row.patron.library.name, but we don't have it. +Strings
(In reply to Tomás Cohen Arazi from comment #38) > (In reply to Jonathan Druart from comment #37) > > + if ( row.patron != null ) { > > + return $patron_to_html( row.patron, { > > display_cardnumber: true, url: true } ); > > + } > > + else { > > + return _("A patron from library > > %s").format(escape_str(row.library.name)); > > } > > > > This is not correct, is it? > > row.library.name should be row.patron.library.name, but we don't have it. > > +Strings lol yes if you want, but that was not my point :D
(In reply to Jonathan Druart from comment #37) > + if ( row.patron != null ) { > + return $patron_to_html( row.patron, { > display_cardnumber: true, url: true } ); > + } > + else { > + return _("A patron from library > %s").format(escape_str(row.library.name)); > } > > This is not correct, is it? > row.library.name should be row.patron.library.name, but we don't have it. It took me a minute, but I think I see what you're saying. "row.library.name" would be the name of the library where the checkout occurred - not the name of the library the patron is from. While it's possible that these two values could be the same, it's also possible that they could be different. Wouldn't it make more sense to just change the text to something like: "Patron details suppressed", or "Unknown Patron", or even ""?
(In reply to David Cook from comment #40) > Wouldn't it make more sense to just change the text to something like: > "Patron details suppressed", or "Unknown Patron", or even ""? "Patron details currently unavailable"
But I could also be misunderstanding how this is supposed to work...
(In reply to David Cook from comment #40) > Wouldn't it make more sense to just change the text to something like: > "Patron details suppressed", or "Unknown Patron", or even ""? We could, but we remove information. IIRC it was part of the specs (bug 18403).
Gave a go at rebasing but can't arbitrate on a conflict about a change of HTML header level. (both in this patch and main/master) Anyway, this doesn't seem to be needing a rebase right now but rather settling on what to do about comment 37 (continued in Comment 40 and Comment 43)
Still love to see this one move forward :S
Gonna do some QA work on bug 33568, and will align this patchset with what Jonathan has done there!
One to strive for early in 24.11.x cycle Tomas?
(In reply to Martin Renvoize from comment #47) > One to strive for early in 24.11.x cycle Tomas? Totally.
Shall we put this one on the roadmap?
ping?