Port Local Admin Org Unit Settings UI to Angular

Bug #1839341 reported by Kyle Huckins
20
This bug affects 4 people
Affects Status Importance Assigned to Milestone
Evergreen
Wishlist
Unassigned

Bug Description

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)
Changed in evergreen:
assignee: nobody → Kyle Huckins (khuckins)
Revision history for this message
Kyle Huckins (khuckins) wrote :

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)
Changed in evergreen:
assignee: Kyle Huckins (khuckins) → nobody
tags: added: admin-pages pullrequest
Revision history for this message
Kyle Huckins (khuckins) wrote :

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.

Revision history for this message
Jane Sandberg (sandbej) wrote :

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.

Revision history for this message
Kyle Huckins (khuckins) wrote :

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

Bill Erickson (berick)
Changed in evergreen:
milestone: none → 3.5-alpha
status: New → Confirmed
importance: Undecided → Wishlist
Revision history for this message
Chris Sharp (chrissharp123) wrote :

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

tags: added: needsrepatch
removed: pullrequest
Revision history for this message
Kyle Huckins (khuckins) wrote :

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.

Changed in evergreen:
milestone: 3.5-beta → 3.5.0
Changed in evergreen:
milestone: 3.5.0 → 3.5.1
Jane Sandberg (sandbej)
Changed in evergreen:
milestone: 3.5.1 → 3.6-beta
Revision history for this message
Andrea Neiman (aneiman) wrote :

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

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

Hi Kyle,

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

TIA!

Revision history for this message
Kyle Huckins (khuckins) wrote :

Hey Mike, Andrea,

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

tags: added: pullrequest
removed: needsrepatch
Changed in evergreen:
assignee: nobody → Jason Etheridge (phasefx)
Revision history for this message
Jason Etheridge (phasefx) wrote :

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'

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

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

Revision history for this message
Jane Sandberg (sandbej) wrote :

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

Changed in evergreen:
assignee: nobody → Jane Sandberg (sandbej)
Revision history for this message
Jane Sandberg (sandbej) wrote :

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).

Changed in evergreen:
assignee: Jane Sandberg (sandbej) → nobody
Revision history for this message
Ruth Frasur (rfrasur) wrote :
Changed in evergreen:
milestone: 3.6-beta → 3.next
Revision history for this message
Lynn Floyd (lfloyd) wrote :

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.

Revision history for this message
Terran McCanna (tmccanna) wrote :

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.

tags: added: needsrepatch
removed: pullrequest
Lynn Floyd (lfloyd)
tags: added: orgunitsettings
Kyle Huckins (khuckins)
Changed in evergreen:
assignee: nobody → Kyle Huckins (khuckins)
Revision history for this message
Kyle Huckins (khuckins) wrote :

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

Revision history for this message
Kyle Huckins (khuckins) wrote :

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

tags: added: pullrequest
removed: needsrepatch
Changed in evergreen:
assignee: Kyle Huckins (khuckins) → nobody
Revision history for this message
Terran McCanna (tmccanna) wrote :

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.

tags: added: needsrepatch
Kyle Huckins (khuckins)
Changed in evergreen:
assignee: nobody → Kyle Huckins (khuckins)
Revision history for this message
Kyle Huckins (khuckins) wrote :

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

tags: removed: needsrepatch
Changed in evergreen:
assignee: Kyle Huckins (khuckins) → nobody
Revision history for this message
Chris Sharp (chrissharp123) wrote :

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

tags: added: needsrepatch
Revision history for this message
Kyle Huckins (khuckins) wrote :

Good catch Chris, I've updated seed values

tags: removed: needsrepatch
Changed in evergreen:
assignee: nobody → Terran McCanna (tmccanna)
Revision history for this message
Terran McCanna (tmccanna) wrote :

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

Changed in evergreen:
assignee: Terran McCanna (tmccanna) → nobody
Revision history for this message
Kyle Huckins (khuckins) wrote :

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

Changed in evergreen:
assignee: nobody → Kyle Huckins (khuckins)
Revision history for this message
Llewellyn Marshall (lbmarshallv) wrote :

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.

Revision history for this message
Andrea Neiman (aneiman) wrote :

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.

Revision history for this message
Kyle Huckins (khuckins) wrote :

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

Revision history for this message
Terran McCanna (tmccanna) wrote :

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

tags: added: needsrepatch
Revision history for this message
Terran McCanna (tmccanna) wrote :

Removing pullrequest until updated patch is available.

tags: removed: pullrequest
Kyle Huckins (khuckins)
Changed in evergreen:
assignee: Kyle Huckins (khuckins) → nobody
tags: added: needswork
removed: needsrepatch
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Other bug subscribers