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.

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

Add more permission checks #1825

Merged
merged 5 commits into from
May 20, 2020
Merged

Add more permission checks #1825

merged 5 commits into from
May 20, 2020

Conversation

spacedmonkey
Copy link
Contributor

@spacedmonkey spacedmonkey commented May 18, 2020

Summary

When testing the editor a subscriber, I that neither the publish story button wasn't disabled. Also none of the current status were showing.

In this PR,

  • Add a check on the publish button.
  • Remove REST API call to status API as this data is not longer used.
  • Allow user that can not publish to select draft.

Relevant Technical Choices

  • The removal of the call to the status api, is needed but seemed like a good idea.

To-do

User-facing changes

Current
Screenshot 2020-05-18 at 18 09 48

After this PR
Screenshot 2020-05-18 at 18 08 04

Testing Instructions

  • Login as contributor
  • try to publish story.

See #1390

@github-actions
Copy link
Contributor

github-actions bot commented May 18, 2020

Size Change: -61 B (0%)

Total Size: 872 kB

Filename Size Change
assets/js/edit-story.js 406 kB -96 B (0%)
assets/js/stories-dashboard.js 460 kB +35 B (0%)
ℹ️ View Unchanged
Filename Size Change
assets/css/edit-story.css 3.04 kB 0 B
assets/css/stories-dashboard.css 3.04 kB 0 B

compressed-size-action

@swissspidy swissspidy added Group: Document Panel Type: Enhancement New feature or improvement of an existing feature labels May 18, 2020
@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #1825 into master will increase coverage by 0.17%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1825      +/-   ##
============================================
+ Coverage     63.79%   63.96%   +0.17%     
  Complexity      235      235              
============================================
  Files           587      588       +1     
  Lines          9572     9602      +30     
============================================
+ Hits           6106     6142      +36     
+ Misses         3466     3460       -6     
Impacted Files Coverage Δ Complexity Δ
assets/src/edit-story/app/api/apiProvider.js 2.27% <ø> (+0.14%) 0.00 <0.00> (ø)
...it-story/components/inspector/inspectorProvider.js 11.76% <ø> (+2.87%) 0.00 <0.00> (ø)
assets/src/edit-story/components/header/buttons.js 87.87% <100.00%> (+0.18%) 0.00 <0.00> (ø)
...edit-story/components/inspector/document/status.js 78.12% <100.00%> (+1.45%) 0.00 <0.00> (ø)
includes/Story_Post_Type.php 59.05% <100.00%> (-0.15%) 72.00 <0.00> (ø)
...ts/src/dashboard/app/views/shared/storyListView.js 20.00% <0.00%> (-1.22%) 0.00% <0.00%> (ø%)
assets/src/dashboard/constants/index.js 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
assets/src/dashboard/components/table/index.js 40.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
assets/src/dashboard/components/viewStyleBar.js 50.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
...c/dashboard/components/cardGridItem/cardPreview.js 100.00% <0.00%> (ø) 0.00% <0.00%> (ø%)
... and 1 more

return (
<Primary onClick={handlePublish} isDisabled={isSaving || isUploading}>
<Primary
Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose it's a UX question but should this be displayed at all without the correct permissions?

If we show anything then maybe we would need to bring in the pending status and use the "Submit for Review" option as in WordPress. This would need to be confirmed by UX though.

I'm just wondering how to make sure the "Publish" button doesn't feel broken if it's disabled due to a lack of permissions.

Maybe keep as is but follow up with UX? WDYT?

@@ -69,17 +67,20 @@ function StatusPanel() {
name: __('Draft', 'web-stories'),
helper: __('Visible to just me', 'web-stories'),
},
{
];
Copy link
Contributor

Choose a reason for hiding this comment

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

If Draft is the only option available in radios then better to not show the radio at all. Might be a bit odd seeing a radio selection with just one option. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't really have UX for this now. The UX might not be the best, but good enough for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good to me to keep as-is for now but please follow up with @samitron7 so that she would be aware of this.

Copy link
Contributor

@miina miina left a comment

Choose a reason for hiding this comment

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

LGTM but please follow up with Sam so that she would be aware of permissions influencing what's displayed.

Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

A bit odd that statuses hasn't really been used in the past.

Overall looks OK, but please get final sign-off from workspace pod & make sure to track this with UX.

@spacedmonkey spacedmonkey merged commit 2209ed4 into master May 20, 2020
@spacedmonkey spacedmonkey deleted the fix/more-permission-checks branch May 20, 2020 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: Document Panel Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants