Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for multiple notifications #4

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Support for multiple notifications #4

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
49bb57e
Check for item type
amckenna-sfdc Jan 24, 2017
2e7592d
Support for adding multiple notifications with new alert
amckenna-sfdc Jan 24, 2017
a8693a0
Merge remote-tracking branch 'upstream/master' into HEAD
amckenna-sfdc Feb 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion argusclient/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

notifications = []
for notification in alert.notifications:
notifications.append(alertobj.notifications.add(notification))
alertobj.notifications = notifications
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if alert.trigger and alert.notifications:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

alertobj.trigger.notificationsIds = []
for notification in alertobj.notifications:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

self.argus.alerts.add_notification_trigger(alertobj.id, notification.id, alertobj.trigger.id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

notification.triggersIds = [alertobj.trigger.id]
alertobj.trigger.notificationsIds.append(notification.id)
elif alert.trigger and alert.notification:
self.argus.alerts.add_notification_trigger(alertobj.id, alertobj.notification.id, alertobj.trigger.id)
alertobj.notification.triggersIds = [alertobj.trigger.id]
alertobj.trigger.notificationsIds = [alertobj.notification.id]
Expand Down
2 changes: 2 additions & 0 deletions argusclient/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,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))
for item in value:
if not isinstance(item, Notification): raise ValueError("array member should be of Notification type, but is: %s" % type(item))
# This is a special case allowed only while adding new alerts, so ensure that argus_id of self and the objects is None.
# TODO Check for item type also
self._notifications = value
Expand Down