wishlist: Improved email and printing from the OPAC

[Andrea Neiman]

Equninox has been contracted by MassLNC to perform development to improve the email and printing options available from the OPAC.

Highlights include:
* Ability to see a preview of the printout or email
* Patron control over how records in the printout or email are sorted, whether holdings information is included, and which library's holdings are displayed
* Staff management of output formats & related Action/Trigger templates

Additionally, as part of this work a new concept will be added to Action Trigger: an Action Trigger Event Definition Group, which will represent a collection of one or more Action Trigger Event Definitions that format output for printing or emailing.

Full specifications can be seen here: https://yeti.esilibrary.com/dev/public/techspecs/opac_email_and_print.pdf

MassLNC's requirements can be seen here: http://masslnc.org/node/3314 (note that we are only doing the first requirement, OPAC-01a-2017).

[Mike Rylander (mrylander)]

Here is a branch implementing this functionality. From the commit message:

This commit provides new functionality for controlling and delivering print and email bibliographic record export options.

Staff will be able to adjust existing and create new print and email formats using the Action/Trigger infrastructure and the new Event Definition Group capability.

Patrons will be able to choose from one of several formats, as configured by staff, in which to receive one or more bibliographic records. Print and email preview is made available for individual records via the existing links on the record detail page, and for lists of records via new entry points on both the Basket (anonymous list) and My Lists interfaces.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp-1749475-print_email_improvements

[Terran McCanna (tmccanna)]

Wonderful! Thanks, Mike!

[Terran McCanna (tmccanna)]

Hi Mike,

We've been testing on a PINES test server and most of it is working great. We are, however, seeing this error crop up in the server logs:

[ERR :25075:EventGroup.pm:98:154720590324625499] Event reacting failed with Not a HASH reference at /usr/local/share/perl/5.22.1/OpenILS/Application/Trigger/Reactor/SendEmail.pm line 52.

We've also noticed a couple of discrepancies between the two different Basket dropdown actions menus - the one that appears at the top-right of the catalog page (in both the staff client and the OPAC) doesn't include the Add to List options, and the one that appears at the top of the View Basket page (in the staff client) doesn't include the Add Basket to Bucket option. Is it feasible to make the two dropdowns consistent?

Thank you!
Terran

[Mike Rylander (mrylander)]

Those basket menu entries are not related to this work, so I would recommend a new bug for them.

As for the log message, are you seeing that with attempts to email records, or with other email-related action/triggers? I have a provisional fix, but knowing that should tell me if it's a global fix or if it needs more work.

[Mike Rylander (mrylander)]

I've pushed two updates to the branch, one to propagate sorting from bucket and basket interfaces, and another to allow sort direction selection (and propagation). The second commit also contains a fix for the log messages reported by Terran.

[Terran McCanna (tmccanna)]

We're getting inconsistent testing results. The good news is, we're no longer seeing the errors in the server logs.

When we're just testing on a Concerto database, it works well for the most part, although it does time out sometimes, particularly when trying to print from a basket (for example: I created a basket with 18 items and tried to print out full details for all branches and it hung and then eventually gave a 504 Gateway Timeout).

