From a7757f22e692147fdc96f2fe4c1926887f2980ad Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 4 Nov 2016 14:04:58 +0000 Subject: [PATCH] Bug 17553: Koha::Patron->get_overdues This patch is for discussion: As you can see it introduces a weird Koha::Object->unblessed_all_relateds subroutine. The idea is to fix a lack we will have soon: A lot of subroutines retrieves data from several tables and put them into a hashref. It's convenient for the use we have here: Send everything to a subroutine to generate a letter. The problem is that I did not find a good and elegant way to do that using DBIX::Class. This new method loops through all the "related resultsets" which are the prefetched tables. --- C4/Members.pm | 21 -------------------- Koha/Object.pm | 45 +++++++++++++++++++++++++++++++++++++++++-- Koha/Patron.pm | 24 +++++++++++++++++++++++ members/print_overdues.pl | 5 +++-- t/db_dependent/Koha/Patrons.t | 22 ++++++++++++++------- 5 files changed, 85 insertions(+), 32 deletions(-) diff --git a/C4/Members.pm b/C4/Members.pm index e334c61..6a3370f 100644 --- a/C4/Members.pm +++ b/C4/Members.pm @@ -85,8 +85,6 @@ BEGIN { &IssueSlip GetBorrowersWithEmail - - GetOverduesForPatron ); #Modify data @@ -1658,25 +1656,6 @@ WHERE borrowernumber = 0 AND DATEDIFF( NOW(), timestamp ) > ?|; return $cnt eq '0E0'? 0: $cnt; } -sub GetOverduesForPatron { - my ( $borrowernumber ) = @_; - - my $sql = " - SELECT * - FROM issues, items, biblio, biblioitems - WHERE items.itemnumber=issues.itemnumber - AND biblio.biblionumber = items.biblionumber - AND biblio.biblionumber = biblioitems.biblionumber - AND issues.borrowernumber = ? - AND date_due < NOW() - "; - - my $sth = C4::Context->dbh->prepare( $sql ); - $sth->execute( $borrowernumber ); - - return $sth->fetchall_arrayref({}); -} - END { } # module clean-up code here (global destructor) 1; diff --git a/Koha/Object.pm b/Koha/Object.pm index b5e11af..e99d1d6 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -190,6 +190,37 @@ sub unblessed { return { $self->_result->get_columns }; } +=head3 $object->unblessed_all_relateds + +my $everything_into_one_hashref = $object->unblessed_all_relateds + +The unblessed method only retrieves column' values for the column of the object. +In a *few* cases we want to retrieve the information of all the prefetched data. + +=cut + +sub unblessed_all_relateds { + my ($self) = @_; + + my %data; + my $related_resultsets = $self->_result->{related_resultsets} || {}; + my $rs = $self; + while ( $related_resultsets and %$related_resultsets ) { + my @relations = keys %{ $related_resultsets }; + if ( @relations ) { + my $relation = $relations[0]; + $rs = $rs->related_resultset($relation)->get_cache; + $rs = $rs->[0]; # Does it makes sense to have several values here? + my $object_class = Koha::Object::_get_object_class( $rs->result_class ); + my $koha_object = $object_class->_new_from_dbic( $rs ); + $related_resultsets = $rs->{related_resultsets}; + %data = ( %data, %{ $koha_object->unblessed } ); + } + } + %data = ( %data, %{ $self->unblessed } ); + return \%data; +} + =head3 $object->_result(); Returns the internal DBIC Row object @@ -221,6 +252,16 @@ sub _columns { return $self->{_columns}; } +sub _get_object_class { + my ( $type ) = @_; + return unless $type; + + if( $type->can('koha_object_class') ) { + return $type->koha_object_class; + } + $type =~ s|Schema::Result::||; + return ${type}; +} =head3 AUTOLOAD @@ -246,13 +287,13 @@ sub AUTOLOAD { } } - my @known_methods = qw( is_changed id in_storage get_column ); + my @known_methods = qw( is_changed id in_storage get_column related_resultset ); Koha::Exceptions::Object::MethodNotCoveredByTests->throw( "The method $method is not covered by tests!" ) unless grep {/^$method$/} @known_methods; my $r = eval { $self->_result->$method(@_) }; if ( $@ ) { - Koha::Exceptions::Object::MethodNotFound->throw( "No method $method for " . ref($self) ); + Koha::Exceptions::Object::MethodNotFound->throw( "No method $method for " . ref($self) . " " . $@ ); } return $r; } diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 9a2fd18..ded37a7 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -449,6 +449,30 @@ sub add_enrolment_fee_if_needed { return $enrolment_fee || 0; } +=head3 get_overdues + +my $overdue_items = $patron->get_overdues + +Return the overdued items + +=cut + +sub get_overdues { + my ($self) = @_; + # FIXME add date_due < now + my $dtf = Koha::Database->new->schema->storage->datetime_parser; + my $issues = Koha::Issues->search( + { + 'me.borrowernumber' => $self->borrowernumber, + 'me.date_due' => { '<' => $dtf->format_datetime(dt_from_string) }, + }, + { + prefetch => { item => { biblio => 'biblioitems' } }, + } + ); + return $issues; +} + =head3 type =cut diff --git a/members/print_overdues.pl b/members/print_overdues.pl index b63dec9..4d3e607 100755 --- a/members/print_overdues.pl +++ b/members/print_overdues.pl @@ -24,9 +24,10 @@ use CGI; use C4::Context; use C4::Auth; use C4::Output; -use C4::Members qw(GetOverduesForPatron); use C4::Overdues qw(parse_overdues_letter); +use Koha::Patrons; + my $input = new CGI; my $flagsrequired = { circulate => "circulate_remaining_permissions" }; @@ -45,7 +46,7 @@ my ( $template, $loggedinuser, $cookie ) = get_template_and_user( my $borrowernumber = $input->param('borrowernumber'); my $branchcode = C4::Context->userenv->{'branch'}; -my $overdues = GetOverduesForPatron($borrowernumber); +my $overdues = Koha::Patrons->find($borrowernumber)->get_overdues->unblessed_all_relateds; my $letter = parse_overdues_letter( { diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t index a40d9c3..f014671 100644 --- a/t/db_dependent/Koha/Patrons.t +++ b/t/db_dependent/Koha/Patrons.t @@ -516,7 +516,7 @@ subtest 'search_patrons_to_anonymise & anonymise_issue_history' => sub { }; subtest 'get_overdues' => sub { - plan tests => 3; + plan tests => 7; my $library = $builder->build( { source => 'Branch' } ); my ($biblionumber_1) = AddBiblio( MARC::Record->new, '' ); @@ -568,14 +568,22 @@ subtest 'get_overdues' => sub { AddIssue( $patron, $item_2->{barcode}, DateTime->now->subtract( days => 5 ) ); AddIssue( $patron, $item_3->{barcode} ); - my $overdues = C4::Members::GetOverduesForPatron( $patron->{borrowernumber} ); - is( @$overdues, 2, 'GetOverduesForPatron should return the correct number of elements' ); - is( $overdues->[0]->{itemnumber}, $item_1->{itemnumber}, 'The issue should be returned in the same order as they have been done, first is correct' ); - is( $overdues->[1]->{itemnumber}, $item_2->{itemnumber}, 'The issue should be returned in the same order as they have been done, second is correct' ); + $patron = Koha::Patrons->find( $patron->{borrowernumber} ); + my $overdues = $patron->get_overdues; + is( $overdues->count, 2, 'Patron should have 2 overdues'); + is( $overdues->next->itemnumber, $item_1->{itemnumber}, 'The issue should be returned in the same order as they have been done, first is correct' ); + is( $overdues->next->itemnumber, $item_2->{itemnumber}, 'The issue should be returned in the same order as they have been done, second is correct' ); + + my $o = $overdues->reset->next; + my $unblessed_overdue = $o->unblessed_all_relateds; + is( exists( $unblessed_overdue->{issuedate} ), 1, 'Fields from the issues table should be filled' ); + is( exists( $unblessed_overdue->{itemcallnumber} ), 1, 'Fields from the items table should be filled' ); + is( exists( $unblessed_overdue->{title} ), 1, 'Fields from the biblio table should be filled' ); + is( exists( $unblessed_overdue->{itemtype} ), 1, 'Fields from the biblioitems table should be filled' ); # Clean stuffs - Koha::Issues->search( { borrowernumber => $patron->{borrowernumber} } )->delete; - Koha::Patrons->find( $patron->{borrowernumber} )->delete; + Koha::Issues->search( { borrowernumber => $patron->borrowernumber } )->delete; + $patron->delete; }; $retrieved_patron_1->delete; -- 2.1.4