Copy Alert Persistence and Suppression Matrix

Bug #1676608 reported by Galen Charlton
32
This bug affects 7 people
Affects Status Importance Assigned to Milestone
Evergreen
Fix Released
Wishlist
Unassigned

Bug Description

Today, Evergreen provides two types of alerts for copy data: on-demand alerts that are displayed when an event occurs, such as the check-in of an item that has been marked Missing or Lost; and one-per-copy notification alert messages that are supplied by staff. Both alerts are presented to the user at check-in and checkout, with some exceptions, and staff intervention is required to force or stop the attempted action.

A new matrix of Alert Types and Events will be created, which will subsume the functionality provided by the current Alert Message and on-demand event alerting functionality. Staff will be able to attach multiple Alert Messages to a copy, and to define during which staff actions, such as check-in, checkout, and renew, those Alert Messages will be displayed. Staff will be able to mark each Alert Message as either Temporary or Persistent. Temporary Alert Messages will offer the alerted staff the option to Acknowledge the alert, at which time the Alert Message in question will be disabled, but not deleted, from the copy.

This will leverage the event matrix described above to generate a record of on-demand Alert Messages that occur at specific events. In addition, the ability to suppress the display of on-demand event alerts on a per-org-unit basis. Suppression will not prevent the generation of an event, but will instead act as if staff had intervened to force the attempted action. On-demand, system-generated alerts will be marked as temporary and auto-close, simply recording the fact of the alert. For direct checkout of Long Overdue items, a single new Org Setting will decided whether the preceding suppression-forced checkin will forgive fines or retain them. We will create a similar setting for Lost items.

Events will react to varying levels of granularity. Less granular events will be displayed at the time of any sub-events, whereas more granular alerts will be displayed only when applicable. For instance, "Non-renewal Check-in at Owning Library" Alert Types will only display when an item is checked in at it’s Owning Lib. However, general "Check-in" event alerts will display for each checkin event for an item, whether owning-library local or not, and even for renewal checkins.

Each ILS administrator with appropriate permissions will have the ability to associate one or more statuses with each Alert Type which, when used as a Temporary Copy Alert, will be presented to the alerted staff for use as the Next Copy Status, overriding the next natural status the copy would take. Persistent Alerts will not be able to make use of the Next Status values, if any are defined for the Alert Type. If one status is supplied by the administrator, that status will be applied to the copy after the Alert is processed, instead of the natural Next Status. If more than one Next Status is supplied, staff will be presented with the list to select from, with the first value in the list being selected by default. Suppressed on-demand events will not make use of the Next Status functionality. Only check-in event Alert Types will have associated logic that makes use of the Next Status selected by staff. Future development may add Alert Types for other events, such as the various transit types, or for events in the hold lifecycle. In that case, logic specific to those events would be added to make use of the Next Status selected by staff.

Finally, each Alert Type can be configured so that it will only display when the triggering event occurs at the owning or circulation library, or either.

Existing Copy Alert Message values will be copied and applied to new, stock Alert Types that occurs at non-renewal check-in and checkout, maintaining existing functionality. The Copy Alert field on the copy will then be cleared.

Full technical specifications for this feature are here:

http://yeti.esilibrary.com/dev/public/techspecs/alert-matrix.html

Revision history for this message
Galen Charlton (gmc) wrote :

The branch collab/gmcharlt/lp1676608_copy_alerts contains an implementation that has been tested by the library sponsoring work on this feature. The branch is rebased against master as of today, but is need in complete.

tags: added: alerts copies
Revision history for this message
Galen Charlton (gmc) wrote :

The last clause in my last comment should read "but is in need of some cleanup".

Revision history for this message
Mike Rylander (mrylander) wrote :
tags: added: pullrequest
Revision history for this message
Kathy Lussier (klussier) wrote :

Mike - Would you be able to rebase this against current master again? I'm getting merge conflicts.

Revision history for this message
Mike Rylander (mrylander) wrote :
Kathy Lussier (klussier)
Changed in evergreen:
assignee: nobody → Kathy Lussier (klussier)
Revision history for this message
Kathy Lussier (klussier) wrote :

Thank you Mike!

I've run into a few problems with this branch.

1. There is a problem with the Item Status screen. If I click the 'view' link from the copy on a bib record page or if I try to scan a barcode in the item status screen, I get no copy details in the item status screen. Also, if I click the 'edit' link from the copy on a bib record page, it retrieves the item status interface instead of the copy editor. I see the following in the Console in all three cases:

