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

Site Title: Add a view mode for non-admins #18760

Open
wants to merge 5 commits into
base: master
from

Conversation

@scruffian
Copy link

scruffian commented Nov 26, 2019

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)
Screenshot 2019-11-26 at 17 08 55

Non-admin
Screenshot 2019-11-26 at 17 08 47

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. .
@@ -9,14 +11,15 @@ import { Button } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { RichText } from '@wordpress/block-editor';

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 26, 2019

Contributor

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

@@ -1,6 +1,8 @@
/**
* WordPress dependencies
*/
import { compose } from '@wordpress/compose';
import { withSelect } from '@wordpress/data';

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 26, 2019

Contributor

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.

<h1>{ title }</h1>
);

return ( props.canUpdate && props.isSelected ) ? editMode : viewMode;

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 26, 2019

Contributor

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.

This comment has been minimized.

Copy link
@mtias

mtias Nov 26, 2019

Contributor

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

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 26, 2019

Member

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.

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

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 26, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Nov 26, 2019

See #18772.

@TimothyBJacobs

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Nov 27, 2019

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

@TimothyBJacobs

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Nov 27, 2019

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 scruffian requested review from nerrad and youknowriad as code owners Dec 4, 2019
@scruffian

This comment has been minimized.

Copy link
Author

scruffian commented Dec 4, 2019

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

This comment has been minimized.

@scruffian

This comment has been minimized.

Copy link
Author

scruffian commented Dec 4, 2019

@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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Contributor

mtias commented Dec 4, 2019

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

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 4, 2019

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

@TimothyBJacobs

This comment has been minimized.

Copy link
Member

TimothyBJacobs commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Author

scruffian commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Author

scruffian commented Dec 5, 2019

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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 5, 2019

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

@scruffian

This comment has been minimized.

Copy link
Author

scruffian commented Dec 5, 2019

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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 5, 2019

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

@scruffian

This comment has been minimized.

Copy link
Author

scruffian commented Dec 6, 2019

I created #18970 to use the new API endpoint

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 6, 2019

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

This comment has been minimized.

Copy link
Author

scruffian commented Dec 6, 2019

@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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 6, 2019

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

This comment has been minimized.

Copy link
Author

scruffian commented Dec 7, 2019

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

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 7, 2019

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

This comment has been minimized.

Copy link
Author

scruffian commented Dec 8, 2019

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

@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 8, 2019

@scruffian

This comment has been minimized.

Copy link
Author

scruffian commented Dec 9, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Full site editing
Awaiting triage
5 participants
You can’t perform that action at this time.