Port Local Admin Org Unit Settings UI to Angular

[Kyle Huckins]

Another Angular UI port, migrating the Local Administration -> Library Settings Editor from Dojo to Angular.

We'll need to support real-time filtering of the grid, viewing the history of all changes to any particular OUS, as well as handle import/export of JSON-formatted OUS.

[Kyle Huckins (khuckins)]

WIP branch here - currently the Grid, History, and Edit functionality is there. Main TODOs are the filtering, making sure the grid gets the proper info when switching context orgs, and displaying the history for all expected entries in the history log - at the moment it just wants to show CONS log entries.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/khuckins/lp1839341-angular-port-org-unit-settings-ui

[Kyle Huckins (khuckins)]

I've squashed down my commits and the above-linked branch contains the latest code. This should resolve bug 1450519 for the Angular UI - it includes a new API call, open-ils.actor.org_unit.settings.history.retrieve, which retrieves the history of an Org Unit Setting in a way that accounts for permissions. It also utilizes a feature added by Bill Erickson in bug 1850555 to cover situations where an Org Unit Setting Type refers to a Shelving Location.

[Jane Sandberg (sandbergja)]

Thanks for taking this on, Kyle! Overall, I really like it.

A few thoughts:

* The filter <input> needs an accessible label. The best options would be to add a visible label (e.g. <label>Filter<input></label>) or to add an aria-label attribute.
* For power users, it would be helpful to have a "name" column that is hidden by default, but could be enabled.
* The previous version of this screen allowed descriptions to include links, but these are not displaying properly in the Angular version (look at lib.timezone, for example)
* When I add a value, the grid update properly. When I then delete that value, the context column clears properly, but the Value remains.

And... these need not hold up this ticket, but would be good follow-up work:
* The description for "Default Classification Scheme" asks users to enter numbers, but your interface is kind enough to give users a combobox so users don't even need to know the scheme's ID. We should update the description accordingly (and look for similar descriptions).
* It would be great to have a validator to make sure that interval values are entered in the correct format.

[Kyle Huckins (khuckins)]

Thanks for catching those, Jane! I've pushed up an additional commit that should resolve those first 4 issues noted

[Chris Sharp (chrissharp123)]

Kyle,

Could you please rebase this branch onto current master? Also, I see that this fix depends on the fix to bug 1850555, which has some unresolved issues per the last comment on that bug. For now, I'm removing the pull request.

Thanks!

Chris

[Kyle Huckins (khuckins)]

Thanks Chris, I've rebased this to the current master for now, though it will still rely on bug 1850555, so those will still need to be resolved.

[Andrea Neiman (aneiman)]

Noting that bug 1850555 was merged to master on 7/19 - can the pullrequest tag be reapplied to this?

[Mike Rylander (mrylander)]

Hi Kyle,

Would you mind rebasing on master now that the code that came out of bug 1850555 is in master?

TIA!

[Kyle Huckins (khuckins)]

Hey Mike, Andrea,

I've rebased and the pullrequest tag has been re-added

[Jason Etheridge (phasefx)]

Hrmm, I'm wondering if my dev system is broken. I get this when I try to load the interface:

ERROR Error: Uncaught (in promise): Error: Cannot find module '@eg/staff/admin/local/org-unit-settings/org-unit-settings.module'
Error: Cannot find module '@eg/staff/admin/local/org-unit-settings/org-unit-settings.module'

[Mike Rylander (mrylander)]

I'm seeing the same error as Jason on a master rebase of the relevant commits.

[Jane Sandberg (sandbergja)]

I bet that it is due to the new loadChildren syntax in Angular 9/10. I can grab this to update that syntax.

[Jane Sandberg (sandbergja)]

Here's a branch with the updated loadChildren syntax: user/sandbergja/lp1839341-angular-port-org-unit-settings-ui-rebase

I also took the liberty to sign off on Kyle's commits and add a cleanup commit of my own (linting, fixing a link that wasn't working, tweaking column widths, and removing some unneeded Import statements).

[Ruth Frasur Davis (redavis)]

Will this fix https://bugs.launchpad.net/evergreen/+bug/1450519?

[Lynn Floyd (lfloyd)]

A couple of things I noticed.

1. Filtering does not filter on Group.
2. The Revert does not revert.
     Set a settings, then changed it to another setting.
     Did a revert to orginial setting, and it did not revert.

