-
Notifications
You must be signed in to change notification settings - Fork 38
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
Feature/request ticket audits per ticket #14
Conversation
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.
Overall, this looks awesome.
Just a couple comments, one change at a "should fix" level.
tap_zendesk/streams.py
Outdated
|
||
last_record_emit = {} | ||
def _buffer_record(self, buf, record): | ||
buf = {} | ||
buf_time = 60 |
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 was thinking about this. I like it, keep the timer to align the same with the parent stream's metric emitting frequency.
tap_zendesk/streams.py
Outdated
|
||
if audits_stream.is_selected(): | ||
LOGGER.info("Syncing ticket_audits per ticket...") | ||
|
||
for ticket in tickets: | ||
if utils.strptime_with_tz(ticket.updated_at) < bookmark: | ||
generated_timestamp_dt = datetime.datetime.utcfromtimestamp(ticket.generated_timestamp).replace(tzinfo=pytz.UTC) | ||
if generated_timestamp_dt < bookmark: |
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.
You know, I wonder if this won't get hit anymore. I bet we were seeing that with updated_at
for the same reason we're using generated_timestamp
now.
tap_zendesk/streams.py
Outdated
if ticket_dict["status"] == "deleted": | ||
LOGGER.warning("Unable to retrieve audits for deleted ticket (ID: %s), skipping...", ticket_dict["id"]) | ||
continue | ||
if is_deleted: |
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.
This should only be printed within the if <sub_stream>.is_selected()
block below, in case one or the other isn't selected.
tap_zendesk/streams.py
Outdated
if is_deleted: | ||
LOGGER.warning("Unable to retrieve audits/metrics for deleted ticket (ID: %s), skipping...", ticket_dict["id"]) | ||
|
||
if audits_stream.is_selected() and not is_deleted: |
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 wouldn't check is_deleted
here, but like this:
if audits_stream.is_selected():
if not is_deleted:
# Do the substream
else:
LOGGER.warning("Unable to retrieve audits for deleted....")
That way it's not confusing throwing other stream names into the logs.
tap_zendesk/streams.py
Outdated
count = 0 | ||
|
||
def sync(self, ticket_id): | ||
ticket_audits = self.client.tickets.audits(ticket=ticket_id) | ||
for ticket_audit in ticket_audits: | ||
ticket_audit_dict = ticket_audit.to_dict() | ||
self.count += 1 | ||
|
||
if utils.strptime_with_tz(ticket_audit_dict["created_at"]) < self.oldest_date: | ||
self.oldest_date = utils.strptime_with_tz(ticket_audit_dict["created_at"]) |
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.
Good catch.
tap_zendesk/streams.py
Outdated
|
||
def sync(self, ticket_id): | ||
ticket_metric = self.client.tickets.metrics(ticket=ticket_id) | ||
ticket_metric_dict = ticket_metric.to_dict() |
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.
The to_dict
isn't actually necessary, not sure why we were doing it for Ticket, but this should be handled by the custom JSON Serializer in this tap.
The zendesk endpoint to get all ticket audits does not return audits for archived tickets (tickets are archived after 4 months).
This PR changes the logic for getting all ticket_audits -- we request all tickets, loop over the tickets, and fetch the audits for each ticket.
Also addresses #10 and #11