-
Notifications
You must be signed in to change notification settings - Fork 63
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
Use email address when customer Id not available #80
Conversation
src/Customers/Customer.php
Outdated
$emails = $this->emails->toArray(); | ||
$email = array_shift($emails); | ||
|
||
return $email ?? null; |
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 toArray()
method returns an array of Email
objects. If that behaviour is expected, this should be return $email->getValue() ?? null;
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.
@miken32 I've updated this. The ConversationTest
was passing a string as an email when creating the collection to test the extract
method. I'm fairly sure this is where the simply $email ?? null
check came from.
After updating that test to pass an Email
entry object into the collection, it became apparent what the solution should be in the Customer::getFirstEmail
method.
Thanks for pointing this out.
Guys any news about this issue, I mean when do your plan to deploy fix? Thanks. |
@dacopavlov This is going to go out today once I make a change to address the first comment on that PR. My apologies for the delay. |
* Add convenience methods and make setters fluent * Add fluent setters and type-hints to customer
…i-php into customer-by-email * 'customer-by-email' of github.com:helpscout/helpscout-api-php: Fix getFirstEmail and update test to pass email object into collection Consolidate logic into trait Fix sniff errors Ensure that either customer ID or customer email are present
Looks like this is showing the commits from #82 in here as well... |
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.
Looks great!
In #79, @miken32 reported that attempting to use an email address to create/look-up a customer was failing with a reported error of
"Return value of HelpScout\Api\Customers\Customer::getId() must be of the type integer, null returned"
. Given the docs say that either is fine, this is an issue.After some investigation, I found 2 issues. First, the return type for the
Customer::getId()
method did not allow for a possiblenull
value. Second, when attempting to set the customer data on a conversation or a thread, we were not accounting for the possibility of only having an email address.I updated the
getId
method return type to allow for anull
value. Then, I updated the logic to consider email address when assigning the customer values to theConversation
and any thread that uses theHasCustomer
trait. Finally, I added a convenience method to get the first email address associated with a customer.