-
Notifications
You must be signed in to change notification settings - Fork 57
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
EZP-31767: Use version contributor name on dashboard #1437
Conversation
* | ||
* @return \eZ\Publish\API\Repository\Values\User\User|null | ||
*/ | ||
protected function getContributor(ContentInfo $contentInfo): ?User | ||
protected function getVersionContributor(VersionInfo $versionInfo): ?User |
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.
Not sure if renaming protected function in abstract class is the safest way to do, it should be internal but maybe someone else have opinion on that too.
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.
May I create a separate getVersionContributor
method, and keep getContributor
as is? And, mark getContributor
with @deprecated
?
$userExists = (new UserExists($this->userService))->isSatisfiedBy($contentInfo->ownerId); | ||
|
||
if (false === $userExists) { | ||
try { |
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.
Any reason why you did not reuse Specification
just with different ID provided?
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.
@ViniTou This method called for each row, so this will save us a few userService->loadUser()
calls, which will make the dashboard page a bit faster.
IMHO, UserExists->isSatisfiedBy
is overcomplicating the simple thing, user service is doing this job quite well already. Probably, it is the reason why it's not used commonly through the code.
Let me know if these few extra calls are not a big deal and I'll change it to use UserExists
.
Thanks.
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.
True, I didnt notice the last removed call, fine with me then.
Thank you for contribution @ITernovtsiy! |
Currently, on content view page, we have displayed name of user who last edited the content.
But creator name displayed as a contributor on the dashboard (In common content block)
I would expect to see name of user who last edited the content.
Checklist:
$ composer fix-cs
)