Contact & Conversation Tagging / Articles / Messages#314
Contact & Conversation Tagging / Articles / Messages#314MCKLtech wants to merge 36 commits intointercom:masterfrom
Conversation
- Added Conversation Tagging - Minor bug fixes
GabrielAnca
left a comment
There was a problem hiding this comment.
Thank you so much for this @MCKLtech, we really appreciate that you're taking the time to bring all these endpoints to the SDK! 🚀 I made some minor comments on the PR, but this is going in a great direction!
Apart from the comments, we'd need to update the README file and write tests for all these new methods too 😃
src/IntercomArticles.php
Outdated
| * Updates an existing Article | ||
| * | ||
| * @see https://developers.intercom.com/intercom-api-reference/v0/reference#update-an-article | ||
| * @param array $options |
There was a problem hiding this comment.
I´m missing the phpdoc for @param $id
| * | ||
| * @see https://developers.intercom.com/intercom-api-reference/reference#update-contact | ||
| * @param string $id | ||
| * Updates an existing Contact |
There was a problem hiding this comment.
It seems these lines are a bit off 😅
src/IntercomContacts.php
Outdated
| */ | ||
| public function getContact(string $id, array $options = []) | ||
|
|
||
| public function getContact($id, $options = []) |
There was a problem hiding this comment.
Why are we removing the strong typing here?
| public function getContact($id, $options = []) | |
| public function getContact(string $id, array $options = []) |
There was a problem hiding this comment.
My bad. I think when I merged the change my version was included which wasn't typed. Fixed.
src/IntercomArticles.php
Outdated
| * @return stdClass | ||
| * @throws Exception | ||
| */ | ||
| public function getArticle($id, $options = []) |
There was a problem hiding this comment.
I would prefer all the new methods to be strongly typed
| public function getArticle($id, $options = []) | |
| public function getArticle(string $id, array $options = []) |
Same for the rest of the methods in this file
src/IntercomContacts.php
Outdated
| * | ||
| * @see https://developers.intercom.com/intercom-api-reference/reference#tag-contact | ||
| * @param string $id | ||
| * @param string $tag_id |
There was a problem hiding this comment.
We should use camelCase for all variables in this repo 🙌
| * @param string $tag_id | |
| * @param string $tagId |
src/IntercomContacts.php
Outdated
| */ | ||
| public function addTag(string $id, string $tag_id) | ||
| { | ||
| $path = $this->contactPath($id); |
There was a problem hiding this comment.
I think it would be cleaner if we create a contactTagsPath method in this class, what do you think?
| { | ||
| return $this->client->post("messages", $options); | ||
| } | ||
|
|
There was a problem hiding this comment.
The message export feature is on the Unstable version at the moment and is subject to changes, so I am not sure if we want to add something not stable to the SDK 🤔 @mmartinic @murtron what do you think?
There was a problem hiding this comment.
Agreed. We actually use it in a production system at the moment hence why we built it.
Move to a beta branch or something?
- Updated Readme (Contacts & Articles) - Fixed strong typing - Various fixes as per pull/314
- Duplicated Contacts declaration
- Removed IntercomCustomers
|
|
||
| ```php | ||
| /** Create an Article */ | ||
| $client->articles->create(["title" => "How To Use the Intercom API", "description" => "A quick guide to the universe of the Intercom API", "body" => "<p>This is the body in html</p>", "author_id" => 1]); |
There was a problem hiding this comment.
Would love to split this into multiple lines:
| $client->articles->create(["title" => "How To Use the Intercom API", "description" => "A quick guide to the universe of the Intercom API", "body" => "<p>This is the body in html</p>", "author_id" => 1]); | |
| $client->articles->create([ | |
| "title" => "How To Use the Intercom API", | |
| "description" => "A quick guide to the universe of the Intercom API", | |
| "body" => "<p>This is the body in html</p>", | |
| "author_id" => 1, | |
| ]); |
| $client->articles->getArticle("123456"); | ||
|
|
||
| /** Update an Article */ | ||
| $client->articles->update("123456", ["title" => "How To Use the Intercom API", "description" => "A quick guide to the universe of the Intercom API", "body" => "<p>This is the body in html</p>"); |
There was a problem hiding this comment.
Same here 😃
| $client->articles->update("123456", ["title" => "How To Use the Intercom API", "description" => "A quick guide to the universe of the Intercom API", "body" => "<p>This is the body in html</p>"); | |
| $client->articles->update( | |
| "123456", | |
| [ | |
| "title" => "How To Use the Intercom API", | |
| "description" => "A quick guide to the universe of the Intercom API", | |
| "body" => "<p>This is the body in html</p>", | |
| ] | |
| ); |
src/IntercomClient.php
Outdated
| /** | ||
| * @var IntercomCustomers $customers | ||
| */ | ||
| //public $customers; |
There was a problem hiding this comment.
I know this is work in progress, but wanted to add a reminder that this and its initializer are commented out 😄
- Added getAttributes to Contacts
|
Is this PR going to be merged anytime soon? |
|
This PR is getting very good shape! @MCKLtech do you have time to work on finishing this one? Otherwise, I'm happy to jump in and finish the last bits. |
|
Any progress on this PR @GabrielAnca ? |
|
I'm not going to have time to write tests etc for this in the next few weeks so I'll need someone else to pick up that torch for now. |
|
@GabrielAnca any chance this, and other PRs, could be escalated internally at Intercom? |
- Added Listing for Tags, Segments & Companies
Why?
How?