Web Client: Patron Alerts not showing to staff from other libraries

[Terran McCanna]

In the web client (3.0.1), alerts created under Messages > Apply Penalty / Message are only appearing in the patron summary bar and on the main alerts page (Other > Display Alert and Messages) to staff that are logged in to workstations registered at the same location.

In the xul client, they appeared to all staff regardless of where they were logged in.

We need all alerts created via Messages > Apply Penalty / Message to appear to all staff like they did in the xul client as we rely on these alerts to share information.

[Chris Sharp (chrissharp123)]

I can confirm this behavior in current master.

[Jason Etheridge (phasefx)]

For the alerts that show up everywhere, is the Library field on the penalty set to the workstation library or the Consortium?

[Jason Etheridge (phasefx)]

For reference, both XUL and the web client are setting the library field to the workstation library, so I would expect for penalties thus created to only be visible for that library and descendents.

[Jason Etheridge (phasefx)]

Oops, forgot about the depth field on the penalty types themselves. That would also influence things; maybe that's where the breakage is if there's a difference between the two environments?

[Jason Etheridge (phasefx)]

Okay, stock system, created a STAFF_CHR penalty at BR1 workstation for the admin user through the web client. I see the penalty in red in the sidebar. In the XUL client I get the stop sign page.

If I switch both environments to a BR3 workstation, I get no stop sign page in the XUL client, and no sidebar modification in the web client. But in the web client, I do still see the penalty in the Messages section, whereas it's not listed in the XUL client.

If I manually change the org_depth on STAFF_CHR from null to 0 and re-try the experiment, I see no change in behavior. :(

[Chris Sharp (chrissharp123)]

Jason, I'm seeing the same issue. Setting the depth seems to make no difference.

[Chris Sharp (chrissharp123)]

Setting to High priority. In a consortium like PINES, it is vital for our circ staff to be able to see these alerts from any library.

[Bill Erickson (berick)]

Issue confirmed. Problem appears to the browser client not applying the correct org unit at message create time. Fix en route.

[Bill Erickson (berick)]

Fix pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1729934-webstaff-penalty-apply-scope

Note that because this bug resulted in the creation of bogus data, the patch only affects penalties created in the browser after the patch is applied. Perhaps we need to write some SQL to repair penalties whose org units do not match the configured penalty type depth?

[Bill Erickson (berick)]

Regarding the question of whether we also need to clean the data, here's a query to find (still-active) penalties whose org unit does not match the org depth of the penalty type. Note that this inconsistency can also happen when a penalty type depth is modified after penalties have already been created.

SELECT COUNT(*),
    DATE_TRUNC('year', ausp.set_date) AS set_year,
    csp.name AS penalty_type,
    csp.org_depth AS configured_depth ,
    aout.depth AS applied_depth
FROM actor.usr_standing_penalty ausp
JOIN config.standing_penalty csp ON (csp.id = ausp.standing_penalty)
JOIN actor.org_unit aou ON (aou.id = ausp.org_unit)
JOIN actor.org_unit_type aout ON (aout.id = aou.ou_type)
WHERE
    csp.org_depth IS NOT NULL
    AND aout.depth <> csp.org_depth
    AND (ausp.stop_date IS NULL OR ausp.stop_date < NOW())
GROUP BY 2, 3, 4, 5 ORDER BY 1, 2;

[Terran McCanna (tmccanna)]

Thank you, Bill! We will test Monday.

[Terran McCanna (tmccanna)]

We have applied the patch and verified that the Org Depth for the alerts is 0, but they're still not showing up as alerts in the patron summary bar. Screenshot attached.

[Terran McCanna (tmccanna)]

Scratch my last comment. I was looking at the Org Depth and not at the Library and the Org Depth was already 0. I see now that when I create a new alert, it sets the Library to PINES and it does show up for everyone. Is there not a way for the Library to continue to store which libraries added the alerts, but to show all of them regardless of which library added them?

[Chris Sharp (chrissharp123)]

I'll mention related bug 1617318 (originally reported as collections API not creating a note because we didn't realize that it was creating a branch-level penalty) because the problem there is that we want the penalty to show everywhere (so we require a depth of "0"), but then we don't know which library the penalty should be associated with. So this particular facet of the issue pre-dates the web client.

[Terran McCanna (tmccanna)]

What I believe is the root of the problem is that the code is ignoring the Org Depth setting.

From a logical standpoint, I expect anything set to Org Depth of 0 should appear to all staff in the consortium and it shouldn't even look at the library name. Then, anything set to Org Depth 1 would display to only staff at the same system, and anything set to Org Depth 2 would appear only to staff at that branch.

We have all of our alerts set to Org Depth of 0, so they should be appearing to all libraries, but they are appearing as if they are set at Org Depth of 2.

[Mike Rylander (mrylander)]

So, to boil this all down, what's desired is:

1) don't actually adjust ausp.org_unit around line 2918 in OpenILS/Application/Actor.pm
2) have the code around 3071 of the same file filter based on the computed full path of ausp.org_unit at csp.org_depth (that currently looks non-trival, btw...)

Is that correct? And am I correct that the requested change is specifically so that the penalty retains information about which library put a patron into collections?

If so, I just want to mention, as noted on the other bug pointed to by Chris, that the collections information is separately available, and could be surfaced for this purpose. That would allow the penalty itself to be set to depth=0 and work as the alert code stands today without potentially breaking other alert uses. One of the design benefits of how it works today (and risks of changing how it works for one use case) is you avoid duplicate alerts with overlapping effective scopes, which certainly reduces confusion for staff, I think.

[Mike Rylander (mrylander)]

I've given this some more thought, I think I've come up with a way to give PINES what they want WRT "who set this penalty" while retaining the benefits of the existing design.

Really, it seems obvious now that I'm writing it down... c'est la vie.

1) add a new column to ausp.org_unit called originating_org_unit that captures the ws_ou in open-ils.actor.user.penalty.apply
2) surface that in appropriate UIs (via IDL addition, fleshing, minor UI addition) next to ausp.org_unit

