Bug 12913 - Fix wrong inventory results
Summary: Fix wrong inventory results
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Fridolin SOMERS
QA Contact: Marcel de Rooy
URL: /cgi-bin/koha/tools/inventory.pl
Whiteboard:
Keywords:
Depends on:
Blocks: 14399
  Show dependency treegraph
 
Reported: 2014-09-12 13:20 UTC by Fridolin SOMERS
Modified: 2017-05-03 03:55 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 12913 - wrong inventory results when notforloan filter is not used (3.37 KB, patch)
2014-09-12 13:45 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED OFF] Bug 12913 - wrong inventory results when notforloan filter is not used (3.44 KB, patch)
2014-09-13 00:43 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug still shows (59.53 KB, image/png)
2014-09-19 11:50 UTC, Kyle M Hall
Details
Bug 12913 - wrong inventory results when notforloan filter is not used (3.52 KB, patch)
2017-03-29 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12913: [QA Follow-up] Reword 'change item status' and add zero status (3.54 KB, patch)
2017-03-29 12:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 12913: [QA Follow-up] Reword 'change item status' and add zero status (3.71 KB, patch)
2017-03-29 14:32 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 Fridolin SOMERS 2014-09-12 13:20:51 UTC
In inventory tool, there is a test on scanned items notforloan value : if the value is not one of authorized values, the inventory results displays the problem "Change item status".
The bug is that if notforloan filter is not used, the notforloan values array is not filled and so all scanned items are marked with this problem.
Comment 1 Fridolin SOMERS 2014-09-12 13:45:22 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2014-09-13 00:43:57 UTC
Created attachment 31568 [details] [review]
[SIGNED OFF] Bug 12913 - wrong inventory results when notforloan filter is not used

In inventory tool, there is a test on scanned items notforloan value : if the value is not one of authorized values, the inventory results displays the problem "Change item status".
The bug is that if notforloan filter is not used, the notforloan values array is not filled and so all scanned items are marked with this problem.

Test plan :
- Check you have in default framework an item subfield mapped with items.notforloan and with an authorized value catergory, for example 'NOTFORLOAN'
- Check the authorized value category has at least value '0'
- Create a new value in this category, for example '9'
- Create a biblio with default framework
- Create an item with barcode='000AAA1', callnumber='ZZZAAA1' and notforloan=0
- Create an item with barcode='000AAA2', callnumber='ZZZAAA2' and notforloan=9
- Delete new value created above, for example '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 'ZZZ' and 'ZZZZ'
- Check 'Compare barcodes list to results'
- Submit
=> without patch, you get 2 results with problem 'Change item status' even for item with notforloan=0
=> with patch, you get 1 result with 'Change item status' on item with notforloan=9

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Comment 3 Kyle M Hall 2014-09-19 11:50:49 UTC
Created attachment 31733 [details]
Bug still shows

Followed the test plan, still got the same "Change item status" message for both items. Please re-test. Perhaps something has changed since the sign-off that has broken the patch. That, or I'm not testing it right!
Comment 4 David Cook 2015-06-17 05:38:16 UTC
I haven't tried the patch, but at a glance it might be trying to fix the wrong part of inventory.pl. 

This is the section of code that probably needs to be changed:

# If we have scanned items with a non-matching notforloan value
if (none { $item->{'notforloancode'} eq $_ } @notforloans) {
    $item->{problem} = 'changestatus';
    push @items_with_problems, { %$item };
}

If you're not using the "Not for loans" filter, @notforloans should be empty. Likewise, if your item isn't "Not for loan", $item->{'notforloancode'} should be false. Thus, this if statement should always be true if you're not using "Not for loans" and the item is not not for loan (ie is available).

So I think all we need here is to wrap this with a "if (@notforloans)" test.

I might take a look at this after I finish filing another Inventory bug report...
Comment 5 Marc Véron 2015-12-24 14:12:54 UTC
Is this bug still valid?
Comment 6 David Cook 2015-12-29 22:22:42 UTC
(In reply to Marc Véron from comment #5)
> Is this bug still valid?

I think so but I haven't tried to confirm it yet.
Comment 7 Marcel de Rooy 2017-03-28 14:54:34 UTC
*** Bug 14399 has been marked as a duplicate of this bug. ***
Comment 8 Marcel de Rooy 2017-03-28 14:55:41 UTC
Still a valid report. Will add a follow-up partially based on comments from David Cook on bug 14399.
Comment 9 Marcel de Rooy 2017-03-28 15:01:17 UTC
Still work in progress!
Comment 10 Marcel de Rooy 2017-03-29 12:39:35 UTC
Created attachment 61670 [details] [review]
Bug 12913 - wrong inventory results when notforloan filter is not used

In inventory tool, there is a test on scanned items notforloan value : if the value is not one of authorized values, the inventory results displays the problem "Change item status".
The bug is that if notforloan filter is not used, the notforloan values array is not filled and so all scanned items are marked with this problem.

Test plan :
- Check you have in default framework an item subfield mapped with items.notforloan and with an authorized value catergory, for example 'NOTFORLOAN'
- Check the authorized value category has at least value '0'
- Create a new value in this category, for example '9'
- Create a biblio with default framework
- Create an item with barcode='000AAA1', callnumber='ZZZAAA1' and notforloan=0
- Create an item with barcode='000AAA2', callnumber='ZZZAAA2' and notforloan=9
- Delete new value created above, for example '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 'ZZZ' and 'ZZZZ'
- Check 'Compare barcodes list to results'
- Submit
=> without patch, you get 2 results with problem 'Change item status' even for item with notforloan=0
=> with patch, you get 1 result with 'Change item status' on item with notforloan=9

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-03-29 12:39:40 UTC
Created attachment 61671 [details] [review]
Bug 12913: [QA Follow-up] Reword 'change item status' and add zero status

The zero status should be added in @notforloans. It is a legitimate status
for available books. The zero status description can be translated in
the template.
The 'Change item status' is confusing. We mean that we saw a notforloan
status that we do not expect (read: does not exist in the table).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2017-03-29 12:42:59 UTC
This looks good to me.
Note that we are not solving all problems in the script now.
But I cannot overload this report and its signed off status.
Will continue on report 14399 (reopened it).
Comment 13 Marcel de Rooy 2017-03-29 14:24:38 UTC
Still needs some attention
Comment 14 Marcel de Rooy 2017-03-29 14:32:09 UTC
Created attachment 61674 [details] [review]
Bug 12913: [QA Follow-up] Reword 'change item status' and add zero status

The zero status should be added in @notforloans. It is a legitimate status
for available books. The zero status description can be translated in
the template.
The 'Change item status' is confusing. We mean that we saw a notforloan
status that we do not expect (read: does not exist in the table).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Kyle M Hall 2017-03-31 13:13:44 UTC
Pushed to master for 17.05, thanks Frido, Marcel!
Comment 16 Katrin Fischer 2017-04-02 17:03:04 UTC
These patches have been pushed to 16.11.x and will be in 16.11.07.
.
Comment 17 Julian Maurice 2017-04-21 10:04:47 UTC
Pushed to 3.22.x for 3.22.20
Comment 18 Mason James 2017-05-03 03:55:52 UTC
Pushed to 16.05.x, for 16.05.12 release