Implement UI for blacklist functionality

Interested in how this UI will evolve. One of the first D7 modules I can see with such a page :)

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+function mollom_admin_blacklist() {
...
+function mollom_admin_blacklist_text_form() {

Missing PHPDoc.

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+  
...
+      
...
+    
...
+      

(and elsewhere) Trailing white-space.

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+
+  $form['text'] = array(
...
+  
+  $result = mollom('mollom.addBlacklistText', array(

No leading white-space.

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+        'spam' => t('Spam'),
+        'profanity' => t('Profanity'),
+        'unwanted' => t('Unwanted'),
+      ),

Wrong indentation.

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+  $form['submit'] = array(

All form buttons/actions live in a dedicated #type = 'container' with class 'form-actions' in D7. The form structure should look this way:

$form['actions'] = array(...);
$form['actions']['submit'] = array('#type' => 'submit',...);

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+  drupal_set_message(t('The word has been added to the blacklist.'));

We should check that the request went actually well.

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+function theme_mollom_admin_blacklist_text_form($variables) {
...
+  $blacklist = mollom('mollom.listBlacklistText');

Why is this data generated in a theme function?

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+      check_plain($entry['text']),
+      check_plain($entry['match']),

We should move 'match' before 'text' (also in the form constructor).

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+      // @todo the text should be encoded, or we should pass in an ID ...
+      l(t('delete'), 'admin/config/content/mollom/delete/' . $entry['text'])

l() runs check_plain() on the resulting URL: http://api.drupal.org/api/function/l/7

However, if we have a proper, unique ID, then we should definitely use it. But I guess we don't?

+++ mollom.admin.inc	24 Jan 2010 22:21:02 -0000
@@ -266,6 +266,177 @@
+  // @todo: not necessary?
+  $form['#submit'][] = 'mollom_admin_blacklist_url_form_submit';

Nope.

On the actual UI:

Powered by Dreditor.

Could you add a new screenshot? I want to give it a review.

I would actually propose using the "Add new text" or "Add new URL" row inside the table, simliar too admin/structure/types/manage/article/fields (without the drag & drop toggle ofcourse).

This because on the contrary to the roles table, this list can get somewhat longer. The pattern used on the roles administration is optimized for 3/5 roles, and is actually a somewhat confusing pattern in core (but we didn't get around to fixing it).

More reasons to hate me. :P

Even more + backport.

+++ mollom.module	1 Feb 2010 08:36:05 -0000
@@ -171,6 +171,24 @@
+  $items['admin/config/content/mollom/blacklist/delete'] = array(
...
+    'page arguments' => array('mollom_admin_blacklist_delete', 6, 7),

Re-opening, because we have to append /%/% here. Otherwise, the menu callback is valid and will be invoked even when the two required additional arguments are not present.

Powered by Dreditor.

+++ mollom.admin.inc	1 Feb 2010 08:36:05 -0000
@@ -267,6 +267,233 @@
+  $form['reason'] = array(
+    '#type' => 'select',
+    '#options' => array(
+      'spam' => t('Spam'),
+      'profanity' => t('Profanity'),
+      'unwanted' => t('Unwanted'),
+    ),
+    '#default_value' => 'spam',
+    '#required' => TRUE,
+  );

Additionally, one reason is missing here: "low-quality" - intended?

Powered by Dreditor.

Here is a proposal for a new blacklist UI implementation using a different and easier backend API (which is not live yet). It also has a more elaborate help text, but it probably is totally not in the right place yet, but I'm not a Drupal hacker :p

This patch is against the HEAD of D6. All comments are welcome.

+++ mollom.admin.inc	26 Feb 2010 19:19:33 -0000
@@ -259,35 +259,36 @@ function mollom_admin_unprotect_form_sub
 function mollom_admin_blacklist() {
-  $output  = '<h2>' . t('Text blacklist') . '</h2>';
-  $output .= drupal_get_form('mollom_admin_blacklist_text_form');
-  $output .= '<h2>' . t('URL blacklist') . '</h2>';
-  $output .= drupal_get_form('mollom_admin_blacklist_url_form');
+  $output  = '<h2>' . t('Blacklist') . '</h2>';
+  $output .= '<p>' . t("Mollom's filters are trained and shared globally over all participating sites. Due to this, some content might still be accepted to your site which you do not desire, even after giving feedback to Mollom. Using the site-specific blacklist, your are able to customize the filters to your specific needs.");
+  $output .= drupal_get_form('mollom_admin_blacklist_form');
+  $output .= '<div class="description"><p>'. t("The blacklist can be applied to specific contexts: it can look in all the content, or only in the links. When analysing links, Mollom will both look in the URL and the link text. You can also define if the blacklisted item is see as spam, profanity or unwanted content. We provided a few example blacklist items to demonstrate its usage.");
+  $output .= '<p>' . t("If you specify a blacklisted item as multiple words, Mollom will try to match it to several combinations. When you for example add the blacklist item \"replica watches\" for links, all the following URLs will be blocked:");
+  $output .= '<ul><li>http://replica-watches.com</li><li>http://replicawatches.com</li><li>http://replica-watches.com</li><li><a href=\".\">replica watches</a></li></ul>';
+  $output .= '<p>' . t("It is also possible to block whole URL domains by defining links blacklists as \"http://replicawatches.com\". This will block all possible subdomains and paths of this base URL. <p>Note that Mollom's global filters are also improved when multiple people put items in their blacklist. <em>This way you can help become a better spam and profanity fighter.</em>") . '</div>'; 
   return $output;

Would be good to put the supportive help text above the form.

+++ mollom.admin.inc	26 Feb 2010 19:19:33 -0000
@@ -363,88 +364,19 @@ function theme_mollom_admin_blacklist_te
-function mollom_admin_blacklist_delete(&$form_state, $blacklist, $key) {
...
+function mollom_admin_blacklist_delete(&$form_state, $key) {

+++ mollom.module	26 Feb 2010 19:19:34 -0000
@@ -214,7 +210,7 @@ function mollom_menu() {
   $items['admin/settings/mollom/blacklist/delete'] = array(
...
-    'page arguments' => array('mollom_admin_blacklist_delete', 5, 6),
+    'page arguments' => array('mollom_admin_blacklist_delete', 5, 6, 7),

Those numbers refer to path arguments in the URL, counting from 0.

You now added one, but the page callback (which is a form builder here, always taking &$form_state by default), only gets one argument (instead of 3).

+++ mollom.admin.inc	26 Feb 2010 19:19:33 -0000
@@ -455,12 +387,7 @@ function mollom_admin_blacklist_delete(&
+  $result = mollom('mollom.removeBlacklist', array('context' => $form['#mollom-blacklist-context'], 'text' => $form['#mollom-blacklist-text'], 'reason' => $form['#mollom-blacklist-reason']));

Do we really need to specify 'reason' here to delete an entry?

+++ mollom.module	26 Feb 2010 19:19:34 -0000
@@ -205,7 +201,7 @@ function mollom_menu() {
+    'description' => 'Configure blacklists.',

With this patch, I think it's singular.

Powered by Dreditor.

Here is a patch which solves Sun's comments.

Hm - this is for D6, right? Let's see what the testbot has to say.

+++ mollom.admin.inc	27 Feb 2010 09:18:50 -0000
@@ -259,35 +259,36 @@ function mollom_admin_unprotect_form_sub
 /**
  * Menu callback; renders the blacklist overview page.
- *
- * @todo Do we want to keep both blacklist on the same page?
  */
 function mollom_admin_blacklist() {
-  $output  = '<h2>' . t('Text blacklist') . '</h2>';
-  $output .= drupal_get_form('mollom_admin_blacklist_text_form');
-  $output .= '<h2>' . t('URL blacklist') . '</h2>';
-  $output .= drupal_get_form('mollom_admin_blacklist_url_form');
+  $output  = '<h2>' . t('Blacklist') . '</h2>';
+  $output .= '<p>' . t("Mollom's filters are trained and shared globally over all participating sites. Due to this, some content might still be accepted to your site which you do not desire, even after giving feedback to Mollom. Using the site-specific blacklist, your are able to customize the filters to your specific needs.");
+  $output .= '<div class="description"><p>'. t("The blacklist can be applied to specific contexts: it can look in all the content, or only in the links. When analysing links, Mollom will both look in the URL and the link text. You can also define if the blacklisted item is see as spam, profanity or unwanted content. We provided a few example blacklist items to demonstrate its usage.");
+  $output .= '<p>' . t("If you specify a blacklisted item as multiple words, Mollom will try to match it to several combinations. When you for example add the blacklist item \"replica watches\" for links, all the following URLs will be blocked:");
+  $output .= '<ul><li>http://replica-watches.com</li><li>http://replicawatches.com</li><li>http://replica-watches.com</li><li><a href=\".\">replica watches</a></li></ul>';
+  $output .= '<p>' . t("It is also possible to block whole URL domains by defining links blacklists as \"http://replicawatches.com\". This will block all possible subdomains and paths of this base URL. <p>Note that Mollom's global filters are also improved when multiple people put items in their blacklist. <em>This way you can help become a better spam and profanity fighter.</em>") . '</div>'; 
+  $output .= drupal_get_form('mollom_admin_blacklist_form');
   return $output;
 }
...
+function mollom_admin_blacklist_form() {

+++ mollom.module	27 Feb 2010 09:18:51 -0000
@@ -96,10 +96,6 @@ function mollom_help($path, $arg) {
-  if ($path == 'admin/settings/mollom/blacklist') {
-    return t('Mollom automatically blocks unwanted content, and learns from all participating sites to make its filters better. On top of automatic spam blocking, you can provide custom blacklists.');
-  }
...
   if ($path == 'admin/help#mollom') {

@@ -205,7 +201,7 @@ function mollom_menu() {
   $items['admin/settings/mollom/blacklist'] = array(
     'title' => 'Blacklist',
...
     'page callback' => 'mollom_admin_blacklist',

I think that we want to move a short summary of this into mollom_help(), and the detailed explanation into the module help page (also defined in mollom_help()).

After doing so, this entire page callback can be removed, and mollom_menu() can directly invoke

'page callback' => 'drupal_get_form',
'page arguments' => array('mollom_admin_blacklist_form'),

+++ mollom.module	27 Feb 2010 09:18:51 -0000
@@ -214,7 +210,7 @@ function mollom_menu() {
   $items['admin/settings/mollom/blacklist/delete'] = array(
...
-    'page arguments' => array('mollom_admin_blacklist_delete', 5, 6),
+    'page arguments' => array('mollom_admin_blacklist_delete', 4, 5, 6),

As mentioned earlier in this issue (before this rewrite), we have to add 3 placeholder arguments to the menu item's path, i.e.

admin/settings/mollom/blacklist/delete/%/%/%
  0  /   1    /  2   /    3    /  4   /5/6/7

And when counting the arguments starting from 0, then the dynamic arguments are 5,6,7 ;)

+++ mollom.module	27 Feb 2010 09:18:51 -0000
@@ -906,14 +902,10 @@ function mollom_theme() {
-    ),
+    )

Please revert - multi-line arrays should always end with a comma in Drupal.

Powered by Dreditor.

As mentioned earlier in this issue (before this rewrite), we have to add 3 placeholder arguments to the menu item's path, i.e.

admin/settings/mollom/blacklist/delete/%/%/%
0 / 1 / 2 / 3 / 4 /5/6/7
And when counting the arguments starting from 0, then the dynamic arguments are 5,6,7 ;)

I did have 5,6,7 in the original patch and you had comments on it so I corrected it :p I'll revert it back.

Here is an updated version of the patch. The backend API is still a bit in flux, but this probably comes close to a final version.

1. The problem with _help is that it has all of the modules help, not only the blacklist specific. This is probably do to the global UI design, I have no idea how to change this.

2. "All" is also fine for me. "Entire post" has a bit of connotation that it only applies to the post body, and not to eg the username, email, ...

Revamped patch, form, and help texts. Looks RTBC to me.

Needs to be forward-ported after commit.

Straight port against HEAD. (untested, so please wait for green light)

+ tiny quick fix for D6.

Sorry, forgot that #value is #markup now, $form_state['input'] always exists now, and when trying to output a link with #type 'link', then one should also assign that #type ;)

The usual fail... #734634: session_id changes without reason during Server response tests