Angular: Notifications/Action Triggers UI Port

[Kyle Huckins]

Evergreen: master

We’ll be porting the Notifications/Action Triggers UI from DOJO to Angular. This looks like it will be effectively four grid UIs in one.

Tab 1. Event Definitions
An inline fm-editor should suffice here.

Tab 2. Hooks
A grid UI for action hooks - double-clicking should bring up an fm-editor modal.

Tab 3. Reactors
A grid UI for Action Trigger Reactors - double-clicking should bring up an fm-editor modal.

Tab 4. Validators
A grid UI for Action Trigger Validators - double-clicking should bring up an fm-editor modal.

[Mike Risher (mrisher)]

I pushed a branch with this work:
https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mrisher/lp1855780-notification-action-triggers-port

There are 2 outstanding bugs related to this work, in particular the filters on the Event Definitions grid.
* The width of the filter UIs is an issue
https://bugs.launchpad.net/evergreen/+bug/1858684
* Interval columns are not currently filterable
https://bugs.launchpad.net/evergreen/+bug/1848579

[Ruth Frasur Davis (redavis)]

Not sure if it's a feature or a bug - in the reactors tab, went to edit #15 fourty_two. Was able to edit/append the Description, but was unable to edit the Module Name

[Chris Sharp (chrissharp123)]

I can confirm Ruth's observation, but I can also see that the Dojo version of the interface acts the same way.

[Chris Sharp (chrissharp123)]

Works for me! My signoff branch is at https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp1855780-notification-action-triggers-port. Deferring to another committer to review after me.

[Chris Sharp (chrissharp123)]

Spoke too soon! Tiffany Little pointed out something that Ruth and I apparently missed: "I don't know how to filter retention_interval - interval" and "I don't know how to filter repeat_delay - interval" - my bad. Removing signoff and pullrequest.

[Chris Sharp (chrissharp123)]

Ah, I see that Mike has already linked to bug 1848579. I guess I'm too excited about having this feature deployed!

[Chris Sharp (chrissharp123)]

Okay, given that bug 1848579 has been filed and the rest of the features work as expected, I will re-apply my blessing/signoff and let another core committer overrule me if they disagree.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp1855780-notification-action-triggers-port

[Tiffany Little (tslittle)]

Tested on the feedback fest server.

* On the Hooks tab, none of the filters actually filter the list. To test, try to filter the Passive column to [is exactly] True. The list is not filtered. Same goes for any of the other columns. Filters appear to work as expected on the Reactors and Validators tabs.

[Chris Sharp (chrissharp123)]

I can confirm Tiffany's observation. No useful console output, but the filters aren't working. While that's known now to be a problem, I would hate to see the UI delayed because of it since the Dojo equivalent is so much worse. Again, I'll defer to another core committer as to whether this finding means we don't push this to master/3.5.

[Chris Sharp (chrissharp123)]

Just added a commit that removes the interval columns from filtering and preventing the confusing "I don't know how to filter" messages.

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/csharp/lp1855780-notification-action-triggers-port

[Chris Sharp (chrissharp123)]

In the hopes that this may enlighten someone with better Angular chops than I have, I can see that the Hook filters that aren't working generate this call in the activity log:

open-ils.pcrud open-ils.pcrud.search.ath "<REDACTED>", [{"key":{"!=":null}}], {"offset":0,"limit":10,"order_by":{"ath":"key"}}

which results in this SQL:

SELECT "ath".key, "ath".core_type, oils_i18n_xlate('action_trigger.hook', 'ath', 'description', 'key', "ath".key::TEXT, 'en-US') AS "description", "ath".passive FROM action_trigger.hook AS "ath" WHERE ( "ath".key IS NOT NULL ) ORDER BY key LIMIT 10 OFFSET 0;

So something about the filtering logic is not activating.

[Mike Risher (mrisher)]

Both hooks (ATH) and reactors (ATREACT) have a "description" field. In testing I notice that when I filter Reactors description fields the changes are applied to Hooks.

To replicate: 1. Go to the reactors tab, 2. Filter description to start with "A". 3. Go to the hooks tab. 4. note that hooks data is filtered so that all descriptions start with A, even though no filters were set up on Hooks.

Not completely sure what's causing this, but it appears the filters are applying to the wrong grid, but only in the case of Hooks and Reactors and only on the description field. Validators also has a "description" field, but it isn't getting confused with the Hooks filters.

[Mike Risher (mrisher)]

The filtering is now working properly on the Hooks tab. I double checked the other tabs as well. This is ready for review again. Branch here:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mrisher/lp1855780-notification-action-triggers-v2

[Jane Sandberg (sandbergja)]

This interface is working well for me, Mike. Thank you for taking this on! Signoff at user/sandbergja/lp1855780-notification-action-triggers-v2

Thanks also to Chris, Tiffany, and Ruth for your good testing. Chris, if your signoff is still good, I think we can get this merged for 3.6. :-D

[Galen Charlton (gmc)]

