-
-
Notifications
You must be signed in to change notification settings - Fork 235
FEATURE: Reimplementation: show contact person for inaccessible media usages #5578
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
FEATURE: Reimplementation: show contact person for inaccessible media usages #5578
Conversation
67309aa to
a7b5ba2
Compare
- rewrite controller function to get data from new workspace structure - changes structure of ´RelatedNodes.html´ to match new structure Ticket: neos#5437
a7b5ba2 to
3a9e942
Compare
- will be shown as ´---´ - removed in list view after ´./flow assetusage:index´ - if 'private' workspace exist wait for next user to join to show this name Ticket: neos#5437
| /** | ||
| * @Flow\Inject | ||
| * @var DomainUserService | ||
| */ | ||
| protected $domainUserService; | ||
|
|
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.
| /** | |
| * @Flow\Inject | |
| * @var DomainUserService | |
| */ | |
| protected $domainUserService; | |
| #[Flow\Inject] | |
| protected DomainUserService $domainUserService; |
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.
I was just taking the format from the other protected variables of the file to make it consistent.
Should I just apply this suggestion, rework the variables as well or leave it as it is? 🤔
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.
I'm fine with both! Also with keeping it as it is. 😬
Using the attributes over annotation does not work for all cases. So you need to be careful rewriting all of them.
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.
Note: Rewriting annotation to attributes work, but not the PHP type definition in all cases. But AFAIS this is an older issue.
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 good by reading! Thanks @Jan3k3y for taking care.
|
Any updates here? 🤗 |
| {inaccessibleRelation.label} | ||
| <f:if condition="{inaccessibleRelation.relevantWorkspaceMetadata.owner}"> | ||
| <f:then> | ||
| (@{inaccessibleRelation.relevantWorkspaceMetadata.owner}) |
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.
This leads to an error
| (@{inaccessibleRelation.relevantWorkspaceMetadata.owner}) | |
| (@{inaccessibleRelation.relevantWorkspaceMetadata.owner.label}) |
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.
Thx for testing!
This will be solved by not giving full Entity into template
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.
Thank you for taking care of the todo!
I had some comments, but some are also personal preference that you can also decline 😄
I noticed some additonal stuff while testing out the feature. Those can be handled in a follow-up since they are not part of the reimplementation.
- Table header for Element in Workspace is 'document', but the node is shown in the table
- icon fa-group and its title tag are not shown, also the title is not translated. Icon should be fa-user-group
| $owner = $workspace->getOwner(); | ||
| return '(' . $owner->getLabel() . ')'; | ||
| $contentRepositoryId = SiteDetectionResult::fromRequest($this->request->getHttpRequest())->contentRepositoryId; | ||
| $contentRepository = $this->contentRepositoryRegistry->get($contentRepositoryId); |
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.
I would prefer for this to be passed to the function avoid duplicate calls
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.
💡
| } | ||
|
|
||
| if ($workspaceMetadata->classification->value === WorkspaceClassification::PERSONAL->value) { | ||
| $structuredReturn['owner'] = $workspaceOwner; |
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.
As far as I can see, we only use the title of the workspace user, do we need to pass the whole object to the view or could we just use the label here?
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.
Sounds good 🤗
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.
Two comments to fix the nullable variables :)
Co-authored-by: pKallert <[email protected]>
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 good!
Tested it out locally and works for me
Ticket: #5437
Checklist
FEATURE|TASK|BUGFIX!!!and have upgrade-instructions