Is there any reason we're not availing of the whitelist function -- http://mollom.com/api#whitelist -- of Mollom API?

CommentFileSizeAuthor
#3 implement-whitelists-2168195-3.patch13.26 KBglekli
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

eshta’s picture

I'm not up on the historical knowledge, but from speaking with the Mollom engineering team, there isn't a reason not to from the service point of view. My hunch is that this feature was originally omitted to try to keep things simpler to set up and configure. Also, there may be some confusion since the white list api only applies to author information (not post content).

damondt’s picture

I would like to see this happen.

glekli’s picture

Status: Active » Needs review
FileSize
13.26 KB

I'm attaching a patch that implements a whitelist configuration page. Please let me know your thoughts on it.

eshta’s picture

Thanks for this! just wanted to give the heads up that this is not unnoticed. I'm hoping to review in the next few days.

eshta’s picture

Status: Needs review » Needs work

Again - so happy that someone kick-started the work here. As you picked up - it's very similar to the black-list functionality. I'd like to see a few things generalized a bit more so that there isn't so much duplication. See below.

  1. +++ b/mollom.admin.inc
    @@ -608,6 +608,173 @@ function mollom_admin_unprotect_form_submit($form, &$form_state) {
    +  $form['whitelist'] = array();
    

    We should add some instructions here. For example from the API docs: "Whitelist entries are checked first. On a positive whitelist match, no other checks are performed." It should be clear that if a term matches the whitelist, the blacklist won't be checked at all.

  2. +++ b/mollom.admin.inc
    @@ -608,6 +608,173 @@ function mollom_admin_unprotect_form_submit($form, &$form_state) {
    +        '#class' => 'mollom-whitelist-context value-' . check_plain($entry['context']),
    

    I don't think this does anything, does it? It's only used for automatic filtering on the blacklist in specific cases.

  3. +++ b/mollom.admin.inc
    @@ -748,6 +915,45 @@ function mollom_admin_blacklist_form_submit($form, &$form_state) {
    + * Formats the whitelist form as table to embed the form.
    

    Could this just re-use the existing theme function that the black-list uses? The name/parameters can be generalized.

  4. +++ b/mollom.module
    @@ -1768,6 +1787,10 @@ function mollom_element_info() {
    +    'mollom_admin_whitelist_form' => array(
    

    See above - no need for a duplicate of the same theming.

After all that - we'll need some tests too. Would be similar to the blacklist tests. This is awesome!