Support for multiple notifications #4

amckenna-sfdc · 2017-01-24T01:39:25Z

The current code silently discards notifications when adding more than one using syntax like so:

        alertobj = Alert(
            alert_name,
            alert_expr,
            alert['alert']['cron_entry'],
            trigger=trigger,
            enabled=True,
            notifications=notifications,
            missingDataNotificationEnabled=True,
        )

hdara-sfdc · 2017-03-23T07:50:55Z

Apologies for not paying attention, I wasn't apparently watching this project and so haven't received notifications of the PR. I will get back to you soon.

hdara-sfdc · 2017-03-27T04:31:02Z

Thanks for highlighting the fact that multiple notifications are not being handled correctly. There is actually a bigger problem here with the overall design and interface, as I don't think loading an alert even works as expected when there are multiple notifications. The same should be applicable for triggers too. We need to think through the interface a bit and redesign it and fix the missing functionality. I have some ideas and will try it out. I will create an issue to track this.

hdara-sfdc

I just submitted a fix for issue #1, which so please rebase against this latest change.

Please add unit tests. I have added a couple of new tests in test_service.py which can serve as basis for you.

hdara-sfdc · 2017-03-27T17:33:31Z

argusclient/client.py

@@ -391,7 +391,18 @@ def add(self, alert):
            alertobj.trigger = alertobj.triggers.add(alert.trigger)
        if alert.notification:
            alertobj.notification = alertobj.notifications.add(alert.notification)
-        if alert.trigger and alert.notification:
+        elif alert.notifications and isinstance(alert.notifications, list):


Can you make the same change for alert.triggers also?

hdara-sfdc · 2017-03-27T17:36:41Z

argusclient/model.py

@@ -324,7 +324,8 @@ def notifications(self):
    @notifications.setter
    def notifications(self, value):
        if not isinstance(value, list): raise ValueError("value should be of list type, but is: %s" % type(value))
-        # TODO Check for item type also
+        for item in value:
+            if not isinstance(item, Notification): raise ValueError("array member should be of Notification type, but is: %s" % type(item))


Need the same change for triggers too.

The notifications and triggers properties would be a list only for new Alert object, as otherwise these would be set to a service instance. Additional validations are required to ensure that argus_id of these objects is None. Also ensure that self.argus_id itself is None.

hdara-sfdc · 2017-03-27T17:54:35Z

argusclient/client.py

+            notifications = []
+            for notification in alert.notifications:
+                notifications.append(alertobj.notifications.add(notification))
+            alertobj.notifications = notifications


By this time, alertobj.notifications is already set by the prior call to _fill() as a service object, so we need to call alertobj.notifications._init_all(notifications) on it.

Actually, I think this is not required now, since alertobj.notifications.add() is already taking care of populating the service cache, so you may just drop the local notifications list.

hdara-sfdc · 2017-03-27T18:12:14Z

argusclient/client.py

+            alertobj.notifications = notifications
+        if alert.trigger and alert.notifications:
+            alertobj.trigger.notificationsIds = []
+            for notification in alertobj.notifications:


This would change to alertobj.notifications.items(), as this would remain a service object at this point. I guess the difference in the type of this object for new vs. existing alert is going to make it confusing, but since the iteration is only useful on an existing alert, it may not be all that bad.

As I mentioned earlier, alertobj.notifications should remain a service object, so this iteration will change.

hdara-sfdc · 2017-03-27T18:12:33Z

argusclient/client.py

+        if alert.trigger and alert.notifications:
+            alertobj.trigger.notificationsIds = []
+            for notification in alertobj.notifications:
+                self.argus.alerts.add_notification_trigger(alertobj.id, notification.id, alertobj.trigger.id)


Looks like this automatically connects notifications, but only when there is a single trigger. Could you add some additional documentation to this method on this behavior?

amckenna-sfdc · 2017-03-27T18:31:14Z

Thanks for reviewing this. I have some Python work slated for next sprint so hopefully i'll have time to work on this then.

hdara-sfdc · 2017-10-25T13:58:55Z

@amckenna-sfdc Any chance you can work on this PR again?

amckenna-sfdc · 2017-10-25T17:38:49Z

Not this week, possibly during the first November spring

…

On Wed, Oct 25, 2017 at 6:58 AM Hari Krishna Dara ***@***.***> wrote: @amckenna-sfdc <https://github.com/amckenna-sfdc> Any chance you can work on this PR again? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AYI7yvWaCVwPxC5h8NrU_niLkTypfquVks5svz6ggaJpZM4Lrw8m> .

amckenna-sfdc · 2017-10-25T17:38:54Z

*Sprint On Wed, Oct 25, 2017 at 10:38 AM Adam McKenna <[email protected]> wrote:

…