[Terran McCanna (tmccanna)]

Oddly enough, I'm not seeing the issues that Lynn mentioned in comment #15, but I am seeing others:

1- The page title should probably be "Library Settings Editor" for consistency with the Local Admin menu (and for user-friendliness)

2- I keep trying to hit Enter after typing in a filter word and it doesn't work. Tab works, but it seems like Enter should do the same thing.

3- The filter appears to be case-sensitive and it should not be. For example, "send" should bring up "Sending email address for patron notices" but it does not ("Send" does).

4- I'm a bit unclear what the import/export buttons are for. To save a backup of a library's settings? To be able to edit the settings outside of Evergreen to be able to apply settings to a new branch? A use case would be helpful for testing purposes. (I did test, and my exports & imports worked, but I'm just not sure what the intention of the feature is.)

5- Clearing a filter does not reset you to the first page of results, which I would expect it to do. For example, If I filter for "holds" then go to the second page of results, then clear the filter, it keeps me on the second page of results even though the results are different.

6- My grid changes are not saving.

I'm going to remove the pullrequest for now, as I think it needs a few more tweaks.

[Kyle Huckins (khuckins)]

I've pushed a rebase here, atop Jane's branch, which resolves the filtering issues that have been noted. I'm having trouble replicating the Revert issue and the issue with the grid not saving, though: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/khuckins/lp1839341-ou-settings-editor-angular

[Kyle Huckins (khuckins)]

Didn't mean to hit send quite yet, but to continue from above, the ranch also covers the title change. I was getting what I thought were issues with saving prior to rebasing it to the latest Evergreen master, so it's possible it was an issue that was fixed elsewhere.

As for the Import/Export buttons, they're a direct port from the Dojo UI, and it seemed best to include them for consistency's sake. My assumption is that the original use case is: if the server is going to be wiped and rebuilt entirely, but you want to preserve Library Settings, hit Export, copy and save to a local file. Once the server is rebuilt and Evergreen is installed, go to the Library Settings Editor on the new instance, hit Import, and paste the contents of your saved settings file

[Terran McCanna (tmccanna)]

Tested against current master (3.6.1+) and it's getting closer - most of the earlier reported issues are resolved, with these exceptions:

1) Context location filter is a little wonky. For example, if I have a setting for BR1 but I'm viewing at the CONS level, it is displaying the BR1 setting where I would expect it to not display anything. And if I'm viewing at the BR2 context location, and there is a setting for BR1 that isn't set for BR2, it's displaying the BR1 setting where it should not. Screenshot attached.

2) The "Actions for Selected Rows" button displays, but rows can't actually be selected and there are no actions, so it probably shouldn't be displayed.

3) Grid settings are still not saving

4) Not sure if this should be part of this port or a separate bug, but for the Default Classification Scheme setting, the description does not match the way the value needs to be entered. It would be better if the combo box listed the three possible values (which must be the text values and not the numeric values) rather than needing to begin typing. But if they must be typed, then the descriptive text should be updated to match the actual options. If you type in the numeric value or a text value that isn't recognized, it gives a successful response message, but doesn't actually update the value.

[Kyle Huckins (khuckins)]

Thanks Terran. I've resolved issues 1 and 3 that you've noted in this branch and rebased it to the latest working master(as of ~4 hours ago): https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/khuckins/lp1839341-ou-settings-editor-angular-v2

For issue 2, as far as I can tell, the button is set to disabled when there are no actions, rather than removed outright in the rest of the UI, so I'm thinking that would be its own bug, and a good idea for an enhancement for the Grid UI itself. I might be missing a new setting though.

As for issue 4, that's come up before, and a bug with a branch exists here that addresses it: https://bugs.launchpad.net/evergreen/+bug/1856097

[Chris Sharp (chrissharp123)]

Kyle,

