Bug 21260

Summary: Improve the Availability line of OPAC XSLT search results
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: OPACAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: david, dcook, fridolin.somers, jonathan.druart, martin.renvoize, nick, victor
Version: masterKeywords: Manual-updated
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5079
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7611
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This report adds preference Reference_NFL_Statuses that allows you to define the not for loan statuses that you want to be reported as Available for reference on the OPAC results display (XSLT based). Basis for further adjustments on bug 26302.
Version(s) released in:
21.05.00
Bug Depends on:    
Bug Blocks: 26302, 28114, 28483, 33567    
Attachments: Flawed search result
Correct data on item detail page
Bug 21260: Indentation at higher levels for Availability/Location
Bug 21260: Simple adjustments in Availability blocks
Bug 21260: Create three main XSLT Availability segments
Bug 21260: Remove item details from reallynotforloan block
Bug 21260: Introduce local pref to affect status grouping
Bug 21260: Move the count before the callnumbers
Bug 21260: (follow-up) Fix typo in holdingbranch segment
Bug 21260: Indentation at higher levels for Availability/Location
Bug 21260: Simple adjustments in Availability blocks
Bug 21260: Create three main XSLT Availability segments
Bug 21260: Remove item details from reallynotforloan block
Bug 21260: Introduce local pref to affect status grouping
Bug 21260: Move the count before the callnumbers
Bug 21260: (follow-up) Fix typo in holdingbranch segment
Bug 21260: Indentation at higher levels for Availability/Location
Bug 21260: Simple adjustments in Availability blocks
Bug 21260: Create three main XSLT Availability segments
Bug 21260: Remove item details from reallynotforloan block
Bug 21260: Introduce local pref to affect status grouping
Bug 21260: Move the count before the callnumbers
Bug 21260: (follow-up) Fix typo in holdingbranch segment
Bug 21260: (follow-up) Consolidate preference

Description Marcel de Rooy 2018-08-23 11:00:24 UTC
It seems that bug 5079 introduced the one item in lines like:

Availability: Items available for loan: [Call number: XXX](2).

The following would imo be more clear:
Availability: Items available for loan (2): [Call number: XXX, YYY].

The Call number part still being a span which could be hidden with css.

Two changes:
[1] Move the item count before the bracket list and make it bold too.
[2] List a few item call numbers (not all). We could control this with a preference providing the number listed (now 1 by the chosen condition in the XSLT sheet).

Essentially, this will be a code simplification. We do not need the generate-id construction in the current code, etc.
Comment 1 David Cook 2020-05-27 05:17:24 UTC
I think that this is actually a bug fix rather than an enhancement, as "[Call number: XXX](2)" is actually 100% incorrect. As you say, it should be "[Call number: XXX, YYY]" or "[Call number: XXX (1), YYY (1)]".

At the moment, the mechanism behind the "Availability", "Location", and related fields in the search results is fundamentally flawed, and is very problematic when you have multiple items and different call numbers.
Comment 2 David Cook 2020-05-27 05:17:43 UTC
Created attachment 105375 [details]
Flawed search result
Comment 3 David Cook 2020-05-27 05:18:06 UTC
Created attachment 105376 [details]
Correct data on item detail page
Comment 4 David Cook 2020-05-27 05:37:47 UTC
It could be interesting to look at using a more familiar "map" solution using EXSLT like so https://www.devsumo.com/technotes/2013/08/xslt-using-lists-and-maps-as-template-parameters/

Alternatively, maybe we should change how we handle items in the search results? Maybe something more like the detail page which could be collapsible / have a "display more" feature.
Comment 5 Marcel de Rooy 2020-08-19 13:52:47 UTC
A TODO from 7611:

vailability line looks like: Block 1 (Alternate holdings) Block 2 (For loan with callno) Block 3 (Reference block with callno) Block 4 (if with colon) Block 5 (reallynotforloan with callno) Block 6 (list of other statuses)

On order status is no longer set in C4/XSLT; the term reallynotforloan is used now and is quite confusing to me?
Block 4 with the colon and the <br> in block 5 are dubious.
No big difference between block reference and block reallynotforloan.
Comment 6 Marcel de Rooy 2020-08-25 14:57:23 UTC
Created attachment 109107 [details] [review]
Bug 21260: Indentation at higher levels for Availability/Location

