Add ability to block profane content

Sounds fun. :)

Proposed psuedo-code / algorithm / requirements:

For bad words in posts
    a. allow the words to be written
    b. upon attempt to post, validate the words in the comments by calling checkContent
    c. if a bad word does NOT show up in the comment, the comment posts normally (assuming it is not spam)
    d. if a bad word DOES show up in the comment, an inline message is posted in red
        i. "Your comment is blocked for inappropriate language."
    e. block the submission
    f. if the writer removes the inappropriate word, then submits again, allow submission

So all I think we'd need to do for this is add a 'mollom_analyze_profanity' global option in the settings tab:

[x] Check for profanity when running textual analysis.
(checkbox title open for debate on getting a simple one-liner)

And in the mollom_validate_analysis() add conditions to check for the profanity value of the API response if the 'mollom_analyze_profanity' is TRUE.

That sounds like a reasonable approach. Instead of making it a global option, we probably should make it a form-specific option on this per-form settings page. That would be preferred as that allows people to treat public facing posts (e.g. comments) different from internal facing posts (e.g. contact form submissions). I think it would also be easier to use as the setting would be 'in context' -- it would only show up when text analysis was chosen. Sounds like we might have a dependency on #785460: Users have a hard time understand how exactly the Mollom mode is set ...

Here's the simple approach with a global option. Still needs tests and work to make this a per-form option. But if someone needs this immediately, it's working.

This looks good but I'll wait to commit it until we have the per-form setting.

Not 100% sure on the threshold -- how does one know how to set it?

Alrighty, here's the revised patch for per-form options like profanity filtering. We can migrate everything over to this {mollom_form}.data field in a follow-up issue.

Yikes there was some nasty code in the MollomWebTestCase->addProtection that was redundant and wrong.

Huzzah, working patch.

Attaching some screenshots so we can get some feedback on the UI.

Looks pretty good to me.

The help text under the "Spam Protection Mode" feels a bit awkward. Perhaps it could read:

If both Text analysis and CAPTCHA backup are selected, the CAPTCHA will not be shown until the form has been submitted and Mollom's textual analysis determines the content is suspicious.

I don't get the impression I can make a considered choice here, the 0.1, 0.2, 0.3 etc. aren't that meaningful. How do I tell if something is 0.4 profane? How do I get a feel for that?

There could be an indicatie list (with examples?) of which profanity violation causes what amount of points.

More feedback: http://twitter.com/skwashd/statuses/14099394739

@mollom grammar issue "only see a CAPTCHA after", should be an 'until' there. i'd prefer a human made a call on profanity. that's just me.

These look pretty good. Like Yoroy, I have trouble with the profanity levels though. I think what this is saying is that if set to disabled, profanity added to the site will go unchecked - and anything higher will scrub more and more profanity If that's the case, the help text could be improved...

Leave disabled to ignore profanity added to the site. A value of 1 will scrub the most profanity.

Brainstormed with Ben some more and we recommend the following changes:

1. Drop the different values for the profanity filter and make it a binary choice (i.e. enabled or disabled). When profanity filtering is enabled, we should block everything with a profanity value of 0.5 or more. I think that should address the UI concerns about the profanity filtering.
2. It would be good to mention that CAPTCHA only mode will not send form data to Mollom.com (except for the IP address). This is a good "privacy mode" but as a result, we can't do profanity filtering either. There is no content to analyze for profanity. In other words: CAPTCHA-only mode is good (i) when a form might contain sensitive data and privacy is a concern or (ii) when a form might not have enough data to do proper text analysis (e.g. password request form). It would be nice if we could capture that in a very concise way. Why? It could help people understand why the profanity option disappears when CAPTCHA-only mode is selected.
3. It is important to know that Mollom's CAPTCHAs are "intelligent" or "smart". That is, we take into account IP reputation. It is a very important differentiator relative to other CAPTCHA modules and most people have no idea. See http://mollom.com/blog/mollom-captchas-are-intelligent for additional details. It would also be nice to capture that in a concise way.

Would love your thoughts and suggestions for points 1-3. Thanks!

Simplifying it down to just a checkbox with a #return_value of 0.5 as the default profanity blocking level makes a lot of sense.

Sounds like a good improvement. Item 3 seems like it wants to live in help vs in the UI.

One more quicky so we don't forget about it: the profanity detection is still in beta so we'd need to mark it as beta in the UI.

"Form
Basic page form"

I could guess what this means but this cloud use better labels, for example changing Form to Affects? Either way its a bit awkward in explaining clearly on what this setting has an influence, if Form can be different.

