It seems like there is a logical flaw somewhere - let's improve our tests and fix it.

P.S.: Sorry, I had to run this test so many times today so that I had to make it run faster.

CommentFileSizeAuthor
#32 mollom-DRUPAL-6--1.sync-checkcontent.32.patch53.81 KBsun
#29 mollom-DRUPAL-6--1.sync-checkcontent.29.patch53.9 KBsun
#28 mollom-DRUPAL-6--1.sync-checkcontent.28.patch53.91 KBsun
#26 mollom-HEAD.sync_.26.patch11.56 KBsun
#26 mollom-DRUPAL-6--1.sync-checkcontent.26.patch53.74 KBsun
#23 mollom-HEAD.profanity.23.patch21.32 KBsun
#21 mollom-HEAD.profanity.21.patch21.66 KBsun
#20 mollom-HEAD.profanity.20.patch16.46 KBsun
#18 mollom-HEAD.profanity.18.patch17.2 KBsun
#17 mollom-HEAD.profanity.17.patch16.32 KBsun
#16 mollom-HEAD.profanity.16.patch14.66 KBsun
#13 mollom-HEAD.profanity.12.patch15.12 KBsun
#11 mollom-HEAD.profanity.11.patch13.11 KBsun
#10 mollom-HEAD.profanity.10.patch18.13 KBsun
#9 mollom-HEAD.profanity.8.patch17.39 KBsun
#7 mollom-HEAD.profanity.7.patch15.62 KBsun
#6 mollom-HEAD.profanity.6.patch16.52 KBsun
#4 mollom-HEAD.profanity.4.patch15.66 KBsun
#2 mollom-HEAD.profanity.2.patch13.33 KBsun
mollom-HEAD.profanity.0.patch8.09 KBsun

Comments

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.profanity.0.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.profanity.2.patch13.33 KB

Still tests only.

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.profanity.2.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.profanity.4.patch15.66 KB

Plus new checkContent sequence server responses assertions.

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.profanity.4.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.profanity.6.patch16.52 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.profanity.7.patch15.62 KB

d'oh - stale debugging code.

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.profanity.7.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.profanity.8.patch17.39 KB
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.profanity.10.patch18.13 KB

Now with the actually game changing changes.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.profanity.11.patch13.11 KB

Still contains unrelated changes, but less.

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.profanity.11.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.profanity.12.patch15.12 KB

This one hopefully fixes all other tests.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Problem: I've tested manually, and in testing mode,

1. I can post a comment containing "unsure", hit Preview
2. get a CAPTCHA and enter "incorrect" there, and at the same time, change the comment into "ham", and hit Preview again
3. I no longer get a CAPTCHA and the logs contain a HAM response from checkContent

Status: Needs review » Needs work

