E-mails cannot be reported

Committed this patch to CVS HEAD (D7) and DRUPAL-6--1.

Let's follow-up with a test for the Drupal 7 version of the module. We can worry about a Drupal 6 test later on.

I'm very totally absolutely sorry - that was a wrong change. :-|

As of now, I'm a bit baffled and don't really know whether contact form mails are actually recorded. Working on tests, which we absolutely need here to be sure.

That makes a little more sense. Is the testbot still processing our nice patchies here?

err, more like this, I meant.

I really wonder what the point of testing patches against release tags is... that's exactly useful for... what?

#7: mollom-DRUPAL-6--1.mail-report.7.patch queued for re-testing.

oh, uhm, heh, the bot was right.

+++ tests/mollom.test	15 Jul 2010 20:30:48 -0000
@@ -1758,10 +1759,50 @@ class MollomContactFormTestCase extends 
+    $this->assertData('session', $data['session_id']);

1. Session ID is supposed to be the 3rd parameter of assertData(). The second parameter should be the entity ID.

2. When reviewing this patch I was confused by assertData(). I couldn't guess what this function does. Looking up the function declaration, I learned it was a Mollom only function. Maybe we should rename it to be a bit more self-explanatory. Maybe something like assertMollomSessionData()?

+++ tests/mollom.test	15 Jul 2010 20:30:48 -0000
@@ -1758,10 +1759,50 @@ class MollomContactFormTestCase extends 
+    if (preg_match('@http.+?/mollom/report/([^/]+)/([^\s]+)@', $email['body'], $matches)) {
+      $found = array(
+        'url' => $matches[0],
+        'entity' => $matches[1],
+        'session_id' => $matches[2],
+        'mail' => $email,
+      );
+    }
+    else {
+      $found = FALSE;
+    }
+    $this->assertTrue($found, t('Report to Mollom link was found in e-mail.'));
+    return $found;

It is not clear why we load all the data in an array (i.e. $found) when all we do is assertTrue()?

+++ mollom.module	15 Jul 2010 20:30:48 -0000
@@ -374,6 +379,11 @@ function mollom_data_load($entity, $id) 
+ * The special $entity type "session" may be used for mails and messages, which
+ * originate from form submissions protected by Mollom, and can be reported by
+ * anyone; $id is expected to be a Mollom session id instead of an entity id
+ * then.

@@ -1792,10 +1802,10 @@ function mollom_mail_alter(&$message) {
+    mollom_data_save('session', $GLOBALS['mollom_response']['session_id']);

+++ tests/mollom.test	15 Jul 2010 20:30:48 -0000
@@ -1758,10 +1759,50 @@ class MollomContactFormTestCase extends 
+    $this->assertData('session', $data['session_id']);

Actually, I hoped that the added documentation would have explained the first issue you're raising. :)

I understand that it's unfortunate that we can't store Mollom session data for mails as cleanly as for any other entity. However, since Drupal's mail system does not turn every mail that is sent into a real entity, we need to record every single that is sent. Therefore, users are only sending feedback on session ids, which technically do not map to any locally stored content.

We can surely rename to assertMollomData(), although I'm not too nervous here -- we're inside of a file called mollom.test after all.

+++ tests/mollom.test	15 Jul 2010 20:30:48 -0000
@@ -1758,10 +1759,50 @@ class MollomContactFormTestCase extends 
+    $data = $this->assertMailReportLink();
+    $this->assertData('session', $data['session_id']);
...
+    $this->drupalGet($data['url']);
...
+    if (preg_match('@http.+?/mollom/report/([^/]+)/([^\s]+)@', $email['body'], $matches)) {
+      $found = array(
+        'url' => $matches[0],
+        'entity' => $matches[1],
+        'session_id' => $matches[2],
+        'mail' => $email,
+      );

Slightly more than just testing for TRUE - we are actually using the 'session_id' and 'url' keys of the returned data.

However, we might additionally want to assert that "session" actually is the entity type used in the URL.

Powered by Dreditor.

I re-read the documentation and #1 is clear now.

Slightly more than just testing for TRUE - we are actually using the 'session_id' and 'url' keys of the returned data.

We're not really using those -- we're storing them in an array, but we aren't really using the array.

I suspect we're talking past each other... could you clarify "we aren't really using the array", please? :)

Aside from that, attached patch implements the additional changes from #12 and #13.

I don't understand why we populate the array when we simply do assertTrue(). assertTrue() doesn't investigate the array so it seems like the array population is unnecessary code.

Hm, I'm not able to follow you here - we are testing more than assertTrue() on the returned array - individual array keys are used in subsequent code lines, and by changing the function's name and moving the assertions out of it, I silently hoped to clarify this a bit more.

D'oh. You're right. Slaps head. Committed to DRUPAL-6--1. Thanks sun. :)

Straight port against HEAD, applied almost cleanly. Let's see what the bot thinks.

#19: mollom-HEAD.mail-report.19.patch queued for re-testing.

Strange, that patch somehow passes for me locally.

Odd. Passes for me on two different systems... (?) Anyone any idea?

I tested this patch and it worked on my local machine. Committed it to CVS HEAD.

Can we revert that commit, please? As usual, the testbot identified that mollom-HEAD does not pass, so it does not test any patches against it.

I badly need the testbot running to work on other patches; running tests manually simply takes too long.

I re-ran the tests on my local hosts and things are still fine. Puzzled.

I temporary rolled back this patch to see if that helps.

#19: mollom-HEAD.mail-report.19.patch queued for re-testing.

Clean URLs are disabled on testbots. Therefore, URL pattern matching failed in this test.

Attached patch should and will pass. But please wait for green anyway ;)

Looks good and the tests pass. Committed to CVS HEAD. The D6 patch seems still valid so just marking this 'fixed'.

Great job figuring this one out, sun.

Unfortunately, DRUPAL-6--1 is broken, too: http://qa.drupal.org/pifr/test/26854

Committed to CVS HEAD. Thanks.