Bug 5687 - Add preference for display of AuthorisedValueImages at staff search results
Summary: Add preference for display of AuthorisedValueImages at staff search results
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: PATCH-Sent (DO NOT USE) enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Bugs List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-03 11:38 UTC by Marcel de Rooy
Modified: 2012-10-25 23:03 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch (for functionality) (3.41 KB, patch)
2011-02-03 12:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch (for install) (3.41 KB, patch)
2011-02-03 12:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch (for install) (2.37 KB, patch)
2011-02-03 12:15 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Screen shot 1 (234.70 KB, image/pjpeg)
2011-02-07 08:55 UTC, Marcel de Rooy
Details
Screen shot 2 (248.19 KB, image/pjpeg)
2011-02-07 08:57 UTC, Marcel de Rooy
Details
Revised patch (functionality) (3.46 KB, patch)
2011-02-07 13:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Revised patch (functionality) (2.73 KB, patch)
2011-03-10 08:38 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Revised patch (install) (10.28 KB, patch)
2011-03-10 08:59 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch for install (updated) (9.59 KB, patch)
2011-03-17 14:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch for install (updated) (10.29 KB, patch)
2011-03-28 07:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2011-02-03 11:38:14 UTC
At OPAC side the preference AuthorisedValueImages already exists. In staff client, the search results always include these images. If they are not found, they are replaced by itemtype desc. This results in results like:

4. Iznik pottery and tiles in the Calouste Gulbenkian collection /
Queiroz Ribeiro, Maria Description: [London] : Scala, 2009 : 136 p. : ; 31 cm. ISBN: 9728848587 Collection catalogs
Collection catalogs Collection catalogs 

NOTE: The three Collection catalogs (itemtype desc) refer to three authorised values: a relator code, a relator term and a itemtype. This make no sense. (We made an authorised list for relator codes and terms.) 

The new preference AuthorisedValueImages adds the option to hide display of the images or the duplicate alternative descriptions.
Comment 1 Marcel de Rooy 2011-02-03 12:11:02 UTC Comment hidden (obsolete)
Comment 2 Marcel de Rooy 2011-02-03 12:12:51 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2011-02-03 12:15:01 UTC Comment hidden (obsolete)
Comment 4 Chris Nighswonger 2011-02-04 00:29:05 UTC
I'm not sure what effect this code has. The TMPL_VAR authorised_value_images is never set by catalogue/search.pl and so should never show up in results.tmpl in spite of being checked for there.

If the result display is malformed, perhaps there is another syntax problem within the results.tmpl template.

Or have I missed something in my testing?
Comment 5 Marcel de Rooy 2011-02-04 07:46:02 UTC
Thank you for testing. Like to respond to your comments however:

The TMPL_VAR is set in a module:

C4/Search.pm, searchResults (called by catalogue/search.pl) 

 $oldbiblio->{'authorised_value_images'}  = C4::Items::get_authorised_value_images( C4::Biblio::get_biblio_authorised_values( $oldbiblio->{'biblionumber'}, $marcrecord ) );

It certainly shows up. See the example below. I eliminated it by first testing with the template.
I do not suspect any syntax problem here.
Comment 6 Chris Nighswonger 2011-02-04 13:42:01 UTC
(In reply to comment #5)
> Thank you for testing. Like to respond to your comments however:
> 
> The TMPL_VAR is set in a module:
> 
> C4/Search.pm, searchResults (called by catalogue/search.pl) 
> 
>  $oldbiblio->{'authorised_value_images'}  =
> C4::Items::get_authorised_value_images(
> C4::Biblio::get_biblio_authorised_values( $oldbiblio->{'biblionumber'},
> $marcrecord ) );
> 
> It certainly shows up. See the example below. I eliminated it by first testing
> with the template.
> I do not suspect any syntax problem here.


I see it now. It is part of the newresults array in search.pl.

So a couple of questions:

1. How do I get authorised_value_images to populate?

2. Can you attach screen shots showing what the problem is and what the fix looks like?

Thanks
Comment 7 Marcel de Rooy 2011-02-07 08:55:58 UTC
Created attachment 3067 [details]
Screen shot 1

Shows duplicate item type descriptions (alternative text for authorised value images)
Comment 8 Marcel de Rooy 2011-02-07 08:57:03 UTC
Created attachment 3068 [details]
Screen shot 2
Comment 9 Marcel de Rooy 2011-02-07 09:01:29 UTC
> How do I get authorised_value_images to populate?

Please see Comment1 also. Attach an authorised value list to a field in your framework, and use that in a record. We did so for item type, relator code and relator term. This generates either images or alternative text (if no image was found) for each one. See e.g. the second or fourth record in the screenshot.
Comment 10 Marcel de Rooy 2011-02-07 13:51:58 UTC Comment hidden (obsolete)
Comment 11 MJ Ray (software.coop) 2011-03-04 10:26:59 UTC
This bug is mentioned in:
Bug 5687: Add pref StaffAuthorisedValueImages for staff search results http://lists.koha-community.org/pipermail/koha-patches/2011-February/013752.html
Bug 5687: Add pref StaffAuthorisedValueImages for staff search results http://lists.koha-community.org/pipermail/koha-patches/2011-February/013753.html
Bug 5687: Add pref StaffAuthorisedValueImages for staff search results http://lists.koha-community.org/pipermail/koha-patches/2011-February/013771.html
Comment 12 Ian Walls 2011-03-09 19:35:54 UTC
I'd recommend that a conditional be added to C4::Search::searchResults to only populate $oldbiblio->{'authorised_value_images'} if they're going to be used by the requesting search results page.  That is:

IF (($search_context == 'opac' and AuthorisedValueImages) OR ($search_context == 'intranet' and StaffAuthorisedValueImages))

The call to get authorised values can be very expensive, and if it's not going to be used in the output, we could save ourselves some processing time (possibly over 1 second's worth) when performing a search by never invoking the subroutine.

