-
Notifications
You must be signed in to change notification settings - Fork 23
Create client for Audit Logs #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.
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
Create client for Audit Logs #9
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9 +/- ##
==========================================
+ Coverage 93.90% 96.06% +2.16%
==========================================
Files 5 7 +2
Lines 82 127 +45
==========================================
+ Hits 77 122 +45
Misses 5 5
Continue to review full report at Codecov.
|
|
|
||
| def test_create_audit_log_event_succeeds(self, mock_request_method): | ||
| event = { | ||
| "group": "Terrace House", |
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.
Geez.. how many terrace house fans do we have at workos now?
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.
I actually got the object from here, but I am a Terrace House fan 😅
tests/test_audit_log.py
Outdated
| mock_response.status_code = 200 | ||
| mock_request_method("post", mock_response, 200) | ||
| response = self.audit_log.create_event(event) | ||
| assert mock_response.status_code == 200 |
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.
Likely essentially the same thing, but did you mean assert response.status_code == 200?
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.
Ah yeah, will fix
workos/audit_log.py
Outdated
| Returns: | ||
| dict: Response from WorkOS | ||
| """ | ||
| metadata_limit = 50 |
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.
How do you feel about having this and events_path as a constant?
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.
I'm happy to re-factor the limit to a constant. The path I think is a bit local, but I'm happy to change that as well for consistency
| from workos.exceptions import ConfigurationException | ||
|
|
||
|
|
||
| def validate_api_key_and_project_id(module_name): |
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.
Nice
henrylamchan
left a comment
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.
Changes look good to me 👍
Looks like one of the checks are complaining about blackifying the code though /shrug
No description provided.