-
Notifications
You must be signed in to change notification settings - Fork 43
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’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[MNO-695][MNOE-773] Api v2 Dashboard missing dependency for KPI Alert Recipients #535
Conversation
x4d3
commented
Oct 17, 2017
•
edited
Loading
edited
- Include Recipients in Alerts
- Use update_recipients instead of recipient_id
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. Although some indentation changes are a bit funny.
I assume this requires maestrano/maestrano-hub#754 to be merged and deployed first?
) | ||
end | ||
end | ||
.flatten | ||
.flatten |
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.
?
@@ -26,8 +26,8 @@ def index | |||
|
|||
begin | |||
response = MnoEnterprise::ImpacClient.send_get('/api/v2/kpis', attrs, basic_auth: auth) | |||
# TODO check there was no error, something like | |||
# return render json: { message: "Unable to retrieve kpis from Impac API | Error #{response.code}" } unless response.success? | |||
# TODO check there was no error, something like |
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.
?
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.
Rubymine...
@@ -2,5 +2,22 @@ module MnoEnterprise | |||
class Alert < BaseResource | |||
property :created_at, type: :time | |||
property :updated_at, type: :time | |||
property :kpi_id |
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.
Should the kpi_id
be exposed? I thought we were getting rid of the foreign keys
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.
It should for now, but will be addressed later in #560