36784 Plugin can not set supervisor in address record #594
Conversation
Pull Request Test Coverage Report for Build 5886855372
💛 - Coveralls |
There was a problem hiding this comment.
@dai-eastgate
Looks good. I just found a missing part in documentation. Please take a look. :)
| @@ -68,7 +68,7 @@ public function __construct( | |||
| * @throws NotFoundException | |||
| */ | |||
| public function createOrCompleteAddress( | |||
| FormData $pFormData, bool $mergeExisting = false, string $contactType = ''): int | |||
| FormData $pFormData, bool $mergeExisting = false, string $contactType = '', int $estateId = null): int | |||
There was a problem hiding this comment.
Could you please add PHPDoc for these arguments: $contactType and $estateId for complete documentation? :)
| @@ -82,6 +82,13 @@ public function createOrCompleteAddress( | |||
| if ( key_exists( 'newsletter', $requestParams ) ) { | |||
| unset( $requestParams['newsletter'] ); | |||
| } | |||
| if (!empty($estateId)) { | |||
| $userName = $this->getSupervisorUsernameByEstateId($estateId); | |||
| if (!empty($userName)) { | |||
There was a problem hiding this comment.
In the past we got minor bugs with using empty() function because "0" and 0 is also empty true. It is unlikely but what happens if the ID or username is 0?
Do you think using isset could be better or could causes other errors?
Edit:
Oh I see getSupervisorUsernameByEstateId returns maybe an empty string. So I guess we need empty() here.
There was a problem hiding this comment.
I'll check and let you know asap
|
@andernath I fixed it, please review again for me. Thanks |
|
Steps to install the approved version:
|
andernath
deleted the
36784-plugin-can-not-set-supervisor-in-address-record
branch
andernath
added
ready for release
and removed
QA
related to #553
changed log :
Add supervisor to address record when sending contact form in detail page