Searching specific location in Advanced Search not limiting correctly in 3.0

Bug #1723977 reported by Jason Boyer
30
This bug affects 5 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Medium
Unassigned

Bug Description

Eg 3.0
OSRF 3.0
Pg 9.4
Ubuntu 14.04

When logged in as a staff member on either the web or staff clients searching at a specific location limits results inconsistently. It's not limited only to the selected location but it is significantly fewer items than searching everywhere. For instance, searching evergreen.lib.in.us/eg/opac for the title Bag of Bones at the location Indiana State Library - Indianpolis the search returns 4 results: 1 large print book and 3 Overdrive records. When staff perform the same search in any client, 11 results are returned: the same 4 as the public search plus 2 totally unrelated print books, a cd and 3 e-music records, none of which have ever had holdings at this location.

Pulling the database query out for the same search logged in and not, there are 2 primary differences:

The staff search includes these 2 column definitions:
,c_attr AS (SELECT (ARRAY_TO_STRING(ARRAY[search.calculate_visibility_attribute_test('circ_lib','{73}',FALSE)],'&'))::query_int AS vis_test FROM asset.patron_default_visibility_mask() x)
,b_attr AS (SELECT (ARRAY_TO_STRING(ARRAY[search.calculate_visibility_attribute_test('luri_org','{1,72,73}',FALSE)],'&'))::query_int AS vis_test FROM asset.patron_default_visibility_mask() x)

where the public search defines them like so:
,c_attr AS (SELECT (ARRAY_TO_STRING(ARRAY[c_attrs,search.calculate_visibility_attribute_test('circ_lib','{73}',FALSE)],'&'))::query_int AS vis_test FROM asset.patron_default_visibility_mask() x)
,b_attr AS (SELECT (ARRAY_TO_STRING(ARRAY[b_attrs,search.calculate_visibility_attribute_test('luri_org','{1,72,73}',FALSE)],'&'))::query_int AS vis_test FROM asset.patron_default_visibility_mask() x)

(Note the 'c_attrs,' and 'b_attrs,' segments in the inner ARRAY, they're fields being pulled from the results of asset.patron_default_visibility_mask().)

Further down, in the WHERE clause is this difference:

staff search:
((EXISTS (SELECT 1 FROM asset.copy_vis_attr_cache WHERE record = m.source AND vis_attr_vector @@ c_attr.vis_test) OR NOT EXISTS (SELECT 1 FROM asset.copy_vis_attr_cache WHERE record = m.source))) OR ((b_attr.vis_test IS NULL OR bre.vis_attr_vector @@ b_attr.vis_test) OR NOT ( int4range(0,268435455,'[]') @> ANY(bre.vis_attr_vector) ))

and public search:

(EXISTS (SELECT 1 FROM asset.copy_vis_attr_cache WHERE record = m.source AND vis_attr_vector @@ c_attr.vis_test)) OR ((b_attr.vis_test IS NULL OR bre.vis_attr_vector @@ b_attr.vis_test))

The first difference strikes me as the potential issue. The patron version selects the *_attrs column of the results from asset.patron_default_visibility_mask() and groups that with the output of the appropriate call to search.calculate_visibility_attribute_test(), where the staff version ignores these *_attrs columns entirely and using only the output of search.calculate_visibility_mask().

Those are my *assumptions*, so I wanted to post this for possible discussion.

Revision history for this message
Chris Sharp (chrissharp123) wrote :

I can confirm the behavior on our 3.0.0 test server running with PINES customizations/data.

Changed in evergreen:
status: New → Confirmed
Revision history for this message
Mike Rylander (mrylander) wrote :

Jason,

This comment will just be informational ... I'll be looking at the details of the WHERE clauses you captured soon.

The differences between staff and public queries in the CTE are intentional. Specifically, we don't want the patron (non-staff) defaults to be used in staff searches, which do things like exclude hidden org units and non-opac_visible statuses. The stored proc will only return one row ever, so the simplest possible change to turn a patron search CTE into a staff search CTE (though, perhaps less self-documenting...) is to just avoid including the output of that function in the array we're building.

The WHERE clauses are also intentionally different. The opac clause basically says "you have to have at least one of an luri at an appropriate org, a trancendent[sic] source, or a copy matching the requirements of both the default filters (general visibility) and the user-requested filters (org, copy location, status, etc). The staff filters, on the other hand, have no defaults, and should only filter bibs out of a result set based on the staff user's requested filtering (inclusive filter on org, luri owner, status, etc), OR the lack of either any entries for the record on the asset.copy_vis_attr_cache table (no holdings) or luri orgs.

Revision history for this message
Mike Rylander (mrylander) wrote :

I'm adding the "expanded" where clause as an attachment for formatting purposes (thanks, launchpad).

Jason, are you bee seeing records that have a non-null bib source, no Located URIs, and copies at "other" locations? I think there be a branch of the logic to enhance for that situation...

Revision history for this message
Jason Boyer (jboyer) wrote :

Thanks for the explanation Mike; that's the kind of background I was hoping for when I wrote this. I'll see what I can narrow down about these records with your list of attributes in #3.

Revision history for this message
Jason Boyer (jboyer) wrote :

Looking over the results, yes, all of the unexpected results have a bib source specified; none have holdings or located URIs at the target library. Some DO have located URIs, as we have Hoopla records with 18 856$9's, but not at the target location.

Revision history for this message
Mike Rylander (mrylander) wrote :

I've got a branch for testing here: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp-1723977-staff-visibility-test

TL;DR: Test staff empt-bib stuff all at once.

Revision history for this message
Mike Rylander (mrylander) wrote :

And, I've updated the branch with an improved test for staff bib record visibility testing.

Revision history for this message
Jason Boyer (jboyer) wrote :

Excelsior! The changes in Mike's branch from #6 fix the issue with the caveat that asset.copy_vis_attr_cache will likely need to have deleted items removed and it needs an upgrade script for the change to asset.cache_copy_visibility () (which is available from bug 1724246: https://bugs.launchpad.net/evergreen/+bug/1724246 )

Signoff lives here: http://git.evergreen-ils.org/?p=working/Evergreen.git;a=commitdiff;h=d5fcb2b160321de5a0bbbe5e5efe497c37571849

Revision history for this message
Jason Boyer (jboyer) wrote :
Andrea Neiman (aneiman)
tags: added: signedoff
Changed in evergreen:
milestone: none → 3.0.2
Revision history for this message
Mike Rylander (mrylander) wrote :
tags: added: pullrequest
Galen Charlton (gmc)
Changed in evergreen:
assignee: nobody → Galen Charlton (gmc)
importance: Undecided → Medium
Revision history for this message
Galen Charlton (gmc) wrote :

Pushed to master and rel_3_0 along with a follow-up correcting an issue with the schema update. Thanks, Mike and Jason!

Changed in evergreen:
status: Confirmed → Fix Committed
assignee: Galen Charlton (gmc) → nobody
Changed in evergreen:
status: Fix Committed → Fix Released
To post a comment you must log in.
This report contains Public information  
Everyone can see this information.

Duplicates of this bug

Other bug subscribers

Remote bug watches

Bug watches keep track of this bug in other bug trackers.