However, when we put it on a test server with the PINES data set, I get very consistent results. It is nearly always very slow, even when only attempting to print or email a single item, and sometimes it will work, sometimes it will give a "Bad Request - Your browser sent a request that this server could not understand," sometimes it will give an Evergreen-generated error of "Error previwing record: No record data returned from server" (see attached screenshot - also there's a typo in that string), and sometimes it will give an Evergreen-generated error of "Error printing record; No record data returned from server" (see attached screenshot).

We're not seeing anything useful in the browser console or in the server logs, so at this point I'm not sure whether it is having problems because of the size of the PINES database or if there is something else causing the problem. It would be useful if another library with a large database could apply it to see if they are seeing similar errors.

[Jason Boyer (jboyer)]

I did some testing on this recently and when I got an ISE I did get this in the logs:
Error processing Trigger template: undef error - Can't call method "textContent" on an undefined value at /usr/local/share/perl/5.18.2/OpenILS/Application/Trigger/Reactor.pm line 452.
That line is trying to pull a pubdate to sort by and 3 of them were indeed missing 008s. They undoubtedly poor records, but these things hide everywhere and throwing an ISE at users is pretty ugly.

[Jason Boyer (jboyer)]

Re 008's: Having an 008 whose date1 is only fill characters also causes an ISE.

[Jason Stephenson (jstephenson)]

The branch needs a rebase on master, I get conflicts in the following files:

 both modified: Open-ILS/src/eg2/package-lock.json
 both modified: Open-ILS/src/eg2/package.json
 both modified: Open-ILS/src/eg2/src/app/staff/admin/basic-admin-page.component.ts
 both modified: Open-ILS/src/sql/Pg/950.data.seed-values.sql

My attempt at such a rebase is in collab/dyrcona/lp-1749475-print_email_improvements-rebase

[Jason Boyer (jboyer)]

Also, on the Record Email preview page, once you click Email Now you're taken to a results page that explains that your email has been queued for delivery. Something happens to the CSS urls on this page so the page is largely un-stlyed. The ?v= part of the cache-buster is missing, so the browser is looking for style.css001, etc.

The Print page (after clicking Print Now) is also un-styled but this appears to be intentional since there aren't any 404's for stylesheets.

[Jason Stephenson (jstephenson)]

So, the rebase branch in comment #10 is broken. Don't use it.

[Dan Wells (dbw2)]

Okay, I spent a large part of yesterday going over (and over) this code, and I would say it is 90% straightforward and good, with 10% curve ball which I am struggling to make sense of.

I have a branch where things all seem to work as designed, and I really appreciate this features, but the sticking point for me is the addition of "Event Definition Groups". I keep running through it in my mind to see if there is something I am missing, but I really think the code would be substantially better if they were simply left out. I think I understand the impetus, but the implementation exists in a strange place of trying to be generic while ending up so specific to the problem at hand that is hard to see very many opportunities to make use of it in the future.

Case in point, this feature creates two groups, but each group has only two "members", and in this case, each "member" points back to the same exact definition, so each group really has just one member in two slightly different shades.

Action trigger is already a complex beast, and I fear this does more harm than good to the structure.

I am eager to hear other perspectives on this. In the meantime, I am going to take a stab at removing this layer, as that also might end up illuminating, for me, the value of it.

Sincerely,
Dan

[Dan Wells (dbw2)]

A first pass at removing the Event Def Groups code resulted in 13 insertions, 187 deletions. I know that is proof of exactly nothing, but there were also no obvious hangups by moving in that direction. Will do more testing on Monday and post the branch if things pan out.

[Mike Rylander (mrylander)]

Thanks for looking at the branch, Dan.

So, first, WRT the simplicity of the initial use of Event Definition Groups (Groups), specifically that I implemented the brief and full "formats" using one definition for each delivery mechanism. I did it that way because the output is essentially the same with some information omitted in the Brief case. It seemed there was little benefit in duplicating the definitions and then forcing admins to edit two, rather than one, to make local adjustments. It also shows how a definition could be leveraged for more than one "format" if they are basically the same.

Regarding the existential question of "why Groups", that is to support (actually) different formats -- for instance, different bibliography standards -- as options for rendering the data. Without some way of saying "here are a set of definitions that can all be seen as alternative formats for a specific purpose" then we'd have to embed all the different formats (various bibliography formats, the extant brief and full, others yet to be thought of) in one definition. With groups in place, new definitions can be created and added to the "pinned" Groups 1 and/or 2 at will by admins. That was a specific, intentional goal of the development because including the development of several bibliography formats in the initial project would have pushed the cost beyond what could be supported in this first work. By intentionally providing the infrastructure that fully supports dropping in new formats to the initial two groups with just a new event definition and group mapping, we are able to move the ball forward and set the stage for more contributions /immediately/ rather than having to rewrite what's there now (or, massively over-complicate the event definitions in question).

More broadly, the Group IDs are in a reserved ID space so that we can depend on a stable hook for "here's how you can format data for email" or "here's how, for print", but other groups can be imagined ("send via NCIP for remote update, with choices for common NCIP applications", etc) for the reserved ID space, and local-use groups could provide integration points for special-purpose (perhaps locally-developed) external "things".

I hope that helps explain why the Groups infrastructure exists...

Thanks again!

[Dan Wells (dbw2)]

Thanks for the quick feedback, Mike. I get where you are coming from, and I see the utility, it just feels like something still isn't sitting quite right for me as it stands. I'm sorry to be a pain! Hopefully we can at least talk through it a little more and see if some improvements can be found.

Starting small, I am wondering about the utility of the flags on Event Def. Group Member (sortable, holdings, external). I understand what they mean, but what part of the process is supposed to consult these? Only 'holdings' is used right now. We also already have event_params and environment. Maybe something better could be done here by extending in that direction instead? I don't have a proposal, just leery of a pool of hard-coded options at this layer. What do you think?

I will also keep thinking and see if I can't get a clearer-headed about it.

[Mike Rylander (mrylander)]

Thanks, Dan.

I'm open to dropping the external flag -- we can bring that in when there's a use case ready for it, like external PDF processing. The initial specific reasoning for the flag was to provide a signal that an external XPath 2.0 processor that can make use of extant bibliography XSLTs will be called and that some special (though currently undefined) handling of the template output may be necessary.

The holdings flag lets us leverage one definition mapped with different settings -- params won't allow that. Looking at it through the lens of the targeted use case(s) in the offered code, I can see how it looks use-case specific, but even if we were dealing with definitions with different hook core types of, say, circulations or holds or serial subscriptions, it would have generally the same meaning and IMO could be used to the same general effect.

The sortable flag is more about whether the choice should be presented to the user (allowing a signal to the UI, rather than to the definition as with "holdings"), so I think it belongs at the current level in the stack, but for the current implementation it isn't strictly necessary (sorting always allowed) and I wouldn't fight too hard to keep it.

[Dan Wells (dbw2)]

Thanks, Mike. It helps to know which pieces are fundamental.

I am really fond of the concept of Action Trigger as a sort of generic Evergreen output processor, and I am glad to see it getting some attention. At this stage I am trying to firm up my mental model to better offer reasonable insights on the design.

For example, I ran into an issue testing the code with the idea of the 'preview' flag being part of the $user_data blob. The branch is trying to have the SendEmail reactor do double duty, generating both the previews and the real emails. This makes sense. The issue, though, is that SendEmail is used in quite a few events, and sometimes grouped, sometimes not. When used in a grouped event, $user_data will (I think) always be an array ref, but when used in a non-grouped event, it will be whatever the event firer wants it to be (typically a hash).

This poses a few problems in trying to use $user_data as a conduit to communicate with the reactor. First, we obviously need to cover more cases just to make the code function, that is, treat $user_data as possibly empty, possibly an array, possibly a hash, and seek out the 'preview' flag in each way. This is perhaps a little ugly, but doable. Second, though, it becomes clear that $user_data is not really a reliable way to affect the behavior of a reactor due to grouping. There is nothing (I think) to prevent events being grouped with (in this case) conflicting 'preview' flags. Perhaps the situation is rare and contrived, but it would still be better to have a fully determinate system which does not rely on the order of possibly unrelated events.

A quick solution would be to give up on the flag approach, and create a new Reactor (e.g. PreviewEmail) which is called in the right cases. This isn't too bad of an option, given that the SendEmail reactor code is so simple. However, we end up with partially duplicated code at the Reactor, and if we still care about fully supporting the old event definition, we also get an almost wholly duplicated event definition. This works fine, is sound, but not ideal.

From this discussion, we can imagine a few missing AT features which would be useful. First, it would be good to have some way for an event firer to communicate reliably with the Reactor, ideally in a way which is also surfaced at the grouping mechanism. Second, it seems like a general benefit could be had by making AT templates sharable across different events in some fashion. (This could also potentially apply generally to this feature quite nicely, of course.)

So, what say others? Is it worth pausing and shaking things up a little more? Does anything stand out as a natural direction to meet some of these weaknesses while also adding the necessary strengths? I know there have been other structural issues with AT over the years, so perhaps someone keenly affected by those can help to further illustrate the big picture.

I think it is worth raising this at the next developer meeting to see if there is broad interest in hashing something out. If not, it might be best to stick with a more isolated approach. In the meantime, any and all proposals or thoughts are welcome here!

[Andrea Neiman (aneiman)]

To be a bit more precise about "Why Groups", MassLNC in their initial requirements for this project specified the ability to choose among multiple output formats.

Requirement #3, here:
http://masslnc.org/node/3314

In later conversations they further clarified that they wanted users to be able to drop in additional output formats beyond the two called for in the seed data. Groups are part of the method we used to implement this requirement, for the reasons Mike outlines in comment #15.

[Dan Wells (dbw2)]

I realized I should probably get at least most of my current code out there to also help the conversation:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/dbwells/lp1749475_rebase_fixup

working/collab/dbwells/lp1749475_rebase_fixup

The commits there start as basic fixes and get progressively deeper, with the last being the concept of a PreviewEmail reactor (which, as noted, is itself an imperfect solution). Without the last commit, the email does not preview correctly (always sends), but I have the alternative fix for that if I've misunderstood the issues above.

[Dan Wells (dbw2)]

If I get another free moment, I'll again try to test and post my DB-less option, not as something I am pushing for, but as more discussion fodder.

[Lugene Shelly (lugene)]

SPARK recently upgraded to 3.3 and started using baskets in the OPAC. One of the things we've noticed is, when printing a basket list, the Bib ID number prints with the item. This isn't helpful information for the patron. We'd like to see it left out or replaced with something the patron can relate to, perhaps the barcode.

[Michele Morgan (mmorgan)]

Lugene, the information that's printed is configurable in the template for the Action Trigger that generates the printout. The Action Trigger is called "biblio.record_entry.print". You may want to ask your system administrator about changing it. Library staff members may use this as well, however, and may find the Bib ID to be useful information.

[Michele Morgan (mmorgan)]

I'm adding a link to related bug 1854986, which may have been a factor in testing this code.

Comment #6 above in particular suggests this is the case.

[Galen Charlton (gmc)]

I've pushed a new rebase:

working/collab/gmcharlt/lp1749475_opac_email_print_rebase
https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/gmcharlt/lp1749475_opac_email_print_rebase

This patch adopts all but the last of Dan Wells' patches and adds follow-ups to:

- move the new admin interfaces over to Local Administration, as was originally intended
- add release notes
- strengthen the checking of whether user_data contains a preview flag (in lieu of Dan's EmailPreview reactor)

I agree that there is an opportunity and warrant for making the A/T infrastructure better able to handle event definitions that can share templates but have multiple or alternate reactors that are invoked in different circumstances. However, as this enhancement has stalled for almost a year, I request that it not become the target of further architectural changes; if the patch series as is is digging us into a whole, I don't think it's a deep one.

I note that bug 1801163 is relevant for testing this one, and in particular, the fix for that one will need to be incorporated into the new open-ils.search.biblio.record.email.send_output method.

[Michele Morgan (mmorgan)]

I've been testing this enhancement on a sandbox kindly set up by Blake Henderson. I've run into problems when adding an additional entry to Trigger Event Definition Group Member Configuration. I'm unable to cleanly switch between different print formats. Here's what I did:

First, in the stock catalog (not logged in), I add 3 records to my basket. Under Basket Actions, I chose Print Title Details and click Go.

I am directed to the preview screen with "Brief" format selected. My preview shows:

1. Bib ID# 88
   Title: Piano concerto ;Variations for orchestra
   Author: Carter, Elliott,
   Publication Info: New World Records, 1986
   Item Type: Musical sound recording

2. Bib ID# 46
   ISBN: 0253367204
   Title: The writings of Elliott Carter :an American composer looks at modern music /
   Author: Carter, Elliott,
   Publication Info: Indiana University, 1977
   Item Type: Language material

3. Bib ID# 65
   Title: Double concerto for harpsichord and two chamber orchestras
   Author: Carter, Elliott,
   Publication Info: Nonesuch, 1975
   Item Type: Musical sound recording

Switching to "Full" format adds holdings information as expected.

Next, I create a new action trigger definition called biblio.record_entry.print.2 by cloning blbio.record_entry.print and editing the template to put the author and title on one line so it outputs as:

1. Piano concerto ;Variations for orchestra by Carter, Elliott,
   New World Records, 1986

2. The writings of Elliott Carter :an American composer looks at modern music / by Carter, Elliott,
   Indiana University, 1977

3. Double concerto for harpsichord and two chamber orchestras by Carter, Elliott,
   Nonesuch, 1975

I add the new definition to the Trigger Event Definition Group Member Configuration as follows:

name: Super Brief
group: Print Record(s)
definition: biblio.record_entry.print.2
sortable: yes
include holdings: no

In the catalog (not logged in), I again select three records and they are added to my basket. Under Basket Actions, I choose Print Title Details and click Go.

I am directed to the preview screen with "Brief" format selected. However, the preview I see is using the template output from the "Super Brief" format I just added. I can choose "Brief", "Full", or "Super Brief" from the dropdown and choose Update. Selecting "Full" does show holdings, but the formatting in the preview always shows the "Super Brief" title info format from the biblio.record_entry.print.2 event definition. See screenshots below.

I removed the Trigger Event Definition Group Member Configuration entry for "Super Brief", but my preview still showed the template output for biblio.record_entry.print.2. Only when I disabled the biblio.record_entry.print.2 trigger definition altogether, did it revert to using the stock action trigger template, biblio.record_entry.print.

Not exactly sure what's happening, but we are unable to define additional output formats and choose between them.

[Michele Morgan (mmorgan)]

Here's a screenshot of the preview of the "Brief" format with the stock groups defined.

[Michele Morgan (mmorgan)]

Here's a preview of the "Super Brief" format which was added to the group configuration.

[Michele Morgan (mmorgan)]

This screenshot shows the "Brief" format chosen, but "Super Brief" is actually displayed, even after the Update button is clicked.

[Michele Morgan (mmorgan)]

This screenshot shows the "Full" format chosen, but "Super Brief" is actually displayed, even after the Update button is clicked.

[Terran McCanna (tmccanna)]

Original Project:

We've put this on a test server with a copy of the PINES data set, and the great news is that the timeout issue that prevented us from using it before seems to be resolved. I was able to create a basket of 110 items and sorted and printed out full records with holdings for all 300 of our branches without it timing out on me - it wasn't super fast, but it worked!

*****************

New Interfaces:

I didn't test the new admin interfaces as fully as Michele did, but I have a few notes about the new admin interface for Event Definition Group Members:

1) The grid has the Event Definition Group ID available as a column, but not the Event Definition Group Name (Print Records / Email Records), which would be more helpful.

2) In the pop-up form, the form has a dropdown selector for the Group ID but not the Group Name. This creates a bit of an obstacle for use if you have to leave the screen and go back to the Event Definition Groups interface to figure out which ID you need to use.

