https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 122832 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122832&action=edit Bug 28709: Refactor Koha::Biblio->custom_cover_image_url to allow passing parameters Currently the search result template receive a Koha::Biblio object and call the method. The searchResults routine already has the isbn, issn, and normalized_isbn calculated. Rather than fetching the object and passing to the template we can pass the parameters, generate the url, and pass only the URL to the template To test: 1 - Set the system preferences: CustomCoverImages : Display OPACCustomCoverImages: Display CustomCoverImagesURL: https://images-na.ssl-images-amazon.com/images/P/{normalized_isbn}.01.LZZZZZZZ.jpg 2 - Search on staff and opac for a term that returns titles with covers, e.g. 'shuffle' 3 - Confirm the covers show 4 - Apply patch 5 - Restart all 6 - Confirm covers still show 7 - prove -v t/db_dependent/Koha/Biblios.t -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122832|0 |1 is obsolete| | --- Comment #2 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 122833 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122833&action=edit Bug 28709: Refactor Koha::Biblio->custom_cover_image_url to allow passing parameters Currently the search result template receive a Koha::Biblio object and call the method. The searchResults routine already has the isbn, issn, and normalized_isbn calculated. Rather than fetching the object and passing to the template we can pass the parameters, generate the url, and pass only the URL to the template To test: 1 - Set the system preferences: CustomCoverImages : Display OPACCustomCoverImages: Display CustomCoverImagesURL: https://images-na.ssl-images-amazon.com/images/P/{normalized_isbn}.01.LZZZZZZZ.jpg 2 - Search on staff and opac for a term that returns titles with covers, e.g. 'shuffle' 3 - Confirm the covers show 4 - Apply patch 5 - Restart all 6 - Confirm covers still show 7 - prove -v t/db_dependent/Koha/Biblios.t -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #3 from David Cook <dcook@prosentient.com.au> --- I like the motivation for the change, but I'm not sure about the style. What if we moved most of the logic out of "custom_cover_image_url" and into a separate class method. Then "custom_cover_image_url" could call that class method, and that class method could be used in the search results. I think that would make the code easier to read and maintain in the longer term. But that's just my two cents. I know that it would be more work so might not be feasible. I've used the same style of checking if $self is a ref for times when I couldn't refactor more fully. Always nice to see an efficiency improvement :) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA CC| |jonathan.druart+koha@gmail. | |com --- Comment #5 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (In reply to David Cook from comment #3)

I like the motivation for the change, but I'm not sure about the style. What if we moved most of the logic out of "custom_cover_image_url" and into a separate class method. Then "custom_cover_image_url" could call that class method, and that class method could be used in the search results. I think that would make the code easier to read and maintain in the longer term.

After I read the patches I was coming here to write that. Missing occurrence: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-opensearch.tt: [% SET custom_cover_image_url = SEARCH_RESULT.biblio_object.custom_cover_image_url %] -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122835|0 |1 is obsolete| | --- Comment #6 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 122962 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=122962&action=edit Bug 28709: Refactor Koha::Biblio->custom_cover_image_url to allow passing parameters Currently the search result template receive a Koha::Biblio object and call the method. The searchResults routine already has the isbn, issn, and normalized_isbn calculated. Rather than fetching the object and passing to the template we can pass the parameters, generate the url, and pass only the URL to the template To test: 1 - Set the system preferences: CustomCoverImages : Display OPACCustomCoverImages: Display CustomCoverImagesURL: https://images-na.ssl-images-amazon.com/images/P/{normalized_isbn}.01.LZZZZZZZ.jpg 2 - Search on staff and opac for a term that returns titles with covers, e.g. 'shuffle' 3 - Confirm the covers show 4 - Apply patch 5 - Restart all 6 - Confirm covers still show 7 - prove -v t/db_dependent/Koha/Biblios.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 --- Comment #8 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Are you sure we don't only need to pass/cache the MARC record? Did you benchmark this patch? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #122962|0 |1 is obsolete| | --- Comment #9 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- Created attachment 127657 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127657&action=edit Bug 28709: Refactor Koha::Biblio->custom_cover_image_url to allow passing parameters Currently the search result template receive a Koha::Biblio object and call the method. The searchResults routine already has the isbn, issn, and normalized_isbn calculated. Rather than fetching the object and passing to the template we can pass the parameters, generate the url, and pass only the URL to the template To test: 1 - Set the system preferences: CustomCoverImages : Display OPACCustomCoverImages: Display CustomCoverImagesURL: https://images-na.ssl-images-amazon.com/images/P/{normalized_isbn}.01.LZZZZZZZ.jpg 2 - Search on staff and opac for a term that returns titles with covers, e.g. 'shuffle' 3 - Confirm the covers show 4 - Apply patch 5 - Restart all 6 - Confirm covers still show 7 - prove -v t/db_dependent/Koha/Biblios.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #127657|0 |1 is obsolete| | --- Comment #11 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 127688 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=127688&action=edit Bug 28709: Refactor Koha::Biblio->custom_cover_image_url to allow passing parameters Currently the search result template receive a Koha::Biblio object and call the method. The searchResults routine already has the isbn, issn, and normalized_isbn calculated. Rather than fetching the object and passing to the template we can pass the parameters, generate the url, and pass only the URL to the template To test: 1 - Set the system preferences: CustomCoverImages : Display OPACCustomCoverImages: Display CustomCoverImagesURL: https://images-na.ssl-images-amazon.com/images/P/{normalized_isbn}.01.LZZZZZZZ.jpg 2 - Search on staff and opac for a term that returns titles with covers, e.g. 'shuffle' 3 - Confirm the covers show 4 - Apply patch 5 - Restart all 6 - Confirm covers still show 7 - prove -v t/db_dependent/Koha/Biblios.t -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 --- Comment #13 from David Cook <dcook@prosentient.com.au> --- (In reply to Nick Clemens from comment #12)

...

Did you benchmark this patch? with numSearchResults at 50 - standard sample data, search for 'a' Before patch ~2 second After patch ~1.85 seconds

Is that averaged or based on 1 test each? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28709 --- Comment #16 from David Cook <dcook@prosentient.com.au> --- (In reply to Jonathan Druart from comment #8)

Are you sure we don't only need to pass/cache the MARC record? Did you benchmark this patch?

Maybe it's worth looking again at this one with Devel::NYTProf to see where speed gains could be made... -- You are receiving this mail because: You are watching all bug changes.