Bug 17680

Summary: C4::Circulation - Replace GetItemIssue with Koha::Checkouts
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: aleisha, alexbuckley, colin.campbell, fridolin.somers, josef.moravec, kyle, m.de.rooy, nick, tomascohen, veron
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18802
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 17679, 17689    
Bug Blocks: 17677, 17829, 18996, 21020    
Attachments: Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: Fix regression - is_overdue must be called
Bug 17680: Add few tests for AddReturn when overdue
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: Fix regression - is_overdue must be called
Bug 17680: Add few tests for AddReturn when overdue
Bug 17680: Fix conflict with bug 18651
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: Fix regression - is_overdue must be called
Bug 17680: Add few tests for AddReturn when overdue
Bug 17680: Fix conflict with bug 18651
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
Bug 17680: C4::Circulation - Remove GetItemIssue
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls
Bug 17680: Fix regression - is_overdue must be called
Bug 17680: Add few tests for AddReturn when overdue
Bug 17680: Fix conflict with bug 18651
Bug 17680: Remove Koha::Upload use incorrectly added
Bug 17680: [QA Follow-up] dt_from_string failing silently
Bug 17680 [QA Follow-up] dt_from_string failing silently

Description Jonathan Druart 2016-11-25 11:59:23 UTC

    
Comment 1 Jonathan Druart 2016-11-28 10:37:00 UTC
Created attachment 57787 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls

C4::Circulation::GetItemIssue returned all the issue and item
informations for a given issue. Moveover it also did some date
manipulations. Most of the time this subroutine was called, there
additional information were useless as the caller usually just needed
the basic issue's infos 'from the issue table).

This first patch updates the simple calls, ie. the ones that just need
the issue's infomations.

Test plan:
The following operations should success:
- transfer a book
- create a rule for on-site checkouts and confirm that a patron cannot
check more items out that it's defined in the rule.
- Renew an issue using ILSDI
- Using SIP confirm that you are able to see your issues
Comment 2 Jonathan Druart 2016-11-28 10:37:03 UTC
Created attachment 57788 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date
Comment 3 Jonathan Druart 2016-11-28 10:37:07 UTC
Created attachment 57789 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase
Comment 4 Aleisha Amohia 2016-11-30 01:14:09 UTC
Patch does not apply:

Applying: Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Comment 5 Josef Moravec 2016-11-30 07:47:22 UTC
(In reply to Aleisha Amohia from comment #4)
> Patch does not apply:
> 
> Applying: Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
> fatal: sha1 information is lacking or useless (C4/Circulation.pm).

Did you apply the dependencies first? With dependencies, I was able apply these patches.
Comment 6 Alex Buckley 2016-11-30 20:08:17 UTC
Created attachment 57847 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 7 Kyle M Hall 2016-12-23 13:18:58 UTC
This patch set needs updated to use Koha::Checkouts
Comment 8 Jonathan Druart 2016-12-27 08:35:15 UTC
Created attachment 58431 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls

C4::Circulation::GetItemIssue returned all the issue and item
informations for a given issue. Moveover it also did some date
manipulations. Most of the time this subroutine was called, there
additional information were useless as the caller usually just needed
the basic issue's infos 'from the issue table).

This first patch updates the simple calls, ie. the ones that just need
the issue's infomations.

Test plan:
The following operations should success:
- transfer a book
- create a rule for on-site checkouts and confirm that a patron cannot
check more items out that it's defined in the rule.
- Renew an issue using ILSDI
- Using SIP confirm that you are able to see your issues
Comment 9 Jonathan Druart 2016-12-27 08:35:18 UTC
Created attachment 58432 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date
Comment 10 Jonathan Druart 2016-12-27 08:35:23 UTC
Created attachment 58433 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 11 Jonathan Druart 2016-12-29 16:05:54 UTC
Created attachment 58501 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date
Comment 12 Josef Moravec 2017-01-13 08:18:00 UTC
could you rebase please?
Comment 13 Jonathan Druart 2017-01-13 09:13:54 UTC
Created attachment 58918 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls

C4::Circulation::GetItemIssue returned all the issue and item
informations for a given issue. Moveover it also did some date
manipulations. Most of the time this subroutine was called, there
additional information were useless as the caller usually just needed
the basic issue's infos 'from the issue table).

This first patch updates the simple calls, ie. the ones that just need
the issue's infomations.

