Wishlist: Store web staff workstation settings on the server

[Bill Erickson]

This idea has come up in various conversations (e.g. the hackaway). I was unable to find an LP bug or hackaway notes, though, so I'm staring over from scratch. If someone has notes, please add them to the bug.

The general idea is that values for staff workstation settings that must persist over time should be stored within the Evergreen server and not within the browser. These are things like column settings (see also bug #1702929), locally modified print templates, etc. Storing on the server lets us avoid the problem of settings getting wiped by clearing browser data.

Additionally, it should be possible for admins to provide default values for some settings and it should be possible for admins to prevent staff from applying values to certain settings.

It should be possible to store settings at the user level, the workstation level, and at the org unit level.

The settings in question are currently stored in localStorage or Hatch. Any values stored just for caching purposes or in cookies or sessionStorage will not be stored on the server, since they are intentionally ephemeral.

What we need:

* A way to link settings to users, workstations, and org units.
* A way to define precedence between them.
* A way to control who can apply values on a per-setting level.
* A way to apply values from the client.
* A migration path for current settings
* Removal of data storage options in Hatch. IOW, make Hatch printing only.

I suspect I've missed a few things. Input appreciated.

[Mike Rylander (mrylander)]

One thing to keep in mind is that we'll still want to keep a local copy (in localStorage or lovefield) of some settings, as they may be used in the offline UI. In particular, I suspect the user registration page may need some workstation settings. I believe we are caching YAOUSen in lovefield already, though.

[Terran McCanna (tmccanna)]

Yes, please! Library staff are becoming more and more mobile, and the ability to switch between computers seamlessly without needing to configure local settings on each one will make the staff client much more user-friendly and appealing.

[Bill Erickson (berick)]

Question for all:

Given that a default value can be applied to any setting at the org unit level, is there a compelling reason for a given setting to also be applied at both the user and workstation level? Or can we safely assume every setting is either a user setting or a workstation setting?

By workstation setting, I mean the value is linked to a row in actor.workstation.

Allowing settings to apply to both users and workstations adds a bit of complexity to the code and UI. Non-admin users would have to make decisions at various points about where settings should be saved. Not to mention the different ways to determine precedence at run time.

Before we go forth and code, I'd like to see if we can avoid this. To clarify, I propose we make it possible for any setting to be a workstation or user setting (via local configuration), but never both. And all settings can have default values applied at the org unit level (with permission).

Thoughts?

[Mike Rylander (mrylander)]

Bill, you bring up a good point, and I think having a switch or flag on a setting-type (that is, type of setting, not setting-ish) table to specific workstation or user seems straight forward. +1

However, I wonder if we need to step back and define some terms, first. Primarily, what is meant by "setting" in this context. We currently have: org settings (YAOUS), user settings (opt-in for stuff, record defaults), in-client "settings" (sticky checkboxes, grid column definitions), things that are not settings but live beside the last type (last record retrieved, etc), settings that live in several places (receipt and other print templates), and maybe more that I'm forgetting.

I think before we can even start designing a solution we need to clarify exactly what we mean by "settings" lest we end up with something like the Windows Registry (or, ugh, systemd) catching all manner of random "things".

I'm almost always for the most general solution, but we've got four general solutions right now and I hope we avoid simply adding a fifth and moving the home of some existing settings over to that.

[Bill Erickson (berick)]

Hey, Mike. Thanks for the reply. "Settings" definitely doesn't capture it...

The key concern for this bug is all the data that may be saved in the browser client that are only persisted in-browser (or Hatch), primarily in localStorage, sessionStorage, and Hatch files. It is a smorgasbord of settings (sticky boxes, grid columns, etc.) and other stuff (modified print templates, etc.).

Many of these data could be persisted using existing mechanisms (org / user settings), but there are 2 new features that don't exist today: settings that follow a workstation and multi-home/cascading values (e.g. try user value first, then try tree-aware org value).

I share your concern about adding Yet Another Settings Container. And arguably some existing user settings could benefit from cascading support. So maybe we should make those tables smarter...

For the sake of discussion, I'll make a dev proposal. This is the more ambitious version.

1. Add a new actor.workstation_setting table, analogous to user_setting/org_unit_setting.

2. Add a new meta settings type table config.setting_type which absorbs and replaces config.org_unit_setting_type and config.user_setting_type (and a theoretical config.workstation_setting_type).

It contains the master list of all settings and it defines which storage options (org/user/workstation) are available to each.

[Bill Erickson (berick)]

I started a proof of concept unified settings structure based on my previous comments.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1750894-ws-settings-on-server

Recap:

1. new config.setting_type table that defines all org, user, and workstation setting types.
2. existing org and user setting type tables are now views over the new table (so we don't have to modify huge amounts of code).
3. new function actor.get_setting that returns the desired value, verifying view_perms as needed.

Pausing here for feedback before proceeding with middle-layer and UI code leveraging the new tables/function.

[Bill Erickson (berick)]

Thinking about this a little more, I believe we could accomplish the same thing with a much less disruptive approach.

1. Create config.workstation_setting_type

2. Create actor.workstation_setting.

3. We decree that any setting whose name matches across multiple setting types (org/user/workstation) is a cascading setting.

For example, if we have both an org setting and a user setting named "eg.circ.checkin.no_precat_alert", we attempt to find a user setting value first, then an org setting value.

4. We create a new stored proc that implements #3 (mostly done w/ my PoC branch above).

This way org and user settings can continue to function as-is and new code that wants to use cascading values will use the new cascade function. Existing code could be retrofitted to use the new function too if/when desired.

[Bill Erickson (berick)]

Working branch updated to reflect the above. I think this will do nicely.

[Bill Erickson (berick)]

I've pushed a few more commits to the working branch. Here's what I have so far.

1. We have workstation settings, similar to user and org unit settings.

2. We have an API that returns the best value for a setting, be it user, workstation, or org unit, depending on the context and local configuration.

3. We have an API that can apply a value to a user or workstation setting depending on local configuration.

Applying org unit settings is unchanged. The existing user settings get/set API is also unchanged for backwards compatibility and for user settings that will only ever be user-only settings.

4. The browser client (via hatch.js) is now able to get and set server-stored settings using the above APIs.

5. I've created enough sample data to move the workstation settings from the browser client checkin UI to the server. (e.g. Ignore Precataloged Items, Suppress Holds and Transits, etc.)

For now, and possibly as a part of the migration strategy, the settings that are supported on the server are hard-coded in Hatch. Until we have all settings supported/migrated on the server, this will likely be necessary to avoid chaos in the browser client.

===

TODO:

1. Create workstation settings for needed browser values. Most values set via hatch.setItem() will need a corresponding workstation setting.

* For consistency with the existing browser client code, I'm assuming we want to migrate existing browser values as workstation settings. However, sites will have the option to change setting types to user and/or org unit settings depending on the desired behavior.

2. Migration plan. At some point, the browser has to send all of its locally stored settings to the server and delete the local copies. We'll need a way to reliably indicate when it's happened so we don't repeat the process. Maybe we track the "has migrated" status with a new workstation setting.

3. Applying values to org unit settings will have to be done on a per-interface basis and it will depend on what permissions a user has and whether an org unit setting type exists for a given setting. At least for storing grid settings, though, we can build a grid widget for handling all of the grids. May want to do this as a follow-up bug once this bug is merged.

3. I suspect that removal of Hatch storage features will have to be a separate bug also, targeting one release after this code is merged.

[Bill Erickson (berick)]

Rebased working branch to current master.

Added support for storing a few more settings on the server:

eg.circ.patron.summary.collapse
circ.bills.receiptonpay
circ.renew.strict_barcode

The full supported list is now:

eg.circ.checkin.no_precat_alert
eg.circ.checkin.noop
eg.circ.checkin.void_overdues
eg.circ.checkin.auto_print_holds_transits
eg.circ.checkin.clear_expired
eg.circ.checkin.retarget_holds
eg.circ.checkin.retarget_holds_all
eg.circ.checkin.hold_as_transit
eg.circ.checkin.manual_float
circ.checkin.strict_barcode
eg.circ.patron.summary.collapse
circ.bills.receiptonpay
circ.renew.strict_barcode

TODO items from previous comment still stand.

[Bill Erickson (berick)]

Local-to-Server migration code pushed.

Migration test and deploy options added to the Stored User Preference page.

New "Server Prefs" tab added to Stored User Preference page.

[Bill Erickson (berick)]

Question about managing server persistence of grid settings... We have roughly 50 grids in the browser client today with a persist key. More will follow, of course. Do we want to say as a rule every grid gets a matching workstation (or user) setting for persistence?

If Yes, it simplifies the logic, but also means we'll have quite a few settings to add (many of which will likely be ignored, I suspect) and devs will have to add a setting any time a new grid is added.

If No, we'll need to know which grids should get settings by default. We could provide settings for a reasonable subset of the grids or potentially just start with one to act as an example.

If we don't assume every grid has a server side setting, then presumably any grid that does not have a server setting should fall back to local storage?

[Kathy Lussier (klussier)]

My first gut reaction is to say every grid gets a server setting for persistence, mainly for consistency. If some have a server side setting and some fall back to local storage, if a staff user clears their cookies or gets a new computer, there will be confusion over why some settings have stuck and some need to be redone, even if the ones that need to be redone are for rarely-used interfaces.

[Mike Rylander (mrylander)]

It's actually not too difficult to find all the static grid keys in the current code base, since we have all followed the pattern of wrapping egGrid attributes across multiple lines:

Open-ILS/src/templates/staff$ grep -r persist-key *| \
  awk '{print $2}'|grep -v '{{'|sed 's/\x27/"/g' | \
  cut -f2 -d'"'

A script to generate the SQL needed to insert any missing ones (blindly, but in a forgiving fashion) could be run on demand by devs for testing. Then also by make_release.sh and the output captured in a baseline /and/ upgrade file and every upgrade would attempt to fill gaps.

Note, there is one dynamic grid persist key in the grid editor, and keys coming from that should fall back to local, I think. (In fact, all settings that don't have a server-side type should fall back to local, IMO, and we should announce when that happens in the browser console log.)

Thoughts?

[Bill Erickson (berick)]

Thanks Kathy and Mike.

Mike, I hand't considered creating a build-time tool to generate them, but I could certainly see that being useful.

Regarding settings that fall back to local, tell me if this is what you are picturing, Mike.

1. Check if a user/ws setting type exists on the server (this can be bundled in to the setting value lookup for speed).

2. If no type exists at value change time, apply the value to local storage.

3. All subsequent value look-ups first check if a type/value exists on the server, then falls back to local storage copy.

4. If a setting type becomes available after its value was previously stored in local storage, the browser automatically migrates the value to the server setting and deletes the local copy.

Sound sane?

==

And as a reminder, there will also be a list of settings that never have server setting types. These will be things like ephemeral / cache data (last patron retrieved, etc.). These will likely be hard-coded in the browser settings lookup code (hatch) to avoid any unnecessary server calls.

[Bill Erickson (berick)]

Thinking more about this, automagic setting migration (from localStorage to server) will make the overall migration to server-side settings a lot easier. It gives us more flexibility in how/when setting types are created and allows for local/custom setting types (e.g. for custom grids) to magically work.

Note to self, going forward, cache/ephemeral settings should use a common setting name prefix so Hatch knows to avoid unnecessary server calls.

[Bill Erickson (berick)]

* Code pushed for local fall-back and auto-magic setting migration.
* Settings that are assumed to be server settings where no setting type exists warn in the logs.
* 64 settings are now covered, most of those are grids configuration settings.
* I've done a fair amount of testing, things seem happy.
* local settings are migrating, server settings (grids, etc) are using the server values, settings are cascading through user => ws => org setting values.

If anyone wants to test:

* I have not propagated SQL changes to the base schema files yet.
* There is a XXXX schema file, an YYYY data file, and a ZZZZ UNDO file to undo all of the SQL changes for this branch.

TODO:
1. Add more settings
2. Release notes
3. Open LP for adding a grid widget for publishing grid settings to org unit settings.
4. Open LP for removing data storage option from Hatch / hatch.js

Going to pause here for a bit in case anyone wants to test / review.

Branch is still at:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1750894-ws-settings-on-server

[Bill Erickson (berick)]

TODO items 2, 3, and 4 above addressed.

TODO: add more settings and propagate SQL to base schema.

[Bill Erickson (berick)]

Squashed branch pushed:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1750894-ws-settings-on-server-squash1

1. Base schema is synchronized with the upgrade script.

2. PGTAP tests added.

3. Several reviews of the browser client suggest the branch has captured the vast majority of browser client settings (125 settings as of today). Warnings are issued for missing settings.

Note to testers, as settings are accessed in an existing browser client instance, they will be automatically migrated to server settings. No manual migration is required. A console log entry will indicate when a setting has been migrated.

I think it's ready, minus loads of testing. Adding pullrequest.

[Bill Erickson (berick)]

Pushed another commit to add a "Server Workstation Prefs" tab to the workstation admin prefs page so staff can see what settings/prefs are stored on the server as workstation settings.

[Bill Erickson (berick)]

On a related note, I started working on a change to move the grid display count limit value out of localStorage and into the same config object that stores the column settings, so it too can go on the server without requiring we add another batch of workstation settings just for the limit value. I'm not sure if everyone agrees those values should be clumped together, though, so I'll leave the patch in a separate branch pending feedback.

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1750894-ws-settings-on-server-squash1-grid-limits

[Kathy Lussier (klussier)]

I haven't had a chance to test much so far, but I did notice an issue with my initial workstation registration.

Because I was using a new database, I had to register my workstation before logging in. I click the button to register it, it stores the information in my local storage and registers the workstation in the database, but the workstation name doesn't then disappear from the text-entry box as it normally does. It also doesn't appear below in the dropdown menu of registered workstations.

It does appear on the login screen, but there is no default workstation displaying on this screen. I have to click the dropdown to see the workstations.

I see the following in the Console when I first retrieve the workstation registration interface:

Error: [ngRepeat:dupes] Duplicates in a repeater are not allowed. Use 'track by' expression to specify unique keys. Repeater: ws in workstations, Duplicate key: string:BR1-masslnc, Duplicate value: BR1-masslnc
https://errors.angularjs.org/1.6.10/ngRepeat/dupes?p0=ws%20in%20workstations&p1=string%3ABR1-masslnc&p2=BR1-masslnc
    at vendor.bundle.js:6
    at vendor.bundle.js:6
    at i (vendor.bundle.js:6)
    at h.$digest (vendor.bundle.js:6)
    at vendor.bundle.js:6
    at r (vendor.bundle.js:6)
    at vendor.bundle.js:6
(anonymous) @ vendor.bundle.js:6

There are no Console errors when I attempt to register the workstation.

[Bill Erickson (berick)]

Thanks, Kathy. I'll take a look...

[Bill Erickson (berick)]

Kathy, are you testing the branch from comment #19 or the extended branch from #21? I'm leaning more toward including the grid display limit in with the grid column configuration (#21) FWIW.

[Bill Erickson (berick)]

I have force-pushed back to:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1750894-ws-settings-on-server-squash1-grid-limits

1. Rebase to current master
2. Update permission IDs to match master.
3. Fix the workstatation registration bug noted by Kathy. This fix is in the tip commit, so it's possible to cherry-pick just that fix into a working branch (if you don't want to reinstall). Requires a npm run build + install or copy.

[Kathy Lussier (klussier)]

Thanks Bill! I was testing the branch in comment #19. I'll test the other one including the grid display limit from here one out.

[Kathy Lussier (klussier)]

I tested the migration of workstation settings earlier today. I have a few bits of feedback for continuing my testing.

I confirmed that the Console displays a message when it moves settings to the server and when there isn't a matching setting type in the database. From the perspective of most end users, the migration will happen seamlessly.

I loaded lp1750894-ws-settings-on-server-squash1-grid-limits, but I'm not seeing the grid limits in the database. Bill, to be clear, when you say grid limits, you mean the number of rows to display per page, right? As far as I can tell, this setting is still being stored in local storage. I haven't seen any Console messages saying that it's been moved to the server when I had a pre-existing local storage setting, nor do I see any changes in the database when I set a new value in the checkin grid.

This was strange. I had a preexisting local storage value of true for eg.circ.checkin.clear_expired. When I used the checkin interface, this setting did not move to the database and there was no Console message saying that there wasn't a matching setting type. The checkin screen also didn't automatically default to clearing the holds shelf. However, when I then manually set this checkin modifier, the setting did get stored in the database and worked as expected when I subsequently retrieved the checkin interface. I didn't test any other checkin modifiers to see if they show the same behavior.

There were a few local storage settings I encountered that did not have an entry in config.workstation_setting_type, but probably should have one.

  * circ.checkout.strict_barcode - I think this was an oversight since every other strict barcode entry setting seems to be covered.
  * cat.holdings_show_empty_org - all checkboxes at the top of holdings view are covered except this one.
  * cat.copy.defaults
  * cat.printlabels.default_template and cat.printlabels.templates

Everything else looks great so far! Thanks Bill!

[Kathy Lussier (klussier)]

Adding a note that a tested migration of a few other checkin modifiers, and I saw the same behavior as a described for eg.circ.checkin.clear_expired

[Bill Erickson (berick)]

Thanks, Kathy!

As it stands, the grid limit data is only added to the workstation setting if the Save Columns action is chosen. Since it's a special case, it's not part of the magic settings migration process. I'm hoping since the grid limits data is easily modified in real time to fix any localStorage data loss, this is not a huge problem. I had forgotten about, though, sorry for the runaround.

I was able to reproduce the checkin page settings issue and I have pushed a fix to:

http://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1750894-ws-settings-on-server-squash1-grid-limits

In short, the batch settings lookup call was not migrating settings, only the single-lookup call.

I'll get those other settings added soon...

[Bill Erickson (berick)]

5 Additional workstation setting types pushed to same branch as my previous comment.

[Kathy Lussier (klussier)]

It all works for me now. I tested the code on a fresh install and also tested the upgrade scripts. PgTap tests all work too.

Merged to master for inclusion in 3.2. Library staff are going to be thrilled with this improvement!

Thank you Bill and also Chris Sharp, who provided test feedback in IRC!