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.
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.
Created attachment 105375 [details] Flawed search result
Created attachment 105376 [details] Correct data on item detail page
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.
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.
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>
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>
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>
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>
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>
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>
NOTE: Obviously we can promote the local pref to a real pref if this development meets sufficient interest.
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>
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>
Is the status "Needs Signoff" correct? All the patches have been signed off.
(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 :)
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
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>
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>
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>
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>
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>
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>
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>
Could you summarise what this patchset changes, perhaps in the 'text for release notes'. I'm struggling to wrap my head around it.
(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?
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>
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>
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>
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>
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>
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>
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>
Passing QA
(In reply to Martin Renvoize from comment #34) > Passing QA Thx
Pushed to master for 21.05, thanks to everybody involved!
This visual change may be too disturbing for stable versions right ? I'd say we do not backport.
(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
I overlooked this one, I didn't notice the use of the "local pref". Why didn't we create a "real" system preference here?
(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?
(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.
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>
Follow-up pushed to master.