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.

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

Add alert actions api #13325

Merged
merged 1 commit into from Jan 24, 2017

Conversation

Projects
None yet
5 participants
@moolitayer
Copy link
Contributor

moolitayer commented Dec 29, 2016

Adds retrieval and creation of alert_actions as a sub resource of alerts.

Route Body
GET /api/alerts/:id/alert_actions/ [1]
GET /api/alerts/:id/alert_actions/:id [2]
POST /api/alerts/:id/alert_actions/ [3]

[1] :
Response:

{
    "name": "alert_actions",
    "count": 3,
    "subcount": 3,
    "resources": [
        {
            "href": "http://localhost:3000/api/alerts/6/alert_actions/8"
        },
        {
            "href": "http://localhost:3000/api/alerts/6/alert_actions/9"
        },
        {
            "href": "http://localhost:3000/api/alerts/6/alert_actions/10"
        }
    ],
    "actions": [
        {
            "name": "create",
            "method": "post",
            "href": "http://localhost:3000/api/alerts/6/alert_actions"
        }
    ]
}

[2] :
Response:

{
    "href": "http://localhost:3000/api/alerts/6/alert_actions/9",
    "id": 9,
    "action_type": "assign",
    "user_id": 1,
    "assignee_id": 1,
    "miq_alert_status_id": 6,
    "created_at": "2017-01-22T12:29:29Z",
    "updated_at": "2017-01-22T12:29:29Z"
}

[3] :
Request:

{
  "action_type": "comment",
  "user_id": 1,
  "comment": "comment text"
}

Response:

{
    "results": [
        {
            "id": 11,
            "action_type": "comment",
            "user_id": 1,
            "comment": "comment text",
            "miq_alert_status_id": 6,
            "created_at": "2017-01-22T12:36:54Z",
            "updated_at": "2017-01-22T12:36:54Z"
        }
    ]
}

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Dec 29, 2016

@miq-bot add_label wip, enhancement, api

@chessbyte chessbyte added the wip label Jan 3, 2017

@moolitayer moolitayer changed the title [WIP] Add alert actions api Add alert actions api Jan 11, 2017

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 11, 2017

@miq-bot remove_label wip

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 11, 2017

@abellotti can you start reviewing 530fdbb9e46ccc7734ded66aa05e8b5afb93c8c7 even though the dependent patch hasn't been merged yet?

It's critical to have all of those patches merged by the end of the week to have the API in place for the next week for other people's work

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 12, 2017

@abellotti PTAL

@chessbyte chessbyte removed the wip label Jan 12, 2017

@abellotti abellotti requested review from abellotti and imtayadeway Jan 12, 2017

@abellotti

This comment has been minimized.

Copy link
Member

abellotti commented Jan 12, 2017

Could you add a description entry showing the expected usage, i.e.

GET /api/alerts
GET /api/alerts/:id/alert_actions
POST /api/alerts/:id/alert_actions  with action names ...
...

It would help us to better review.

@abellotti
Copy link
Member

abellotti left a comment

comments above.

/cc @imtayadeway for more input. Thanks!!

app/controllers/api/subcollections/alerts.rb Outdated
module Subcollections
module Alerts
def alerts_query_resource(object)
object.try(:miq_alert_statuses).to_a

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 12, 2017

Member

could just be:

object.miq_alert_statuses || []
- :name: read
:identifier: alert_status_action_show_list
:post:
- :name: create

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 12, 2017

Member

does it make sense to allow creates as both /api/alert_actions as well as /api/alerts/:id/alert_actions ? Doesn't the former need a relationship to the /api/alerts resource ?

This comment has been minimized.

Copy link
@moolitayer

moolitayer Jan 22, 2017

Author Contributor

Right, should be supported only for /api/alerts/:id/alert_actions now

spec/requests/api/alert_actions_spec.rb Outdated
expect(response.parsed_body["results"].count).to eq(1)
expect(response.parsed_body["results"][0]).to include(specification)
end
# TODO: add tests for sub collection (only related for alert returned, id is ignored and taken from parent etc)

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 12, 2017

Member

we'd need this test added too.

spec/requests/api/alert_actions_spec.rb Outdated
run_post(alert_actions_url, specification)
expect(response).to have_http_status(:ok)
expect(response.parsed_body["results"].count).to eq(1)
expect(response.parsed_body["results"][0]).to include(specification)

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 12, 2017

Member

32/33 could be a single expect.

