Skip to content
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.

Sign up for GitHub

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

Jump to bottom

IBX-715: Disabled buttons on the right sidebar if user has no permissions #1794

Merged
merged 8 commits into from
Aug 26, 2021
Merged

IBX-715: Disabled buttons on the right sidebar if user has no permissions #1794

merged 8 commits into from
Aug 26, 2021

Conversation

mateuszdebinski
Copy link
Contributor

@mateuszdebinski mateuszdebinski commented Jul 6, 2021 •
edited
Loading

Question Answer
Tickets IBX-715
Bug fix? no
New feature? no
BC breaks? no
Tests pass? yes/no
Doc needed? no
License GPL-2.0

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@mateuszdebinski mateuszdebinski self-assigned this Jul 6, 2021
@mateuszdebinski mateuszdebinski marked this pull request as ready for review July 8, 2021 08:09
@mateuszdebinski mateuszdebinski requested a review from a team July 8, 2021 08:09
@mateuszdebinski mateuszdebinski changed the title IBX-715: The buttons on the right sidebar are disabled if the user has no permissions IBX-715: The buttons on the right sidebar should be disabled if the user has no permissions Jul 8, 2021
@alongosz alongosz changed the title IBX-715: The buttons on the right sidebar should be disabled if the user has no permissions IBX-715: Disabled buttons on the right sidebar if user has no permissions Jul 13, 2021
adamwojs
adamwojs requested changes Jul 13, 2021
View reviewed changes
Copy link
Member

@adamwojs adamwojs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about others limitations which could be assigned to content/create ?

src/lib/Menu/ContentRightSidebarBuilder.php Outdated Show resolved Hide resolved
@adamwojs adamwojs requested a review from a team July 13, 2021 09:45
mikadamczyk
mikadamczyk requested changes Jul 23, 2021
View reviewed changes
Copy link
Contributor

@mikadamczyk mikadamczyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Policy "content/manage_locations" is required to copy content. Also, shouldn't "Field Group" limitation be taken into account? Read access to the whole source subtree is checked to perform the "move Subtree" or "copy Subtree" actions.

@mateuszdebinski
Copy link
Contributor Author

after an internal talk with @mikadamczyk I changed the conception for this PR.

Now, we only verify if it has the appropriate permissions, not looking at the limitations because the number of verification would slow down the loading of the content only to turn the button off or on. As verification is when trying to perform an action, we do not have to worry about the fact that the person will be able to perform the selected action. After the conversation, Sylvain also agreed to such a change

mikadamczyk
mikadamczyk approved these changes Aug 23, 2021
View reviewed changes
src/lib/Menu/ContentRightSidebarBuilder.php Outdated Show resolved Hide resolved
src/lib/Menu/ContentRightSidebarBuilder.php Outdated Show resolved Hide resolved
src/lib/Menu/ContentRightSidebarBuilder.php Outdated Show resolved Hide resolved
@sonarcloud
Copy link

sonarcloud bot commented Aug 23, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@bogusez bogusez self-assigned this Aug 23, 2021
@adamwojs adamwojs merged commit 8149993 into 1.5 Aug 26, 2021
@adamwojs adamwojs deleted the IBX-715-disable-button-right-sidebar-permission branch August 26, 2021 05:55
@adamwojs
Copy link
Member

Could you please merge up changes?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bogusez bogusez bogusez approved these changes

@ciastektk ciastektk ciastektk approved these changes

@alongosz alongosz alongosz approved these changes

@mikadamczyk mikadamczyk mikadamczyk approved these changes

@Steveb-p Steveb-p Awaiting requested review from Steveb-p

@adamwojs adamwojs Awaiting requested review from adamwojs

@ViniTou ViniTou Awaiting requested review from ViniTou

Assignees

@mateuszdebinski mateuszdebinski

@bogusez bogusez

Labels
Improvement QA approved
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@mateuszdebinski @adamwojs @mikadamczyk @Steveb-p @alongosz @ciastektk @ViniTou @bogusez