Only whitespace changes and few comments.

Test plan:
Nothing to test. Count the spaces ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Marcel de Rooy 2020-08-25 14:57:26 UTC
Created attachment 109108 [details] [review]
Bug 21260: Simple adjustments in Availability blocks

[1] Add reallynotforloan=0 test to 'Block 1'. Add a period after No items available.
[2] Remove the if with colon block. Remove the <br> and add a span for reallynotforloan (items on order).
[3] Add a 'Not available' label for the reallynotforloan block.
[4] Remove the On order status, since it is not used anymore since 7611.

Test plan:
See subsequent patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2020-08-25 14:57:30 UTC
Created attachment 109109 [details] [review]
Bug 21260: Create three main XSLT Availability segments

XSLT changes:
[1] Add an item count at the start. Also add variables for counting
    status available and reference. Use these vars in the corresponding
    'blocks'.
[2] Refine the No items-test with the new itemcount.
[3] Combine the reallynotforloan block with the other statuses by
    refining (extending) its initial test. All if's are moved up into
    the former block but are unchanged.

Result of these changes makes that the Availability line consists of three
segments: 1 Available items, 2 Reference items, 3 Other statuses.

Test plan:
[1] Check a biblio without any items in the OAPC results. You should
    see the No items only here.
[2] Check a biblio with one item checked out. You should see only the
    'third' segment with Checked out(1).
[3] Check a biblio with one available and a notforloan==1. You should
    see segment 1 and 2. Both listing item call numbers.