db/fixtures/miq_product_features.yml Outdated
- :name: Add
:description: Display Individual Alert Status
:feature_type: control
:identifier: alert_status_action_new

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 12, 2017

Member

/cc @h-kataria need your 👀 on above miq_product_features.yml changes. Thanks!!

@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Jan 19, 2017

This pull request is not mergeable. Please rebase and repush.

@miq-bot miq-bot added the unmergeable label Jan 19, 2017

@moolitayer moolitayer force-pushed the moolitayer:alert_actions_api branch Jan 22, 2017

@miq-bot miq-bot removed the unmergeable label Jan 22, 2017

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 22, 2017

/cc @h-kataria need your on above miq_product_features.yml changes. Thanks!!

@h-kataria could you please take a look at the product features for added here?
this is part of the API needed for "Monitor" added here does it make sense to have separate features?

@imtayadeway @abellotti addressed comments, and made some changes based on the review of #13025 can you take a look? Thanks

@moolitayer moolitayer force-pushed the moolitayer:alert_actions_api branch Jan 23, 2017

@moolitayer moolitayer closed this Jan 23, 2017

@moolitayer moolitayer reopened this Jan 23, 2017

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 23, 2017

@imtayadeway @abellotti Please review, Feedback appriciated

app/controllers/api/subcollections/alert_actions.rb Outdated
module Subcollections
module AlertActions
def alert_actions_query_resource(object)
MiqAlertStatusAction.where(:miq_alert_status_id => object.id)

This comment has been minimized.

Copy link
@imtayadeway

imtayadeway Jan 23, 2017

Contributor

Do alert statuses not have alert status actions? i.e. something like object.miq_alert_status_actions?

This comment has been minimized.

Copy link
@moolitayer

moolitayer Jan 23, 2017

Author Contributor

Sure, will change

app/controllers/api/subcollections/alert_actions.rb Outdated
end

def alert_actions_create_resource(object, type, _id, data)
data['miq_alert_status_id'] = object.id

This comment has been minimized.

Copy link
@imtayadeway

imtayadeway Jan 23, 2017

Contributor

This is changing the data that's passed into this method - perhaps you could dup the data before you do this?

This comment has been minimized.

Copy link
@moolitayer

moolitayer Jan 23, 2017

Author Contributor

Maybe using merge would be cleaner?

spec/requests/api/alerts_spec.rb Outdated
api_basic_authorize subcollection_action_identifier(:alerts, :alert_actions, :create, :post)
run_post(actions_subcollection_url, attributes)
expect(response).to have_http_status(:ok)
alert_action = MiqAlertStatusAction.find(response.parsed_body["results"].first["id"])

This comment has been minimized.

Copy link
@imtayadeway

imtayadeway Jan 23, 2017

Contributor

Are the attributes not sent back in the response? Can you not put the expectation on those?

@moolitayer moolitayer force-pushed the moolitayer:alert_actions_api branch Jan 23, 2017

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 23, 2017

@imtayadeway addressed comments, thanks

spec/requests/api/alerts_spec.rb Outdated
api_basic_authorize subcollection_action_identifier(:alerts, :alert_actions, :create, :post)
run_post(actions_subcollection_url, attributes)
expect(response).to have_http_status(:ok)
expect(response.parsed_body["results"].first).to have_attributes(attributes)

This comment has been minimized.

Copy link
@imtayadeway

imtayadeway Jan 23, 2017

Contributor

This is fine, but in the case that it breaks you may get a NoMethodError on nil, instead of a diff. So it may be better to write these and others like it something like:

expected = {
  "results" => [
    a_hash_including(attributes)
  ]
}
expected(response.parsed_body). to include(expected)
spec/requests/api/alerts_spec.rb Outdated
"comment" => "comment text",
)
expect(response).to have_http_status(:bad_request)
expect(response.parsed_body["error"]["message"]).to eq(

This comment has been minimized.

Copy link
@imtayadeway

imtayadeway Jan 23, 2017

Contributor

There's a helper that you can use here that will better handle the case outlined above:

expect(response).to have_error_with_message("Failed etc...")
@imtayadeway

This comment has been minimized.

Copy link
Contributor

imtayadeway commented Jan 23, 2017

@moolitayer this looks good, just some minor nits about tests. @abellotti any thoughts?

@moolitayer moolitayer force-pushed the moolitayer:alert_actions_api branch 2 times, most recently Jan 23, 2017

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 23, 2017

Thanks @imtayadeway!
Addressed comments, @abellotti please review

Good night 🌔 💤

@abellotti
Copy link
Member

abellotti left a comment

looks good, other than the user reference support, minor changes needed. Thanks!!

app/controllers/api/subcollections/alert_actions.rb Outdated
)
if alert_action.invalid?
raise BadRequestError, "Failed to add a new alert action resource" \
" - #{alert_action.errors.full_messages.join(', ')}"

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 23, 2017

