Bug 14399 - Fix inventory.pl part two (following 12913)
Summary: Fix inventory.pl part two (following 12913)
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
: 14398 (view as bug list)
Depends on: 12913
Blocks: 19324 19023
  Show dependency treegraph
 
Reported: 2015-06-17 06:03 UTC by David Cook
Modified: 2018-06-04 20:15 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14399: Fix the code for missing items in inventory.pl (3.08 KB, patch)
2017-03-29 14:44 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: Prevent display of double zero notforloan status (1.60 KB, patch)
2017-04-12 15:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: Numerous small refinements to the inventory script (20.18 KB, patch)
2017-04-12 15:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: Interface changes (9.52 KB, patch)
2017-04-12 15:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: Results form also needs a few interface changes (3.31 KB, patch)
2017-04-12 15:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: Interface changes (9.76 KB, patch)
2017-04-13 09:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: Results form also needs a few interface changes (3.31 KB, patch)
2017-04-13 09:22 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14399: Prevent display of double zero notforloan status (1.63 KB, patch)
2017-04-26 15:54 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14399: Numerous small refinements to the inventory script (20.21 KB, patch)
2017-04-26 15:54 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14399: Interface changes (9.79 KB, patch)
2017-04-26 15:55 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14399: Results form also needs a few interface changes (3.34 KB, patch)
2017-04-26 15:55 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 14399: Prevent display of double zero notforloan status (1.68 KB, patch)
2017-05-15 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14399: Numerous small refinements to the inventory script (20.26 KB, patch)
2017-05-15 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14399: Interface changes (9.84 KB, patch)
2017-05-15 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14399: Results form also needs a few interface changes (3.39 KB, patch)
2017-05-15 09:48 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 14399: [16.05.X] Prevent display of double zero notforloan status (1.73 KB, patch)
2017-10-03 13:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: [16.05.X] Numerous small refinements to the inventory script (20.44 KB, patch)
2017-10-03 13:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: [16.05.X] Interface changes (9.90 KB, patch)
2017-10-03 13:43 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 14399: [16.05.X] Results form also needs a few interface changes (3.33 KB, patch)
2017-10-03 13:43 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 David Cook 2015-06-17 06:03:45 UTC
At the moment, there are two different states (described as "problems") in the code of inventory.pl.

One is for "missingitem" and the other is for "not_scanned".

After much whingeing and discussion, I decided that these are basically the same state, although they're obtained in slightly different ways:

1) "not_scanned" is determined by comparing the scanned barcodes against the inventory list.
2) "missingitem" is determined by comparing the inventory's "datelastseen" against the inventory list's items' "datelastseen". 

The first one seems logical. It's "not_scanned" if it wasn't scanned.

However, the second one is strange. It's not even in relation to the actual task of taking inventory. This state also isn't determined "after" running the inventory, which might almost make sense. No, it's right after creating an inventory list. On top of this weirdness, the code also has an unintentional bug. Observe the following block of code:

# If "compare barcodes list to results" has been checked, we want to alert for missing items
if ( $compareinv2barcd ) {
    # set "missing" flags for all items with a datelastseen (dls) before the choosen datelastseen (cdls)
    my $dls = output_pref( { dt => dt_from_string( $datelastseen ),
                             dateformat => 'iso' } );
    foreach my $item ( @$inventorylist ) {
        my $cdls = output_pref( { dt => dt_from_string( $_->{datelastseen} ),
                                  dateformat => 'iso' } );
        if ( $cdls lt $dls ) {
            $item->{problem} = 'missingitem';
            # We have to push a copy of the item, not the reference
            push @items_with_problems, { %$item };
        }
    }
}

Firstly, contrary to the comment, "dls" is actually the inventory's "datelastseen", while "cdls" is the "datelastseen" of the item. Annoying? Yes, but not super important.

The important thing here is "my $cdls = output_pref( { dt => dt_from_string( $_->{datelastseen} ),".

It turns out that when you use "foreach my $item ( @$inventorylist )" instead of "foreach ( @$inventorylist )", "$_" will actually be null and not the element from the list!

So "dt_from_string" seems to create a DateTime object with today's date. So $cdls will always have a date of today. The only way to get "missingitem" to appear is if your "Inventory date:" (not to be mistaken with "Set inventory date to:" inventory date is in the future. Which... would be weird to do... but it's possible.

And because this only happens when you're comparing the uploaded barcodes against the inventory list... and because missingitem is done on the inventory list... everything is going to be marked missing in your inventory report!
Comment 1 David Cook 2015-06-17 06:46:43 UTC
Overall, I'm in favour of being able to mark inventory items as "missing" both in the "inventory report" and in the actual Koha system.

