Bug 25167

Summary: Fix not for loan filter in inventory tool
Product: Koha Reporter: Didier Gautheron <didier.gautheron>
Component: ToolsAssignee: Didier Gautheron <didier.gautheron>
Status: CLOSED FIXED QA Contact: Nick Clemens <nick>
Severity: normal    
Priority: P5 - low CC: aleisha, jonathan.druart, lucas, nick, severine.queune
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.06
Bug Depends on: 12913, 25608    
Bug Blocks:    
Attachments: Bug 25167: Fix to a fix wrong inventory results
Bug 25167: Fix to a fix wrong inventory results
Bug 25167: (QA follow-up) Don't add a new variable
Bug 25167: Fix to a fix wrong inventory results
Bug 25167: (QA follow-up) Don't add a new variable

Description Didier Gautheron 2020-04-16 07:43:39 UTC
Hi,

Bug https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12913 solves the case when notforloan filter is not used, but it breaks the case when a filter is used.

Regards
Didier
Comment 1 Katrin Fischer 2020-04-16 08:33:06 UTC
Hi didier, when adding a new bug and you know where the problem comes from or which bug it's related to, you can use the "depends on" and "see also" to add a link. This will also make sure people on the other bug get an email :)
Also the Importance preselects to enhancement - important to switch for bugs!
Comment 2 Katrin Fischer 2020-05-23 11:18:44 UTC
Can you detail how the bheaviour is broken?
Comment 3 Didier Gautheron 2020-05-27 04:01:48 UTC
Created attachment 105374 [details] [review]
Bug 25167: Fix to a fix wrong inventory results

Test plan :
- Check you have in default framework an item subfield mapped with items.notforloan and with an authorized value catergory,
  for example 'NOTFORLOAN'
- Create a new value in this category, for example '9'
- Create a biblio with default framework
- Create an item with barcode='000AAA1', callnumber='A-1' and notforloan=0
- Create an item with barcode='000AAA2', callnumber='A-2' and notforloan=9
- Create a file 'barecodes.txt' containing 2 lines '000AAA1' and '000AAA2'
- Go to inventory tool : /cgi-bin/koha/tools/inventory.pl
- Choose file in 'Barcode file'
- Enter item callnumber between 'A-0' and 'A-9'
- Check 'Compare barcodes list to results'
- Submit=> without patch, you get no warnings
- Redo the same inventory but with checking items.notforloan 'For loan'
  (using the browser back page is ok)
- submit => still no warning, Bug 000AAA2 must be listed it's not 'For loan'
- Apply patch, repeat, with tems.notforloan 'For loan' checked you get the
  right warning
Comment 4 Katrin Fischer 2020-05-28 22:45:40 UTC
I am looking at the code, but not understanding it well enough to see if this is the right fix. I wonder why we need no extra treatment for the other available checkbox options (withdrawn, lost and damaged). 

Leaving this for someone more familiar with inventory code in the QA team.
Comment 5 Didier Gautheron 2020-05-29 06:31:20 UTC
Hi,

Other available checkbox options (withdrawn, lost and damaged) weren't in the list in the original functionality. 

This patch only restore the previous behaviour, at least I hope so.

On the other hand not testing withdrawn, lost and so on doesn't seem to make sense.

Regards
Didier
Comment 6 Séverine Queune 2020-10-23 12:53:08 UTC
Created attachment 112262 [details] [review]
Bug 25167: Fix to a fix wrong inventory results

Test plan :
- Check you have in default framework an item subfield mapped with items.notforloan and with an authorized value catergory,
  for example 'NOTFORLOAN'
- Create a new value in this category, for example '9'
- Create a biblio with default framework
- Create an item with barcode='000AAA1', callnumber='A-1' and notforloan=0
- Create an item with barcode='000AAA2', callnumber='A-2' and notforloan=9
- Create a file 'barecodes.txt' containing 2 lines '000AAA1' and '000AAA2'
- Go to inventory tool : /cgi-bin/koha/tools/inventory.pl
- Choose file in 'Barcode file'
- Enter item callnumber between 'A-0' and 'A-9'
- Check 'Compare barcodes list to results'
- Submit=> without patch, you get no warnings
- Redo the same inventory but with checking items.notforloan 'For loan'
  (using the browser back page is ok)
- submit => still no warning, Bug 000AAA2 must be listed it's not 'For loan'
- Apply patch, repeat, with tems.notforloan 'For loan' checked you get the
  right warning

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 7 Séverine Queune 2020-10-23 12:54:50 UTC
Behavior seems correct, now we have a warning on scanned barcode with a notforloan different from the one checked (on master, we just have a warning on the not scanned items).
CSV export works fine.

For what I saw, warning are still broken for the other 3 status Katrin mentioned.
Comment 8 Nick Clemens 2020-10-24 01:53:08 UTC
Created attachment 112353 [details] [review]
Bug 25167: (QA follow-up) Don't add a new variable
Comment 9 Nick Clemens 2020-10-24 01:54:06 UTC
This fixes the issue mentioned, others should be on a new bug, there could be a lot of refactoring here to make this a bit more readable
Comment 10 Jonathan Druart 2020-10-25 22:07:44 UTC
Missing QA stamp.
Comment 11 Katrin Fischer 2020-11-01 14:01:25 UTC
Nick, can you fix your QA please?
Comment 12 Nick Clemens 2020-11-02 13:45:40 UTC
Created attachment 112833 [details] [review]
Bug 25167: Fix to a fix wrong inventory results

Test plan :
- Check you have in default framework an item subfield mapped with items.notforloan and with an authorized value catergory,
  for example 'NOTFORLOAN'
- Create a new value in this category, for example '9'
- Create a biblio with default framework
- Create an item with barcode='000AAA1', callnumber='A-1' and notforloan=0
- Create an item with barcode='000AAA2', callnumber='A-2' and notforloan=9
- Create a file 'barecodes.txt' containing 2 lines '000AAA1' and '000AAA2'
- Go to inventory tool : /cgi-bin/koha/tools/inventory.pl
- Choose file in 'Barcode file'
- Enter item callnumber between 'A-0' and 'A-9'
- Check 'Compare barcodes list to results'
- Submit=> without patch, you get no warnings
- Redo the same inventory but with checking items.notforloan 'For loan'
  (using the browser back page is ok)
- submit => still no warning, Bug 000AAA2 must be listed it's not 'For loan'
- Apply patch, repeat, with tems.notforloan 'For loan' checked you get the
  right warning

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 13 Nick Clemens 2020-11-02 13:45:42 UTC
Created attachment 112834 [details] [review]
Bug 25167: (QA follow-up) Don't add a new variable

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 14 Jonathan Druart 2020-11-03 09:58:45 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 15 Lucas Gass 2020-11-16 20:43:37 UTC
backported to 20.05.x for 20.05.06
Comment 16 Aleisha Amohia 2020-11-17 05:24:32 UTC
missing dependencies, not backported to 19.11.x