[Koha-bugs] [Bug 32680] Add hooks to allow cover images to be provided by plugins

[bugzilla-daemon at bugs.koha-community.org]

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=32680

Nick Clemens <nick at bywatersolutions.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |nick at bywatersolutions.com
             Status|Signed Off                  |Failed QA

--- Comment #28 from Nick Clemens <nick at bywatersolutions.com> ---
I like this idea, it expands upon the current: CustomCoverImagesURL system
preference, and would allow for adding services that need authentication, or
more specialized formats.

There are a few issues I see:
1 - In both the staff and opac we have lines like below in the scripts:
use Koha::Template::Plugin::KohaPlugins;
...
my $intranet_cover_images_plugins =
Koha::Template::Plugin::KohaPlugins->get_plugins_intranet_cover_images;
if($intranet_cover_images_plugins){
    $template->param( CoverImagesRequired => 1 )
}

I don't think we should be calling template plugins in the scripts.

We can do something like:
[% SET CoverImagesRequired = KohaPlugins.get_plugins_intranet_cover_images %]
early in the template, then add the code in later like:
[% CoverImagesRequired | $raw %]

2 - I am not sure the code below is necessary, we add normalized isbn to the
results, and the plugin seems to simply fetch the results with JS " const
search_results_images = document.querySelectorAll('.cover_images_required');"
 Am I correct?
+    [% IF ( CoverImagesRequired ) %]
+        <script>
+            const search_results = {};
+            [% FOREACH SEARCH_RESULT IN SEARCH_RESULTS %]
+                var cover_index = "[% loop.count | html %]";
+                search_results[cover_index] = {};
+                search_results[cover_index].isbn = "[%
SEARCH_RESULT.normalized_isbn | html %]";
+                search_results[cover_index].biblionumber = "[%
SEARCH_RESULT.biblionumber | html %]";
+                search_results[cover_index].processedBiblio = "[% PROCESS
biblio_a_href biblionumber => SEARCH_RESULT.biblionumber | html %]";
+            [% END %]
+        </script>
+    [% END %]

3 - CoverImagesRequired is misleading. I think it should be renamed like
'CoverImagePlugins' 

4 - I would suggest squashing the patches, as the second rewrites the first, it
would make for easier review

-- 
You are receiving this mail because:
You are watching all bug changes.