Skip to:
Content

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#6719 closed enhancement (fixed)

Activity link moderation doesn't output useful error message to end users

Reported by: r-a-y Owned by: r-a-y
Milestone: 2.6 Priority: normal
Severity: normal Version: 1.6
Component: Core Keywords: has-patch commit
Cc:

Description

Reported on the buddypress.org forums:

In a BP Group, when posting an activity update, commenting on an activity update, or replying to an existing activity update, if the post contains more than the allowed number of links for WP comment moderation (as set in Admin >> Settings >> Discussion >> Comment Moderation >> “Hold a comment in the queue if it contains X links”) the post is rejected with an unhelpful error message: “There was an error posting your reply. Please try again.”

https://buddypress.org/support/topic/buddypress-groups-activity-posting-fails-based-on-wp-comment-moderation-setting/

I haven't verified the report, but this sounds about right.

The bare minimum we need to do is add a better error message.

This will require some form of WP_Error addition and handling, similar to what was done in #6535.

On the flip side, the most work that would need to be done is to introduce some form of moderation status:
https://buddypress.trac.wordpress.org/browser/tags/2.3.4/src/bp-activity/bp-activity-filters.php?marks=149-152#L136

But that's another ticket for another day :)

Attachments (2)

6719.01.patch (10.2 KB) - added by r-a-y 2 years ago.
Refreshed for trunk.
6719.02.patch (17.2 KB) - added by r-a-y 2 years ago.

Download all attachments as: .zip

Change History (31)

#1 @r-a-y
3 years ago

  • Owner set to r-a-y
  • Status changed from new to assigned

#2 @boonebgorges
2 years ago

Marked #6819 as a duplicate.

#3 @hnla
2 years ago

Related aspect in comment_max_links & bp $num_links bailing out if '>=' rather than '>' on ticket: #6821

#4 @r-a-y
2 years ago

  • Keywords has-patch dev-feedback added; needs-patch removed

01.patch is a first pass at generating better error messages when someone trips up the moderation settings in the activity stream.

There are still some tweaks that need to be made to the moderation check such as grabbing the "Settings > Discussion" moderation settings from the BP root blog, but it's getting late here :)

I'll add some unit tests a bit later on, but let me know what you think of the approach or whether we should just suspend the moderation feature as discussed in #6822.

#5 @r-a-y
2 years ago

  • Milestone changed from 2.5 to 2.6

@r-a-y
2 years ago

Refreshed for trunk.

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


2 years ago

#7 @hnla
2 years ago

@r-a-y
Tested refreshed patch. This is a vast improvement on what we had and addresses some of the concerns raised in #6822 vis a vis:

  • Add a status update, exceed links allowed, receive error message, however the status update is actually accepted and displayed with no actual moderation and we can see the update in user screens and members loops.

Now we have stronger messages highlighting the exact reason.

For links we halt and notify there are too many however now it's clearer why the error and we do NOT publish those links regardless as we were before, user does have the option to review those links and amend as we have retained the textarea data.

We correctly stop blacklisted words from being displayed, although we still do not add the comment to the WP moderation queue - this behaviour is better than posting the comment though.

For WP spam list words we do not trap those and do publish them although these are meant to be held too.

Posting a comment to a WP post from the act stream is still problematical we acknowledge an issue with the comment but not what i.e bad-word, spam-word, or too many links.

Having thrown up the error message and not posted the comment in the act stream we find that in visiting the post in question directly the comment has been posted bypassing the WP moderation queue.

I think we have a real issue here in providing a means to circumvent the WP process and user set blacklists, links etc.

The error message provision doesn't tackle that aspect although it does improve the BP status update behaviour and puts in place a proper WP_Error handling class?

imho I do think we still need to either pull moderation altogether until we can correct things or decide to tackle this fully but I would like my cake and eat it and add the patch here as it does start to put in place proper error handling?

This ticket was mentioned in Slack in #buddypress by hnla. View the logs.


2 years ago

#9 @r-a-y
2 years ago

@hnla - Thanks for testing!

Posting a comment to a WP post from the act stream is still problematical we acknowledge an issue with the comment but not what i.e bad-word, spam-word, or too many links.

I missed doing activity comment checks for moderation. 02.patch should address this.

The "Comment Moderation" section from the "Settings > Discussion" page should now be implemented for activity items.

In BuddyPress, when an activity update or comment matches any of these rules, we do not record an activity item into the database.

In WordPress, if a post comment matches any of these rules, WP does record the post comment, but sets the 'comment_status' DB field to 0.


For WP spam list words we do not trap those and do publish them although these are meant to be held too.

I noticed that I missed implementing the "Comment Blacklist" check from the "Settings > Discussion" admin page. I hope this is what you meant by WP spam list words, hnla!

