Bug 22683 - Make issue and old_issue prefetchable from Koha::Account::Line
Summary: Make issue and old_issue prefetchable from Koha::Account::Line
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 22407
  Show dependency treegraph
 
Reported: 2019-04-11 08:31 UTC by Martin Renvoize
Modified: 2020-01-30 11:03 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 22683: Allow prefetch with Koha::Account::Line->checkouts (2.66 KB, patch)
2019-04-11 08:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22683: Allow prefetch with Koha::Account::Line->checkouts (2.66 KB, patch)
2019-04-16 12:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22683: (follow-up) Rename DBIC relationship accessor (1.40 KB, patch)
2019-04-16 12:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22683: (follow-up) Rename issue to checkout when call the relation (1.16 KB, patch)
2019-04-17 08:59 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-04-11 08:31:01 UTC
See omnibus bug for more details description.
Comment 1 Martin Renvoize 2019-04-11 08:33:38 UTC
Created attachment 87751 [details] [review]
Bug 22683: Allow prefetch with Koha::Account::Line->checkouts

This patch adds the required DBIC side relationships for the
Koha::Account::Line checkouts relationship accessor to support
prefetching and also updates the method to enable the capability.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 2 Tomás Cohen Arazi 2019-04-12 14:29:57 UTC
Comment on attachment 87751 [details] [review]
Bug 22683: Allow prefetch with Koha::Account::Line->checkouts

>+__PACKAGE__->belongs_to(
>+  "issue",
>+  "Koha::Schema::Result::Issue",
>+  { "foriegn.issue_id" => "self.issue_id" },

"foriegn"
Comment 3 Josef Moravec 2019-04-15 09:37:14 UTC
Comment on attachment 87751 [details] [review]
Bug 22683: Allow prefetch with Koha::Account::Line->checkouts

Review of attachment 87751 [details] [review]:
-----------------------------------------------------------------

::: Koha/Account/Line.pm
@@ +68,4 @@
>  
> +    my $old_issue_rs = $self->_result->old_issue;
> +    return Koha::Old::Checkout->_new_from_dbic( $old_issue_rs ) if $old_issue_rs;
> +    return undef;

should be only 'return;' I think...
Comment 4 Martin Renvoize 2019-04-16 11:50:13 UTC
(In reply to Tomás Cohen Arazi from comment #2)
> Comment on attachment 87751 [details] [review] [review]
> Bug 22683: Allow prefetch with Koha::Account::Line->checkouts
> 
> >+__PACKAGE__->belongs_to(
> >+  "issue",
> >+  "Koha::Schema::Result::Issue",
> >+  { "foriegn.issue_id" => "self.issue_id" },
> 
> "foriegn"

oops, good catch
Comment 5 Martin Renvoize 2019-04-16 11:51:06 UTC
(In reply to Josef Moravec from comment #3)
> Comment on attachment 87751 [details] [review] [review]
> Bug 22683: Allow prefetch with Koha::Account::Line->checkouts
> 
> Review of attachment 87751 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Account/Line.pm
> @@ +68,4 @@
> >  
> > +    my $old_issue_rs = $self->_result->old_issue;
> > +    return Koha::Old::Checkout->_new_from_dbic( $old_issue_rs ) if $old_issue_rs;
> > +    return undef;
> 
> should be only 'return;' I think...

It should amount to the same thing.. but I think you're right that our linter would complain about returning explicit undef.
Comment 6 Martin Renvoize 2019-04-16 12:14:20 UTC
Created attachment 88061 [details] [review]
Bug 22683: Allow prefetch with Koha::Account::Line->checkouts

This patch adds the required DBIC side relationships for the
Koha::Account::Line checkouts relationship accessor to support
prefetching and also updates the method to enable the capability.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2019-04-16 12:23:14 UTC
Created attachment 88062 [details] [review]
Bug 22683: (follow-up) Rename DBIC relationship accessor

Rename the DBIC side accessors to match the Koha::Object side one to
make the 'prefetch' simpler to guess.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Josef Moravec 2019-04-17 08:59:32 UTC
Created attachment 88147 [details] [review]
Bug 22683: (follow-up) Rename issue to checkout when call the relation
Comment 9 Josef Moravec 2019-04-17 09:00:46 UTC
There are some failing tests:

t/db_dependent/Accounts.t .............. 24/34 Value not allowed for auto_incr issue_id in Issue at /home/vagrant/kohaclone/t/lib/TestBuilder.pm line 372.

    #   Failed test 'Accountline created'
    #   at t/db_dependent/Accounts.t line 404.
    #          got: '0'
    #     expected: '1'
    # Looks like you planned 8 tests but ran 1.
    # Looks like you failed 1 test of 1 run.
t/db_dependent/Accounts.t .............. 26/34 
#   Failed test 'More Koha::Account::pay tests'
#   at t/db_dependent/Accounts.t line 430.
Can't call method "amountoutstanding" on an undefined value at /home/vagrant/kohaclone/Koha/Account.pm line 101.
# Looks like your test exited with 255 just after 26.
t/db_dependent/Accounts.t .............. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 9/34 subtests 
t/db_dependent/Koha/Account/Offsets.t .. Value not allowed for auto_incr issue_id in Issue at /home/vagrant/kohaclone/t/lib/TestBuilder.pm line 372.
    # No tests run!
t/db_dependent/Koha/Account/Offsets.t .. 1/1 
#   Failed test 'No tests run for subtest "total_outstanding() tests"'
#   at t/db_dependent/Koha/Account/Offsets.t line 70.
Can't call method "id" on an undefined value at t/db_dependent/Koha/Account/Offsets.t line 46.
# Looks like your test exited with 255 just after 1.
t/db_dependent/Koha/Account/Offsets.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 1/1 subtests 
t/db_dependent/Members.t ............... 35/47 Value not allowed for auto_incr issue_id in Issue at /home/vagrant/kohaclone/t/lib/TestBuilder.pm line 372.

    #   Failed test 'DeleteExpiredOpacRegistrations doesn't delete borrower with fine and no checkout'
    #   at t/db_dependent/Members.t line 382.
    #          got: '1'
    #     expected: '0'
    # Looks like you planned 5 tests but ran 4.
    # Looks like you failed 1 test of 4 run.
t/db_dependent/Members.t ............... 46/47 
#   Failed test 'purgeSelfRegistration'
#   at t/db_dependent/Members.t line 387.
Can't call method "delete" on an undefined value at t/db_dependent/Members.t line 384.
# Looks like your test exited with 255 just after 46.
t/db_dependent/Members.t ............... Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 2/47 subtests 

Test Summary Report
-------------------
t/db_dependent/Accounts.t            (Wstat: 65280 Tests: 26 Failed: 1)
  Failed test:  26
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 34 tests but ran 26.
t/db_dependent/Koha/Account/Offsets.t (Wstat: 65280 Tests: 1 Failed: 1)
  Failed test:  1
  Non-zero exit status: 255
t/db_dependent/Members.t             (Wstat: 65280 Tests: 46 Failed: 1)
  Failed test:  46
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 47 tests but ran 46.
Comment 10 Jonathan Druart 2020-01-30 11:03:28 UTC
You are adding 2 FK on the same column? I do not think this is possible.

Moreover you add a note for the 2 constraints:
+Note: This DBIC only relationship should be removed (as it will get automatically recreated)
+if we add a proper foreign key for accountlines.issue_id

But we will not be able to create this constraint at DB level.