Bug 22049 - MarkIssueReturned should rely on returndate only
Summary: MarkIssueReturned should rely on returndate only
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Josef Moravec
URL:
Keywords:
Depends on: 22051
Blocks: 14591 23038
  Show dependency treegraph
 
Reported: 2018-12-27 16:20 UTC by Tomás Cohen Arazi
Modified: 2020-06-04 20:32 UTC (History)
5 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 22049: Make MarkIssueReturned rely on returndate only (9.92 KB, patch)
2018-12-28 17:47 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22049: Make MarkIssueReturned rely on returndate only (9.98 KB, patch)
2018-12-28 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22049: Add tests (4.13 KB, patch)
2018-12-28 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22049: Add tests (9.77 KB, patch)
2018-12-31 11:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22049: (follow-up) Remove leftover variable (1.67 KB, patch)
2018-12-31 11:33 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 22049: Make MarkIssueReturned rely on returndate only (10.03 KB, patch)
2019-01-03 16:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22049: Add tests (9.82 KB, patch)
2019-01-03 16:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22049: (follow-up) Remove leftover variable (1.72 KB, patch)
2019-01-03 16:35 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 22049: Make MarkIssueReturned rely on returndate only (10.08 KB, patch)
2019-01-04 19:33 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 22049: Add tests (9.88 KB, patch)
2019-01-04 19:33 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 22049: (follow-up) Remove leftover variable (1.78 KB, patch)
2019-01-04 19:33 UTC, Charles Farmer
Details | Diff | Splinter Review
Bug 22049: Make MarkIssueReturned rely on returndate only (10.14 KB, patch)
2019-01-08 18:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22049: Add tests (9.94 KB, patch)
2019-01-08 18:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22049: (follow-up) Remove leftover variable (1.83 KB, patch)
2019-01-08 18:47 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22049: (QA follow-up) update MarkIssueReturned call in process_koc.pl (1.44 KB, patch)
2019-01-08 18:48 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 22049: Do not pass Koha::Patron to AddIssue (1.87 KB, patch)
2019-02-21 22:10 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 (kidclamp) 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 (kidclamp) 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.