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

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

Open
wants to merge 3 commits into
base: master
from

Conversation

@epiqueras
Copy link
Contributor

epiqueras commented Nov 26, 2019

Description

This PR addresses a need found by #18760 and is an alternative to it.

Certain entity props, like a site's title, should not always be editable depending on editing context and/or user permissions. The framework should provide a declarative way of "locking" these props.

This PR provides a way to define default "locked" values for different entity types in their configs, and a way to track and edit these values on a per-entity-record basis. The "locked" values can either be literals that will be wrapped in getters, i.e. true, becomes () => true and means a prop is locked, false or undefined means it's not, or they can be functions that will be pre-bound to select.

This approach allows for simplicity and the flexibility for consumers to derive the values based on any state, like the site entity config does in this PR with canUser.

Additionally, useEntityProp's setter now throws a warning when trying to set a value for a locked prop, and it also returns a third variable, the evaluated "locked" value for the specified prop. This made the changes needed in the Site Title block minimal and it made it really easy to avoid forking the rendering path when the site title is locked.

How has this been tested?

It was verified that using the Site Title block logged in as a contributor renders it disabled.

Types of Changes

New Feature: core-data entities now support locked entity props for managing props that shouldn't be editable under certain context and/or with certain user permission levels.

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. .
Copy link
Contributor

youknowriad left a comment

Not sure I understand why we need the ability to lock properties, for me this is more a value that comes built-in depending on the role of the logged-in user instead of something you should/can set.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 27, 2019

Locking will be necessary in situations outside of user permissions/authentication (think Newspack blocks) and even the way we query user permissions can vary enough that we benefit from making this general in the framework.

Compare the changes to SiteTitleEdit in this PR vs. #18760. Locking leads to much more maintainable block authoring.

@epiqueras epiqueras force-pushed the add/entity-prop-locking branch from 4908880 to 63378bf Nov 27, 2019
@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 28, 2019

I'm still not convinced here to be honest. It's not about the consuming API itself which I'm fine with but it's just a duplicate of canUser.

It's more about the responsibilities. I don't think locked properties should be defined in the entity config as they can change per user, we should just consume them from the existing Rest APIs and I don't think a public setLockedEntityProps action make sense.

@epiqueras

This comment has been minimized.

Copy link
Contributor Author

epiqueras commented Nov 28, 2019

I don't think locked properties should be defined in the entity config as they can change per user, we should just consume them from the existing Rest APIs and I don't think a public setLockedEntityProps action make sense.

Think about cases where a prop may be locked for different reasons in different circumstances and this doesn't map directly to things like canUser. What if, for example, a user can edit a post's title, but you want to lock it when you switch to a certain editor mode. This could certainly be done at the block level, but what if there are multiple blocks that edit the same prop? This would avoid that code duplication.

@mtias also has more thoughts on this.

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