From d533a92aa8283de29dd64cdf698588dd707af353 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 12 May 2022 11:52:45 +0200 Subject: [PATCH] Bug 23991: Move SearchSuggestion to Koha::Suggestions The C4::Suggestions::SearchSuggestion subroutine is badly written and can be replaced by calls to Koha::Suggestions->search. The hard part in this patch is suggestion.pl, the other occurrences have been replaced easily. Test plan: The idea is to test the whole suggestion workflow. 1. Create a suggestion on OPAC 2. Create a suggestion on the staff interface 3. Edit suggestions 4. Filter suggestions (use the different filters and "organize by" values) Signed-off-by: Tomas Cohen Arazi Signed-off-by: Nick Clemens Bug 23991: Remove SearchSuggestion tests Signed-off-by: Tomas Cohen Arazi Signed-off-by: Nick Clemens Bug 23991: (QA follow-up) Save some DB queries This patch makes the suggestion-related pages rely on array size instead of querying the DB each time they need to. In the case of suggestion/suggestion.pl it goes from 4 COUNT(*) to 1. To test, with KTD: 1. Run on the host machine: $ docker exec -ti koha_db_1 bash $ mysql -ppassword > SET GLOBAL general_log_file='/var/log/mysql/mycustom.log'; > SET GLOBAL log_output = 'FILE'; > SET GLOBAL general_log = 'ON'; > \q $ tail -f /var/log/mysql/mycustom.log | grep suggestions 2. Visit the different pages changed on this bug => SUCCESS: Some queries 3. Apply this patch 4. Repeat 2 => SUCCESS: Less queries! 5. Sign off :-D Signed-off-by: Tomas Cohen Arazi Signed-off-by: Nick Clemens Bug 23991: Fix branchcode and budgetid filtering Signed-off-by: Nick Clemens Bug 23991: Fix conflict with bug 28941 Well, this patchset fixed the security bug... Redoing on top of bug 28941 Signed-off-by: Nick Clemens Bug 23991: (follow-up) Missing semicolon Signed-off-by: Nick Clemens Bug 23991: Fix 'all' libraries Signed-off-by: Nick Clemens Bug 23991: (follow-up) Add value to filter_archived Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi --- C4/Suggestions.pm | 150 ------------------ acqui/newordersuggestion.pl | 22 +-- .../en/modules/acqui/newordersuggestion.tt | 50 +++--- .../modules/members/purchase-suggestions.tt | 10 +- .../prog/en/modules/suggestion/suggestion.tt | 136 ++++++++-------- .../bootstrap/en/modules/opac-suggestions.tt | 68 ++++---- members/deletemem.pl | 8 +- members/purchase-suggestions.pl | 8 +- opac/opac-suggestions.pl | 73 ++++----- suggestion/suggestion.pl | 83 +++++++--- t/db_dependent/Suggestions.t | 71 +-------- 11 files changed, 235 insertions(+), 444 deletions(-) diff --git a/C4/Suggestions.pm b/C4/Suggestions.pm index 88186655d9..2d30913f0b 100644 --- a/C4/Suggestions.pm +++ b/C4/Suggestions.pm @@ -41,7 +41,6 @@ our @EXPORT = qw( ModStatus ModSuggestion NewSuggestion - SearchSuggestion DelSuggestionsOlderThan GetUnprocessedSuggestions MarcRecordFromNewSuggestion @@ -72,155 +71,6 @@ Suggestions done by other borrowers can be seen when not "AVAILABLE" =head1 FUNCTIONS -=head2 SearchSuggestion - -(\@array) = &SearchSuggestion($suggestionhashref_to_search) - -searches for a suggestion - -return : -C<\@array> : the aqorders found. Array of hash. -Note the status is stored twice : -* in the status field -* as parameter ( for example ASKED => 1, or REJECTED => 1) . This is for template & translation purposes. - -=cut - -sub SearchSuggestion { - my ($suggestion) = @_; - my $dbh = C4::Context->dbh; - my @sql_params; - my @query = ( - q{ - SELECT suggestions.*, - U1.branchcode AS branchcodesuggestedby, - B1.branchname AS branchnamesuggestedby, - U1.surname AS surnamesuggestedby, - U1.firstname AS firstnamesuggestedby, - U1.cardnumber AS cardnumbersuggestedby, - U1.email AS emailsuggestedby, - U1.borrowernumber AS borrnumsuggestedby, - U1.categorycode AS categorycodesuggestedby, - C1.description AS categorydescriptionsuggestedby, - U2.surname AS surnamemanagedby, - U2.firstname AS firstnamemanagedby, - B2.branchname AS branchnamesuggestedby, - U2.email AS emailmanagedby, - U2.branchcode AS branchcodemanagedby, - U2.borrowernumber AS borrnummanagedby, - U3.surname AS surnamelastmodificationby, - U3.firstname AS firstnamelastmodificationby, - BU.budget_name AS budget_name - FROM suggestions - LEFT JOIN borrowers AS U1 ON suggestedby=U1.borrowernumber - LEFT JOIN branches AS B1 ON B1.branchcode=U1.branchcode - LEFT JOIN categories AS C1 ON C1.categorycode=U1.categorycode - LEFT JOIN borrowers AS U2 ON managedby=U2.borrowernumber - LEFT JOIN branches AS B2 ON B2.branchcode=U2.branchcode - LEFT JOIN categories AS C2 ON C2.categorycode=U2.categorycode - LEFT JOIN borrowers AS U3 ON lastmodificationby=U3.borrowernumber - LEFT JOIN aqbudgets AS BU ON budgetid=BU.budget_id - WHERE 1=1 - } - ); - - # filter on biblio informations - foreach my $field ( - qw( title author isbn publishercode copyrightdate collectiontitle )) - { - if ( $suggestion->{$field} ) { - push @sql_params, '%' . $suggestion->{$field} . '%'; - push @query, qq{ AND suggestions.$field LIKE ? }; - } - } - - # filter on user branch - if ( C4::Context->preference('IndependentBranches') - && !C4::Context->IsSuperLibrarian() ) - { - # If IndependentBranches is set and the logged in user is not superlibrarian - # Then we want to filter by the user's library (i.e. cannot see suggestions from other libraries) - my $userenv = C4::Context->userenv; - if ($userenv) { - { - push @sql_params, $$userenv{branch}; - push @query, q{ - AND (suggestions.branchcode=? OR suggestions.branchcode='') - }; - } - } - } - elsif (defined $suggestion->{branchcode} - && $suggestion->{branchcode} - && $suggestion->{branchcode} ne '__ANY__' ) - { - # If IndependentBranches is not set OR the logged in user is not superlibrarian - # AND the branchcode filter is passed and not '__ANY__' - # Then we want to filter using this parameter - push @sql_params, $suggestion->{branchcode}; - push @query, qq{ AND suggestions.branchcode=? }; - } - - # filter on nillable fields - foreach my $field ( - qw( STATUS itemtype suggestedby managedby acceptedby budgetid biblionumber ) - ) - { - if ( exists $suggestion->{$field} - and defined $suggestion->{$field} - and $suggestion->{$field} ne '__ANY__' - and ( - $suggestion->{$field} ne q|| - or $field eq 'STATUS' - ) - ) { - if ( $suggestion->{$field} eq '__NONE__' ) { - push @query, qq{ AND (suggestions.$field = '' OR suggestions.$field IS NULL) }; - } - else { - push @sql_params, $suggestion->{$field}; - push @query, qq{ AND suggestions.$field = ? }; - } - } - } - - # filter on date fields - my $dtf = Koha::Database->new->schema->storage->datetime_parser; - foreach my $field (qw( suggesteddate manageddate accepteddate )) { - my $from = $field . "_from"; - my $to = $field . "_to"; - my $from_dt; - $from_dt = eval { dt_from_string( $suggestion->{$from} ) } if ( $suggestion->{$from} ); - my $to_dt; - $to_dt = eval { dt_from_string( $suggestion->{$to} ) } if ( $suggestion->{$to} ); - if ( $from_dt ) { - push @query, qq{ AND suggestions.$field >= ?}; - push @sql_params, $dtf->format_date($from_dt); - } - if ( $to_dt ) { - push @query, qq{ AND suggestions.$field <= ?}; - push @sql_params, $dtf->format_date($to_dt); - } - } - - # By default do not search for archived suggestions - unless ( exists $suggestion->{archived} && $suggestion->{archived} ) { - push @query, q{ AND suggestions.archived = 0 }; - } - - my $sth = $dbh->prepare("@query"); - $sth->execute(@sql_params); - my @results; - - # add status as field - while ( my $data = $sth->fetchrow_hashref ) { - $data->{ $data->{STATUS} } = 1; - push( @results, $data ); - } - - return ( \@results ); -} - =head2 GetSuggestion \%sth = &GetSuggestion($suggestionid) diff --git a/acqui/newordersuggestion.pl b/acqui/newordersuggestion.pl index 0ee10365cb..9d35ec48de 100755 --- a/acqui/newordersuggestion.pl +++ b/acqui/newordersuggestion.pl @@ -93,10 +93,11 @@ use Modern::Perl; use CGI qw ( -utf8 ); use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_html_with_http_headers ); -use C4::Suggestions qw( ConnectSuggestionAndBiblio SearchSuggestion ); +use C4::Suggestions qw( ConnectSuggestionAndBiblio ); use C4::Budgets; use Koha::Acquisition::Booksellers; +use Koha::Suggestions; my $input = CGI->new; @@ -127,23 +128,22 @@ if ( $op eq 'connectDuplicate' ) { ConnectSuggestionAndBiblio( $suggestionid, $duplicateNumber ); } -# getting all suggestions. -my $suggestions_loop = SearchSuggestion( +my $suggestions = [ Koha::Suggestions->search_limited( { - author => $author, - title => $title, - publishercode => $publishercode, - STATUS => 'ACCEPTED' - } -); + ( $author ? ( author => $author ) : () ), + ( $title ? ( title => $title ) : () ), + ( $publishercode ? ( publishercode => $publishercode ) : () ), + STATUS => 'ACCEPTED' + }, + { prefetch => ['managedby', 'suggestedby'] }, +)->as_list ]; my $vendor = Koha::Acquisition::Booksellers->find( $booksellerid ); $template->param( - suggestions_loop => $suggestions_loop, + suggestions => $suggestions, basketno => $basketno, booksellerid => $booksellerid, name => $vendor->name, - loggedinuser => $borrowernumber, "op_$op" => 1, ); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/newordersuggestion.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/newordersuggestion.tt index 0946308afb..278984d2c7 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/newordersuggestion.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/newordersuggestion.tt @@ -41,7 +41,7 @@

Suggestions

- [% IF ( suggestions_loop ) %] + [% IF suggestions.size %] Show only mine | Show all suggestions @@ -59,49 +59,45 @@ - [% FOREACH suggestions_loo IN suggestions_loop %] + [% FOREACH suggestion IN suggestions %] - + + + - - @@ -136,7 +132,7 @@ })); $("#show_only_mine").on('click', function(e){ e.preventDefault(); - suggestionst.fnFilter('^[% loggedinuser | html %]$', 0, true); + suggestionst.fnFilter('^[% logged_in_user.borrowernumber | html %]$', 0, true); }); $("#show_all").on('click', function(e){ e.preventDefault(); diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt index fdfbbb143a..24da899b90 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/purchase-suggestions.tt @@ -42,7 +42,7 @@ New purchase suggestion - [% IF suggestions %] + [% IF suggestions.size %]
[% suggestions_loo.managedby | html %][% suggestion.managedby | html %] -

[% suggestions_loo.title | html %] - [% suggestions_loo.author | html %]

+

[% suggestion.title | html %] - [% suggestion.author | html %]

- [% IF ( suggestions_loo.copyrightdate ) %]© [% suggestions_loo.copyrightdate | html %] [% END %] - [% IF ( suggestions_loo.volumedesc ) %]volume: [% suggestions_loo.volumedesc | html %] [% END %] - [% IF ( suggestions_loo.isbn ) %]ISBN: [% suggestions_loo.isbn | html %] [% END %] - [% IF ( suggestions_loo.publishercode ) %]
published by: [% suggestions_loo.publishercode | html %] [% END %] - [% IF ( suggestions_loo.publicationyear ) %] in [% suggestions_loo.publicationyear | html %] [% END %] - [% IF ( suggestions_loo.place ) %] in [% suggestions_loo.place | html %] [% END %] - [% IF ( suggestions_loo.note ) %]

([% suggestions_loo.note | html %])

[% END %] + [% IF ( suggestion.copyrightdate ) %]© [% suggestion.copyrightdate | html %] [% END %] + [% IF ( suggestion.volumedesc ) %]volume: [% suggestion.volumedesc | html %] [% END %] + [% IF ( suggestion.isbn ) %]ISBN: [% suggestion.isbn | html %] [% END %] + [% IF ( suggestion.publishercode ) %]
published by: [% suggestion.publishercode | html %] [% END %] + [% IF ( suggestion.publicationyear ) %] in [% suggestion.publicationyear | html %] [% END %] + [% IF ( suggestion.place ) %] in [% suggestion.place | html %] [% END %] + [% IF ( suggestion.note ) %]

([% suggestion.note | html %])

[% END %]

[% INCLUDE 'patron-title.inc' patron => suggestion.suggester %][% INCLUDE 'patron-title.inc' patron => suggestion.manager %] - [% suggestions_loo.surnamesuggestedby | html %][% IF ( suggestions_loo.firstnamesuggestedby ) %],[% END %] [% suggestions_loo.firstnamesuggestedby | html %] + [% Branches.GetName(suggestion.branchcode) | html %] - [% suggestions_loo.surnamemanagedby | html %][% IF ( suggestions_loo.firstnamemanagedby ) %],[% END %] [% suggestions_loo.firstnamemanagedby | html %] + [% suggestion.fund.budget_name | html %] - [% Branches.GetName(suggestions_loo.branchcode) | html %] + [% suggestion.price | $Price %] - [% suggestions_loo.budget_name | html %] - - [% suggestions_loo.price | $Price %] - - [% IF (suggestions_loo.quantity > 0) %] - [% suggestions_loo.quantity | html %] + [% IF (suggestion.quantity > 0) %] + [% suggestion.quantity | html %] [% END %] - [% suggestions_loo.total | $Price %] + [% suggestion.total | $Price %] - [% IF ( suggestions_loo.biblionumber ) %] - [% tp('verb', 'Order') | html %] + [% IF ( suggestion.biblionumber ) %] + [% tp('verb', 'Order') | html %] [% ELSE %] - [% tp('verb', 'Order') | html %] + [% tp('verb', 'Order') | html %] [% END %]
@@ -79,13 +79,7 @@ - [% FOREACH suggestions_loo IN suggestion.suggestions_loop %] + [% FOREACH s IN suggestion.suggestions %] - - + - - - [% END # /FOREACH suggestions_loo %] + [% END # /FOREACH s %]
[% s.note | html %] - [% IF ( s.surnamemanagedby ) %] - [% s.surnamemanagedby | html %] - [% IF ( s.firstnamemanagedby ) %],[% END %] - [% s.firstnamemanagedby | html %] - [% ELSE %] -   - [% END %] + [% INCLUDE 'patron-title.inc' patron => s.manager %] [% s.manageddate | $KohaDates %] diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt index 8a3fec4128..b4cd1a4dfc 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/suggestion/suggestion.tt @@ -707,19 +707,19 @@ No name [% END %] [% END %] - ([% suggestion.suggestions_loop.size | html %]) - - + ([% suggestion.suggestions.size| html %]) + + [% END # /FOREACH suggestion %]
[% END # /UNLESS notabs %] [% FOREACH suggestion IN suggestions %] -
+
- [% IF ( suggestion.suggestions_loop ) %] + [% IF suggestion.suggestions.size %]

Check all | Uncheck all

@@ -744,114 +744,117 @@
- + - - [% suggestions_loo.title | html %][% IF ( suggestions_loo.author ) %], by [% suggestions_loo.author | html %][% END %] + + [% s.title | html %][% IF ( s.author ) %], by [% s.author | html %][% END %]
- [% IF ( suggestions_loo.copyrightdate ) %] - © [% suggestions_loo.copyrightdate | html %] + [% IF ( s.copyrightdate ) %] + © [% s.copyrightdate | html %] [% END %] - [% IF ( suggestions_loo.volumedesc ) %] - ; Volume:[% suggestions_loo.volumedesc | html %] + [% IF ( s.volumedesc ) %] + ; Volume:[% s.volumedesc | html %] [% END %] - [% IF ( suggestions_loo.isbn ) %] - ; ISBN: [% suggestions_loo.isbn | html %] + [% IF ( s.isbn ) %] + ; ISBN: [% s.isbn | html %] [% END %] - [% IF ( suggestions_loo.publishercode ) %] - ; Published by [% suggestions_loo.publishercode | html %] + [% IF ( s.publishercode ) %] + ; Published by [% s.publishercode | html %] [% END %] - [% IF ( suggestions_loo.publicationyear ) %] - in [% suggestions_loo.publicationyear | html %] + [% IF ( s.publicationyear ) %] + in [% s.publicationyear | html %] [% END %] - [% IF ( suggestions_loo.place ) %] - in [% suggestions_loo.place | html %] + [% IF ( s.place ) %] + in [% s.place | html %] [% END %] - [% IF ( suggestions_loo.collectiontitle ) %] - ; [% suggestions_loo.collectiontitle | html %] + [% IF ( s.collectiontitle ) %] + ; [% s.collectiontitle | html %] [% END %] - [% IF ( suggestions_loo.itemtype ) %] - ; [% AuthorisedValues.GetByCode( 'SUGGEST_FORMAT', suggestions_loo.itemtype, 0 ) | html %] + [% IF ( s.itemtype ) %] + ; [% AuthorisedValues.GetByCode( 'SUGGEST_FORMAT', s.itemtype, 0 ) | html %] [% END %]
- [% IF ( suggestions_loo.note ) %] -
[% suggestions_loo.note | html %]
+ [% IF ( s.note ) %] +
[% s.note | html %]
[% END %] - [% IF suggestions_loo.archived %] + [% IF s.archived %]
Archived [% END %] 		- [% suggestions_loo.surnamesuggestedby | html %][% IF ( suggestions_loo.firstnamesuggestedby ) %], [% suggestions_loo.firstnamesuggestedby | html %][% END %] [% IF (suggestions_loo.cardnumbersuggestedby ) %]([% suggestions_loo.cardnumbersuggestedby | html %])[% END %] + [% SET suggester = s.suggester %] + [% suggester.surname | html %][% IF suggester.firstname %], [% suggester.firstname | html %][% END %] [% IF suggester.cardnumber %]([% suggester.cardnumber | html %])[% END %] - [% IF ( suggestions_loo.suggesteddate ) %][% suggestions_loo.suggesteddate | $KohaDates %][% END %] + + [% IF ( s.suggesteddate ) %][% s.suggesteddate | $KohaDates %][% END %] [% AuthorisedValues.GetByCode( 'OPAC_SUG', suggestions_loo.patronreason ) | html %][% AuthorisedValues.GetByCode( 'OPAC_SUG', s.patronreason ) | html %] - [% suggestions_loo.surnamemanagedby | html %][% IF ( suggestions_loo.firstnamemanagedby ) %], [% suggestions_loo.firstnamemanagedby | html %][% END %] + [% SET manager = s.manager %] + [% manager.surname | html %][% IF manager.firstname %], [% manager.firstname | html %][% END %] - [% IF ( suggestions_loo.manageddate ) %][% suggestions_loo.manageddate | $KohaDates %][% END %] + + [% IF ( s.manageddate ) %][% s.manageddate | $KohaDates %][% END %] - [% suggestions_loo.surnamelastmodificationby | html %][% IF ( suggestions_loo.firstnamelastmodificationby ) %], [% suggestions_loo.firstnamelastmodificationby | html %][% END %] + [% SET last_modifier = s.last_modifier %] + [% last_modifier.surname | html %][% IF last_modifier.firstname %], [% last_modifier.firstname | html %][% END %] - [% IF ( suggestions_loo.lastmodificationdate ) %][% suggestions_loo.lastmodificationdate | $KohaDates %][% END %] + + [% IF ( s.lastmodificationdate ) %][% s.lastmodificationdate | $KohaDates %][% END %] - [% suggestions_loo.date | $KohaDates %] + [% s.lastmodificationdate | $KohaDates %] - [% Branches.GetName( suggestions_loo.branchcode ) | html %] + [% Branches.GetName( s.branchcode ) | html %] - [% suggestions_loo.budget_name | html %] + [% s.fund.budget_name | html %] - [% IF ( suggestions_loo.ASKED ) %] + [% IF s.STATUS == 'ASKED' %] Pending - [% ELSIF ( suggestions_loo.ACCEPTED ) %] + [% ELSIF s.STATUS == 'ACCEPTED' %] Accepted - [% ELSIF ( suggestions_loo.ORDERED ) %] + [% ELSIF s.STATUS == 'ORDERED' %] Ordered - [% ELSIF ( suggestions_loo.REJECTED ) %] + [% ELSIF s.STATUS == 'REJECTED' %] Rejected - [% ELSIF ( suggestions_loo.CHECKED ) %] + [% ELSIF s.STATUS == 'CHECKED' %] Checked - [% ELSIF ( suggestions_loo.AVAILABLE ) %] + [% ELSIF s.STATUS == 'AVAILABLE' %] Available - [% ELSIF AuthorisedValues.GetByCode( 'SUGGEST_STATUS', suggestions_loo.STATUS ) %] - [% AuthorisedValues.GetByCode( 'SUGGEST_STATUS', suggestions_loo.STATUS ) | html %] + [% ELSIF AuthorisedValues.GetByCode( 'SUGGEST_STATUS', s.STATUS ) %] + [% AuthorisedValues.GetByCode( 'SUGGEST_STATUS', s.STATUS ) | html %] [% ELSE %] Status unknown [% END %] - [% IF ( suggestions_loo.reason ) %] -
([% suggestions_loo.reason | html %]) + [% IF ( s.reason ) %] +
([% s.reason | html %]) [% END %]
- Edit -
@@ -960,16 +963,6 @@ -
-
- Archive selected - - -
- -
-
-
[% ELSE %] @@ -1067,9 +1060,9 @@
  • [% IF filter_archived %] - + [% ELSE %] - + [% END %]
  • @@ -1321,9 +1314,10 @@ if( $("#suggestiontabs .tab-pane.active").length < 1 ){ $("#suggestiontabs a:first").tab("show"); } + table_settings = [% TablesSettings.GetTableSettings( 'acqui', 'suggestions', 'suggestions', 'json' ) | $raw %] [% FOREACH suggestion IN suggestions %] - [% IF ( suggestion.suggestions_loop ) %] + [% IF suggestion.suggestions.size %] KohaTable("table_[% loop.count| html %]", { "sorting": [[ 3, "asc" ]], "autoWidth": false, diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-suggestions.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-suggestions.tt index 0a2a26460f..cdc7927a5c 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-suggestions.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-suggestions.tt @@ -309,7 +309,8 @@ [% IF ( deleted ) %]
    The selected suggestions have been deleted.
    [% END %] - [% IF suggestions_loop OR title_filter %] + [% IF suggestions.size > 0 OR title_filter %] + [% SET can_delete_suggestion = 0 %]
    @@ -336,7 +337,7 @@
    [% END %] - [% IF suggestions_loop %] + [% IF suggestions.size > 0 %] [% SET can_delete_suggestion = 0 %]
    @@ -380,74 +381,73 @@ - [% FOREACH suggestions_loo IN suggestions_loop %] + [% FOREACH suggestion IN suggestions %] - [% IF ( loggedinusername ) %] + [% IF logged_in_user %] - [% IF ( suggestions_loo.showcheckbox ) %] + [% IF logged_in_user.borrowernumber == suggestion.suggester.borrowernumber %] [% SET can_delete_suggestion = 1 %] - + [% END %] [% END %]

    -

    [% IF ( suggestions_loo.author ) %][% suggestions_loo.author | html %],[% END %] - [% IF ( suggestions_loo.copyrightdate ) %] - [% suggestions_loo.copyrightdate | html %],[% END %] - [% IF ( suggestions_loo.publishercode ) %] - [% suggestions_loo.publishercode | html %][% END %] - [% IF ( suggestions_loo.place ) %]([% suggestions_loo.place | html %])[% END %] - [% IF ( suggestions_loo.collectiontitle ) %] , [% suggestions_loo.collectiontitle | html %][% END %] - [% IF ( suggestions_loo.itemtype ) %] - [% AuthorisedValues.GetByCode( 'SUGGEST_FORMAT', suggestions_loo.itemtype, 1 ) | html %][% END %] +

    [% IF ( suggestion.author ) %][% suggestion.author | html %],[% END %] + [% IF ( suggestion.copyrightdate ) %] - [% suggestion.copyrightdate | html %],[% END %] + [% IF ( suggestion.publishercode ) %] - [% suggestion.publishercode | html %][% END %] + [% IF ( suggestion.place ) %]([% suggestion.place | html %])[% END %] + [% IF ( suggestion.collectiontitle ) %] , [% suggestion.collectiontitle | html %][% END %] + [% IF ( suggestion.itemtype ) %] - [% AuthorisedValues.GetByCode( 'SUGGEST_FORMAT', suggestion.itemtype, 1 ) | html %][% END %]

    - [% IF ( suggestions_loo.suggesteddate ) %][% suggestions_loo.suggesteddate |$KohaDates %][% END %] + [% IF ( suggestion.suggesteddate ) %][% suggestion.suggesteddate |$KohaDates %][% END %] - [% IF ( suggestions_loo.note ) %] + [% IF ( suggestion.note ) %] Note: - [% suggestions_loo.note | html %] + [% suggestion.note | html %] [% END %] [% IF Koha.Preference( 'OPACViewOthersSuggestions' ) == 1 %] - [% IF ( suggestions_loo.branchcodesuggestedby ) %] + [% IF suggestion.suggestedby %] Suggested for: - [% suggestions_loo.branchcodesuggestedby | html %] + [% Branches.GetName(suggestion.suggester.branchcode) | html %] [% END %] [% END %] [% IF Koha.Preference( 'OpacSuggestionManagedBy' ) %] - [% IF ( suggestions_loo.surnamemanagedby ) %] + [% IF suggestion.managedby %] Managed by: - [% suggestions_loo.surnamemanagedby | html %] - [% IF ( suggestions_loo.firstnamemanagedby ) %] , [% suggestions_loo.firstnamemanagedby | html %] - [% END %] + [% INCLUDE 'patron-title.inc' patron = suggestion.manager %] + [% IF ( suggestion.manageddate ) %] - [% suggestion.manageddate | $KohaDates %][% END %] [% END %] [% END %] Status: - [% IF ( suggestions_loo.ASKED ) %]Requested - [% ELSIF ( suggestions_loo.CHECKED ) %]Checked by the library - [% ELSIF ( suggestions_loo.ACCEPTED ) %]Accepted by the library - [% ELSIF ( suggestions_loo.ORDERED ) %]Ordered by the library - [% ELSIF ( suggestions_loo.REJECTED ) %]Suggestion declined - [% ELSIF ( suggestions_loo.AVAILABLE ) %]Available in the library - [% ELSE %] [% AuthorisedValues.GetByCode( 'SUGGEST_STATUS', suggestions_loo.STATUS, 1 ) | html %] [% END %] - [% IF ( suggestions_loo.reason ) %]([% suggestions_loo.reason | html %])[% END %] + [% IF ( suggestion.STATUS == 'ASKED' ) %]Requested + [% ELSIF ( suggestion.STATUS == 'CHECKED' ) %]Checked by the library + [% ELSIF ( suggestion.STATUS == 'ACCEPTED' ) %]Accepted by the library + [% ELSIF ( suggestion.STATUS == 'ORDERED' ) %]Ordered by the library + [% ELSIF ( suggestion.STATUS == 'REJECTED' ) %]Suggestion declined + [% ELSIF ( suggestion.STATUS == 'AVAILABLE' ) %]Available in the library + [% ELSE %] [% AuthorisedValues.GetByCode( 'SUGGEST_STATUS', suggestion.STATUS, 1 ) | html %] [% END %] + [% IF ( suggestion.reason ) %]([% suggestion.reason | html %])[% END %] - [% END # / FOREACH suggestions_loo %] + [% END # / FOREACH suggestions %] @@ -486,7 +486,7 @@

    New purchase suggestion

    [% END %] [% END %] - [% END # / IF suggestions_loop %] + [% END # / IF suggestions.size %] [% END # IF op_else %] diff --git a/members/deletemem.pl b/members/deletemem.pl index 5faf87dc47..31d930cc00 100755 --- a/members/deletemem.pl +++ b/members/deletemem.pl @@ -30,10 +30,10 @@ use Try::Tiny qw( catch try ); use C4::Context; use C4::Output qw( output_and_exit_if_error output_and_exit output_html_with_http_headers ); use C4::Auth qw( get_template_and_user ); -use C4::Suggestions; use Koha::Patrons; use Koha::Token; use Koha::Patron::Categories; +use Koha::Suggestions; my $input = CGI->new; @@ -99,11 +99,7 @@ my $countholds = $dbh->selectrow_array("SELECT COUNT(*) FROM reserves WHERE borr # Add warning if patron has pending suggestions $template->param( - pending_suggestions => scalar @{ - C4::Suggestions::SearchSuggestion( - { suggestedby => $member, STATUS => 'ASKED' } - ) - } + pending_suggestions => Koha::Suggestions->search({ suggestedby => $member, STATUS => 'ASKED' })->count, ); $template->param( diff --git a/members/purchase-suggestions.pl b/members/purchase-suggestions.pl index 6a9725115e..1739e841ae 100755 --- a/members/purchase-suggestions.pl +++ b/members/purchase-suggestions.pl @@ -23,9 +23,8 @@ use CGI qw ( -utf8 ); use C4::Auth qw( get_template_and_user ); use C4::Context; use C4::Output qw( output_and_exit_if_error output_and_exit output_html_with_http_headers ); -use C4::Members; -use C4::Suggestions qw( SearchSuggestion ); use Koha::Patrons; +use Koha::Suggestions; my $input = CGI->new; @@ -49,7 +48,10 @@ $template->param( suggestionsview => 1, ); -my $suggestions = SearchSuggestion( { suggestedby => $borrowernumber } ); +my $suggestions = [ + Koha::Suggestions->search_limited( { suggestedby => $borrowernumber }, + { prefetch => 'managedby' } )->as_list +]; $template->param( suggestions => $suggestions ); diff --git a/opac/opac-suggestions.pl b/opac/opac-suggestions.pl index fcc95801aa..b2ab7609bc 100755 --- a/opac/opac-suggestions.pl +++ b/opac/opac-suggestions.pl @@ -28,7 +28,6 @@ use C4::Suggestions qw( DelSuggestion MarcRecordFromNewSuggestion NewSuggestion - SearchSuggestion ); use C4::Koha qw( GetAuthorisedValues ); use C4::Scrubber; @@ -100,36 +99,43 @@ else { ); } +my $suggested_by; if ( $op eq 'else' ) { if ( C4::Context->preference("OPACViewOthersSuggestions") ) { if ( $borrowernumber ) { # A logged in user is able to see suggestions from others - $suggestion->{suggestedby} = $suggested_by_anyone + $suggested_by = $suggested_by_anyone ? undef : $borrowernumber; } - else { - # Non logged in user is able to see all suggestions - $suggestion->{suggestedby} = undef; - } + # else: Non logged in user is able to see all suggestions } else { if ( $borrowernumber ) { - $suggestion->{suggestedby} = $borrowernumber; + $suggested_by = $borrowernumber; } else { - $suggestion->{suggestedby} = -1; + $suggested_by = -1; } } } else { if ( $borrowernumber ) { - $suggestion->{suggestedby} = $borrowernumber; + $suggested_by = $borrowernumber; } else { - $suggestion->{suggestedby} = C4::Context->preference("AnonymousPatron"); + $suggested_by = C4::Context->preference("AnonymousPatron"); } } +$suggestion = { + map { + my $p = $suggestion->{$_}; + # Keep parameters that are not an empty string + ( defined $p && $p ne '' ? ( $_ => $p ) : () ) + } keys %$suggestion +}; +$suggestion->{suggestedby} = $borrowernumber; + if ( $op eq "add_validate" && not $biblionumber ) { # If we are creating the suggestion from an existing record we do not want to search for duplicates $op = 'add_confirm'; my $biblio = MarcRecordFromNewSuggestion($suggestion); @@ -155,7 +161,7 @@ if ( $borrowernumber ){ } if ( $op eq "add_confirm" ) { - my $suggestions_loop = &SearchSuggestion($suggestion); + my $suggestions = Koha::Suggestions->search($suggestion); if ( C4::Context->preference("MaxTotalSuggestions") ne '' && $patrons_total_suggestions_count >= C4::Context->preference("MaxTotalSuggestions") ) { push @messages, { type => 'error', code => 'total_suggestions' }; @@ -164,15 +170,15 @@ if ( $op eq "add_confirm" ) { { push @messages, { type => 'error', code => 'too_many' }; } - elsif ( @$suggestions_loop >= 1 ) { + elsif ( $suggestions->count >= 1 ) { #some suggestion are answering the request Donot Add - for my $s (@$suggestions_loop) { + while ( my $suggestion = $suggestions->next ) { push @messages, { type => 'error', code => 'already_exists', - id => $s->{suggestionid} + id => $suggestion->suggestionid }; last; } @@ -197,24 +203,23 @@ if ( $op eq "add_confirm" ) { $patrons_pending_suggestions_count++; $patrons_total_suggestions_count++; - # delete empty fields, to avoid filter in "SearchSuggestion" - foreach my $field ( qw( title author publishercode copyrightdate place collectiontitle isbn STATUS ) ) { - delete $suggestion->{$field}; #clear search filters (except borrower related) to show all suggestions after placing a new one - } - $suggestions_loop = &SearchSuggestion($suggestion); - push @messages, { type => 'info', code => 'success_on_inserted' }; } $op = 'else'; } -my $suggestions_loop = &SearchSuggestion( +my $suggestions = [ Koha::Suggestions->search_limited( { - suggestedby => $suggestion->{suggestedby}, - title => $title_filter, + $suggestion->{suggestedby} + ? ( suggestedby => $suggestion->{suggestedby} ) + : (), + $title_filter + ? ( title => $title_filter ) + : (), } -); +)->as_list ]; + if ( $op eq "delete_confirm" ) { my @delete_field = $input->multi_param("delete_field"); foreach my $delete_field (@delete_field) { @@ -225,24 +230,6 @@ if ( $op eq "delete_confirm" ) { exit; } -map{ - my $s = $_; - my $library = Koha::Libraries->find($s->{branchcodesuggestedby}); - $library ? $s->{branchcodesuggestedby} = $library->branchname : () -} @$suggestions_loop; - -foreach my $suggestion(@$suggestions_loop) { - if($suggestion->{'suggestedby'} == $borrowernumber) { - $suggestion->{'showcheckbox'} = $borrowernumber; - } else { - $suggestion->{'showcheckbox'} = 0; - } - if($suggestion->{'patronreason'}){ - my $av = Koha::AuthorisedValues->search({ category => 'OPAC_SUG', authorised_value => $suggestion->{patronreason} }); - $suggestion->{'patronreason'} = $av->count ? $av->next->opac_description : ''; - } -} - my $patron_reason_loop = GetAuthorisedValues("OPAC_SUG", "opac"); my @mandatoryfields; @@ -279,7 +266,7 @@ my @unwantedfields; $template->param( %$suggestion, - suggestions_loop => $suggestions_loop, + suggestions => $suggestions, patron_reason_loop => $patron_reason_loop, "op_$op" => 1, $op => 1, diff --git a/suggestion/suggestion.pl b/suggestion/suggestion.pl index 6bea518b0e..c38367256f 100755 --- a/suggestion/suggestion.pl +++ b/suggestion/suggestion.pl @@ -92,7 +92,7 @@ my $displayby = $input->param('displayby') || ''; my $tabcode = $input->param('tabcode'); my $save_confirmed = $input->param('save_confirmed') || 0; my $notify = $input->param('notify'); -my $filter_archived = $input->param('filter_archived'); +my $filter_archived = $input->param('filter_archived') || 0; my $reasonsloop = GetAuthorisedValues("SUGGEST"); @@ -109,6 +109,12 @@ delete $$suggestion_ref{$_} foreach qw( suggestedbyme op displayby tabcode notif foreach (keys %$suggestion_ref){ delete $$suggestion_ref{$_} if (!$$suggestion_ref{$_} && ($op eq 'else' )); } +delete $suggestion_only->{branchcode} if $suggestion_only->{branchcode} eq '__ANY__'; +delete $suggestion_only->{budgetid} if $suggestion_only->{budgetid} eq '__ANY__'; +while ( my ( $k, $v ) = each %$suggestion_only ) { + delete $suggestion_only->{$k} if $v eq ''; +} + my ( $template, $borrowernumber, $cookie, $userflags ) = get_template_and_user( { template_name => "suggestion/suggestion.tt", @@ -214,13 +220,12 @@ if ( $op =~ /save/i ) { } } else { ###FIXME:Search here if suggestion already exists. - my $suggestions_loop = - SearchSuggestion( $suggestion_only ); - if (@$suggestions_loop>=1){ + my $suggestions= Koha::Suggestions->search_limited( $suggestion_only ); + if ( $suggestions->count ) { #some suggestion are answering the request Donot Add my @messages; - for my $suggestion ( @$suggestions_loop ) { - push @messages, { type => 'error', code => 'already_exists', id => $suggestion->{suggestionid} }; + while ( my $suggestion = $suggestions->next ) { + push @messages, { type => 'error', code => 'already_exists', id => $suggestion->suggestionid }; } $template->param( messages => \@messages ); } @@ -363,31 +368,65 @@ if ($op=~/else/) { unshift @criteria_dv, 'ASKED'; } + unless ( exists $suggestion_ref->{branchcode} ) { + $suggestion_ref->{branchcode} = C4::Context->userenv->{'branch'}; + } + my @allsuggestions; foreach my $criteriumvalue ( @criteria_dv ) { + my $search_params = {%$suggestion_ref}; # By default, display suggestions from current working branch - unless ( exists $$suggestion_ref{'branchcode'} ) { - $$suggestion_ref{'branchcode'} = C4::Context->userenv->{'branch'}; - } my $definedvalue = defined $$suggestion_ref{$displayby} && $$suggestion_ref{$displayby} ne ""; next if ( $definedvalue && $$suggestion_ref{$displayby} ne $criteriumvalue ) and ($displayby ne 'branchcode' && $branchfilter ne '__ANY__' ); - $$suggestion_ref{$displayby} = $criteriumvalue; - my $suggestions = &SearchSuggestion({ %$suggestion_ref, archived => $filter_archived }); - foreach my $suggestion (@$suggestions) { - if ($suggestion->{budgetid}){ - my $bud = GetBudget( $suggestion->{budgetid} ); - $suggestion->{budget_name} = $bud->{budget_name} if $bud; + $search_params->{$displayby} = $criteriumvalue; + + # filter on date fields + foreach my $field (qw( suggesteddate manageddate accepteddate )) { + my $from = $field . "_from"; + my $to = $field . "_to"; + my $from_dt = + $suggestion_ref->{$from} + ? eval { dt_from_string( $suggestion_ref->{$from} ) } + : undef; + my $to_dt = + $suggestion_ref->{$to} + ? eval { dt_from_string( $suggestion_ref->{$to} ) } + : undef; + + if ( $from_dt || $to_dt ) { + my $dtf = Koha::Database->new->schema->storage->datetime_parser; + if ( $from_dt && $to_dt ) { + $search_params->{$field} = { -between => [ $from_dt, $to_dt ] }; + } elsif ( $from_dt ) { + $search_params->{$field} = { '>=' => $from_dt }; + } elsif ( $to_dt ) { + $search_params->{$field} = { '<=' => $to_dt }; + } } } - push @allsuggestions,{ - "suggestiontype"=>$criteriumvalue||"suggest", - "suggestiontypelabel"=>GetCriteriumDesc($criteriumvalue,$displayby)||"", - "suggestionscount"=>scalar(@$suggestions), - 'suggestions_loop'=>$suggestions, - 'reasonsloop' => $reasonsloop, - } if @$suggestions; + if ( $search_params->{budgetid} && $search_params->{budgetid} eq '__NONE__' ) { + $search_params->{budgetid} = [undef, '' ]; + } + for my $f (qw (branchcode budgetid)) { + delete $search_params->{$f} + if $search_params->{$f} eq '__ANY__' + || $search_params->{$f} eq ''; + } + + my @suggestions = + Koha::Suggestions->search_limited( + { %$search_params, archived => $filter_archived } )->as_list; + + push @allsuggestions, + { + "suggestiontype" => $criteriumvalue || "suggest", + "suggestiontypelabel" => GetCriteriumDesc( $criteriumvalue, $displayby ) || "", + 'suggestions' => \@suggestions, + 'reasonsloop' => $reasonsloop, + } + if scalar @suggestions > 0; delete $$suggestion_ref{$displayby} unless $definedvalue; } diff --git a/t/db_dependent/Suggestions.t b/t/db_dependent/Suggestions.t index c48b2d5e00..481dfaa3d7 100755 --- a/t/db_dependent/Suggestions.t +++ b/t/db_dependent/Suggestions.t @@ -18,7 +18,7 @@ use Modern::Perl; use DateTime::Duration; -use Test::More tests => 107; +use Test::More tests => 91; use Test::Warn; use t::lib::Mocks; @@ -34,7 +34,7 @@ use Koha::Patrons; use Koha::Suggestions; BEGIN { - use_ok('C4::Suggestions', qw( NewSuggestion GetSuggestion ModSuggestion GetSuggestionInfo GetSuggestionFromBiblionumber GetSuggestionInfoFromBiblionumber GetSuggestionByStatus ConnectSuggestionAndBiblio SearchSuggestion DelSuggestion MarcRecordFromNewSuggestion GetUnprocessedSuggestions DelSuggestionsOlderThan )); + use_ok('C4::Suggestions', qw( NewSuggestion GetSuggestion ModSuggestion GetSuggestionInfo GetSuggestionFromBiblionumber GetSuggestionInfoFromBiblionumber GetSuggestionByStatus ConnectSuggestionAndBiblio DelSuggestion MarcRecordFromNewSuggestion GetUnprocessedSuggestions DelSuggestionsOlderThan )); } my $schema = Koha::Database->new->schema; @@ -329,73 +329,6 @@ is( $connect_suggestion_and_biblio, '1', 'ConnectSuggestionAndBiblio returns 1' $suggestion = GetSuggestion($my_suggestionid); is( $suggestion->{biblionumber}, $biblio_2->biblionumber, 'ConnectSuggestionAndBiblio updates the biblio number correctly' ); -my $search_suggestion = SearchSuggestion(); -is( @$search_suggestion, 3, 'SearchSuggestion without arguments returns all suggestions' ); - -$search_suggestion = SearchSuggestion({ - title => $mod_suggestion1->{title}, -}); -is( @$search_suggestion, 1, 'SearchSuggestion returns the correct number of suggestions' ); -$search_suggestion = SearchSuggestion({ - title => 'another title', -}); -is( @$search_suggestion, 0, 'SearchSuggestion returns the correct number of suggestions' ); - -$search_suggestion = SearchSuggestion({ - author => $mod_suggestion1->{author}, -}); -is( @$search_suggestion, 1, 'SearchSuggestion returns the correct number of suggestions' ); -$search_suggestion = SearchSuggestion({ - author => 'another author', -}); -is( @$search_suggestion, 0, 'SearchSuggestion returns the correct number of suggestions' ); - -$search_suggestion = SearchSuggestion({ - publishercode => $mod_suggestion1->{publishercode}, -}); -is( @$search_suggestion, 1, 'SearchSuggestion returns the correct number of suggestions' ); -$search_suggestion = SearchSuggestion({ - publishercode => 'another publishercode', -}); -is( @$search_suggestion, 0, 'SearchSuggestion returns the correct number of suggestions' ); - -$search_suggestion = SearchSuggestion({ - STATUS => $mod_suggestion3->{STATUS}, -}); -is( @$search_suggestion, 2, 'SearchSuggestion returns the correct number of suggestions' ); - -$search_suggestion = SearchSuggestion({ - STATUS => q|| -}); -is( @$search_suggestion, 0, 'SearchSuggestion should not return all suggestions if we want the suggestions with a STATUS=""' ); -$search_suggestion = SearchSuggestion({ - STATUS => 'REJECTED', -}); -is( @$search_suggestion, 0, 'SearchSuggestion returns the correct number of suggestions' ); - -$search_suggestion = SearchSuggestion({ - budgetid => '', -}); -is( @$search_suggestion, 3, 'SearchSuggestion (budgetid = "") returns the correct number of suggestions' ); -$search_suggestion = SearchSuggestion({ - budgetid => $budget_id, -}); -is( @$search_suggestion, 2, 'SearchSuggestion (budgetid = $budgetid) returns the correct number of suggestions' ); -$search_suggestion = SearchSuggestion({ - budgetid => '__NONE__', -}); -is( @$search_suggestion, 1, 'SearchSuggestion (budgetid = "__NONE__") returns the correct number of suggestions' ); -$search_suggestion = SearchSuggestion({ - budgetid => '__ANY__', -}); -is( @$search_suggestion, 3, 'SearchSuggestion (budgetid = "__ANY__") returns the correct number of suggestions' ); - -$search_suggestion = SearchSuggestion({ budgetid => $budget_id }); -is( @$search_suggestion[0]->{budget_name}, GetBudget($budget_id)->{budget_name}, 'SearchSuggestion returns the correct budget name'); -$search_suggestion = SearchSuggestion({ budgetid => "__NONE__" }); -is( @$search_suggestion[0]->{budget_name}, undef, 'SearchSuggestion returns the correct budget name'); - - my $del_suggestion = { title => 'my deleted title', STATUS => 'CHECKED', -- 2.39.5