angular.min.js:119 TypeError: Cannot read property 'copy_alerts' of undefined
    at item.js:155
    at angular.min.js:132
    at m.$eval (angular.min.js:147)
    at m.$digest (angular.min.js:144)
    at angular.min.js:147
    at f (angular.min.js:45)
    at angular.min.js:48

2. When I edit a copy or add volumes/copies, the Copy Alert button and the Copy Tag button are disabled. I'm unfamiliar with the Copy Tag feature, but I'm assuming this is supposed to be active too. I am using a superuser login, so it's not a permissions issue.

With those two issues, I don't think there is a way to add an alert to a copy to do further testing on alerts applied to copies. I'll see if I can get some testing done to suppress certain alerts for certain events.

Kathy Lussier (klussier)
tags: added: needsreleasenote
Revision history for this message
Kathy Lussier (klussier) wrote :

If I try to create a Copy Alert Suppression event, nothing appears in the alert type dropdown menu. I have verified there are alert types available (the sample ones that came with the branch) in the database. They also display in the Copy Alert Types admin interface.

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

Kathy,

There were a /tonne/ of conflicts, some caused by the separate of the new egItem service. I'm certain some of my rebasing missed stuff, and it's obvious some wires got crossed as well with functions that bring up new interfaces.

I can try again tomorrow morning, and may pull in others to look as well. It may be easier to handle if it's all squashed first, though. I'll update when I have something to look at...

Thanks for testing. :(

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

s/separate/seperation/ ... no type gud today.

Revision history for this message
Galen Charlton (gmc) wrote :

Kathy and Mike: given the merge conflicts, the wide-ranging changes to circulation status and alerts managing introduced by this branch, and the lack of community feedback or attention, I suggest that we declare victory... for 3.1. In other words, I propose that we bump this back to target 3.next and plan on making a special effort (maybe a dedicated dev meeting?) in the late Fall to work on this and ensure that everybody is on the same page.

Revision history for this message
Kathy Lussier (klussier) wrote :

I understand your concerns, but, all the same, I'm not too happy about pushing this project back since it's been in the works for such a long time. If we need to hold off on it, I think we need a smaller branch that adds an alert field to the copy editor to store our current-day alerts. I think it was okay to go without one while the web client was still in a pilot phase, but now that we're calling it production ready, I think people will need to be able to add alerts to their copies.

Changed in evergreen:
assignee: Kathy Lussier (klussier) → nobody
Revision history for this message
Galen Charlton (gmc) wrote :

I understand your frustration; this has been a long project. We can commit to ensuring a branch is created to implement support for legacy copy alert messages to achieve XUL parity and ready to be merged for 3.0. Per the note I sent to open-ils-dev, such a branch would fall on the list of exceptions to today's feature freeze deadline.

Revision history for this message
Kathy Lussier (klussier) wrote :

So that we do not lose track of the copy alert field for 3.0, I have filed a new bug at https://bugs.launchpad.net/evergreen/+bug/1719967

Revision history for this message
Galen Charlton (gmc) wrote :

I've pushed a rebase branch that fixes the errors Kathy saw here:

collab/gmcharlt/lp1676608_copy_alerts_rebase_2017_11

Revision history for this message
Dan Wells (dbw2) wrote :

I checked this out a bit on Blake's test machine, and have just a couple quick reactions to share.

1) We have settings of "At Owning Library?" and "At Circulation Library?" with options of "Do not care" and "Yes". This seems awkward. One possible fix here would be to reword the settings as "Only at Owning Library?", etc. (adding the word "Only"), which would allow the answers to change to the more typical "Yes"/"No" or "True"/"False". We might even go further and have it read "Show Only at Owning Library?" or "Alert Only at Owning Library?" if we want to make it even more expressive. I can construct the simple patch needed for this if we agree on a direction.

2) I noticed the "event" name is listed in the alert. I can see how this might be useful, but it feels a bit "debuggy" rather than natural and useful in the average case. I also notice it did not display in the doc screenshots, so somebody made the conscious decision to add it, so I wonder what the expressed need was. If we don't want to remove it outright, I can think of a number of ways to de-emphasize it vs. the actual alert message, which seems useful to me.

Other than these small things, looks great. My mind is going over ways this might help us with reserve check-in/check-out. Thank you for making this happen.

Revision history for this message
Blake GH (bmagic) wrote :

From the testing that we have done, it looks like the functionality is all there. When the dropdown menus become longer it would be nice if the list was alphabetical. See screen shot.

Revision history for this message
Blake GH (bmagic) wrote :

More thoughts:

1. The "Manage" button should probably not be clickable, or removed from the UI when there are 0 copy alerts to manage.

2. The "Add" and "Manage" buttons appear on the opposite side of the label in the "Item Status" -> Detailed View (see attached screenshot)

All in all though, I am liking this feature more and more!

Dan Wells (dbw2)
Changed in evergreen:
milestone: 3.next → 3.1-beta
Revision history for this message
Galen Charlton (gmc) wrote :

The branch collab/gmcharlt/lp1676608_copy_alerts_rebase_2018_02 has the latest and greatest, consistently of a squash of the previous branch, release notes, and patches to address feedback given during the hackfest. In particular:

- I've tweaked the labeling of the At Circulation and At Ownining Library fields in the copy alert type editor
- Copy alert types are now sorted in modal drop-downs
- Copy alerts are now sorted by ID in the management modal
- I've tweaked the location of the copy alert buttons on the item status detail view
- The Manage button on the item status detail view is now enabled only when there are copy alerts to manage. Also, refreshes are handled better.
- I've fixed a bug in egPCRUD's handling of the apply action

I did not remove the event from the copy alerts acknowledgement dialog, as my notes indicated that that was specifically requested to be there. That said, I'm open to patches to tweak how it styled.

With this, I now ask for a final round of review and merging to master.

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

Just a note about the "Do not care" option, that sets the flag to NULL, not FALSE. I need to look at the code to provide the full context (and my desktop machine just fell over), but that's intentionally distinct from either TRUE or FALSE. More in a bit...

Revision history for this message
Kathy Lussier (klussier) wrote :

Can somebody show me an example of the event that displays in the copy alerts acknowledgement dialog? I'm sure I was probably the person who requested it, but I am having a difficult time finding any reference to it in my notes.

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

Background:

OK, so the idea with the "at" restrictions is that if either or both of the "At blah" flags is set, it will only allow the alert to get back to the client if the context org (say, the circulation's circ lib) is equal to or a descendent of copy circ or owning lib (per the flag), as Dan's comment and suggestion implies.

If both are TRUE, but the owning lib is outside the scope, then it will /not/ send the alert to the client. Likewise, if both are TRUE and the circulation's circ lib is equal to or a descendent of the copy owning lib but is not for the copy's circ lib, it will not send the alert to the client. IOW, /all/ conditions defined on the alert must be met, and so these flags are more about denying the alert than allowing the alert at given locations. That was the thinking behind the label "Do not care" -- that means "we'll accept it" rather than "we DO (or DO NOT) want to see it".

I'm fine with the changes as they stand if they seem clear enough to everyone else, though I'll offer one (very late) option to consider:

  Event location matches restriction locations? [invert=Cannot match|Must match, default=Must match]
  Event can only occur {invert ? 'outside' : 'at'} copy circulation library or descendent: [at_circ=Yes|Not required, default=Not required]
  And event can only occur {invert ? 'outside' : 'at'} copy owning library or descendent: [at_owning=Yes|Not required, default=Not required]

Note: "Must match" for the first (invert) flag is FALSE. "Not required" for the other two means NULL. This is all just UI, and at_circ and at_owning would still be either TRUE or NULL.

Thoughts?

And, finally, apologies if all of this is already crystal clear to everyone, in which case, just ignore the noise. :)

Revision history for this message
Kathy Lussier (klussier) wrote :

How about something like this? You need to select a checkbox to mandate any location restrictions. Unchecked leaves it at null. The subsequent dropdown is disabled until you select the checkbox.

I think this would make it fairly easy for the user to understand what's happening here. With the Invert and Yes/Don't Care option, I had to stop and think for a few minutes to digest what was happening.

Revision history for this message
Kathy Lussier (klussier) wrote :

Adding my mockup again with the proper file extension.

Revision history for this message
Kathy Lussier (klussier) wrote :
Download full text (4.1 KiB)

I took some time to test the latest branch tonight. I'm very sorry that I didn't test the earlier branch sooner since I know it would have been better to have more time to address issues.

I did find a few issues in my testing:

1. I had a couple of problems with checkout alerts. I created a checkout alert owned by the consortium that would display for a normal checkout.
  - If an item was in transit when the item was being checked out, the item in transit alert boxes would display, and I never saw the alert message that was added to the copy.
  - If there were some other kind of alert on the for the transaction (patron exceeds max fines, copy needed to fill a hold), a COPY_ALERT_MESSAGE also displayed, but the message says "The requested copy has an alert message attached." In previous testing, the actual alert message showed, I created a screencast showing three instances of checking out the same copy with an alert message. In each case, we saw different behavior on if and how the alert displayed.

2. If I check in a copy that has an alert with a 'next status' and the copy also needs to go into transit or fill a hold, I can't force the new 'next status' to be applied to the copy. Instead, it wants to go into transit or fill the hold. The next copy status should override those actions.