I've skimmed this and it looks good as far as it goes, but unless I'm missing something, there's a pretty big omission: there's no way to set an event definition's environment and parameters and there's no equivalent of the Dojo event definition test. In the Dojo interface, these three bits are accessed by clicking on the linked definition title (as opposed to double-clicking on the row to open the main editing modal).

I'm tacking on the needsrepatch tag on this, but to be clear, I think the current patch is a very solid start.

The most direct way to deal with the missing bits might be to add grid actions to edit the environment, edit the parameters, and run the tests. Alternatively, having the main edit modal (possibly by adding tabs?) be able to handle the environment and parameters would be an UX improvement, but would be a bit more work.

Also noting that the recent upgrade to Angular 10 means that the loadChildren attribute will need to be updated. It would also be good to replace ngbTab with ngbNav.

[Mike Risher (mrisher)]

Galen, thanks for pointing out the omission. I've improved this so now when you edit the event def you are taken to a new page that lets you edit parameters, environments, and run tests. I've also updated ngbTab with ngbNav.

It's ready for another look. Branch here:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mrisher/lp1855780-notification-action-triggers-v4

[Jennifer Weston (jweston)]

Thanks to Mike and all of the testers on this. I tested and can confirm the new pages display to allow for editing environment and parameters and to run tests. I was able to create a new event definition environment (DefID 120) and a new parameter (DefID 121).

However, when I clone an existing event definition, it does not clone the environment nor does it provide a prompt asking me if I want to clone it the way Dojo does. (ex: Cloned DefID 108 to create DefID 118)

[Jennifer Weston (jweston)]

adding note that my testing today was on https://tiffany-master.gapines.org/eg/staff

[Mike Risher (mrisher)]

Thanks Jennifer Weston. I'm unable to reproduce what you experienced in DOJO. I have tried using my own server and https://tiffany-master.gapines.org/. Here are the steps I've gone through:

1. Find a Trigger Event Definition with an environment. I used "45 Day Account Expiration" since it's at BR1 and my workstation is set to BR1. I also used Event Def 108 ("30 Day Account Expiration Courtesy Notice")
2. Click the clone button.
3. Give it a unique name.
4. Press "save"

The console shows an error message at that point. "Uncaught _CUD: Error creating, deleting or updating." I never see a prompt asking me if I want to clone it.

Could you please give me a list of steps to take so that when trying to clone the DOJO interface gives you a prompt?

Input is welcome on the desired behavior of this port. Do we want a prompt before cloning the environment, like DOJO gives, or do we want it cloned without a prompt?

[Mike Risher (mrisher)]

After some more experimentation I managed to clone an event definition within the DOJO interface.

I've re-created the behavior of the old interface in Angular. If cloning an event definition with environments the user is asked if they also want to clone environments, and they are cloned if desired. This is ready for another look:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mrisher/lp1855780-notification-action-triggers-v5

[Jessica Woolford (jwoolford)]

I notice there is no "Cancel" button when going in to edit an event definition. Would love to see this button added back if possible. Otherwise, everything looks great (and is a big improvement IMHO).

[Christine Burns (christine-burns)]

We have tested this on
https://bugsquash2.mobiusconsortium.org/eg/staff/

Thank you for your work on this! Overall it looks great. We like the ability of toggle among Event def, Hooks, Reactors and Validators tabs.

A few comments / issues

1. Filter is case sensitive> For example, in Operator, filtering by courtesy results in no trigger, but Courtesy has results.

2. On Edit def screen, there is Save only, no Cancel in case you want to abandon your edit

3. No way to get out of Edit def screen. After clicking Save, you are still on Edit screen. You have to use Back. But you are not back to the exact previous screen. For example, you entered Edit screen from a filtered list, you are back to the whole list.

4. On Create def screen, clicking the up/down arrow in the fields with dropdow list does not display the list. You have to type in something.

5. I tried to create two event definitions with duplicate names. Though I saw the green confirmation popup (Update successful), the records were not saved.
- note other events did save = however I wanted to note the false positive

[Mike Risher (mrisher)]

Thanks for taking a look and giving your feedback!

1. The notification / action trigger port uses the normal filters that the rest of the Angular grids use, so problems with filters is not specific to this work and will affect all our Angular interfaces. It's possible that you're seeing a problem related to bug #1910424 (case insensitive filters). If not, a new bug should be opened so we can resolve the problem.

2. There's no cancel button because the editor isn't a modal here. It was designed to have no "Cancel" button when it appears on a page like this. In this interface you'd cancel by not saving and going back to the previous page.

It does make sense to add a button to take you back. The button has been added above all the tabs, and also it's at the bottom of the long Edit Event Definitions form. I amended my latest commit with the change:

https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/mrisher/lp1855780-notification-action-triggers-v5

3. Unfortunately I don't know of an existing way to make the button link to the page's previous state (preserving the filter settings, for example). It would be an enhancement.

