From 7b9b566ccd53bfed9f312d18a6a32f8afb8e63e4 Mon Sep 17 00:00:00 2001 From: Julian Maurice Date: Wed, 18 Nov 2015 15:52:14 +0100 Subject: [PATCH] Bug 15219: Add pagination on checkouts tables Some libraries have patrons with more than 1000 checkouts. Loading all of them at once can be very long, and sometimes can cause timeout errors. This patch prevent that by enabling pagination on checkouts tables. This affects patron's checkouts and patron's relatives' checkouts tables on pages circ/circulation.pl and members/moremember.pl. As pagination can be useless and cumbersome with small sets of data, a new system preference is introduced to control this behaviour (pagination is disabled by default). Additionally, this patch adds a switch to turn off and on row grouping (today's checkouts vs previous checkouts) (on by default) Test plan: 1. Find (or create) a patron that have at least 10 checkouts and where the relatives checkouts table contain at least 10 checkouts. 2. Go to the patron's detail page and check everything works fine in both tables (sorting, pagination, the data itself, ...) 3. Do the same on circulation page (circulation.pl) 4. Enable system preference ServerSideCheckoutsTable 5. Repeat steps 2 and 3 --- admin/columns_settings.yml | 60 ----- .../data/mysql/atomicupdate/bug-15219.pl | 15 ++ installer/data/mysql/mandatory/sysprefs.sql | 3 +- .../en/includes/checkouts-table-footer.inc | 9 +- .../prog/en/includes/checkouts-table.inc | 10 +- .../en/includes/relatives-issues-table.inc | 2 - .../admin/preferences/circulation.pref | 6 + .../prog/en/modules/circ/circulation.tt | 1 + .../prog/en/modules/members/moremember.tt | 1 + koha-tmpl/intranet-tmpl/prog/js/checkouts.js | 207 +++++++++++------- svc/checkouts | 138 +++++++----- 11 files changed, 240 insertions(+), 212 deletions(-) create mode 100644 installer/data/mysql/atomicupdate/bug-15219.pl diff --git a/admin/columns_settings.yml b/admin/columns_settings.yml index 7b9e4e7ec87..20227c30855 100644 --- a/admin/columns_settings.yml +++ b/admin/columns_settings.yml @@ -1313,21 +1313,6 @@ modules: moremember: issues-table: columns: - - - columnname: sort_order - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - - - columnname: todays_or_previous_checkouts - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - - - columnname: due_date_unformatted - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - columnname: due_date - @@ -1342,11 +1327,6 @@ modules: columnname: location - columnname: homebranch - - - columnname: checkout_on_unformatted - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - columnname: checkout_on - @@ -1431,11 +1411,6 @@ modules: relatives-issues-table: columns: - - - columnname: due_date_unformatted - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - columnname: due_date - @@ -1448,11 +1423,6 @@ modules: columnname: collection - columnname: location - - - columnname: checkout_on_unformatted - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - columnname: checkout_on - @@ -1687,21 +1657,6 @@ modules: circulation: issues-table: columns: - - - columnname: sort_order - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - - - columnname: todays_or_previous_checkouts - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - - - columnname: due_date_unformatted - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - columnname: due_date - @@ -1716,11 +1671,6 @@ modules: columnname: location - columnname: homebranch - - - columnname: checkout_on_unformatted - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - columnname: checkout_on - @@ -1750,11 +1700,6 @@ modules: relatives-issues-table: columns: - - - columnname: due_date_unformatted - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - columnname: due_date - @@ -1767,11 +1712,6 @@ modules: columnname: collection - columnname: location - - - columnname: checkout_on_unformatted - cannot_be_toggled: 1 - cannot_be_modified: 1 - is_hidden: 1 - columnname: checkout_on - diff --git a/installer/data/mysql/atomicupdate/bug-15219.pl b/installer/data/mysql/atomicupdate/bug-15219.pl new file mode 100644 index 00000000000..19d386dda92 --- /dev/null +++ b/installer/data/mysql/atomicupdate/bug-15219.pl @@ -0,0 +1,15 @@ +use Modern::Perl; + +return { + bug_number => "15219", + description => "Add syspref ServerSideCheckoutsTables", + up => sub { + my ($args) = @_; + my ($dbh, $out) = @$args{qw(dbh out)}; + + $dbh->do(q{ + INSERT IGNORE INTO systempreferences (variable,value,explanation,options,type) + VALUES ('ServerSideCheckoutsTables','0',NULL,'If enabled, use the "server-side processing" feature for checkouts tables. Can improve performance when there is a big number of checkouts to display.','YesNo') + }); + }, +}; diff --git a/installer/data/mysql/mandatory/sysprefs.sql b/installer/data/mysql/mandatory/sysprefs.sql index 67cd533dafb..3809cc39331 100644 --- a/installer/data/mysql/mandatory/sysprefs.sql +++ b/installer/data/mysql/mandatory/sysprefs.sql @@ -738,6 +738,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('SerialsDefaultEmailAddress', '', NULL, 'Default email address used as reply-to for notices sent by the serials module.', 'Free'), ('SerialsDefaultReplyTo', '', NULL, 'Default email address that serials notices are sent from.', 'Free'), ('SerialsSearchResultsLimit', '', NULL, 'Serials search results limit', 'integer'), +('ServerSideCheckoutsTables','0',NULL,'If enabled, use the "server-side processing" feature for checkouts tables. Can improve performance when there is a big number of checkouts per patron.','YesNo'), ('SessionRestrictionByIP','1','Check for change in remote IP address for session security. Disable only when remote IP address changes frequently.','','YesNo'), ('SessionStorage','mysql','mysql|Pg|tmp','Use database or a temporary file for storing session data','Choice'), ('ShelfBrowserUsesCcode','1','0','Use the item collection code when finding items for the shelf browser.','YesNo'), @@ -889,4 +890,4 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, ` ('z3950AuthorAuthFields','701,702,700',NULL,'Define the MARC biblio fields for Personal Name Authorities to fill biblio.author','free'), ('z3950NormalizeAuthor','0','','If ON, Personal Name Authorities will replace authors in biblio.author','YesNo'), ('z3950Status','','','This syspref allows to define custom YAML based rules for marking items unavailable in z3950 results.','Textarea') -; \ No newline at end of file +; diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/checkouts-table-footer.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/checkouts-table-footer.inc index 498469ddc86..99edb4f70ae 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/checkouts-table-footer.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/checkouts-table-footer.inc @@ -1,15 +1,10 @@ - Totals: + Totals: - [% IF Koha.Preference('ExportCircHistory') %] - [% SET td_colspan = 4 %] - [% ELSE %] - [% SET td_colspan = 3 %] - [% END %] - +