However, this also presents its own set of problems. BuddyPress currently sets activity items that have a blacklisted word as activity spam (#4284). Since the activity item is technically recorded, the way I have set up activity error handling means I cannot throw up a custom error message.

To outline what WordPress does, they mark blacklisted comments to either 'trash' or 'spam' in the 'comment_status' DB field. I don't believe they add a user message if a comment hits the blacklist. Needs testing.

We could workaround this another way by doing spam checks after an activity item is recorded and then adding a custom error message that way. This adds another DB query though.

Or we can simply stop recording activity items that match a blacklisted word.


imho I do think we still need to either pull moderation altogether until we can correct things or decide to tackle this fully but I would like my cake and eat it and add the patch here as it does start to put in place proper error handling?

Activity moderation is a big task. In order to match WP functionality, we would need to introduce an 'activity_status' DB column in the activity DB table and rework some of our functions to handle recording this new flag. There are also admin UI considerations that would need to be done to the Activity admin dashboard, as well as other things like whether we should tie activity moderation to WP comment moderation settings?

See #2786.

I do agree that the error handling in this ticket is far and away better than what we currently have and improves the UX greatly. But whether we want to hold off on this for #2786 requires dev feedback.

@r-a-y
2 years ago

#10 @hnla
2 years ago

I noticed that I missed implementing the "Comment Blacklist"
hope this is what you meant by WP spam list words, hnla!

Sorry, yes I was confusing things with terminology, not sure why I used the word 'Spam', but WP provides for two status labels 'pending', and 'spam'

'Blacklist' == words prohibited and if found in comment trashed by WP and no message is shown to user - this is a silent action!.

'Comment Moderation' == kicks in on either exceeding a set number of links or inclusion of a proscribed work in moderation list and the offending comment is queued in the 'Pending' list ( not 'Spam'! .

We correctly trapped excessive links and words in the moderation panel ( previously my ref to 'spam list')

Yes missed the blacklist in this implementation (prior to .02 patch), blacklisted words that would cause WP to trash comment are published in the act stream.

However again crux of our issue with post comments remains that whether we publish act stream comment or trap it with error message we do pass them through to the post proper bypassing the WP moderation or blacklisting.

Activity moderation is a big task. In order to match WP functionality, we would need to introduce an 'activity_status'...

I began to realise this when I was looking at code base to see what could be done, in lieu of being able to tackle this we need to ensure if the moderation /blacklist check returns true we need to unlink the post sync.

Or we can simply stop recording activity items that match a blacklisted word.

Yes we need to do this for blacklist words, that will roughly equate to that which WP does (although we technically destruct data where WP retains it for admins to deal with in 'trash')

#11 @hnla
2 years ago

Testing the .02 patch:

Adding a comment to a post from the act stream with a blacklist word in results in:

The comment isn't trapped with error message and appears to be initially published to stream(i.e ajax wise before page refresh) subsequently comment is not published though!

Comment is seen later to be added to BP activity spam list in dashboard.

Comment however IS added to the WP post comment list.

Adding a comment with a moderated word to a post from the act stream results in:

BP trapping the comment with an error message.

NOT publishing the comment to the WP post ( good!).

NOT adding the comment to the BP activity spam list in dashboard.

Last edited 2 years ago by hnla (previous) (diff)

This ticket was mentioned in Slack in #buddypress by r-a-y. View the logs.


2 years ago

#13 @boonebgorges
2 years ago

I feel the same way about this approach as I felt in https://buddypress.trac.wordpress.org/ticket/6535#comment:4 - it stinks that an error_type param is the only way to ensure backward compatibility, but so be it.

I have not tested all of @hnla's scenarios, but the general strategy looks good to me.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

#15 @hnla
2 years ago

Lets commit what we have for 2.6!

But make a mental note that we still need to look at issues where we may push comments through to a post that WP settings wouldn't allow!

#16 @hnla
2 years ago

  • Keywords commit added; dev-feedback removed

#17 @r-a-y
2 years ago

Adding a comment to a post from the act stream with a blacklist word in results in:
Comment is seen later to be added to BP activity spam list in dashboard.
Comment however IS added to the WP post comment list.

I've created #7107 for this specific issue.

hnla and I were discussing if the "Comment Blacklist" option should just block activity items from being recorded instead of marking them as spam as we do now. FWIW, for "Comment Moderation", we currently just block the activity items from being recorded.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

#19 @r-a-y
2 years ago

In 10858:

Activity: Introduce error handling to the BP_Activity_Activity class.

To support frontend user messages for activity failures, we need the
ability to add errors.

This commit:

  • Introduces the $errors object and $error_type property to the BP_Activity_Activity class.
  • Adds an 'error_type' parameter to all applicable, activity functions.
  • Modifies bp-legacy to output an error message for activity failures.

See #6719.

#20 @r-a-y
2 years ago

In 10859:

Moderation: Add WP error handling.

bp_core_check_for_moderation() and bp_core_check_for_blacklist() now
supports returning WP errors.

See #6719.

This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.


2 years ago

#22 @r-a-y
2 years ago

In 10860:

Activity: Set WP error if activity content hits a moderation filter.

One important change we are making here is if the activity content matches
a word from the "Comment Blacklist" discussion option from WordPress, we
now stop recording the activity item.

Previously, we were still recording the activity item, but marking it as
spam.

See #6719.

#23 @r-a-y
2 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

Okay, going to mark this one as fixed.

Thanks for the feedback everyone!

#24 @r-a-y
2 years ago

In 10869:

Activity: Bail out of activity post functions if an error occurred.

See #6719.

#25 @mercime
2 years ago

@r-a-y Might I suggest changing the word inputted to posted in the following error messages:

  • 'You have inputted too many links' to 'You have posted too many links.'
  • 'You have inputted an inappropriate word.' to 'You have posted an inappropriate word.'

Thanks for your work on this.

#26 @r-a-y
2 years ago

Sounds good, @mercime!

Will make those changes early next week. Also gives others a chance to chime in before I get to this.

Last edited 2 years ago by r-a-y (previous) (diff)

#27 @dcavins
2 years ago

I prefer the strings suggested by @mercime. Even though 'inputted' is not technically incorrect, it sounds funny when I say it out loud. (Related: I've never putted something in my toolbox. You wonder where the words diverged, ha ha.)

#28 @r-a-y
2 years ago

In 10881:

Moderation: Change 'input' to 'posted' for error message strings.

Props mercime.

See #6719.

#29 @DJPaul
2 years ago

  • Component changed from General - UX/UI to Core
Note: See TracTickets for help on using tickets.