3) What is the Definition column / field? I assume from the release notes that that is the ID for the Action Trigger Event Definition, but it doesn't say that anywhere in the interface, so it's confusing. Either renaming the field or adding a contextual help popup or something like that would help.

[Terran McCanna (tmccanna)]

Question: Is it worth splitting up this work to the portion that was originally requested (which looks to be working well now, but would like to test again if it gets split apart) and the new interfaces that we can get the first part accepted sooner?

Or would it be better to wait until the open issues surrounding the new interfaces are resolved and then get it all tested and signed off on at once?

[Michele Morgan (mmorgan)]

My preference would be to move forward with accepting the patron facing enhancements and addressing the issues with the new interfaces separately.

The patron side enhancement provides a vastly improved experience for patrons, and the existing triggers are already a great improvement over current functionality.

Even given my findings in comment 26, a lot of customization can still be done with the existing triggers until the staff interface issues are addressed.

[Ruth Frasur Davis (redavis)]

Galen, have you had a chance to revisit this? It would be great if we could get it resolved in time to get into 3.5. If not, and understanding that time is always valuable and pretty weird right now, would it be feasible for you to split your fixes to the original work from the new components that you built so we could get the original work signed off on?

[Michele Morgan (mmorgan)]

I've done some additional testing on the latest branch of this development on the Feedback Fest test server.

