[Bug 22049] New: MarkIssueReturned should rely on returndate only
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Bug ID: 22049 Summary: MarkIssueReturned should rely on returndate only Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Architecture, internals, and plumbing Assignee: koha-bugs@lists.koha-community.org Reporter: tomascohen@gmail.com QA Contact: testopia@bugs.koha-community.org Target Milestone: --- There's no need to pass the responsability of calculating the dropbox mode return date inside of it. We should get rid of it, and calculate dates in a single place to avoid bugs and code duplication. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Blocks| |14591 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14591 [Bug 14591] book drop / drop box mode incorrectly decrements accrued overdue fines -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kyle@bywatersolutions.com, | |martin.renvoize@ptfs-europe | |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #1 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 83542 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83542&action=edit Bug 22049: Make MarkIssueReturned rely on returndate only This patch changes the params accepted by C4::Circulation::MarkIssueReturned by removing the $dropbox_branch param. This passed branchcode was only used to initialize the Koha::Calendar object, but the date arithmetic has already taken place in a couple places before we reach this point. This logic needs to be simplified (bug 14591), and this is the starting point. To test: - Apply this patch - Run: $ git grep MarkIssueReturned => SUCCESS: Check all the uses of the function either originally passed undef, or now pass the same date that would've been calculated anyway, in the returndate param. - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! - Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83542|0 |1 is obsolete| | --- Comment #2 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 83543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83543&action=edit Bug 22049: Make MarkIssueReturned rely on returndate only This patch changes the params accepted by C4::Circulation::MarkIssueReturned by removing the $dropbox_branch param. This passed branchcode was only used to initialize the Koha::Calendar object, but the date arithmetic has already taken place in a couple places before we reach this point. This logic needs to be simplified (bug 14591), and this is the starting point. To test: - Apply this patch - Run: $ git grep MarkIssueReturned => SUCCESS: Check all the uses of the function either originally passed undef, or now pass the same date that would've been calculated anyway, in the returndate param. - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! - Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 83544 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83544&action=edit Bug 22049: Add tests The current tests only check failure use cases. As we are changing the way we store data, some tests are needed. To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83544|0 |1 is obsolete| | --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 83548 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83548&action=edit Bug 22049: Add tests The current tests only check some failure use cases. This patch makes it cover more use cases To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #5 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 83549 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83549&action=edit Bug 22049: (follow-up) Remove leftover variable The $circControlBranch variable was originally set to be used to pick the right dropbox branch. It was only used in MarkIssueReturned, to get the right Koha::Calendar object. As this responsability was moved top to the AddReturn caller, and the fact that _GetCircControlBranch is actually used for fines rules, there's no use for it in this context. And it was left on the previous patch as a mistake. To test: - Make sure the variable is not actually used: $ git grep '$circControlBranch' => SUCCESS: removed variable is not actually used. - Sign off :-D -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |22051 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22051 [Bug 22051] Make Koha::Object->store translate 'Incorrect <type> value' exceptions -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83543|0 |1 is obsolete| | Attachment #83548|0 |1 is obsolete| | Attachment #83549|0 |1 is obsolete| | --- Comment #6 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 83625 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83625&action=edit Bug 22049: Make MarkIssueReturned rely on returndate only This patch changes the params accepted by C4::Circulation::MarkIssueReturned by removing the $dropbox_branch param. This passed branchcode was only used to initialize the Koha::Calendar object, but the date arithmetic has already taken place in a couple places before we reach this point. This logic needs to be simplified (bug 14591), and this is the starting point. To test: - Apply this patch - Run: $ git grep MarkIssueReturned => SUCCESS: Check all the uses of the function either originally passed undef, or now pass the same date that would've been calculated anyway, in the returndate param. - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #7 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 83626 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83626&action=edit Bug 22049: Add tests The current tests only check some failure use cases. This patch makes it cover more use cases To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #8 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 83627 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83627&action=edit Bug 22049: (follow-up) Remove leftover variable The $circControlBranch variable was originally set to be used to pick the right dropbox branch. It was only used in MarkIssueReturned, to get the right Koha::Calendar object. As this responsability was moved top to the AddReturn caller, and the fact that _GetCircControlBranch is actually used for fines rules, there's no use for it in this context. And it was left on the previous patch as a mistake. To test: - Make sure the variable is not actually used: $ git grep '$circControlBranch' => SUCCESS: removed variable is not actually used. - Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Charles Farmer <charles.farmer@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83625|0 |1 is obsolete| | --- Comment #9 from Charles Farmer <charles.farmer@inlibro.com> --- Created attachment 83654 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83654&action=edit Bug 22049: Make MarkIssueReturned rely on returndate only This patch changes the params accepted by C4::Circulation::MarkIssueReturned by removing the $dropbox_branch param. This passed branchcode was only used to initialize the Koha::Calendar object, but the date arithmetic has already taken place in a couple places before we reach this point. This logic needs to be simplified (bug 14591), and this is the starting point. To test: - Apply this patch - Run: $ git grep MarkIssueReturned => SUCCESS: Check all the uses of the function either originally passed undef, or now pass the same date that would've been calculated anyway, in the returndate param. - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Charles Farmer <charles.farmer@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83626|0 |1 is obsolete| | --- Comment #10 from Charles Farmer <charles.farmer@inlibro.com> --- Created attachment 83655 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83655&action=edit Bug 22049: Add tests The current tests only check some failure use cases. This patch makes it cover more use cases To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Charles Farmer <charles.farmer@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83627|0 |1 is obsolete| | --- Comment #11 from Charles Farmer <charles.farmer@inlibro.com> --- Created attachment 83656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83656&action=edit Bug 22049: (follow-up) Remove leftover variable The $circControlBranch variable was originally set to be used to pick the right dropbox branch. It was only used in MarkIssueReturned, to get the right Koha::Calendar object. As this responsability was moved top to the AddReturn caller, and the fact that _GetCircControlBranch is actually used for fines rules, there's no use for it in this context. And it was left on the previous patch as a mistake. To test: - Make sure the variable is not actually used: $ git grep '$circControlBranch' => SUCCESS: removed variable is not actually used. - Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |josef.moravec@gmail.com |y.org | CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #83654|0 |1 is obsolete| | Attachment #83655|0 |1 is obsolete| | Attachment #83656|0 |1 is obsolete| | --- Comment #12 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 83727 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83727&action=edit Bug 22049: Make MarkIssueReturned rely on returndate only This patch changes the params accepted by C4::Circulation::MarkIssueReturned by removing the $dropbox_branch param. This passed branchcode was only used to initialize the Koha::Calendar object, but the date arithmetic has already taken place in a couple places before we reach this point. This logic needs to be simplified (bug 14591), and this is the starting point. To test: - Apply this patch - Run: $ git grep MarkIssueReturned => SUCCESS: Check all the uses of the function either originally passed undef, or now pass the same date that would've been calculated anyway, in the returndate param. - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! - Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #13 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 83728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83728&action=edit Bug 22049: Add tests The current tests only check some failure use cases. This patch makes it cover more use cases To test: - Apply this patchset - Run: $ kshell k$ prove t/db_dependent/Circulation/MarkIssueReturned.t => SUCCESS: Tests pass! Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #14 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 83729 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83729&action=edit Bug 22049: (follow-up) Remove leftover variable The $circControlBranch variable was originally set to be used to pick the right dropbox branch. It was only used in MarkIssueReturned, to get the right Koha::Calendar object. As this responsability was moved top to the AddReturn caller, and the fact that _GetCircControlBranch is actually used for fines rules, there's no use for it in this context. And it was left on the previous patch as a mistake. To test: - Make sure the variable is not actually used: $ git grep '$circControlBranch' => SUCCESS: removed variable is not actually used. - Sign off :-D Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Charles Farmer <charles.farmer@inLibro.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #15 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 83730 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83730&action=edit Bug 22049: (QA follow-up) update MarkIssueReturned call in process_koc.pl Test plan: Try to upload koc file with some returns Success: the file should be correctly processed Signed-off-by: Josef Moravec <josef.moravec@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |nick@bywatersolutions.com --- Comment #16 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 19.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED --- Comment #17 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Enhancement, will not be backported to 18.11.x series. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- We should hide the SQL error: t/db_dependent/Circulation/SwitchOnSiteCheckouts.t .. 1/10 DBD::mysql::st execute failed: Duplicate entry '1208' for key 'itemnumber' [for Statement "INSERT INTO `issues` ( `auto_renew`, `borrowernumber`, `branchcode`, `date_due`, `issuedate`, `itemnumber`, `onsite_checkout`) VALUES ( ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0=1, 1='248', 2="dc1yJjgj6j", 3='2019-02-20 23:59:00', 4='2019-02-15 10:15:15', 5='1208', 6=0] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- We should hide the SQL error: t/db_dependent/Circulation/MarkIssueReturned.t ...... 1/3 DBD::mysql::st execute failed: Incorrect datetime value: 'bad_date' for column 'returndate' at row 1 [for Statement "UPDATE `issues` SET `returndate` = ? WHERE ( `issue_id` = ? )" with ParamValues: 0='bad_date', 1='281'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #20 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Comment on attachment 83728 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=83728 Bug 22049: Add tests Review of attachment 83728: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=22049&attachment=83728) ----------------------------------------------------------------- $patron switched to a Koha::Patron bug still sent as it to AddIssue => Wrong! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 85498 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=85498&action=edit Bug 22049: Do not pass Koha::Patron to AddIssue The tests are not supposed to pass with such obvious mistake (?) Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #22 from Nick Clemens <nick@bywatersolutions.com> --- (In reply to Jonathan Druart from comment #21)
Created attachment 85498 [details] [review] Bug 22049: Do not pass Koha::Patron to AddIssue
The tests are not supposed to pass with such obvious mistake (?)
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Follow up pushed to master -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Bug 22049 depends on bug 22051, which changed state. Bug 22051 Summary: Make Koha::Object->store translate 'Incorrect <type> value' exceptions https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22051 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Stable |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 --- Comment #23 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Enhancement, will not be backported to 18.11.x series. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22049 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |23038 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23038 [Bug 23038] Expected warnings displayed by tests -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org