flarum / framework

Simple forum software for building great communities.
http://flarum.org/
6.24k stars 828 forks source link

fix(1.x,approval): correct `PostWasApproved` event trigger condition #3925

Closed rafaucau closed 8 months ago

rafaucau commented 8 months ago

Fixes #3921

Changes proposed in this pull request:

Ensure the PostWasApproved event is only raised for posts that are not hidden and introduce the PostWasUnapproved event to handle scenarios where a post is unapproved, particularly when it's the first post and the discussion needs to be hidden as a result.

QA

Necessity

Confirmed

rafaucau commented 8 months ago

I have noticed a problem (not introduced in this PR) with hiding discussions. When a post is hidden, the discussion is left empty. I will add a fix to this PR.

rafaucau commented 8 months ago

When the first post is hidden and the discussion is empty, the entire discussion is hidden away from other users so I don't think we need to change anything about it.

I thought everyone could see the discussion without any post:

https://github.com/flarum/framework/assets/25438601/d9ab6552-c616-4024-963b-eaf6accb0494

Now I see that other users except the admin and the author don't see it.


The PostWasUnpproved even is identical to the Hidden event in terms of when it is triggered so a bit redundant.

Maybe so, but it is more specific and can be useful in some cases such as here: https://github.com/flarum/framework/pull/3925/files#diff-dd866324feaa8087e4ee73330d8affaf8352fd52dde6ca007a837e49179957e7R27

SychO9 commented 8 months ago

I'm afraid it only seems to add more confusion, because the reason the PostWasUnapproved would get triggered is because isHidden was set true, which is what the Hidden event is for.

Also hiding != unapproving thought the two are already confusing right now

rafaucau commented 8 months ago

hiding != unapproving but unapproving = hiding so it's more specific and can be useful in cases where someone wants to listen for a post unapproval event only

SychO9 commented 8 months ago

there is no unapproving. You can only change is_approved to true. The endpoint will not change it to false even with isApproved: false. Triggering PostWasUnapproved when the state change is from hidden=false;approved=any to hidden=true;approved=unchanged is misleading. It only means what you actually intend to use is the Hidden event

rafaucau commented 8 months ago

I have reverted the commit that added the PostWasUnapproved event and incorporated your changes.

rafaucau commented 8 months ago

@SychO9 Can this be merged and released as v1.8.4? By sending real spam as false-positive it messes up Akismet's spam detection a bit (#3921), so it's worth releasing.

SychO9 commented 8 months ago

@rafaucau that's the plan, I should be able to get around to it today