"If text analysis and CAPTCHAs backup is selected,*"

I think this whole sentence should either be rewritten to flow better or it should be considered for help text . Because unless you encountered this, I doubt users will understand.

@Dries #17

1. Sounds good.
2. I think upon selection it should give help text, what this selection will end-up showing? There seem to be more considerations(= more help text), so I would also argue that we should put it in help text and link too it.
3. Why display it? Its not going to be a selling point, they already installed it :P

New patch that incorporates some of my own and other people's feedback. Changes:

1. Replaced the profanity thresholds with a binary switch. This was the number one concern with this patch.
2. Removed the tests for the URL blacklist UI. The URL blacklist UI was removed recently but the tests weren't updated. Result: unrelated test failures.
3. Tweaked some of the labels.

Todos (in follow-up patches but tracking them here for now):

1. Review and tweak some of the help text. Consider mentioning that Mollom CAPTCHAs are smart.
2. It is currently not possible to use profanity blocking without spam blocking. Some people might want to use profanity blocking only?
3. When protecting a node type with text analysis or profanity blocking, the fields are not automatically checked. See screenshot in #11.
4. Backport this profanity patch to Drupal 6.

Screenshots please :) ?

The .1, .2 etc. does not make sense to me, these are tenths of what?

People understand integers, a scale from 1 or 0 to N.

0/1 is clean enough to say out loud to your grandmother, N is all of the words in George Carlin's monologue. Have enough granularity to make most people feel they can tweak it to their needs.

Just my thoughts, thanks for the new feature!

Screenshot! :)

Where two radios are used, a checkbox often works as well:

[] Use profanity blocking (beta)

(not a fan of enable/disable, though it's more consistent with Drupal lingo)

I tried with a checkbox but it doesn't look as pleasing visually. I decided to go ahead with the patch in #23 but we'll have to come back with some refinements anyway.

Todos (in follow-up patches but tracking them here for now):

1. Review and tweak some of the help text. Consider mentioning that Mollom CAPTCHAs are smart.
2. It is currently not possible to use profanity blocking without spam blocking. Some people might want to use profanity blocking only?
3. When protecting a node type with text analysis or profanity blocking, the fields are not automatically checked. See screenshot in #11.
4. Backport this profanity patch to Drupal 6.

@Dries the decision for this pattern in core wasn't made because it looks visually pleasing, but because it adds clarity. Sadly, this is one of the cases where we have to choose one or the other - where core chose for clarity.

Any progress with this issue?

Reviewed these changes in the past hours. I guess this should have rather gone into a new 2.x branch or similar, as it introduced an inconsistency in the module's form protection and configuration. Primarily, because the form protection mode (also the PHP constants) will no longer sufficiently describe whether a form will be protected by Mollom or not. As we are going to extend in this direction and allow for profanity blocking only, the UI, but also the configuration storage, will have to be revamped.

+++ mollom.admin.inc	23 May 2010 20:27:46 -0000
@@ -137,13 +137,27 @@
-        '#title' => t('Protection mode'),
+        '#title' => t('Spam blocking'),
...
+        '#title' => t('Profanity blocking (beta)'),
+        '#options' => array(t('Disabled'), t('Enabled')),

1) The new labels are a bit confusing. Since both end in "blocking", it suggests they are substitutes or independent to each other.

2) "(beta)": Let's not forget about translators. If we absolutely need this warning/suffix, then we should move it into a separate string.

3) Agreed with yoroy + Bojhan, profanity blocking should really be a checkbox.

+++ mollom.admin.inc	23 May 2010 20:27:46 -0000
@@ -137,13 +137,27 @@
+        '#states' => array(
+          'visible' => array(
+            'select[name="mollom[mode]"]' => array('value' => (string) MOLLOM_MODE_ANALYSIS),
+          ),
+        ),

#states should always use ":input" instead of special form element types ("select").