Test plan:
The following operations should success:
- transfer a book
- create a rule for on-site checkouts and confirm that a patron cannot
check more items out that it's defined in the rule.
- Renew an issue using ILSDI
- Using SIP confirm that you are able to see your issues
Comment 14 Jonathan Druart 2017-01-13 09:13:59 UTC
Created attachment 58919 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 15 Jonathan Druart 2017-01-13 09:14:04 UTC
Created attachment 58920 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date
Comment 16 Jonathan Druart 2017-03-31 19:15:11 UTC
Created attachment 61761 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls

C4::Circulation::GetItemIssue returned all the issue and item
informations for a given issue. Moveover it also did some date
manipulations. Most of the time this subroutine was called, there
additional information were useless as the caller usually just needed
the basic issue's infos 'from the issue table).

This first patch updates the simple calls, ie. the ones that just need
the issue's infomations.

Test plan:
The following operations should success:
- transfer a book
- create a rule for on-site checkouts and confirm that a patron cannot
check more items out that it's defined in the rule.
- Renew an issue using ILSDI
- Using SIP confirm that you are able to see your issues
Comment 17 Jonathan Druart 2017-03-31 19:15:17 UTC
Created attachment 61762 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Comment 18 Jonathan Druart 2017-03-31 19:15:24 UTC
Created attachment 61763 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date
Comment 19 Jonathan Druart 2017-03-31 19:15:39 UTC
Just a rebase.
Comment 20 Marc Véron 2017-04-10 21:37:47 UTC
First two patches apply cleanly, with 3rd patch (complex calls) I get:
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Comment 21 Jonathan Druart 2017-04-10 21:52:57 UTC
Created attachment 62034 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date
Comment 22 Marc Véron 2017-04-12 12:34:45 UTC
Typo in 3rd patch, circ/returns.pl , line 314:

my $date_due_dt = dt_strom_string( $issue->date_due, 'sql' );
Comment 23 Jonathan Druart 2017-04-12 13:26:52 UTC
Created attachment 62086 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date
Comment 24 Marc Véron 2017-04-12 15:49:58 UTC
Worked through test plans from comments #1 - #3, everything OK, will sign off.
Comment 25 Marc Véron 2017-04-12 15:53:30 UTC
Created attachment 62102 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls

C4::Circulation::GetItemIssue returned all the issue and item
informations for a given issue. Moveover it also did some date
manipulations. Most of the time this subroutine was called, there
additional information were useless as the caller usually just needed
the basic issue's infos 'from the issue table).

This first patch updates the simple calls, ie. the ones that just need
the issue's infomations.

Test plan:
The following operations should success:
- transfer a book
- create a rule for on-site checkouts and confirm that a patron cannot
check more items out that it's defined in the rule.
- Renew an issue using ILSDI
- Using SIP confirm that you are able to see your issues

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 26 Marc Véron 2017-04-12 15:54:37 UTC
Created attachment 62103 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 27 Marc Véron 2017-04-12 15:55:57 UTC
Created attachment 62104 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 28 Jonathan Druart 2017-06-13 14:19:54 UTC
Created attachment 64257 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls

C4::Circulation::GetItemIssue returned all the issue and item
informations for a given issue. Moveover it also did some date
manipulations. Most of the time this subroutine was called, there
additional information were useless as the caller usually just needed
the basic issue's infos 'from the issue table).

This first patch updates the simple calls, ie. the ones that just need
the issue's infomations.

Test plan:
The following operations should success:
- transfer a book
- create a rule for on-site checkouts and confirm that a patron cannot
check more items out that it's defined in the rule.
- Renew an issue using ILSDI
- Using SIP confirm that you are able to see your issues

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 29 Jonathan Druart 2017-06-13 14:19:59 UTC
Created attachment 64258 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 30 Jonathan Druart 2017-06-13 14:20:04 UTC
Created attachment 64259 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 31 Nick Clemens 2017-06-13 20:18:19 UTC
looking here - SIP item_information request failed for me in testing (worked on master) - will try again tomorrow
Comment 32 Nick Clemens 2017-06-14 10:38:51 UTC
SIP fails on the date, this needs to be converted to a string for returning

Checking in an overdue item fails to calculate the fine

Test fails: 
t/db_dependent/Circulation.t .. 44/95 
    #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
    #   at t/db_dependent/Circulation.t line 671.
    #          got: 'auto_too_much_oweing'
    #     expected: 'auto_renew'
    # Looks like you failed 1 test of 6.

