EZP-28917: As a Developer I want API to manipulate User Tokens #2270
Conversation
mikadamczyk
force-pushed
the
ezp-28792-reset-forgotten-password
branch
from
b8d2930
to
b719ff3
Compare
alongosz
changed the title
There was a problem hiding this comment.
This is clearly a separate thing, so I've added dedicated sub-task (should be a Story but we have to deliver it in this Sprint).
We've discussed internally most of the issues and needed changes. I summarized them in EZP-28917.
Other things that I've noticed:
| /** | ||
| * Update the user acoount key specified by the user account key struct. | ||
| * | ||
| * @param UserTokenUpdateStruct $userTokenUpdateStruct |
| */ | ||
| public function loadUserByToken($hash) | ||
| { | ||
| $query = $this->handler->createSelectQuery(); |
There was a problem hiding this comment.
Hmm, the whole DatabaseHandler interface is deprecated since 6.13.
We're now using directly \Doctrine\DBAL\Connection which here can be obtained by $this->handler->getConnection(). Later on we'll replace it with injected connection.
So the request may appear inconsistent with the rest of the code, but to make future upgrade easier please rewrite this query using Doctrine Query Builder ($connection->createQueryBuilder()). Example could be found here.
Sorry for the inconvenience.
// Edit: Will be handled when improving entire Gateway.
| $statement = $query->prepare(); | ||
| $statement->execute(); | ||
|
|
||
| return $statement->fetchAll(\PDO::FETCH_ASSOC); |
There was a problem hiding this comment.
Please make sure it always returns array, so for empty result set [] instead of false. We tend to forget that PDO drivers aren't perfect ;)
// Edit: disregard, that one works properly.
| /** | ||
| * Update or insert the user token information specified by the user token struct. | ||
| * | ||
| * @param UserTokenUpdateStruct $userTokenUpdateStruct |
| */ | ||
| public function updateUserToken(UserTokenUpdateStruct $userTokenUpdateStruct) | ||
| { | ||
| $query = $this->handler->createSelectQuery(); |
There was a problem hiding this comment.
Same request about using methods from the deprecated interface.
| * | ||
| * {@inheritdoc} | ||
| * | ||
| * @param string $email |
| * @param \eZ\Publish\API\Repository\Values\User\UserTokenUpdateStruct $userTokenUpdateStruct | ||
| * | ||
| * @throws \eZ\Publish\API\Repository\Exceptions\ContentFieldValidationException if a field in the $userTokenUpdateStruct is not valid | ||
| * @throws \eZ\Publish\API\Repository\Exceptions\ContentValidationException if a required field is set empty |
There was a problem hiding this comment.
Do these exceptions apply here?
| new UpdateUserTokenSignal( | ||
| array( | ||
| 'userId' => $user->id, | ||
| ) |
There was a problem hiding this comment.
Please use short array syntax for new code.
| * | ||
| * @var int | ||
| */ | ||
| public $userId; |
There was a problem hiding this comment.
This is not needed because the usage of that struct is UserService::updateUserToken($user, $userTokenUpdateStruct).
Of course similar property on SPI level is ok.
| * @param string $hash | ||
| * @param array $prioritizedLanguages | ||
| * | ||
| * @return User |
|
|
||
| $this->assertQueryResult( | ||
| [['0800fc577294c34e0b28ad2839435945', 1, 2234567890, 42]], | ||
| $this->handler->createSelectQuery()->select('*')->from('ezuser_accountkey'), |
There was a problem hiding this comment.
Hmm, unit tests should not use real database, they do in some places by accident. I tried to do some integration on gateway level, but the code there also will need to be refactored.
What we need instead of this are tests in \eZ\Publish\API\Repository\Tests\UserServiceTest.
The more practical reason for that is that is that unit tests if use DB by accident, it is Sqlite only. Integration tests do it for MySQL, PostgreSQL and Sqlite.
Update: I've added integration tests. Let's keep this, I'll handle it in the future when we decide where to put gateway <-> db integration tests.
alongosz
force-pushed
the
ezp-28792-reset-forgotten-password
branch
from
b9dc18b
to
68887c3
Compare
It allows Users w/o content / edit policy to update their password BC: If User has content / edit policy, but no user / password policy, updating password works as well
alongosz
force-pushed
the
ezp-28792-reset-forgotten-password
branch
from
68887c3
to
2d0741e
Compare
|
Added/fixed some PhpDocs (changes not affecting review). Waiting for fresh CI (some other things got merged after this one ran) |
|
Merged, I'll double check CI when triggered for |
master* We need to document that changing password requires at least
user / passwordpolicy.This PR adds the new loadUserByToken, updateUserToken and expireUserToken methods which is needed to be able to reset user password.
TODO:
$ composer fix-cs).