Bug 18799 - XSLTresultsdisplay hides the icons
Summary: XSLTresultsdisplay hides the icons
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Searching (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Victor Grousset/tuxayo
QA Contact: Nick Clemens
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-06-14 09:04 UTC by Baptiste
Modified: 2018-06-05 07:27 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bub 18799: Display icons on results.tt (1.46 KB, patch)
2017-06-14 09:09 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18799: Display icons on results.tt (1.46 KB, patch)
2017-06-14 09:10 UTC, Baptiste
Details | Diff | Splinter Review
Bug 18799: Display icons on results.tt (1.52 KB, patch)
2017-06-14 09:24 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Alternate patch which preserves status quo on biblio vs. item-level itemtypes (4.85 KB, application/mbox)
2017-06-15 14:04 UTC, Owen Leonard
Details
Alternate patch which preserves status quo on biblio vs. item-level itemtypes (4.95 KB, patch)
2017-06-29 18:57 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18799 [ Alternate ] XSLTresultsdisplay hides the icons (4.77 KB, patch)
2018-03-31 18:52 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 18799: XSLTresultsdisplay hides the icons (4.81 KB, patch)
2018-04-18 08:22 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 18799: XSLTresultsdisplay hides the icons (4.87 KB, patch)
2018-04-20 10:31 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Baptiste 2017-06-14 09:04:16 UTC
to reproduce :
'xsltresultsdisplay' => default

In intranet search, icons are not displayed
Comment 1 Baptiste 2017-06-14 09:09:42 UTC
Created attachment 64276 [details] [review]
Bub 18799: Display icons on results.tt

TEST PLAN:
1 - Set xsltresultsdisplay to default
2 - Search something in the catalog => you shouldn't see any icon
3 - Apply patch
4 - Search again, you shall see icons beside the titles of resources
Comment 2 Baptiste 2017-06-14 09:10:22 UTC
Created attachment 64277 [details] [review]
Bug 18799: Display icons on results.tt

TEST PLAN:
1 - Set xsltresultsdisplay to default
2 - Search something in the catalog => you shouldn't see any icon
3 - Apply patch
4 - Search again, you shall see icons beside the titles of resources
Comment 3 Fridolin SOMERS 2017-06-14 09:14:03 UTC
You must have item-level_itypes on biblio record and biblio.itemtypes defined on itemtypes with an icon.
Comment 4 Fridolin SOMERS 2017-06-14 09:24:18 UTC
Created attachment 64279 [details] [review]
Bug 18799: Display icons on results.tt

TEST PLAN:
1 - Set xsltresultsdisplay to default
2 - Search something in the catalog => you shouldn't see any icon
3 - Apply patch
4 - Search again, you shall see icons beside the titles of resources

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 5 Fridolin SOMERS 2017-06-14 09:25:46 UTC
I signoff.

Anyone outside Biblibre is free to sign also.
Comment 6 Owen Leonard 2017-06-14 12:06:19 UTC
This change doesn't make sense to me. Generally I think we consider biblio-level itemtype to be superseded by item-level itemtype when it comes to displaying itemtype information. I think without this patch it works correctly.
Comment 7 Baptiste 2017-06-14 12:48:54 UTC
In this case, shouldn't the non XSLT interface hide those icons to ?
Comment 8 Fridolin SOMERS 2017-06-14 12:54:09 UTC
(In reply to Owen Leonard from comment #6)
> This change doesn't make sense to me. Generally I think we consider
> biblio-level itemtype to be superseded by item-level itemtype when it comes
> to displaying itemtype information. I think without this patch it works
> correctly.

But in OPAC icons are displayed and in intranet if no XSLT.
Behavior with or without XSLT should be the same outside XSLT generate HTML code.

One can always hide the icons with syspref noItemTypeImages.
Comment 9 Jonathan Druart 2017-06-14 17:14:36 UTC
Regarding last comments another signoff is needed.
Comment 10 Owen Leonard 2017-06-15 12:20:57 UTC
I think this bug can be fixed in a different way which will be acceptable to all. I'm preparing an alternate patch.
Comment 11 Owen Leonard 2017-06-15 14:04:55 UTC
Created attachment 64327 [details]
Alternate patch which preserves status quo on biblio vs. item-level itemtypes

    Bug 18799 [ Alternate ] XSLTresultsdisplay hides the icons

    If item-level_itypes is turned off and  XSLTResultsDisplay is turned on,
    biblio-level itemtype images are not shown in staff client search
    results. This patch corrects the problem.

    This patch also adds some markup and CSS to the itemtype image to make
    it look a litte cleaner and work better in result sets in which some
    titles have images and some don't.

    To test, apply the patch and view the staff client search results page
    with various settings:

    - XSLT turned on and off
    - item-level_itypes turned on and off
    - noItemTypeImages turned on and off

    Confirm that item type images are shown or not shown correctly according
    to each setting.

    The difference between this patch and the other patch currently on this
    bug is that this patch continues the practice of hiding biblio-level
    itemtype information when item-level_itypes is ON.
Comment 12 M. Tompsett 2017-06-29 18:57:00 UTC
Created attachment 64728 [details] [review]
Alternate patch which preserves status quo on biblio vs. item-level itemtypes

Hopefully this makes Owen's patch a nice patch.
Comment 13 M. Tompsett 2017-06-29 19:03:52 UTC
I hope Fridolin doesn't mind, but I really believe Owen's patch is better. Style inlined is really bad HTML, not to mention Owen actually optimizes the UNLESS code into one spot and brings code consistency. It's beautiful, Owen.
Comment 14 M. Tompsett 2017-06-29 19:04:16 UTC
Comment on attachment 64279 [details] [review]
Bug 18799: Display icons on results.tt

Owen's patch is better, in my opinion.
Comment 15 M. Tompsett 2017-06-29 19:34:18 UTC
Comment on attachment 64279 [details] [review]
Bug 18799: Display icons on results.tt

Review of attachment 64279 [details] [review]:
-----------------------------------------------------------------

::: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
@@ +532,5 @@
>  			    [% IF ( SEARCH_RESULT.XSLTResultsRecord ) %]
>                  [% SEARCH_RESULT.result_number %].
> +                                [% IF !noItemTypeImages && SEARCH_RESULT.imageurl %]
> +                                    <img src="[% SEARCH_RESULT.imageurl %]" title="[% SEARCH_RESULT.description %]" style="float: left; margin: .1em;" alt="[% SEARCH_RESULT.description %]" />
> +                                [% END %]

Now after testing, I get it. This will always display for XSLT records regardless of the item-itype system preference setting.
Comment 16 M. Tompsett 2017-06-29 19:35:18 UTC
The icons should ALWAYS display, regardless of the item-itype system preference, should they not?
Comment 17 Fridolin SOMERS 2017-06-30 07:28:33 UTC
(In reply to M. Tompsett from comment #13)
> I hope Fridolin doesn't mind, but I really believe Owen's patch is better.
> Style inlined is really bad HTML, not to mention Owen actually optimizes the
> UNLESS code into one spot and brings code consistency. It's beautiful, Owen.

No problem, this patch is indeed better, Owen is a first class patcher ;).

I'll test.
Comment 18 Baptiste 2017-06-30 08:20:54 UTC
I don't see any difference between  DisplayIconsXSLT setted and not setted, in all case, all icons are displayed in intranet results and not in intranet details.

I tested on this sandbox because I have locally no Marc21:
http://pro.user03-koha.sandbox.biblibre.eu/cgi-bin/koha/catalogue/detail.pl?biblionumber=73&searchid=scs_1498810298579

If you have an idea of what went wrong.. ;)
Comment 19 Fridolin SOMERS 2017-06-30 11:27:12 UTC
(In reply to Baptiste from comment #18)
> I don't see any difference between  DisplayIconsXSLT setted and not setted,
> in all case, all icons are displayed in intranet results and not in intranet
> details.
> 
> I tested on this sandbox because I have locally no Marc21:
> http://pro.user03-koha.sandbox.biblibre.eu/cgi-bin/koha/catalogue/detail.
> pl?biblionumber=73&searchid=scs_1498810298579
> 
> If you have an idea of what went wrong.. ;)

XSLT sysprefs are empty on this sandbox, use "default"
Comment 20 M. Tompsett 2017-06-30 14:40:00 UTC
The issue isn't the XSLT sysprefs per say.
The issue is the item-itype system preference.
Toggle that in your testing too!
And make sure to have 2 records one with a biblio level item type and another with an item level item type.
Comment 21 Baptiste 2017-07-03 07:36:15 UTC
Hum ok.. So the settings:

item-level_itype => biblio record
XSLTDetailsDisplay,  XSLTListsDisplay ,  XSLTResultsDisplay  => "default"
DisplayIconsXSLT  => show 
noItemTypeImages => show

I can find icons on this page 
http://pro.user03-koha.sandbox.biblibre.eu/cgi-bin/koha/catalogue/detail.pl?biblionumber=384&searchid=scs_1499067251739

but with all others settings it works
Comment 22 Fridolin SOMERS 2017-07-05 07:24:08 UTC
I have tested, works well.

But with biblio level item type, the image is displayed right of record text.
Without patch it is left.

Shouldn't it be CSS :
  .result-biblio-itemtype { float: left; ...
?

I think display is better with image left.
Comment 23 Victor Grousset/tuxayo 2018-03-31 18:52:57 UTC
Created attachment 73514 [details] [review]
Bug 18799 [ Alternate ] XSLTresultsdisplay hides the icons

If item-level_itypes is turned off and XSLTResultsDisplay is turned on,
biblio-level itemtype images are not shown in staff client search
results. This patch corrects the problem.

This patch also adds some markup and CSS to the itemtype image to make
it look a litte cleaner and work better in result sets in which some
titles have images and some don't.

To test, apply the patch and view the staff client search results page
with various settings:

- XSLT turned on and off
- item-level_itypes turned on and off
- noItemTypeImages turned on and off

Confirm that item type images are shown or not shown correctly according
to each setting.

The difference between this patch and the other patch currently on this
bug is that this patch continues the practice of hiding biblio-level
itemtype information when item-level_itypes is ON.
Comment 24 Victor Grousset/tuxayo 2018-03-31 18:53:57 UTC
Rebased the patch and solved conflicts.
Comment 25 Victor Grousset/tuxayo 2018-04-18 08:18:54 UTC Comment hidden (obsolete)
Comment 26 Fridolin SOMERS 2018-04-18 08:21:25 UTC
(In reply to Fridolin SOMERS from comment #22)
> I have tested, works well.
> 
> But with biblio level item type, the image is displayed right of record text.
> Without patch it is left.
> 
> Shouldn't it be CSS :
>   .result-biblio-itemtype { float: left; ...
> ?
> 
> I think display is better with image left.

In the end, no worries I'm ok with this change.
It can be customized with intranetusercss syspref anyway.
Comment 27 Victor Grousset/tuxayo 2018-04-18 08:22:44 UTC
Created attachment 74417 [details] [review]
Bug 18799: XSLTresultsdisplay hides the icons

If item-level_itypes is turned off and XSLTResultsDisplay is turned on,
biblio-level itemtype images are not shown in staff client search
results. This patch corrects the problem.

This patch also adds some markup and CSS to the itemtype image to make
it look a litte cleaner and work better in result sets in which some
titles have images and some don't.

To test, apply the patch and view the staff client search results page
with various settings:

- XSLT turned on and off
- item-level_itypes turned on and off
- noItemTypeImages turned on and off

Confirm that item type images are shown or not shown correctly according
to each setting.

The difference between this patch and the other patch currently on this
bug is that this patch continues the practice of hiding biblio-level
itemtype information when item-level_itypes is ON.

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>
Comment 28 Victor Grousset/tuxayo 2018-04-18 08:24:24 UTC
(sorry for comment 25, by searching how to get diff of a patch, I somehow triggered a review...)
Comment 29 Nick Clemens 2018-04-20 10:31:43 UTC
Created attachment 74609 [details] [review]
Bug 18799: XSLTresultsdisplay hides the icons

If item-level_itypes is turned off and XSLTResultsDisplay is turned on,
biblio-level itemtype images are not shown in staff client search
results. This patch corrects the problem.

This patch also adds some markup and CSS to the itemtype image to make
it look a litte cleaner and work better in result sets in which some
titles have images and some don't.

To test, apply the patch and view the staff client search results page
with various settings:

- XSLT turned on and off
- item-level_itypes turned on and off
- noItemTypeImages turned on and off

Confirm that item type images are shown or not shown correctly according
to each setting.

The difference between this patch and the other patch currently on this
bug is that this patch continues the practice of hiding biblio-level
itemtype information when item-level_itypes is ON.

Signed-off-by: Victor Grousset <victor.grousset@biblibre.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 30 Jonathan Druart 2018-04-20 15:49:21 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 31 Fridolin SOMERS 2018-06-05 07:27:34 UTC
Pushed to 17.11.x for 17.11.07