|
Description
Tomás Cohen Arazi (tcohen)
2018-12-27 16:20:27 UTC
Created attachment 83542 [details] [review] 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 Created attachment 83543 [details] [review] 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 Created attachment 83544 [details] [review] 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! Created attachment 83548 [details] [review] 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! Created attachment 83549 [details] [review] 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 Created attachment 83625 [details] [review] 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> Created attachment 83626 [details] [review] 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> Created attachment 83627 [details] [review] 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> Created attachment 83654 [details] [review] 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> Created attachment 83655 [details] [review] 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> Created attachment 83656 [details] [review] 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> Created attachment 83727 [details] [review] 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> Created attachment 83728 [details] [review] 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> Created attachment 83729 [details] [review] 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> Created attachment 83730 [details] [review] 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> Awesome work all! Pushed to master for 19.05 Enhancement, will not be backported to 18.11.x series. 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. 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. Comment on attachment 83728 [details] [review] Bug 22049: Add tests Review of attachment 83728 [details] [review]: ----------------------------------------------------------------- $patron switched to a Koha::Patron bug still sent as it to AddIssue => Wrong! 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> (In reply to Jonathan Druart from comment #21) > Created attachment 85498 [details] [review] [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 Enhancement, will not be backported to 18.11.x series. |