Metarecord constituents search result page should use standard search code

[Blake GH]

The current code (2.11) utilizes different search code to present the metarecord constituents. This fork in the code causes the page to present potentially unwanted bib records. Search language/format constraints are not respected. There are several bugs related to this. Most of the issues could be resolved if the code would execute the standard search code path.

I would like to open the discussion for a solution. Instead of listing file names and line numbers, I would like to share the idea in the form of preliminary code changes:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/blake/metarecord_constituent_result_reroute

Anyone have thoughts on this?

[Blake GH (bmagic)]

I have pushed my latest code to the branch. This addresses the format icons in the result pages. I welcome testing!

[Kathy Lussier (klussier)]

Thanks Blake!

Adding a note that one issue we've come across in initial testing is that duplicate icons display for metarecords where there are multiple editions using the same format. I've added a screenshot below.

There was a brief period of time when the same issue occurred with the previous method for metarecord searching. Maybe the fix there will be helpful for addressing the same issue here. https://bugs.launchpad.net/evergreen/+bug/1401177

Other testing has been promising. I only see icons that match the formats that were searched, the metarecord counts take the format filter into account, and the grouped record page is only displaying records that match the format filter.

[Blake GH (bmagic)]

I have made changes to that working branch. This change addresses the format icons repeating and addresses the (sometimes) incorrect parenthetical record count on the grouped results page.

[Kathy Lussier (klussier)]

I've spent quite a bit of time testing this code, and Blake has addressed most issues through recent code changes.

We're left with one outstanding issue and would like broader feedback from the community on the best approach to take. When limiting by an attribute, a format limiter for example, the search results page now only displays metarecord results that contain a bib record with that limiter and also display a record count for those grouped records that match that limiter.

Overall, it's working very well.

The one area where numbers don't add up correctly is in the copy counts on the metarecrod result screen and then the list of copies available when you click Show More Details on the metarecord result page. The displayed copy counts are for copies attached to all records in the MR group, not just the ones that match the limiter.

For example, the screenshot at https://www.screencast.com/t/fXsShrvrU2, shows the metarecord search results page for a Harry Potter search, limited to the Large Print Format. There are two large print editions of Harry Potter and the Sorcerer's Stone with a total of 11 copies attached to those two records. However, the total displayed copy count is 28 because it includes the copies attached to other formats of this metarecord group.

From what I understand, there is nothing unapi.holdings_xml now that filters, probably because if the records that don't match the filter typically wouldn't display on the search results page. Any ideas on the best approach to get the copy counts accurate?

[Mike Rylander (mrylander)]

Kathy,

Today, the search code returns a field containing the constituent bib if only one matches the requirements of the search. If there are more than one, that field is null. That's used to facilitate jumping straight through to the bib from the result screen.

I think to address this we'll probably need to add a field to the result that contains the list of relevant constituent bibs, and then pass this back somehow to unapi.mmr_holdings_xml code when getting the XML of the holdings. While that stored proc is already "special" and there's no real worry about making its arguments unique, the unapi.mmr stored proc is meant to match the other real-object unapi calls and it's parameters really shouldn't change.

To get that information back to the unapi code we could probably use a specially formatted string passed via the includes parameter. That would let us avoid changing the function signature, and should be fairly straight forward to use on the perl side. While "includes" is usually used to specify the types of objects we should embed, I think it could be useful to invent a general purpose encoding for specifying both the type and the specific IDs to include. I'm pretty sure that could be built in a backward-compatible way.

[Kathy Lussier (klussier)]

Thank you for the guidance Mike! After talking it over with Blake, I would like to create a new bug to address the copy count issue and consider merging this code sooner so that we can address all of the bugs that will be fixed by changing the search path.

After we add a test to this code, I plan to sign off on it, but not merge it immediately. It works well in my testing, but I welcome additional eyeballs on the code.

I also am curious if this code would be eligible for backporting. On the one hand, the change in search path for metarecord searches feels like a new feature to me. However, it also fixes several outstanding bugs related to limiting, the ability to correctly retrieve e-resources, and the ability to page through the metarecord results. Any opinions on that?

[Blake GH (bmagic)]

I just pushed the pgTAP test on the top of that branch

[Kathy Lussier (klussier)]

I signed off on the code and am leaving it in the working repo for now to give others an opportunity to review it:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/lp1629108-metarecord-constituent-result-reroute

I also created a release notes entry in case it isn't considered to be backportable.

Thanks Blake! We're very excited to see these improvements in our systems!

[Bill Erickson (berick)]

I have some housekeeping requests for this one.

Reviewing the branch I see 4 commits in a row with the summary message "LP1629108 metarecord_constituent_result_reroute". Can we please get meaningful summary messages for these commits?

I'm also guessing some of these can be squashed. A separate commit for a pgtap test makes sense, for example, but some of these small fixes-to-fixes commits are probably best absorbed by earlier commits.

[Blake GH (bmagic)]

Bill,

No problem. I squashed those 4 commits into a single commit. I then created a commit over that which finalizes the Search.pm changes removing the if block. Then I added the pgTAP test on top of that. The link to the branch is unchanged. Let me know.

[Kathy Lussier (klussier)]

Hi Blake,

We're getting a merge conflict on this now, most likely because I merged the realign search layers branch this afternoon. Could you resolve the conflict and update the branch?

Thanks!
Kathy

[Kathy Lussier (klussier)]

A new signed-off branch that has been rebased against current master is available at:
http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/kmlussier/lp1629108-metarecord-constituent-result-reroute

[Blake GH (bmagic)]

And a small production tweak for the branch:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/blake/LP1629108-metarecord-constituent-result-reroute

[Kathy Lussier (klussier)]

Thank you for your work on this Blake! Merged to master for inclusion in 2.12.