Features/1349 improve comment moderation possibilities

[jensbrak]

First draft of the code trying to implement some of the features in #1349 .
Choices made:

• Adding an enum CommentStatus replacing the IsApproved property of Comment model
• Comments submitted are now in initial state CommentStatus.Pending only once until moderated by/from some part of the system. CommentStatus.Approved effectively replaces the IsApproved = true.
• Adding property StatusReason to CommentModel to hold an optional reason for status change. The framework does NOT set this value (I played around with it but felt it would complicate things with respect to localization)
• In the comment list, the Manager shows an icon under column Rejected ONLY if CommentStatus.Rejected is the state of the comment. In other words: comments with Approved off and Rejected off is pending (ie submitted but not reviewed). Comments with Approved on and Rejected off is as before - approved. Comments with Approved off and Rejected on has been toggled to rejected by user in the Manager interface or a module/hook.
• One working migration for SQLite

At a minimum - things to revise/left to consider:

• The use of Comment.StatusReason in comment list of Manager. Remove for now? Add a separate line as suggested?
• The migration; maybe limit length of StatusReason?
• The new Status property of Comment. Is this inconsistant with respect to ContentState in the Manager? Both with respect to naming and enum vs class. State vs Status seems to be used inconsistant already? And the use of a class in Manager for Page/Posts seems not the same as an enum within Core for Comments. Still a question that popped up for me.
• IsApproved is removed from data/ef and migration will convert it to Status for comments. It is kept in Comment class in core though but marked obsolete. Perhaps not force exceptions initially?

I've tested a migration with MySql as well and it worked ok after som tweaks. Also tested the feature with the SpamDetector module with success.

[tidyui]

Shouldn't the second parameter be set to false as you've added a fallback implementation? Setting the boolean to true will render compilation errors when using the property instead of giving a compiler warning.

[jensbrak]

I was not sure on the decision but noted it (things to revise/left to consider) since it made sense to set it to true.

[jensbrak]

Note that I removed the IsValid-property completely from Data layer, replaced by the Status. I considered to be more clean to replace it "under the hood" rather than having both properties present in DB until the old one is eventually removed.

[tidyui]

So I've checked out the branch and looked at the manager. My initial thought is that the three-way enum doesn't give that much extra in functionality. It works much like a bool? where Pending would be null, and Approved and Rejected is true/false.

Also by having a single field in the data-layer, manually approving a comment that has been marked as been rejected by a moderation plugin as spam effectively removes the historical information that it was rejected. Also, if the status message is set by the moderation plugin and not updated when being manually approved it will say different things to the user.

When we discussed having multiple fields I was thinking about maybe have multiple fields for approving and validating in the database as well, then a moderation plugin would set Approved=true and IsSpam=false for a valid comment, and Approved=false and IsSpam=true for a rejected comment.

However it would be fully possible for the manager to reject a comment that has passed moderation or the other way around without loosing history of how the moderation plugin worked. Only the approved column would be possible to set from the manager interface.

Then again, maybe this is overkill, I haven't worked that much with Wordpress and their comment moderation system to know what users expect from this kind of functionality :)

Note: The name IsSpam is just a suggestion, there's probably a better more generic term here.

[jensbrak]

@tidyui - very good points and I can confirm I've went through similar thoughts. :)

So I've checked out the branch and looked at the manager. My initial thought is that the three-way enum doesn't give that much extra in functionality. It works much like a bool? where Pending would be null, and Approved and Rejected is true/false.

Yes. My initial thought was to go for an additional bool to represent the rejection state. The enum does not add more - as I mentioned before we have three states with the rejection state - whether it is represented by IsApproved = null, true, false, State=Enum.Pending/Enum.Approved/Enum.Rejected or IsApproved=true/false + IsValidated=true/false.

In the latter case there's a fourth state that is illogical and that's the primary reason for going with the enum tbh. Felt more clear to have three possible values for the three possible states. What then became apparent is that the naming in the UI of the manager and in the code does make a good name and representation a little hard:

