Bug 9995 - For reference items no longer listed in XSLT result lists
Summary: For reference items no longer listed in XSLT result lists
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: OPAC (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords: regression
Depends on: 5079
Blocks: 6886
  Show dependency treegraph
 
Reported: 2013-04-07 14:01 UTC by Katrin Fischer
Modified: 2014-05-26 21:04 UTC (History)
4 users (show)

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


Attachments
Bug 9995 - For reference items no longer listed in XSLT result lists (2.48 KB, patch)
2013-04-08 13:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9995 - For reference items no longer listed in XSLT result lists (2.63 KB, patch)
2013-04-15 05:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9995 - For reference items no longer listed in XSLT result lists (2.74 KB, patch)
2013-04-17 15:04 UTC, Marcel de Rooy
Details | Diff | Splinter Review
9995 Followup for Availability (7.19 KB, patch)
2013-04-17 15:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9995: Followup for Location line (4.84 KB, patch)
2013-04-17 15:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9995: Followup for Location line (4.82 KB, patch)
2013-04-18 07:48 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9995 - For reference items no longer listed in XSLT result lists (2.72 KB, patch)
2013-04-18 13:17 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 Katrin Fischer 2013-04-07 14:01:19 UTC
For reference items (itemtype = not for loan) no longer show up in OPAC result lists with MARC21 and XSLT.

This was caused by the change for bug 5079.
commit 43c875b6834cac7242bf27547c840338e628c8b8

http://git.koha-community.org/gitweb/?p=koha.git;a=blobdiff;f=koha-tmpl/opac-tmpl/prog/en/xslt/MARC21slim2OPACResults.xsl;h=cfcc2dd7735e8ef862817f3df7b2a1fda3b9f733;hp=cdc803e6ec78a2219098d5bcf65755e6125c2349;hb=43c875b6834cac7242bf27547c840338e628c8b8;hpb=9aba512ceec2b584f0df6df90fe5ef32b587c4a6
Comment 1 Kyle M Hall 2013-04-08 13:08:24 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2013-04-09 18:39:45 UTC
This patch lacks a test plan.
Comment 3 Marcel de Rooy 2013-04-10 12:44:17 UTC
(In reply to comment #2)
> This patch lacks a test plan.

I also see some code at the end of the xslt referring to item-status reference and doing things with OPACItemLocation pref.
But the not-for-loan item I added does not appear in the Location line of the opac results?
Your patch does show the reference item again in the Availability line. But it makes a bad pair for OPACItemLocation<>callnumber for some reason..

Leaving it at Failed QA
Comment 4 Katrin Fischer 2013-04-15 05:14:23 UTC
Hi Marcel,
did you add the not for loan status to the item or to the itemtype?
I am sure that itemtype status used to show up for reference, but not sure about item level not for loan. Adding the not for loan to the itemtype works nicely for me.
Comment 5 Katrin Fischer 2013-04-15 05:30:43 UTC
Hi Marcel, 

I wanted to see if I could fix the problem you describe, but I am not able to reproduce it. :(

I have tested with the 2 possible ways to set an item not for loan:
- marking the item type not for loan
- marking the item not for loan

For both the counts and display seem to be restored now to the former functionality. But I agree that there is something odd about the behaviour introduced by 5078.

Problems I encountered were:
1) I get a lot of results with Location(s): without any information.
2) In one case I get only the callnumber, but no location or collection.

I think the problem is, that I have not consistently cataloged locations and collections. Some items have one, some both, some none of those. And that throws the display off.

But: I see that with and without this patch being applied, so it seems like a partial fix to me that doesn't make things worse.
Comment 6 Katrin Fischer 2013-04-15 05:31:46 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2013-04-15 12:48:34 UTC
Hm, guess I should have added a test plan.
Comment 8 Marcel de Rooy 2013-04-15 13:37:05 UTC
I was looking again a few moments :) But I still have my doubts over this patch.
Comment 9 Katrin Fischer 2013-04-15 13:45:37 UTC
I agree there is something wrong with the feature. I was only questioning if this made it worse or a little bit better. This is quite a big bug for at least one of our libraries.
Comment 10 Marcel de Rooy 2013-04-17 14:35:15 UTC
Working on a followup for this..
Comment 11 Marcel de Rooy 2013-04-17 15:04:55 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2013-04-17 15:05:00 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2013-04-17 15:05:05 UTC Comment hidden (obsolete)
Comment 14 Marcel de Rooy 2013-04-17 15:25:56 UTC
Have tried to simplify the code which obviously contained some bugs handling branch level and item level data. Also some when's should have been if's (if A and B are true, when A when B does not work.)

The Available line now contains just numbers per status.
The Location line respects OPACItemLocation and SingleBranchMode. And shows only available/reference items.