There's a missing closing parens in Open-ILS/src/sql/Pg/950.data.seed-values.sql:

), (
    'eg.grid.asset.ouSettings', 'gui', 'object',
    oils_i18n_gettext(
        'eg.grid.asset.ouSettings',
        'Grid Config: asset.ouSettings',
        'cwst', 'label'
   <missing closing parens here>
), (

Could you please repair that and rebase?

Thanks!

Chris

[Kyle Huckins (khuckins)]

Good catch Chris, I've updated seed values

[Terran McCanna (tmccanna)]

After applying this patch on a test server, I'm seeing each entry listed twice. See attached screenshot.

[Kyle Huckins (khuckins)]

Thanks Terran, looking into this. Loaded up individually on my dev environment, I wasn't receiving this error, however upon rebasing to the latest master, it appears, so that should at least narrow down what's causing the repeated entry issue

[Llewellyn Marshall (lbmarshallv)]

I didn't encounter the glitch that Terran mentioned. I clicked through, filtered by name & context location, edited some stuff, downloaded CSV, . Here are my thoughts:
+ being able to see the name of the setting is super helpful!
+ the tooltips are a bit confusing. Like when the "edit" tooltip shows a copy of the edit button.
+ delete button being green almost tripped me up.
+ the history section is a lot nicer than the old one, but I don't think showing the ID is necessary and it would be nice to see the time a row was changed and not just the date.

[Andrea Neiman (aneiman)]

I didn't set out to test this but I encountered it on terran-master while looking at something else.

When I entered a filter value, I got duplicate rows (see screenshot). I do not see duplicate rows when the list is unfiltered.

[Kyle Huckins (khuckins)]

Noting that it appears this is happening on the initial load followed by a filter, but not after a refresh of the grid data(as would happen after changing the context org) followed by a filter

[Terran McCanna (tmccanna)]

I can confirm comments 25-27 on a fresh install.

[Terran McCanna (tmccanna)]

Removing pullrequest until updated patch is available.

[Mike Rylander (mrylander)]

I've pushed an updated branch to https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1839341-ou-settings-editor-angular-v2 with a fix for the duplicated rows post-filter, and an additional fix for an issue with typed linked objects. I have not removed the ID column because the grid requires some unique index column and that works as well as any, or tried to change the tooltips. That should really be handled in another bug that improves the grid's tooltip smarts generally, IMO.

[Mike Rylander (mrylander)]

Hit send too soon!

I'll be rebasing the branch soon, but wanted to get eyes on it now.

Thanks, all!

[Mike Rylander (mrylander)]

UPDATE: I've pushed a new branch to https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1839341-ou-settings-editor-angular-rebase that

 1) rebases against master
 2) fixes the hiding of the ID column on the history table (just an Angular syntax issue, it turns out)
 3) disables the tooltips for action columns on both grids, as that is already a feature on grids!

Adding pullrequest now!

[Jennifer Pringle (jpringle-u)]

I agree with Llewellyn that the "Delete Setting" button being green isn't ideal.

The default list of settings isn't alphabetical by group like it is in dojo and you can't sort by the columns to make it alphabetical. I'm not sure how much of an issue this is since you can put a group name in the filter (though it does bring up more results than just the group if the word is somewhere else in the setting). I know I've had to skim through the whole list more than once to try and figure out what setting I'm looking for and it's much easier when the settings are grouped, especially since I don't always remember what all the groups are.

[Lindsay Stratton (lstratton)]

Also agree that "Delete Setting" should not be green and "Update Setting" yellow. Minimally, the colors should be reversed.

It would also be helpful to separate the setting description from the Edit dialog. Having this as it's own (optional) display column would help determining which setting is needed. I fond myself often opening up edit just to figure out what the setting means.

Greater description visibility might also help towards Jennifer's comment that filters also match on keywords in the description field. It would also allow using Ctrl+F to search within filtered results.

[Tiffany Little (tslittle)]

I'm not sure if this work originally predates the existence of sorting and filtering in the Angular grids, but I think they would be beneficial here. As Jennifer said, you can't sort the columns and on first glance the master list doesn't seem to be sorted logically.

I'm also getting some odd results when I filter. Like if I search "acq", in the list I got Holds>Canceled holds/requests display age and Holds>Canceled holds/requests display count. And when searching "cat" I got GUI>Button bar.

[Terran McCanna (tmccanna)]

1. I agree that the "Update" option should be green instead of the "Delete" option. Even though I had read those comments before testing, my hand instinctively went to the Delete button to save a change because it was green.

2. I'd really like to be able to sort the name, group, setting, context, and value columns by clicking on the headers. I agree with Tiffany in comment #35 that having the Angular grid sorting and filtering options that we see in the other new admin interfaces would be good for functionality and consistency. I don't think it should replace the current filter since that one searches all the fields, but it'd be nice to have both options.

