Skip to content

Tweak the Response Alert, AlertQuery change_status, set_ignored, assign methods - #1

Open
llyon-cb wants to merge 3 commits into
jjfallete:patch-1from
llyon-cb:response-models
Open

Tweak the Response Alert, AlertQuery change_status, set_ignored, assign methods#1
llyon-cb wants to merge 3 commits into
jjfallete:patch-1from
llyon-cb:response-models

Conversation

@llyon-cb

Copy link
Copy Markdown

Pull request checklist

Please check if your PR fulfills the following requirements:

  • Docs have been reviewed and added / updated if needed (for bug fixes / features)
  • Tests have been added that prove the fix is effective or that the feature works.
  • New and existing tests pass locally with the changes.
  • Code follows the style guidelines of this project (PEP8, clean code).
  • Linter has passed locally and any fixes were made for failures.
  • A self-review of the code has been done.

Pull request type

Please check the type of change your PR introduces:

  • Bugfix
  • Feature
  • Code style update (formatting, renaming)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • Documentation content changes (not tied to bugs/features)
  • Other (please describe):

Pull Request Description

I've modified the Alert and AlertQuery set_ignored() and change_status() methods a bit based on some manual testing.

Alert.set_ignored() and AlertQuery.set_ignored():

  • Added a docstring to specify what happens with this method
  • Modified the payload keys based on manual testing

Alert.change_status() and AlertQuery.change_status():

  • Added a status check to ensure it's a valid status

Looks like there's also a bug in the API where a change in status is required for a change in assignee to take effect, and I'm reaching out to the Response team about it now

Does this introduce a breaking change?

  • Yes
  • No

How Has This Been Tested?

The attached file was used for testing -- change the .txt extension to .py and modify ALERT_ID and ALERT_QUERY to run

Other information:

test_response_alerts.txt

@llyon-cb

Copy link
Copy Markdown
Author

@jjfallete Thanks for your additions to the Response models. I've tweaked your PR a bit based on manual testing. I'm sorry it's taken so long to get around to this!

@jjfallete

Copy link
Copy Markdown
Owner

@jjfallete Thanks for your additions to the Response models. I've tweaked your PR a bit based on manual testing. I'm sorry it's taken so long to get around to this!

Thanks! Were you able to get this to work correctly in the single alert change_status def? In my testing I didn't think I could, but perhaps I missed something.

return self._cb.post_object("/api/v1/alert/{0}".format(self.unique_id), payload)

@llyon-cb

llyon-cb commented Oct 27, 2020

Copy link
Copy Markdown
Author

@jjfallete Thanks for your additions to the Response models. I've tweaked your PR a bit based on manual testing. I'm sorry it's taken so long to get around to this!

Thanks! Were you able to get this to work correctly in the single alert change_status def? In my testing I didn't think I could, but perhaps I missed something.

return self._cb.post_object("/api/v1/alert/{0}".format(self.unique_id), payload)

I was able to get that call to work, with a payload like this:

{
"status": "Resolved",
"unique_id": "my-alert-id"
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants