Dan.c/2.0

[RaneLafraze]

Why?

Fulfills many of the suggestions on the current PR #306

How?

• Created dedicated methods inside IntercomCompanies for attaching and detaching contacts
• Replace sendRequest() calls with the respective post() and get() methods
• Update phpdocs for new methods

[GabrielAnca]

I have mixed feelings about something, so hope to get your view on this too 😄 On one hand, this API wrapper is super-lightweight and we generally just take one parameter for the id and other for the payload (options). In this case, this call requires the options to contain a specific parameter, so I think being more specific here would be helpful, ie

Suggested change

	public function attachContact($contactId, $options)
	public function attachContact(string $contactId, string $companyId)
	{
	$options = [ "id" => $companyId ];
	...
	}

What do you think about that change? I'm happy with keeping it as you did for consistency, but also happy to move to what I proposed 😄

[RaneLafraze]

You raise a good point, and I agree that the function should take a specific parameter for the $companyId. In order to provide as much flexibility as possible, I was thinking of keeping $options as a parameter and using array_merge like so:

Suggested change

	public function attachContact($contactId, $options)
	public function attachContact(string $contactId, string $companyId, $options)
	{
	$options = array_merge($options, ["id" => $companyId]);
	...
	}

What are your thoughts on this?

[GabrielAnca]

Makes total sense! That would definitely make it more flexible in case we add additional parameters in the future 😄 Let's do that, but adding a default for $options:

Suggested change

	public function attachContact($contactId, $options)
	public function attachContact(string $contactId, string $companyId, array $options = [])
	{
	$options = array_merge($options, ["id" => $companyId]);
	...
	}

[GabrielAnca]

Would be great to create a method to store the complexity of this path, ie $this->companyDetachPath($contactId, $companyId);

[GabrielAnca]

Thank you so much for your contribution @RaneLafraze 🚀 I've made a couple of comments and I think we'd be ready to merge into the main PR 😄

[RaneLafraze]

Thank you @GabrielAnca ! I've made the changes discussed above. I'm on board in terms of merging into the main branch, but I'd greatly appreciate your advice and final review. Thanks again!

[GabrielAnca]

Thanks for making the changes @RaneLafraze 🎉 I just found one more thing I didn't see before. Other than that, we are ready to merge this into @lloydliyu's branch

[RaneLafraze]

Perfect, thanks again @GabrielAnca ! Please let me know if there is anything else I need to do on my end, or if there are any other ways I can help out!

[GabrielAnca]

Looks great @RaneLafraze 🎉 Sorry I was out of the office for the last couple of days. Let's merge this one into the original PR and I'll have a look at it right now 😄