For backfilling that data we can either copy ausp.org_unit, or leave it NULL. I lean pretty strongly towards towards NULL for both technical (less painful upgrade) and semantics (we really don't know, and already have the ausp.org_unit "effective org" data).

Thoughts?

[Cesar V (cesardv)]

Tested and signed off on Bill's fix here:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/cesardv/berick_lp1729934-webstaff-penalty-apply-scope_signoff

Thanks Bill, that fixes the issue going forward at least.

[Chris Sharp (chrissharp123)]

Mike,

That sounds like a good and reasonable option to me.

Thanks!

[Terran McCanna (tmccanna)]

+1

[Jason Stephenson (jstephenson)]

OK. So, what's the deal here?

We've got a signed off branch from Bill Erickson that apparently doesn't really address the problem and a discussion about a better way to fix the issue.

Should the pullrequest tag be removed from this bug, or is it OK for someone to review and push the signed off branch?

[Terran McCanna (tmccanna)]

I've removed the pullrequest tag. It would have solved one problem but created another problem.

[Bill Erickson (berick)]

I think we're getting a little lost on this one..

My understanding of the patch I posted is it makes the browser client behave like the XUL client. The sticking point is it only works for /new/ data. Data created in the browser client before this patch is applied will continue to be displayed wrongly, until the data is fixed. (See my SQL from above about that).

What's more, the longer we go without apply my patch, the more bogus data we are creating.

Mike's proposal for tracking which branch applied the penalty (regardless of the depth) sounds great to me, but it's also a new feature. I propose we fix the existing data-destroying bug first, then improve the functionality.

[Terran McCanna (tmccanna)]

We've talked this over on the PINES team and agree that although this patch doesn't resolve the underlying problems, it does create a workable solution for the moment. We'll test more thoroughly over the next few days.

[Terran McCanna (tmccanna)]

Thanks Bill, I'll sign off on this and create a new Wish List ticket for improvements.

I have tested this code and consent to signing off on it with my name, Terran McCanna, and my email address, <email address hidden>

[Terran McCanna (tmccanna)]

Wish list added here: https://bugs.launchpad.net/evergreen/+bug/1757461

[Kathy Lussier (klussier)]

I'm just catching up to this discussion. Since I was looking at patron messages anyway, I decided to give Bill's patch a try. FWIW, I have not tried the patch with a depth applied to the penalty type.

I tested the patch on a stock system where the default ALERT_NOTE, SILENT_NOTE, and STAFF_C standing penalties have nothing set in the depth field.

In the xul client, if you applied a note, alert or block without the depth field set, those notes only displayed at the branch. In the web client, with this patch, I am seeing the notes, alerts, blocks displaying across the consortium. I applied these messages/penalties after the patch was loaded.

I always hated the previous xul behavior and feel a little funny reporting this change as a problem, but if we're looking to restore the previous behavior, it doesn't seem to be working as expected with stock settings for those penalty types.

[Terran McCanna (tmccanna)]

Our thought at PINES is that this is good enough for the moment because it does allow us to show all the alerts and blocks to everyone(we don't want anything to be visible solely at the branch level at this time). Since the MassLNC Development Initiative group is tackling further development as part of the project to merge all alerts and notes into one place, I'm fine with this behavior until we can get the larger project done.

[Kathy Lussier (klussier)]

Yeah, I'm okay with it too, but my concern is with those libraries that may have been using these messages all along with the stock settings with the understanding that it would only display at the branch. I don't know if we're going against somebody's expectations for desired behavior by making this change.

[Kathy Lussier (klussier)]

OK, I've had a chance to take a more thorough look at this.

The alerts are displaying as expected when the depth field is null as well as when there is a value there. That all looks good.

What I was reporting yesterday is that the notes continue to display in the messages interface regardless of what value is in the library field. For example, I created an alert for BR4 when the depth was set to 2. The alert does not display when I retrieve the patron on a BR1 workstation, but it does display in the Messages interface. In the xul client, it doesn't display.

Bill, I'm happy to sign off on the patch the way it is and file a new bug on the other issue. However, since there is a freeze on pushing new code anyway, if this is something you're willing and able to address in this patch, let me know. I'll commit to testing the new patch and hold off on signing off on it for now.

[Bill Erickson (berick)]

I have confirmed the display issue, Kathy. It is also behaving differently from the XUL client.
 Creating secondary patch...

[Bill Erickson (berick)]

Second commit pushed to same branch. Branch also rebased to master. This recovers the XUL-like behavior of only display org-scoped messages.

From the commit:

To test:

[1] Create a patron Message which applies to a branch using a branch or null-depth penalty (e.g. at BR1).

[2] Create a second message that links to the root org unit (e.g. CONS) via depth=0 penalty.

[2] Log out and back in to the browser client at a workstation under a different section of the org unit tree (e.g. BR3) and confirm the Messages created at BR1 do not display, but the messages created at CONS do.

[Bill Erickson (berick)]

Oops, I lost a sign-off with my branch push. Fixing that now.

[Bill Erickson (berick)]

New branch pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1729934-webstaff-penalty-scope-repairs

The 1st commit now has Cesar's sign-off. The 2nd commit needs a new sign off.

[Terran McCanna (tmccanna)]

We have the updated patch in production in PINES and it's working well. Chris wrote a script that took all of the old pre-change messages and changed them to the consortium level and tacked on the staff id and branch of the person who originally created them.

I have tested this code and consent to signing off on it with my name, Terran McCanna, and my email address, <email address hidden>

[Kathy Lussier (klussier)]

It works for me as well! Sorry Terran! I merged the changes before I saw your signoff, so it got missed in the commit.

Merged to master, release 3.1 and release 3.0.