Not this week, possibly during the first November spring On Wed, Oct 25, 2017 at 6:58 AM Hari Krishna Dara < ***@***.***> wrote: > @amckenna-sfdc <https://github.com/amckenna-sfdc> Any chance you can > work on this PR again? > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#4 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AYI7yvWaCVwPxC5h8NrU_niLkTypfquVks5svz6ggaJpZM4Lrw8m> > . >

hdara-sfdc · 2017-10-26T06:16:27Z

Sounds good!

salesforce-cla · 2018-02-12T21:46:18Z

Thanks for the contribution! Before we can merge this, we need @amckenna-sfdc to sign the Salesforce Contributor License Agreement.

hdara-sfdc

@amckenna-sfdc Thanks for fixing the conflicts! I am not sure if I should have waited for another change, but I took a quick look anyway and here is my overall feedback:

Your Salesforce CLA is still pending
At least some of the previous review comments are still valid
I added a few new comments, as I got some additional clarity on the service objects
We need new unit tests to cover these new scenarios
Some documentation updates are needed

hdara-sfdc · 2018-02-13T10:50:19Z

argusclient/client.py

+            notifications = []
+            for notification in alert.notifications:
+                notifications.append(alertobj.notifications.add(notification))
+            alertobj.notifications = notifications


alertobj.notifications is expected to be a service object of type AlertNotificationsServiceClient, so this will break the contract. You can however use the notifications list to initialize the collection that is behind the service object so that it will avoid one API call.

hdara-sfdc · 2018-02-13T10:55:02Z

argusclient/client.py

@@ -416,7 +416,18 @@ def add(self, alert):
            alertobj.trigger = alertobj.triggers.add(alert.trigger)
        if alert.notification:
            alertobj.notification = alertobj.notifications.add(alert.notification)
-        if alert.trigger and alert.notification:
+        elif alert.notifications and isinstance(alert.notifications, list):


The notifications member of a new alert should never be anything other than a list type. It is worth establishing this at the beginning of the method and drop the check here. Also, what if both notification and notifications members exist?

hdara-sfdc · 2018-02-13T10:56:32Z

argusclient/client.py

+            alertobj.notifications = notifications
+        if alert.trigger and alert.notifications:
+            alertobj.trigger.notificationsIds = []
+            for notification in alertobj.notifications:


As I mentioned earlier, alertobj.notifications should remain a service object, so this iteration will change.

hdara-sfdc · 2018-02-13T10:59:34Z

argusclient/client.py

+            for notification in alert.notifications:
+                notifications.append(alertobj.notifications.add(notification))
+            alertobj.notifications = notifications
+        if alert.trigger and alert.notifications:


Isn't the logic here same as the existing logic for a single notification? You should be able to condense both into one loop (i.e., if you initialize a list with a single notification).

amckenna-sfdc added 2 commits January 23, 2017 17:29

Check for item type

49bb57e

Support for adding multiple notifications with new alert

2e7592d

hdara-sfdc requested changes Mar 27, 2017

View reviewed changes

Merge remote-tracking branch 'upstream/master' into HEAD

a8693a0

salesforce-cla bot added the cla:missing label Feb 12, 2018

hdara-sfdc requested changes Feb 13, 2018

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for multiple notifications #4

Support for multiple notifications #4

amckenna-sfdc commented Jan 24, 2017 •

edited

Loading

hdara-sfdc commented Mar 23, 2017

hdara-sfdc commented Mar 27, 2017

hdara-sfdc left a comment

hdara-sfdc Mar 27, 2017

hdara-sfdc Mar 27, 2017

hdara-sfdc Mar 27, 2017

hdara-sfdc Mar 27, 2017

hdara-sfdc Mar 27, 2017

hdara-sfdc Mar 27, 2017

hdara-sfdc Feb 13, 2018

hdara-sfdc Mar 27, 2017

amckenna-sfdc commented Mar 27, 2017

hdara-sfdc commented Oct 25, 2017

amckenna-sfdc commented Oct 25, 2017 via email

amckenna-sfdc commented Oct 25, 2017 via email

hdara-sfdc commented Oct 26, 2017

salesforce-cla bot commented Feb 12, 2018

hdara-sfdc left a comment

hdara-sfdc Feb 13, 2018

hdara-sfdc Feb 13, 2018

hdara-sfdc Feb 13, 2018

hdara-sfdc Feb 13, 2018

Support for multiple notifications #4

Are you sure you want to change the base?

Support for multiple notifications #4

Conversation

amckenna-sfdc commented Jan 24, 2017 • edited Loading

hdara-sfdc commented Mar 23, 2017

hdara-sfdc commented Mar 27, 2017

hdara-sfdc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amckenna-sfdc commented Mar 27, 2017

hdara-sfdc commented Oct 25, 2017

amckenna-sfdc commented Oct 25, 2017 via email

amckenna-sfdc commented Oct 25, 2017 via email

hdara-sfdc commented Oct 26, 2017

salesforce-cla bot commented Feb 12, 2018

hdara-sfdc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amckenna-sfdc commented Jan 24, 2017 •

edited

Loading