In current implementation (not the PR) we have no distinction between pending comments and comments rejected by a moderator or module. That's what I wanted to achieve to have. Looking at the comment filter in the Manager there's the term Pending but in the code there's use of Approve and UnApprove.

If history is desired, I'd say that there's an even more "overkill" solution that would allow for true history: to have a separate moderation table. I was considering this, since I felt the 'ValidationReasondid not really belong to theComment`. No matter how the comment state is represented (enum, one nullable bool or two bools etc) - the state transition could be held in a table, one for each change made. Such a table would hold the comment it belongs to, the state before and after, the source of the change and an additional reason for it.

Also by having a single field in the data-layer, manually approving a comment that has been marked as been rejected by a moderation plugin as spam effectively removes the historical information that it was rejected.

That's why I toyed with having the manager to set a reason (and implemented it - I removed it). The manager added a reason like Approved by manager (one could even consider adding what user did it, if we can get the HttpContext (I believe). And I also added the reason Approved by config in the Core code (PostService / PageService). The issue I was facing was that there's no good/easy solution for dealing with localization of messages withing core code. I mean, there are three places that this field would be updated from: within core, within manager and possibly from within a module.

Note: The name IsSpam is just a suggestion, there's probably a better more generic term here.

Yeah. That's why I tried Rejected and when I did the first implementation I used IsRejected rather than IsSpam. And that's when I also realized a reason for the rejection would make sense: it being spam or someone manually rejecting it.

Wordpress and their comment moderation system to know what users expect from this kind of functionality :)

I have to check, since I don't remember myself. I have a couple of WP installs I manage occationally for friends!

Steps forward/things to decide/discuss further (a suggestion):

1. One nullable bool, two bools or one enum?
2. Reason or no reason?
3. History with separate table or just by (re)implementing reason set by manager/core in the PR?
4. Other?

As for one, I actually favor the nullable bool. I mean, that's the most clean solution that most accurately represent what we have: a comment state is primarly not evaluated at all first. It's pending. Submitted. Not validated at all. Makes sense to have this a null as in "no validation has been made" (neither by config or by manual approval or module approval). At some stage validation is made and the only two reasonable states to go between is being approved or not.

I am fond of the idea of having a reason but I am not convinced that it should be part of the comment. I am also not sure how to solve the localization issue but maybe that's a no brainer?

[jensbrak]

This is how the comment moderation feature (built in) of WP looks like.

[tidyui]

So by the looks of it Wordpress has four distinct states? Pending, Approved, Unapproved and Spam.

[tidyui]

Or maybe it's just three states with an extra field marking the comment as "spam"?

[jensbrak]

So by the looks of it Wordpress has four distinct states? Pending, Approved, Unapproved and Spam.

Yeah. The fourth being "waste bin". Whether to call it Unapproved or Rejected or Trash :) I'll check what they call it in English, just for the fun of it.

I believe Trash can be emptied both explicit or by a setting after n days. Lemme check. Question is why they distinguish between spam and trash and what transitions/actions can be made on them.

[jensbrak]

All (45) | Mine (23) | Pending (0) | Approved (45) | Spam (0) | Trash (0)

From the looks of it it's
edit_comments.php?comment_status=all
edit_comments.php?comment_status=mine (For Pending)
edit_comments.php?comment_status=moderated
edit_comments.php?comment_status=spam
edit_comments.php?comment_status=trash

A quick look in the database of the WP install I get: that they actually store the comment status in one single column that is a varchar where pending seems to be 0, approved 1, spam is spam and trash is trash. Don't know if I find that very logical or neat.

