Skip to content

Adding new exception:ArgusObjectNotFoundException #9

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

Jump to bottom
Merged
merged 1 commit into from
Dec 18, 2017
Merged

Adding new exception:ArgusObjectNotFoundException #9

merged 1 commit into from
Dec 18, 2017

Conversation

rrajeevan
Copy link
Contributor

  1. Addresses issue: HTTP not found on Alert Obj #8
  2. Couldn't get unit test to run without adding /meta for tests related to get_user_alert
  3. typo

Output of unit test:

python -m unittest discover tests/
...................................................................................................................
----------------------------------------------------------------------
Ran 115 tests in 0.033s

OK

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @rrajeevan to sign the Salesforce Contributor License Agreement.

@rrajeevan
Copy link
Contributor Author

er...cla signed..try again

@rrajeevan rrajeevan closed this Dec 16, 2017
@rrajeevan rrajeevan reopened this Dec 16, 2017
hdara-sfdc
hdara-sfdc reviewed Dec 17, 2017
View reviewed changes
argusclient/client.py Outdated
@@ -741,7 +746,7 @@ def check_success(resp, decCls):
raise ArgusException(resp.text)
return res
elif resp.status_code == httplib.NOT_FOUND:
raise ArgusException("Object not found at endpoint: %s message: %s" % (resp.request.url, resp.text))
raise ArgusObjectNotFoundException("Object not found at endpoint: %s message: %s" % (resp.url, resp.text))
elif resp.status_code == httplib.UNAUTHORIZED:
raise ArgusAuthException("Failed to authenticate at endpoint: %s message: %s" % (resp.request.url, resp.text))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you change here also from resp.reqeust.url to resp.url for consistency sake?

hdara-sfdc
hdara-sfdc reviewed Dec 17, 2017
View reviewed changes
tests/test_service.py
@@ -423,18 +424,18 @@ def testGetUserAlert(self, mockGet):
res = self.argus.alerts.get_user_alert(testId, testId)
self.assertTrue(isinstance(res, Alert))
self.assertEquals(res.to_dict(), alert_D)
self.assertIn((os.path.join(endpoint, "alerts"),), tuple(mockGet.call_args))
self.assertIn((os.path.join(endpoint, "alerts/meta"),), tuple(mockGet.call_args))
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for fixing this. This must have been me forgetting to run the unittests with my last change.

@hdara-sfdc
Copy link
Contributor

Thanks for the change, it looks good. If you could just do the one change above for consistency, I can go ahead and merge.

@rrajeevan
Copy link
Contributor Author

@hdara-sfdc made the requested changes. Let me know what you think

Added one more unit test (testUnauthorized()) to check for 401.

python -m unittest discover tests/
....................................................................................................................
----------------------------------------------------------------------
Ran 116 tests in 0.033s

OK

@hdara-sfdc
Copy link
Contributor

Added one more unit test (testUnauthorized()) to check for 401.

Nice! Have you already pushed it? I don't see it in the PR.

@rrajeevan
Copy link
Contributor Author

Yeah.. You should see it below. Let me know if you cant. Squashed my commits :)

https://github.com/salesforce/python-argusclient/pull/9/files#diff-c95766f217f901a70ccf06e3586e8218R51

@hdara-sfdc
Copy link
Contributor

OK, I didn't realize you squashed. Since github allows "Squash and merge" you don't have to do it, besides, squashing early makes it harder to review incrementally.

I can see your changes and I will merge now.

@hdara-sfdc hdara-sfdc merged commit d0e6b72 into salesforce:master Dec 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hdara-sfdc hdara-sfdc hdara-sfdc left review comments

Assignees
No one assigned
Labels
cla:signed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rrajeevan @hdara-sfdc