We're working on a patch to make AuthorContact compatible with Mollom. (#553424: Can I use Mollom for this modules Contact Forms?)
It's seems that the only way to add a 'Report as inappropriate'-link to the mails sent by AuthorContact is by copying the mollom_mail_alter function, rename it to authorcontact_mail_alter, and replace the $valid_ids array with an array containing the message id sent by AuthorContact.

Any suggestions for a more flexible way of defining which message id's are concerned?
Extending hook_mollom_form_info() ?

Comments

dries’s picture

I'd be OK with extending hook_mollom_form_info() with a 'mail id' => array('foo', 'bar') or 'mail id callback' => '_my_mail_ids' (or something similar).

A 'mail id' field would probably cover 90% of the use cases so I don't think we need a callback. The advantage of a callback is that it becomes possible to deal with dynamic mail ids, but I'm not sure that actually exists. In the cases a list of hard-coded ids is not sufficient, people could always fallback on a custom _mail_alter() function like we do now.

Thoughts?

dave reid’s picture

Title: A better way for other modules to add the Mollom-'Report'-link to mails sent » Provide 'mail ids' in hook_mollom_form_info()
Assigned: Unassigned » dave reid

Providing mail IDs in hook_mollom_form_info() makes a whole lotta sense instead of us hard-coding them. I also agree with just going with the simpler array of mail IDs rather than a callback. Like Dries says other modules can implement hook_mail_alter(). Patch to arrive shortly.

dave reid’s picture

Version: 6.x-1.x-dev » 7.x-1.x-dev
Status: Active » Needs review
StatusFileSize
new4.8 KB

Initial patch for Drupal 7.

Status: Needs review » Needs work

The last submitted patch, 791358-mollom-mail-ids-D7.patch, failed testing.

dries’s picture

This patch implements what I had in mind, and I think it makes sense to associate the mail ids with the hook_mollom_form_list().

dave reid’s picture

Status: Needs work » Needs review
StatusFileSize
new6.62 KB

Fixed the exceptions and tested locally.

dave reid’s picture

I think this one is ready.

pieterdc’s picture

I think it fits the need.

Ps: I've never seen such a quick feature request response. Wow!

dries’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs review » Patch (to be ported)

Committed to CVS HEAD. Thanks Dave.

1. Let's backport this patch to DRUPAL-6--1.

2. Would also be good to write some tests but I think we have an issue for testing the mail functionality.

dave reid’s picture

Status: Patch (to be ported) » Needs review
StatusFileSize
new6.59 KB

Re-rolled for D6, tested locally on my install and added the correct report links.

dave reid’s picture

StatusFileSize
new6.59 KB

I don't get it. Do I need to upload with out the '-D6' at the end of the patch file to get it to run for testing? :/

Status: Needs review » Needs work

The last submitted patch, 791358-mollom-mail-ids.patch, failed testing.

dries’s picture

The tests still have ...

* @todo Test mail sending with assertMail() now that it is available.

... so it might not be super useful to test this. I'll try to run the test on my local machine.

dries’s picture

Status: Needs work » Fixed

Tests passed locally so committed this to DRUPAL-6--1. Thanks Dave.

Status: Fixed » Closed (fixed)

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

  • Commit e07553b on master, fai6, 8.x-2.x, fbajs, actions by Dries:
    - Patch #791358 by Dave Reid: provide 'mail ids' in...

  • Commit e07553b on master, fai6, 8.x-2.x, fbajs, actions by Dries:
    - Patch #791358 by Dave Reid: provide 'mail ids' in...