Bug 22049

Summary: MarkIssueReturned should rely on returndate only
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, josef.moravec, kyle, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 22051    
Bug Blocks: 14591, 23038    
Attachments: Bug 22049: Make MarkIssueReturned rely on returndate only
Bug 22049: Make MarkIssueReturned rely on returndate only
Bug 22049: Add tests
Bug 22049: Add tests
Bug 22049: (follow-up) Remove leftover variable
Bug 22049: Make MarkIssueReturned rely on returndate only
Bug 22049: Add tests
Bug 22049: (follow-up) Remove leftover variable
Bug 22049: Make MarkIssueReturned rely on returndate only
Bug 22049: Add tests
Bug 22049: (follow-up) Remove leftover variable
Bug 22049: Make MarkIssueReturned rely on returndate only
Bug 22049: Add tests
Bug 22049: (follow-up) Remove leftover variable
Bug 22049: (QA follow-up) update MarkIssueReturned call in process_koc.pl
Bug 22049: Do not pass Koha::Patron to AddIssue

Description Tomás Cohen Arazi 2018-12-27 16:20:27 UTC
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.
Comment 1 Tomás Cohen Arazi 2018-12-28 17:47:17 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2018-12-28 18:35:31 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2018-12-28 18:35:34 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2018-12-31 11:01:53 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2018-12-31 11:33:54 UTC Comment hidden (obsolete)
Comment 6 Owen Leonard 2019-01-03 16:35:45 UTC Comment hidden (obsolete)
Comment 7 Owen Leonard 2019-01-03 16:35:50 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2019-01-03 16:35:57 UTC Comment hidden (obsolete)
Comment 9 Charles Farmer 2019-01-04 19:33:14 UTC
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>
Comment 10 Charles Farmer 2019-01-04 19:33:17 UTC
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>
Comment 11 Charles Farmer 2019-01-04 19:33:22 UTC
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>
Comment 12 Josef Moravec 2019-01-08 18:47:50 UTC
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>
Comment 13 Josef Moravec 2019-01-08 18:47:54 UTC
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>
Comment 14 Josef Moravec 2019-01-08 18:47:58 UTC
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>
Comment 15 Josef Moravec 2019-01-08 18:48:03 UTC
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>
Comment 16 Nick Clemens 2019-01-25 20:50:00 UTC
Awesome work all!

Pushed to master for 19.05
Comment 17 Martin Renvoize 2019-02-01 11:52:43 UTC
Enhancement, will not be backported to 18.11.x series.
Comment 18 Jonathan Druart 2019-02-15 13:16:17 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2019-02-15 13:16:38 UTC
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 20 Jonathan Druart 2019-02-21 21:01:41 UTC
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!
Comment 21 Jonathan Druart 2019-02-21 22:10:29 UTC
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>
Comment 22 Nick Clemens 2019-02-22 15:06:53 UTC
(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
Comment 23 Martin Renvoize 2019-02-26 10:05:27 UTC
Enhancement, will not be backported to 18.11.x series.