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

Add media upload capability check #9301

Closed
wants to merge 9 commits into from
Closed

Add media upload capability check #9301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f1d9073
Add MediaUploadCheck component and unit tests
talldan Aug 22, 2018
592137d
Use MediaUploadCheck in MediaPlaceholder
talldan Aug 22, 2018
36d7d58
Simplify rendering logic of PostFeaturedImage
talldan Aug 22, 2018
4d234ed
Hard code hasUploadPermissions in redux state instead of MediaUploadC…
talldan Aug 22, 2018
27fa4fc
Implement MediaUploadCheck wherever a user might interact with file u…
talldan Aug 22, 2018
de838cc
Remove tests that can no longer be run due to use of withSelect in ne…
talldan Aug 22, 2018
790a643
Add state handling for upload permissions
talldan Aug 23, 2018
1131d6a
Preload OPTIONS request to media endpoint used to determine if user h…
talldan Aug 23, 2018
4b4b4d6
Add test for OPTIONS requests in preloading middleware
talldan Aug 24, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion docs/data/data-core.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,18 @@ get back from the oEmbed preview API.

Is the preview for the URL an oEmbed link fallback.

### hasUploadPermissions

Return whether the user has media upload permissions.

*Parameters*

* state: Data state.

*Returns*

Does the user have media upload permissions?

## Actions

### receiveUserQuery
Expand Down Expand Up @@ -182,4 +194,12 @@ a given URl has been received.
*Parameters*

* url: URL to preview the embed for.
* preview: Preview data.
* preview: Preview data.

### receiveUploadPermissions

Returns an action object used in signalling that Upload permissions have been received.

*Parameters*

