Webstaff: Claims returned/neverchecked clobbered by edit in alt tab

[Bill Erickson]

Bug #929172 take II -- This time it's personal (and webstaff).

Steps to reproduce:

1. Open a patron in the webby that has items checkout out: Tab 1.
2. Open a second tab to the edit interface for the same patron (e.g. ctrl-click the Edit tab link in Tab 1): Tab 2.
3. In Tab 1, mark an item as claims returned.
4. Confirm actor.usr.claims_returned_count value successfully incremented in the DB for the affected user.
5. In tab 2, note the claims_returned_count has not been incremented (as one might expect), make a change in the edit form (e.g. edit the alert message), then save the user.
6. Confirm the actor.usr.claims_returned_count value returned to its previous state.

One proposed but abandoned fix from bug #929172 that likely would have prevented this is to ensure the affected patron's last_xact_id value is updated when a claims returned or never-checked-out event occurs. With that, the secondary save will fail and force a page reload.

Normally, last_xact_id is set from open-ils.cstore/pcrud. Since the update occurs in a stored procedure, those services are bypassed. We could teach the action.circulation_claims_returned() DB function to do it. Or following the lead from #929172, add a dummy update via cstore into the API's to force the claims returned / checked counts to update the last_xact_id.

[Jason Boyer (jboyer)]

Another option that would address this is making use of incremental updates rather than full. As it is now, if you only change a single field and click Save, (almost) every field has its value specified in the UPDATE statement that's sent to the db. Changing that still wouldn't make it a *good* idea to modify the same record in multiple tabs simultaneously but it would make data trampling issues a lot more rare.

(Not saying kicking cstore/pcrud isn't a fine way to take care of it at the moment; just raising the question since I don't know if it's come up before.)

[Bill Erickson (berick)]

Jason, there's an open-ils.storage "remote_update" API for just this type of thing. Instead of passing the IDL object, the caller passes a hash of fields to update. IIRC, it was added for (pre-2.10) password updates. I don't believe this has been ported to cstore. In any event, I agree field-specific updates would improve the likelihood of clobberation. It would also be a significant undertaking at the API and UI level.

Of the remaining options, modifying action.circulation_claims_returned() could be problematic because it does not (to my knowledge) have access to the same transaction identifer string that is normally used by cstore to set the last_xact_id value. Though we could probably use any random string that has roughly the same shape/size.

Modifying the mark claims-returned and never-checked-out middle layer code seems a little hacky, but would get the job done using the standard last_xact_id update mechanism. Note: this requires 2 edits, one per mark-as operation.

[Bill Erickson (berick)]

*reduce the likelihood of clobberation.

[Bill Erickson (berick)]

Fixes pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1694529-clobbered-claims

I took the original route started in bug #929172, no-op updates from the middle layer, as it seemed most expedient. Branch contains 3 commits: claims returned fix, claims never checked out fix, and an improvement to the browser client handling of XACT_COLLISION events -- it shows an alert dialog now, instead of a debug JSON alert() dump.

See bug description for test steps. Now when the patron is saved, an alert dialog is shown to the user before the page reloads to fetch the updated data.

[Galen Charlton (gmc)]

Thanks, Bill! Pushed to master and rel_2_12 along with a tweak of the record collision error message.