|
Description
Nick Clemens (kidclamp)
2021-07-13 18:26:17 UTC
Created attachment 122832 [details] [review] 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 Created attachment 122833 [details] [review] 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 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 :) Created attachment 122835 [details] [review] 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 Signed-off-by: Owen Leonard <oleonard@myacpl.org> (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 %] Created attachment 122962 [details] [review] 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 Doesn't apply. Are you sure we don't only need to pass/cache the MARC record? Did you benchmark this patch? Created attachment 127657 [details] [review] 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 Patch rebased (and tidied searchResults changes). Created attachment 127688 [details] [review] 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 (In reply to Jonathan Druart from comment #8) > Are you sure we don't only need to pass/cache the MARC record? I suppose we could, but we have already called TransformMarcToKoha, why would we not pass the values we calculated? > Did you benchmark this patch? with numSearchResults at 50 - standard sample data, search for 'a' Before patch ~2 second After patch ~1.85 seconds (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? (In reply to David Cook from comment #13) > (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? I repeated the tests 10 times each, that was the average I'm getting an average improvement of .03 seconds, and I'm not sure what to say about that. (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... The performance increase was lessened as the removal of the biblio fetch was lost during a rebase Bug 3142 added a use of the biblio_object for checking holdability Closing this one out |