+++ mollom.install	23 May 2010 20:27:46 -0000
@@ -131,6 +131,12 @@
+      'data' => array(
+        'description' => "Serialized data containing the form's Mollom options.",

Why was a serialized anonymous data array chosen for storage here? I'm asking, because Drupal core is trying hard to move away from such data structures, which are hard to identify and almost impossible to lookup. {users}.data serving as popular example. Especially if this functionality is being "beta", we will very likely need to update the stored configurations at some point.

Powered by Dreditor.

Postponing on #785460: Users have a hard time understand how exactly the Mollom mode is set

Profanity checking. Further analysis and questions from me:

Can profanity checking lead to a CAPTCHA being displayed? Or does it have a spam/ham treshold only?

Does profanity checking work without fields to analyze? I guess not? (The already committed code assumes it does, but only because it's part of spam checking currently.) Alright, Dries clarified in #17.2 already. Also contains some information that would be good to add to Mollom's help page.

The workflow proposed in #2 contains a hidden expectation, which does not seem to be covered by tests currently: 1) User tries to post spam. 2) Mollom blocks. 3) User corrects post into ham. 4) Mollom re-analyzes and accepts the post. In #271570-11: Log contains multiple "ham" messages, but no comment exists, I just recently mentioned and assumed the opposite: that we would stop analyzing a post once a definite decision can be taken. And yet, that does not seem to be the case - consequently, also leading to multiple watchdog messages per post, and possibly diverging Mollom statistics (#849826: Statistics don't map to actual post counts). To be discussed in aforementioned issues, just wanted to note this while reading through the comments.

The overall configuration options get more complex on this form/page now. With the new options, users may have to jump back and forth to Mollom's help page to understand the implications of each configuration and protection option on this form. We should therefore consider to show at least short hints for each option. I'd suggest to put descriptions below every single radio button and checkbox -- core is equally doing this on forms concerning similarly complex topics and decisions.

As mentioned in #23 already, we want to auto-enable all fields to analyze, if any.

In #22, Bojhan interestingly questioned the entire output of the "Form" label in the configuration/protection form. While it would be possible to use the page title for this, I'm not sure whether that isn't too hidden, invisible, or away from the main interface. We should give this a try, as Bojhan's general suggestion/question makes totally sense.

--

Now, as already mentioned in #31, I'm still not entirely happy that we're doing all of this in the 1.x branch. While I'm cool with progressive enhancement and slowly extending features, this kind of change has a much larger impact in my point of view. I'm especially focusing on the next best task we will do: allowing to check for profanity only, without checking for spam.

User experience-wise,

1. it's quite a challenge to communicate this paradigm shift to users: Mollom can no longer be understood as simple spam filter, it's becoming a thing to analyze text and a user's reputation. Neither the drupal.org project page nor the module's help page are sufficiently explaining this, leading to the issue that
2. the module now allows users to protect forms against spam, but provides configuration options to NOT check against spam. "WTF?" ;)
3. users updating from the current 1.x are expecting the same interface as before; documentation and tutorials need to be updated, resp., split by point releases (< 1.13 or not).

Technically,

1. the MOLLOM_MODE_DISABLED (0), MOLLOM_MODE_CAPTCHA (1), MOLLOM_MODE_ANALYSIS (2) constants are no longer sufficient to describe the protection mode of a form. They most probably should be converted into bit flags, so we can bitwise-add MOLLOM_MODE_ANALYSIS_SPAM | MOLLOM_MODE_ANALYSIS_PROFANITY.
2. the MOLLOM_ANALYSIS_UNKNOWN (0), MOLLOM_ANALYSIS_HAM (1), MOLLOM_ANALYSIS_SPAM (2), MOLLOM_ANALYSIS_UNSURE (3) constants are no longer sufficient to describe the analysis result of a post. They should equally be converted to bit flags, so there can be a new MOLLOM_ANALYSIS_PROFANITY (8) flag. Or alternatively (and that's one of the open design questions), MOLLOM_ANALYSIS_SPAM may be replaced by MOLLOM_ANALYSIS_BAD instead, thus being a suitable result constant for more than a single filter.
3. the current code that controls/routes form validation options and actions in $form_state['mollom']['require_analysis'] needs to be restructured to account for two different kinds of analysis.

Feature-wise,

1. with the addition of optional spam checking, the module turns into an arbitrator for form validation options. There can be
   • CAPTCHA-only validation
   • textual analysis for profanity
   • textual analysis for spam
   • textual analysis for profanity and spam

   ...and potentially more in the future.

2. with the addition of #364575: Support alternative spam rejection methods than a CAPTCHA (e.g. unpublishing), we get into the territory of a form validation router + rules engine. You can take aforementioned list and build the mathematical product to the following list of actions that apply to either a good or a bad validation result:
   • accept the form submission
   • block the form submission
   • unpublish the resulting entity (i.e., put into moderation queue, if supported)
   • invoke a fallback validation and continue with its own rules (i.e., show a CAPTCHA)
3. effectively turning this module into a form registration/configuration -> additional form validation -> actions

The fact that it has been considered to label a UI element with "beta" here only manifests these considerations. It implies a next generation feature, which may not be safe to be used by everyone yet, and should therefore go into a new branch, so users can (pro-)actively decide whether they want to participate in the experiment or not.

Regardless of branch and release management decisions, here's a mockup of how I think this could work out -- for now:

Any feedback?

Just to let you know that I'm actively working on this.

looks good :) I mean, there isn't much there and I think its very clear. I might add Comment fields to analyze : - rather then just Fields to analyze but otherwise still good.

Thanks, Bojhan!

There one major issue with the current labels though... in case of a form validation error, the error message is slightly confusing...

;)