3. If I try to apply the next status in situations where the item doesn't need to go somewhere else, I get the Star Trek warning alert, but the copy does check in with the new status applied. I see the following in the javascript console (I saw similar messages when trying to apply a Discard/Weed and custom Mending status too)

angular.min.js:119 TypeError: model.assign is not a function
    at ui.js:23
    at angular.min.js:160
    at f (angular.min.js:45)
    at angular.min.js:48
(anonymous) @ angular.min.js:119
circ.js:1472 Unhandled checkin copy status: 14 : Damaged

4. I'm sure this was an issue in my original testing, but I didn't notice it as a problem until now. The use case is a library checks in another library's item and then applies a temporary alert with a Next Status to send a message to the owning library (DVD needs to be cleaned, for example). The assumption is the owning library is the next one that will check in the item and apply that status. However, items sometimes get misdirected in delivery, and a third library may check in the item. There doesn't seem to be a way to proceed with the checkin without applying that next status. I think we need an option there to not apply a Next Status and proceed with the checkin.

5. Our standard system alerts (I tried Missing, Lost, Lost & Paid) are not appearing at checkin, even when I create an alert type for those events. They do appear at checkout. However, if I added them to Copy Alert Suppression grid, they do not suppress at checkout. I didn't try Claims Returned, Claims Never Checked Out or Damaged.

6. In our earlier testing, we had two Library Settings determining how to handle checkout of Lost or Long Overdue items if those alerts are suppressed. I don't see those settings on my test server, but I may just be staring right at them and not seeing them.

