-
Notifications
You must be signed in to change notification settings - Fork 16
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
IBX-6645: User profile #1020
IBX-6645: User profile #1020
Conversation
9bf5cc5
to
15655f3
Compare
1cc5565
to
2561093
Compare
@adamwojs conflict needs to be resolved |
->booleanNode('enabled') | ||
->defaultFalse() | ||
->end() | ||
->arrayNode('content_types') |
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.
what is the point of this configuration? Cant we just not relay on user_content_type_identifier
parameter? I think introducing another one would just add confusion, even more when this is site access aware, and we decided that this dosent have much sense in terms of defining content types.
Unless, there is something else behind this config, then please, elaborate.
@adamwojs
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.
We want to enable user profile only for marketer persona, what is only small part of the user base.
src/bundle/Resources/views/themes/admin/account/profile/view.html.twig
Outdated
Show resolved
Hide resolved
src/bundle/ui-dev/src/modules/sub-items/components/table-view/table.view.item.component.js
Outdated
Show resolved
Hide resolved
The thing that raises my concerns the most - why do you want to sudo load user roles just to display them? If the user doesn't have permissions I think it should be respected and not create a loophole in the system to look up assigned user roles. |
3f9278a
to
5f39694
Compare
b052b83
to
514747a
Compare
The thing that raises my concerns the most - why do you want to sudo load user roles just to display them? If the user doesn't have permissions I think it should be respected and not create a loophole in the system to look up assigned user roles. I've added logic to hide "Roles" block when user doesn't have permissions to view roles. |
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 4 New issues |
Thank you for review, all your core review suggestions has been applied. Merging due to the time constraints, however if there will be any further comment I will apply them as follow up PR. |
Depends on ibexa/core#301 ✅ and ibexa/content-forms#57 ✅
https://www.figma.com/file/NwbQ3GhHqJlZ0Xspd4wd7n/User-profile?type=design&node-id=5301-64169&mode=design&t=Qwf09rz382J0pe3B-0
Screenshot
Checklist:
$ composer fix-cs
)