From 03152ad836aa15d468d8ecda457d185e03bbfa55 Mon Sep 17 00:00:00 2001 From: Lebbeous Fogle-Weekley Date: Mon, 29 Apr 2013 15:51:18 -0400 Subject: [PATCH] More correct (but slow?) holdings/visibility tests; caching; letter paging links The big VIEWs created in this commit probably need improvement or might even need to be removed (and the bits in metabib._browse_joins_and_where() that use them need would need to be reverted) if they can't be made fast. See VIEWs, generated query, and EXPLAIN ANALYZE output here: http://nox.esilibrary.com/~lebbeous/explain-analyze-vis-view.txt Signed-off-by: Lebbeous Fogle-Weekley --- .../perlmods/lib/OpenILS/WWW/EGCatLoader/Browse.pm | 163 ++++++++++++++------- .../sql/Pg/upgrade/YYYY.schema.bib-auth-browse.sql | 122 +++++++++++++-- Open-ILS/src/templates/opac/browse.tt2 | 12 +- Open-ILS/src/templates/opac/parts/config.tt2 | 9 ++ grand-visibility.sql | 57 ------- 5 files changed, 237 insertions(+), 126 deletions(-) delete mode 100644 grand-visibility.sql 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 62cccf21f4..91888ce135 100644 --- a/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Browse.pm +++ b/Open-ILS/src/perlmods/lib/OpenILS/WWW/EGCatLoader/Browse.pm @@ -8,75 +8,134 @@ use OpenILS::Utils::CStoreEditor qw/:funcs/; use OpenILS::Utils::Fieldmapper; use OpenILS::Application::AppUtils; use OpenSRF::Utils::JSON; +use OpenSRF::Utils::Cache; +use OpenSRF::Utils::SettingsClient; -use Apache2::Const -compile => qw( - OK DECLINED FORBIDDEN - HTTP_INTERNAL_SERVER_ERROR - REDIRECT HTTP_BAD_REQUEST -); +use Digest::MD5 qw/md5_hex/; +use Apache2::Const -compile => qw/OK/; use Data::Dumper; $Data::Dumper::Indent = 0; my $U = 'OpenILS::Application::AppUtils'; +my $browse_cache; +my $browse_timeout; + +sub _init_browse_cache { + if (not defined $browse_cache) { + my $conf = new OpenSRF::Utils::SettingsClient; + + $browse_timeout = $conf->config_value( + "apps", "open-ils.search", "app_settings", "cache_timeout" + ) || 300; + $browse_cache = new OpenSRF::Utils::Cache("global"); + } +} + +# Returns cache key and a list of parameters for DB proc metabib.browse(). +sub prepare_browse_parameters { + my ($self) = @_; + + no warnings 'uninitialized'; + + my $limit = int($self->cgi->param('limit') || 10); + my $offset = int($self->cgi->param('offset') || 0); + + my @params = ( + scalar($self->cgi->param('qtype')), + scalar($self->cgi->param('bterm')), + $self->ctx->{copy_location_group_org} || + $self->ctx->{aou_tree}->()->id, + $self->ctx->{copy_location_group}, + $self->ctx->{is_staff} ? 't' : 'f' + ); + + # We do need $limit and $offset as part of the cache key, but we also + # need to keep them separate from other parameters for purposes of + # paging link generation. + return ( + "oils_browse_" . md5_hex( + OpenSRF::Utils::JSON->perl2JSON([@params, $limit, $offset]) + ), + $limit, $offset, @params + ); +} + +sub load_browse_impl { + my ($self, $limit, $offset, @params) = @_; + + my $results = $self->editor->json_query({ + from => [ + "metabib.browse", ( + @params, ($offset >= 0 ? $limit + 1 : $limit), $offset + ) + ] + }); + + if (not $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; + } + + return $results; +} + +# $results can be modified by this function. +sub deduce_possible_paging { + my ($self, $results, $limit, $offset) = @_; + + while (scalar @$results > $limit) { + pop @$results; + $self->ctx->{more_forward} = 1; + } + + # 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 @$results < $limit) { + $self->ctx->{more_back} = 0; + } else { + $self->ctx->{more_back} = 1; + } +} sub load_browse { my ($self) = @_; + _init_browse_cache(); + $self->ctx->{more_forward} = 0; $self->ctx->{more_back} = 0; if ($self->cgi->param('qtype') and $self->cgi->param('bterm')) { - no warnings 'uninitialized'; - - my $limit = int($self->cgi->param('limit') || 10); - my $offset = int($self->cgi->param('offset') || 0); - - my $browse_results = $self->editor->json_query({ - from => [ - "metabib.browse", - scalar($self->cgi->param('qtype')), - scalar($self->cgi->param('bterm')), - $self->ctx->{copy_location_group_org}, - $self->ctx->{copy_location_group}, - ($offset >= 0) ? $limit + 1 : $limit, - $offset # NOTE Yes metabib.browse() allows negative offsets. - ] - }); - - 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; - } + my ($cache_key, $limit, $offset, @params) = + $self->prepare_browse_parameters; - 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; - } + my $results = $browse_cache->get_cache($cache_key) || + $self->load_browse_impl($limit, $offset, @params); - if ($offset > 0) { - $self->ctx->{more_back} = 1; - } elsif (scalar @$browse_results < $limit) { - $self->ctx->{more_back} = 0; - } else { - $self->ctx->{more_back} = 1; + if ($results) { + $browse_cache->put_cache($cache_key, $results, $browse_timeout); + $self->deduce_possible_paging($results, $limit, $offset); + $self->ctx->{browse_results} = $results; } - $self->ctx->{browse_results} = $browse_results; + # We don't need an else clause to send the user a 5XX error or + # anything. Errors will have been logged, and $ctx will be + # prepared so a template can show a nicer error to the user. } 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 fbbe7bfc9a..daaed50631 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 @@ -6885,6 +6885,96 @@ Revision 1.2 - Added Log Comment 2003/03/24 19:37:42 ckeith -- ---------------------------------------------------- -- XXX TODO From here down, add this to stock SQL files +-- The following view supports checking for visibility for OPAC browse. +CREATE OR REPLACE VIEW asset.visible_holdings AS +SELECT + bre.id AS record, + COALESCE( + aovc.circ_lib, acn_for_uri_map.owning_lib, acp_for_peer_bibs.circ_lib + ) AS ou, + COALESCE(aovc_acp.location, acp_for_peer_bibs.location) AS copy_location +FROM biblio.record_entry bre +LEFT JOIN asset.opac_visible_copies aovc + ON (aovc.record = bre.id) +LEFT JOIN asset.copy aovc_acp + ON (aovc.copy_id = aovc_acp.id) +LEFT JOIN asset.call_number acn_for_uri_map + ON (acn_for_uri_map.record = aovc.record) +LEFT JOIN asset.uri_call_number_map aucnm + ON (aucnm.call_number = acn_for_uri_map.id) +LEFT JOIN asset.uri auri + ON (aucnm.uri = auri.id) +LEFT JOIN biblio.peer_bib_copy_map bpbcm + ON (bpbcm.peer_record = bre.id) +LEFT JOIN asset.copy acp_for_peer_bibs + ON (bpbcm.target_copy = acp_for_peer_bibs.id ) +LEFT JOIN asset.copy_location peer_copy_loc + ON (peer_copy_loc.id = acp_for_peer_bibs.location ) +LEFT JOIN config.copy_status peer_copy_status + ON (peer_copy_status.id = acp_for_peer_bibs.status) +WHERE + NOT bre.deleted AND + COALESCE(auri.active, TRUE) AND + NOT COALESCE(acp_for_peer_bibs.deleted, FALSE) AND + COALESCE( + acp_for_peer_bibs.opac_visible, + peer_copy_loc.opac_visible, + peer_copy_status.opac_visible, + TRUE -- URIs and things on aovc are already known to be visible + ) +UNION +SELECT + bre.id AS record, + aou.id AS ou, + NULL AS copy_location +FROM biblio.record_entry bre +INNER JOIN config.bib_source cbs + ON (cbs.id = bre.source AND cbs.transcendant) +INNER JOIN actor.org_unit aou ON (true) +WHERE NOT bre.deleted ; + +-- This view supports checking for holdings in scope for staff browse. + +CREATE OR REPLACE VIEW asset.staff_holdings AS +SELECT + bre.id AS record, + COALESCE( + acp.circ_lib, acn_for_uri_map.owning_lib, acp_for_peer_bibs.circ_lib + ) AS ou, + COALESCE(acp.location, acp_for_peer_bibs.location) AS copy_location +FROM biblio.record_entry bre +LEFT JOIN asset.call_number acn + ON (acn.record = bre.id) +LEFT JOIN asset.copy acp + ON (acp.call_number = acn.id) +LEFT JOIN asset.call_number acn_for_uri_map + ON (acn_for_uri_map.record = bre.id) +LEFT JOIN asset.uri_call_number_map aucnm + ON (aucnm.call_number = acn_for_uri_map.id) +LEFT JOIN asset.uri auri + ON (aucnm.uri = auri.id) +LEFT JOIN biblio.peer_bib_copy_map bpbcm + ON (bpbcm.peer_record = bre.id) +LEFT JOIN asset.copy acp_for_peer_bibs + ON (bpbcm.target_copy = acp_for_peer_bibs.id ) +LEFT JOIN asset.copy_location peer_copy_loc + ON (peer_copy_loc.id = acp_for_peer_bibs.location ) +LEFT JOIN config.copy_status peer_copy_status + ON (peer_copy_status.id = acp_for_peer_bibs.status) +WHERE + NOT bre.deleted AND + COALESCE(auri.active, TRUE) AND + NOT COALESCE(acp_for_peer_bibs.deleted, FALSE) +UNION +SELECT + bre.id AS record, + aou.id AS ou, + NULL AS copy_location +FROM biblio.record_entry bre +INNER JOIN config.bib_source cbs + ON (cbs.id = bre.source AND cbs.transcendant) +INNER JOIN actor.org_unit aou ON (true) +WHERE NOT bre.deleted ; ALTER TABLE metabib.browse_entry ADD COLUMN sort_value TEXT; CREATE OR REPLACE FUNCTION metabib.browse_entry_sort_value() @@ -6928,11 +7018,13 @@ CREATE OR REPLACE FUNCTION metabib._browse_joins_and_where( search_field INT[], browse_term TEXT, context_org INT DEFAULT NULL, - context_loc_group INT DEFAULT NULL + context_loc_group INT DEFAULT NULL, + staff BOOL DEFAULT FALSE ) RETURNS TEXT[] AS $p$ DECLARE joins TEXT; where_clause TEXT; + scope_test_view TEXT; BEGIN joins := ' JOIN metabib.browse_entry_def_map mbedm ON ( @@ -6941,27 +7033,29 @@ BEGIN ) '; where_clause := ''; + IF staff THEN + scope_test_view := 'asset.staff_holdings'; + ELSE + scope_test_view := 'asset.visible_holdings'; + END IF; + IF context_org IS NOT NULL OR context_loc_group IS NOT NULL THEN - -- 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. - joins := joins || ' - JOIN asset.opac_visible_copies aovc ON (aovc.record = mbedm.source) - '; + + joins := joins || ' JOIN ' || scope_test_view || + ' scope_test ON (scope_test.record = mbedm.source) '; IF context_org IS NOT NULL THEN where_clause := where_clause || - 'aovc.circ_lib IN ( + 'scope_test.ou IN ( SELECT id FROM actor.org_unit_descendants(' || context_org || ')) AND '; END IF; IF context_loc_group IS NOT NULL THEN - joins := joins ||' JOIN asset.copy acp ON (acp.id = aovc.copy_id) '; where_clause := where_clause || - 'acp.location IN (SELECT location FROM asset.copy_location_group_map WHERE lgroup = ' || - context_loc_group || ') AND '; + '(scope_test.copy_location IS NULL OR + scope_test.copy_location IN (SELECT location FROM asset.copy_location_group_map WHERE lgroup = ' || + context_loc_group || ')) AND '; END IF; END IF; @@ -7004,6 +7098,7 @@ CREATE OR REPLACE FUNCTION metabib.browse( browse_term TEXT, context_org INT DEFAULT NULL, context_loc_group INT DEFAULT NULL, + staff BOOL DEFAULT FALSE, result_limit INT DEFAULT 10, result_offset INT DEFAULT 0 -- Can be negative! ) RETURNS SETOF metabib.flat_browse_entry_appearance AS $p$ @@ -7081,6 +7176,7 @@ BEGIN ') x ORDER BY sort_value ASC'; -- Un-reverse the result set. END IF; +-- RAISE NOTICE 'query is { % }', result_query; FOR f IN EXECUTE result_query LOOP r.browse_entry := f.id; r.value := f.value; @@ -7098,6 +7194,7 @@ CREATE OR REPLACE FUNCTION metabib.browse( browse_term TEXT, context_org INT DEFAULT NULL, context_loc_group INT DEFAULT NULL, + staff BOOL DEFAULT FALSE, result_limit INT DEFAULT 10, result_offset INT DEFAULT 0 -- Can be negative! ) RETURNS SETOF metabib.flat_browse_entry_appearance AS $p$ @@ -7108,6 +7205,7 @@ BEGIN browse_term, context_org, context_loc_group, + staff, result_limit, result_offset ); diff --git a/Open-ILS/src/templates/opac/browse.tt2 b/Open-ILS/src/templates/opac/browse.tt2 index 5543d54f57..227689c2c0 100644 --- a/Open-ILS/src/templates/opac/browse.tt2 +++ b/Open-ILS/src/templates/opac/browse.tt2 @@ -52,13 +52,15 @@ [% IF ctx.more_back %] ← [%l ('Back') %] [% END %] + [% IF browse.english_pager; # XXX how to apply i18n here? + current_qtype = CGI.param('qtype') || 'title' %] - 0-9 - A - B - C - [%# XXX TODO. Make off by default in config.tt2 %] + 0-9 + [% FOR letter IN ['A'..'Z'] %] + [% letter %] + [% END %] + [% END %] [% IF ctx.more_forward %] [%l ('Forward') %] → diff --git a/Open-ILS/src/templates/opac/parts/config.tt2 b/Open-ILS/src/templates/opac/parts/config.tt2 index c8e87ef95f..39d7d71116 100644 --- a/Open-ILS/src/templates/opac/parts/config.tt2 +++ b/Open-ILS/src/templates/opac/parts/config.tt2 @@ -149,4 +149,13 @@ search.basic_config = { # Set to 1 or 'true' to enable ctx.google_books_preview = 0; +############################################################################## +# Browse settings +# Set to 1 or 'true' to enable. This controls whether or not the +# "0-9 A B C D ..." links appear on the browse page. We don't yet have a +# serviceable way to internationalize these links, so sites must choose to +# turn on this feature. + +browse.english_pager = 0; + %] diff --git a/grand-visibility.sql b/grand-visibility.sql deleted file mode 100644 index ea61d43f4c..0000000000 --- a/grand-visibility.sql +++ /dev/null @@ -1,57 +0,0 @@ -CREATE OR REPLACE VIEW asset.visible_holdings AS -SELECT - bre.id AS record, - COALESCE( - aovc.circ_lib, acn_for_uri_map.owning_lib, acp_for_peer_bibs.circ_lib - ) AS ou, --- aovc.copy_id, --- aucnm.id AS uri_id, --- FALSE as transcendant, --- bre.deleted OR --- NOT COALESCE(auri.active, TRUE) OR --- COALESCE(acp_for_peer_bibs.deleted, FALSE) AS deleted, - COALESCE(aovc_acp.location, acp_for_peer_bibs.location) AS copy_location -FROM biblio.record_entry bre -LEFT JOIN asset.opac_visible_copies aovc - ON (aovc.record = bre.id) -LEFT JOIN asset.copy aovc_acp - ON (aovc.copy_id = aovc_acp.id) -LEFT JOIN asset.call_number acn_for_uri_map - ON (acn_for_uri_map.record = aovc.record) -LEFT JOIN asset.uri_call_number_map aucnm - ON (aucnm.call_number = acn_for_uri_map.id) -LEFT JOIN asset.uri auri - ON (aucnm.uri = auri.id) -LEFT JOIN biblio.peer_bib_copy_map bpbcm - ON (bpbcm.peer_record = bre.id) -LEFT JOIN asset.copy acp_for_peer_bibs - ON (bpbcm.target_copy = acp_for_peer_bibs.id ) -LEFT JOIN asset.copy_location peer_copy_loc - ON (peer_copy_loc.id = acp_for_peer_bibs.location ) -LEFT JOIN config.copy_status peer_copy_status - ON (peer_copy_status.id = acp_for_peer_bibs.status) -WHERE - NOT bre.deleted AND - COALESCE(auri.active, TRUE) AND - NOT COALESCE(acp_for_peer_bibs.deleted, FALSE) AND - COALESCE( - acp_for_peer_bibs.opac_visible, - peer_copy_loc.opac_visible, - peer_copy_status.opac_visible, - TRUE -- URIs and things on aovc are already considered visible - ) -UNION -SELECT - bre.id AS record, - aou.id AS ou, --- NULL AS copy_id, --- NULL AS uri_id, --- TRUE AS transcendant, --- bre.deleted, - NULL AS copy_location - --TRUE AS visible -FROM biblio.record_entry bre -INNER JOIN config.bib_source cbs - ON (cbs.id = bre.source AND cbs.transcendant) -INNER JOIN actor.org_unit aou ON (true) -WHERE NOT bre.deleted -- 2.11.0