[4] Check a biblio with one available and a notforloan==-1. You should
    see segment 1 and 3. Both listing item call numbers. (See also the
    following patch.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2020-08-25 14:57:34 UTC
Created attachment 109110 [details] [review]
Bug 21260: Remove item details from reallynotforloan block

No need to display itemcallnumbers and branch info for items that
are ('really') not available.
We only show a status and a count for the reallynotforloan ('ordered')
categories (not per branch). This simplifies the code too.
We use the preceding-sibling axis to loop over the unique substatuses.

Test plan:
[1] Pick a biblio with an available item A, an item B with notforloan 1,
    an item C with notforloan 2 and two damaged items.
    Include it in a OPAC search. You should see:
        Available: A. Reference: B, C. Not-available: Damaged(2).
[2] Edit item C (notforloan 2). Change to notforloan -1.
    Search again. You should see:
        Available: A. Reference: B. Not-available: On order(1), Damaged(2).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2020-08-25 14:57:38 UTC
Created attachment 109111 [details] [review]
Bug 21260: Introduce local pref to affect status grouping

Introducing a local preference Available_NFL to control which not for
loan statuses are considered to be 'available for reference'.
Standard value is '1|2' which comes down to the former >0 when using
the initial Koha defaults.

Test plan:
[1] Pick a biblio with an available item A, an item B with notforloan 1,
    an item C with notforloan 2 and two damaged items. (Former patch.)
    Include it in a OPAC search. You should see:
        Available: A. Reference: B, C. Not-available: Damaged(2).
[2] Add local pref Available_NFL with value '1'. Repeat the search.
    You should see now:
        Available: A. Reference: B. Not-available: S.C.(1), Damaged(2).
    [where S.C. stands for Staff Collection]
[3] Run test t/db_dependent/XSLT.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2020-08-25 14:57:42 UTC
Created attachment 109112 [details] [review]
Bug 21260: Move the count before the callnumbers

The current display is a bit confusing [Call number: A](2) seems
to indicate that we have two call numbers A. But what it means here,
is: we have two items and we list only one, being A.

So, we'd better do something like:
    Items available for loan: Centerville (2) [Call number: A, ..]
Which says we have two items at Centerville, listing only one (A),
but indicating with the two dots that there are more.

Test plan:
Pick a biblio with multiple available items at a branch.
Include it in a search and check results display.
Test the same but with only one branch in Koha.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2020-08-25 15:00:43 UTC
NOTE: Obviously we can promote the local pref to a real pref if this development meets sufficient interest.
Comment 13 Marcel de Rooy 2020-08-26 13:42:20 UTC
FIXME
                                           <span class="CallNumberAndLabel">
                                                <span class="LabelCallNumber">Call number: </span>
                                                <span class="CallNumber"><xsl:value-of select="items:itemcallnumber"/><xsl:if test="count(key('item-by-status-and-branch-home', concat(items:status, ' ', items:homebranch)))>1"><xsl:text>, ..</xsl:text></xsl:if></span>
                                           </span>
Comment 14 Marcel de Rooy 2020-08-26 14:02:26 UTC
Created attachment 109182 [details] [review]
Bug 21260: (follow-up) Fix typo in holdingbranch segment

Copy-and-paste error. We need holdingbranch here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Victor Grousset/tuxayo 2020-11-28 18:45:57 UTC
Is the status "Needs Signoff" correct? All the patches have been signed off.
Comment 16 Marcel de Rooy 2020-11-30 06:59:54 UTC
(In reply to Victor Grousset/tuxayo from comment #15)
> Is the status "Needs Signoff" correct? All the patches have been signed off.

By the author only. So he tested :)
Comment 17 Victor Grousset/tuxayo 2020-12-08 20:10:35 UTC
Indeed, that makes sense ^^

Is there a step to do before testing except restarting all?
I'm getting
> Availability: Items available for loan: 1 , 1 , 2 . 
Instead of 
> Availability: Items available for loan: Midway (1), Centerville (1), Fairview (2). 
https://wtf.roflcopter.fr/pics/zlFdbnni/w3vBdmWN.png
Comment 18 Ava 2021-01-19 01:13:47 UTC
Created attachment 115265 [details] [review]
Bug 21260: Indentation at higher levels for Availability/Location

Only whitespace changes and few comments.

Test plan:
Nothing to test. Count the spaces ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: ava li <rubyli208@gmail.com>
Comment 19 Ava 2021-01-19 01:13:51 UTC
Created attachment 115266 [details] [review]
Bug 21260: Simple adjustments in Availability blocks

[1] Add reallynotforloan=0 test to 'Block 1'. Add a period after No items available.
[2] Remove the if with colon block. Remove the <br> and add a span for reallynotforloan (items on order).
[3] Add a 'Not available' label for the reallynotforloan block.
[4] Remove the On order status, since it is not used anymore since 7611.

Test plan:
See subsequent patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: ava li <rubyli208@gmail.com>
Comment 20 Ava 2021-01-19 01:13:56 UTC
Created attachment 115267 [details] [review]
Bug 21260: Create three main XSLT Availability segments

XSLT changes:
[1] Add an item count at the start. Also add variables for counting
    status available and reference. Use these vars in the corresponding
    'blocks'.
[2] Refine the No items-test with the new itemcount.
[3] Combine the reallynotforloan block with the other statuses by
    refining (extending) its initial test. All if's are moved up into
    the former block but are unchanged.

Result of these changes makes that the Availability line consists of three
segments: 1 Available items, 2 Reference items, 3 Other statuses.

Test plan:
[1] Check a biblio without any items in the OAPC results. You should
    see the No items only here.
[2] Check a biblio with one item checked out. You should see only the
    'third' segment with Checked out(1).
[3] Check a biblio with one available and a notforloan==1. You should
    see segment 1 and 2. Both listing item call numbers.
[4] Check a biblio with one available and a notforloan==-1. You should
    see segment 1 and 3. Both listing item call numbers. (See also the
    following patch.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: ava li <rubyli208@gmail.com>
Comment 21 Ava 2021-01-19 01:13:59 UTC
Created attachment 115268 [details] [review]
Bug 21260: Remove item details from reallynotforloan block

No need to display itemcallnumbers and branch info for items that
are ('really') not available.
We only show a status and a count for the reallynotforloan ('ordered')
categories (not per branch). This simplifies the code too.
We use the preceding-sibling axis to loop over the unique substatuses.

Test plan:
[1] Pick a biblio with an available item A, an item B with notforloan 1,
    an item C with notforloan 2 and two damaged items.
    Include it in a OPAC search. You should see:
        Available: A. Reference: B, C. Not-available: Damaged(2).
[2] Edit item C (notforloan 2). Change to notforloan -1.
    Search again. You should see:
        Available: A. Reference: B. Not-available: On order(1), Damaged(2).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: ava li <rubyli208@gmail.com>
Comment 22 Ava 2021-01-19 01:14:03 UTC
Created attachment 115269 [details] [review]
Bug 21260: Introduce local pref to affect status grouping

Introducing a local preference Available_NFL to control which not for
loan statuses are considered to be 'available for reference'.
Standard value is '1|2' which comes down to the former >0 when using
the initial Koha defaults.

Test plan:
[1] Pick a biblio with an available item A, an item B with notforloan 1,
    an item C with notforloan 2 and two damaged items. (Former patch.)
    Include it in a OPAC search. You should see:
        Available: A. Reference: B, C. Not-available: Damaged(2).
[2] Add local pref Available_NFL with value '1'. Repeat the search.
    You should see now:
        Available: A. Reference: B. Not-available: S.C.(1), Damaged(2).
    [where S.C. stands for Staff Collection]
[3] Run test t/db_dependent/XSLT.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: ava li <rubyli208@gmail.com>
Comment 23 Ava 2021-01-19 01:14:07 UTC
Created attachment 115270 [details] [review]
Bug 21260: Move the count before the callnumbers

The current display is a bit confusing [Call number: A](2) seems
to indicate that we have two call numbers A. But what it means here,
is: we have two items and we list only one, being A.

So, we'd better do something like:
    Items available for loan: Centerville (2) [Call number: A, ..]
Which says we have two items at Centerville, listing only one (A),
but indicating with the two dots that there are more.

Test plan:
Pick a biblio with multiple available items at a branch.
Include it in a search and check results display.
Test the same but with only one branch in Koha.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: ava li <rubyli208@gmail.com>
Comment 24 Ava 2021-01-19 01:14:10 UTC
Created attachment 115271 [details] [review]
Bug 21260: (follow-up) Fix typo in holdingbranch segment

Copy-and-paste error. We need holdingbranch here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: ava li <rubyli208@gmail.com>
Comment 25 Martin Renvoize 2021-03-22 10:59:41 UTC
Could you summarise what this patchset changes, perhaps in the 'text for release notes'.

I'm struggling to wrap my head around it.
Comment 26 Marcel de Rooy 2021-03-22 12:32:16 UTC
(In reply to Martin Renvoize from comment #25)
> Could you summarise what this patchset changes, perhaps in the 'text for
> release notes'.
> 
> I'm struggling to wrap my head around it.

I am willing to condense the commit messages and copy them into the release notes. These messages should probably be enough for now?
Comment 27 Martin Renvoize 2021-03-25 13:51:16 UTC
Created attachment 118800 [details] [review]
Bug 21260: Indentation at higher levels for Availability/Location

Only whitespace changes and few comments.

Test plan:
Nothing to test. Count the spaces ;)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: ava li <rubyli208@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2021-03-25 13:51:19 UTC
Created attachment 118801 [details] [review]
Bug 21260: Simple adjustments in Availability blocks

[1] Add reallynotforloan=0 test to 'Block 1'. Add a period after No items available.
[2] Remove the if with colon block. Remove the <br> and add a span for reallynotforloan (items on order).
[3] Add a 'Not available' label for the reallynotforloan block.
[4] Remove the On order status, since it is not used anymore since 7611.

Test plan:
See subsequent patches.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: ava li <rubyli208@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2021-03-25 13:51:23 UTC
Created attachment 118802 [details] [review]
Bug 21260: Create three main XSLT Availability segments

XSLT changes:
[1] Add an item count at the start. Also add variables for counting
    status available and reference. Use these vars in the corresponding
    'blocks'.
[2] Refine the No items-test with the new itemcount.
[3] Combine the reallynotforloan block with the other statuses by
    refining (extending) its initial test. All if's are moved up into
    the former block but are unchanged.

Result of these changes makes that the Availability line consists of three
segments: 1 Available items, 2 Reference items, 3 Other statuses.

Test plan:
[1] Check a biblio without any items in the OAPC results. You should
    see the No items only here.
[2] Check a biblio with one item checked out. You should see only the
    'third' segment with Checked out(1).
[3] Check a biblio with one available and a notforloan==1. You should
    see segment 1 and 2. Both listing item call numbers.
[4] Check a biblio with one available and a notforloan==-1. You should
    see segment 1 and 3. Both listing item call numbers. (See also the
    following patch.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: ava li <rubyli208@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 30 Martin Renvoize 2021-03-25 13:51:26 UTC
Created attachment 118803 [details] [review]
Bug 21260: Remove item details from reallynotforloan block

No need to display itemcallnumbers and branch info for items that
are ('really') not available.
We only show a status and a count for the reallynotforloan ('ordered')
categories (not per branch). This simplifies the code too.
We use the preceding-sibling axis to loop over the unique substatuses.

Test plan:
[1] Pick a biblio with an available item A, an item B with notforloan 1,
    an item C with notforloan 2 and two damaged items.
    Include it in a OPAC search. You should see:
        Available: A. Reference: B, C. Not-available: Damaged(2).
[2] Edit item C (notforloan 2). Change to notforloan -1.
    Search again. You should see:
        Available: A. Reference: B. Not-available: On order(1), Damaged(2).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: ava li <rubyli208@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 31 Martin Renvoize 2021-03-25 13:51:29 UTC
Created attachment 118804 [details] [review]
Bug 21260: Introduce local pref to affect status grouping

Introducing a local preference Available_NFL to control which not for
loan statuses are considered to be 'available for reference'.
Standard value is '1|2' which comes down to the former >0 when using
the initial Koha defaults.

Test plan:
[1] Pick a biblio with an available item A, an item B with notforloan 1,
    an item C with notforloan 2 and two damaged items. (Former patch.)
    Include it in a OPAC search. You should see:
        Available: A. Reference: B, C. Not-available: Damaged(2).
[2] Add local pref Available_NFL with value '1'. Repeat the search.
    You should see now:
        Available: A. Reference: B. Not-available: S.C.(1), Damaged(2).
    [where S.C. stands for Staff Collection]
[3] Run test t/db_dependent/XSLT.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: ava li <rubyli208@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2021-03-25 13:51:33 UTC
Created attachment 118805 [details] [review]
Bug 21260: Move the count before the callnumbers

The current display is a bit confusing [Call number: A](2) seems
to indicate that we have two call numbers A. But what it means here,
is: we have two items and we list only one, being A.

So, we'd better do something like:
    Items available for loan: Centerville (2) [Call number: A, ..]
Which says we have two items at Centerville, listing only one (A),
but indicating with the two dots that there are more.

Test plan:
Pick a biblio with multiple available items at a branch.
Include it in a search and check results display.
Test the same but with only one branch in Koha.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: ava li <rubyli208@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2021-03-25 13:51:36 UTC
Created attachment 118806 [details] [review]
Bug 21260: (follow-up) Fix typo in holdingbranch segment

Copy-and-paste error. We need holdingbranch here.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: ava li <rubyli208@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Martin Renvoize 2021-03-25 13:51:50 UTC
Passing QA
Comment 35 Marcel de Rooy 2021-03-26 07:02:56 UTC
(In reply to Martin Renvoize from comment #34)
> Passing QA

Thx
Comment 36 Jonathan Druart 2021-04-01 16:53:23 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 37 Fridolin Somers 2021-04-07 13:26:01 UTC
This visual change may be too disturbing for stable versions right ?
I'd say we do not backport.
Comment 38 Marcel de Rooy 2021-04-07 13:41:21 UTC
(In reply to Fridolin Somers from comment #37)
> This visual change may be too disturbing for stable versions right ?
> I'd say we do not backport.

Agree
Comment 39 Jonathan Druart 2021-04-13 13:31:49 UTC
I overlooked this one, I didn't notice the use of the "local pref".

Why didn't we create a "real" system preference here?
Comment 40 Marcel de Rooy 2021-04-13 13:33:34 UTC
(In reply to Jonathan Druart from comment #39)
> I overlooked this one, I didn't notice the use of the "local pref".
> 
> Why didn't we create a "real" system preference here?

Comment12
Do you need a follow-up here?
Comment 41 Jonathan Druart 2021-04-13 13:56:17 UTC
(In reply to Marcel de Rooy from comment #40)
> (In reply to Jonathan Druart from comment #39)
> > I overlooked this one, I didn't notice the use of the "local pref".
> > 
> > Why didn't we create a "real" system preference here?
> 
> Comment12
> Do you need a follow-up here?

Yes please.
Comment 42 Marcel de Rooy 2021-04-14 12:27:01 UTC
Created attachment 119569 [details] [review]
Bug 21260: (follow-up) Consolidate preference

The bug added a local preference. This should now become a real one.
Renamed in the process to better reflect its use.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 43 Jonathan Druart 2021-04-14 14:09:08 UTC
Follow-up pushed to master.