* hasUploadPermissions: Does the user have permission to upload files?
54 changes: 40 additions & 14 deletions lib/client-assets.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -762,28 +762,28 @@ function gutenberg_register_scripts_and_styles() {
* data to be attached to the page. Expected to be called in the context of
* `array_reduce`.
*
* @param array $memo Reduce accumulator.
* @param string $path REST API path to preload.
* @return array Modified reduce accumulator.
* @param array $memo Reduce accumulator.
* @param array $args REST API path and method to preload.
* @return array Modified reduce accumulator.
*/
function gutenberg_preload_api_request( $memo, $path ) {
function gutenberg_preload_api_request( $memo, $args ) {

// array_reduce() doesn't support passing an array in PHP 5.2
// so we need to make sure we start with one.
if ( ! is_array( $memo ) ) {
$memo = array();
}

if ( empty( $path ) ) {
if ( empty( $args ) || empty( $args['path'] ) || empty( $args['method'] ) ) {
Copy link
Contributor Author

@talldan talldan Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Comment from old PR
https://github.com/WordPress/gutenberg/pull/4155/files#r201324762

Aside: GET will certainly be the majority usage here. I'd expect it to be reasonable as a default value. Maybe even to the extreme (i.e. debatable) of supporting mixed value arrays like...

[
	'/get/to/path/1',
	'/get/to/path/2',
	[
		'path'   => '/options/to/path',
		'method' => 'OPTIONS',
	],
]

But this is less part of a public API than I originally expected, so I don't feel strongly about it at this point.

Copy link
Contributor Author

@talldan talldan Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not a fan of the idea of mixing types in the array. I feel like at least this way it's obvious what's happening.

I would maybe say that this could throw an error if the args are not provided, at the moment it just skips preloading the request when really it is an obvious user error if they're not provided.

Also not sure if we'd consider splitting some code out of client-assets, we might be able to make this a bit more expressive.

return $memo;
}

$path_parts = parse_url( $path );
$path_parts = parse_url( $args['path'] );
if ( false === $path_parts ) {
return $memo;
}

$request = new WP_REST_Request( 'GET', $path_parts['path'] );
$request = new WP_REST_Request( $args['method'], $path_parts['path'] );
if ( ! empty( $path_parts['query'] ) ) {
parse_str( $path_parts['query'], $query_params );
$request->set_query_params( $query_params );
Expand All @@ -802,7 +802,11 @@ function gutenberg_preload_api_request( $memo, $path ) {
$data['_links'] = $links;
}

$memo[ $path ] = array(
if ( 'OPTIONS' === $args['method'] ) {
$response = rest_send_allow_header( $response, $server, $request );
}

$memo[ $args['method'] ][ $args['path'] ] = array(
'body' => $data,
'headers' => $response->headers,
);
Expand Down Expand Up @@ -1278,12 +1282,34 @@ function gutenberg_editor_scripts_and_styles( $hook ) {

// Preload common data.
$preload_paths = array(
'/',
'/wp/v2/types?context=edit',
'/wp/v2/taxonomies?per_page=-1&context=edit',
sprintf( '/wp/v2/%s/%s?context=edit', $rest_base, $post->ID ),
sprintf( '/wp/v2/types/%s?context=edit', $post_type ),
sprintf( '/wp/v2/users/me?post_type=%s&context=edit', $post_type ),
array(
'path' => '/',
'method' => 'GET',
),
array(
'path' => '/wp/v2/types?context=edit',
'method' => 'GET',
),
array(
'path' => '/wp/v2/taxonomies?per_page=-1&context=edit',
'method' => 'GET',
),
array(
'path' => sprintf( '/wp/v2/%s/%s?context=edit', $rest_base, $post->ID ),
'method' => 'GET',
),
array(
'path' => sprintf( '/wp/v2/types/%s?context=edit', $post_type ),
'method' => 'GET',
),
array(
'path' => sprintf( '/wp/v2/users/me?post_type=%s&context=edit', $post_type ),
'method' => 'GET',
),
array(
'path' => '/wp/v2/media',
'method' => 'OPTIONS',
),
Copy link
Member

Choose a reason for hiding this comment

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

Related #10268

If we change the function signature, there may be some compatibility concerns.

);

// Ensure the global $post remains the same after
Expand Down
5 changes: 3 additions & 2 deletions packages/api-fetch/src/middlewares/preloading.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ const createPreloadingMiddleware = ( preloadedData ) => ( options, next ) => {
const method = options.method || 'GET';
const path = getStablePath( options.path );

if ( 'GET' === method && preloadedData[ path ] ) {
return Promise.resolve( preloadedData[ path ].body );
if ( preloadedData[ method ] && preloadedData[ method ][ path ] ) {
const response = 'OPTIONS' === method ? preloadedData[ method ][ path ] : preloadedData[ method ][ path ].body;
Copy link
Contributor Author

@talldan talldan Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Coment from old PR
#4155 (comment)

We should have a unit test for this. I'm also not sure why we ought to need to special-case OPTIONS requests. Who's to say we don't want other headers from the request? Just seems like we're making inconsistency for not an entirely valid reason.

Copy link
Contributor Author

@talldan talldan Aug 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I can see that this would be quite a big refactor, so maybe worth splitting into a separate PR?

I will add the tests though.

return Promise.resolve( response );
}
}

Expand Down
45 changes: 37 additions & 8 deletions packages/api-fetch/src/middlewares/test/preloading.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,53 @@
import createPreloadingMiddleware from '../preloading';

describe( 'Preloading Middleware', () => {
it( 'should return the preloaded data if provided', () => {
const body = {
status: 'this is the preloaded response',
it( 'should return the preloaded response body if provided when using a GET request', () => {
const postsResponse = {
body: {
status: 'this is the preloaded response',
},
};
const preloadedData = {
'wp/v2/posts': {
body,
GET: {
'wp/v2/posts': postsResponse,
},
};
const prelooadingMiddleware = createPreloadingMiddleware( preloadedData );
const preloadingMiddleware = createPreloadingMiddleware( preloadedData );
const requestOptions = {
method: 'GET',
path: 'wp/v2/posts',
};

const response = prelooadingMiddleware( requestOptions );
const response = preloadingMiddleware( requestOptions );
return response.then( ( value ) => {
expect( value ).toEqual( postsResponse.body );
} );
} );

it( 'should return the preloaded full response data if provided when using an OPTIONS request', () => {
const postsResponse = {
headers: {
test: 'this is a preloaded header',
},
body: {
status: 'this is the preloaded response',
},
};

const preloadedData = {
OPTIONS: {
'wp/v2/posts': postsResponse,
},
};
const preloadingMiddleware = createPreloadingMiddleware( preloadedData );
const requestOptions = {
method: 'OPTIONS',
path: 'wp/v2/posts',
};

const response = preloadingMiddleware( requestOptions );
return response.then( ( value ) => {
expect( value ).toEqual( body );
expect( value ).toEqual( postsResponse );
} );
} );

Expand Down
107 changes: 0 additions & 107 deletions packages/block-library/src/audio/test/__snapshots__/index.js.snap
View file
Open in desktop

This file was deleted.

13 changes: 0 additions & 13 deletions packages/block-library/src/audio/test/index.js
View file
Open in desktop

This file was deleted.

33 changes: 18 additions & 15 deletions packages/block-library/src/cover-image/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
BlockAlignmentToolbar,
MediaPlaceholder,
MediaUpload,
MediaUploadCheck,
AlignmentToolbar,
RichText,
} from '@wordpress/editor';
Expand Down Expand Up @@ -154,21 +155,23 @@ export const settings = {
setAttributes( { contentAlign: nextAlign } );
} }
/>
<Toolbar>
<MediaUpload
onSelect={ onSelectImage }
type="image"
value={ id }
render={ ( { open } ) => (
<IconButton
className="components-toolbar__control"
label={ __( 'Edit image' ) }
icon="edit"
onClick={ open }
/>
) }
/>
</Toolbar>
<MediaUploadCheck>
<Toolbar>
<MediaUpload
onSelect={ onSelectImage }
type="image"
value={ id }
render={ ( { open } ) => (
<IconButton
className="components-toolbar__control"
label={ __( 'Edit image' ) }
icon="edit"
onClick={ open }
/>
) }
/>
</Toolbar>
</MediaUploadCheck>
</BlockControls>
{ !! url && (
<InspectorControls>
Expand Down
Loading