Member

would prefer:

          raise BadRequestError,
                "Failed to add a new alert action resource - #{alert_action.errors.full_messages.join(', ')}"

should still be good rubucop line length limit-wise.

spec/requests/api/alerts_spec.rb Outdated
expect(response).to have_http_status(:forbidden)
end

it "creats an alert action under an alert" do

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 23, 2017

Member

creats -> creates

spec/requests/api/alerts_spec.rb Outdated
user = FactoryGirl.create(:user)
attributes = {
"action_type" => "comment",
"user_id" => user.id,

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 23, 2017

Member

should support user reference signatures in addition to user_id,

i.e. support:

   "user" : { "id" : ### }

as well as:

  "user" : { "href" : "http://localhost:3000/api/users/###" }

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 23, 2017

Member

and the related tests for both additional signatures.

This comment has been minimized.

Copy link
@moolitayer

moolitayer Jan 24, 2017

Author Contributor

OK I Hope I understood. I should not accept a parameter named user_id anymore?

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 24, 2017

Member

keep support for user_id as you do today (that's what is sent to the model), but the API should support the user reference and generate the user_id accordingly, see what was done for arbitration_rules creation for specifying arbitration_profile (we define the arbitration_profile_id if the arbitration_profile reference is specified, using parse_id) https://github.com/ManageIQ/manageiq/pull/13525/files

This comment has been minimized.

Copy link
@moolitayer

moolitayer Jan 24, 2017

Author Contributor

Thanks for the clarification.

Now that I think about it

  • the user should always be the current user
  • your comment should be applied on assignee_id

user_id === who created this action
assignee_id === assignment target, if this is an assignment action

I'm working on it, let me know if that does not make sense

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 24, 2017

Member

if user_id is always who created this action, why is it passed in below as part of attributes in the run_post (line 102). Maybe it should not and the API forces it to be User.current_user.id before sending to model ?

This comment has been minimized.

Copy link
@moolitayer

moolitayer Jan 24, 2017

Author Contributor

That is should be the situation now. I wrote a test to show a user_id is always ignored in favor of the current user ("creates an alert action on the current user")

spec/requests/api/alerts_spec.rb Outdated
"subcount" => 1,
"resources" => [
{
"href" => "http://www.example.com/api/alerts/#{alert.id}/alert_actions/#{alert_action.id}"

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 23, 2017

Member

would prefer:

  "resources" = [
     {
         "href" => a_string_matching("#{alerts_url(alert.id)}/alert_actions/#{alert_action.id}")
     }

including the ones in the alerts_spec.rb, but those can be a different PR.

spec/requests/api/alerts_spec.rb Outdated
run_get("#{actions_subcollection_url}/#{alert_action.id}")
expect(response).to have_http_status(:ok)
expect(response.parsed_body).to have_attributes(
"href" => "http://www.example.com/api/alerts/#{alert.id}/alert_actions/#{alert_action.id}",

This comment has been minimized.

Copy link
@abellotti

abellotti Jan 23, 2017

Member

same as above, would prefer the a_string_matching, leveraging alerts_url.

@moolitayer moolitayer force-pushed the moolitayer:alert_actions_api branch Jan 24, 2017

@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 24, 2017

@abellotti addressed comments, added tests for the new functionalities
(assignee can be refrenced by either id or href, user value should be ignored and always set to the current user)
please take a look

@moolitayer moolitayer force-pushed the moolitayer:alert_actions_api branch to 6bb0bf1 Jan 24, 2017

@miq-bot

This comment has been minimized.

Copy link
Member

miq-bot commented Jan 24, 2017

Checked commit moolitayer@6bb0bf1 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
4 files checked, 1 offense detected

spec/requests/api/alerts_spec.rb

@abellotti

This comment has been minimized.

Copy link
Member

abellotti commented Jan 24, 2017

LGTM!! Thanks @moolitayer for the API Enhancement.

@abellotti abellotti merged commit 1d00c06 into ManageIQ:master Jan 24, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.005%) to 42.462%
Details
@moolitayer

This comment has been minimized.

Copy link
Contributor Author

moolitayer commented Jan 24, 2017

Awesome @abellotti @imtayadeway Thanks for reviewing & improving my work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.