--

Holy cow. Doing radio buttons with descriptions is a royal pain, I can tell you...

Why doesn't it say; Atleast one choice is required: Analyze for

still somewhat messy, lets hope no one leaves it empty.

1) I wonder whether we really need those radio button descriptions... how about this:

Protection mode
( ) Text analysis, falling back to a CAPTCHA if unsure
( ) CAPTCHA only

Note that we will likely have to revamp this selection/field soon, when we implement configurable fallback methods.

2) I think we should use "proper" labels... how about this:

Text analysis checks
[ ] Spam
[ ] Profanity

Checked form fields
[ ] Subject
[ ] Message
[ ] ...

1) Yes, thats better.

2) No, I find the whole "checks" concepts confusing too. What is clear from the current labels (not to common though) that "what its doing" and "where its doing it" that's what you need to convey, having it like this is more on topic than actually descriptive.

Didn't change the UI according to #42 yet. But regardless of that, this patch slowly reaches the size I'm expecting.

The new profanity checking feature on the backend is not really returning results I'm expecting though. Unless the backend is corrected, tests will fail. However, I've added + changed the module tests (and fake Mollom server implementation) to properly account for the new options, so at least those tests should pass.

There's no way that anyone will ever remark "trailing white-space" on my patches.

I don't think that which checks to run should be a required set. It would be nice to be able to temporarily disable mollom checks without having to remove the entire form rule from mollom. If the checks really do become a bitfield, then this is an easy way to allow disabling of a form rule.

@rwohleb: Hm, while I can see your point, I think that would add unnecessary complexity and lead to a suboptimal interface containing a "hidden feature". If we really want to allow to temporarily disable ("unpublish") form protections, then we should add a proper 'status' column to the database. If I'm not mistaken, then exportables (#717874: Provide exportables for Mollom forms) would need such a flag anyway, so adding an enabled/disabled status might be merged into that patch.

That said, the protection mode is that status already; we even have a PHP constant MOLLOM_MODE_DISABLED, which is unused currently. Thus, there would just have to be another radio button for the existing "Protection mode" selection, which says "None" or "Disabled". Ideally, however, I'd still like to defer that to a follow-up issue, since those changes would have to touch different lines of code.

Um, originally planned to attach a new patch including the proposed UX suggestions, but unfortunately just stumbled over an really odd Form API validation bug.

For reference: #875722: Custom #required messages broken made me step through Form API the past hours, oh well. :(

New screenshot, proper error messages:

I think this looks good, I am trying to come up with a better word for "check" as it doesn't imply "what its doing" very well. But can't come up with any solution atm, either way not very critical - so good to go mark if needed.

