From b4003ff27e5260a95d498e20c4d2e16e47ffd85f Mon Sep 17 00:00:00 2001 From: Jonny Harris Date: Thu, 3 Nov 2022 11:06:55 +0000 Subject: [PATCH] Story Locking: Ensure author users and below correctly see information (#12559) --- includes/Admin/Dashboard.php | 2 +- includes/REST_API/Stories_Lock_Controller.php | 65 +++++++++++++++++-- includes/templates/admin/edit-story.php | 2 +- .../wp-dashboard/src/api/constants/index.js | 2 +- .../src/api/utils/reshapeStoryObject.js | 21 +++--- .../src/api/utils/test/reshapeStoryObject.js | 36 +++++----- .../src/api/constants/index.js | 2 +- packages/wp-story-editor/src/api/storyLock.js | 6 +- .../src/api/utils/transformStoryResponse.js | 6 +- .../src/components/postLock/postLock.js | 24 ++++--- .../REST_API/Stories_Lock_Controller.php | 41 ++++++++++++ 11 files changed, 152 insertions(+), 55 deletions(-) diff --git a/includes/Admin/Dashboard.php b/includes/Admin/Dashboard.php index 46eb643cac89..8e7a08386007 100644 --- a/includes/Admin/Dashboard.php +++ b/includes/Admin/Dashboard.php @@ -333,7 +333,7 @@ public function load_stories_dashboard(): void { '_embed' => rawurlencode( implode( ',', - [ 'wp:lock', 'wp:lockuser', 'author' ] + [ 'wp:lock', 'author' ] ) ), 'context' => 'edit', diff --git a/includes/REST_API/Stories_Lock_Controller.php b/includes/REST_API/Stories_Lock_Controller.php index 0e27bc4886fb..03a40b9f3537 100644 --- a/includes/REST_API/Stories_Lock_Controller.php +++ b/includes/REST_API/Stories_Lock_Controller.php @@ -303,10 +303,17 @@ public function prepare_item_for_response( $item, $request ) { $lock_data = [ 'locked' => false, 'time' => '', - 'user' => 0, + 'user' => [ + 'name' => '', + 'id' => 0, + ], 'nonce' => $nonce, ]; + if ( get_option( 'show_avatars' ) ) { + $lock_data['user']['avatar'] = []; + } + if ( ! empty( $item ) ) { /** This filter is documented in wp-admin/includes/ajax-actions.php */ $time_window = apply_filters( 'wp_check_post_lock_window', 150 ); @@ -318,6 +325,19 @@ public function prepare_item_for_response( $item, $request ) { 'user' => isset( $item['user'] ) ? (int) $item['user'] : 0, 'nonce' => $nonce, ]; + if ( isset( $item['user'] ) ) { + $user = get_user_by( 'id', $item['user'] ); + if ( $user ) { + $lock_data['user'] = [ + 'name' => $user->display_name, + 'id' => $item['user'], + ]; + + if ( get_option( 'show_avatars' ) ) { + $lock_data['user']['avatar'] = rest_get_avatar_urls( $user ); + } + } + } } } @@ -433,13 +453,50 @@ public function get_item_schema(): array { 'context' => [ 'view', 'edit', 'embed' ], ], 'user' => [ - 'description' => __( 'The ID for the author of the lock.', 'web-stories' ), - 'type' => 'integer', - 'context' => [ 'view', 'edit', 'embed' ], + 'description' => __( 'User', 'web-stories' ), + 'type' => 'object', + 'properties' => [ + 'id' => [ + 'description' => __( 'The ID for the author of the lock.', 'web-stories' ), + 'type' => 'integer', + 'readonly' => true, + 'context' => [ 'view', 'edit', 'embed' ], + ], + 'name' => [ + 'description' => __( 'Display name for the user.', 'web-stories' ), + 'type' => 'string', + 'readonly' => true, + 'context' => [ 'embed', 'view', 'edit' ], + ], + ], ], ], ]; + if ( get_option( 'show_avatars' ) ) { + $avatar_properties = []; + + $avatar_sizes = rest_get_avatar_sizes(); + + foreach ( $avatar_sizes as $size ) { + $avatar_properties[ $size ] = [ + /* translators: %d: Avatar image size in pixels. */ + 'description' => sprintf( __( 'Avatar URL with image size of %d pixels.', 'web-stories' ), $size ), + 'type' => 'string', + 'format' => 'uri', + 'context' => [ 'embed', 'view', 'edit' ], + ]; + } + + $schema['properties']['user']['properties']['avatar'] = [ + 'description' => __( 'Avatar URLs for the user.', 'web-stories' ), + 'type' => 'object', + 'context' => [ 'embed', 'view', 'edit' ], + 'readonly' => true, + 'properties' => $avatar_properties, + ]; + } + $this->schema = $schema; return $this->add_additional_fields_schema( $this->schema ); diff --git a/includes/templates/admin/edit-story.php b/includes/templates/admin/edit-story.php index 4a8616e9edc5..eb69165a59f7 100644 --- a/includes/templates/admin/edit-story.php +++ b/includes/templates/admin/edit-story.php @@ -95,7 +95,7 @@ '_embed' => rawurlencode( implode( ',', - [ 'wp:lockuser', 'author', 'wp:publisherlogo', 'wp:term' ] + [ 'wp:lock', 'author', 'wp:publisherlogo', 'wp:term' ] ) ), 'context' => 'edit', diff --git a/packages/wp-dashboard/src/api/constants/index.js b/packages/wp-dashboard/src/api/constants/index.js index 38ca210a4722..e8da92c6b56b 100644 --- a/packages/wp-dashboard/src/api/constants/index.js +++ b/packages/wp-dashboard/src/api/constants/index.js @@ -35,7 +35,7 @@ export const STORY_FIELDS = [ export const SEARCH_PAGES_FIELDS = ['id', 'title']; export const GET_PAGE_FIELDS = ['title', 'link']; -export const STORY_EMBED = 'wp:lock,wp:lockuser,author'; +export const STORY_EMBED = 'wp:lock,author'; export const REST_LINKS = { EDIT: 'wp:action-edit', diff --git a/packages/wp-dashboard/src/api/utils/reshapeStoryObject.js b/packages/wp-dashboard/src/api/utils/reshapeStoryObject.js index 48cfadbfde8b..8457db177209 100644 --- a/packages/wp-dashboard/src/api/utils/reshapeStoryObject.js +++ b/packages/wp-dashboard/src/api/utils/reshapeStoryObject.js @@ -32,16 +32,16 @@ export default function reshapeStoryObject(originalStoryData) { preview_link: previewLink, edit_link: editStoryLink, story_poster: storyPoster, - _embedded: { - author = [{ name: '', id: 0 }], - 'wp:lock': lock = [{ locked: false }], - 'wp:lockuser': lockUser = [{ id: 0, name: '' }], - } = {}, + _embedded: { author, 'wp:lock': lock } = {}, _links: links = {}, } = originalStoryData; if (!id) { return null; } + + const { locked = false, user: lockUser = { id: 0, name: '' } } = + lock?.[0] || {}; + const storyAuthor = author?.[0] || { id: 0, name: '' }; const capabilities = { hasEditAction: Object.prototype.hasOwnProperty.call(links, REST_LINKS.EDIT), hasDeleteAction: Object.prototype.hasOwnProperty.call( @@ -59,14 +59,13 @@ export default function reshapeStoryObject(originalStoryData) { modified, modifiedGmt: `${modified_gmt}Z`, author: { - name: author[0].name, - id: author[0].id, + id: storyAuthor.id, + name: storyAuthor.name, }, - locked: lock[0]?.locked, + locked, lockUser: { - id: lockUser[0].id, - name: lockUser[0].name, - avatar: lockUser[0]?.avatar_urls?.['96'] || null, + ...lockUser, + avatar: lockUser?.avatar?.['96'] || null, }, bottomTargetAction: editStoryLink, featuredMediaUrl: storyPoster?.url, diff --git a/packages/wp-dashboard/src/api/utils/test/reshapeStoryObject.js b/packages/wp-dashboard/src/api/utils/test/reshapeStoryObject.js index 89050973a3b5..5a75686dbb7f 100644 --- a/packages/wp-dashboard/src/api/utils/test/reshapeStoryObject.js +++ b/packages/wp-dashboard/src/api/utils/test/reshapeStoryObject.js @@ -51,18 +51,18 @@ describe('reshapeStoryObject', () => { }, _embedded: { author: [{ id: 1, name: 'admin' }], - 'wp:lock': [{ locked: true, time: '1628506372', user: 1 }], - 'wp:lockuser': [ + 'wp:lock': [ { - id: 1, - name: 'admin', - url: 'http://localhost:8899', - description: '', - link: 'http://localhost:8899/author/admin', - avatar_urls: { - 24: 'http://2.gravatar.com/avatar/b642b4217b34b1e8d3bd915fc65c4452?s=24&d=mm&r=g', - 48: 'http://2.gravatar.com/avatar/b642b4217b34b1e8d3bd915fc65c4452?s=48&d=mm&r=g', - 96: 'http://2.gravatar.com/avatar/b642b4217b34b1e8d3bd915fc65c4452?s=96&d=mm&r=g', + locked: true, + time: '1628506372', + user: { + id: 1, + name: 'admin', + avatar: { + 24: 'http://2.gravatar.com/avatar/b642b4217b34b1e8d3bd915fc65c4452?s=24&d=mm&r=g', + 48: 'http://2.gravatar.com/avatar/b642b4217b34b1e8d3bd915fc65c4452?s=48&d=mm&r=g', + 96: 'http://2.gravatar.com/avatar/b642b4217b34b1e8d3bd915fc65c4452?s=96&d=mm&r=g', + }, }, }, ], @@ -127,14 +127,14 @@ describe('reshapeStoryObject', () => { }, _embedded: { author: [{ id: 1, name: 'admin' }], - 'wp:lock': [{ locked: true, time: '1628506372', user: 1 }], - 'wp:lockuser': [ + 'wp:lock': [ { - id: 1, - name: 'admin', - url: 'http://localhost:8899', - description: '', - link: 'http://localhost:8899/author/admin', + locked: true, + time: '1628506372', + user: { + id: 1, + name: 'admin', + }, }, ], }, diff --git a/packages/wp-story-editor/src/api/constants/index.js b/packages/wp-story-editor/src/api/constants/index.js index 6d52c2a10de7..d405104f3aab 100644 --- a/packages/wp-story-editor/src/api/constants/index.js +++ b/packages/wp-story-editor/src/api/constants/index.js @@ -35,7 +35,7 @@ export const STORY_FIELDS = [ '_links', ].join(','); -export const STORY_EMBED = 'wp:lockuser,author,wp:publisherlogo,wp:term'; +export const STORY_EMBED = 'wp:lock,author,wp:publisherlogo,wp:term'; export const MEDIA_FIELDS = [ 'id', diff --git a/packages/wp-story-editor/src/api/storyLock.js b/packages/wp-story-editor/src/api/storyLock.js index 254a26adad52..2cad8791d18d 100644 --- a/packages/wp-story-editor/src/api/storyLock.js +++ b/packages/wp-story-editor/src/api/storyLock.js @@ -24,11 +24,7 @@ import { addQueryArgs } from '@googleforcreators/url'; import apiFetch from '@wordpress/api-fetch'; export function getStoryLockById(storyId, stories) { - const path = addQueryArgs(`${stories}${storyId}/lock/`, { - _embed: 'author', - }); - - return apiFetch({ path }); + return apiFetch({ path: `${stories}${storyId}/lock/` }); } export function setStoryLockById(storyId, stories) { diff --git a/packages/wp-story-editor/src/api/utils/transformStoryResponse.js b/packages/wp-story-editor/src/api/utils/transformStoryResponse.js index e2c516db4733..3d374c449607 100644 --- a/packages/wp-story-editor/src/api/utils/transformStoryResponse.js +++ b/packages/wp-story-editor/src/api/utils/transformStoryResponse.js @@ -36,9 +36,9 @@ function transformStoryResponse(post) { capabilities: {}, extras: { lockUser: { - id: embedded?.['wp:lockuser']?.[0].id || 0, - name: embedded?.['wp:lockuser']?.[0].name || '', - avatar: embedded?.['wp:lockuser']?.[0].avatar_urls?.['96'] || '', + id: embedded?.['wp:lock']?.[0].user.id || 0, + name: embedded?.['wp:lock']?.[0].user.name || '', + avatar: embedded?.['wp:lock']?.[0].user.avatar?.['96'] || '', }, }, featuredMedia: storyPoster diff --git a/packages/wp-story-editor/src/components/postLock/postLock.js b/packages/wp-story-editor/src/components/postLock/postLock.js index 8a5568dee40f..462167fb6109 100644 --- a/packages/wp-story-editor/src/components/postLock/postLock.js +++ b/packages/wp-story-editor/src/components/postLock/postLock.js @@ -96,13 +96,17 @@ function PostLock() { // When async call only if dialog is true, current user is loaded and post locking is enabled. const doGetStoryLock = useCallback(() => { - if (showLockedDialog && currentUserLoaded) { - getStoryLockById(storyId, stories) - .then(({ locked, nonce: newNonce, _embedded }) => { + (async () => { + if (showLockedDialog && currentUserLoaded) { + try { + const { + locked, + nonce: newNonce, + user, + } = await getStoryLockById(storyId, stories); const lockAuthor = { - id: _embedded?.author?.[0]?.id || 0, - name: _embedded?.author?.[0]?.name || '', - avatar: _embedded?.author?.[0]?.avatar_urls?.['96'] || '', + ...user, + avatar: user?.avatar?.['96'] || '', }; if (locked && initialOwner === null) { setInitialOwner(lockAuthor); @@ -114,11 +118,11 @@ function PostLock() { } // Refresh nonce on every request. setNonce(newNonce); - }) - .catch((err) => { + } catch (err) { trackError('post_lock', err.message); - }); - } + } + } + })(); }, [ setCurrentOwner, storyId, diff --git a/tests/phpunit/integration/tests/REST_API/Stories_Lock_Controller.php b/tests/phpunit/integration/tests/REST_API/Stories_Lock_Controller.php index f13098562ed2..783453e1c505 100644 --- a/tests/phpunit/integration/tests/REST_API/Stories_Lock_Controller.php +++ b/tests/phpunit/integration/tests/REST_API/Stories_Lock_Controller.php @@ -195,6 +195,47 @@ public function test_get_item_with_lock(): void { $data = $response->get_data(); $links = $response->get_links(); $this->assertArrayHasKey( 'locked', $data ); + $this->assertArrayHasKey( 'user', $data ); + $this->assertArrayHasKey( 'id', $data['user'] ); + $this->assertArrayHasKey( 'name', $data['user'] ); + $this->assertArrayHasKey( 'avatar', $data['user'] ); + $this->assertSame( self::$author_id, $data['user']['id'] ); + $this->assertTrue( $data['locked'] ); + + $this->assertArrayHasKey( 'self', $links ); + $this->assertArrayHasKey( 'author', $links ); + } + + /** + * @covers ::get_item + * @covers ::prepare_item_for_response + * @covers ::prepare_links + * @covers ::get_item_permissions_check + */ + public function test_get_item_with_lock_disabled_avatar(): void { + update_option( 'show_avatars', false ); + $this->controller->register(); + + wp_set_current_user( self::$author_id ); + $story = self::factory()->post->create( + [ + 'post_type' => \Google\Web_Stories\Story_Post_Type::POST_TYPE_SLUG, + 'post_status' => 'draft', + 'post_author' => self::$author_id, + ] + ); + $new_lock = ( time() - 100 ) . ':' . self::$author_id; + update_post_meta( $story, '_edit_lock', $new_lock ); + $request = new WP_REST_Request( \WP_REST_Server::READABLE, '/web-stories/v1/web-story/' . $story . '/lock' ); + $response = rest_get_server()->dispatch( $request ); + $data = $response->get_data(); + $links = $response->get_links(); + $this->assertArrayHasKey( 'locked', $data ); + $this->assertArrayHasKey( 'user', $data ); + $this->assertArrayHasKey( 'id', $data['user'] ); + $this->assertArrayHasKey( 'name', $data['user'] ); + $this->assertArrayNotHasKey( 'avatar', $data['user'] ); + $this->assertSame( self::$author_id, $data['user']['id'] ); $this->assertTrue( $data['locked'] ); $this->assertArrayHasKey( 'self', $links );