Description
Marcel de Rooy
2018-03-28 14:03:27 UTC
Created attachment 73390 [details] [review] Bug 20487: AddReturn should always clear items.onloan If an item is no longer issued but somehow still has a date in the onloan column, checking it in should clear that date. Realized by a simple change in AddReturn, moving the ModItem call outside the if-doreturn statement and testing if the change is needed. 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> Could you explain how this is supposed to relate to the doreturn logic in the code? Something seems not quite right about the move. (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. Created attachment 73409 [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> Created attachment 74723 [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> Trivial test count rebase 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> And another trivial rebase 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> (In reply to Charles Farmer from comment #9) > Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Thanks. What about local use? Should not we provide an update DB entry? (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. (In reply to Jonathan Druart from comment #12) > Should not we provide an update DB entry? Yes, we could. Will add a few lines. 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> 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. Rebased first patch too 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...? (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. 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> 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. Simple rebase on Circulation.t 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> 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> (In reply to Josef Moravec from comment #24) > Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Thx Awesome work all! Pushed to master for 18.11 Pushed to 18.05.x for 18.05.03 I prefer not to push to 17.11.x because UT changes does not apply easily and circulation is a hot module ;) (In reply to Fridolin SOMERS from comment #28) > I prefer not to push to 17.11.x because UT changes does not apply easily and > circulation is a hot module ;) What if I made it apply easy? |