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

Reusable blocks: Improve UX for non-privileged users #12378

Merged
merged 16 commits into from Jan 23, 2019

Conversation

@noisysocks
Copy link
Member

noisysocks commented Nov 28, 2018

What this is

Fixes #12338.

Improves the UX of creating, editing, and deleting a reusable block when logged in as an author or contributor by disabling the Add to Reusable Blocks, Edit, and Remove from Reusable Blocks buttons when necessary.

This is accomplished under the hood by introducing the canUser() selector to core-data which allows callers to query whether the REST API supports performing a given action on a given resource, e.g. one can query whether the logged in user can create posts by running wp.data.select( 'core' ).canUser( 'create', 'posts' ).

The existing hasUploadPermissions() selector is changed to use canUser( 'create', 'media' ) under the hood.

How it looks

Add to Reusable Blocks button is visible for admins and editors:

admin - add

Add to Reusable Blocks button is hidden for contributors:

contributor - add

Edit button is enabled and Remove from Reusable Blocks is visible for admins and editors:

admin - edit remove

Edit button is disabled and Remove from Reusable Blocks is hidden for authors:

author - edit remove

How to test

  1. Try to create a reusable block as a contributor. The Add to Reusable Blocks button should be disabled.
  2. Try to updating another user's reusable block as an author. The Edit button should be disabled.
  3. Try to deleting another user's reusable block as an author or contributor. The Remove from Reusable Blocks button should be disabled.

@noisysocks noisysocks added this to the 4.7 milestone Nov 28, 2018

@mtias mtias modified the milestones: 4.7, 4.8 Nov 29, 2018

@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch from a1856af to 9a4cb69 Dec 5, 2018

@noisysocks noisysocks requested review from youknowriad , gziolo and talldan Dec 5, 2018

@noisysocks

This comment has been minimized.

Copy link
Member Author

noisysocks commented Dec 5, 2018

This should be ready to review!

@mtias: Reckon we could plop this back into 4.7? I'd like to have this fixed for 5.0.1 since it's a pretty awful UX otherwise. Not sure where we are timing wise. edit: Eh, 5.0.2 is OK 🙂

@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch from 9a4cb69 to 3c1db96 Dec 6, 2018

@talldan
Copy link
Contributor

talldan left a comment

I like the canUser function, it's a nice way to express that functionality.

I think that flash of the disabled state of the block settings menu item is something that needs to be fixed.

Preloading could work, but I think checking the resolution state is also possible and worth doing as well, since it shouldn't just depend on preloading. I think this is what embeds do:
https://github.com/WordPress/gutenberg/blob/master/packages/core-data/src/selectors.js#L41

