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

Chrome: Hide Sidebar Panels if the user don't have the right capabilities #2585

Merged
merged 2 commits into from Sep 6, 2017

Conversation

Projects
None yet
2 participants
@youknowriad
Contributor

youknowriad commented Aug 29, 2017

If the user doesn't have the "publish_posts" capability (a contributor), several panel sidebars should be hidden.

In this PR I'm hiding the "pending" toggle, the "schedule" panel and the post visibility is not editable. (similar to the current editor)

@youknowriad youknowriad added the Chrome label Aug 29, 2017

@youknowriad youknowriad self-assigned this Aug 29, 2017

@youknowriad youknowriad requested a review from aduth Aug 29, 2017

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Aug 29, 2017

Codecov Report

Merging #2585 into master will increase coverage by 1.21%.
The diff coverage is 63.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2585      +/-   ##
==========================================
+ Coverage   31.42%   32.63%   +1.21%     
==========================================
  Files         177      178       +1     
  Lines        5407     5531     +124     
  Branches      946      959      +13     
==========================================
+ Hits         1699     1805     +106     
- Misses       3135     3150      +15     
- Partials      573      576       +3
Impacted Files Coverage Δ
editor/sidebar/post-status/index.js 0% <0%> (ø) ⬆️
editor/sidebar/post-author/index.js 85% <100%> (ø) ⬆️
editor/sidebar/post-pending-status/index.js 58.33% <58.33%> (ø)
editor/sidebar/post-visibility/index.js 46.15% <83.33%> (+46.15%) ⬆️
editor/sidebar/post-schedule/index.js 65% <83.33%> (+65%) ⬆️
blocks/library/latest-posts/index.js 7.4% <0%> (-2.6%) ⬇️
editor/sidebar/post-schedule/clock.js 0% <0%> (ø) ⬆️
blocks/api/registration.js 100% <0%> (ø) ⬆️
...ebar/post-taxonomies/hierarchical-term-selector.js 0% <0%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5885233...40accca. Read the comment docs.

codecov bot commented Aug 29, 2017

Codecov Report

Merging #2585 into master will increase coverage by 1.21%.
The diff coverage is 63.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2585      +/-   ##
==========================================
+ Coverage   31.42%   32.63%   +1.21%     
==========================================
  Files         177      178       +1     
  Lines        5407     5531     +124     
  Branches      946      959      +13     
==========================================
+ Hits         1699     1805     +106     
- Misses       3135     3150      +15     
- Partials      573      576       +3
Impacted Files Coverage Δ
editor/sidebar/post-status/index.js 0% <0%> (ø) ⬆️
editor/sidebar/post-author/index.js 85% <100%> (ø) ⬆️
editor/sidebar/post-pending-status/index.js 58.33% <58.33%> (ø)
editor/sidebar/post-visibility/index.js 46.15% <83.33%> (+46.15%) ⬆️
editor/sidebar/post-schedule/index.js 65% <83.33%> (+65%) ⬆️
blocks/library/latest-posts/index.js 7.4% <0%> (-2.6%) ⬇️
editor/sidebar/post-schedule/clock.js 0% <0%> (ø) ⬆️
blocks/api/registration.js 100% <0%> (ø) ⬆️
...ebar/post-taxonomies/hierarchical-term-selector.js 0% <0%> (ø) ⬆️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5885233...40accca. Read the comment docs.

@youknowriad youknowriad requested review from mtias and iseulde Aug 31, 2017

@aduth

aduth approved these changes Sep 1, 2017

Nice 👍

One issue, which I expect is present on master, is that the author dropdown does not show (correctly) for contributor role, but only because it attempts to make the API request for users and fails. Up to you if you'd like to address that here or separately.

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2017

Contributor

One issue, which I expect is present on master, is that the author dropdown does not show (correctly) for contributor role

Do you mean, instead of relying on the available users, we should rely on the capabilities?

Contributor

youknowriad commented Sep 4, 2017

One issue, which I expect is present on master, is that the author dropdown does not show (correctly) for contributor role

Do you mean, instead of relying on the available users, we should rely on the capabilities?

@youknowriad

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Sep 4, 2017

Contributor

@aduth I went ahead and updated to add the capability check. but the API request is still being fetched, because it's declared on the same withAPIData HoC. I don't know if we want to create another inner component just to avoid this failing request. An alternative could be to pass the dataProps to the mapPropsToData callback, this way we could decide to include an API call depending on the previous result of another API call. This could be something to try separately;

Contributor

youknowriad commented Sep 4, 2017

@aduth I went ahead and updated to add the capability check. but the API request is still being fetched, because it's declared on the same withAPIData HoC. I don't know if we want to create another inner component just to avoid this failing request. An alternative could be to pass the dataProps to the mapPropsToData callback, this way we could decide to include an API call depending on the previous result of another API call. This could be something to try separately;

@youknowriad youknowriad merged commit 1251c92 into master Sep 6, 2017

3 checks passed

codecov/project 32.63% (+1.21%) compared to 5885233
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the fix/panels-visibility branch Sep 6, 2017

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Sep 6, 2017

Member

Yeah, either of those seem worth exploring. There should ideally be no 403 requests issued if we know they can be avoided.

Member

aduth commented Sep 6, 2017

Yeah, either of those seem worth exploring. There should ideally be no 403 requests issued if we know they can be avoided.

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