From 02db36b95ce3611f818fb2fd2c3d37632e2b5d10 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Wed, 22 Sep 2021 15:46:26 -0300 Subject: [PATCH] Bug 29083: Update article requests-related Koha::Patron methods to use relationships This patch makes Koha::Patron->article_requests use the underlying DBIC relationship and _new_from_dbic instead of a plain search. It also refactors 'article_requests_current' and 'article_requests_finished' to use ->article_requests, as well as the new methods introduced by bug 29082 for filtering. No behavior change should take place. To test: 1. Apply the unit tests patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Patron.t \ t/db_dependent/ArticleRequests.t => SUCCESS: Tests pass! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! 5. Sign off :-D Signed-off-by: Martin Renvoize Signed-off-by: Nick Clemens Bug 29083: Unit tests This patch adds missing tests for Koha::Patron->article_requests and moves (and extends) tests for 'article_requests_current' and 'article_requests_finished' that were originally in ArticleRequests.t into Koha/Patron.t as we now do. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/ArticleRequests.t \ t/db_dependent/Koha/Patron.t => SUCCESS: Tests pass! Signed-off-by: Martin Renvoize Signed-off-by: Nick Clemens Bug 29083: (QA follow-up) Remove unused param Signed-off-by: Tomas Cohen Arazi Signed-off-by: Martin Renvoize Signed-off-by: Nick Clemens Bug 29083: Fix OPAC listing of article requests This patch makes the OPAC template reuse a precalculated value for the active article requests for the patron (and its count). The original code relied on the methods returning a list, which is not the case for _new_from_dbic until bug 28883 is pushed. This patch fixes that. Note: there was an odd behavior when ArticleRequests was enabled but no active article requests were present: the tab wasn't rendered but the 'empty table' with the 'You have no article requests currently.' message was displayed below the Checkouts tab. I'm not sure that was caused by this patches, or other. Fixed on this patch. To test: 1. In the OPAC, go to 'your summary' => FAIL: Things don't show for article requests 2. Add some article requests and repeat 1 => FAIL: Something's wrong there 3. Apply this patch and repeat 1 => Yes! Things show correctly! 4. Cancel all your article requests => SUCCESS: Things render as they should 5. Re-enter the 'your summary' page (to force re-rendering) => SUCCESS: Things render correctly for empty article requests 6. Sign off :-D Signed-off-by: Tomas Cohen Arazi Signed-off-by: Martin Renvoize Signed-off-by: Nick Clemens Bug 29083: Remove article_requests_finished and article_requests_current This patch removes those methods that are not really needed. Templates are adjusted to use the expected combination of ->article_requests->filter_by_current. To test: 1. Apply this patch 2. Visit a patron with article requests => SUCCESS: All works 3. Run: $ kshell k$ prove t/db_dependent/Koha/Patron.t => SUCCESS: Tests pass, less tests. 4. Sign off :-D Signed-off-by: Tomas Cohen Arazi Signed-off-by: Jonathan Druart --- Koha/Patron.pm | 62 ++----------------- .../en/includes/patron-article-requests.inc | 6 +- .../prog/en/modules/circ/circulation.tt | 4 +- .../prog/en/modules/members/moremember.tt | 4 +- .../bootstrap/en/modules/opac-user.tt | 10 +-- opac/opac-user.pl | 7 +++ t/db_dependent/ArticleRequests.t | 27 +------- t/db_dependent/Koha/Patron.t | 35 ++++++++++- 8 files changed, 62 insertions(+), 93 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index c1a86a9da0..9fd93e8c44 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1003,70 +1003,16 @@ sub can_request_article { =head3 article_requests -my @requests = $borrower->article_requests(); -my $requests = $borrower->article_requests(); + my $article_requests = $patron->article_requests; -Returns either a list of ArticleRequests objects, -or an ArtitleRequests object, depending on the -calling context. +Returns the patron article requests. =cut sub article_requests { - my ( $self ) = @_; - - $self->{_article_requests} ||= Koha::ArticleRequests->search({ borrowernumber => $self->borrowernumber() }); - - return $self->{_article_requests}; -} - -=head3 article_requests_current - -my @requests = $patron->article_requests_current - -Returns the article requests associated with this patron that are incomplete - -=cut - -sub article_requests_current { - my ( $self ) = @_; - - $self->{_article_requests_current} ||= Koha::ArticleRequests->search( - { - borrowernumber => $self->id(), - -or => [ - { status => Koha::ArticleRequest::Status::Requested }, - { status => Koha::ArticleRequest::Status::Pending }, - { status => Koha::ArticleRequest::Status::Processing } - ] - } - ); - - return $self->{_article_requests_current}; -} - -=head3 article_requests_finished - -my @requests = $biblio->article_requests_finished - -Returns the article requests associated with this patron that are completed - -=cut - -sub article_requests_finished { - my ( $self, $borrower ) = @_; - - $self->{_article_requests_finished} ||= Koha::ArticleRequests->search( - { - borrowernumber => $self->id(), - -or => [ - { status => Koha::ArticleRequest::Status::Completed }, - { status => Koha::ArticleRequest::Status::Canceled } - ] - } - ); + my ($self) = @_; - return $self->{_article_requests_finished}; + return Koha::ArticleRequests->_new_from_dbic( scalar $self->_result->article_requests ); } =head3 add_enrolment_fee_if_needed diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/patron-article-requests.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/patron-article-requests.inc index b2a25a0580..a22325e785 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/patron-article-requests.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/patron-article-requests.inc @@ -1,5 +1,7 @@ +[% USE Context %] +[% SET current_article_requests = Context.Scalar( Context.Scalar( patron, 'article_requests' ), 'filter_by_current' ) %]
-[% IF patron.article_requests_current.count %] +[% IF current_article_requests.count > 0 %] @@ -20,7 +22,7 @@ - [% FOREACH ar IN patron.article_requests_current %] + [% FOREACH ar IN current_article_requests %]
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt index 4a563b6c92..d5e83f673a 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt @@ -1,5 +1,6 @@ [% USE raw %] [% USE Asset %] +[% USE Context %] [% USE Koha %] [% USE Branches %] [% USE KohaDates %] @@ -780,8 +781,9 @@ [% IF Koha.Preference('ArticleRequests') %] + [% SET current_article_requests = Context.Scalar( Context.Scalar( patron, 'article_requests' ), 'filter_by_current' ) %]
  • - [% patron.article_requests_current.count | html %] Article requests + [% current_article_requests.count | html %] Article requests
    • [% END %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt index 300d642bea..55c4d8e740 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ -1,5 +1,6 @@ [% USE raw %] [% USE Asset %] +[% USE Context %] [% USE Koha %] [% USE Branches %] [% USE ItemTypes %] @@ -695,8 +696,9 @@ [% END %] [% IF Koha.Preference('ArticleRequests') %] + [% SET article_requests = Context.Scalar( Context.Scalar( patron, 'article_requests' ), 'filter_by_current') %]
  • - [% patron.article_requests_current.count | html %] Article requests + [% article_requests.count | html %] Article requests
    • [% END %] diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt index 730400e7e1..dac477d909 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt @@ -230,7 +230,7 @@ [% END %] [% IF ( RESERVES.count ) %]
  • Holds ([% RESERVES.count | html %])
    • [% END %] - [% IF Koha.Preference('ArticleRequests') && logged_in_user.article_requests_current %]
  • Article requests ([% logged_in_user.article_requests_current.count | html %])
    • [% END %] + [% IF Koha.Preference('ArticleRequests') %]
  • Article requests ([% current_article_requests_count || 0 | html %])
    • [% END %] [% IF ( OverDriveCirculation ) %]
  • OverDrive account
    • [% END %] @@ -765,7 +765,7 @@ [% IF Koha.Preference('ArticleRequests') %]
    - [% IF logged_in_user.article_requests_current.count %] + [% IF current_article_requests_count %] @@ -788,7 +788,7 @@ - [% FOREACH ar IN logged_in_user.article_requests_current %] + [% FOREACH ar IN current_article_requests %]
    Article requests
    [% INCLUDE 'biblio-title.inc' biblio=ar.biblio %] @@ -866,7 +866,7 @@
    Article requests
    You have no article requests currently.
    - [% END # IF article_requests_current.count %] + [% END # IF current_article_requests_count %]
    [% END %] @@ -904,7 +904,7 @@ } } $(document).ready(function(){ - $('#opac-user-article-requests caption .count').html(AR_CAPTION_COUNT.format('[% logged_in_user.article_requests_current.count | html %]')); + $('#opac-user-article-requests caption .count').html(AR_CAPTION_COUNT.format('[% current_article_requests_count | html %]')); $('#opac-user-views').tabs(); $(".modal-nojs").addClass("modal").addClass("hide").removeClass("modal-nojs"); $(".suspend-until").prop("readonly",1); diff --git a/opac/opac-user.pl b/opac/opac-user.pl index 8deb054e72..0515b162a3 100755 --- a/opac/opac-user.pl +++ b/opac/opac-user.pl @@ -386,6 +386,13 @@ if ( C4::Context->preference('AllowPatronToSetFinesVisibilityForGuarantor') $template->param( relatives_with_fines => \@relatives_with_fines ); } +if ( C4::Context->preference("ArticleRequests") ) { + my @current_article_requests = $patron->article_requests->filter_by_current->as_list; + $template->param( + current_article_requests => \@current_article_requests, + current_article_requests_count => scalar @current_article_requests, + ); +} $template->param( patron_messages => $patron_messages, diff --git a/t/db_dependent/ArticleRequests.t b/t/db_dependent/ArticleRequests.t index 2313e1bc63..acc5704121 100755 --- a/t/db_dependent/ArticleRequests.t +++ b/t/db_dependent/ArticleRequests.t @@ -19,7 +19,7 @@ use Modern::Perl; use POSIX qw(strftime); -use Test::More tests => 54; +use Test::More tests => 45; use Test::MockModule; use t::lib::TestBuilder; @@ -110,30 +110,7 @@ is( $article_request->biblio->id, $biblio->id, '$ar->biblio() gets correspondi is( $article_request->item->id, $item->id, '$ar->item() gets corresponding Koha::Item object' ); is( $article_request->borrower->id, $patron->id, '$ar->borrower() gets corresponding Koha::Patron object' ); -my $ar = $patron->article_requests(); -is( ref($ar), 'Koha::ArticleRequests', '$patron->article_requests returns Koha::ArticleRequests object' ); -is( $ar->next->id, $article_request->id, 'Returned article request matches' ); - -is( $patron->article_requests_current()->count(), 1, 'Open request returned for article_requests_current' ); -$article_request->process(); -is( $patron->article_requests_current()->count(), 1, 'Processing request returned for article_requests_current' ); -$article_request->complete(); -is( $patron->article_requests_current()->count(), 0, 'Completed request not returned for article_requests_current' ); -$article_request->cancel(); -is( $patron->article_requests_current()->count(), 0, 'Canceled request not returned for article_requests_current' ); - -$article_request->set_pending(); - -is( $patron->article_requests_finished()->count(), 0, 'Open request returned for article_requests_finished' ); -$article_request->process(); -is( $patron->article_requests_finished()->count(), 0, 'Processing request returned for article_requests_finished' ); -$article_request->complete(); -$article_request->cancel(); -is( $patron->article_requests_finished()->count(), 1, 'Canceled request not returned for article_requests_finished' ); - -$article_request->set_pending(); - -$ar = $biblio->article_requests(); +my $ar = $biblio->article_requests(); is( ref($ar), 'Koha::ArticleRequests', '$biblio->article_requests returns Koha::ArticleRequests object' ); is( $ar->next->id, $article_request->id, 'Returned article request matches' ); diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index dc6a949436..2929349661 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -19,7 +19,7 @@ use Modern::Perl; -use Test::More tests => 9; +use Test::More tests => 10; use Test::Exception; use Test::Warn; @@ -813,3 +813,36 @@ subtest 'can_request_article() tests' => sub { $schema->storage->txn_rollback; }; + +subtest 'article_requests() tests' => sub { + + plan tests => 3; + + $schema->storage->txn_begin; + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + + my $article_requests = $patron->article_requests; + is( ref($article_requests), 'Koha::ArticleRequests', + 'In scalar context, type is correct' ); + is( $article_requests->count, 0, 'No article requests' ); + + foreach my $i ( 0 .. 3 ) { + + my $item = $builder->build_sample_item; + + Koha::ArticleRequest->new( + { + borrowernumber => $patron->id, + biblionumber => $item->biblionumber, + itemnumber => $item->id, + title => "Title", + } + )->request; + } + + $article_requests = $patron->article_requests; + is( $article_requests->count, 4, '4 article requests' ); + + $schema->storage->txn_rollback; +}; -- 2.20.1