Everything else is fairly small (I ...

Read more...

Galen Charlton (gmc)
Changed in evergreen:
assignee: nobody → Galen Charlton (gmc)
Revision history for this message
Galen Charlton (gmc) wrote :

I've started a new collab branch: collab/gmcharlt/lp1676608_copy_alerts_rebase_2018_02-2

This addresses most of the issues; my responses are detailed below. They use Kathy's numbering from comment 24. I expect to address the remaining ones tomorrow.

3. I've fixed the warning tossed by ui.js.

4. I agree that this would be useful, but suggest that this be handled as an additional bug once this one is merged (and thus not block the overall feature for feature freeze).

5. There was a bug where suppressing the checkout of missing items alert didn't work, which I've fixed. Otherwise, suppression during checkout worked for me. As far as the checkin alerts are concerned, when I activated the relevant copy alert types, I did get new-style alerts.

6: The new OU settings and stock copy alert types are now included in the seed data, not just the upgrade scripts.

7. The copy alert type manager now knows (again) to disabled the Next Status select if the event is not 'Checkin'.

8. I've removed the old alert message field from the copy editor. The branch also includes a DB update path to transfer legacy alert messages to new normal checkin and checkout alerts. Note that the changes to the schema update are such that they are best applied to fresh master database.

9. I've tweaked it so that the new copy alerts button now uses the same editor defaults key as the legacy field, so users who set copy editor defaults should see the new button be active if they had made the old field active.

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

Kathy,

A question about (2) from your comment #24: ISTM that, as long as the copy's status is actually changed before going in transit, that's the best outcome; do you see the expected value in the transit's copy_status column after the copy is, in fact, in transit? If so, when it pops out on the other end of the transit, it will get the expected status and the next action (capture for hold, etc) will have to react appropriately.

Revision history for this message
Kathy Lussier (klussier) wrote :

Mike,

I don't remember OTTOMH, but I can load the branch again and report back.

Revision history for this message
Galen Charlton (gmc) wrote : Re: [Bug 1676608] Re: Copy Alert Persistence and Suppression Matrix

> A question about (2) from your comment #24: ISTM that, as long as the
> copy's status is actually changed before going in transit, that's the
> best outcome; do you see the expected value in the transit's copy_status
> column after the copy is, in fact, in transit? If so, when it pops out
> on the other end of the transit, it will get the expected status and the
> next action (capture for hold, etc) will have to react appropriately.

I've confirmed that this is, in fact, how it's currently behaving.

Revision history for this message
Galen Charlton (gmc) wrote :

The branch now has a commit that address item 1 from comment #24, meaning that all items on that comment should now be addressed.

Revision history for this message
Galen Charlton (gmc) wrote :

By the way, I like Kathy's suggestion in #23, but think that should be the topic of another post-merge bug.

Kathy Lussier (klussier)
Changed in evergreen:
assignee: Galen Charlton (gmc) → Kathy Lussier (klussier)
Revision history for this message
Kathy Lussier (klussier) wrote :

I'm still testing the latest branch, but I have a couple of things to report back on.

For #3, I am still getting the Star Trek alert sound and errors in my Console, regardless of the status I try to apply.

For #2, I don't agree with the expected behavior for applying a next copy status. The idea is to force this copy into that status, not to continue circulating it to people who have holds placed on the item. In the case of transits, I think it is reasonable to adopt that status if the intent of the transit is to send the item home. However, it shouldn't be going into transit to fill a hold.

The primary use case of the next-alert functionality is a scenario where a patron returns an item to a library that does not own the item and reports that there is an issue with the item (the disc needs to be cleaned, pages are falling out, it has a funky smell.) When the owning library receives that item, they can then determine if the item needs to go into mending or should be discarded or should be handled in some other way. If the pages are falling out of a book, you really don't want to circulate that item again until it has been repaired. In some cases, the next copy status is a non-holdable status, but the item will still be sent to the holds shelf or go into transit to fill a hold after that next-copy status is applied. I wouldn't want this behavior to hinge on the holdability of a status (we would want to keep mending as holdable, for example, because it might not take long to clean a disc), but we clearly don't want copies put into a non-holdable status going out to fill more holds.

For #4, I'm having second thoughts on this one. I'm thinking the best practice for next-copy-status would be to display the alert only for the owning/circulation library. However, I could see use cases where you just decide not to apply that copy status, so a follow-up bug may be worthwhile nonetheless.

Revision history for this message
Kathy Lussier (klussier) wrote :

For #5 and #6 - Alert suppression works for me now and the new-style alerts are working when expected. However, when the system alerts are not changed to active, the old-style alerts do not work when checking in a Missing, Lost or Long Overdue item. I think you said as much in my comment above. I do think these should appear since previous behavior was to show a popup alert upon checking in these items. I expect there are some libraries that will not choose (or know) to set these new alerts to active, and they will suddenly stop getting these alerts.

We do see them when checking in a Claims Returned item, so there is also an inconsistency in which old-style alerts display and which no longer appear. I didn't try the checkin or checkout of the other special status transactions.

#6 the library settings work as expected. Thanks for putting them in there!

Revision history for this message
Kathy Lussier (klussier) wrote :

Everything else looks good. Thanks Galen!

Changed in evergreen:
assignee: Kathy Lussier (klussier) → nobody
Revision history for this message
Galen Charlton (gmc) wrote :

So...

#2 - I think this should be punted into a separate bug to resolve during the beta period.

#3 - I've replaced the klaxon with a success sounds, under the assumption that with new-style copy alerts, "unexpected" copy statuses when a successful checkin has occurred can now be expected.

#4 - that settles it then... a new bug to finish discuss this :)

#5 - hmm, since April of 2016, many of the old-style copy status events are explicitly bypassed when new_copy_alerts is in effect, as is always the case in the webstaff client. See around line 3849 of Open-ILS/src/perlmods/lib/OpenILS/Application/Circ/Circulate.pm. I suggest that we ship with the status-based copy alerts active (for both new installation and during upgrades).

Revision history for this message
Galen Charlton (gmc) wrote :

I've also updated the branch, and assuming that there's a +1 for my suggestion in regarding #5, will do so again.

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

I'm a +1 to including the ZZZA script, though we may want to call it out specifically in the upgrade notes and tell people they can remove that section from the omnibus upgrade script if they plan to stick with the XUL client until the bitter end.

Also, we may want to include the same script in 3.2 to force the issue at that point.

Revision history for this message
Kathy Lussier (klussier) wrote :

I'm +1 to setting them to active as well. When we were first looking at this work, there was an expectation that most sites would still be using xul while this code was in place. By 3.1, we should be seeing a good number of libraries exclusively on the web client.

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

Kathy,

IIUC, your goal is to suppress hold capture on any alert-forced "next status". Is that correct? If so, we can add the nocapture parameter to the subsequent override checking that applies the new status, and commit this. If it's more nuanced than that (say, it depends on what "next status" is) then I think we should LP that and get this in for 3.1, as Galen suggests.

Thoughts?

Revision history for this message
Kathy Lussier (klussier) wrote :

Yes, my goal is to suppress hold capture on any alert-forced "next status."

Changed in evergreen:
assignee: nobody → Mike Rylander (mrylander)
Revision history for this message
Mike Rylander (mrylander) wrote :

Pushed to master for great justice. Thus ends the Copy Alert Persistence and Suppression Matrix project; may it forever live on in our hearts.

Changed in evergreen:
status: New → Fix Committed
assignee: Mike Rylander (mrylander) → 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.