Bug 17996 - longoverdue.pl should not set 'datereturned' when --mark-returned option is used.
Summary: longoverdue.pl should not set 'datereturned' when --mark-returned option is u...
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 10 votes (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-01-25 19:45 UTC by Barton Chittenden
Modified: 2021-09-23 11:38 UTC (History)
8 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 17996 - longoverdue.pl should not set 'datereturned' when --mark-returned option is used (3.21 KB, patch)
2017-01-25 19:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 17996 - longoverdue.pl should not set 'datereturned' when --mark-returned option is used (4.23 KB, patch)
2020-03-06 14:21 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Barton Chittenden 2017-01-25 19:45:27 UTC
When longoverdue.pl returns items, it sets old_issues.returndate to the current date. This means that when staff look at the borrowers' circulation history, they see a returndate displayed, which is confusing.

We don't lose information by not setting old_issues.returndate, because items.itemlost_on will contain the date that the item was lost.
Comment 1 Kyle M Hall 2017-01-25 19:57:05 UTC
Created attachment 59560 [details] [review]
Bug 17996 - longoverdue.pl should not set 'datereturned' when --mark-returned option is used

When longoverdue.pl returns items, it sets old_issues.returndate to the
current date. This means that when staff look at the borrowers'
circulation history, they see a returndate displayed, which is
confusing.

We don't lose information by not setting old_issues.returndate, because
items.itemlost_on will contain the date that the item was lost.

Test Plan:
1) Apply this patch
2) Mark an item lost using longoverdue.pl with --mark-returned
3) Note the item's old_issues line does not have a returndate
Comment 2 Barton Chittenden 2017-01-31 19:57:53 UTC
In my initial testing, I've found that

    longoverdue.pl  --lost 1=2 --charge=2 --mark-returned --confirm

Does indeed check the item in and set old_issues.returndate to NULL, but the item still shows as 'Checked out' on the patron and item circ history.

See koha-tmpl/intranet-tmpl/prog/en/modules/members/readingrec.tt line 125

125             [% IF  issue.returndate %]
126               <span title="[% issue.returndate %]">[% issue.returndate |$KohaDates with_hours => 1 %]</span>
127             [% ELSE %]
128               <span title="Checked out"><small>Checked out</small></span>
129             [% END %]


koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/issuehistory.tt line 86

 86                 <td>[% IF ( issue.returndate ) %]
 87                         <span title="[% issue.returndate %]">[% issue.returndate |$KohaDates with_hours => 1  %]</span>
 88                     [% ELSE %]
 89                         <span title="Checked out"><small>Checked out</small></span>
 90                     [% END %]</td>

... unfortunately I don't think that it's as simple as adding a test for lost lost status, because it *is* possible for items to be checked out and lost. the underlying problem is that there's an implicit assumption that an item is checked out if either isssues.returndate or old_issues.returndate is NULL.
Comment 3 Katrin Fischer 2017-10-15 11:30:33 UTC
Changes in the patch are small and testing should be easy. Can you please rebase?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17996 - longoverdue.pl should not set 'datereturned' when --mark-returned option is used
Using index info to reconstruct a base tree...
M	C4/Circulation.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 17996 - longoverdue.pl should not set 'datereturned' when --mark-returned option is used
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-17996---longoverduepl-should-not-set-dateretur-IwbaAP.patch
Comment 4 Nick Clemens 2020-03-06 14:21:22 UTC
Created attachment 100272 [details] [review]
Bug 17996 - longoverdue.pl should not set 'datereturned' when --mark-returned option is used

When longoverdue.pl returns items, it sets old_issues.returndate to the
current date. This means that when staff look at the borrowers'
circulation history, they see a returndate displayed, which is
confusing.

We don't lose information by not setting old_issues.returndate, because
items.itemlost_on will contain the date that the item was lost.

Test Plan:
1) Apply this patch
2) Mark an item lost using longoverdue.pl with --mark-returned
3) Note the item's old_issues line does not have a returndate
Comment 5 Nick Clemens 2020-03-06 14:22:58 UTC
Rebased, but this needs test coverage - the pod is explicit that we set the date, so maybe we have moved on from here
Comment 6 Andrew Fuerste-Henry 2021-09-23 11:38:11 UTC
I'm not sure about this patch. If I manually mark an item lost while I have MarkLostItemsAsReturned on, that *also* sets the returndate. And I'm not sure that's wrong? After all, we've elected to mark it as returned.