This would also simplify the template: we wouldn't need a nested conditional for both the system preference AuthorisedValueImages and the existence of the images themselves; they would only exist if they were supposed to.
Comment 13 Chris Nighswonger 2011-03-09 20:22:56 UTC
+1
Comment 14 Marcel de Rooy 2011-03-10 08:38:11 UTC
Created attachment 3278 [details] [review]
Revised patch (functionality)

Revised patch populates authvalimages only if needed. No changes to template or search.pl needed anymore.
Comment 15 Marcel de Rooy 2011-03-10 08:59:50 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2011-03-17 14:22:46 UTC Comment hidden (obsolete)
Comment 17 Ian Walls 2011-03-26 00:02:08 UTC
The revised patch for functionality works!

Running a search through NYTProf before, I was getting over 1.29 s inclusive time spent on 20 instances of get_biblios_authorised_values.  That has now disappeared, and has not recurred in other places.

Also tested manually, by counting seconds for searches to return.  Dropped from 4 seconds to 3 seconds, which I would call quite significant.

If you pass along the full install patch, I'll happily sign off
Comment 18 Marcel de Rooy 2011-03-28 07:48:42 UTC
Created attachment 3459 [details] [review]
Patch for install (updated)

Rebased on March 28
Comment 19 Marcel de Rooy 2011-03-28 07:50:27 UTC
(In reply to comment #17)
> The revised patch for functionality works!
> Running a search through NYTProf before, I was getting over 1.29 s inclusive
> time spent on 20 instances of get_biblios_authorised_values.  That has now
> disappeared, and has not recurred in other places.
> Also tested manually, by counting seconds for searches to return.  Dropped from
> 4 seconds to 3 seconds, which I would call quite significant.
> If you pass along the full install patch, I'll happily sign off

Thanks for testing, Ian.
I rebased the install patch. So if you could sign both patches, please go ahead.
Comment 20 Ian Walls 2011-03-30 00:51:48 UTC
Signed off patches sent to list
Comment 21 Chris Cormack 2011-03-30 06:53:40 UTC
Patch pushed, please test
Comment 22 Marcel de Rooy 2011-03-30 11:38:35 UTC
Works for me. Thanks for signoff and push. Anyone else to close it?
Comment 23 Julian Maurice 2011-04-05 14:48:43 UTC
Can someone give me the steps to follow in order to view these images? I don't manage to get them...
Comment 24 Chris Nighswonger 2011-04-05 14:51:40 UTC
(In reply to comment #23)
> Can someone give me the steps to follow in order to view these images? I don't
> manage to get them...

Just clicking on the links causes them to display in my browser (FF).
Comment 25 Julian Maurice 2011-04-05 15:22:17 UTC
(In reply to comment #24)
> Just clicking on the links causes them to display in my browser (FF).

Maybe I wasn't clear, setting StaffAuthorisedValueImages to "Show" or "Don't Show" doesn't make any changes. The only images I see in search results are the itemtype images. The description of this system preference is talking about lost statuses and locations, so I try to "attach" an image to theses authorised values, and change the status of some items to "Lost", for example. I don't see any changes.
Can you be more precise please?
Comment 26 Marcel de Rooy 2011-04-06 12:07:26 UTC
(In reply to comment #25)
> (In reply to comment #24)
> > Just clicking on the links causes them to display in my browser (FF).
> Maybe I wasn't clear, setting StaffAuthorisedValueImages to "Show" or "Don't
> Show" doesn't make any changes. The only images I see in search results are the
> itemtype images. The description of this system preference is talking about
> lost statuses and locations, so I try to "attach" an image to theses authorised
> values, and change the status of some items to "Lost", for example. I don't see
> any changes.
> Can you be more precise please?

Please see problem description. In screen shots you see repeated item type descriptions like "Collection catalogs Collection catalogs Collection catalogs". (The text is already an alternative for an image.) This comes from authorized values used on relator codes and relator terms. If you would not use authorized values on such fields, you will not see it.
Comment 27 MJ Ray (software.coop) 2011-04-11 08:32:00 UTC
This bug is mentioned in:
Bug 5687: Add pref StaffAuthorisedValueImages for staff search results http://lists.koha-community.org/pipermail/koha-patches/2011-March/014018.html
Bug 5687: Add pref StaffAuthorisedValueImages for staff search results (install) http://lists.koha-community.org/pipermail/koha-patches/2011-March/014019.html
Bug 5687: Add pref	StaffAuthorisedValueImages for staff search results (install) http://lists.koha-community.org/pipermail/koha-patches/2011-March/014113.html
Bug 5687: Add pref StaffAuthorisedValueImages for staff search results (install) http://lists.koha-community.org/pipermail/koha-patches/2011-March/014255.html
Bug 5687: Add pref	StaffAuthorisedValueImages for staff search results http://lists.koha-community.org/pipermail/koha-patches/2011-March/014332.html
Bug 5687: Add pref	StaffAuthorisedValueImages for staff search results (install) http://lists.koha-community.org/pipermail/koha-patches/2011-March/014333.html
Comment 28 Marcel de Rooy 2011-05-12 07:29:50 UTC
Closing this bug myself.