Show resolved Hide resolved packages/core-data/src/selectors.js
@@ -80,6 +83,7 @@ export default compose( [
! isReusableBlock( blocks[ 0 ] ) ||
! getReusableBlock( blocks[ 0 ].attributes.ref )
),
canCreateBlocks: canUser( 'create', 'blocks' ),

This comment has been minimized.

@talldan

talldan Dec 6, 2018

Contributor

For media upload permissions the media options request is preloaded. I think it might be worth doing the same here.

I noticed a 'flash' where the menu item went from being enabled to disabled when opening the block settings menu while the blocks options request was in progress.

This comment has been minimized.

@noisysocks

noisysocks Dec 7, 2018

Author Member

👌 I've added this OPTIONS request to the list of requests we preload in bc0709f.

Noting that this is a change we'll need to make to core after this PR is merged.

Show resolved Hide resolved ...itor/src/components/block-settings-menu/reusable-block-convert-button.js Outdated
*
* @return {boolean} Whether or not the user can perform the action.
*/
export function canUser( state, action, resource, id ) {

This comment has been minimized.

@talldan

talldan Dec 6, 2018

Contributor

I like the expressiveness of the canUser selector 😄

This comment has been minimized.

@noisysocks

noisysocks Dec 6, 2018

Author Member

Props to @gziolo for suggesting the API 🙂

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

I'm glad you like it :)

return {
reusableBlock: block && isReusableBlock( block ) ? getReusableBlock( block.attributes.ref ) : null,
id,
isDisabled: !! id && (

This comment has been minimized.

@talldan

talldan Dec 6, 2018

Contributor

I think same comment here regarding disabling vs. hiding.

This comment has been minimized.

@talldan

talldan Dec 6, 2018

Contributor

Also noting that in testing I didn't see the flash of this switching from enabled to disabled. That seems to be because the GET request for the block has already been made and cached. If an OPTIONS request was made instead of a GET request the issue would resurface.

This comment has been minimized.

@noisysocks

noisysocks Dec 7, 2018

Author Member

Have changed this to remove the button instead of disabling in 708a504.

We can't preload these requests because we don't know the reusable block ID ahead of time.

This comment has been minimized.

@talldan

talldan Dec 10, 2018

Contributor

Yep, I do understand that we can't preload. My worry is that it's only by coincidence that the user's capability is known ahead of time. If the underlying code switches to use OPTIONS instead of GET, users would start seeing this button initially be clickable but then suddenly disappear.

This is probably a nitpick though and not a blocker. I'm not even sure what the best UX would be.

This comment has been minimized.

@noisysocks

noisysocks Dec 14, 2018

Author Member

Ah, got it! Not sure if defaulting to disabled versus defaulting to enabled would be a better experience. Both aren't ideal. I matched the existing hasUploadPermissions behaviour which defaults to enabled.

I think that, further down the track, core-data should modify the canUser state whenever it receives an Allow header from any response, and not just when the canUser() resolver is called.

This comment has been minimized.

@talldan

talldan Dec 14, 2018

Contributor

Yeah, that's a nice idea. Not sure how it'd work, but a nice idea 😄

path,
// Ideally this would always be an OPTIONS request, but unfortunately there's
// a bug in the REST API which causes the Allow header to not be sent on
// OPTIONS requests to /posts/:id routes.

This comment has been minimized.

@talldan

talldan Dec 6, 2018

Contributor

Is that just for /post/:id of for the all resources? I mentioned that because the logic below happens for all resources (so doesn't match the comment).

Is there a ticket/issue covering the bug?

This comment has been minimized.

@noisysocks

noisysocks Dec 6, 2018

Author Member

I've only checked posts but I suspect the bug affects all REST endpoints. Will do some investigating and open up a Trac ticket 👍

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

If this is the case, it should be fixed /cc @danielbachhuber @kadamwhite

This comment has been minimized.

@TimothyBJacobs

TimothyBJacobs Dec 14, 2018

Contributor

If you just want the Allow header, what about doing a HEAD request instead of GET.

This comment has been minimized.

@danielbachhuber

danielbachhuber Dec 15, 2018

Member

Looking into this further:

  1. The Allow header is generated by rest_send_allow_header(), which is called on rest_post_dispatch.
  2. rest_post_dispatch is called for every response (I think):

It seems like Allow should be present for OPTIONS. It's an officially supported header: https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/OPTIONS

Can you provide steps to reproduce?

This comment has been minimized.

@noisysocks

noisysocks Dec 24, 2018

Author Member

I briefly debugged this when I noticed the issue and from memory it was something to do with rest_handle_options_request not calling $request->set_url_params() unlike dispatch which correctly parses and sets URL parameters.

Can you provide steps to reproduce?

  1. Create a reusable block
  2. Perform a GET http://local.wordpress.test/wp-json/wp/v2/blocks request and note the ID of the reusable block
  3. Perform a OPTIONS http://local.wordpress.test/wp-json/wp/v2/blocks/27 request, replacing 27 with the ID from step 2. Note that an Allow header is not sent back
  4. Perform a GET http://local.wordpress.test/wp-json/wp/v2/blocks/27 request, replacing 27 with the ID from step 2. Note that an Allow header is sent back

Above steps also work with posts: just change blocks to posts.

This comment has been minimized.

@noisysocks

This comment has been minimized.

@noisysocks

noisysocks Dec 24, 2018

Author Member

Added a link to the Trac ticket in cf2d701.

@@ -103,7 +103,46 @@ export function* getEmbedPreview( url ) {
* Requests Upload Permissions from the REST API.
*/
export function* hasUploadPermissions() {

This comment has been minimized.

@noisysocks

noisysocks Dec 6, 2018

Author Member

Should I mark this as deprecated? I'm unsure on what the policy is post 5.0 🙂

This comment has been minimized.

@talldan

talldan Dec 10, 2018

Contributor

I don't know either. Might be worth pinging someone on slack. Will hold off approving until the answer is known.

This comment has been minimized.

@noisysocks

This comment has been minimized.

@noisysocks

noisysocks Dec 14, 2018

Author Member

It's probably fine leaving this as is where we support both canUser and hasUploadPermissions. We can always deprecate later.

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

We might want to use a soft deprecation by using @deprecated in JSDoc and maybe filter out all actions and selectors from the generated docs.

This comment has been minimized.

@youknowriad

youknowriad Dec 14, 2018

Contributor

Yes, for now, we should leave those, we can add a deprecation message if needed but without specifying a version.

This is something we should discuss more in the next #core-js chats.

This comment has been minimized.

@noisysocks

noisysocks Dec 24, 2018

Author Member

This comment has been minimized.

@talldan

talldan Jan 7, 2019

Contributor

@noisysocks I noticed you @deprecated the selector, but the resolver hasn't been deprecated. I guess technically they're the same interface and the resolver wouldn't be called directly, but I wanted to double-check if it was intentional.

This comment has been minimized.

@noisysocks

noisysocks Jan 7, 2019

Author Member

That's a good point. I think selectors are the public interface here but it wouldn't hurt to have the @deprecated attribute in the resolver as well.

This comment has been minimized.

@noisysocks

noisysocks Jan 7, 2019

Author Member

Done in 4ea46a5!

@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch 2 times, most recently from 708a504 to 9ec72e8 Dec 7, 2018

@noisysocks

This comment has been minimized.

Copy link
Member Author

noisysocks commented Dec 14, 2018

@gziolo any thoughts on this one?

@@ -103,7 +103,46 @@ export function* getEmbedPreview( url ) {
* Requests Upload Permissions from the REST API.
*/
export function* hasUploadPermissions() {

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

We might want to use a soft deprecation by using @deprecated in JSDoc and maybe filter out all actions and selectors from the generated docs.


const method = methods[ action ];
if ( ! method ) {
throw new Error( `'${ action }' is not a valid action` );

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

We aren't consistent, but probably we should end all sentences with a period.

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

👍 ccf4936

throw new Error( `'${ action }' is not a valid action` );
}

const path = id ? `/wp/v2/${ resource }/${ id }` : `/wp/v2/${ resource }`;

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member
Suggested change Beta
const path = id ? `/wp/v2/${ resource }/${ id }` : `/wp/v2/${ resource }`;
const path = '/wp/v2/${ resource }' + ( id ? `/${ id }` : '' );

would be shorter, not sure what reads better though :)

This comment has been minimized.

@noisysocks

noisysocks Jan 2, 2019

Author Member

I like that with the more verbose version that you can glance at the code and see the "shape" of the URL.

@@ -116,5 +155,7 @@ export function* hasUploadPermissions() {
allowHeader = get( response, [ 'headers', 'Allow' ], '' );
}

yield receiveUploadPermissions( includes( allowHeader, 'POST' ) );
const key = compact( [ action, resource, id ] ).join( '/' );

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

Nice trick :)

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

You'd need template strings for the first part:

Suggested change Beta
const key = compact( [ action, resource, id ] ).join( '/' );
const path = `/wp/v2/${ resource }` + ( id ? `/${ id }` : '' );

I think it's a bit easier to sort out though, yeah.

This comment has been minimized.

@noisysocks

noisysocks Jan 2, 2019

Author Member

Was this suggestion meant to go in this thread? #12378 (comment)

yield receiveUploadPermissions( includes( allowHeader, 'POST' ) );
const key = compact( [ action, resource, id ] ).join( '/' );
const isAllowed = includes( allowHeader, method );
yield receiveUserPermissions( key, isAllowed );

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

Should it be named receiveUserPermission using singular version to reflect that you can provide only one permission at the time?

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

At the very least could we add the ticket URL in the comment so it can be tracked/checked on?

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

Should it be named receiveUserPermission using singular version to reflect that you can provide only one permission at the time?

👍 02d9502

At the very least could we add the ticket URL in the comment so it can be tracked/checked on?

I think this is referring to this thread? #12378 (comment) If so I added a link to the Trac ticket in
cf2d701.

const state = deepFreeze( {
userPermissions: {},
} );
expect( canUser( state, 'create', 'media' ) ).toBe( true );

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

That's surprising that we set true as a default value. What if REST API call takes 5 seconds and UI would be rendered before that happens?

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

Yeah, true as the default seems wrong to me. I'd rather UI update after API requests and have extra settings added than them removed.

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

I've changed canUser() to return false by default in 62bcb19.

};
} ),
withDispatch( ( dispatch, { onToggle = noop } ) => {
withDispatch( ( dispatch, { clientId, onToggle = noop }, { select } ) => {

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

select - that's interesting to see it have much broader application than I anticipated. I hope there are no drawbacks of that approach :)

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

I like that in this case it lets us avoid passing unnecessary props to the component.

! isReusableBlock( blocks[ 0 ] ) ||
! getReusableBlock( blocks[ 0 ].attributes.ref )
// Show 'Convert to Regular Block' when selected block is a reusable block
isVisible: isReusable || (

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

It's probably better to move isVisible outside of the object - the way it was before. It makes return value harder follow at the moment.

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

👍 1c3f722

path,
// Ideally this would always be an OPTIONS request, but unfortunately there's
// a bug in the REST API which causes the Allow header to not be sent on
// OPTIONS requests to /posts/:id routes.

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

If this is the case, it should be fixed /cc @danielbachhuber @kadamwhite

* @param {string} resource REST resource to check, e.g. 'media' or 'posts'.
* @param {?string} id ID of the rest resource to check.
*
* @return {boolean} Whether or not the user can perform the action.

This comment has been minimized.

@gziolo

gziolo Dec 14, 2018

Member

Can we add the information about the default behavior when it is not found in the store?

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

👍 Done as part of 62bcb19.

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Dec 14, 2018

The existing hasUploadPermissions() selector is changed to use canUser( 'create', 'media' ) under the hood.

@imath - I think you implemented the original version of this behavior when working on media upload permissions. Can you double check if the generalized version is still correct?

@danielbachhuber or @kadamwhite - it would be nice to get your review on the bits that interact with REST API.

@noisysocks - nice work on the PR. I personally think it is quite close to be considered ready. My comments are mostly nitpicks. The only important thing from my perspective is to ensure that canUser has the best strategy for returning the default value. It feels like assuming there is no permission is safer.

@danielbachhuber

This comment has been minimized.

Copy link
Member

danielbachhuber commented Dec 15, 2018

Improves the UX of creating, editing, and deleting a reusable block when logged in as an author or contributor by disabling the Add to Reusable Blocks, Edit, and Remove from Reusable Blocks buttons when necessary.

Does this change the UI at all? If so, can you include screenshots and/or GIFs in the description?

@danielbachhuber

This comment has been minimized.

Copy link
Member

danielbachhuber commented Dec 15, 2018

Also:

The only important thing from my perspective is to ensure that canUser has the best strategy for returning the default value.

At first glance, I'm not sure how well this canUser abstraction is going to work in practice. WordPress' roles and capabilities system is quite nuanced, and abstractions on top of it can be problematic. Just want to flag that.

@imath

This comment has been minimized.

Copy link
Contributor

imath commented Dec 15, 2018

@gziolo Hi,

I've just tested the PR. I confirm that being logged in as an Administrator I'm still able to upload files and that being logged in as a contributor I can only use external URLs to add media. So it seems to be working as expected for this part 👍

@noisysocks FYI I also tested the Reusable blocks UI changes being logged in as a contributor:

  • I confirm I cannot create new reusable block.
  • I can use another user reusable block but i cannot edit it.
  • I can access to the /wp-admin/edit.php?post_type=wp_block manage screen but can only export the blocks created by others as json.

Feedback addressed

@tofumatt
Copy link
Member

tofumatt left a comment

This is looking good—I like the canUser selector API, but I think there's some code cleanup here to do and I have two concerns:

  1. why is true the default response for canUser()?
  2. Why are we ignoring the API request throwing an error?

* state: Data state.
* action: Action to check. One of: 'create', 'read', 'update',
'delete'.

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

This indentation is kinda odd; I'm not sure why this is indented where it is...

This comment has been minimized.

@aduth

aduth Jan 2, 2019

Member

This indentation is kinda odd; I'm not sure why this is indented where it is...

Likely because it's unprocessed from the space-aligned JSDoc. Could probably be improved with some additional processing (lines.map( trim ).join( ' ' )?). It has no difference in the Markdown preview though, since the whitespace is collapsed.

@@ -159,6 +162,7 @@ export default compose( [
isFetching: isFetchingReusableBlock( ref ),
isSaving: isSavingReusableBlock( ref ),
block: reusableBlock ? getBlock( reusableBlock.clientId ) : null,
canUpdateBlock: !! reusableBlock && ! reusableBlock.isTemporary && canUser( 'update', 'blocks', ref ),

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

Any reason you're checking !! reusableBlock rather than just using reusableBlock? Just curious!

This comment has been minimized.

@aduth

aduth Jan 2, 2019

Member

Any reason you're checking !! reusableBlock rather than just using reusableBlock? Just curious!

If a value is falsey in this scenario, the specific value is returned in the left-hand of the condition.

If reusableBlock was undefined for example, without the !!, canUpdateBlock would be passed as undefined, despite its name indicating that it would be a boolean. As written, it satisfies an implicit contract of providing a boolean value. Practically speaking this would rarely have much an impact, except in a scenario where someone expected it to be the particular boolean form (=== false).

@@ -116,5 +155,7 @@ export function* hasUploadPermissions() {
allowHeader = get( response, [ 'headers', 'Allow' ], '' );
}

yield receiveUploadPermissions( includes( allowHeader, 'POST' ) );
const key = compact( [ action, resource, id ] ).join( '/' );

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

You'd need template strings for the first part:

Suggested change Beta
const key = compact( [ action, resource, id ] ).join( '/' );
const path = `/wp/v2/${ resource }` + ( id ? `/${ id }` : '' );

I think it's a bit easier to sort out though, yeah.

yield receiveUploadPermissions( includes( allowHeader, 'POST' ) );
const key = compact( [ action, resource, id ] ).join( '/' );
const isAllowed = includes( allowHeader, method );
yield receiveUserPermissions( key, isAllowed );

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

At the very least could we add the ticket URL in the comment so it can be tracked/checked on?

parse: false,
} );
} catch ( error ) {
return;

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

😟 This seems like it should throw. Why are we catching an ignoring the error without doing anything with it?

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

error here refers to the API error that resulted from our OPTIONS request. Doing nothing is correct in this case as we want to leave the store as is. I've added a comment to explain this in e0a7269.

const state = deepFreeze( {
userPermissions: {},
} );
expect( canUser( state, 'create', 'media' ) ).toBe( true );

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

Yeah, true as the default seems wrong to me. I'd rather UI update after API requests and have extra settings added than them removed.

blocks.length === 1 &&
blocks[ 0 ] &&
isReusableBlock( blocks[ 0 ] ) &&
!! getReusableBlock( blocks[ 0 ].attributes.ref )

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

Any reason to use !! here? I just always find it weird to see 🤷‍♂️

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

It's to ensure that typeof isReusable === 'boolean'.


every( blocks, ( block ) => (
// Guard against the case where a regular block has *just* been converted
!! block &&

This comment has been minimized.

@tofumatt

tofumatt Dec 20, 2018

Member

Again, any reason for the !!?

This comment has been minimized.

@noisysocks

noisysocks Jan 3, 2019

Author Member

It's to ensure that typeof isVisible === 'boolean'.

@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch from 9ec72e8 to cf2d701 Dec 24, 2018

@danielbachhuber danielbachhuber removed their request for review Dec 26, 2018

@noisysocks noisysocks force-pushed the fix/non-privileged-reusable-blocks-ux branch from 15ff121 to 7b8d040 Jan 23, 2019

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 23, 2019

IIRC this one also needed a change in core to preload that request.

https://github.com/WordPress/gutenberg/pull/12378/files#diff-620130744b6b73b822a3df609671d930R1089

@noisysocks noisysocks merged commit 33d2acb into master Jan 23, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@noisysocks noisysocks deleted the fix/non-privileged-reusable-blocks-ux branch Jan 23, 2019

@noisysocks

This comment has been minimized.

Copy link
Member Author

noisysocks commented Jan 23, 2019

Getting this in early so that there's time for it to bake in 5.0.

IIRC this one also needed a change in core to preload that request.

Thanks for the reminder. I wonder what the best way to do this is... Do we check how the plugin PHP files differ when updating Core's script dependencies? (e.g. git diff v4.9 v5.0 **.php) If so, we'll catch this change. If not, perhaps opening a ticket is the safest bet, though I'm not actually sure when we plan on updating Core to use Gutenberg 5.0 scripts.

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Jan 23, 2019

@noisysocks Not sure. This one was done as a trac ticket, so maybe that's the best option:
https://core.trac.wordpress.org/changeset/43833

@@ -24,6 +24,7 @@
"@babel/runtime": "^7.0.0",
"@wordpress/api-fetch": "file:../api-fetch",
"@wordpress/data": "file:../data",
"@wordpress/deprecated": "file:../deprecated",

This comment has been minimized.

@aduth

aduth Jan 23, 2019

Member

There should have been a corresponding change to the root package-lock.json. I'm not sure how this passed the build task which checks for local uncommitted changes, but it shouldn't have.

This comment has been minimized.

@noisysocks

noisysocks Jan 23, 2019

Author Member

Huh! Looks like check-local-changes doesn't pick this up because precheck-local-changes doesn't run npm install. CI doesn't pick it up because it uses npm ci.

Will fix this up in a seperate PR.

This comment has been minimized.

@noisysocks
@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Feb 6, 2019

A test that I made:
Contributor made a new post. Submitted for Review.
Logged in as admin. Went to the newly created contributor post and took it over. Made a reusable block. Clicked to save as pending.
Logged in as contributor. Went to post. Noticed that contributor could delete the reusable block.

@talldan

This comment has been minimized.

Copy link
Contributor

talldan commented Feb 6, 2019

@paaljoachim What version are you testing against? This has only just been released in the plugin today afaik.

I couldn't reproduce testing against master.

@paaljoachim

This comment has been minimized.

Copy link

paaljoachim commented Feb 6, 2019

Hey Daniel @talldan

Lets just call it an error on my end. I made the following comment:
https://make.wordpress.org/test/2019/02/05/call-for-testing-gutenberg-5-0/#comment-1170

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