In addition they have a meta data table for comments with akismet data and the moderation actions (somewhat like the one I proposed in my previous comment above). For instance, one can trace moderations like these:

  | Edit | Copy | Delete | 1595 | 331 | akismet_history | a:3:{s:4:"time";d:1615117587.5978720188140869140625;s:5:"event";s:11:"report-spam";s:4:"user";s:5:"admin";}
  | Edit | Copy | Delete | 1596 | 331 | akismet_user_result | true
  | Edit | Copy | Delete | 1597 | 331 | akismet_user | admin
  | Edit | Copy | Delete | 1598 | 331 | _wp_trash_meta_status | 1
  | Edit | Copy | Delete | 1599 | 331 | _wp_trash_meta_time | 1615117589
  | Edit | Copy | Delete | 1600 | 329 | akismet_history | a:3:{s:4:"time";d:1615117599.9898130893707275390625;s:5:"event";s:12:"status-trash";s:4:"user";s:5:"admin";}
  | Edit | Copy | Delete | 1601 | 329 | _wp_trash_meta_status | 1
  | Edit | Copy | Delete | 1602 | 329 | _wp_trash_meta_time | 1615117599

Here one can see I just put comment no 329 in trash.

[jensbrak]

@tidyui If you have any preferences let me know but I believe an update is rather quick to do as far as I'm concerned.

[tidyui]

@jensbrak I've started sketching on the next major release in the branch version/v10.0. One of the major redesigns here is that all of the content edit functionality in the manager (page, posts & generic content) will use the same edit model, vue-application and UI-components. This will enable much better reusability across these entities.

Also the services are built around the native CLR content-types and does not use the dynamic models which also means that we will finally be able to remove these in v10.0.

I'm thinking we should revisit this PR and start looking into merging it into the v10.0 branch. Here we should also create a new UI-components for the comments that can be rendered directly in the edit view of the application, instead of jumping to another page in the manager. Edit views will also feature a list of the available pages/posts/content to the left for moving between entities faster as discussed in #899.

The new functionality is not 100% complete atm, for example it's not possible to add new pages, posts or content through the manager yet, only editing of existing items. If you want you can always take a look at the current state of things in this branch and maybe send a PR towards that branch.

Regards

[jensbrak]

I have been thinking of revisiting this PR. Have been awaiting your response: to me it really doesn't matter how the functionality is implemented more precisely but your input is crucial since you have the best knowledge about what works best for Piranha in general.
Let's make this happen! I'm more than willing to do what it takes.

[tidyui]

So @jensbrak, should we start looking at this again for the 10.0 release or are you busy at work :)

[jensbrak]

So @jensbrak, should we start looking at this again for the 10.0 release or are you busy at work :)

Sounds like a plan ! I'll dig into it asap and will get back with either a new proposal or questions 😉

[jensbrak]

I'll go ahead and merge 10 branch into my fork and try to revise the code as discussed above. It shouldn't be that much hazzle I hope. Question is how the design should be but as for now I'll focus on getting up to speed with the 10 version here.

[tidyui]

@jensbrak Heads up! The branch version/v10.0 is old, stale, and only applicable to a specific feature that was worked on as you can see from the commit history! The current working branch for 10.0 is master!

[jensbrak]

@tidyui - As for the design choice of this I've come to change my mind. Your suggestion to have an additional field to indicate spam status seems to be a better choice. The obvious choice would be a bool IsSpam. Clean and simple and in line with what Akismet use (ther antispam service I've tried and use with my plugin). However, I know other SaaS for antispam use other means, for instance a score between 0.0 and 1.0 to indicate likelyhood of being spam. One could argue that a solution like this leaves more headroom when deciding what to do with the comment. I'd say, for the sake of simplicity, Piranha should only care for a "yes or no".

Another thing would be to add a text field (like I did in my first PR) to allow for adding details about moderation or status change. I didn't quite follow your reasoning about keeping history intact - part from it's a good idea. Do we want status changes in a separate table? Ie, add IsSpam to the Comment data and the rest (comment - or perhaps content id, status, date and explanation) in a new table?

Either way we do it, it seems to be rather straight forward - so it's more of a design preference on your part. The main thing here is that only two states seems inadequate for comment moderation. When you have the time, let me know your thoughts and I'll do a new PR reflecting our ideas and choices.