I think there are spots where the date isn't being converted from a DateTime object when needed
Comment 33 Jonathan Druart 2017-06-14 13:49:34 UTC
Created attachment 64284 [details] [review]
Bug 17680: Fix regression - is_overdue must be called

is_overdue must be called even if there is not dropbox date (!)
Comment 34 Jonathan Druart 2017-06-14 13:51:20 UTC
(In reply to Nick Clemens from comment #32)
> SIP fails on the date, this needs to be converted to a string for returning

Any chances you could try to fix this one?

> Checking in an overdue item fails to calculate the fine

Ha, terrible! Will be fixed with the last patch, thanks for catching that.

> Test fails: 
> t/db_dependent/Circulation.t .. 44/95 
>     #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
>     #   at t/db_dependent/Circulation.t line 671.
>     #          got: 'auto_too_much_oweing'
>     #     expected: 'auto_renew'
>     # Looks like you failed 1 test of 6.

I manage to recreate that with prove t/db_dependent/Circulation*
but then the test failed on master too. Sounds like a master bug, depending on data.

> I think there are spots where the date isn't being converted from a DateTime
> object when needed

Did you spot where?
Comment 35 Jonathan Druart 2017-06-14 14:24:05 UTC
(In reply to Jonathan Druart from comment #34)
> > Test fails: 
> > t/db_dependent/Circulation.t .. 44/95 
> >     #   Failed test 'Can auto renew, OPACFineNoRenewals=10, patron has 10'
> >     #   at t/db_dependent/Circulation.t line 671.
> >     #          got: 'auto_too_much_oweing'
> >     #     expected: 'auto_renew'
> >     # Looks like you failed 1 test of 6.
> 
> I manage to recreate that with prove t/db_dependent/Circulation*
> but then the test failed on master too. Sounds like a master bug, depending
> on data.

It depends on the value of finesMode. I have opened bug 18802.
Comment 36 Jonathan Druart 2017-06-15 15:23:46 UTC
Created attachment 64337 [details] [review]
Bug 17680: Add few tests for AddReturn when overdue

To make sure the last patch fixes the issue
Comment 37 Tomás Cohen Arazi 2017-06-26 18:34:59 UTC
Code looks ok, but I cannot apply the 'complex calls' patch.
Comment 38 Jonathan Druart 2017-07-05 16:15:06 UTC
Created attachment 64805 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 39 Jonathan Druart 2017-07-05 16:15:11 UTC
Created attachment 64806 [details] [review]
Bug 17680: Fix regression - is_overdue must be called

is_overdue must be called even if there is not dropbox date (!)
Comment 40 Jonathan Druart 2017-07-05 16:15:17 UTC
Created attachment 64807 [details] [review]
Bug 17680: Add few tests for AddReturn when overdue

To make sure the last patch fixes the issue
Comment 41 Tomás Cohen Arazi 2017-07-05 19:32:35 UTC
wihtout the patches:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Circulation/Returns.t 
t/db_dependent/Circulation/Returns.t .. ok   
All tests successful.
Files=1, Tests=4,  2 wallclock secs ( 0.02 usr  0.01 sys +  2.09 cusr  0.35 csys =  2.47 CPU)
Result: PASS

with the patches:

kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Circulation/Returns.t 
t/db_dependent/Circulation/Returns.t .. 1/4 
    #   Failed test 'No account lines should exist on old issue_id'
    #   at t/db_dependent/Circulation/Returns.t line 311.
    #          got: '1'
    #     expected: '0'

    #   Failed test 'Two account lines should exist on new issue_id'
    #   at t/db_dependent/Circulation/Returns.t line 314.
    #          got: '1'
    #     expected: '2'
    # Looks like you failed 2 tests of 4.
t/db_dependent/Circulation/Returns.t .. 4/4 
#   Failed test 'Handle ids duplication'
#   at t/db_dependent/Circulation/Returns.t line 318.
# Looks like you failed 1 test of 4.
t/db_dependent/Circulation/Returns.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/4 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation/Returns.t (Wstat: 256 Tests: 4 Failed: 1)
  Failed test:  4
  Non-zero exit status: 1
Files=1, Tests=4,  3 wallclock secs ( 0.02 usr  0.01 sys +  2.14 cusr  0.33 csys =  2.50 CPU)
Result: FAIL
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$
Comment 42 Jonathan Druart 2017-07-05 22:09:38 UTC
Created attachment 64833 [details] [review]
Bug 17680: Fix conflict with bug 18651

$issue is now a Koha::Checkout, not a hashref
Comment 43 Jonathan Druart 2017-07-05 22:10:33 UTC
(In reply to Tomás Cohen Arazi from comment #41)

Thanks for catching that, fixed!
Comment 44 Jonathan Druart 2017-07-06 19:05:04 UTC
Created attachment 64866 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls

C4::Circulation::GetItemIssue returned all the issue and item
informations for a given issue. Moveover it also did some date
manipulations. Most of the time this subroutine was called, there
additional information were useless as the caller usually just needed
the basic issue's infos 'from the issue table).

This first patch updates the simple calls, ie. the ones that just need
the issue's infomations.

Test plan:
The following operations should success:
- transfer a book
- create a rule for on-site checkouts and confirm that a patron cannot
check more items out that it's defined in the rule.
- Renew an issue using ILSDI
- Using SIP confirm that you are able to see your issues

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 45 Jonathan Druart 2017-07-06 19:05:10 UTC
Created attachment 64867 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 46 Jonathan Druart 2017-07-06 19:05:16 UTC
Created attachment 64868 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 47 Jonathan Druart 2017-07-06 19:05:21 UTC
Created attachment 64869 [details] [review]
Bug 17680: Fix regression - is_overdue must be called

is_overdue must be called even if there is not dropbox date (!)
Comment 48 Jonathan Druart 2017-07-06 19:05:27 UTC
Created attachment 64870 [details] [review]
Bug 17680: Add few tests for AddReturn when overdue

To make sure the last patch fixes the issue
Comment 49 Jonathan Druart 2017-07-06 19:05:32 UTC
Created attachment 64871 [details] [review]
Bug 17680: Fix conflict with bug 18651

$issue is now a Koha::Checkout, not a hashref
Comment 50 Tomás Cohen Arazi 2017-07-07 19:46:07 UTC
Created attachment 64925 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls

C4::Circulation::GetItemIssue returned all the issue and item
informations for a given issue. Moveover it also did some date
manipulations. Most of the time this subroutine was called, there
additional information were useless as the caller usually just needed
the basic issue's infos 'from the issue table).

This first patch updates the simple calls, ie. the ones that just need
the issue's infomations.

Test plan:
The following operations should success:
- transfer a book
- create a rule for on-site checkouts and confirm that a patron cannot
check more items out that it's defined in the rule.
- Renew an issue using ILSDI
- Using SIP confirm that you are able to see your issues

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 51 Tomás Cohen Arazi 2017-07-07 19:46:13 UTC
Created attachment 64926 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue

This patch simply removes the C4::Circulation::GetItemIssue subroutine

Test plan:
At this point, `git grep GetItemIssue` should not return any occurrence
in the codebase

Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 52 Tomás Cohen Arazi 2017-07-07 19:46:21 UTC
Created attachment 64927 [details] [review]
Bug 17680: C4::Circulation - Remove GetItemIssue, complex calls

There are a few calls to GetItemIssue where it's not as easy to make
sure everything will be fine just replacing the calls with a
Koha::Issues->find
- In AddReturn the overdue flag is used (that's why this patch depends
on bug 17689)
- In CanBookBeRenewed, as well as the overdue flag the dates converted
to DateTime were used. It's now our job to convert them when we need
them.
- Same in AddRenewal but we also call _CalculateAndUpdateFine, so we
need to update the variables in this subroutine.

Note that, prior to this patch, AddReturn returned the GetItemIssue
hashref in the $iteminformation. Most of the time this variable is not
used, I have found only 1 place where it's used: circ/returns.pl
TODO: In this script we should call ->is_overdue instead of the
DateTime->compare calls

Test plan:
All the circulation tests must pass (it's how I have caught the specific
cases).
Do some checkins/checkouts/renewal and focus on the due date

Signed-off-by: Marc Véron <veron@veron.ch>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 53 Tomás Cohen Arazi 2017-07-07 19:46:33 UTC
Created attachment 64928 [details] [review]
Bug 17680: Fix regression - is_overdue must be called

is_overdue must be called even if there is not dropbox date (!)

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 54 Tomás Cohen Arazi 2017-07-07 19:46:40 UTC
Created attachment 64929 [details] [review]
Bug 17680: Add few tests for AddReturn when overdue

To make sure the last patch fixes the issue

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 55 Tomás Cohen Arazi 2017-07-07 19:46:49 UTC
Created attachment 64930 [details] [review]
Bug 17680: Fix conflict with bug 18651

$issue is now a Koha::Checkout, not a hashref

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 56 Tomás Cohen Arazi 2017-07-07 19:55:07 UTC
Created attachment 64931 [details] [review]
Bug 17680: Remove Koha::Upload use incorrectly added

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 57 Tomás Cohen Arazi 2017-07-07 19:57:03 UTC
I tested this patchset on the UI. It looks trivial in most of the cases. I haven't tested ILS-DI and SIP beyond the included unit tests, but the changes seems pretty straight-forward, and tests pass of course.

The QA script is happy too.
Comment 58 Jonathan Druart 2017-07-10 17:52:46 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 59 Fridolin Somers 2017-07-18 11:51:31 UTC
Enhancement not pushed to 17.05.x
Comment 60 Marcel de Rooy 2017-07-28 08:26:45 UTC
(In reply to Tomás Cohen Arazi from comment #57)
> I tested this patchset on the UI. It looks trivial in most of the cases. I
> haven't tested ILS-DI and SIP beyond the included unit tests, but the
> changes seems pretty straight-forward, and tests pass of course.

I think I found a problem in SIP/ILS/Item.
It fails silently on dt_from_string when checking in an item.
Comment 61 Marcel de Rooy 2017-07-28 08:43:20 UTC
Created attachment 65302 [details] [review]
Bug 17680: [QA Follow-up] dt_from_string failing silently

This is a trivial fix, but finding the cause for a failing checkin in
SIPServer surely was not.
Without this simple change, I could not checkin an item while the SIP server
just closed the connection and syslog contained nothing useful.

The error can be easily reproduced without this patch by:
    use Modern::Perl;
    use C4::SIP::ILS::Item;
    my $item = C4::SIP::ILS::Item->new('some_barcode');
where some_barcode should refer to an item checked out.
You should trigger the error (with your due date):
    2017-07-28 23:59:00 is not an object at line 3.

Why the ILS/Item.pm module still compiles without a warning, is not clear
though.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 62 Colin Campbell 2017-07-28 10:54:57 UTC
(In reply to Marcel de Rooy from comment #61)
> Created attachment 65302 [details] [review] [review]
> Bug 17680: [QA Follow-up] dt_from_string failing silently
> 
> This is a trivial fix, but finding the cause for a failing checkin in
> SIPServer surely was not.
> Without this simple change, I could not checkin an item while the SIP server
> just closed the connection and syslog contained nothing useful.
> 
> The error can be easily reproduced without this patch by:
>     use Modern::Perl;
>     use C4::SIP::ILS::Item;
>     my $item = C4::SIP::ILS::Item->new('some_barcode');
> where some_barcode should refer to an item checked out.
> You should trigger the error (with your due date):
>     2017-07-28 23:59:00 is not an object at line 3.
> 
> Why the ILS/Item.pm module still compiles without a warning, is not clear
> though.
> 
> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Nice find - these are painful to locate. Suspect that the reason there is no compile time warning is because ILS::Item is using autoloading so that the routine gets resolved at runtime.
Comment 63 Colin Campbell 2017-07-28 11:00:41 UTC
Created attachment 65310 [details] [review]
Bug 17680 [QA Follow-up] dt_from_string failing silently

Added independent sign off to last patch
Comment 64 Jonathan Druart 2017-07-28 14:17:42 UTC
Would be great to have the SIP tests moved to t
Comment 65 Jonathan Druart 2017-07-28 14:25:35 UTC
Last patch pushed to master for 17.11. Thanks Marcel and Colin!
Comment 66 Marcel de Rooy 2017-07-28 15:14:45 UTC
(In reply to Colin Campbell from comment #62)
> Suspect that the reason there is no
> compile time warning is because ILS::Item is using autoloading so that the
> routine gets resolved at runtime.

Yes, nice effects of autoloading of course. We could improve that some day ;)
Comment 67 Marcel de Rooy 2017-07-28 15:28:00 UTC
(In reply to Jonathan Druart from comment #64)
> Would be great to have the SIP tests moved to t

Will try to add at least a test for checkin on 18996. Bit by bit extending what we have in t/db_dependent/SIP. I am not that comfortable with C4/SIP/t and suspect that it does not create its own data too.
Comment 68 Fridolin Somers 2017-07-31 08:36:34 UTC
Enhancement not pushed to 17.05.x