+++ mollom.admin.inc	6 Aug 2010 14:26:40 -0000
@@ -222,41 +227,38 @@ function mollom_admin_configure_form($fo
-function mollom_admin_configure_form_after_build($form, &$form_state) {
...
+    // Make field checkboxes required, if protection mode is textual analysis.
+    // @see http://drupal.org/node/875722

As long as core does not support custom error messages for #required, our checkboxes elements will not have a required marker on the label when initially accessing the form. That's a Drupal core "bug" (known issue) and cannot/should not be fixed or worked around here.

+++ mollom.admin.inc	6 Aug 2010 14:26:40 -0000
@@ -222,41 +227,38 @@ function mollom_admin_configure_form($fo
+function mollom_admin_configure_form_next_submit($form, &$form_state) {
+  $form_id = $form_state['values']['mollom']['form_id'];
+  $form_state['redirect'] = $_GET['q'] . '/' . $form_id;

This simplification is awesome, especially for testing, but we should retain the existing code, as we'll need exactly that to inject #ajax at some point.

+++ mollom.install	5 Aug 2010 19:32:11 -0000
@@ -83,6 +83,12 @@ function mollom_schema() {
+      'checks' => array(
+        'description' => 'A list of checks to perform for textual analyis.',
+        'type' => 'text',

Only now I realize that I asked for a integer field storing bitmasked check options. That would require additional conversion from constants to corresponding strings and vice-versa. Not sure whether it would actually be better. Important is that we are able to quickly figure out which configured forms are enabled for a certain check, which is also possible with the serialized string list.

+++ mollom.module	6 Aug 2010 11:20:14 -0000
@@ -735,6 +735,7 @@ function mollom_form_info($form_id, $mod
+    'checks' => array(),

Bogus addition: 'checks' doesn't belong into mollom_form_info().

Powered by Dreditor.

This simplification is awesome, especially for testing, but we should retain the existing code, as we'll need exactly that to inject #ajax at some point.

Looking a bit closer into that, I'm no longer sure about this statement. Let's just change it for now. The lines are really moved only, so it's no big deal to copy them back when we need to after all.

--

With this patch, the only remaining task seems to be to get the backend/server responses aligned with the client's expectations.

I'm starting to rewrite more parts of the module than actually required for this issue, so we better get this patch in now.

#55: mollom-HEAD.profanity.55.patch queued for re-testing.

Discussed final server response changes (in testing mode) with Ben. This patch will pass when those minor changes have been applied.

I tested patch #55 (not patch #60). Haven't looked at the code yet, but the Javascript didn't work as expected:

+++ mollom.admin.inc	7 Aug 2010 13:18:25 -0000
@@ -121,55 +127,50 @@ function mollom_admin_configure_form($fo
+          '#title' => t('Analyze text for'),
...
+          '#states' => array(
+            'visible' => array(
+              ':input[name="mollom[mode]"]' => array('value' => (string) MOLLOM_MODE_ANALYSIS),
+            ),
+          ),
...
+          '#title' => t('Text fields to analyze'),
...
           '#states' => array(
+            'visible' => array(
+              ':input[name="mollom[mode]"]' => array('value' => (string) MOLLOM_MODE_ANALYSIS),
             ),
           ),

Strange. That's solely based on #states and works just fine for me:

Powered by Dreditor.

Not a problem with the Mollom module but a core problem:

The one thing that is incorrect about the UI is that the 'falling back to CAPTCHA when unsure' only applies to spam filtering, not to profanity filtering. The profanity filter never returns 'unsure'.

Maybe we should do the following instead:

Update: 'CAPTCHA only' should probably become 'CAPTCHA' in that case.

I'd be fine with deferring that potential-fallback-to-a-CAPTCHA explanation to a separate issue. Shortened the labels as you suggested.

Additionally we also need to clean up the {mollom} table columns. Originally wanted to suggest to simply store the entire server response in a serialized text column, but then recalled that we purposively store the response values in individual columns, so as to be able to filter and sort on those values at some point in the future.

Some additional clean-up.

+++ mollom.admin.inc	10 Aug 2010 12:17:27 -0000
@@ -121,55 +127,50 @@ function mollom_admin_configure_form($fo
+          '#title' => t('Analyze text for'),

@@ -193,13 +193,12 @@ function mollom_admin_configure_form($fo
+          '#title' => t('Text fields to analyze'),

I'm happy with the "Analyze text for" label, because it clarifies the relationship to the Text analysis protection mode selected above.

I'm not yet sure whether "Text" is needed in front of "fields to analyze". The idea was to communicate that only fields where users can enter text freely are of interest. Though perhaps we are repeating "text" too often in the labels now.

Powered by Dreditor.

Reviewed this patch and it looks good.

Quality has nothing to do with spam but it is probably OK to do what you did.

Just got this error in update.php:

Failed: PDOException: SQLSTATE[01000]: Warning: 1265 Data truncated for column 'quality' at row 2: ALTER TABLE {mollom} CHANGE `quality` `quality` FLOAT DEFAULT NULL; Array ( )  in db_change_field() (line 2861 of includes\database\database.inc).

Didn't get this error on my other testing site, so I'm not sure what's the issue, but will analyze.

I tested an upgrade path earlier, and didn't get that error message either.

I still have to re-review and re-test the upgrade path with live data, but for now, we urgently have to fix a major bug in the current text analysis validation.

Committed to CVS HEAD. Thanks sun.

I have double-checked the update path from 7.x-1.x-alpha1 today and was not able to reproduce the failures I mentioned in #68.

We still have to test the upgrade path from Drupal 6, but I'd rather defer that to a separate issue and until D7 is out of alpha.