-
Notifications
You must be signed in to change notification settings - Fork 141
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
Contact & Conversation Tagging / Articles / Messages #314
base: master
Are you sure you want to change the base?
Conversation
- Added Conversation Tagging - Minor bug fixes
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I´m missing the phpdoc for @param $id
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.
Adding Fix.
@@ -25,13 +24,18 @@ public function create(array $options) | |||
* | |||
* @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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems these lines are a bit 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.
Woops. Fixing
src/IntercomContacts.php
Outdated
@@ -57,7 +62,8 @@ public function getContacts(array $options = []) | |||
* @return stdClass | |||
* @throws Exception | |||
*/ | |||
public function getContact(string $id, array $options = []) | |||
|
|||
public function getContact($id, $options = []) |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.
Fixed.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use camelCase for all variables in this repo 🙌
* @param string $tag_id | |
* @param string $tagId |
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.
WeCanCertainlyDoThat.
Fixed.
src/IntercomContacts.php
Outdated
*/ | ||
public function addTag(string $id, string $tag_id) | ||
{ | ||
$path = $this->contactPath($id); |
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.
I think it would be cleaner if we create a contactTagsPath
method in this class, what do you think?
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.
Good call, adding now.
@@ -19,4 +19,45 @@ public function create($options) | |||
{ | |||
return $this->client->post("messages", $options); | |||
} | |||
|
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?