|
Description
Katrin Fischer
2013-04-07 14:01:19 UTC
Created attachment 17288 [details] [review] Bug 9995 - For reference items no longer listed in XSLT result lists Re-added the xsl that shouldn't have been removed. This patch lacks a test plan. (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 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. 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. Created attachment 17434 [details] [review] [SIGNED OFF] 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. Hm, guess I should have added a test plan. I was looking again a few moments :) But I still have my doubts over this patch. 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. Working on a followup for this.. Created attachment 17508 [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> Including a QA followup. Created attachment 17509 [details] [review] 9995 Followup for Availability Replacing erroneous code trying to mix information from two levels (item level and branch level) by simpler numbers per status. The callnumber information should follow on the Location line. Fixing this in a second followup. Created attachment 17510 [details] [review] Bug 9995: Followup for Location line Simplifying erroneous code mixing information from two levels. Takes into account Singlebranchmode. All items, available or for reference, are shown on this line. 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. 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.. 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 :)
(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.. Created attachment 17519 [details] [review] Bug 9995: Followup for Location line Simplifying erroneous code mixing information from two levels. Takes into account Singlebranchmode. All items, available or for reference, are shown on this line. (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. Added a related patch under bug 10073. Have fun :) Do you know if this problem exists with UNIMARC ? M. Saby (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.. Additionally: If you have both items on available and reference, you will have incomplete results due to the construction with When's.. 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 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 As we concluded on IRC, the followups go somewhere else :) 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> QA Comment: I am not spending much more time on this. If bug 5079 is not reverted, this could improve things. Passed QA Ok, so I will fill an other bug for the unimarc followup when BZ9995 is pushed. M. Saby This patch has been pushed to master and 3.12.x. |