(Also comment to #35 - it's retrieving the same results as it does currently because "acq" is showing up in either the setting field or the text that appears when you click on Edit. Using the new angular grid filtering would allow filtering based on a specific field to make this more clear.)

3. The Revert option still isn't working. It saves and generates a new line in the History pop-up, but it just saves the same value rather than saving to the previous value.

4. I agree with comment #34 that having a column option that shows the description would be really nice. (I LOVE being able to see the name as a column now.)

5. Now for the biggie - I suspect there is an issue with the way it is saving true/false values which is breaking things.

Example 1: When I change the Allow Patron Self-Registration option on the test server, the OPAC does not recognize the change. It sees it as true regardless of whether it's blank, true, or false. (Noting that on a different test server without this branch, it's working fine on both TPAC and BooPAC.)

Example 2: If I set Curbside to true at the CONS level and false at BR1, it's treats it as true regardless of my workstation location. If I change CONS to false and BR1 to true, it still treats it as true even at BR2. If I change both CONS and BR1 to false, with nothing set for BR2, it's still acting as if it's true for BR2.

It's almost acting like if the setting were EVER true, then it will always be true.

I am logging out and clearing cache after changing settings each time. I even tried logging in as a different user from a different browser and a different branch workstation and it's still treating the setting as true.

[Mike Rylander (mrylander)]

Hi all, I've pushed an update to the branch that addresses the issues above.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1839341-ou-settings-editor-angular-rebase

From comment #36:

1) I've swapped both the position and color of the Update and Delete buttons, so that Update is green and first, Delete is orange and last.

2) I have enabled sort and multisort (available via Manage Columns) for the grid, using the local-sort functionality (no server-side sorting, but that's ok because we fetch all and redraw when necessary). You can click the column headers to I have /not/ enabled the built-in filters, because it looks like it will take a good bit of work on the server side to support that, but the top filter does pretty well for now.

3) I fixed Revert -- it was pulling the "old" new value, rather than the original value.

4) I've added the Description as an initially hidden column.

5) I fixed the boolean issue. Booleans were being passed as strings, and we needed to make them actual JSON true/false objects.

[Bill Erickson (berick)]

Here's another branch:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/berick/lp1839341-ou-settings-editor-angular-signoff

1. Includes signoffs to existing commits.
2. Adds a commit with a number of improvements to the batch export/import process

