Bug 26302 - OPAC XSLT Results: List variable number of itemcallnumbers
Summary: OPAC XSLT Results: List variable number of itemcallnumbers
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords: Manual
Depends on: 21260
Blocks: 28831
  Show dependency treegraph
 
Reported: 2020-08-26 13:00 UTC by Marcel de Rooy
Modified: 2021-09-09 12:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Text to go in the release notes:
This enhancement allows customizing the number of call numbers displayed for OPAC search results for items available and not available by changing two new system preferences: - OPACResultsMaxItems: maximum number of available items displayed in search results (default = 1) - OPACResultsMaxItemsUnavailable - maximum number of unavailable items displayed in search results (such as when checked out and damaged) (default = 0) This is useful when records have a large number of items, for example larger libraries with many branches, union catalogues, and university libraries with course text books.
Version(s) released in:
21.11.00


Attachments
Bug 26302: OPAC XSLT Results: List variable number of itemcallnumbers (9.37 KB, patch)
2020-08-26 14:56 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: OPAC XSLT Results: List variable number of itemcallnumbers (9.37 KB, patch)
2021-04-02 09:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Add syspref resultsMaxItemsUnavailable (1.06 KB, patch)
2021-04-02 09:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Use resultsMaxItemsUnavailable in stylesheet (24.21 KB, patch)
2021-04-02 09:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Use resultsMaxItemsUnavailable in stylesheet (8.64 KB, patch)
2021-04-02 09:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: OPAC XSLT Results: List variable number of itemcallnumbers (9.40 KB, patch)
2021-06-24 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Add syspref resultsMaxItemsUnavailable (1.06 KB, patch)
2021-06-24 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Use resultsMaxItemsUnavailable in stylesheet (8.63 KB, patch)
2021-06-24 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Add dbrev for two new prefs (3.59 KB, patch)
2021-06-24 13:46 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Apply resultsMaxItemsUnavailable to the NFLs too (2.70 KB, patch)
2021-06-24 14:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: OPAC XSLT Results: List variable number of itemcallnumbers (9.41 KB, patch)
2021-06-26 23:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: Add syspref resultsMaxItemsUnavailable (1.07 KB, patch)
2021-06-26 23:17 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: Use resultsMaxItemsUnavailable in stylesheet (8.64 KB, patch)
2021-06-26 23:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: Add dbrev for two new prefs (3.59 KB, patch)
2021-06-26 23:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: Apply resultsMaxItemsUnavailable to the NFLs too (2.71 KB, patch)
2021-06-26 23:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: (follow-up) Handle zero callnumbers-case (5.23 KB, patch)
2021-06-29 12:27 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Add dbrev for two new prefs (3.60 KB, patch)
2021-08-05 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Add resultbranch and other status in C4/XSLT (3.40 KB, patch)
2021-08-05 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Changes for substatus, resultbranch in XSLT.t (4.62 KB, patch)
2021-08-05 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Refactor Availability block in OPAC results xslt (34.59 KB, patch)
2021-08-05 14:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Add dbrev for two new prefs (3.61 KB, patch)
2021-08-05 18:51 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26302: Add resultbranch and other status in C4/XSLT (3.41 KB, patch)
2021-08-05 18:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26302: Changes for substatus, resultbranch in XSLT.t (4.63 KB, patch)
2021-08-05 18:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26302: Refactor Availability block in OPAC results xslt (34.60 KB, patch)
2021-08-05 18:52 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 26302: Add dbrev for two new prefs (3.66 KB, patch)
2021-08-05 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: Add resultbranch and other status in C4/XSLT (3.46 KB, patch)
2021-08-05 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: Changes for substatus, resultbranch in XSLT.t (4.68 KB, patch)
2021-08-05 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: Refactor Availability block in OPAC results xslt (34.65 KB, patch)
2021-08-05 19:47 UTC, David Nind
Details | Diff | Splinter Review
Bug 26302: (QA follow-up) Reset preference values (2.93 KB, patch)
2021-08-09 12:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: (QA follow-up) Prevent empty call numbers (3.32 KB, patch)
2021-08-09 12:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: (QA follow-up) Prevent empty call numbers (3.16 KB, patch)
2021-08-09 12:49 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: Add dbrev for two new prefs (3.71 KB, patch)
2021-08-09 13:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26302: Add resultbranch and other status in C4/XSLT (3.51 KB, patch)
2021-08-09 13:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26302: Changes for substatus, resultbranch in XSLT.t (4.74 KB, patch)
2021-08-09 13:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26302: Refactor Availability block in OPAC results xslt (34.70 KB, patch)
2021-08-09 13:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26302: (QA follow-up) Reset preference values (2.94 KB, patch)
2021-08-09 13:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26302: (QA follow-up) Prevent empty call numbers (3.18 KB, patch)
2021-08-09 13:03 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 26302: Make syspref's names start with uppercase (9.02 KB, patch)
2021-08-30 13:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 26302: Make syspref's names start with uppercase (9.10 KB, patch)
2021-08-30 14:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 26302: (QA follow-up) Prefix prefs with OPAC (9.45 KB, patch)
2021-08-30 14:25 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 2020-08-26 13:00:40 UTC

    
Comment 1 Marcel de Rooy 2020-08-26 14:56:24 UTC Comment hidden (obsolete)
Comment 2 Ella 2021-01-18 23:16:23 UTC
I do not see that preference, can you please tell me where to find it? 
Thanks
Comment 3 Katrin Fischer 2021-01-18 23:23:50 UTC
I believe you are supposed to add one yourself in the "local use" tab, but I am not sure where this patch is headed with that approach. Marcel, can you please explain? There seems to be no other linked bugs to take this further.
Comment 4 Marcel de Rooy 2021-01-19 12:09:10 UTC
(In reply to Ella from comment #2)
> I do not see that preference, can you please tell me where to find it? 
> Thanks

Sorry, it is a local preference. Note the Local tab in the Preferences section under Administration.
Comment 5 Marcel de Rooy 2021-01-19 12:10:19 UTC
(In reply to Katrin Fischer from comment #3)
> I believe you are supposed to add one yourself in the "local use" tab, but I
> am not sure where this patch is headed with that approach. Marcel, can you
> please explain? There seems to be no other linked bugs to take this further.

Depending on further feedback and consensus, it will be rather easy to promote this to an official preference.
Comment 6 Marcel de Rooy 2021-04-02 09:36:42 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2021-04-02 09:36:46 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2021-04-02 09:36:50 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2021-04-02 09:40:07 UTC
@Nick: Would this address your concerns about hiding call numbers for certain unavailable categories?
Comment 10 Marcel de Rooy 2021-04-02 09:41:47 UTC Comment hidden (obsolete)
Comment 11 Marcel de Rooy 2021-04-02 09:43:34 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2021-06-24 13:46:33 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2021-06-24 13:46:36 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2021-06-24 13:46:40 UTC Comment hidden (obsolete)
Comment 15 Marcel de Rooy 2021-06-24 13:46:43 UTC Comment hidden (obsolete)
Comment 16 Marcel de Rooy 2021-06-24 14:34:51 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2021-06-24 14:35:33 UTC
(In reply to Marcel de Rooy from comment #11)
> We start here with Checked out until On hold.
> Not sure if we should do exactly the same for the remaining
> not for loan categories.

Note that the last patch now fills that gap.
Comment 18 David Nind 2021-06-26 23:17:54 UTC Comment hidden (obsolete)
Comment 19 David Nind 2021-06-26 23:17:58 UTC Comment hidden (obsolete)
Comment 20 David Nind 2021-06-26 23:18:02 UTC Comment hidden (obsolete)
Comment 21 David Nind 2021-06-26 23:18:06 UTC Comment hidden (obsolete)
Comment 22 David Nind 2021-06-26 23:18:10 UTC Comment hidden (obsolete)
Comment 23 David Nind 2021-06-26 23:20:18 UTC
Testing notes (using koha-testing-docker):

Revised test plan:

1. Setup records and items to test:
   . Have records with multiple items and call numbers (one with 20 items and one with 50) that will appear in the OPAC results using the same keyword (use 'Add multiple copies of this item' option at the bottom of the add item form to bulk add multiple items).
   . Make sure (using batch item editing) that there are a mix of libraries, not for loan categories, call numbers, and damaged status'
   . As a bonus, add some additional not for loan categories (Administration > Basic parameters > Authorized values), such as 'Not for Loan' and 'Staff Collection'
   . Check out some items to patrons
2. Confirm the current behavour when searching in the OPAC (see the example at the end):
   . The first call number per branch is displayed under availablility in the results (for items available for loan and items available for reference)
   . Where there are multiple call numbers for an item, "..." is used to indicate more call numbers
   . For items not available, such as damaged: no call numbers are displayed.
3. Apply the patch
4. Update the database (updatedatabase when using koha-testing-docker)
5. Run flush_memcached and restart_all (when using koha-testing-docker)
6. Without changing the default values for the new system preferences, check that the current behavour still works (as per step 2). Note: the default setting is to display 2 call numbers for items available (only 1 was displayed before) and 1 call number for items not available (no call numbers were displayed before).
7. Experiment with different values for the new system preferences (for example 10 for resultsMaxItems and 0 for resultsMaxItemsUnavailable):
   . resultsMaxItems - Maximum number of available items displayed in search results
   . resultsMaxItemsUnavailable - Maximum number of unavailable items displayed in search results
8. Confirm that the correct number of call numbers are displayed.
9. Sign off!

Example
-------

Before patch applied:
~~~~~~~~~~~~~~~~~~~~~

Availability: Items available for loan: Centerville (16) Call number: PERL A1, .., Fairview (1) Call number: PERL A1, Franklin (7) Call number: PERL B2, .., Midway (9) Call number: 560.897 ZAK, .., Springfield (6) Call number: 965.009 PLO, ... Items available for reference: Fairview Not For Loan (1) Call number: PERL A1, Franklin Not For Loan (1) Call number: PERL B2, Midway Not For Loan (2) Call number: 560.897 ZAK, .., Centerville Not For Loan (1) Call number: PERL B2, Springfield Not For Loan (3) Call number: 965.009 PLO, ... Not available: Damaged (7). 

Patch applied:
~~~~~~~~~~~~~~

Availability: Items available for loan: Centerville (16) Call number: PERL A1, PERL A1, .., Fairview (1) Call number: PERL A1, PERL A1, Franklin (7) Call number: PERL A1, PERL A1, .., Midway (9) Call number: PERL A1, PERL A1, .., Springfield (6) Call number: PERL A1, PERL A1, ... Items available for reference: Fairview Not For Loan (1) Call number: PERL A1, PERL B2, Franklin Not For Loan (1) Call number: PERL A1, PERL B2, Midway Not For Loan (2) Call number: PERL A1, PERL B2, Centerville Not For Loan (1) Call number: PERL A1, PERL B2, Springfield Not For Loan (3) Call number: PERL A1, PERL B2, ... Not available: Damaged (7) [ PERL A1, .. ]. 

Note differences:
- Default number of call numbers peated for items available for loan (before patch was applied they didn't), 
- Not available items didn't originally dispaly Call numbers for Damaged

Different values
~~~~~~~~~~~~~~~~

Results with:
- resultsMaxItems: 10
- resultsMaxItemsUnavailable: 0

Availability: Items available for loan: Centerville (16) Call number: PERL A1, PERL A1, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, 560.897 ZAK, .., Fairview (1) Call number: PERL A1, PERL A1, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, 560.897 ZAK, Franklin (7) Call number: PERL A1, PERL A1, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, 560.897 ZAK, Midway (9) Call number: PERL A1, PERL A1, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, 560.897 ZAK, Springfield (6) Call number: PERL A1, PERL A1, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, PERL B2, 560.897 ZAK. Items available for reference: Fairview Not For Loan (1) Call number: PERL A1, PERL B2, 560.897 ZAK, 560.897 ZAK, PERL B2, 965.009 PLO, 965.009 PLO, 965.009 PLO, Franklin Not For Loan (1) Call number: PERL A1, PERL B2, 560.897 ZAK, 560.897 ZAK, PERL B2, 965.009 PLO, 965.009 PLO, 965.009 PLO, Midway Not For Loan (2) Call number: PERL A1, PERL B2, 560.897 ZAK, 560.897 ZAK, PERL B2, 965.009 PLO, 965.009 PLO, 965.009 PLO, Centerville Not For Loan (1) Call number: PERL A1, PERL B2, 560.897 ZAK, 560.897 ZAK, PERL B2, 965.009 PLO, 965.009 PLO, 965.009 PLO, Springfield Not For Loan (3) Call number: PERL A1, PERL B2, 560.897 ZAK, 560.897 ZAK, PERL B2, 965.009 PLO, 965.009 PLO, 965.009 PLO. Not available: Damaged (7). 

Results with:
- resultsMaxItems: 1
- resultsMaxItemsUnavailable: 0

Availability: Items available for loan: Centerville (16) Call number: PERL A1, .., Fairview (1) Call number: PERL A1, Franklin (7) Call number: PERL A1, .., Midway (9) Call number: PERL A1, .., Springfield (6) Call number: PERL A1, ... Items available for reference: Fairview Not For Loan (1) Call number: PERL A1, Franklin Not For Loan (1) Call number: PERL A1, Midway Not For Loan (2) Call number: PERL A1, .., Centerville Not For Loan (1) Call number: PERL A1, Springfield Not For Loan (3) Call number: PERL A1, ... Not available: Damaged (7). 

Results with:
- resultsMaxItems: 0
- resultsMaxItemsUnavailable: 0

Availability: Items available for loan: Centerville (16) Call number: PERL A1, .., Franklin (7) Call number: PERL A1, .., Midway (9) Call number: PERL A1, .., Springfield (6) Call number: PERL A1, ... Items available for reference: Fairview Not For Loan (1) Call number: PERL A1, Franklin Not For Loan (1) Call number: PERL A1, Midway Not For Loan (2) Call number: PERL A1, .., Centerville Not For Loan (1) Call number: PERL A1, Springfield Not For Loan (3) Call number: PERL A1, ... Not available: Checked out (1). Damaged (7). 

Note: no change from setting to 1 and 0
Comment 24 David Nind 2021-06-26 23:29:05 UTC
A couple of things I noted:

1. Should the call number be repeated if it is the same? That is, should only distinct call numbers be displayed for items available by library branch or not available categories?

2. Should using 0 for resultsMaxItems suppress the display of call numbers? That is the behavour when resultsMaxItemsUnavailable is 0.
Comment 25 Marcel de Rooy 2021-06-28 05:24:02 UTC
(In reply to David Nind from comment #24)
> A couple of things I noted:
> 
> 1. Should the call number be repeated if it is the same? That is, should
> only distinct call numbers be displayed for items available by library
> branch or not available categories?
> 
> 2. Should using 0 for resultsMaxItems suppress the display of call numbers?
> That is the behavour when resultsMaxItemsUnavailable is 0.

Thx for testing, David! Great comments. Will be looking further shortly.
Comment 26 Marcel de Rooy 2021-06-28 12:46:38 UTC
David,
This is the culprit:
$sp ||= 1 if $syspref eq 'resultsMaxItems'; # needs default

A zero becomes a 1 here. We should probably test for defined here with //.
Comment 27 Marcel de Rooy 2021-06-29 12:27:21 UTC Comment hidden (obsolete)
Comment 28 Marcel de Rooy 2021-06-29 12:40:45 UTC
(In reply to David Nind from comment #24)
> 1. Should the call number be repeated if it is the same? That is, should
> only distinct call numbers be displayed for items available by library
> branch or not available categories?

This is far from trivially adding a distinct keyword. We are relying here on the output of the xslt key function. Making this change would require a bit more than we are doing here now to control the number displayed.

> 2. Should using 0 for resultsMaxItems suppress the display of call numbers?
> That is the behavour when resultsMaxItemsUnavailable is 0.

The follow-up now resolves this.
Comment 29 Nick Clemens 2021-07-21 13:15:17 UTC
I am seeing the same callnumbers for every branch, to recreate:
1 - sudo koha-mysql kohadev
2 - UPDATE items SET itemcallnumber=barcode;
3 - Add 5 items for a single branch (IPT) with IPT1-5 as barcodes and "IPT" as item callnumber
4 - Display in opac:
Availability: Items available for loan: Midway (1) Call number: 39999000013351, 39999000013375, 39999000013399, Centerville (3) Call number: 39999000013351, 39999000013375, 39999000013399, Fairview (1) Call number: 39999000013351, 39999000013375, 39999000013399, Institut Protestant de Théologie (5) Call number: 39999000013351, 39999000013375, 39999000013399, ...
Comment 30 Marcel de Rooy 2021-07-28 12:06:39 UTC
(In reply to Nick Clemens from comment #29)
> I am seeing the same callnumbers for every branch, to recreate:
> 1 - sudo koha-mysql kohadev
> 2 - UPDATE items SET itemcallnumber=barcode;
> 3 - Add 5 items for a single branch (IPT) with IPT1-5 as barcodes and "IPT"
> as item callnumber
> 4 - Display in opac:
> Availability: Items available for loan: Midway (1) Call number:
> 39999000013351, 39999000013375, 39999000013399, Centerville (3) Call number:
> 39999000013351, 39999000013375, 39999000013399, Fairview (1) Call number:
> 39999000013351, 39999000013375, 39999000013399, Institut Protestant de
> Théologie (5) Call number: 39999000013351, 39999000013375, 39999000013399,
> ...

Thx for testing, Nick. Will have another look
Comment 31 Marcel de Rooy 2021-08-05 14:20:44 UTC Comment hidden (obsolete)
Comment 32 Marcel de Rooy 2021-08-05 14:20:48 UTC Comment hidden (obsolete)
Comment 33 Marcel de Rooy 2021-08-05 14:20:52 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2021-08-05 14:20:56 UTC Comment hidden (obsolete)
Comment 35 Marcel de Rooy 2021-08-05 14:23:32 UTC
 .../en/xslt/MARC21slim2OPACResults.xsl        | 383 +++++++-----------
 1 file changed, 144 insertions(+), 239 deletions(-)
Comment 36 Marcel de Rooy 2021-08-05 14:24:45 UTC
David, would you mind to test this change again?
Hopefully, Nick could pick it up after that :)
Comment 37 Marcel de Rooy 2021-08-05 14:28:51 UTC
(In reply to Marcel de Rooy from comment #28)
> (In reply to David Nind from comment #24)
> > 1. Should the call number be repeated if it is the same? That is, should
> > only distinct call numbers be displayed for items available by library
> > branch or not available categories?
> 
> This is far from trivially adding a distinct keyword. We are relying here on
> the output of the xslt key function. Making this change would require a bit
> more than we are doing here now to control the number displayed.

Looking back, I must have misinterpreted this comment. ;)
Comment 38 Owen Leonard 2021-08-05 18:51:59 UTC
Created attachment 123526 [details] [review]
Bug 26302: Add dbrev for two new prefs

Adding two prefs:
    resultsMaxItems
    resultsMaxItemsUnavailable

Test plan:
Check Preferences/OPAC/Appearance.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 39 Owen Leonard 2021-08-05 18:52:03 UTC
Created attachment 123527 [details] [review]
Bug 26302: Add resultbranch and other status in C4/XSLT

Groundwork for changes in the OPAC results xslt.

NOTE: Adds both new prefs too.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 40 Owen Leonard 2021-08-05 18:52:08 UTC
Created attachment 123528 [details] [review]
Bug 26302: Changes for substatus, resultbranch in XSLT.t

Test plan:
Run t/db_dependent/XSLT.t

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 41 Owen Leonard 2021-08-05 18:52:12 UTC
Created attachment 123529 [details] [review]
Bug 26302: Refactor Availability block in OPAC results xslt

Decided finally to do a larger refactor here. Hopefully we improved
consistency and removed some redundancy. And the two new prefs provide
additional functionality.

Most significant changes:
[1] Clearer distinction of the Availability line in three parts:
    Available, Reference and Unavailable.
    For Unavailable we loop thru branches now too.
[2] Calling template to list item data (including location or ccode).
    Made the separate Location line obsolete.
[3] The tests around OPACItemLibrary are removed since we now look at
    resultbranch from XSLT.pm.
[4] Removed code replication for various 'other' statuses like Checked out.
[5] Obsoleted three xslt key indexes, singleBranchMode.
[6] Apply the two prefs to control number of listed items.

Test plan:
You may play with: OPACResultsLibrary (home/holding), OPACItemLocation
(library, callno, location, ccode), resultsMaxItems[Unavailable] (numbers),
Reference_NFL_Statuses (list of notforloan codes in reference part).

[1] Create a biblio with various items on a few branches. Fill call number,
    location and ccode too. Set home branch and holdingbranch differently.
[2] Toggle the preferences, and verify display within OAPC search results.

Example with prefs (home, callnumber, 2, 2):
Availability: Items available for loan: Centerville (2)Call number: perl A 4, PERL D 1. Items available for reference: Fairfield: Not For Loan (1)Call number: PERL A 5. Not available: Centerville: Checked out (1)Call number: PERL A 3. Centerville: Ordered (1)Call number: PERL B 1. Centerville: Staff Collection (2)Call number: PERL A 2, PERL E 1. Centerville: Withdrawn (1)Call number: PERL B 2. Fairfield: Withdrawn (1)Call number: PERL C 1.

Same data with prefs (holding, callnumber, 2, 2):
Availability: Items available for loan: Centerville (1)Call number: PERL D 1. Liberty (1)Call number: perl A 4. Items available for reference: Centerville: Not For Loan (1)Call number: PERL A 5. Not available: Centerville: Checked out (1)Call number: PERL A 3. Centerville: Staff Collection (1)Call number: PERL A 2. Centerville: Withdrawn (1)Call number: PERL B 2. Fairfield: Ordered (1)Call number: PERL B 1. Fairfield: Withdrawn (1)Call number: PERL C 1. Liberty: Staff Collection (1)Call number: PERL E 1.

Same data with prefs (holding, library, n/a, n/a):
Availability: Items available for loan: Centerville (1). Liberty (1). Items available for reference: Centerville: Not For Loan (1). Not available: Centerville: Checked out (1). Centerville: Staff Collection (1). Centerville: Withdrawn (1). Fairfield: Ordered (1). Fairfield: Withdrawn (1). Liberty: Staff Collection (1).

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 42 David Nind 2021-08-05 19:47:42 UTC
Created attachment 123531 [details] [review]
Bug 26302: Add dbrev for two new prefs

Adding two prefs:
    resultsMaxItems
    resultsMaxItemsUnavailable

Test plan:
Check Preferences/OPAC/Appearance.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>
Comment 43 David Nind 2021-08-05 19:47:47 UTC
Created attachment 123532 [details] [review]
Bug 26302: Add resultbranch and other status in C4/XSLT

Groundwork for changes in the OPAC results xslt.

NOTE: Adds both new prefs too.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>
Comment 44 David Nind 2021-08-05 19:47:52 UTC
Created attachment 123533 [details] [review]
Bug 26302: Changes for substatus, resultbranch in XSLT.t

Test plan:
Run t/db_dependent/XSLT.t

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>
Comment 45 David Nind 2021-08-05 19:47:58 UTC
Created attachment 123534 [details] [review]
Bug 26302: Refactor Availability block in OPAC results xslt

Decided finally to do a larger refactor here. Hopefully we improved
consistency and removed some redundancy. And the two new prefs provide
additional functionality.

Most significant changes:
[1] Clearer distinction of the Availability line in three parts:
    Available, Reference and Unavailable.
    For Unavailable we loop thru branches now too.
[2] Calling template to list item data (including location or ccode).
    Made the separate Location line obsolete.
[3] The tests around OPACItemLibrary are removed since we now look at
    resultbranch from XSLT.pm.
[4] Removed code replication for various 'other' statuses like Checked out.
[5] Obsoleted three xslt key indexes, singleBranchMode.
[6] Apply the two prefs to control number of listed items.

Test plan:
You may play with: OPACResultsLibrary (home/holding), OPACItemLocation
(library, callno, location, ccode), resultsMaxItems[Unavailable] (numbers),
Reference_NFL_Statuses (list of notforloan codes in reference part).

[1] Create a biblio with various items on a few branches. Fill call number,
    location and ccode too. Set home branch and holdingbranch differently.
[2] Toggle the preferences, and verify display within OAPC search results.

Example with prefs (home, callnumber, 2, 2):
Availability: Items available for loan: Centerville (2)Call number: perl A 4, PERL D 1. Items available for reference: Fairfield: Not For Loan (1)Call number: PERL A 5. Not available: Centerville: Checked out (1)Call number: PERL A 3. Centerville: Ordered (1)Call number: PERL B 1. Centerville: Staff Collection (2)Call number: PERL A 2, PERL E 1. Centerville: Withdrawn (1)Call number: PERL B 2. Fairfield: Withdrawn (1)Call number: PERL C 1.

Same data with prefs (holding, callnumber, 2, 2):
Availability: Items available for loan: Centerville (1)Call number: PERL D 1. Liberty (1)Call number: perl A 4. Items available for reference: Centerville: Not For Loan (1)Call number: PERL A 5. Not available: Centerville: Checked out (1)Call number: PERL A 3. Centerville: Staff Collection (1)Call number: PERL A 2. Centerville: Withdrawn (1)Call number: PERL B 2. Fairfield: Ordered (1)Call number: PERL B 1. Fairfield: Withdrawn (1)Call number: PERL C 1. Liberty: Staff Collection (1)Call number: PERL E 1.

Same data with prefs (holding, library, n/a, n/a):
Availability: Items available for loan: Centerville (1). Liberty (1). Items available for reference: Centerville: Not For Loan (1). Not available: Centerville: Checked out (1). Centerville: Staff Collection (1). Centerville: Withdrawn (1). Fairfield: Ordered (1). Fairfield: Withdrawn (1). Liberty: Staff Collection (1).

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>
Comment 46 Marcel de Rooy 2021-08-06 05:48:30 UTC
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
> 
> Signed-off-by: David Nind <david@davidnind.com>

Great. Thx Owen and David !
Comment 47 Nick Clemens 2021-08-06 11:31:06 UTC
Overall this looks good, a few things:
1 - We should default the prefs to 1,0 to more closely mimic the behavior before
2 - Items with blank callnumbers now show: [Call number: ], before they only showed the count
3 - Not blocker: it would be nice to have a '-1' option for resultsMaxItemsUnavailable to preserve previous grouping - in large consortia I know they have not wanted to display individual NFL items

Before:
Availability: Items available for loan: Fairview (2) , Midway (1) , Union (1) Call number: J Qam. Items available for reference: Riverside Not For Loan (2) Call number: J Qam, .., Pleasant Valley Staff Collection (1) Call number: YA 615 Qam, Fairfield Not For Loan (1) Call number: FIC Qam. Not available: Ordered (3). Checked out (1). 

After:
Availability: Items available for loan: Fairview (2)Call number: , . Midway (1)Call number: . Union (1)Call number: J Qam. Items available for reference: Fairfield: Not For Loan (1)Call number: FIC Qam. Pleasant Valley: Staff Collection (1)Call number: YA 615 Qam. Riverside: Not For Loan (2)Call number: J Qam, YA 667 Qam. Not available: Fairview: Checked out (1)Call number: . Franklin: Ordered (1)Call number: YA Qam. Midway: Ordered (1)Call number: J 301 Qam. Springfield: Ordered (1)Call number: FIC 354 Qam.
Comment 48 Marcel de Rooy 2021-08-09 12:02:29 UTC
Created attachment 123640 [details] [review]
Bug 26302: (QA follow-up) Reset preference values

Mimic current behavior.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Marcel de Rooy 2021-08-09 12:02:33 UTC
Created attachment 123641 [details] [review]
Bug 26302: (QA follow-up) Prevent empty call numbers

This does not make the code easier, but here we go:

Suppose resultsMaxItems == 2.
If we would normally list two call numbers like:
    Liberty (2) [Call number: PERL F 1, PERL F 2].
If one call number would be empty, we now list:
    Liberty (2) [Call number: PERL F 1, ...].
And when both are empty, we only show a number:
    Liberty (2).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 50 Marcel de Rooy 2021-08-09 12:16:09 UTC
(In reply to Nick Clemens from comment #47)

Thx

> 1 - We should default the prefs to 1,0 to more closely mimic the behavior
> before

Fixed in patch/follow-up 5

> 2 - Items with blank callnumbers now show: [Call number: ], before they only
> showed the count

Fixed in patch/follow-up 6

> 3 - Not blocker: it would be nice to have a '-1' option for
> resultsMaxItemsUnavailable to preserve previous grouping - in large
> consortia I know they have not wanted to display individual NFL items

Yeah can imagine. Willing to add it on a new report. Not here anymore.
Bug 28831
Comment 51 Marcel de Rooy 2021-08-09 12:49:05 UTC
Created attachment 123643 [details] [review]
Bug 26302: (QA follow-up) Prevent empty call numbers

This does not make the code easier, but here we go:

Suppose resultsMaxItems == 2.
If we would normally list two call numbers like:
    Liberty (2) [Call number: PERL F 1, PERL F 2].
If one call number would be empty, we now list:
    Liberty (2) [Call number: PERL F 1, ...].
And when both are empty, we only show a number:
    Liberty (2).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 52 Nick Clemens 2021-08-09 13:03:02 UTC
Created attachment 123644 [details] [review]
Bug 26302: Add dbrev for two new prefs

Adding two prefs:
    resultsMaxItems
    resultsMaxItemsUnavailable

Test plan:
Check Preferences/OPAC/Appearance.

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 53 Nick Clemens 2021-08-09 13:03:07 UTC
Created attachment 123645 [details] [review]
Bug 26302: Add resultbranch and other status in C4/XSLT

Groundwork for changes in the OPAC results xslt.

NOTE: Adds both new prefs too.

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 54 Nick Clemens 2021-08-09 13:03:10 UTC
Created attachment 123646 [details] [review]
Bug 26302: Changes for substatus, resultbranch in XSLT.t

Test plan:
Run t/db_dependent/XSLT.t

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 55 Nick Clemens 2021-08-09 13:03:13 UTC
Created attachment 123647 [details] [review]
Bug 26302: Refactor Availability block in OPAC results xslt

Decided finally to do a larger refactor here. Hopefully we improved
consistency and removed some redundancy. And the two new prefs provide
additional functionality.

Most significant changes:
[1] Clearer distinction of the Availability line in three parts:
    Available, Reference and Unavailable.
    For Unavailable we loop thru branches now too.
[2] Calling template to list item data (including location or ccode).
    Made the separate Location line obsolete.
[3] The tests around OPACItemLibrary are removed since we now look at
    resultbranch from XSLT.pm.
[4] Removed code replication for various 'other' statuses like Checked out.
[5] Obsoleted three xslt key indexes, singleBranchMode.
[6] Apply the two prefs to control number of listed items.

Test plan:
You may play with: OPACResultsLibrary (home/holding), OPACItemLocation
(library, callno, location, ccode), resultsMaxItems[Unavailable] (numbers),
Reference_NFL_Statuses (list of notforloan codes in reference part).

[1] Create a biblio with various items on a few branches. Fill call number,
    location and ccode too. Set home branch and holdingbranch differently.
[2] Toggle the preferences, and verify display within OAPC search results.

Example with prefs (home, callnumber, 2, 2):
Availability: Items available for loan: Centerville (2)Call number: perl A 4, PERL D 1. Items available for reference: Fairfield: Not For Loan (1)Call number: PERL A 5. Not available: Centerville: Checked out (1)Call number: PERL A 3. Centerville: Ordered (1)Call number: PERL B 1. Centerville: Staff Collection (2)Call number: PERL A 2, PERL E 1. Centerville: Withdrawn (1)Call number: PERL B 2. Fairfield: Withdrawn (1)Call number: PERL C 1.

Same data with prefs (holding, callnumber, 2, 2):
Availability: Items available for loan: Centerville (1)Call number: PERL D 1. Liberty (1)Call number: perl A 4. Items available for reference: Centerville: Not For Loan (1)Call number: PERL A 5. Not available: Centerville: Checked out (1)Call number: PERL A 3. Centerville: Staff Collection (1)Call number: PERL A 2. Centerville: Withdrawn (1)Call number: PERL B 2. Fairfield: Ordered (1)Call number: PERL B 1. Fairfield: Withdrawn (1)Call number: PERL C 1. Liberty: Staff Collection (1)Call number: PERL E 1.

Same data with prefs (holding, library, n/a, n/a):
Availability: Items available for loan: Centerville (1). Liberty (1). Items available for reference: Centerville: Not For Loan (1). Not available: Centerville: Checked out (1). Centerville: Staff Collection (1). Centerville: Withdrawn (1). Fairfield: Ordered (1). Fairfield: Withdrawn (1). Liberty: Staff Collection (1).

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

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 56 Nick Clemens 2021-08-09 13:03:19 UTC
Created attachment 123648 [details] [review]
Bug 26302: (QA follow-up) Reset preference values

Mimic current behavior.

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 57 Nick Clemens 2021-08-09 13:03:22 UTC
Created attachment 123649 [details] [review]
Bug 26302: (QA follow-up) Prevent empty call numbers

This does not make the code easier, but here we go:

Suppose resultsMaxItems == 2.
If we would normally list two call numbers like:
    Liberty (2) [Call number: PERL F 1, PERL F 2].
If one call number would be empty, we now list:
    Liberty (2) [Call number: PERL F 1, ...].
And when both are empty, we only show a number:
    Liberty (2).

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

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 58 Jonathan Druart 2021-08-30 13:19:15 UTC
Created attachment 124243 [details] [review]
Bug 26302: Make syspref's names start with uppercase
Comment 59 Jonathan Druart 2021-08-30 14:00:16 UTC
1. Do we really need status=>other to be passed?

2. Shouldn't the pref's names contain "OPAC"?

3. One test is failing

    #   Failed test 'All records have at least one item available'
    #   at t/db_dependent/Search.t line 500.
    #          got: 'false'
    #     expected: 'true'
Comment 60 Jonathan Druart 2021-08-30 14:06:08 UTC
(In reply to Jonathan Druart from comment #59)
> 3. One test is failing
> 
>     #   Failed test 'All records have at least one item available'
>     #   at t/db_dependent/Search.t line 500.
>     #          got: 'false'
>     #     expected: 'true'

I cannot replicate this after a reset_all..
Comment 61 Marcel de Rooy 2021-08-30 14:13:29 UTC
(In reply to Jonathan Druart from comment #59)
> 1. Do we really need status=>other to be passed?

Yes, this is part of the change. An attempt to not pass it, may probably result in more obfuscated xslt code.

> 2. Shouldn't the pref's names contain "OPAC"?

Good point. They strictly relate to OPAC now. Will add a follow-up for it.
Comment 62 Marcel de Rooy 2021-08-30 14:14:27 UTC
(In reply to Jonathan Druart from comment #60)
> (In reply to Jonathan Druart from comment #59)
> > 3. One test is failing
> > 
> >     #   Failed test 'All records have at least one item available'
> >     #   at t/db_dependent/Search.t line 500.
> >     #          got: 'false'
> >     #     expected: 'true'
> 
> I cannot replicate this after a reset_all..

Passes with me too.
Comment 63 Marcel de Rooy 2021-08-30 14:25:37 UTC
Created attachment 124244 [details] [review]
Bug 26302: Make syspref's names start with uppercase

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 64 Marcel de Rooy 2021-08-30 14:25:42 UTC
Created attachment 124245 [details] [review]
Bug 26302: (QA follow-up) Prefix prefs with OPAC

git grep -l "ResultsMaxItems" | xargs sed -i -e "/ResultsMaxItems/ s/ResultsMaxItems/OPACResultsMaxItems/g"

And line:
koha-tmpl/opac-tmpl/bootstrap/en/xslt/MARC21slim2OPACResults.xsl:    <xsl:variable name="OPACResultsMaxItems" select="number(marc:sysprefs/marc:syspref[@name='resultsMaxItems']+0)"/>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 65 Marcel de Rooy 2021-08-30 14:27:15 UTC
-    <xsl:variable name="resultsMaxItems" select="number(marc:sysprefs/marc:syspref[@name='resultsMaxItems']+0)"/>
-    <xsl:variable name="resultsMaxItemsUnavailable" select="number(marc:sysprefs/marc:syspref[@name='resultsMaxItemsUnavailable']+0)"/>
+    <xsl:variable name="ResultsMaxItems" select="number(marc:sysprefs/marc:syspref[@name='resultsMaxItems']+0)"/>
+    <xsl:variable name="ResultsMaxItemsUnavailable" select="number(marc:sysprefs/marc:syspref[@name='ResultsMaxItemsUnavailable']+0)"/>

Why did you miss the second occurrence on the first change and not on the second ?
Comment 66 Jonathan Druart 2021-08-30 15:03:57 UTC
(In reply to Marcel de Rooy from comment #65)
> -    <xsl:variable name="resultsMaxItems"
> select="number(marc:sysprefs/marc:syspref[@name='resultsMaxItems']+0)"/>
> -    <xsl:variable name="resultsMaxItemsUnavailable"
> select="number(marc:sysprefs/marc:
> syspref[@name='resultsMaxItemsUnavailable']+0)"/>
> +    <xsl:variable name="ResultsMaxItems"
> select="number(marc:sysprefs/marc:syspref[@name='resultsMaxItems']+0)"/>
> +    <xsl:variable name="ResultsMaxItemsUnavailable"
> select="number(marc:sysprefs/marc:
> syspref[@name='ResultsMaxItemsUnavailable']+0)"/>
> 
> Why did you miss the second occurrence on the first change and not on the
> second ?

Oops, forgot 'g' in my sed command.
Comment 67 Jonathan Druart 2021-08-30 15:14:13 UTC
Pushed to master for 21.11, thanks to everybody involved!