249 saas connector zendesk ticket erasure #775
Conversation
ghost
requested a review
from 
docker-compose.yml
Outdated
| @@ -25,7 +25,7 @@ services: | |||
| - /fidesops/src/fidesops.egg-info | |||
| environment: | |||
| - FIDESOPS__DEV_MODE=True | |||
| - FIDESOPS__LOG_PII=${FIDESOPS__LOG_PII} | |||
There was a problem hiding this comment.
Let's revert this back to ${FIDESOPS__LOG_PII}.
pytest.ini
Outdated
| @@ -1,7 +1,8 @@ | |||
| [pytest] | |||
| env = | |||
| TESTING=True | |||
| log_cli=False | |||
There was a problem hiding this comment.
Let's also set log_cli back to True and get rid of the log_level line.
| method=HTTPMethod.POST, | ||
| path=f"/api/v2/users", | ||
| headers=headers, | ||
| body=body, |
There was a problem hiding this comment.
We can just use json=body so we don't have to pass in the Content-Type header.
| } | ||
| } | ||
| ) | ||
| updated_headers, formatted_body = format_body({}, body) |
There was a problem hiding this comment.
We don't need to call json.dumps or format_body we can just use the user dict directly like you did on line 150-161.
| base_url = f"https://{zendesk_secrets['domain']}" | ||
| headers = { | ||
| "Content-Type": "application/json", | ||
| "Authorization": "Basic {}".format( |
There was a problem hiding this comment.
The requests library has a shorthand for this
https://requests.readthedocs.io/en/latest/user/authentication/
you should just able to define the auth then use it in your request
auth = zendesk_secrets['username'], zendesk_secrets['api_key']
...
requests.post(url=..., auth=auth, json=...)
| ), | ||
| } | ||
|
|
||
| connector = SaaSConnector(zendesk_connection_config) |
There was a problem hiding this comment.
Can you rewrite this call to use requests instead of creating a SaaSConnector. I'd like to stay away from using SaaS connector logic to create test data.
| base_url = f"https://{zendesk_secrets['domain']}" | ||
| headers = { | ||
| "Content-Type": "application/json", | ||
| "Authorization": "Basic {}".format( |
There was a problem hiding this comment.
Same as the previous comment.
| # Since user is deleted, it won't be available so response is 404 | ||
| assert response.status_code == 404 | ||
|
|
||
| tickets = x[f"{dataset_name}:tickets"] |
There was a problem hiding this comment.
Looks like you're reading how many tickets were deleted and not doing anything with the value.
| assert response.status_code == 404 | ||
|
|
||
| tickets = x[f"{dataset_name}:tickets"] | ||
| ticket_id = v[f"{dataset_name}:tickets"][0]["id"] |
There was a problem hiding this comment.
I think a better approach would be to iterate through all the tickets in v[f"{dataset_name}:tickets"] and make sure they're all deleted.
| tickets = x[f"{dataset_name}:tickets"] | ||
| ticket_id = v[f"{dataset_name}:tickets"][0]["id"] | ||
| response = requests.get( | ||
| url=f"{base_url}/v2/tickets/f{ticket_id}.json", |
There was a problem hiding this comment.
Looks like you have an extra character f before the {ticket_id}, this is most likely returning a 404 because it's an invalid path and not because it's actually checking the ticket. Make sure you have the correct path.
galvana
added
run unsafe ci checks
Co-authored-by: Hamza W <hamza@Hamzas-MacBook-Pro.local> Co-authored-by: Adrian Galvan <adriang430@gmail.com>
Purpose
Purpose of this PR is to add Zendesk User and Ticket Erasure endpoints.
Changes
Checklist
CHANGELOG.mdfileCHANGELOG.mdfile is being appended toUnreleasedsection in an appropriate category. Add a new category from the list at the top of the file if the needed one isn't already there.Run Unsafe PR Checkslabel has been applied, and checks have passed, if this PR touches any external servicesTicket
Fixes #648