Skip to content
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

Added "spam" and "deleted" results to the sync of tickets #26

Merged
merged 3 commits into from
Nov 9, 2018

Conversation

justedro
Copy link
Contributor

@justedro justedro commented Nov 2, 2018

By default, an endpoint doesn't return the deleted and marked as spam tickets. Needed to specify the predefined filter to retrieve them.
https://developers.freshdesk.com/api/#list_all_tickets

I have created separate streams for them: tickets_deleted and tickets_spam, the state is being written properly.

{
  "type": "STATE",
  "value": {
    "tickets": "2018-11-02T09:44:37Z",
    ...
    "tickets_deleted": "2018-11-02T09:26:57Z",
    "tickets_spam": "2018-11-02T08:51:06Z"
  }
}

@justedro
Copy link
Contributor Author

justedro commented Nov 2, 2018

The main problem is that we can't see what of the previously synced tickets were deleted or marked as spam because we don't get the updates.

@dmosorast
Copy link
Contributor

Thanks for this @justedro! At a glance, it looks pretty good. Changes that add new streams can be quite tricky to review, and I'd like to be able to have a deep dive into this before merging. We should be able to get back to you at some point early next week!

One small thing that jumped out, is that, since this is a feature addition rather than a fix, I would say the version number should increase by a full minor version (to 0.10.0).

@justedro
Copy link
Contributor Author

justedro commented Nov 5, 2018

Sure, @dmosorast, increased the version number.

@nick-mccoy
Copy link
Contributor

Hey @justedro -- this looks good, and if you can provide some gists of log snippets showing records being emitted for tickets_deleted and tickets_spam, then I will go ahead and merge/release this change.

@justedro
Copy link
Contributor Author

justedro commented Nov 7, 2018

@nick-mccoy, unfortunately, I can't send you the full log so I prepared some examples with non-sensitive info:
Spam: https://gist.github.com/JustEdro/d3d648861788d688c30602c48a9c6b60
Deleted: https://gist.github.com/JustEdro/5a5ab04a947e016a161742c9ecbb97bc

@dmosorast thanks for the review, fixed it as you suggested.

@nick-mccoy nick-mccoy merged commit a929ebd into singer-io:master Nov 9, 2018
@nick-mccoy
Copy link
Contributor

This is now merged and deployed in Stitch

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.

3 participants