Bug 20487 - AddReturn should clear items.onloan for unissued items
Summary: AddReturn should clear items.onloan for unissued items
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Marcel de Rooy
QA Contact: Josef Moravec
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-28 14:03 UTC by Marcel de Rooy
Modified: 2019-10-14 19:57 UTC (History)
8 users (show)

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


Attachments
Bug 20487: AddReturn should always clear items.onloan (2.49 KB, patch)
2018-03-28 14:45 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20487: AddReturn should clear items.onloan for unissued items (2.36 KB, patch)
2018-03-29 07:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20487: AddReturn should clear items.onloan for unissued items (2.38 KB, patch)
2018-04-23 13:17 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20487: AddReturn should clear items.onloan for unissued items (2.29 KB, patch)
2018-05-03 07:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20487: AddReturn should clear items.onloan for unissued items (2.31 KB, patch)
2018-05-11 22:30 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 20487: AddReturn should clear items.onloan for unissued items (2.45 KB, patch)
2018-05-31 07:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20487: (QA follow-up) Requested db revision (1.24 KB, patch)
2018-05-31 07:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20487: AddReturn should clear items.onloan for unissued items (2.40 KB, patch)
2018-07-02 11:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20487: (QA follow-up) Requested db revision (1.24 KB, patch)
2018-07-02 11:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20487: AddReturn should clear items.onloan for unissued items (2.41 KB, patch)
2018-08-21 16:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 20487: (QA follow-up) Requested db revision (1.26 KB, patch)
2018-08-21 16:26 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2018-03-28 14:03:27 UTC

    
Comment 1 Marcel de Rooy 2018-03-28 14:45:03 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2018-03-29 03:12:37 UTC
Could you explain how this is supposed to relate to the doreturn logic in the code? Something seems not quite right about the move.
Comment 3 Marcel de Rooy 2018-03-29 06:52:55 UTC
(In reply to M. Tompsett from comment #2)
> Could you explain how this is supposed to relate to the doreturn logic in
> the code? Something seems not quite right about the move.

There are three triggers for doreturn==0 before this point:
NotIssued
Wrongbranch => exited already
Withdrawn (depends on BlockReturnOfWithdrawnItems)

Actually, NotIssued is my main target. So you are right that this change might adversely affect the Withdrawn case.
Comment 4 Marcel de Rooy 2018-03-29 07:17:12 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2018-04-23 13:17:48 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2018-04-23 13:18:07 UTC
Trivial test count rebase
Comment 7 Marcel de Rooy 2018-05-03 07:00:03 UTC
Created attachment 74995 [details] [review]
Bug 20487: AddReturn should clear items.onloan for unissued items

If an item is no longer issued but somehow still has a date in the onloan
column, checking it in should clear that date.
Adding a ModItem call in the NotIssued section.

Test plan:
[1] Run t/db_dependent/Circulation.t
[2] Bonus: Checkout item, delete issue from table, checkin. Verify that
    items.onloan has been cleared.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2018-05-03 07:00:22 UTC
And another trivial rebase
Comment 9 Charles Farmer 2018-05-11 22:30:52 UTC
Created attachment 75290 [details] [review]
Bug 20487: AddReturn should clear items.onloan for unissued items

If an item is no longer issued but somehow still has a date in the onloan
column, checking it in should clear that date.
Adding a ModItem call in the NotIssued section.

Test plan:
[1] Run t/db_dependent/Circulation.t
[2] Bonus: Checkout item, delete issue from table, checkin. Verify that
    items.onloan has been cleared.

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

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 10 Marcel de Rooy 2018-05-14 06:31:24 UTC
(In reply to Charles Farmer from comment #9)
> Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Thanks.
Comment 11 Jonathan Druart 2018-05-28 18:57:09 UTC
What about local use?
Comment 12 Jonathan Druart 2018-05-28 18:59:40 UTC
Should not we provide an update DB entry?
Comment 13 Marcel de Rooy 2018-05-31 06:47:35 UTC
(In reply to Jonathan Druart from comment #11)
> What about local use?

Can't imagine a working use in this scenario (without making much more changes). Does not seem something we should be dealing with here.
If you have something in mind, please be more specific.
Comment 14 Marcel de Rooy 2018-05-31 06:48:14 UTC
(In reply to Jonathan Druart from comment #12)
> Should not we provide an update DB entry?

Yes, we could. Will add a few lines.
Comment 15 Marcel de Rooy 2018-05-31 07:05:01 UTC
Created attachment 75694 [details] [review]
Bug 20487: AddReturn should clear items.onloan for unissued items

If an item is no longer issued but somehow still has a date in the onloan
column, checking it in should clear that date.
Adding a ModItem call in the NotIssued section.

Test plan:
[1] Run t/db_dependent/Circulation.t
[2] Bonus: Checkout item, delete issue from table, checkin. Verify that
    items.onloan has been cleared.

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

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 16 Marcel de Rooy 2018-05-31 07:05:06 UTC
Created attachment 75695 [details] [review]
Bug 20487: (QA follow-up) Requested db revision

As requested in comment12, we can update these cases in a dbrev too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Checked out item, deleted issue record, ran dbrev.
Comment 17 Marcel de Rooy 2018-05-31 07:06:42 UTC
Rebased first patch too
Comment 18 Martin Renvoize 2018-06-06 11:13:44 UTC
Do we know how an item gets into this state in the first place?  It feels like this solution may be hiding a bug further up the tree rather than resolving it...?
Comment 19 Marcel de Rooy 2018-06-06 11:58:02 UTC
(In reply to Martin Renvoize from comment #18)
> Do we know how an item gets into this state in the first place?  It feels
> like this solution may be hiding a bug further up the tree rather than
> resolving it...?

I am not suspecting a bug here. Trigger might be related to database actions outside the interface ;) But what struck me is that checking in such an item did not clear the onload column. I think it just should; any user would expect it. I do not see an advantage in leaving it.
Comment 20 Marcel de Rooy 2018-07-02 11:53:11 UTC
Created attachment 76615 [details] [review]
Bug 20487: AddReturn should clear items.onloan for unissued items

If an item is no longer issued but somehow still has a date in the onloan
column, checking it in should clear that date.
Adding a ModItem call in the NotIssued section.

Test plan:
[1] Run t/db_dependent/Circulation.t
[2] Bonus: Checkout item, delete issue from table, checkin. Verify that
    items.onloan has been cleared.

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

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>
Comment 21 Marcel de Rooy 2018-07-02 11:53:15 UTC
Created attachment 76616 [details] [review]
Bug 20487: (QA follow-up) Requested db revision

As requested in comment12, we can update these cases in a dbrev too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Checked out item, deleted issue record, ran dbrev.
Comment 22 Marcel de Rooy 2018-07-02 11:53:51 UTC
Simple rebase on Circulation.t
Comment 23 Josef Moravec 2018-08-21 16:26:13 UTC
Created attachment 78050 [details] [review]
Bug 20487: AddReturn should clear items.onloan for unissued items

If an item is no longer issued but somehow still has a date in the onloan
column, checking it in should clear that date.
Adding a ModItem call in the NotIssued section.

Test plan:
[1] Run t/db_dependent/Circulation.t
[2] Bonus: Checkout item, delete issue from table, checkin. Verify that
    items.onloan has been cleared.

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

Signed-off-by: Charles Farmer <charles.farmer@inLibro.com>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 24 Josef Moravec 2018-08-21 16:26:17 UTC
Created attachment 78051 [details] [review]
Bug 20487: (QA follow-up) Requested db revision

As requested in comment12, we can update these cases in a dbrev too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Checked out item, deleted issue record, ran dbrev.

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

Thx
Comment 26 Nick Clemens 2018-08-22 15:00:25 UTC
Awesome work all!

Pushed to master for 18.11
Comment 27 Martin Renvoize 2018-08-24 11:45:56 UTC
Pushed to 18.05.x for 18.05.03
Comment 28 Fridolin Somers 2018-09-21 13:40:51 UTC
I prefer not to push to 17.11.x because UT changes does not apply easily and circulation is a hot module ;)
Comment 29 Mark Tompsett 2018-09-21 13:44:02 UTC Comment hidden (obsolete)