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

Deprecating Customer's Chat in favor of ChatHandle #102

Merged
merged 3 commits into from Mar 22, 2019

Conversation

Projects
None yet
3 participants
@bkuhl
Copy link

bkuhl commented Mar 6, 2019

I'd like to get some feedback on whether this is a good idea and/or if we should either toss it or tackle it differently.

Currently we use the terminology "chat" and "chats" for Customers. This can be a bit confusing because it's really a "chat handle", not any kind of conversation. If we add Chat reports or any other chat-related functionality it could create confusion for SDK users. Since we built the SDK, the api has been updated to use the terminology "chat handle".

To maintain backwards compatibility and adjust terminology to be consistent and more descriptive here's what I'm proposing with this PR:

  • Chat entity is deprecated in favor of ChatHandle, but ChatHandle currently extends Chat to maintain compatibility with scenarios such as if ($chat instanceOf Chat)
  • A Customer would hydrate all eagerly loaded chat attributes to ChatHandle, which is partially why ^ is important
  • A Customer's setChats/getChats are deprecated in favor of using setChatHandles/getChatHandles
  • Because the API itself hasn't changed outside of just the documentation, the attribute that comes back in the json response is still chats, we would just be mapping it to ChatHandle

What I don't like about this solution is having so much deprecation. But I like the naming improvement over the deprecation especially considering we have other aspects of the product/business where "Chat" refers to something completely different that may eventually be added to the API/SDK, especially from a reporting perspective.

We're still early in SDK adoption so deprecating now would have much lower impact than later. This PR is related to #100

What are your thoughts?

bkuhl added some commits Mar 6, 2019

@bkuhl bkuhl requested review from davidstanley01 and tompedals Mar 6, 2019

@tompedals
Copy link

tompedals left a comment

This makes sense to me. Since you've kept Chat and just extended it I see no problem with merging this now either. Nice work.

@davidstanley01
Copy link
Contributor

davidstanley01 left a comment

I like how you've handled this. Good work, sir.

@bkuhl bkuhl merged commit 9c82776 into v2 Mar 22, 2019

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@bkuhl bkuhl deleted the deprecating-chat-methods branch Mar 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.