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

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

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

Patch does not apply:

Applying: Bug 17680: C4::Circulation - Remove GetItemIssue, simple calls
fatal: sha1 information is lacking or useless (C4/Circulation.pm).

(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.

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>

This patch set needs updated to use Koha::Checkouts

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

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

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>

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

could you rebase please?

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

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>

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

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

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>

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

Just a rebase.

First two patches apply cleanly, with 3rd patch (complex calls) I get:
fatal: sha1 information is lacking or useless (C4/Circulation.pm).

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

Typo in 3rd patch, circ/returns.pl , line 314:

my $date_due_dt = dt_strom_string( $issue->date_due, 'sql' );

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

Worked through test plans from comments #1 - #3, everything OK, will sign off.

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>

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>

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>

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>

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>

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>

looking here - SIP item_information request failed for me in testing (worked on master) - will try again tomorrow

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

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 (!)

(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?

(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.

Created attachment 64337 [details] [review]
Bug 17680: Add few tests for AddReturn when overdue

To make sure the last patch fixes the issue

Code looks ok, but I cannot apply the 'complex calls' patch.

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>

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 (!)

Created attachment 64807 [details] [review]
Bug 17680: Add few tests for AddReturn when overdue

To make sure the last patch fixes the issue

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$

Created attachment 64833 [details] [review]
Bug 17680: Fix conflict with bug 18651

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

(In reply to Tomás Cohen Arazi from comment #41)

Thanks for catching that, fixed!

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>

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>

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>

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 (!)

Created attachment 64870 [details] [review]
Bug 17680: Add few tests for AddReturn when overdue

To make sure the last patch fixes the issue

Created attachment 64871 [details] [review]
Bug 17680: Fix conflict with bug 18651

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

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>

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>

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>

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>

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>

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>

Created attachment 64931 [details] [review]
Bug 17680: Remove Koha::Upload use incorrectly added

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

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.

Pushed to master for 17.11, thanks to everybody involved!

Enhancement not pushed to 17.05.x

(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.

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>

(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.

Created attachment 65310 [details] [review]
Bug 17680 [QA Follow-up] dt_from_string failing silently

Added independent sign off to last patch

Would be great to have the SIP tests moved to t

Last patch pushed to master for 17.11. Thanks Marcel and Colin!

(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 ;)

(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.

Enhancement not pushed to 17.05.x