My testing showed that Terran's concerns #1 and #2 in comment 25 are both resolved. #3 could be resolved by changing "Definition" to "Event Definition", but as it is, the values that populate the dropdown are easily recognized as trigger event definitions.

I would opt for a minor tweak in the stock email output trigger to remove extra white space, but as it is, the patron experience is 1000x better.

This code also fixes bug 1854986.

I will add a sign off branch soon.

The problem with adding to Trigger Event Definition Group Members stated in comment #26 is still an issue, but I would defer that issue to a separate bug. The stock groups and triggers provide plenty to work with as they are.

[Michele Morgan (mmorgan)]

My signoff branch is at:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mmorgan/lp1749475_tweaks_and_signoff

This branch includes my signoff and two additional commits:

- One relabels some fields in the admin pages for clarity
- The other strips extra white space from the email action trigger template.

Since the upgrade script for this feature overwrites action trigger templates using the hooks biblio.format.record_entry.email and biblio.format.record_entry.print, an upgrade note is needed warning that any customizations to action trigger event definitions using those hooks will need to be reapplied.

Removing the needsdiscussion tag and adding signedoff.

[Galen Charlton (gmc)]

Thanks for testing Michele, and for pointing out about the upgrade issue. After discussing, Mike will work on a change that should preserve customizations and follow what was called for in the spec anyway:

"During upgrade, if the Action Trigger Event Definitions for the biblio.format.record_entry.print or biblio.format.record_entry.email hooks exactly match stock seed data from previous versions of Evergreen, they will be completely replaced by the new Event Definition Group, Group Member, and Event Definitions seed data.

Otherwise, existing biblio.format.record_entry.print and biblio.format.record_entry.email Event Definitions will become members of new Event Definition Groups as "Brief" Members, with ownership of each Event Definition Groups set to match those of the converted Event Definitions. The converted Event Definition Group Members will not have the sortable flag set. The upgrade will then add a new Full Member to each of the new Event Definition Groups."

[Mike Rylander (mrylander)]

Hi folks,

You'll find a branch at https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1749475-upgrade_script-more_signoff that signs off Michele's commits and adds a new one that does what Galen described in #37.

[Michele Morgan (mmorgan)]

I tested Mike's upgrade script and it works as described, preserving any customizations that have been made to the action trigger definitions. This branch fixes a syntax error in the upgrade script and adds my signoff:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mmorgan/lp1749475_signoff

[Galen Charlton (gmc)]

Pushed to master for inclusion in 3.6. Thanks, Mike, Dan, and Michele!