-
Notifications
You must be signed in to change notification settings - Fork 154
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.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
ELE-2398 - move sort_alerts from API to data monitoring alerts flow #1387
ELE-2398 - move sort_alerts from API to data monitoring alerts flow #1387
Conversation
馃憢 @IDoneShaveIt |
) | ||
|
||
def _sort_alerts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same code as it was on the alerts API
alerts_to_send.append(valid_alert) | ||
return SortedAlertsSchema(send=alerts_to_send, skip=alerts_to_skip) | ||
|
||
def _get_suppressed_alerts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same code as it was on the alerts API
return suppressed_alerts | ||
|
||
@staticmethod | ||
def _get_latest_alerts( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same code as it was on the alerts API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 馃帀
def _fetch_data(self, days_back: int) -> List[PendingAlertSchema]: | ||
return self.alerts_api.get_new_alerts( | ||
days_back=days_back, | ||
) | ||
|
||
def _filter_data(self, data: SortedAlertsSchema) -> SortedAlertsSchema: | ||
return SortedAlertsSchema( | ||
send=filter_alerts(alerts=data.send, alerts_filter=self.selector_filter), | ||
skip=filter_alerts(alerts=data.skip, alerts_filter=self.selector_filter), | ||
def _filter_data(self, data: List[PendingAlertSchema]) -> List[PendingAlertSchema]: | ||
return filter_alerts(data, alerts_filter=self.selector_filter) | ||
|
||
def _fetch_last_sent_times(self, days_back: int) -> Dict[str, datetime]: | ||
return self.alerts_api.get_alerts_last_sent_times( | ||
days_back=days_back, | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure we really need these functions
if isinstance(alert.data, TestAlertDataSchema) | ||
] | ||
) == [ | ||
"alert_id_1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should change the IDs, so we won't have the same id on a different alert type.
This way we could be sure that we are not mixing between alerts types as well
The idea behind this change is that the alerts API should not be aware of the monitoring logic for filtering / skipping alerts.
This makes much more sense + it will help the SaaS alerts be more aligned with the OSS alerts code 馃檪
The main changes here is moving code from the alerts API to the DataMonitoringAlerts flow and updating tests.