select all | none

Renew

select all visible rows | none

Check in

select all | none

Check in

select all visible rows | none

Return claims Export

select all | none

Export

select all visible rows | none

diff --git a/koha-tmpl/intranet-tmpl/prog/en/includes/relatives-issues-table.inc b/koha-tmpl/intranet-tmpl/prog/en/includes/relatives-issues-table.inc index f541cacb8aa..b2a3b99b78d 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/includes/relatives-issues-table.inc +++ b/koha-tmpl/intranet-tmpl/prog/en/includes/relatives-issues-table.inc @@ -3,14 +3,12 @@ - - diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index ef3c7acc011..451366c1d73 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -57,6 +57,12 @@ Circulation: asc: earliest to latest desc: latest to earliest - checkout time. + - + - pref: ServerSideCheckoutsTables + choices: + 1: Enable + 0: Do not enable + - server-side processing on checkouts tables. It can improve performance when there is a big number of checkouts to display. - - pref: SpecifyDueDate choices: 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 1ca767f7295..4823df7db16 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/circ/circulation.tt @@ -1107,6 +1107,7 @@ relatives_borrowernumbers.push("[% b | html %]"); [% END %] var SuspendHoldsIntranet = [% ( Koha.Preference('SuspendHoldsIntranet') ) ? 1 : 0 | html %]; + var ServerSideCheckoutsTables = [% Koha.Preference('ServerSideCheckoutsTables') ? 'true' : 'false' | $raw %]; [% Asset.js("js/pages/circulation.js") | $raw %] [% IF Koha.Preference('ClaimReturnedLostValue') || Koha.Preference('BundleLostValue') %] 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 cdb10f8e48b..0fdc6f79547 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/members/moremember.tt @@ -783,6 +783,7 @@ relatives_borrowernumbers.push("[% b | html %]"); [% END %] var SuspendHoldsIntranet = [% ( Koha.Preference('SuspendHoldsIntranet') ) ? 1 : 0 | html %]; + var ServerSideCheckoutsTables = [% Koha.Preference('ServerSideCheckoutsTables') ? 'true' : 'false' | $raw %]; [% Asset.js("js/pages/circulation.js") | $raw %] [% IF Koha.Preference('ClaimReturnedLostValue') || Koha.Preference('BundleLostValue') %] diff --git a/koha-tmpl/intranet-tmpl/prog/js/checkouts.js b/koha-tmpl/intranet-tmpl/prog/js/checkouts.js index 185a5488db5..737d1157c65 100644 --- a/koha-tmpl/intranet-tmpl/prog/js/checkouts.js +++ b/koha-tmpl/intranet-tmpl/prog/js/checkouts.js @@ -1,4 +1,4 @@ -/* global __ */ +/* global __, ServerSideCheckoutsTables: false */ function CheckRenewCheckinBoxes() { $("#RenewChecked").prop("disabled", !$(".renew:checked").length); @@ -61,37 +61,11 @@ function LoadIssuesTable() { processing: msg_loading, }, autoWidth: false, - dom: '<"table_controls"B>rt', + order: [], columns: [ - { - data: function (oObj) { - return oObj.sort_order; - }, - }, - { - data: function (oObj) { - if (oObj.issued_today) { - return ( - "" + - __("Today's checkouts") + - "" - ); - } else { - return ( - "" + - __("Previous checkouts") + - "" - ); - } - }, - }, { data: "date_due", - visible: false, - }, - { - orderData: 2, // Sort on hidden unformatted date due column - data: function (oObj) { + render: function (data, type, oObj) { let date_due_formatted = $datetime(oObj.date_due, { as_due_date: true, no_tz_adjust: true, @@ -142,7 +116,8 @@ function LoadIssuesTable() { }, }, { - data: function (oObj) { + data: "title", + render: function (data, type, oObj) { let title = "' + @@ -320,7 +300,8 @@ function LoadIssuesTable() { className: "nowrap", }, { - data: function (oObj) { + data: "fine", + render: function (data, type, oObj) { if (!oObj.fine) oObj.fine = 0; return ( '' + @@ -331,7 +312,8 @@ function LoadIssuesTable() { className: "nowrap", }, { - data: function (oObj) { + data: "price", + render: function (data, type, oObj) { if (!oObj.price) oObj.price = 0; return ( '' + @@ -343,7 +325,7 @@ function LoadIssuesTable() { }, { orderable: false, - data: function (oObj) { + render: function (data, type, oObj) { var content = ""; var msg = ""; var span_style = ""; @@ -555,7 +537,7 @@ function LoadIssuesTable() { }, { orderable: false, - data: function (oObj) { + render: function (data, type, oObj) { if (oObj.can_renew_error == "recalled") { return ( "') + .attr("id", "issues-table-row-grouping-switch") + .attr("checked", "checked") + .css("vertical-align", "middle") + .change(function () { + if (this.checked) { + issuesTable.api().rowGroup().enable(); + issuesTable.api().draw(); + } else { + issuesTable.api().rowGroup().disable(); + issuesTable.api().draw(); + } + }); + $("
") + .css("float", "left") + .css("padding", "0.3em 0.5em") + .css("line-height", "1.9em") + .append(groupingSwitch) + .append(" ") + .append( + '" + ) + .appendTo("#issues-table_wrapper .top.pager"); + if ($("#issues-table").length) { $("#issues-table_processing").position({ of: $("#issues-table"), @@ -1091,16 +1113,11 @@ $(document).ready(function () { relativesIssuesTable = $("#relatives-issues-table").kohaTable( { autoWidth: false, - dom: '<"table_controls"B>rt', order: [], columns: [ { data: "date_due", - visible: false, - }, - { - orderData: 0, // Sort on hidden unformatted date due column - data: function (oObj) { + render: function (data, type, oObj) { var today = new Date(); var due = new Date(oObj.date_due); let date_due_formatted = $datetime( @@ -1119,7 +1136,8 @@ $(document).ready(function () { }, }, { - data: function (oObj) { + data: "title", + render: function (data, type, oObj) { let title = "
"borrowernumber=%s".format(b)) - .join("&") - ), + url: "/cgi-bin/koha/svc/checkouts", + data(data) { + return Object.assign({}, data, { + borrowernumber: relatives_borrowernumbers, + date_due_sorton: "issues.date_due", + title_sorton: "biblio.title", + recordtype_sorton: "biblioitems.itemtype", + itemtype_sorton: "items.itype", + collection_sorton: "items.ccode", + location_sorton: "items.location", + homebranch_sorton: "items.homebranch", + issuedate_sorton: "issues.issuedate", + branchcode_sorton: "issues.branchcode", + itemcallnumber_sorton: "items.itemcallnumber", + copynumber_sorton: "items.copynumber", + }); + }, }, bKohaAjaxSVC: true, }, diff --git a/svc/checkouts b/svc/checkouts index a8c92354d7d..6134579168d 100755 --- a/svc/checkouts +++ b/svc/checkouts @@ -48,26 +48,66 @@ unless ( haspermission( $userid, { circulate => 'circulate_remaining_permissions exit 0; } -my @sort_columns = qw/date_due title itype issuedate branchcode itemcallnumber/; +my $dbh = C4::Context->dbh(); -my @borrowernumber = $input->multi_param('borrowernumber'); -my $offset = $input->param('iDisplayStart'); -my $results_per_page = $input->param('iDisplayLength') || -1; +my @borrowernumbers; +my $borrowernumber = $input->param('borrowernumber'); +if ($borrowernumber) { + push @borrowernumbers, $borrowernumber; +} else { + @borrowernumbers = $input->multi_param('borrowernumber[]'); +} +my $offset = $input->param('start'); +my $count = $input->param('length'); -my $sorting_column = $input->param('iSortCol_0') || q{}; -$sorting_column = ( $sorting_column && $sort_columns[$sorting_column] ) ? $sort_columns[$sorting_column] : 'issuedate'; +my %vars = $input->Vars(); +my $i = 0; +my @orderbys; +while (exists $vars{"order[$i][column]"}) { + my $iSortCol = $vars{"order[$i][column]"}; + my $sSortDir = $vars{"order[$i][dir]"}; + my $mDataProp = $vars{"columns[$iSortCol][data]"}; + my @sort_fields = $vars{$mDataProp.'_sorton'} + ? split(' ', $vars{$mDataProp.'_sorton'}) + : (); + if(@sort_fields > 0) { + push @orderbys, "$_ $sSortDir" foreach (@sort_fields); + } else { + push @orderbys, "$mDataProp $sSortDir"; + } + $i++; +} +@orderbys = grep { /^\w+\.\w+ (asc|desc)$/ } @orderbys; +my @sanitized_orderbys; +for my $orderby (@orderbys) { + my ($identifier, $direction) = split / /, $orderby, 2; + my ($table, $column) = split /\./, $identifier, 2; + my $sanitized_identifier = $dbh->quote_identifier(undef, $table, $column); + my $sanitized_direction = $direction eq 'asc' ? 'ASC' : 'DESC'; + push @sanitized_orderbys, "$sanitized_identifier $sanitized_direction"; +} +my $orderby = @sanitized_orderbys ? " ORDER BY " . join(',', @sanitized_orderbys) . " " : ''; -my $sorting_direction = $input->param('sSortDir_0') || q{}; -$sorting_direction = $sorting_direction eq 'asc' ? 'asc' : 'desc'; +my $todaysIssuesDefaultSortOrder = + C4::Context->preference('todaysIssuesDefaultSortOrder'); +my $previousIssuesDefaultSortOrder = + C4::Context->preference('previousIssuesDefaultSortOrder'); -$results_per_page = undef if ( $results_per_page == -1 ); - -binmode STDOUT, ":encoding(UTF-8)"; -print $input->header( -type => 'application/json', -charset => 'UTF-8' ); +# Default sort when user hasn't clicked on a column header yet. +# When row grouping is enabled, DataTables ask to sort on issued_today column, +# so we need to treat this case the same as the "no sorting" case +if (!$orderby || $orderby =~ m/ORDER BY issued_today desc\s*$/) { + $orderby = qq{ + ORDER BY + issued_today DESC, + CASE WHEN issued_today = 0 THEN date_due END $previousIssuesDefaultSortOrder, + CASE WHEN issued_today != 0 THEN date_due END $todaysIssuesDefaultSortOrder + }; +} my @parameters; my $sql = ' - SELECT + SELECT SQL_CALC_FOUND_ROWS issues.issue_id, issues.issuedate, issues.date_due, @@ -114,13 +154,12 @@ my $sql = ' items.enumchron, items.materials, - DATEDIFF( issues.issuedate, CURRENT_DATE() ) AS not_issued_today, + DATEDIFF( issues.issuedate, CURRENT_DATE() ) = 0 AS issued_today, return_claims.id AS return_claim_id, return_claims.notes AS return_claim_notes, return_claims.created_on AS return_claim_created_on, return_claims.updated_on AS return_claim_updated_on - FROM issues LEFT JOIN items USING ( itemnumber ) LEFT JOIN biblio USING ( biblionumber ) @@ -129,21 +168,29 @@ my $sql = ' LEFT JOIN branches ON ( issues.branchcode = branches.branchcode ) LEFT JOIN branches branches2 ON ( items.homebranch = branches2.branchcode ) LEFT JOIN return_claims USING ( issue_id ) - WHERE issues.borrowernumber + WHERE '; -if ( @borrowernumber == 1 ) { - $sql .= '= ?'; +my $borrowernumber_sql = 'issues.borrowernumber'; +if ( @borrowernumbers == 1 ) { + $borrowernumber_sql .= ' = ?'; } else { - $sql .= ' IN (' . join( ',', ('?') x @borrowernumber ) . ') '; + $borrowernumber_sql .= ' IN (' . join( ',', ('?') x @borrowernumbers ) . ') '; } -push( @parameters, @borrowernumber ); +$sql .= $borrowernumber_sql; +push( @parameters, @borrowernumbers ); -$sql .= " ORDER BY $sorting_column $sorting_direction "; +$sql .= " $orderby"; + +if ($count && $count > 0) { + $sql .= " LIMIT ?,?"; + push @parameters, $offset, $count; +} -my $dbh = C4::Context->dbh(); my $sth = $dbh->prepare($sql); +warn $sql; $sth->execute(@parameters); +my ($iTotalDisplayRecords) = $dbh->selectrow_array('SELECT FOUND_ROWS()'); my $item_level_itypes = C4::Context->preference('item-level_itypes'); my $claims_returned_lost_value = C4::Context->preference('ClaimReturnedLostValue'); @@ -152,8 +199,7 @@ my $confirm_parts_required = C4::Context->preference("CircConfirmItemParts") my $itemtypes = { map { $_->{itemtype} => $_->{translated_description} } @{ Koha::ItemTypes->search_with_localization->unblessed } }; -my @checkouts_today; -my @checkouts_previous; +my @checkouts; while ( my $c = $sth->fetchrow_hashref() ) { my ($charge) = GetIssuingCharges( $c->{itemnumber}, $c->{borrowernumber} ); my $fine = GetFine( $c->{itemnumber}, $c->{borrowernumber} ); @@ -251,7 +297,9 @@ while ( my $c = $sth->fetchrow_hashref() ) { author => $c->{author}, barcode => $c->{barcode}, type_for_stat => $type_for_stat || q{}, + itemtype => $c->{itype} || q{}, itemtype_description => $itemtype || q{}, + recordtype => $c->{itemtype} || q{}, recordtype_description => $recordtype || q{}, collection => $collection, location => $location, @@ -306,36 +354,26 @@ while ( my $c = $sth->fetchrow_hashref() ) { firstname => $c->{firstname}, cardnumber => $c->{cardnumber}, }, - issued_today => !$c->{not_issued_today}, - recalled => $recalled, + issued_today => $c->{issued_today} ? JSON::true : JSON::false, + recalled => $recalled, }; - if ( $c->{not_issued_today} ) { - push( @checkouts_previous, $checkout ); - } else { - push( @checkouts_today, $checkout ); - } + push @checkouts, $checkout; } -@checkouts_today = sort { $a->{timestamp} cmp $b->{timestamp} } @checkouts_today; # latest to earliest -@checkouts_today = reverse(@checkouts_today) - if ( C4::Context->preference('todaysIssuesDefaultSortOrder') eq 'desc' ); # earliest to latest +binmode STDOUT, ":encoding(UTF-8)"; +print $input->header( -type => 'application/json', -charset => 'UTF-8' ); -@checkouts_previous = - sort { $a->{date_due} cmp $b->{date_due} || $a->{timestamp} cmp $b->{timestamp} } - @checkouts_previous; # latest to earliest -@checkouts_previous = reverse(@checkouts_previous) - if ( C4::Context->preference('previousIssuesDefaultSortOrder') eq 'desc' ); # earliest to latest +my $draw = $input->param('draw'); +my ($iTotalRecords) = $dbh->selectrow_array( + "SELECT COUNT(*) FROM issues WHERE $borrowernumber_sql", undef, + @borrowernumbers); -my @checkouts = ( @checkouts_today, @checkouts_previous ); +my $response = { + recordsTotal => $iTotalRecords, + recordsFiltered => $iTotalDisplayRecords, + draw => $draw, + data => \@checkouts, +}; -my $i = 1; -map { $_->{sort_order} = $i++ } @checkouts; - -my $data; -$data->{'iTotalRecords'} = scalar @checkouts; -$data->{'iTotalDisplayRecords'} = scalar @checkouts; -$data->{'sEcho'} = $input->param('sEcho') || undef; -$data->{'aaData'} = \@checkouts; - -print to_json($data); +print to_json($response); -- 2.39.5
Due date (unformatted, hidden) Due date Title Record-level item type Item type Collection LocationChecked out on (hidden, unformatted) Checked out on Checked out from Call number