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.

Sign up for GitHub

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

Jump to bottom

Event capabilities #215

Merged
merged 26 commits into from
Apr 22, 2024
Merged

Event capabilities #215

merged 26 commits into from
Apr 22, 2024

Conversation

psrpinto
Copy link
Member

@psrpinto psrpinto commented Apr 16, 2024 •
edited
Loading

This PR introduces the following custom capabilities, and changes the whole codebase so that these capabilities are used, through calls to current_user_can():

  • create_translation_event
  • view_translation_event
  • edit_translation_event
  • delete_translation_event

There should be no changes to how things work, everything should work the same as before.

References

This function also accepts an ID of an object to check against if the capability is a meta capability. Meta capabilities such as edit_post and edit_user are capabilities used by the map_meta_cap() function to map to primitive capabilities that a user or role has, such as edit_posts and edit_others_posts.

@psrpinto psrpinto mentioned this pull request Apr 16, 2024
Closed
@psrpinto psrpinto force-pushed the event-capabilities branch from 9342e5f to c66c38a Compare April 16, 2024 15:37
@psrpinto psrpinto self-assigned this Apr 16, 2024
@psrpinto psrpinto force-pushed the event-capabilities branch 3 times, most recently from 4464bcb to 2a8a778 Compare April 17, 2024 14:57
@psrpinto psrpinto mentioned this pull request Apr 17, 2024
Closed
@psrpinto psrpinto force-pushed the event-capabilities branch 2 times, most recently from 7447138 to aa0cd44 Compare April 18, 2024 13:23
@psrpinto psrpinto force-pushed the event-capabilities branch from aa0cd44 to 813c5e4 Compare April 18, 2024 13:56
@psrpinto psrpinto force-pushed the event-capabilities branch from 1551e30 to 196745c Compare April 18, 2024 14:57
psrpinto added 6 commits April 18, 2024 15:57
@psrpinto
Return 404 if event is not found
ccc278f
@psrpinto
Use capabilities to check if user can view the edit page
5f652d8
@psrpinto
GlotPress admins can always edit
a785a4b
@psrpinto
Check for delete_post instead of manage_options
d990113 
We were checking for manage_options to know whether the user can view the edit page,
and for delete_post when handling form submission.

We will now standardise on checking for delete_post in all places.
@psrpinto
Use current_user_can() to check for permissions
3260aea
@psrpinto
Move variables to where they are first needed
0a551ea
@psrpinto psrpinto force-pushed the event-capabilities branch from 8f2004c to 0a551ea Compare April 18, 2024 15:24
@psrpinto psrpinto force-pushed the event-capabilities branch from 2246235 to 3d2bab7 Compare April 18, 2024 16:18
@psrpinto psrpinto marked this pull request as ready for review April 18, 2024 16:20
@psrpinto psrpinto force-pushed the event-capabilities branch from 3d5116c to a9fced4 Compare April 18, 2024 16:44
akirk
akirk approved these changes Apr 19, 2024
View reviewed changes
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job, this vastly improves the readability and ensures consistency. Also thanks for the tests!

akirk
akirk requested changes Apr 19, 2024
View reviewed changes
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have found one thing that no longer works: When an event doesn't have a host, the creator should be able to edit.

akirk
akirk approved these changes Apr 19, 2024
View reviewed changes
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to re-approve this. I think this might not change the current behavior but we might need to discuss it wrt when editing an event should remain possible.

includes/event/event-capabilities.php Outdated Show resolved Hide resolved
includes/event/event-capabilities.php
Comment on lines +114 to +116
if ( $this->stats_calculator->event_has_stats( $event->id() ) ) {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is too early and/or we need more fine grained controls (which is easy now!) But I think it should be possible to change at least the event description (and maybe the event end, if it should be extended) even when there are stats.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, more fine grained capabilities could make sense, e.g. edit_translation_event_with_stats. Also agree that it it should be possible to edit certain fields of a past event. I opened #227 to track that.

@psrpinto psrpinto mentioned this pull request Apr 19, 2024
Closed
@psrpinto
Copy link
Member Author

I have found one thing that no longer works: When an event doesn't have a host, the creator should be able to edit.

I'm working on a fix for this

@akirk
Copy link
Member

akirk commented Apr 19, 2024

@psrpinto I'm no longer sure this is true. I think I had tested with an event that already had contributions.

@psrpinto
Copy link
Member Author

There was indeed an issue when the event_id is a string and not an int. Now fixed.

@psrpinto psrpinto requested a review from akirk April 19, 2024 13:50
@psrpinto
Copy link
Member Author

Merging since @akirk had approved, and after his approval there was only the bug fix in the last commit.

@psrpinto psrpinto merged commit 730c34a into trunk Apr 22, 2024
3 checks passed
@psrpinto psrpinto deleted the event-capabilities branch April 22, 2024 14:27
@akirk
Copy link
Member

akirk commented Apr 22, 2024

Sorry for missing the re-review, thanks!

@psrpinto
Copy link
Member Author

No worries!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amieiro amieiro Awaiting requested review from amieiro

@trymebytes trymebytes Awaiting requested review from trymebytes

@akirk akirk Awaiting requested review from akirk

Assignees

@psrpinto psrpinto

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@psrpinto @akirk