Add UserMerge command #26
Conversation
foglcz
requested review from
kubasimon,
davidkmenta,
jirinovak,
kdosiodjinud,
MortalFlesh,
hokypierce,
MarketaSebkova,
florianjiri and
a user
src/Model/Command/UserMerge.php
Outdated
| private $targetUserId; | ||
|
|
||
| /** | ||
| * Merge source user to target user AND DELETE SOURCE USER. |
There was a problem hiding this comment.
Same as in #27. I think we should merge constructor and class comments in this particular case.
There was a problem hiding this comment.
So you'd like to have the PHPDoc as class comment or as constructor comment?
IMHO we may have some more detailed information and documentation in the class commend, and basic information in the constructor comment.
src/Model/Command/UserMerge.php
Outdated
| /** | ||
| * Merge source user to target user AND DELETE SOURCE USER. | ||
| */ | ||
| public function __construct(string $sourceUserId, string $targetUserId) |
There was a problem hiding this comment.
All other Commands (ok, both of them :-D) could be instantiated only through the static constructor. My idea was to not mix two approaches (new vs static constructor) and even though some commands do need just one way of instantiation (thus the ::create() method), I would prefer to make the static constructors the only way of Command instantiation (and make the __construct() private), so there is less cognitive load for the developer and also the usage is unified.
src/Model/Command/UserMerge.php
Outdated
| /** | ||
| * Merge source user to target user AND DELETE SOURCE USER. | ||
| */ | ||
| public static function create(string $sourceUserId, string $targetUserId): self |
There was a problem hiding this comment.
I'm just thinking if having both of those constructors (which differs just with parameter order) will not make more mess... 🤔
src/Model/Command/UserMerge.php
Outdated
| /** | ||
| * Merge source user to target user AND DELETE SOURCE USER. | ||
| */ | ||
| public static function create(string $sourceUserId, string $targetUserId): self |
There was a problem hiding this comment.
I tend to place the static named constructors right after __construct, so they are "together" on one place.
src/Model/Command/UserMerge.php
Outdated
| /** | ||
| * Merge source user into target user AND DELETE SOURCE USER. | ||
| */ | ||
| public static function mergeInto(string $targetUserId, string $sourceUserId): self |
There was a problem hiding this comment.
Maybe make the "delete" thing more obvious by naming the variable eg. $sourceUserIdToBeDeleted :)?
| class UserMergeTest extends TestCase | ||
| { | ||
| /** | ||
| * @test |
There was a problem hiding this comment.
I prefer to use one-line annotations if it is possible.
|
|
||
| use PHPUnit\Framework\TestCase; | ||
|
|
||
| class UserMergeTest extends TestCase |
There was a problem hiding this comment.
We extend "our" TestCase in other tests... It has no added value here right now, but we may do it here as well to make it unified.
OndraM
force-pushed
the
feature/user-merge-standalone
branch
from
8e193e7
to
23b8429
Compare
First half of #23
This does not implement any actual call to Matej endpoints - those will be implemented in separate PRs.