Templated notifications #4291

jladdjr · 2019-07-09T06:24:46Z

See #79 (see notes on templated notifications)

ISSUE TYPE

Feature Pull Request

COMPONENT NAME

API
UI

ADDITIONAL INFORMATION

Currently a work in progress PR, but think this will be wrapped up soon.

Home stretch:

Show default values for messages in GET
White-list job fields
Templated notifications w/ bodies
Using awx to send out test notifications when the NT includes custom notifications
Ensure that fields are properly formatted (per how this was done previously for limited set of fields)
e2e tests pass

softwarefactory-project-zuul · 2019-07-09T06:32:53Z

Build failed.

awx-api-lint : SUCCESS in 2m 16s
awx-ui-lint : FAILURE in 2m 32s
awx-ui-next-lint : SUCCESS in 2m 58s
awx-api : FAILURE in 6m 13s
awx-ui : SUCCESS in 4m 27s
awx-ui-next : SUCCESS in 3m 46s
awx-swagger : SUCCESS in 6m 49s
awx-detect-schema-change : FAILURE in 7m 40s (non-voting)

awx/ui/client/lib/components/syntax-highlight/index.js

awx/ui/client/lib/components/syntax-highlight/syntax-highlight.directive.js

awx/ui/client/lib/components/syntax-highlight/syntax-highlight.partial.html

awx/ui/client/src/notifications/edit/edit.controller.js

@@ -35,6 +38,16 @@ export default ['Rest', 'Wait',
                }
            });

+            Rest.setUrl(GetBasePath('notification_templates'));


mabashian · 2019-07-12T13:11:50Z

@keithjgrant this is probably unrelated to your changes and is pretty low priority but it looks like an error gets thrown if the user changes the notif type back to the empty state:

I'm pretty sure this error is benign and the user isn't impacted. If this isn't a regression I'd be 👌 with creating a new low priority issue unrelated to this PR.

mabashian · 2019-07-12T13:29:35Z

After having read through the code I think I understand why we're showing <DEFAULT_MESSAGE> to read-only users when no changes to the structure of a particular message has changed:

The UI doesn't have access to those default message templates because they're provided in the POST section of the options response (system auditors won't have this in their response). I feel like this UX is kind of weird though. It makes it seem like there's something hidden. This may not be a huge deal but I was curious if there were any discussions around that @jladdjr.

mabashian · 2019-07-12T13:32:40Z

Last thing I'm curious about:

Do we actually need this checkbox? As far as I can tell it really just acts like a show/hide for those codemirrors. If the user changes the notif type then the fields in that section change. I'm sure you all probably talked about this but I'm just wondering if we need that extra click.

keithjgrant · 2019-07-12T18:12:14Z

The UI doesn't have access to those default message templates because they're provided in the POST section of the options response (system auditors won't have this in their response). I feel like this UX is kind of weird though. It makes it seem like there's something hidden. This may not be a huge deal but I was curious if there were any discussions around that @jladdjr.

We're looking into adding the defaults onto the GET section as well, so all users can see the default values. I pushed this up as a sort of stable halfway point toward that end.

Do we actually need this checkbox?

I did this in an effort to reduce the visual noise of the form, basically hiding "advanced" options until the user needs them. These fields add a lot of length to the form, especially for e-mail notifications which also have longer body field for each notification type (started/succeeded/failed).

jladdjr · 2019-07-15T01:37:53Z

(keith:) We're looking into adding the defaults onto the GET section as well, so all users can see the default values. I pushed this up as a sort of stable halfway point toward that end.

Just now catching up on all the threads here. @mabashian, I mentioned this in response to one of the code comments, and @keithjgrant basically already said this, but wanted to echo that I'm working on getting the defaults ~~posted outside of the POST section~~ listed under the GET section (you're exactly right, btw, about the issue being that we need to move the default info outside of the POST section). My other comment has more details on some of the hurdles I'm seeing w/ moving the default info out. Tackling those hurdles now.

jladdjr · 2019-07-15T04:47:26Z

@keithjgrant (cc @mabashian) - I pushed a commit that surfaces the messages's default field under OPTIONS -> GET.

By default, we explicitly filter out default (and several other fields) under OPTIONS -> GET. So, in order to get this to work, I had to make a tweak to our framework so that we can specifically call out fields in the model that should not get dropped (the change consisted of creating a new field preserve_metadata, and adding some logic to our Metadata class that would read that field and prevent any items it saw listed from getting dropped.

While this works, I'm not sure if it's the best approach. Changing our framework feels like a pretty heavy hammer, although since we explicitly drop default under GET, I didn't really see an alternative. I'm going to run this change by the rest of the API team to see what they think. I'll be in touch with another update.

awx/api/serializers.py

@@ -4167,6 +4167,9 @@ class NotificationTemplateSerializer(BaseSerializer):
    class Meta:
        model = NotificationTemplate
        fields = ('*', 'organization', 'notification_type', 'notification_configuration', 'messages')
+        # preserve metadata for `messages` so default value can be shown for users with read-only access
+        preserve_metadata = {'messages': 'default'}


jladdjr · 2019-07-15T16:41:28Z

@keithjgrant @mabashian - the API team seems to think that what I have for exposing the messages default under OPTIONS -> GET is reasonable. There are still a couple folks that haven't weighed in, but I'm going to run with what I have for now.

jbradberry

This seems like a reasonable approach to me.

awx/api/metadata.py

+                        preserve_metadata = serializer.Meta.preserve_metadata
+                        if field in preserve_metadata:
+                            if type(preserve_metadata[field]) is str:
+                                preserve_metadata[field] = {preserve_metadata[field]}


mabashian · 2019-07-15T17:58:47Z

@jladdjr re: default messages in GET 👍 thanks for that!

mabashian

UI changes lgtm. Ping @unlikelyzero I think this will be ready for your eyes soon.

awx/main/models/notifications.py

+
+        from awx.api.serializers import UnifiedJobSerializer  # Need to import here (To avoid circular import?)
+        job_serialization = UnifiedJobSerializer(self).to_representation(self) # TODO: Filter job serialization to only include whitelisted fields
+        context = dict(job=job_serialization,


…ifications

jladdjr · 2019-08-26T12:52:38Z

@unlikelyzero @one-t - I believe everything has been addressed for notification templates. Can either of you think of anything else that needs to be addressed before we plan a merge mtg?

unlikelyzero · 2019-08-26T13:44:23Z

@jladdjr just a clean yolo run. Otherwise the only open issue can be resolved post-merge
#4521

softwarefactory-project-zuul · 2019-08-27T16:29:19Z

Build succeeded (gate pipeline).

awx-api-lint : SUCCESS in 5m 20s
awx-ui-lint : SUCCESS in 6m 00s
awx-ui-next-lint : SUCCESS in 6m 16s
awx-api : SUCCESS in 11m 29s
awx-ui : SUCCESS in 7m 16s
awx-ui-next : SUCCESS in 7m 41s
awx-swagger : SUCCESS in 11m 24s
awx-detect-schema-change : FAILURE in 12m 23s (non-voting)
awx-push-new-schema : SUCCESS in 10m 54s (non-voting)

jladdjr added type:enhancement state:in_progress component:api component:ui labels Jul 9, 2019

jladdjr force-pushed the templated_messages branch from 9fcb5bc to cc834b8 Compare July 9, 2019 07:17

This comment has been minimized.

Sign in to view