(For ref https://git.evergreen-ils.org/?p=working/Evergreen.git;a=commit;h=2537a4642201f1fc76d8e08582e0cee829810e48)

I think one more eye on my changes and we're good to go.

[Mike Rylander (mrylander)]

I have another followup branch!

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1839341-ou-settings-editor-angular-signoff-also

This signs off on Bill's addition, and adds 2 commits to it.

The first addresses a recurrence of the duplicate row issue -- the grid is asking for a page of rows two times, concurrently, so we check for an in-flight request and silence it -- and provides a sane initial sort of settings by group and then label.

The second commit teaches the grid how to hide the selection count and "action for selected" menu when row selection has been explicitly disabled for the grid. This could be dropped by the committer if desired.

I ... think that's it for YAOUS editing. In 3.9, at least.

Of specific note: because of how the API works, per-column filtering is not possible today. Hopefully with the combination of sorting and top-row filtering, this is not a deal breaker.

Thanks, Bill and all!

[Jennifer Pringle (jpringle-u)]

The library settings editor is looking great when signed in as a global admin, but when I sign in as a System Administrator none of the settings are visible.

I can view and update settings with the same user on another test server in the old library settings editor.

Based on the console error I'm guessing it's a permissions issue:

main.953b188b933f6c37946e.js:1 ERROR TypeError: _.view_perm(...).code is not a function
    at o.allocateSettingTypes (8973.0bafb242068d76e1f2fc.js:2:11619)
    at P.pcrud.retrieveAll.subscribe.refreshSettings [as _next] (8973.0bafb242068d76e1f2fc.js:2:9324)
    at P.__tryOrUnsub (main.953b188b933f6c37946e.js:1:455524)
    at P.next (main.953b188b933f6c37946e.js:1:454753)
    at j._next (main.953b188b933f6c37946e.js:1:453925)
    at j.next (main.953b188b933f6c37946e.js:1:453699)
    at P._next (main.953b188b933f6c37946e.js:1:502660)
    at P.__tryOrUnsub (main.953b188b933f6c37946e.js:1:455524)
    at P.next (main.953b188b933f6c37946e.js:1:454753)
    at j._next (main.953b188b933f6c37946e.js:1:453925)

[Jennifer Pringle (jpringle-u)]

There's an additional issue with the boolean. Currently False works the same as True.

For example, set the library setting "Upload Create PO" to False and then go to Acquisitions -> Load MARC Order Records. The Create Purchase Order box at the top of the page is checked. It should not be checked when set to False. It is also checked when set to True (which is the expected behaviour.) The only way to have the box unchecked by default is to delete the value for "Upload Create PO".

When "Upload Create PO" is set to False in the dojo library settings editor the box is not checked in Load MARC Order Records.

This happens with every true/false setting I've tested. Some other examples are:
"Allow others to use patron account (privacy waiver)"
"Allow Patron Self-Registration"
"Default showing suggested patron registration fields" (you have to log in and out for the setting to take affect.)

[Jennifer Pringle (jpringle-u)]

I've tested the rest of the library settings editor.

The initial display is now alphabetical and the column sort works great! The filter and context location work, Import/Export, and Revert all work as expected.

The Edit pop up looks good with the button colours fixed.

We're down to just the two outstanding issues: True/False doesn't work as expected and only Global Admin accounts can see library settings.

[Jennifer Pringle (jpringle-u)]

I did a bit more testing around the accounts and can confirm that System Administrators, Local Administrators, Circulation Administrators, Cataloging Administrators, and Acquisitions Administrators can all view the Library Settings Editor in dojo. The Concerto users have VIEW_ORG_SETTINGS granted to Staff at a depth of 1.

System Administrators and Local Administrators can edit library settings in dojo. I'm guessing the Everything permission gives them this ability as neither permission group has UPDATE_ORG_SETTINGS granted.

I played around with the permissions, updating the VIEW_ORG_SETTTINGS to a depth of 0 and giving System Administrator UPDATE_ORG_SETTING at a depth of 0. I could still only see the library settings when logged in as a global admin.

[Mike Rylander (mrylander)]

Hi all,

Addressing the above-mentioned issues:

 * Boolean "false" is true => While the "revert" version of this problem was addressed before, I have now addressed the direct setting version of the issue. The Acquisitions -> Load MARC Order Records example is working correctly for me.

 * The permission issue exists because the ppl class (permission.perm_list table) requires one of CREATE_PERM, UPDATE_PERM, or DELETE_PERM, applied globally, in order to retrieve the extant permissions. We need to add VIEW_PERMISSION, which is granted to Staff by default, to that list for pcrud retrieval of ppl objects.

I'll push a branch implementing these fixes soon.

[Mike Rylander (mrylander)]

Branch as promised:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/miker/lp1839341-ou-settings-editor-angular-rebase-perms-bools

[Susan Morrison (smorrison425)]

Tested various settings with true/false values and all worked as expected, though the revert option is not working completely: If I changed the value a couple times, I could revert to the previous value, but it did not allow a revert back to the original setting and likewise did not allow a revert after the setting is deleted. For example, if I change a setting from null to true, I cannot revert back to null. I can only delete the setting. If I change a setting 1) from (original) null to true and then 2) from true to false, I can revert from false to true but not from true to null.

Also, in the History window dropdown, the option to Save Grid Settings is missing - I don't know if anyone would want to use that setting but noting in case.

[Terran McCanna (tmccanna)]

I'm certainly open to argument, but in my opinion the improvements to this interface are worth moving forward with even with the minor usability issues in comment 46. Another minor issue is that if you attempt to edit as a non-authorized user, it says it succeeded when it did not (this behavior occurs in multiple interfaces). I'll go ahead and add my signoff and create new bug reports for the other issues but if others disagree with moving forward, please speak up:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mccanna/1839341-ou-settings-editor-angular-rebase-perms-bools-signoff

[Jane Sandberg (sandbergja)]

Merged for inclusion in 3.10. Thanks, Kyle, Bill, Mike, and Terran! I also added a small follow-up commit to avoid throwing a console error when reverting an org value that was previously null (for example, reverting the first time a value was applied for a particular org unit).