Angular: Local Admin Circulation Policy Port

[Kyle Huckins]

Evergreen: main

We’ll be porting the Circulation Policies UI from DOJO to Angular. This appears to be a grid with a fairly extensive fm-editor, and linked (Circulation) Limit Sets. Some of the code done for the Surveys Port(bug 1845240) may be useful inspiration.

[Zavier (zavierbanks)]

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/zbanks/lp1855781-circulation-policy-configuration

The branch is ready for review.

[Jane Sandberg (sandbergja)]

Thanks very much for the branch, Zavier and Kyle. I think there are two things that this needs before being signed off:

* When I run ng lint, there are several lint errors. Most of them look pretty easy to fix (like missing semicolons, etc.)
* The dojo interface had a small line dividing fields used to match circulation policies vs. the actual policy effects. I think that some sort of separation is necessary. It doesn't have to be that little line -- actually, it would be nice to have a heading or something else that says what the groups actually are.

[Zavier (zavierbanks)]

I fixed the linting issues, and added in the dividers. Here's the branch:
https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/zbanks/lp1855781-circulation-policy-configuration

[Tiffany Little (tslittle)]

I'm looking at this on the feedback fest server and have a few observations.

* I do see the dividers, but it looks like there's no padding between the dividers and the fields, so it causes the dividers to overlap into the fields slightly.

* The Cancel and Save buttons at the bottom of the form are misaligned. Cancel is too far down.

* Aside from the Permission Group, it doesn't appear to show which fields are required. It looks like at the very least Active, Org Unit and Group can't be null but only Permission Group appears to have the "Required?" flag.

* Following up on the previous bullet, if you save the editor with only active=true and permission group set (no org unit set), the form saves successfully. However, it appears to randomly pick an org unit.

* I'm an avid proponent of help popovers, so a help popover next to the Add button for Circ Limit Set Name saying that these are set in Local Admin > Circulation Limit Sets would be lovely. But that's a wishlist item and others may not think it necessary, so not a dealbreaker for me either way.

[Joan Kranich (jkranich)]

I am also looking at the feedback fest server.

When I click in the Circ Limit Set Name nothing displays. If I start to type Test then I see the Testme set.

When I click in boxes Copy Circ Lib, Copy Owning Lib, and Copy Location a list of codes display. It would be good if the Circ Limit Set Name displayed a list.

I wasn't able to add a Circulation Modifier so I could not test the behavior of that box.

[Zavier (zavierbanks)]

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/zbanks/lp1855781-circulation-policy-configuration

Here's the updated branch.There is still an issue with the required fields however. Checkboxes
and org unit selectors cannot be labeled as required. So
there are fields that are required, but can't be labeled
as such.

[Jennifer Bruch (jbruchpails)]

Can someone check to see if this bug persists? I just ran into it on 3.3.4 for PaILS.

https://bugs.launchpad.net/evergreen/+bug/1771875

[Jane Sandberg (sandbergja)]

I did some more review of this branch today. It is looking good (and Jennifer, it does take care of bug 1771875), but I did catch two issues:

1) The shelving location dropdown is not showing the org unit labels next to the names of the shelving location. Right now, the dropdown includes entries like:

Stacks
Stacks
Stacks

which should actually be:

Stacks (BR1)
Stacks (CONS)

and so forth. The new <eg-item-location-select> component seems like a good tool for this task, but it's not quite ready to do so. That component takes an OU (or the workstation's OU) and generate a list of locations at that OU or its ancestors. However, an administrator might be working on shelving locations at a sibling or cousin OU, and would be unable to do so with the <eg-item-location-select> as it currently stands.

2) A few of the boolean fields can be NULL in the database. In the dojo UI, this was represented by dropdowns with the values "True", "False", and "Unset"/"Inherited". The Angular version doesn't give this option -- it only allows for True/False. Here are the ones that can be NULL:
* Renewal?
* Juvenile?
* Reference?
* Circulate?

[Jane Sandberg (sandbergja)]

Oh, and one other thing! I wasn't able to save my grid settings (additional columns, etc.)

[Lynn Floyd (lfloyd)]

I was testing this on the bug during Bug squashing week, and among the things in comment #8. I could not see any thing in the grids at all. I checked this on several different Bug Squashing Week servers.

[Terran McCanna (tmccanna)]

In addition to the issues Jane found in comment #8, I have a few more:

