wishlist: Improved email and printing from the OPAC

Bug #1749475 reported by Andrea Neiman on 2018-02-14
34
This bug affects 5 people
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Dan Wells

Bug Description

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).

Andrea Neiman (aneiman) on 2018-02-14
description: updated
Mike Rylander (mrylander) wrote :

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

tags: added: pullrequest
Terran McCanna (tmccanna) wrote :

Wonderful! Thanks, Mike!

Terran McCanna (tmccanna) wrote :

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) wrote :

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) wrote :

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.

Andrea Neiman (aneiman) on 2019-02-06
Changed in evergreen:
milestone: none → 3.3-beta1
Terran McCanna (tmccanna) wrote :

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.

Terran McCanna (tmccanna) wrote :
Jason Boyer (jboyer) wrote :

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) wrote :

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

Jason Stephenson (jstephenson) wrote :

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) wrote :

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) wrote :

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

Dan Wells (dbw2) on 2019-02-20
Changed in evergreen:
assignee: nobody → Dan Wells (dbw2)
Dan Wells (dbw2) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) wrote :

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) on 2019-02-27
tags: removed: pullrequest
Dan Wells (dbw2) wrote :

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) wrote :

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.

Changed in evergreen:
milestone: 3.3-beta1 → 3.3-rc
Changed in evergreen:
milestone: 3.3-rc → 3.next
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers