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

[rafaucau]

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

• 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

• Backend changes.
• Core developer confirmed locally this works as intended.
• Tests have been added, or are not appropriate here.

[rafaucau]

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.

[SychO9]

Thanks for the PR!

I have dived into the issue to understand the flow, and the main problem is that the PostWasApproved event is triggered always when hiding or restoring a post, regardless of actual approval status/action. Which does not make sense. If I am listening to the post approval even, I expect that there was a state change from approved to unapproved. But right now even if you hide any approved post, the event is triggered.

The changes here seem close but based on testing I feel this is closer to what we would want: bed386f

What do you think?

I don't understand the second commit though:

• 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.
• The PostWasUnpproved even is identical to the Hidden event in terms of when it is triggered so a bit redundant.

[rafaucau]

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:

2023-11-10.11-31-02.mp4

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]

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]

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]

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]

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

[rafaucau]

@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]

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