1) The option to "Save Grid Settings" is missing

2) The interface has the option to select multiple rows and Edit Selected rows, but it looks like that only edits one of the selected items, which is confusing.

3) I don't see an option to Delete a circulation policy row, but I don't see that in the old interface either, so that may be intentional.

I'm going to remove the pullrequest for the time being.

[Lynn Floyd (lfloyd)]

Update on refreshed version on test server during Bug squashing Week.

1. Grid settings cannot be saved.
2. Copy Location still does not contain owning Org unit
3. Nullability does not exist on fields that are nullable.

[Jason Etheridge (phasefx)]

Top five commits at collab/phasefx/lp1855781-circulation-policy-configuration

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/phasefx/lp1855781-circulation-policy-configuration

For those interested, more granular commits at:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/collab/phasefx/lp1855781-circulation-policy-configuration-wip

[Jennifer Pringle (jpringle-u)]

I noticed that the angular interface is still using the outdated term "Copy Location" (which was used in the dojo interface). This should be updated to "Shelving Location" to match the rest of Evergreen.

(based on the screenshots shown in EDU Spotlight on 2023 Evergreen Development February 16th)

[John Amundson (jamundson)]

Noting that the patch added for bug squashing week is still using the term "Copy Location" instead of "Shelving Location".

There is also a typo. The second portion of the interfaces reads "Circulation Policy Effecs" instead of "Effects"

[Terran McCanna (tmccanna)]

Removing pullrequest due to comments #14 & 15

[Lindsay Stratton (lstratton)]

Reviewing other issues noted in previous versions, all appear to be resolved and functioning as expected:

