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

Contact & Conversation Tagging / Articles / Messages #314

Open
wants to merge 36 commits into
base: master
Choose a base branch
from

Conversation

MCKLtech
Copy link

@MCKLtech MCKLtech commented Sep 2, 2020

Why?

  • Added Articles endpoint
  • Added Contact Tagging
  • Added Conversation Tagging
  • Added additional Messages endpoints including Exports

How?

  • N/A

Copy link
Contributor

@GabrielAnca GabrielAnca left a 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 😃

* Updates an existing Article
*
* @see https://developers.intercom.com/intercom-api-reference/v0/reference#update-an-article
* @param array $options
Copy link
Contributor

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

Copy link
Author

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
Copy link
Contributor

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 😅

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops. Fixing

@@ -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 = [])
Copy link
Contributor

@GabrielAnca GabrielAnca Sep 2, 2020

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?

Suggested change
public function getContact($id, $options = [])
public function getContact(string $id, array $options = [])

Copy link
Author

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.

* @return stdClass
* @throws Exception
*/
public function getArticle($id, $options = [])
Copy link
Contributor

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

Suggested change
public function getArticle($id, $options = [])
public function getArticle(string $id, array $options = [])

Same for the rest of the methods in this file

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

*
* @see https://developers.intercom.com/intercom-api-reference/reference#tag-contact
* @param string $id
* @param string $tag_id
Copy link
Contributor

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 🙌

Suggested change
* @param string $tag_id
* @param string $tagId

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WeCanCertainlyDoThat.

Fixed.

*/
public function addTag(string $id, string $tag_id)
{
$path = $this->contactPath($id);
Copy link
Contributor

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?

Copy link
Author

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);
}

Copy link
Contributor

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?

Copy link
Author

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?

Colin Longworth and others added 3 commits September 10, 2020 15:06
- 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]);
Copy link
Contributor

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:

Suggested change
$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>");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here 😃

Suggested change
$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>",
]
);

/**
* @var IntercomCustomers $customers
*/
//public $customers;
Copy link
Contributor

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
@harrisbp
Copy link

harrisbp commented Dec 3, 2020

Is this PR going to be merged anytime soon?

@GabrielAnca
Copy link
Contributor

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.

@marijnbent
Copy link

Any progress on this PR @GabrielAnca ?

@MCKLtech
Copy link
Author

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.

@MCKLtech
Copy link
Author

@GabrielAnca any chance this, and other PRs, could be escalated internally at Intercom?

Colin Longworth added 2 commits February 24, 2021 09:18
- Added Listing for Tags, Segments & Companies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants