Site Title: Add a view mode for non-admins by scruffian · Pull Request #18760 · WordPress/gutenberg

scruffian · 2019-11-26T17:10:07Z

Description

This updates the Site Title block to account for non-admins.

It's possible that this block could be added to a post that anyone has access to. Only admins should be able to edit the content, but all users should be able to see this block so that they have an accurate preview.

How has this been tested?

Create a 'non-admin' account on your WordPress site. Add the Site Title block to a post. You should be able to see the site title, but not edit it:

Screenshots

Admin (unchanged)

Non-admin

Types of changes

New feature (non-breaking change which adds functionality)

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR. .

epiqueras · 2019-11-26T18:36:39Z

 import { RichText } from '@wordpress/block-editor';

-export default function SiteTitleEdit() {
+export function SiteTitleEdit( props ) {


We should follow the style of the other components in this package and destructure props.

epiqueras · 2019-11-26T18:39:27Z

 * WordPress dependencies
 */
+import { compose } from '@wordpress/compose';
+import { withSelect } from '@wordpress/data';


withSelect uses useSelect under the hood and we are moving towards using that to better compose with other hooks, take advantage of functional component performance improvements, and improve readability.

epiqueras · 2019-11-26T18:42:44Z

+		<h1>{ title }</h1>
+	);
+
+	return ( props.canUpdate && props.isSelected ) ? editMode : viewMode;


Can we disable the input instead of introducing this split rendering path?

It seems innocuous in this context, but split edit/view paths will make it really hard to keep a consistent rendering style, so it will be better if we enforce using a single rendering path from the start.

+1 to thinking more about how split render paths can be avoided.

I agree. We explored a read only mode for RichText a while ago that could be enabled based on the context (a locked template or block for example). Additionally it would be good to disable attribute updating.

epiqueras · 2019-11-26T18:46:16Z

+	withSelect( ( select, {} ) => {
+		const { canUser } = select( 'core' );
+		return {
+			canUpdate: canUser( 'update', 'settings' ),


Just a note that I intend to make this part of the framework.

I envision that core-data will keep track of which entity properties are locked, actions can change that, entity configs provide default values/selectors, and useEntityProp will return a third boolean variable that says whether the specified prop is locked or not.

epiqueras · 2019-11-26T23:08:03Z

See #18772.

TimothyBJacobs · 2019-11-27T18:53:26Z

The site title is available via the index in the REST API under name.

TimothyBJacobs · 2019-11-27T19:37:01Z

I've also opened https://core.trac.wordpress.org/ticket/48816 since it seems like using the actual site title with display filters applied would also be wanted?

scruffian · 2019-12-04T17:33:17Z

I updated this to use the index path in the REST API for name.

To avoid splitting the render path we need to do some more work on RichText so it has a read only mode. I'll start on this in a different PR, but maybe we can get this merged anyway, and circle back?

epiqueras · 2019-12-04T17:46:22Z

Have you looked at: https://github.com/WordPress/gutenberg/pull/18772/files#diff-655f76535495d8ae1d82693b57a4ffba?

scruffian · 2019-12-04T18:07:37Z

@epiqueras, the problem is that non-admins can't access the REST API for this path at all, so I'm not sure that solution is going to help. :(

epiqueras · 2019-12-04T18:12:59Z

We should find a way to unify these two APIs. Either make name editable to admins in 'base' or make title viewable to non-admins in 'site'.

In the meantime, f6ce79c can still be applied to #18772.

See #18772 (comment) for more reasoning.

mtias · 2019-12-04T18:39:45Z

Also, I imagine the entity engine should be able to switch the API request if needed based on the locked context.

epiqueras · 2019-12-04T19:09:35Z

Also, I imagine the entity engine should be able to switch the API request if needed based on the locked context.

That starts to become too convoluted. There could be multiple reasons why a property is locked and no way to map them to a single "correct" API request without a lot of extra configuration. Different consumers will also have vastly different requirements for this.

This should be handled at the endpoint level where the required data and functionality already exists. There is no reason we couldn't have an additional endpoint that delegates to / and /settings under the hood depending on the request context.

TimothyBJacobs · 2019-12-04T19:51:34Z

They are two entirely separate contexts, one is for editing a setting, the other is for getting the rendered site title.

We're not going to support updating properties on the site index. Allow users read only access to the settings endpoint has a huge number of security implications.

epiqueras · 2019-12-04T19:55:35Z

There is no reason we couldn't have an additional endpoint that delegates to / and /settings under the hood depending on the request context.

Is that viable? Only exposing properties which are safe to be read by users of course.

TimothyBJacobs · 2019-12-04T19:57:02Z

I think those properties would be on the index. If it would be architecturally simpler, we could potential nest them under something like settings on the root response.

epiqueras · 2019-12-04T19:58:31Z

Yeah, we just need to use the same endpoint for reading and updating.

TimothyBJacobs · 2019-12-04T20:00:49Z

I don't see why switching the request on the client to the base is a problem? It seems exactly the type of logic the API client would do instead of coding a specific endpoint on the server. That data should also already be loaded.

Further, the values themselves should be different. Whatever is on settings is in an editable format. Whereas the site index should have the rendered data.

epiqueras · 2019-12-04T20:05:08Z

Because it breaks a single resource into two when there's no need to. It's inconsistent and will make it harder to maintain. Other properties have .raw and .rendered for that.

scruffian · 2019-12-04T20:16:46Z

make title viewable to non-admins in 'site'.

This won't be possible, see: https://core.trac.wordpress.org/ticket/48812.

I'll investigate the reverse...

scruffian · 2019-12-05T12:29:55Z

The index route isn't currently writable: https://github.com/WordPress/WordPress/blob/master/wp-includes/rest-api/class-wp-rest-server.php#L91

I can't forsee that changing either. Maybe we need to create new routes for each option?

epiqueras · 2019-12-05T16:21:42Z

I think we should do this: #18760 (comment).

scruffian · 2019-12-05T17:15:43Z

I started a patch for this on the REST API: https://core.trac.wordpress.org/attachment/ticket/48885

I'm not sure how to go about implementing this in Gutenberg...

epiqueras · 2019-12-05T17:56:31Z

Add a new entity for the new resource like you did here for "base": f6ce79c#diff-c5eaec560b039f2dc76f93fa76ba89da.

scruffian · 2019-12-06T12:08:52Z

I created #18970 to use the new API endpoint

epiqueras · 2019-12-06T17:43:29Z

I don't think we should have a specific endpoint just for the title. Won't there be other site settings that need this functionality? Why not something like /settings/public or something like that.

scruffian · 2019-12-06T21:23:49Z

@epiqueras the endpoint supports /settings/<option>, so:

/settings/title
/settings/description
etc.

I wasn't sure how to extend the definition of entities to allow this.

epiqueras · 2019-12-06T22:42:08Z

Yes, but why?

All of these properties follow the same reading and updating logic. They should be grouped under something like /settings/public instead of introducing a new path for each individual property.

scruffian · 2019-12-07T23:50:48Z

All of these properties follow the same reading and updating logic. They should be grouped under something like /settings/public instead of introducing a new path for each individual property.

Would this be identical to what /settings is at the moment, just with different access rights?

epiqueras · 2019-12-07T23:57:37Z

No:

We're not going to support updating properties on the site index. Allow users read only access to the settings endpoint has a huge number of security implications.

Only for a safe subset of settings.

scruffian · 2019-12-08T00:02:02Z

Ah, ok, that makes sense thanks. I'll update the patch to take this approach on Monday. Sorry for the confusion.

epiqueras · 2019-12-08T00:03:17Z

Awesome! Thanks :-)

scruffian · 2019-12-09T13:55:50Z

I updated this, and the corresponding core patch (https://core.trac.wordpress.org/ticket/48885) to put all the public settings in the same route.

noahtallen · 2020-03-17T22:21:10Z

Reading the core ticket, it looks like the API endpoint needs to be implemented in the Gutenberg plugin?

gziolo · 2020-10-28T13:47:58Z

@scruffian, is it something that still needs to be worked on?

scruffian · 2020-10-28T15:45:37Z

The issue is how we can let non-admins edit site level blocks like this. I don't think that problem has been solved yet.

annezazu · 2022-07-27T23:01:37Z

From what I understand, this can be closed thanks to this work: #32817 I'm going to do so but, if that's wrong, happy to reopen :)

Update site title to consider what happens when non-admins try to use it

e6779cb

scruffian requested review from Soean, ajitbohra, jorgefilipecosta and talldan as code owners November 26, 2019 17:10

scruffian mentioned this pull request Nov 26, 2019

REST API: Settings endpoint - read access WordPress/wordpress-develop#120

Closed

epiqueras requested changes Nov 26, 2019

View reviewed changes

mtias added the [Feature] Full Site Editing label Nov 26, 2019

epiqueras mentioned this pull request Nov 26, 2019

Core Data: Add entity prop locking and use it in the Site Title block. #18772

Closed

6 tasks

scruffian added 2 commits November 27, 2019 15:17

destructure props

5a17d24

switch to useSelect

92e47fa

Load the site title for non-admin users

f6ce79c

scruffian requested review from nerrad and youknowriad as code owners December 4, 2019 17:31

add a not-allowed cursor

bbd0872

scruffian mentioned this pull request Jan 30, 2020

WIP: initial commit for the rest api settings change #19967

Closed

6 tasks

earnjam mentioned this pull request Mar 21, 2020

Site Title: Access the new API endpoint for editing the site title #18970

Closed

6 tasks

gziolo added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Oct 28, 2020

jameskoster mentioned this pull request Oct 29, 2020

Full Site Editing: Map features and block edit-ability to capabilities and update the UI where necessary #26573

Closed

Base automatically changed from master to trunk March 1, 2021 15:42

annezazu closed this Jul 27, 2022

Conversation

scruffian commented Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

Uh oh!

epiqueras Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

epiqueras Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

epiqueras Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

mtias Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

ellatrix Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

epiqueras Nov 26, 2019

Choose a reason for hiding this comment

Uh oh!

epiqueras commented Nov 26, 2019

Uh oh!

TimothyBJacobs commented Nov 27, 2019

Uh oh!

TimothyBJacobs commented Nov 27, 2019

Uh oh!

scruffian commented Dec 4, 2019

Uh oh!

epiqueras commented Dec 4, 2019

Uh oh!

scruffian commented Dec 4, 2019

Uh oh!

epiqueras commented Dec 4, 2019

Uh oh!

mtias commented Dec 4, 2019

Uh oh!

epiqueras commented Dec 4, 2019

Uh oh!

TimothyBJacobs commented Dec 4, 2019

Uh oh!

epiqueras commented Dec 4, 2019

Uh oh!

TimothyBJacobs commented Dec 4, 2019

Uh oh!

epiqueras commented Dec 4, 2019

Uh oh!

TimothyBJacobs commented Dec 4, 2019

Uh oh!

epiqueras commented Dec 4, 2019

Uh oh!

scruffian commented Dec 4, 2019

Uh oh!

scruffian commented Dec 5, 2019

Uh oh!

epiqueras commented Dec 5, 2019

Uh oh!

scruffian commented Dec 5, 2019

Uh oh!

epiqueras commented Dec 5, 2019

Uh oh!

scruffian commented Dec 6, 2019

Uh oh!

epiqueras commented Dec 6, 2019

Uh oh!

scruffian commented Dec 6, 2019

Uh oh!

epiqueras commented Dec 6, 2019

Uh oh!

scruffian commented Dec 7, 2019

Uh oh!

epiqueras commented Dec 7, 2019

Uh oh!

scruffian commented Dec 8, 2019

scruffian commented Nov 26, 2019 •

edited

Loading