-
Notifications
You must be signed in to change notification settings - Fork 27
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
Add new functionality to HelpScout::Threads (create, update, get) #13
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.
@jjustinwhite thanks for the PR! I've left a few small comments but generally speaking this looks great. I'm looking in to fixing our Travis configuration so that PR builds will run correctly.
I think our VCR sanitization is strong enough that you should be able to record cassettes for these if you'd like, but if you'd prefer, I'm happy to record them myself and add them to the PR. Just let me know.
Thanks again for your work here 🤘
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.
@andremleblanc -- I went ahead and implemented my suggested changes. This could probably use another quick set of eyes for final approval.
Thanks again @jjustinwhite for the initial work!
Eek, I missed my notifications on this! Nice work @prsimp :) I'd never used VCR, and I tried figuring out how to record my own, but never figured it out. I'll have to look through what you did and figure out what I was doing wrong. Thanks for finishing that off! |
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.
Couple of nitpicks, but overall looks good to me. Will test this a little later tonight.
@@ -2,6 +2,15 @@ | |||
|
|||
RSpec.describe HelpScout::Thread do | |||
let(:conversation_id) { ENV.fetch('TEST_CONVERSATION_ID') } | |||
let(:thread_id) { ENV.fetch('TEST_THREAD_ID') } | |||
|
|||
describe '.get' do |
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.
Feels like our getable shared example could be expanded to handle multiple args, but since I think this is the first instance of it, I'm cool with saving that for later.
I think it would also be nice to add subclasses to thread (or some other abstraction) to document what valid thread types are. For instance, to create a chat thread, you'd do something like:
What do you think about something like:
That being said, I could see adding this abstraction later and not having to break the interface, so I'm good with this for now. |
Tested this locally and works as expected 👍 |
Thanks for the review @andremleblanc & @prsimp! I agree, subclasses like |
Absolutely. I think the same can be said for
@jjustinwhite thanks! Feel free to take a stab at it if you find some time -- I'll open an Issue for some ongoing discussion about the topic. |
This adds functionality to create and update a thread via the API. It also attempts to provide a "get" functionality, despite the HelpScout 2.0 API not supporting such an endpoint. It also update the README to reflect the API functionality lacking for a true GET and DELETE for Threads.