4. The notification / action trigger port uses the normal dropdown lists that the rest of the Angular codebase uses, so those problems will affect all our Angular interfaces. A new bug should be opened for this. (Unless that bug already exists, of course!)

5. I tried to replicate this on my end. First I created an event definition with a duplicate name and it was successful. I think that's because it was in a different org. So I tried to create an event definition with the same org and same name, and got an error message when I tried to save. Tried again and got an error message again. I checked the error logs and see those two failures were caused by duplicate names. So this looks good to me. If you're still seeing this could you please give steps to replicate?

[Mike Risher (mrisher)]

Jessica Woolford, as you can see in my comment above, a button has been added. Thanks for the feedback!

[Jessica Woolford (jwoolford)]

I have tested this code and consent to signing off on it with my name, Jessica Woolford, and my email <email address hidden>

[Galen Charlton (gmc)]

user/mrisher/lp1855780-notification-action-triggers-v5 doesn't rebase cleanly against master; I'm getting a "both added" warnings when I attempt it. Could you rebase, Mike?

[Rogan Hamby (rogan-hamby)]

I rebuilt Mike's second commit and it d8578dd60d48b9420a7b5c6cb50db9a1e65e2ea3 and it's parent fc9f00c245dc4fcf53c38f59f76f2ab36c9999c9 picked clean for me. This is an odd one since I did the most recent commit though it's not my code but I'm going to assume I shouldn't sign off on it. But I am slapping a pull request back on.

[Evergreen Bug Maintenance (bugmaster)]

I'm assuming that the current working branch is user/rogan/lp1855780_action_triggers_port_rebased

Noting a couple issues:

- The modal for editing a definitions environment doesn't have a working drop-down for the collector; the atcol IDL class needs to be made known to pcrud
- none of the grids are set up to save settings

[Rogan Hamby (rogan-hamby)]

I'll try to find time to look at that but not assigning the bug to myself yet in case someone else has time before I do.

[Galen Charlton (gmc)]

Taking this over.

[Galen Charlton (gmc)]

An updated patch series that addresses (as far as I can tell) all of the reported issues to date is now available in:

working/user/gmcharlt/lp1855780_angular_a_t_port / https://git.evergreen-ils.org/?p=working/Evergreen.git;a=shortlog;h=refs/heads/user/gmcharlt/lp1855780_angular_a_t_port

[Erica Rohlfs (erohlfs)]

A few additional comments:
1. On the main page, the buttons "Open in New Window" and "Refresh" no longer exist. Not sure how much they were utilized (or if Refresh is even needed) - just noting the difference.
2. When creating / editing an event definition, I'm not sure why I'm seeing the Context Bib Path, Context Item Path, Context Library Path, and Context User Path fields.
3. I really like having the "Edit Event Definition" option within the Action menu.
4. I also like the "Back to Notifications/Action Triggers" button, but it would be nice for it to go back to the filtered selection.
5. I also like the settings and columns options and filtering as part of the overall Angular project.
6. I agree with Christine about the dropdown options.
7. I like the required fields indicators. However, Owning Library is currently not showing as a required field.
   7a. If you attempt to create a new event definition without an Org Unit, EG reads "Update Succeeded" -when, it actually did not save. In the older interface, if you do not enter an Org Unit, EG simply doesn't save the event (but it also doesn't tell you why it won't save).
   7b. Slight wording variation...when you try to save an event definition without an org unit, EG reads "Update Succeeded" - if you create an event definition with an org unit (and it actually saves) EG reads "New Entry Added."
8. Granularity should also be a dropdown option. It looks like it currently isn't set up to be one. Also, when you attempt to enter a known granularity option, EG doesn't populate the option. (For comparison, in the Reactor field, if you start typing "send" EG will populate options such as SendEmail and SendSMS.)

[Jane Sandberg (sandbergja)]

Thank you to Mike, Galen, Rogan, Chris, Terran, Christine, Ruth, Jennifer, Jessica, and Erica. I have pushed this to master for inclusion in 3.9.

I took the opportunity to make two small follow-up commits: one just removes some unnecessary whitespace, the other makes the dropdowns behave as Christine expected in comment #22 point #4 and Erica expected in comment #32 point #6.

To address some of Erica and Christine's points:
* Comment #32 point #7 (owning library currently doesn't show as a required field) is a manifestation of an existing bug: bug 1933498.
* Comment #22 point #5 and #32 point #7a (An "update succeeded" toast appears when the update didn't succeed) is also present in a few other screens: bug 1913816 and bug 1912553, for example.
* Comment #32 point #2: those fields were new to me, too. They were introduced very recently, in bug 1207533. I don't know enough about that development to understand how staff will be interacting with them. Perhaps somebody with more knowledge of them could open up a follow-up bug -- I could see some kind of tooltip being helpful for these fields.

Also a note that I opened up bug 1951307 as a small UI follow-up.

Apologies to Jessica: I neglected to include your signoff on the relevant commits!

[Galen Charlton (gmc)]

Thanks, Jane. I've also opened bug 1951311 regarding Erica's comment #32 point #8.