Add support for parameterised ticket fetches, and add support for timesheet fetches

[oskarpearson]

This patch adds support for ticket fetches in 'hash' format, so that we can do filtered ticket fetches. So far as I can see, this should work correctly, but our use has been limited to only a few specific types of fetches.

It also adds support for fetching timesheets from Freshdesk.

Thanks for the useful library!

[tsmacdonald]

Thanks for the contribution!

I don't really like the explicit type check (Line 51). It seems better to pass in an options hash that was either { id: ticket_id} or { params: { filter_name: "all", page: value}}. Sort of like how we do attachments in fd_define_post. Would you mind refactoring?

[oskarpearson]

Hi

I was trying to keep backwards compatibility with the previous calling convention, so other library users wouldn't need to change their code after the patch.

My understanding is that if we do this, anyone that uses the library for fetching individual tickets will need to change to a new structure. Are you happy with that?

For example the existing calling convention is something like this (from the library readme):

• client.get_tickets(id - optional)
• client.get_companies(id - optional)

If I remove the explicit type check (and support for substituting in the string) all users would need to change to something like this:

• client.get_tickets(:id => ticket_id)

I'm happy to do it, but not sure if all the other users would be happy ;)

Let me know if you want me to go ahead.

Thanks again for a useful library.

Oskar

[oskarpearson]

Two additional points:

1. On review, the current code doesn't correctly handle being passed numeric IDs (only strings) - so you're 100% correct the current type check is incorrect. Sorry about that.
2. I'm not sure if you'd be ok with me reversing the check? i.e. I check if it's a Hash and then use the hash values, but otherwise default to using the substitution. This would preserve backwards compatibility, albeit with a different sort of type check.

Thanks,

Oskar

[tsmacdonald]

I was trying to keep backwards compatibility with the previous calling convention

:+1:

I'm not sure if you'd be ok with me reversing the check

I like this better—it seems like in most cases we'd just want a stringified version of something to appear as the ID, and that hashes are a very specific use case to pass in more data. Happy to merge if you reverse it.

Thank dvliman for the library—he did the majority of the work. I just added a few minor patches and then became the maintainer.

Thanks again for the feature!