The last submitted patch, mollom-HEAD.profanity.12.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new mollom-HEAD.profanity.16.patch14.66 KB 
+++ mollom.module	11 Sep 2010 03:27:18 -0000
@@ -1277,7 +1276,7 @@ function mollom_validate_analysis(&$form
   if (isset($result['spam'])) {
     switch ($result['spam']) {
       case MOLLOM_ANALYSIS_HAM:
-        $form_state['mollom']['require_captcha'] = FALSE;
+        #$form_state['mollom']['require_captcha'] = FALSE;

Now without that commented out line.

Powered by Dreditor.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.profanity.17.patch16.32 KB

All tests are passing locally for me.

@todo:
- Documentation badly needs to be updated in various locations.
- Double-check whether we really need the added tests/assertions.
- Find a proper name for the added tests.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.profanity.18.patch17.2 KB

Reverted the testing profile change.

dries’s picture

dries commented

In addition to your TODOs:

+++ tests/mollom.test	11 Sep 2010 14:55:56 -0000
@@ -1437,6 +1525,68 @@ class MollomProfanityTestCase extends Mo
+  function testExaminer() {

I'd give this a better name.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.profanity.20.patch16.46 KB

ok, I get it now -- not sure how and why tests passed for me locally, but obviously, $this->disableDefaultSetup() does (also) not install Mollom module, so I'm really not sure how tests were able to call mollom() at all. So hopefully, this one will pass.

Working on the other todos now.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom-HEAD.profanity.21.patch21.66 KB

Please wait for the bot to come back green. Since it doesn't seem to report back to d.o currently, you need to manually click "View details".

dries’s picture

dries commented 
+++ mollom.module	11 Sep 2010 20:23:45 -0000
@@ -1268,12 +1272,25 @@ function mollom_validate_analysis(&$form
+  // invocations for a single user/post session. When a content is re-checked

"When a content" should be "When content".

+  // @todo Revisit this behavior and figure out whether the backend could still
+  //   return UNSURE, resp. the actual spam check result, even after solving a
+  //   CAPTCHA correctly, so that 1) posts passing a CAPTCHA cannot be turned
+  //   into spam, and 2) we are able to store the actual spam check result
+  //   locally.

This can probably be removed?

+++ mollom.module	11 Sep 2010 20:23:45 -0000
@@ -1268,12 +1272,25 @@ function mollom_validate_analysis(&$form
+  // are submitted (which e.g. can change during previews). Only in case the
+  // spam check led to a MOLLOM_ANALYSIS_UNSURE result, and the user solved the
+  // CAPTCHA correctly, subsequent spam check results will be
+  // MOLLOM_ANALYSIS_HAM.

This is not guaranteed. Maybe instead of 'will be' we should write 'may be' or 'are likely to be'?

+++ mollom.module	11 Sep 2010 20:23:45 -0000
@@ -1318,22 +1342,29 @@ function mollom_validate_analysis(&$form
+  // CAPTCHA validation may only be skipped, if we do not require it in the
+  // first place, or if the user already solved a CAPTCHA correctly. We need to
+  // validate, if $form_state['mollom']['require_captcha'] is TRUE, which is
+  // either set during initialization of $form_state['mollom'] in
+  // mollom_process_form(), or after performing a text analysis with a
+  // MOLLOM_ANALYSIS_UNSURE result. The second return condition,
+  // $form_state['mollom']['passed_captcha'], may only ever be set by this
+  // validation handler and must not be changed elsewhere.

This might need to be updated as well.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-HEAD.profanity.23.patch21.32 KB

Thanks! Incorporated your comments.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Great job, sun! Committed to CVS HEAD. Thanks a lot.

dries’s picture

dries commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Fixed » Patch (to be ported)
sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Patch (to be ported) » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.sync-checkcontent.26.patch53.74 KB
new mollom-HEAD.sync_.26.patch11.56 KB

Ok, next to backporting this change, I'm using this issue for a massive effort to additionally get both branches synchronized again.

For that sake, I'm attaching both a patch for HEAD and D6 - let's test HEAD first, commit, and afterwards re-test D6. Will manually run tests for D6 anyway.

Btw, we also have to test the update path for D6 since 6.13 before doing a new release. Will do that tomorrow.

Status: Needs review » Needs work

The last submitted patch, mollom-DRUPAL-6--1.sync-checkcontent.26.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new mollom-DRUPAL-6--1.sync-checkcontent.28.patch53.91 KB

Manual testing for D6 revealed some hiccups, fixed in this one.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new mollom-DRUPAL-6--1.sync-checkcontent.29.patch53.9 KB

Fixed the update path.

dries’s picture

dries commented

Powered by Dreditor.

+++ mollom.admin.inc	12 Sep 2010 21:07:26 -0000
@@ -125,34 +132,40 @@ function mollom_admin_configure_form(&$f
+          '#default_value' => $mollom_form['checks'],
+          // D7 only.
+          '#value' => array('spam' => 'spam'),
+          '#access' => FALSE,

A D7 comment in the D6 module?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Yes, as long as we do not backport the entire new profanity checking feature to D6, we are enforcing 'spam' as value for the text analysis 'checks'. It's not the only D7 note in the D6 module though.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new mollom-DRUPAL-6--1.sync-checkcontent.32.patch53.81 KB

Attached patch passes for me locally.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

  • Commit 8357baa on master, fai6, 8.x-2.x, fbajs, actions by Dries: 
    - Patch #907016 by sun: profanity checking does not always block profane...
  • Commit 9a19a6b on master, fai6, 8.x-2.x, fbajs, actions by Dries:
    - Patch #907016 by sun: profanity checking does not always block profane...

  • Commit 8357baa on master, fai6, 8.x-2.x, fbajs, actions by Dries: 
    - Patch #907016 by sun: profanity checking does not always block profane...
  • Commit 9a19a6b on master, fai6, 8.x-2.x, fbajs, actions by Dries:
    - Patch #907016 by sun: profanity checking does not always block profane...