Core Data: Add new useEntityRecordsWithPermissions hook #63857
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
youknowriad
added
[Type] Code Quality
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
youknowriad
commented
Jul 23, 2024
|
Size Change: +228 B (+0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
ellatrix
reviewed
Jul 24, 2024
| permissions: permissions[ index ], | ||
| } ) ) ?? [], | ||
| [ data, permissions ] | ||
| ); |
There was a problem hiding this comment.
Why not do this is a single memoized selector?
There was a problem hiding this comment.
I think that would also reduce the loops from 3 to 1?
There was a problem hiding this comment.
I think that would also reduce the loops from 3 to 1?
I don't think it reduces anything as the loop will exist but within the selector. I'm not sure which is better to be honest. A huge memoized selector or smaller ones with some in between memoizations. I have a feeling that the latter is better in terms of performance but the former is easier to work with.
There was a problem hiding this comment.
Yes, but it would just be one loop, that immediately fetches the permission and sets it on the item?
ellatrix
reviewed
Jul 24, 2024
This is what I want to do in a follow-up but I found that the way we fetch patterns and template parts is so complex (it's actually hard to even call a simple selector there). Doable but that part is not great code to be honest. |
|
Yes, just mentioning it because maybe it's relevant in making a decision? Fine with me to try in a follow-up |
ellatrix
approved these changes
Jul 24, 2024
|
I'm afraid, I have to do a bigger refactoring to the templates and patterns pages to be able to do that easily. I'll try to either open a refactoring PR or open a follow-up PR to this one before merging the current one. |
| export const getEntityRecordsPermissions = createSelector( | ||
| ( state: State, kind: string, name: string, ids: string[] ) => { | ||
| return ids.map( ( id ) => ( { | ||
| delete: canUser( state, 'delete', { |
There was a problem hiding this comment.
We now have two canUser calls, but I checked and we still make one request per id and not two so it is fine.
There was a problem hiding this comment.
Yes, requests are deduped for related actions - #43480.
jorgefilipecosta
approved these changes
Jul 24, 2024
Mamaduka
approved these changes
Jul 24, 2024
There was a problem hiding this comment.
I think this is OK as a temporary solution, and I see it has already helped us highlight some pain points.
I'm not updating the actions yet, so in reality the current PR has no impact but it's the first step towards achieving that goal.
@youknowriad, what actions updates do you have in mind
|
@Mamaduka All the actions in post-actions/actions.js basically. Anything using useCanUserEligibilityCheckPostType |
youknowriad
force-pushed
the
update/explore-use-entity-records-with-permissions
branch
from
0b55895
to
a78b7ed
Compare
youknowriad
deleted the
update/explore-use-entity-records-with-permissions
branch
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related #55083
What?
In different dataviews, we need a way to check whether the user has the right permissions to edit or delete a given item, we've tried an approach by dynamically updating the "action" object's is Eligible but that was bad for several reasons:
As discussed in slack, the right approach might be to embed the permissions within the "data". https://wordpress.slack.com/archives/C02QB2JS7/p1721732016850379
The current PR adds a new
useEntityRecordsWithPermissionstocore-datato be able to do that in an easy way. I'm not updating the actions yet, so in reality the current PR has no impact but it's the first step towards achieving that goal.Testing Instructions
Nothing to test really at the moment.