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

feat(permission): add delete own posts permission #3784

Merged
merged 6 commits into from
Apr 17, 2023
Merged

Conversation

OrdinaryJellyfish
Copy link
Contributor

Fixes https://discuss.flarum.org/d/31993-adding-permission-to-delete-own-posts

Changes proposed in this pull request:

  • Add permission for users to delete their own posts as a time-based permission

Reviewers should focus on:

  • Does the permission work properly?
  • Is it in the right spot on the permission grid?

Screenshot

image

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

None

@OrdinaryJellyfish OrdinaryJellyfish requested a review from a team as a code owner April 7, 2023 00:37
@SychO9 SychO9 added this to the 1.8 milestone Apr 10, 2023
Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

Nice!

One thing I'd like us to make consistent is the key of the permission and locale label. Let's make tem consistent and use allow_hide_own_posts for te permission key AND the label allow_hide_own_posts_label, andeplace any allow_delete_own_posts

framework/core/js/src/admin/components/PermissionGrid.tsx Outdated Show resolved Hide resolved
@@ -71,6 +71,14 @@ public function edit(User $actor, Post $post)
*/
public function hide(User $actor, Post $post)
{
return $this->edit($actor, $post);
if ($post->user_id == $actor->id && (! $post->hidden_at || $post->hidden_user_id == $actor->id) && $actor->can('reply', $post->discussion)) {
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should drop the bit about only being able to hide when the discussion is still open 🤔

Suggested change
if ($post->user_id == $actor->id && (! $post->hidden_at || $post->hidden_user_id == $actor->id) && $actor->can('reply', $post->discussion)) {
if ($post->user_id == $actor->id && (! $post->hidden_at || $post->hidden_user_id == $actor->id)) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to keep the behavior close to the post-editing one for consistency so that's why that's there! I was wondering if it would be necessary or not.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not sure, let's keep it as is for now, then we'll see I guess unless other core devs have an opinion on is.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

I'd prefer to keep it. If a user can't participate in a discussion going forward, i don't think they should be able to participate retroactively either.

Copy link
Member

@SychO9 SychO9 left a comment

Choose a reason for hiding this comment

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

LGTM!

@SychO9 SychO9 requested a review from a team April 10, 2023 20:03
Copy link
Sponsor Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Looks good to me, left a few comments though.

: app.translator.trans('core.admin.permissions_controls.allow_indefinitely_button'),
key: 'allow_hide_own_posts',
options: [
{ value: '-1', label: app.translator.trans('core.admin.permissions_controls.allow_indefinitely_button') },
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Perhaps this should be extracted into a util? Although probably not, if we restructure the permissions system during 2.0, that would become redundant.

Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we will end up restructuring it in 2.0, we need some brainstorming .

@SychO9 SychO9 merged commit 818a100 into main Apr 17, 2023
@SychO9 SychO9 deleted the tk/delete-own-post-perm branch April 17, 2023 07:53
@github-actions github-actions bot mentioned this pull request May 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants