From 021c80df03b710e7994616a0cf4cb64ded408c7c Mon Sep 17 00:00:00 2001 From: Lebbeous Fogle-Weekley Date: Thu, 25 Apr 2013 15:12:41 -0400 Subject: [PATCH] Browse now goes nicely backwards and forwards page by page. Signed-off-by: Lebbeous Fogle-Weekley --- .../perlmods/lib/OpenILS/WWW/EGCatLoader/Browse.pm | 57 ++++++++++++--- .../sql/Pg/upgrade/YYYY.schema.bib-auth-browse.sql | 80 +++++++++++++++------- Open-ILS/src/templates/opac/browse.tt2 | 53 ++++++++++---- 3 files changed, 143 insertions(+), 47 deletions(-) diff --git a/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Browse.pm b/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Browse.pm index 8a328b1be5..62cccf21f4 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Browse.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Browse.pm @@ -22,22 +22,61 @@ my $U = 'OpenILS::Application::AppUtils'; sub load_browse { my ($self) = @_; + $self->ctx->{more_forward} = 0; + $self->ctx->{more_back} = 0; + if ($self->cgi->param('qtype') and $self->cgi->param('bterm')) { - # Let's get some browse results! + no warnings 'uninitialized'; + + my $limit = int($self->cgi->param('limit') || 10); + my $offset = int($self->cgi->param('offset') || 0); - $self->ctx->{results} = $self->editor->json_query({ + my $browse_results = $self->editor->json_query({ from => [ "metabib.browse", - scalar $self->cgi->param('qtype'), - scalar $self->cgi->param('bterm'), + scalar($self->cgi->param('qtype')), + scalar($self->cgi->param('bterm')), $self->ctx->{copy_location_group_org}, $self->ctx->{copy_location_group}, - scalar $self->cgi->param('limit'), - scalar $self->cgi->param('offset') + ($offset >= 0) ? $limit + 1 : $limit, + $offset # NOTE Yes metabib.browse() allows negative offsets. ] - }) or $self->apache->log->warn( - "error in browse: " . $self->editor->event->{textcode} - ); + }); + + if (not $browse_results) { # DB error, not empty result set. + + # The choice of ->warn instead of ->error for the Apache logs is a + # conscious one. Let's save ->error for more process-ending stuff. + # We're not necessarily crashing in this case. + + $self->apache->log->warn( + "error in browse: " . $self->editor->event->{textcode} + ); + $self->ctx->{browse_error} = 1; + + return Apache2::Const::OK; + } + + while (scalar @$browse_results > $limit) { + pop @$browse_results; + $self->ctx->{more_forward} = 1; + } + + # No, this comparison for setting more_forward does not fold into + # those for setting more_back. + if ($offset < 0) { + $self->ctx->{more_forward} = 1; + } + + if ($offset > 0) { + $self->ctx->{more_back} = 1; + } elsif (scalar @$browse_results < $limit) { + $self->ctx->{more_back} = 0; + } else { + $self->ctx->{more_back} = 1; + } + + $self->ctx->{browse_results} = $browse_results; } return Apache2::Const::OK; diff --git a/Open-ILS/src/sql/Pg/upgrade/YYYY.schema.bib-auth-browse.sql b/Open-ILS/src/sql/Pg/upgrade/YYYY.schema.bib-auth-browse.sql index 53dbe3dc5c..5e6fa8b986 100644 --- a/Open-ILS/src/sql/Pg/upgrade/YYYY.schema.bib-auth-browse.sql +++ b/Open-ILS/src/sql/Pg/upgrade/YYYY.schema.bib-auth-browse.sql @@ -6903,8 +6903,17 @@ UPDATE metabib.browse_entry SET value = value; ALTER TABLE metabib.browse_entry ALTER COLUMN sort_value SET NOT NULL; -CREATE INDEX browse_entry_sort_value_idx - ON metabib.browse_entry USING BTREE (sort_value ASC); +CREATE INDEX CONCURRENTLY browse_entry_sort_value_idx + ON metabib.browse_entry USING BTREE (sort_value); + +-- NOTE If I understand ordered indices correctly, an index on sort_value DESC +-- is not actually needed, even though we do have a query that does ORDER BY +-- on this column in that direction. The previous index serves for both +-- directions, and ordering in an index is only helpful for multi-column +-- indices. + +-- CREATE INDEX CONCURRENTLY browse_entry_sort_value_idx_desc +-- ON metabib.browse_entry USING BTREE (sort_value DESC); CREATE TYPE metabib.slim_browse_entry AS ( browse_entry BIGINT, @@ -6931,7 +6940,7 @@ BEGIN where_clause := ''; IF context_org IS NOT NULL OR context_loc_group IS NOT NULL THEN - -- XXX At some point we surely need more comprehensive/correct + -- XXX TODO At some point we surely need more comprehensive/correct -- holdings testing that just a join on aovc (located URIs, etc). -- And we probably need to support a browse equivalent of search's -- #staff modifier, i.e. scope but don't limit on visibility. @@ -6972,8 +6981,7 @@ DECLARE result TEXT; BEGIN joins_and_where := metabib._browse_joins_and_where( - search_field, browse_term, - context_org, context_loc_group + search_field, browse_term, context_org, context_loc_group ); joins := joins_and_where[1]; @@ -6981,8 +6989,9 @@ BEGIN EXECUTE 'SELECT mbe.sort_value FROM metabib.browse_entry mbe ' || joins || 'WHERE ' || where_clause || - ' mbe.sort_value >= ' || quote_literal(browse_term) || - ' ORDER BY mbe.sort_value LIMIT 1 ' INTO result; + ' mbe.sort_value >= public.search_normalize(' || + quote_literal(browse_term) || + ') ORDER BY mbe.sort_value LIMIT 1 ' INTO result; RETURN result; -- note, can be NULL END; @@ -6994,7 +7003,7 @@ CREATE OR REPLACE FUNCTION metabib.browse( context_org INT DEFAULT NULL, context_loc_group INT DEFAULT NULL, result_limit INT DEFAULT 10, - result_offset INT DEFAULT 0 + result_offset INT DEFAULT 0 -- Can be negative! ) RETURNS SETOF metabib.slim_browse_entry AS $p$ DECLARE joins TEXT; @@ -7004,10 +7013,10 @@ DECLARE pivot_sort_value TEXT; f RECORD; r metabib.slim_browse_entry%ROWTYPE; + use_offset INT; BEGIN pivot_sort_value := metabib.browse_pivot( - search_field, browse_term, - context_org, context_loc_group + search_field, browse_term, context_org, context_loc_group ); IF pivot_sort_value IS NULL THEN @@ -7015,31 +7024,50 @@ BEGIN END IF; joins_and_where := metabib._browse_joins_and_where( - search_field, browse_term, - context_org, context_loc_group + search_field, browse_term, context_org, context_loc_group ); joins := joins_and_where[1]; where_clause := joins_and_where[2]; - -- Now we query for the "page" of records starting at the pivot. If we - -- wanted to put the pivot in the middle, I guess we'd need two queries? - - -- XXX I really don't see the value of finding the pivot point as we could - -- get the exact same results below by just using the browse_term - -- directly where we use pivot_sort_value. If we wanted before and after - -- context, we'd still need two queries (or maybe fancy windowing?) exactly - -- as we would even if we didn't find the pivot first. - result_query := 'SELECT mbe.id, mbe.value, mbedm.authority, mbe.sort_value FROM metabib.browse_entry mbe ' || joins || - 'WHERE ' || where_clause || + 'WHERE ' || where_clause; + + -- PostgreSQL is not magic. We can't actually pass a negative offset. + IF result_offset >= 0 THEN + use_offset := result_offset; + result_query := result_query || ' mbe.sort_value >= ' || quote_literal(pivot_sort_value) || - ' GROUP BY 1,2,3,4 ORDER BY mbe.sort_value ' || - ' LIMIT ' || result_limit || - ' OFFSET ' || result_offset; + ' GROUP BY 1,2,3,4 ORDER BY mbe.sort_value '; + ELSE + -- Step 1 of 2 to deliver what the user wants with a negative offset: + result_query := result_query || + ' mbe.sort_value < ' || quote_literal(pivot_sort_value) || + ' GROUP BY 1,2,3,4 ORDER BY mbe.sort_value DESC '; + + use_offset := ABS(result_offset) - result_limit; + IF use_offset < 0 THEN + -- When result_offset is negative, it is required that + -- result_limit <= ABS(result_offset) + + -- Higher level interfaces should not lead users down + -- this path anyway. + + RAISE EXCEPTION 'Straddling the pivot point is not supported.'; + END IF; + END IF; + + result_query := result_query || + ' LIMIT ' || result_limit || ' OFFSET ' || use_offset; + + IF result_offset < 0 THEN + -- Step 2 of 2 to deliver what the user wants with a negative offset: + result_query := 'SELECT * FROM (' || result_query || + ') x ORDER BY sort_value ASC'; -- Un-reverse the result set. + END IF; FOR f IN EXECUTE result_query LOOP r.browse_entry := f.id; @@ -7057,7 +7085,7 @@ CREATE OR REPLACE FUNCTION metabib.browse( context_org INT DEFAULT NULL, context_loc_group INT DEFAULT NULL, result_limit INT DEFAULT 10, - result_offset INT DEFAULT 0 + result_offset INT DEFAULT 0 -- Can be negative! ) RETURNS SETOF metabib.slim_browse_entry AS $p$ BEGIN RETURN QUERY SELECT * FROM metabib.browse( diff --git a/Open-ILS/src/templates/opac/browse.tt2 b/Open-ILS/src/templates/opac/browse.tt2 index ef03b1279a..04655f9549 100644 --- a/Open-ILS/src/templates/opac/browse.tt2 +++ b/Open-ILS/src/templates/opac/browse.tt2 @@ -5,9 +5,14 @@ PROCESS "opac/parts/org_selector.tt2"; WRAPPER "opac/parts/base.tt2"; INCLUDE "opac/parts/topnav.tt2"; - ctx.page_title = l("Browse the Catalog"); %] + ctx.page_title = l("Browse the Catalog"); + limit = CGI.param('limit') || 10; + offset = CGI.param('offset') || 0; + %] +
- [%# XXX TODO Give searchbar smarts # INCLUDE "opac/parts/searchbar.tt2" %] + [%# XXX TODO Give searchbar smarts so we can just do: + # INCLUDE "opac/parts/searchbar.tt2" %]
[% l('Search the Catalog') %]
- - [% INCLUDE "opac/parts/qtype_selector.tt2" id="browse-search-class" browse_only=1 %] +
+ + + + [% INCLUDE "opac/parts/qtype_selector.tt2" + id="browse-search-class" browse_only=1 %] - + + - - [% INCLUDE build_org_selector id='browse-context' show_loc_groups=1 arialabel=l('Select holding library') %] + + [% INCLUDE build_org_selector id='browse-context' + show_loc_groups=1 + arialabel=l('Select holding library') %] - + +
[% BLOCK browse_pager %]
- ← [%l ('Back') %] + [% IF ctx.more_back %] + ← [%l ('Back') %] + [% END %] 0-9 A @@ -41,15 +60,25 @@ [%# XXX TODO. Make off by default in config.tt2 %] - [%l ('Forward') %] → + [% IF ctx.more_forward %] + [%l ('Forward') %] → + [% END %]
[% END %] [% PROCESS browse_pager %]
- [% FOR result IN browse_results %] - [% result %]
+ [% IF ctx.browse_error %] + + [% l("An error occurred browsing records. " _ + "Please try again in a moment or report the issue " _ + "to library staff.") %] + + [% ELSE %] + [% FOR result IN ctx.browse_results %] + [% result.value | html %]
+ [% END %] [% END %]
-- 2.11.0