Following considerations remain: The difference between Available and Reference is lost on Location line. Does that really matter? 
We do not provide status numbers per branch. IMO better than providing erroneous or incomplete results.

Would appreciate an additional signoff from Katrin or Kyle. After that we can switch to Passed QA imo.
Comment 15 Marcel de Rooy 2013-04-17 15:31:22 UTC
Forgetting still another one:
If OPACItemLocation=field A but field A is blank. We now produce silly output. How should we resolve that?
Exclude this item or fall back to another field (B or C), or .. ?
Ideas are welcome..
Comment 16 Katrin Fischer 2013-04-17 15:53:45 UTC
Hi Marcel,
thx for your work on this! I will be travelling from tomorrow to Sunday, so won't have time for testing before next week.

Reading throuch your commit message I was wondering about 2 things:

>The Available line now contains just numbers per status.

I am a bit worried about this, does it mean we don't show the callnumber for available and reference items anymore? 

Also, how do you define branch level/item level information? I think understanding how this is supposed to work will help testing :)
Comment 17 Marcel de Rooy 2013-04-18 06:50:16 UTC
(In reply to comment #16)
Hi Katrin,

> I am a bit worried about this, does it mean we don't show the callnumber for
> available and reference items anymore? 

The call number goes to the Location line, but this depends on OPACItemLocation.

> Also, how do you define branch level/item level information? I think
> understanding how this is supposed to work will help testing :)

In the code was a test with generate-id to select one item per branch (branch level) but later on callnumber was inserted as if we were on item level, resulting in the listing of one callnumber per branch while there could be more..
Comment 18 Marcel de Rooy 2013-04-18 07:48:11 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2013-04-18 07:50:51 UTC
(In reply to comment #15)
> If OPACItemLocation=field A but field A is blank. We now produce silly
> output. How should we resolve that?
> Exclude this item or fall back to another field (B or C), or .. ?
> Ideas are welcome..

Just looked a little bit more. Restored behavior of preceding ccode or location before call number.
So, now my question becomes more hypothetical: Show what if location and callnumber are empty?

Amended the last followup.
Comment 20 Marcel de Rooy 2013-04-18 08:17:22 UTC
Added a related patch under bug 10073. Have fun :)
Comment 21 Mathieu Saby 2013-04-18 11:10:17 UTC
Do you know if this problem exists with UNIMARC ?

M. Saby
Comment 22 Marcel de Rooy 2013-04-18 11:45:02 UTC
(In reply to comment #21)
> Do you know if this problem exists with UNIMARC ?

Hm. A look in the UNIMARC counterpart shows me that the pref OPACItemLocation is not there. But I do see the construction with generate-id followed by an itemcallnumber.
So please check if you can replicate this: Have a biblio with multiple items. Check if available count is correct. If available>1, please check if all call numbers are listed. Looks to me that you will only see one.
Same for items with status reference (Not-for-loan).

If you confirm the bug, please open a new report for UNIMARC.
Thanks..
Comment 23 Marcel de Rooy 2013-04-18 11:47:51 UTC
Additionally: If you have both items on available and reference, you will have incomplete results due to the construction with When's..
Comment 24 Mathieu Saby 2013-04-18 11:50:42 UTC
BZ 5079 only changed XSLT file for MARC21...
So a followup of 5079 for unimarc is needed. And this followup must take into account BZ9995.

M. Saby
Comment 25 Marcel de Rooy 2013-04-18 13:03:33 UTC
From IRC:

cait>: marcelr: the referece bug is a bit monstrous now too  
<marcelr>: monstrous? 
<cait>: marcelr: I think it was by design to only display one callnumber for each  
<marcelr>: sounds odd to me 
<cait>: marcelr: because we for example have individual callnumbers for each item around here 
<cait>: think serials 
<cait>: with 120 items with unique callnumbers 
<marcelr>: fun 
<cait>: it's always been like that 
<cait>: the result list would be exploding
Comment 26 Marcel de Rooy 2013-04-18 13:11:24 UTC
As we concluded on IRC, the followups go somewhere else :)
Comment 27 Marcel de Rooy 2013-04-18 13:17:44 UTC
Created attachment 17523 [details] [review]
Bug 9995 - For reference items no longer listed in XSLT result lists

Re-added the xsl that shouldn't have been removed.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tested with different settings for OpacItemLocation
and not for loan set in the item and for the item type.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2013-04-18 13:25:33 UTC
QA Comment:
I am not spending much more time on this.
If bug 5079 is not reverted, this could improve things.

Passed QA
Comment 29 Mathieu Saby 2013-04-18 15:07:47 UTC
Ok, so I will fill an other bug for the unimarc followup when BZ9995 is pushed.

M. Saby
Comment 30 Jared Camins-Esakov 2013-04-21 13:55:29 UTC
This patch has been pushed to master and 3.12.x.