Bug 16834 - Changing the "Set Inventory Date to" date doesn't do anything, date last seen always defaults to current date.
Summary: Changing the "Set Inventory Date to" date doesn't do anything, date last seen...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Blou
QA Contact: Testopia
URL:
Keywords:
Depends on: 14001
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-29 21:08 UTC by Amy Schapiro
Modified: 2018-06-04 20:10 UTC (History)
7 users (show)

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


Attachments
Bug 16834 inventory.pl's Set Inventory Date to is ignored (3.72 KB, patch)
2017-03-01 22:19 UTC, Blou
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Amy Schapiro 2016-06-29 21:08:04 UTC
Our partner reports about the Inventory Tool:

 I've tested it out by uploading a set of recently scanned barcodes, but have discovered that changing the "set inventory date to" date doesn't really do anything. No matter what I do, after using the inventory tool, the "date last seen" is always changed to the current date by default.
Comment 1 Mark Tompsett 2016-06-29 21:27:28 UTC
What version are they running?
Comment 2 Amy Schapiro 2016-06-29 21:35:28 UTC
3.18.09.000
Comment 3 Mark Tompsett 2016-06-29 22:07:04 UTC
Upgrading to 3.18.13 will fix the problem.
Comment 4 Mark Tompsett 2016-06-29 22:13:34 UTC
(In reply to M. Tompsett from comment #3)
> Upgrading to 3.18.13 will fix the problem.

Hmmm... or maybe not? Because bug 14001 seems to be in 3.18.9.
Comment 5 Mark Tompsett 2016-06-29 23:28:05 UTC
I'm a little groggy right now, but I think there is a setdata vs lastseendate competition in the code. I'll try to make sense later.
Comment 6 Jonathan Druart 2016-07-01 14:51:46 UTC
Does the issue exist on master?
Comment 7 Nick Clemens 2016-08-17 18:12:38 UTC
Yes it does
Comment 8 Jonathan Druart 2016-08-18 15:58:32 UTC
(In reply to Nick Clemens from comment #7)
> Yes it does

Confirmed!

So I found what is the problem, but no idea how to fix it:
inventory.pl calls ModItem and update datelastseen (l.210), but a call to ModDateLastSeen is done later (l.332). ModDateLastSeen does not accept any parameters and update datelastseen with the today's date.

My supposition is that bug 14001 introduced this bug by trying to fix something else.
Maybe we could just remove the call to ModDateLastSeen and handle $moddatecount somewhere else.
Comment 9 Blou 2017-03-01 22:19:28 UTC
Created attachment 60788 [details] [review]
Bug 16834 inventory.pl's Set Inventory Date to is ignored

When entering a value in "Set Inventory date to" at the top of the UI, this value is ignored and the items are always updated with today.
This is (in part) caused by ModDateLastSeen not allowing to set current day.

This patch:
- Makes relevant the date entered at the top (Set Inventory date to).
- Makes relevant the date entered at the bottow (Inventory date).
- Doesn't brake most functionalities.
- Doesn't fix any of the many other bugs and issues with this broken tool.
- remove a warning that was flooding the logs on every call.
- Just makes the UI intuitive for common usage regarding dates.

TEST:
1) find a barcode in your db, write it in a text file
 - set item.datelastseen to some date in the past
2) go to tools/inventory
3) Browse to select the file from 1)
4) Select a date (top) before today, and after the datelastseen entered.
5) Click submit.
6) Validate:
   - Datelastseen specified at the top is the one you entered.
   - Date Last Seen in the table is the old from the DB
   - item.datelastseen is now today
7) Apply the patch.  Validate:
   - item.datelastseen is now the date you entered.
Comment 10 Blou 2017-03-01 22:23:55 UTC
This is by no mean a great patch.  But our users encountered the problem, and this fixes it.  When debugging, I found MANY other issues, but I tried to stay focused on just what was needed for their usage.

A complete overhaul would be needed to make this tool intuitive.  That's not the goal here.  Just get the users the basics they need.

If more problem encountered (or side effects), we'll fix...
Comment 11 Marcel de Rooy 2017-03-28 12:20:43 UTC
(In reply to Blou from comment #9)
> This patch:
> - Doesn't brake most functionalities.
What does it break? :)

> - Doesn't fix any of the many other bugs and issues with this broken tool.
OK I am looking at several and will try if it is possible to start here..
Comment 12 Marcel de Rooy 2017-03-28 15:00:12 UTC
Blocking this report temporarily in favor of rescuing bug 12913, taking into account the comments of David on bug 14399.
The datelastseen field on the inventory form is actually useless.
Comment 13 Marcel de Rooy 2017-03-29 14:15:50 UTC
(In reply to Marcel de Rooy from comment #12)
> The datelastseen field on the inventory form is actually useless.

Not completely btw. Bug 12631 describes a scenario with two partial barcode files and using this inventory date.
Comment 14 Marcel de Rooy 2017-03-29 14:58:11 UTC
Small code review:

+my $datelastseen = $input->param('datelastseen') || $input->param('setdate');
We should not do that. It will disrupt some workflows.
Datelastseen is the inventory date.

-    my $dls = output_pref( { dt => dt_from_string( $datelastseen ),
-                             dateformat => 'iso' } );
+1 for removing those two lines :)
I am removing this whole block on bug 14399. It is weird code.

-    if (none { $item->{barcode} eq $_->{barcode} && !$_->{'onloan'} } @$wrongplacelist) {
+    if (none { $_->{barcode} && $item->{barcode} eq $_->{barcode} && !$_->{'onloan'} } @$wrongplacelist) {
Looks like rescuable lines to me now.

-    ModDateLastSeen($item->{'itemnumber'});
+    ModItem({ itemlost => 0, datelastseen => $dls }, undef, $item->{'itemnumber'});
I understand why you do it. But I guess we might need a checkbox for that ?
Comment 15 Marcel de Rooy 2017-06-02 05:36:07 UTC
Closed in favor of bug 14399