1) The shelving location dropdown is not showing the org unit labels next to the names of the shelving location. (#8, #12) -- org unit labels now display

2) A few of the boolean fields can be NULL in the database. (#8, #12) -- all fields can be Unset

3) The option to "Save Grid Settings" is missing (#9, #11, #12) -- present and works; if columns are reset but not saved, refreshing or closing the tab will return to last saved column state

4) The interface has the option to select multiple rows and Edit Selected rows, but it looks like that only edits one of the selected items, which is confusing. (#11) -- if multiple rows are selected, the Edit Selected action is inactive; the action is only valid when one row is selected

5) I don't see an option to Delete a circulation policy row, but I don't see that in the old interface either, so that may be intentional. (#11) -- deleting circ policies has never been an option. This would be REALLY useful functionality!

[Lindsay Stratton (lstratton)]

New issue, using terran-master.gapines.org server:

The list of policies cannot be filtered. As an organization that has hundreds of circ rules this is a deal breaker.

[Lindsay Stratton (lstratton)]

Minor nitpick, using terran-master.gapines.org server:

The Actions drop down only includes "Edit Selected" but there is a blank space below that *looks* like another option should be present. It is a little confusing - is just extra blank space in the list, a page load problem, or the user's permissions are not sufficient to view the second option?

[John Amundson (jamundson)]

+1 to filtering.

CW MARS has thousands of circ policies, and it is an absolute necessity to be able to filter.

[Jennifer Pringle (jpringle-u)]

The check boxes for "Renewal", "Reference", "Juvenile", and "Circulate" are confusing. There no visible way on the pop-up to tell if the value of the checkbox is No or Unset.

If you want to set "Renewal", "Reference", "Juvenile", and "Circulate" to No you have to check and then un-check the box which is not intuitive.

"Early Renewal Extends Due Date" always has a value of No when unchecked. If you click Unset for that option you can no longer save the circulation policy, since Unset isn't a valid option for that field.

[Jennifer Pringle (jpringle-u)]

+1 to filtering. This is a deal breaker for us too.

I'd also like to advocate for the library org selector to be on this page too so you can see relevant policies at glance without having to filter on columns. (The Hold Policies interface is always a jumble of policies from across our consortium anytime we open it.)

We currently have a Context Org Unit drop down on our circulation policies page (which I didn't realize until today is a local customization we've had for over a decade) and it definitely makes the interface easier to use. Our Context Org Unit drop down grabs the user's workstation OU and all ancestors of that org, then further limits that list to org units where the user has the VIEW_CIRC_MATRIX_MATCHPOINT perm.

[Lindsay Stratton (lstratton)]

+1 to a library org selector, including recognizing the user's workstation / working location perm OUs.

[Jennifer Pringle (jpringle-u)]

A few more small things

"Copy Circ Lib" and "Copy Owning Lib" should be "Item Circ Lib" and "Item Owning Lib".

Is there any reason to stick with "Lib" instead of using "Library"?

I think Org Unit should be "Check Out Library" (Even if the value that goes in is sometimes a system where the check out library is actually a branch I think it tells the user more than Org Unit does.)

I know that technically we're creating a new circ matrix match point but since we generally call them circulation policies and everywhere else in the interface does, I think the button should be "New Circulation Policy" not "New Circ Matrix Matchpoint". (It'll also be easier to document the new interface if there's consistent terminology.)

[Andrea Neiman (aneiman)]

Thanks for the feedback - we will take a look at this shortly.

[Joan Kranich (jkranich)]

I agree with consistent terminology in Jennifer's #24 comment.
And of course I agree with filtering. Filtering is necessary.

[Jason Etheridge (phasefx)]

Force pushed a new branch at collab/phasefx/lp1855781-circulation-policy-configuration

    Additional tweaks since the last pullrequest:

    * replacing checkboxes with eg-bool-select in fm-editor
    * required and validation for eg-bool-select
    * Change Copy Location to Shelving Location in the IDL

        If we wanted to catch every instance of Copy Location, we
        could do something like

        ack -li 'copy location' > /tmp/list.txt
        # vim /tmp/list.txt # some care needed with pruning the list. Worthy of its own ticket and commit, IMO
        for x in `cat /tmp/list.txt` ; do sed -i 's/copy location/Shelving Location/gi' $x ; done

    * fix a typo and relabel the new policy button
    * relabel some more fields in the IDL

        "Copy Circ Lib" and "Copy Owning Lib" become "Item Circ Library" and "Item Owning Library", respectively.

    * Org Unit becomes Checkout Library for the circ matrix. Checkout is more prevalent in the code than Check Out, but we should pick one.
    * grid filters and some cruft removal
    * org selector. We should really find a way to leverage the AdminPageComponent here rather than crib code from it.

[Jennifer Pringle (jpringle-u)]

Took a look at this on bugsquash2.mobius server - newest fixes (in comment #27) are not on this server

[Jennifer Pringle (jpringle-u)]

This looks great! I can confirm that all of the issues I reported have been resolved.

I have a bit more testing I want to do tomorrow morning but unless I run into something new and unexpected I think it's ready for a sign off.

[Jennifer Pringle (jpringle-u)]

So I found something new and unexpected.

Evergreen won't allow you to apply a Circ Limit Set to more than one policy. The error it gives is "Linked Set Name Already Exists on Another Matchpoint". The dojo circ policy interface does allow the limit sets to be re-used.

This is an issue for us as we create circ limit sets that are then used for policies for different permission groups. (So the same circ limit set is used for an individual library for their policies for Public Library Users, Public Library Staff, Public Library Juvenile, etc.)

If you have to create a unique circ limit set for each policy that multiplies our circ limit sets by at least 5 and we already have almost 400 of them (so we'd be looking at having to manage over 2000 of them). I'm also unclear what that would do at upgrade to our circ policies that where circ limit sets are already used by multiple policies.

[Jennifer Pringle (jpringle-u)]

In the drop down menus for Duration Rule, Recurring Fine Rule, and Max Fine Rule the values start to be repeated as you work with the policies interface.

See the screenshots.

[Jennifer Pringle (jpringle-u)]

In the Linked Limit Set section of the New Circulation Policy pop-up there is a question mark for a tool tip, but there's no tool tip when you hover or click on the question mark.

[Jennifer Pringle (jpringle-u)]

I'm not sure if it's my testing, the limited data, or a problem with the circulation policies code but my circulation policy with an attached limit set isn't applying the limit at check out.

Because circulation and circ policies are such a critical piece of Evergreen and the concerto dataset is limited and doesn't reflect the complex permission and org unit structures some of us have, once the issues with linked circ limit sets are fixed I think we need at least a couple consortia to test this with their own data before it gets signed off.

[Jane Sandberg (sandbergja)]

Thanks for all the work on this, folks! Here's a rebased branch with signoffs for the existing commits, with an additional commit to address the tooltip issue that Jennifer mentioned in #34: collab/sandbergja/lp1855781-circulation-policy-configuration-2 . This is an issue that affects many of the help popovers since the bootstrap upgrade -- we may need to pull that out into its own bug if we're not able to merge this soon.

Jennifer's feedback in #30, #31, and #35 are not yet addressed.

[Jennifer Pringle (jpringle-u)]

I consider the issues I reported in comments #30 and #35 to be a blockers for this feature as it appears the angular interface will break at least some existing circulation policies without fixes for those issues.

[Lindsay Stratton (lstratton)]

I also have concerns re: limit sets applied to more than one circ rule (Jennifer Pringle's comment #30).

We also have a large number of shared limit sets applied to multiple circulation policies. If shared limit sets break circ rules, that would be a massive problem - for example, jamming up circulations of A/V materials system wide.

[Jane Sandberg (sandbergja)]

I've addressed Jennifer's feedback in #30 with an additional commit at collab/sandbergja/lp1855781-circulation-policy-configuration-2

I tried the following steps to confirm that limit sets are still applying in the enhanced concerto set (#35):

1. Modify an existing limit set to limit items to 5 items
2. Use the "linked entities" link to add a shelving location to the limit set
3. Looked up a list of barcodes for items at that location
4. Tried checking them out to a single patron.
5. On the sixth such checkout I attempted, I got the PATRON_EXCEEDS_CHECKOUT_COUNT warning

More testing is required, but from a reading of the new code, it does not appear to touch how circ limit sets are applied. It just had an unnecessary restriction in the UI.

Speaking of limit sets, there is an accessibility issue: the limit set combobox, "fallthrough" checkbox, and "active" checkbox are not associated with their labels.

Also, #31 still needs to be addressed.

[Jane Sandberg (sandbergja)]

Pushed a commit to fix the accessibility issue I mention in #39. Still need to address #31.

[Jane Sandberg (sandbergja)]

#31 is addressed in collab/sandbergja/lp1855781-circulation-policy-configuration-2. This branch is now ready for some more review.

[Jason Etheridge (phasefx)]

Thanks Jane! My mistake for not grabbing ownership; we had some concurrent work happening. :-) Circ limit sets look good. I was inclined to rip out the help popover and put the help inline (see bug 2021862), but that looks good too. I'm more worried about combobox; there's been a lot of churn there in other branches, that I don't think are public yet. #31 looks addressed, though I notice we can still get duplicates with Permission Group and Circulation Modifier. I have an alternative strategy in a follow-up commit. Here's a branch with that and my sign-offs for the other changes: collab/phasefx/lp1855781-circulation-policy-configuration2

[Jane Sandberg (sandbergja)]

Thanks, Jason! I like that alternative approach -- seems much sturdier! Signoff here: user/sandbergja/lp1855781-circulation-policy-configuration3

[Chris Sharp (chrissharp123)]

I'm seeing an error when building Angular with this branch (Ubuntu 22.04):

Error: src/app/staff/sandbox/sandbox.component.html:10:31 - error TS2339: Property 'end' does not exist on type 'SandboxComponent'.

10 <eg-help-popover [placement]="end" helpText="This page is for random ng stuff!"></eg-help-popover>
                                 ~~~

  src/app/staff/sandbox/sandbox.component.ts:28:16
    28 templateUrl: 'sandbox.component.html',
                      ~~~~~~~~~~~~~~~~~~~~~~~~
    Error occurs in the template of component SandboxComponent.

Angular CLI: 15.2.10
Node: 16.17.1
Package Manager: npm 8.15.0
OS: linux x64

Angular:
...

Package Version
------------------------------------------------------
@angular-devkit/architect 0.1502.10 (cli-only)
@angular-devkit/core 15.2.10 (cli-only)
@angular-devkit/schematics 15.2.10 (cli-only)
@schematics/angular 15.2.10 (cli-only)

[Chris Sharp (chrissharp123)]

Removing signoff tag to allow for code fix.

[Jane Sandberg (sandbergja)]

Thanks, Chris! Pushed a fix to the same branch: collab/sandbergja/lp1855781-circulation-policy-configuration-2

[Terran McCanna (tmccanna)]

Jane, I'm getting a conflict when applying to main today - could you rebase please? It might benefit from a squash, too. Thanks!

[Jane Sandberg (sandbergja)]

Thanks, Terran! Rebased and lightly squashed branch here: https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/sandbergja/lp1855781-circulation-policy-configuration3

Also, please disregard the branch I mentioned in #46 -- it has all the wrong commits in it :-(

[Terran McCanna (tmccanna)]

Testing today:

1) Still a problem from comment #8:

A few of the boolean fields can be NULL in the database. In the dojo UI, this was represented by dropdowns with the values "True", "False", and "Unset"/"Inherited". The Angular version doesn't give this option -- it only allows for True/False. Here are the ones that can be NULL:
* Renewal?
* Juvenile?
* Reference?
* Circulate?

2) In the modal: Under Linked Limit Sets > Circ Limit Set Name - fields squished together and misaligned; also, when adding a new one, the layout is visually confusing; also, this section was at the bottom in the old interface - not sure if that was an intentional change or not (screenshot attached)

3) In the modal: Under Record Editor: Circulation Matrix Matchpoint > Active - there is a "required" indicator bar but it is not aligned with the yes/no radio buttons (see screenshot)

4) It would still be valuable to be able to delete a circulation policy row, but that could be a separate wish list if it doesn't make it into this branch.

[Terran McCanna (tmccanna)]

Removing 3.12 milestone as it still needs a bit of work.

[Jason Etheridge (phasefx)]

This is just a rebase of Jane's last branch; well, a cherry-pick, and I also changed more labels in the IDL, but I need to look into the other issues reported. I just wanted to save anyone the pain of trying to rebase this.

collab/phasefx/lp1855781-circulation-policy-configuration3-rebased

[Jason Etheridge (phasefx)]

Terran, you should be seeing a checkbox and an (Unset) link instead of radio buttons, as shown with the attached image. Caching issue?

[Jason Etheridge (phasefx)]

I see why Xavier put the limit sets above the fm editor; if you simply re-order the components, the limit sets UI will end up below the Save button, and the Save button closes the dialog. I tried projecting the limit sets UI into the fm editor, but was running into subtle bugs from, I think, the lifecycles of the different components. I have pushed a commit to the branch, however, that cleans up the look of the limit sets UI a bit, and adds a link directly to the admin interface for creating limit sets.

[Terran McCanna (tmccanna)]

I'm getting a number of modify/delete conflicts when I try to apply to a main branch today.

[Jason Etheridge (phasefx)]

Terran, I've rebased it again and it now lives at collab/phasefx/lp1855781-circulation-policy-configuration3-rebased2

Thanks!

[Terran McCanna (tmccanna)]

Thanks, Jason. These are the issues I'm currently seeing:

1) Circulate is shown as a checkbox - the old interface has a dropdown with an "Inherited" option. (Although, I'm not sure if that's actually needed? Is it inherited automatically if Inherited isn't selected?)

2) When editing an existing circ policy, there still isn't a way to tell if an existing checkbox value is No or Unset.

3) Early Renewal Extends Due date has an Unset option that it shouldn't have because it can only be Yes or No and Save will fail

CIRC LIMIT SET ISSUES:

4) I'm seeing a lot of weirdness with the Circ Limit Set selector. If I have used it with one circ policy and then try to create another circ policy, it selects the previous Circ Limit Set (it shouldn't). Also, if I've used it and then I open a pre-existing circ policy that didn't have a circ limit set applied at all, it selects it anyway.

5) It doesn't appear to be possible to remove a Circ Limit Set from a circ policy. Unless you have to backspace over the existing value? Because of the problems in #4, I'm not sure what it's actually saving.

6) I think having the Circ Limit Set at the top of the modal is confusing - in the old interface it was at the bottom of the form.

7) There is an extra box around the Circ Limit Set field which looks weird.

8) Note that I have not tested comment 35 due to the other circ limit issues.

[Jane Sandberg (sandbergja)]

I mentioned this in today's DIG meeting, but I'm wondering, could we:

1. merge this, acknowledging that there are some outstanding issues
2. keep the link to the dojo interface, rather than pointing it to the new interface
3. Address bugs once it's merged
4. Change the link from dojo to Angular once those bugs are addressed?

Offering this as a way that we as a community could spend more time fixing bugs and less time rebasing.

[Terran McCanna (tmccanna)]

I would be okay with that!

[Jason Etheridge (phasefx)]

Just a slight tweak to restore the radio buttons for booleans. Somewhere along the line the change got eaten and I confused myself over it because I saw checkboxes, but the radio buttons were intended.

collab/phasefx/lp1855781-circulation-policy-configuration3-rebased2-with-eg-bool-select