So... I'm not sure if it's better to remove this code and start over with that feature, or to refactor this code, or...
Comment 2 Marcel de Rooy 2017-03-28 14:45:26 UTC
(In reply to David Cook from comment #0)
> However, the second one is strange. It's not even in relation to the actual
> task of taking inventory. This state also isn't determined "after" running
> the inventory, which might almost make sense. No, it's right after creating
> an inventory list. On top of this weirdness, the code also has an
> unintentional bug. 

The bug with $_ is no longer in the code. But this code is still weird.
Actually, the "not scanned" items are the missing items! If these items also have a lower datelastseen, they are also marked as 'missing'. But note that if you scanned an item, datelastseen has been updated. This is useless.

I tried to imagine why this was introduced; perhaps it was meant as a compare with the last inventory date. But since all scanned items are already updated, this compare does not work.

I will remove this section on bug 12913.

> Firstly, contrary to the comment, "dls" is actually the inventory's
> "datelastseen", while "cdls" is the "datelastseen" of the item. Annoying?
> Yes, but not super important.

Yes, this comment was confusing and will be removed.
Comment 3 Marcel de Rooy 2017-03-28 14:54:34 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2017-03-29 11:59:39 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2017-03-29 12:50:54 UTC
*** Bug 14398 has been marked as a duplicate of this bug. ***
Comment 6 Marcel de Rooy 2017-03-29 14:44:58 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2017-03-29 14:46:25 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2017-04-03 11:37:22 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2017-04-12 15:00:52 UTC
Created attachment 62096 [details] [review]
Bug 14399: Prevent display of double zero notforloan status

On bug 12913 a zero status is added on the inventory form. This prevents
a lot of false warnings for a wrong notforloan status. The zero status
is not included in the default setup. But if you would add one, the
status will display here twice now.

This patch checks if the status already exists.

Test plan:
[1] Add a zero NOTFORLOAN status if it does not yet exist in Authorized
    values.
[2] Check that you do not see two "For loan" statuses on inventory form.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2017-04-12 15:00:57 UTC
Created attachment 62097 [details] [review]
Bug 14399: Numerous small refinements to the inventory script

This patch contains the following changes:

[01] Label "Inventory date" reworded to "Last inventory date", adding a
small explanation for its purpose.
[02] Restructured the results: it was an array with items and possible
error messages. Multiple messages duplicated individual items. Now the
results are in a hash, pulling all error messages for one item together.
At the end of the script they are copied to an array. (A helper sub
additemtoresults is added in this regard.) We no longer use array
@items_with_problems.
[03] Both datepickers are no longer connected to the same class. This
prevents changing the set date by filling the last inventory date.
[04] Input markseen in the template and $markseen in the script are
no longer needed.
[05] The paragraph before the detail link in the results table in the
Title column has been removed. Same for problems column. This makes
vertical spacing consistent.
[06] Problem status 'missingitem' is no longer used; the missing items
are marked as 'not_scanned'. Two additional statuses are: no_barcode and
checkedout.
[07] Removed unused $itemtype, $totalrecords and $count. We use variable
$moddatecount to report a count to the template.
[08] The script updated scanned items twice. The first time with ModItem
and the second time with ModDateLastSeen. The second call is removed.
[09] If a book is checked in, we do no longer return an error message when
the checkin is successful (ERR_ONLOAN_RET). The updated datelastseen is
passed to the results.
[10] $wrongplacelist is renamed to $rightplacelist. It is only built when
we need it. (Same for inventorylist now.)
[11] Datelastseen (last inventory date) is always used for building the
inventory list. It allows you to process partial barcode lists or make
a list of items not seen after some date. We do no longer use variable
$paramdatelastseen.
[12] The section where items.datelastseen was compared with the inventory
date has been removed. Scanned items were already updated; to get items
seen before some date, you can now use last inventory date without passing
barcodes.

The form can mainly be used for the following three cases:
[1] Prepare an inventory list or csv file; we do not upload barcodes.
[2] Update items for uploaded barcodes without comparing to inventory.
    Last inventory date is useless in this case.
    Errors wrongplace, checkedout and changestatus are reported.
    Use this scenario for partial scanned barcode lists (all but last).
[3] Update items for uploaded barcodes and compare to inventory, filtered
    by an optional last inventory date.
    Apart from the errors mentioned under [2], this also reports
    not_scanned ("missing") and no_barcode.
    Use this scenario too for the last partial barcode file (together with
    inventory date).

Test plan:
See next patch ("Interface changes").

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2017-04-12 15:01:01 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2017-04-12 15:01:06 UTC Comment hidden (obsolete)
Comment 13 Marcel de Rooy 2017-04-12 15:25:59 UTC
This is not a complete refactoring. I tried to resolve a lot of smaller issues within the current code. Rearranged the interface a bit too.
The test plan of the third patch is the crux. It may look impressive, but you should be able to finish it in say 10-15 minutes (or faster).
Comment 14 Marcel de Rooy 2017-04-13 09:14:58 UTC
Will adjust the test plan a little bit still
Comment 15 Marcel de Rooy 2017-04-13 09:18:15 UTC
(In reply to Marcel de Rooy from comment #14)
> Will adjust the test plan a little bit still
If you do not set datelastseen back to yesterday or earlier, you will not have B3 in the results of the last step because it was created today.
Comment 16 Marcel de Rooy 2017-04-13 09:22:49 UTC
Created attachment 62139 [details] [review]
Bug 14399: Interface changes

A part of the confusion around the inventory script may arise from the
fact that the form offers several options that are only used under
certain conditions. This patch hopefully rearranges a few options more
logically and only offers options when appropriate.

The barcode fieldset now also contains Compare barcodes and Do not check in
checkboxes. These are meaningful when a barcode file is uploaded.

The fieldset Item location filters (new name) contains fields that are
always used. Same for tne only control left under Additional options,
Export to CSV.

The fieldset Optional filters depends on the status of the barcode file
and the Compare checkbox. It is now shown or hidden depending on what
you select: if you do not upload a file, it is shown; or if you upload
a file and check Compare, it is shown. Otherwise we hide it, since the
script will not look at these values. Under this fieldset last inventory
date and Skip items on loan are added, since their behavior is the same
as the various item statuses.

Test plan:
In this test plan we test both the script changes from the previous patch
and the interface changes here. We follow the three main scenario's as
mentioned in the previous patch.

[1] First we prepare a few test items.
    Pick two biblios A, B and create five items say A1,A2,B1,B2,B3.
    Pick a not-existing callnumber range you want to test and move these
    five items there. Add barcodes too (say A1..B3).
    Edit one item A1 to a not-existing notforloan status (doing this on
    the mysql command line is fastest).
    Like: update items set notforloan = '9' where barcode='A1';
    Now simulate that we did not add/edit these items today:
    update items set datelastseen='2017-01-01' where barcode in ('A1','A2','B1','B2','B3');
    Note: We need this when comparing with last inventory date in the last
    scenario.

Scenario 1 (no barcodes uploaded)
[2] Enter the callnumber range on inventory form.
    Verify that "Set inventory date", Compare barcodes and "Do not check
    in" are disabled on the form. Check that you see the Optional filters
    box.
    Submit the form. Verify that you see all five items.
    Do the same. Check Export to CSV. Check result file contents.

Scenario 2 (upload barcodes, do not compare)
[3] Create a barcode file with the barcodes of A1, A2 and B1. Add another
    existing barcode outside the test callnumber range.
    After uploading this file, verify that "Set inventory date", Compare and
    "Do not check in" are enabled. The Optional filters should be hidden.
    Leave "Set inventory date" to today. Enter the callnumber range again.
    Submit the form.
    What do we expect? Four items should have been updated (alert). We
    should see barcode A1 with problem Unknown status. We should see
    also the barcode from the other range (Found in wrong place).
    Repeat this step with the same file. But now export to CSV. Verify that
    you see two barcodes with problems again in the csv file.

Scenario 3 (upload barcodes, compare)
[4] Create another barcode file with barcodes of B2 and one existing barcode
    outside the test callnumber range.
    After uploading this file, check the Compare checkbox. Verify now that
    the Optional filters box is displayed again.
    Leave "Set inventory date" to today. Enter the callnumber range again.
    Also set "Last inventory date" to today (important!).
    Submit the form.
    What do we expect now? Two items should be updated (see alert).
    We should see barcode B3 with problem Missing. We should also see the
    barcode from the other range (wrong place).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2017-04-13 09:22:53 UTC
Created attachment 62140 [details] [review]
Bug 14399: Results form also needs a few interface changes

Currently, the value of compareinv2barcd is used to determine if the
Seen column, the Select/Clear all buttons and the Mark seen buttons are
displayed. But if we scanned barcodes, we already marked items as seen.
So we should only display these buttons when we did not upload barcodes.

Test plan:
[1] Upload a barcode file. Check that the result form does not show
    the buttons.
[2] Generate an inventory list, so do not upload a barcode file. Verify
    that you still see the buttons.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2017-04-20 07:08:06 UTC
Fridolin or David: Would you see any chance of testing this bug?
Comment 19 David Cook 2017-04-24 08:15:30 UTC
(In reply to Marcel de Rooy from comment #18)
> Fridolin or David: Would you see any chance of testing this bug?

I have my hands full at the moment but I'll keep it on my to do list.
Comment 20 Josef Moravec 2017-04-26 15:54:42 UTC
Created attachment 62733 [details] [review]
[SIGNED-OFF] Bug 14399: Prevent display of double zero notforloan status

On bug 12913 a zero status is added on the inventory form. This prevents
a lot of false warnings for a wrong notforloan status. The zero status
is not included in the default setup. But if you would add one, the
status will display here twice now.

This patch checks if the status already exists.

Test plan:
[1] Add a zero NOTFORLOAN status if it does not yet exist in Authorized
    values.
[2] Check that you do not see two "For loan" statuses on inventory form.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 21 Josef Moravec 2017-04-26 15:54:53 UTC
Created attachment 62734 [details] [review]
[SIGNED-OFF] Bug 14399: Numerous small refinements to the inventory script

This patch contains the following changes:

[01] Label "Inventory date" reworded to "Last inventory date", adding a
small explanation for its purpose.
[02] Restructured the results: it was an array with items and possible
error messages. Multiple messages duplicated individual items. Now the
results are in a hash, pulling all error messages for one item together.
At the end of the script they are copied to an array. (A helper sub
additemtoresults is added in this regard.) We no longer use array
@items_with_problems.
[03] Both datepickers are no longer connected to the same class. This
prevents changing the set date by filling the last inventory date.
[04] Input markseen in the template and $markseen in the script are
no longer needed.
[05] The paragraph before the detail link in the results table in the
Title column has been removed. Same for problems column. This makes
vertical spacing consistent.
[06] Problem status 'missingitem' is no longer used; the missing items
are marked as 'not_scanned'. Two additional statuses are: no_barcode and
checkedout.
[07] Removed unused $itemtype, $totalrecords and $count. We use variable
$moddatecount to report a count to the template.
[08] The script updated scanned items twice. The first time with ModItem
and the second time with ModDateLastSeen. The second call is removed.
[09] If a book is checked in, we do no longer return an error message when
the checkin is successful (ERR_ONLOAN_RET). The updated datelastseen is
passed to the results.
[10] $wrongplacelist is renamed to $rightplacelist. It is only built when
we need it. (Same for inventorylist now.)
[11] Datelastseen (last inventory date) is always used for building the
inventory list. It allows you to process partial barcode lists or make
a list of items not seen after some date. We do no longer use variable
$paramdatelastseen.
[12] The section where items.datelastseen was compared with the inventory
date has been removed. Scanned items were already updated; to get items
seen before some date, you can now use last inventory date without passing
barcodes.

The form can mainly be used for the following three cases:
[1] Prepare an inventory list or csv file; we do not upload barcodes.
[2] Update items for uploaded barcodes without comparing to inventory.
    Last inventory date is useless in this case.
    Errors wrongplace, checkedout and changestatus are reported.
    Use this scenario for partial scanned barcode lists (all but last).
[3] Update items for uploaded barcodes and compare to inventory, filtered
    by an optional last inventory date.
    Apart from the errors mentioned under [2], this also reports
    not_scanned ("missing") and no_barcode.
    Use this scenario too for the last partial barcode file (together with
    inventory date).

Test plan:
See next patch ("Interface changes").

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 22 Josef Moravec 2017-04-26 15:55:05 UTC
Created attachment 62735 [details] [review]
[SIGNED-OFF] Bug 14399: Interface changes

A part of the confusion around the inventory script may arise from the
fact that the form offers several options that are only used under
certain conditions. This patch hopefully rearranges a few options more
logically and only offers options when appropriate.

The barcode fieldset now also contains Compare barcodes and Do not check in
checkboxes. These are meaningful when a barcode file is uploaded.

The fieldset Item location filters (new name) contains fields that are
always used. Same for tne only control left under Additional options,
Export to CSV.

The fieldset Optional filters depends on the status of the barcode file
and the Compare checkbox. It is now shown or hidden depending on what
you select: if you do not upload a file, it is shown; or if you upload
a file and check Compare, it is shown. Otherwise we hide it, since the
script will not look at these values. Under this fieldset last inventory
date and Skip items on loan are added, since their behavior is the same
as the various item statuses.

Test plan:
In this test plan we test both the script changes from the previous patch
and the interface changes here. We follow the three main scenario's as
mentioned in the previous patch.

[1] First we prepare a few test items.
    Pick two biblios A, B and create five items say A1,A2,B1,B2,B3.
    Pick a not-existing callnumber range you want to test and move these
    five items there. Add barcodes too (say A1..B3).
    Edit one item A1 to a not-existing notforloan status (doing this on
    the mysql command line is fastest).
    Like: update items set notforloan = '9' where barcode='A1';
    Now simulate that we did not add/edit these items today:
    update items set datelastseen='2017-01-01' where barcode in ('A1','A2','B1','B2','B3');
    Note: We need this when comparing with last inventory date in the last
    scenario.

Scenario 1 (no barcodes uploaded)
[2] Enter the callnumber range on inventory form.
    Verify that "Set inventory date", Compare barcodes and "Do not check
    in" are disabled on the form. Check that you see the Optional filters
    box.
    Submit the form. Verify that you see all five items.
    Do the same. Check Export to CSV. Check result file contents.

Scenario 2 (upload barcodes, do not compare)
[3] Create a barcode file with the barcodes of A1, A2 and B1. Add another
    existing barcode outside the test callnumber range.
    After uploading this file, verify that "Set inventory date", Compare and
    "Do not check in" are enabled. The Optional filters should be hidden.
    Leave "Set inventory date" to today. Enter the callnumber range again.
    Submit the form.
    What do we expect? Four items should have been updated (alert). We
    should see barcode A1 with problem Unknown status. We should see
    also the barcode from the other range (Found in wrong place).
    Repeat this step with the same file. But now export to CSV. Verify that
    you see two barcodes with problems again in the csv file.

Scenario 3 (upload barcodes, compare)
[4] Create another barcode file with barcodes of B2 and one existing barcode
    outside the test callnumber range.
    After uploading this file, check the Compare checkbox. Verify now that
    the Optional filters box is displayed again.
    Leave "Set inventory date" to today. Enter the callnumber range again.
    Also set "Last inventory date" to today (important!).
    Submit the form.
    What do we expect now? Two items should be updated (see alert).
    We should see barcode B3 with problem Missing. We should also see the
    barcode from the other range (wrong place).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 23 Josef Moravec 2017-04-26 15:55:13 UTC
Created attachment 62736 [details] [review]
[SIGNED-OFF] Bug 14399: Results form also needs a few interface changes

Currently, the value of compareinv2barcd is used to determine if the
Seen column, the Select/Clear all buttons and the Mark seen buttons are
displayed. But if we scanned barcodes, we already marked items as seen.
So we should only display these buttons when we did not upload barcodes.

Test plan:
[1] Upload a barcode file. Check that the result form does not show
    the buttons.
[2] Generate an inventory list, so do not upload a barcode file. Verify
    that you still see the buttons.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 24 Marcel de Rooy 2017-05-01 11:10:22 UTC
(In reply to Josef Moravec from comment #23)
> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Thanks for testing, Josef.
In the meantime we are using these changes in production on 3.22 already.
Comment 25 Martin Renvoize 2017-05-15 09:48:23 UTC
Created attachment 63465 [details] [review]
Bug 14399: Prevent display of double zero notforloan status

On bug 12913 a zero status is added on the inventory form. This prevents
a lot of false warnings for a wrong notforloan status. The zero status
is not included in the default setup. But if you would add one, the
status will display here twice now.

This patch checks if the status already exists.

Test plan:
[1] Add a zero NOTFORLOAN status if it does not yet exist in Authorized
    values.
[2] Check that you do not see two "For loan" statuses on inventory form.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2017-05-15 09:48:27 UTC
Created attachment 63466 [details] [review]
Bug 14399: Numerous small refinements to the inventory script

This patch contains the following changes:

[01] Label "Inventory date" reworded to "Last inventory date", adding a
small explanation for its purpose.
[02] Restructured the results: it was an array with items and possible
error messages. Multiple messages duplicated individual items. Now the
results are in a hash, pulling all error messages for one item together.
At the end of the script they are copied to an array. (A helper sub
additemtoresults is added in this regard.) We no longer use array
@items_with_problems.
[03] Both datepickers are no longer connected to the same class. This
prevents changing the set date by filling the last inventory date.
[04] Input markseen in the template and $markseen in the script are
no longer needed.
[05] The paragraph before the detail link in the results table in the
Title column has been removed. Same for problems column. This makes
vertical spacing consistent.
[06] Problem status 'missingitem' is no longer used; the missing items
are marked as 'not_scanned'. Two additional statuses are: no_barcode and
checkedout.
[07] Removed unused $itemtype, $totalrecords and $count. We use variable
$moddatecount to report a count to the template.
[08] The script updated scanned items twice. The first time with ModItem
and the second time with ModDateLastSeen. The second call is removed.
[09] If a book is checked in, we do no longer return an error message when
the checkin is successful (ERR_ONLOAN_RET). The updated datelastseen is
passed to the results.
[10] $wrongplacelist is renamed to $rightplacelist. It is only built when
we need it. (Same for inventorylist now.)
[11] Datelastseen (last inventory date) is always used for building the
inventory list. It allows you to process partial barcode lists or make
a list of items not seen after some date. We do no longer use variable
$paramdatelastseen.
[12] The section where items.datelastseen was compared with the inventory
date has been removed. Scanned items were already updated; to get items
seen before some date, you can now use last inventory date without passing
barcodes.

The form can mainly be used for the following three cases:
[1] Prepare an inventory list or csv file; we do not upload barcodes.
[2] Update items for uploaded barcodes without comparing to inventory.
    Last inventory date is useless in this case.
    Errors wrongplace, checkedout and changestatus are reported.
    Use this scenario for partial scanned barcode lists (all but last).
[3] Update items for uploaded barcodes and compare to inventory, filtered
    by an optional last inventory date.
    Apart from the errors mentioned under [2], this also reports
    not_scanned ("missing") and no_barcode.
    Use this scenario too for the last partial barcode file (together with
    inventory date).

Test plan:
See next patch ("Interface changes").

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2017-05-15 09:48:30 UTC
Created attachment 63467 [details] [review]
Bug 14399: Interface changes

A part of the confusion around the inventory script may arise from the
fact that the form offers several options that are only used under
certain conditions. This patch hopefully rearranges a few options more
logically and only offers options when appropriate.

The barcode fieldset now also contains Compare barcodes and Do not check in
checkboxes. These are meaningful when a barcode file is uploaded.

The fieldset Item location filters (new name) contains fields that are
always used. Same for tne only control left under Additional options,
Export to CSV.

The fieldset Optional filters depends on the status of the barcode file
and the Compare checkbox. It is now shown or hidden depending on what
you select: if you do not upload a file, it is shown; or if you upload
a file and check Compare, it is shown. Otherwise we hide it, since the
script will not look at these values. Under this fieldset last inventory
date and Skip items on loan are added, since their behavior is the same
as the various item statuses.

Test plan:
In this test plan we test both the script changes from the previous patch
and the interface changes here. We follow the three main scenario's as
mentioned in the previous patch.

[1] First we prepare a few test items.
    Pick two biblios A, B and create five items say A1,A2,B1,B2,B3.
    Pick a not-existing callnumber range you want to test and move these
    five items there. Add barcodes too (say A1..B3).
    Edit one item A1 to a not-existing notforloan status (doing this on
    the mysql command line is fastest).
    Like: update items set notforloan = '9' where barcode='A1';
    Now simulate that we did not add/edit these items today:
    update items set datelastseen='2017-01-01' where barcode in ('A1','A2','B1','B2','B3');
    Note: We need this when comparing with last inventory date in the last
    scenario.

Scenario 1 (no barcodes uploaded)
[2] Enter the callnumber range on inventory form.
    Verify that "Set inventory date", Compare barcodes and "Do not check
    in" are disabled on the form. Check that you see the Optional filters
    box.
    Submit the form. Verify that you see all five items.
    Do the same. Check Export to CSV. Check result file contents.

Scenario 2 (upload barcodes, do not compare)
[3] Create a barcode file with the barcodes of A1, A2 and B1. Add another
    existing barcode outside the test callnumber range.
    After uploading this file, verify that "Set inventory date", Compare and
    "Do not check in" are enabled. The Optional filters should be hidden.
    Leave "Set inventory date" to today. Enter the callnumber range again.
    Submit the form.
    What do we expect? Four items should have been updated (alert). We
    should see barcode A1 with problem Unknown status. We should see
    also the barcode from the other range (Found in wrong place).
    Repeat this step with the same file. But now export to CSV. Verify that
    you see two barcodes with problems again in the csv file.

Scenario 3 (upload barcodes, compare)
[4] Create another barcode file with barcodes of B2 and one existing barcode
    outside the test callnumber range.
    After uploading this file, check the Compare checkbox. Verify now that
    the Optional filters box is displayed again.
    Leave "Set inventory date" to today. Enter the callnumber range again.
    Also set "Last inventory date" to today (important!).
    Submit the form.
    What do we expect now? Two items should be updated (see alert).
    We should see barcode B3 with problem Missing. We should also see the
    barcode from the other range (wrong place).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2017-05-15 09:48:33 UTC
Created attachment 63468 [details] [review]
Bug 14399: Results form also needs a few interface changes

Currently, the value of compareinv2barcd is used to determine if the
Seen column, the Select/Clear all buttons and the Mark seen buttons are
displayed. But if we scanned barcodes, we already marked items as seen.
So we should only display these buttons when we did not upload barcodes.

Test plan:
[1] Upload a barcode file. Check that the result form does not show
    the buttons.
[2] Generate an inventory list, so do not upload a barcode file. Verify
    that you still see the buttons.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 29 Martin Renvoize 2017-05-15 09:50:31 UTC
Passing QA, code is solid, workflows are clear and all tests pass.

Cheers for all the hard work on this one!
Comment 30 Kyle M Hall 2017-05-17 13:58:00 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 31 Katrin Fischer 2017-05-21 20:48:51 UTC
This is a candidate for 16.11.09 as we are in string freeze for 16.11.08.
Comment 32 Katrin Fischer 2017-06-05 13:13:09 UTC
These patches have been pushed to 16.11.x and will be in 16.11.09.
Comment 33 Fridolin SOMERS 2017-06-13 10:49:00 UTC
Thanks all for you work, sorry I could not find the time to participate
Comment 34 Mason James 2017-09-20 01:39:34 UTC
Patchset has conflicts on 16.05.x branch

-----------------
mason@xen1:~/g/k/16.05.x$ git bz apply -s 14399
You seem to have moved HEAD since the last 'am' failure.
Not rewinding to ORIG_HEAD
No rebase in progress?
Bug 14399 - Fix inventory.pl part two (following 12913)

63465 - Bug 14399: Prevent display of double zero notforloan status
63466 - Bug 14399: Numerous small refinements to the inventory script
63467 - Bug 14399: Interface changes
63468 - Bug 14399: Results form also needs a few interface changes

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 14399: Prevent display of double zero notforloan status
Applying: Bug 14399: Numerous small refinements to the inventory script
Using index info to reconstruct a base tree...
M       koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt
M       tools/inventory.pl
Falling back to patching base and 3-way merge...
Auto-merging tools/inventory.pl
CONFLICT (content): Merge conflict in tools/inventory.pl
Auto-merging koha-tmpl/intranet-tmpl/prog/en/modules/tools/inventory.tt
Failed to merge in the changes.
Patch failed at 0001 Bug 14399: Numerous small refinements to the inventory script
The copy of the patch that failed is found in:
   /home/mason/g/k/16.05.x/.git/rebase-apply/patch
Comment 35 Mason James 2017-09-20 01:41:16 UTC
(In reply to Mason James from comment #34)
> Patchset has conflicts on 16.05.x branch

Please consider providing a patchset for 16.05.x branch :)
Comment 36 Marcel de Rooy 2017-10-03 13:43:41 UTC
Created attachment 67546 [details] [review]
Bug 14399: [16.05.X] Prevent display of double zero notforloan status

On bug 12913 a zero status is added on the inventory form. This prevents
a lot of false warnings for a wrong notforloan status. The zero status
is not included in the default setup. But if you would add one, the
status will display here twice now.

This patch checks if the status already exists.

Test plan:
[1] Add a zero NOTFORLOAN status if it does not yet exist in Authorized
    values.
[2] Check that you do not see two "For loan" statuses on inventory form.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Marcel de Rooy 2017-10-03 13:43:47 UTC
Created attachment 67547 [details] [review]
Bug 14399: [16.05.X] Numerous small refinements to the inventory script

This patch contains the following changes:

[01] Label "Inventory date" reworded to "Last inventory date", adding a
small explanation for its purpose.
[02] Restructured the results: it was an array with items and possible
error messages. Multiple messages duplicated individual items. Now the
results are in a hash, pulling all error messages for one item together.
At the end of the script they are copied to an array. (A helper sub
additemtoresults is added in this regard.) We no longer use array
@items_with_problems.
[03] Both datepickers are no longer connected to the same class. This
prevents changing the set date by filling the last inventory date.
[04] Input markseen in the template and $markseen in the script are
no longer needed.
[05] The paragraph before the detail link in the results table in the
Title column has been removed. Same for problems column. This makes
vertical spacing consistent.
[06] Problem status 'missingitem' is no longer used; the missing items
are marked as 'not_scanned'. Two additional statuses are: no_barcode and
checkedout.
[07] Removed unused $itemtype, $totalrecords and $count. We use variable
$moddatecount to report a count to the template.
[08] The script updated scanned items twice. The first time with ModItem
and the second time with ModDateLastSeen. The second call is removed.
[09] If a book is checked in, we do no longer return an error message when
the checkin is successful (ERR_ONLOAN_RET). The updated datelastseen is
passed to the results.
[10] $wrongplacelist is renamed to $rightplacelist. It is only built when
we need it. (Same for inventorylist now.)
[11] Datelastseen (last inventory date) is always used for building the
inventory list. It allows you to process partial barcode lists or make
a list of items not seen after some date. We do no longer use variable
$paramdatelastseen.
[12] The section where items.datelastseen was compared with the inventory
date has been removed. Scanned items were already updated; to get items
seen before some date, you can now use last inventory date without passing
barcodes.

The form can mainly be used for the following three cases:
[1] Prepare an inventory list or csv file; we do not upload barcodes.
[2] Update items for uploaded barcodes without comparing to inventory.
    Last inventory date is useless in this case.
    Errors wrongplace, checkedout and changestatus are reported.
    Use this scenario for partial scanned barcode lists (all but last).
[3] Update items for uploaded barcodes and compare to inventory, filtered
    by an optional last inventory date.
    Apart from the errors mentioned under [2], this also reports
    not_scanned ("missing") and no_barcode.
    Use this scenario too for the last partial barcode file (together with
    inventory date).

Test plan:
See next patch ("Interface changes").

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Marcel de Rooy 2017-10-03 13:43:51 UTC
Created attachment 67548 [details] [review]
Bug 14399: [16.05.X] Interface changes

A part of the confusion around the inventory script may arise from the
fact that the form offers several options that are only used under
certain conditions. This patch hopefully rearranges a few options more
logically and only offers options when appropriate.

The barcode fieldset now also contains Compare barcodes and Do not check in
checkboxes. These are meaningful when a barcode file is uploaded.

The fieldset Item location filters (new name) contains fields that are
always used. Same for tne only control left under Additional options,
Export to CSV.

The fieldset Optional filters depends on the status of the barcode file
and the Compare checkbox. It is now shown or hidden depending on what
you select: if you do not upload a file, it is shown; or if you upload
a file and check Compare, it is shown. Otherwise we hide it, since the
script will not look at these values. Under this fieldset last inventory
date and Skip items on loan are added, since their behavior is the same
as the various item statuses.

Test plan:
In this test plan we test both the script changes from the previous patch
and the interface changes here. We follow the three main scenario's as
mentioned in the previous patch.

[1] First we prepare a few test items.
    Pick two biblios A, B and create five items say A1,A2,B1,B2,B3.
    Pick a not-existing callnumber range you want to test and move these
    five items there. Add barcodes too (say A1..B3).
    Edit one item A1 to a not-existing notforloan status (doing this on
    the mysql command line is fastest).
    Like: update items set notforloan = '9' where barcode='A1';
    Now simulate that we did not add/edit these items today:
    update items set datelastseen='2017-01-01' where barcode in ('A1','A2','B1','B2','B3');
    Note: We need this when comparing with last inventory date in the last
    scenario.

Scenario 1 (no barcodes uploaded)
[2] Enter the callnumber range on inventory form.
    Verify that "Set inventory date", Compare barcodes and "Do not check
    in" are disabled on the form. Check that you see the Optional filters
    box.
    Submit the form. Verify that you see all five items.
    Do the same. Check Export to CSV. Check result file contents.

Scenario 2 (upload barcodes, do not compare)
[3] Create a barcode file with the barcodes of A1, A2 and B1. Add another
    existing barcode outside the test callnumber range.
    After uploading this file, verify that "Set inventory date", Compare and
    "Do not check in" are enabled. The Optional filters should be hidden.
    Leave "Set inventory date" to today. Enter the callnumber range again.
    Submit the form.
    What do we expect? Four items should have been updated (alert). We
    should see barcode A1 with problem Unknown status. We should see
    also the barcode from the other range (Found in wrong place).
    Repeat this step with the same file. But now export to CSV. Verify that
    you see two barcodes with problems again in the csv file.

Scenario 3 (upload barcodes, compare)
[4] Create another barcode file with barcodes of B2 and one existing barcode
    outside the test callnumber range.
    After uploading this file, check the Compare checkbox. Verify now that
    the Optional filters box is displayed again.
    Leave "Set inventory date" to today. Enter the callnumber range again.
    Also set "Last inventory date" to today (important!).
    Submit the form.
    What do we expect now? Two items should be updated (see alert).
    We should see barcode B3 with problem Missing. We should also see the
    barcode from the other range (wrong place).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Marcel de Rooy 2017-10-03 13:43:56 UTC
Created attachment 67549 [details] [review]
Bug 14399: [16.05.X] Results form also needs a few interface changes

Currently, the value of compareinv2barcd is used to determine if the
Seen column, the Select/Clear all buttons and the Mark seen buttons are
displayed. But if we scanned barcodes, we already marked items as seen.
So we should only display these buttons when we did not upload barcodes.

Test plan:
[1] Upload a barcode file. Check that the result form does not show
    the buttons.
[2] Generate an inventory list, so do not upload a barcode file. Verify
    that you still see the buttons.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Marcel de Rooy 2017-10-03 13:44:59 UTC
(In reply to Mason James from comment #35)
> (In reply to Mason James from comment #34)
> > Patchset has conflicts on 16.05.x branch
> 
> Please consider providing a patchset for 16.05.x branch :)

Rebased with care on 16.05.17, but note not tested !
Comment 41 Marcel de Rooy 2017-10-31 13:13:22 UTC
Mason: Any reason why you did not pick this one ?
Comment 42 Mason James 2018-03-14 03:27:45 UTC
(In reply to Marcel de Rooy from comment #41)
> Mason: Any reason why you did not pick this one ?

hi Marcel, very sorry to miss your 16.05.x patchset for this

i might consider making a future 16.05.x release with this patchset included