Add an in-module developer mode toggle

Are you logged in when you're posting the test spam? Possibly the Mollom service has already identified your account as a trusted and the developer mode doesn't ignore that. I don't know for sure since I don't have the access to check that, but it might be something to look into.

I'm not sure. Hopefully either Dries or Ben can check in more detail on the server side for you. You do have the 'textual analysis with captcha backup' enabled for your forms and not just the 'captcha' protection right?

The developer mode has gone. You can use Mollom module in testing mode by replacing your API keys with those defined in tests/mollom.test, and using those, I'm not able to replicate this issue with the latest code.

I don't actually see how Developer mode is gone. Is it being phased on from the mollom.com interface or servers?

Are we talking about different things? IIRC, I saw a module setting on Mollom's global configuration page called "developer mode" in earlier versions. If that is not meant here, then we talked past each other. ;)

Yeah, its not in the module settings, its on the subscription settings on mollom.com. :) I did put a feature on the backburner to add a 'developer mode' option to the actual module that would replace the current keys with the testing keys. That seems like it might be a good idea.

Yes, you set your keys in developer mode on mollom.com. The keys provided in mollom.test are keys that are (permanently) put in developer mode on mollom.com. So it shouldn't matter whether you use the keys from mollom.test, or your own keys put in developer mode on mollom.com.

Is there an XML-RPC method to toggle developer mode? :) I'm a big fan of ...clickety-click-click..., you know =)

@sun: It's in the feature request list for Mollom (since it's not specific to the module).

There is no API for it today. The reason is two-fold, but it could be revisited:

1. It is a Mollom developer feature. Normal end-users would never put their keys into developer mode. The people that would use the "developer mode" feature is probably us three, and maybe a handful of other people.
2. It takes up to 30 minute for "developer mode" changes to propagate to all Mollom servers. As such, it is not really something you turn on and off "clickety-click-click". We might be able to propagate those changes faster but that would require additional backend work.

Untested (not even tried), but this should work.

Mmm, I'll want to think about this a bit more. There is a risk that people incorrectly leave things in test mode.

The only thing I can think of would be an additional warning message (dsm) on the admin/config/mollom* pages when the developer mode is enabled.

Yes we'd need to add a hook_requirements() check and it would be very useful to add a dsm() whenever the Mollom form element is added explaining developer mode (like only using 'correct' or 'incorrect' in CAPTCHAS and spam/ham/unsure for textual analysis).

I'd love to get this done. Any further feedback or suggestions?

+++ mollom.admin.inc	5 Jan 2010 19:16:24 -0000
@@ -317,20 +317,52 @@ function mollom_admin_settings($form, &$
+  // Always show this setting if it is enabled, or when Devel module is enabled.
+  if (variable_get('mollom_debug_mode', FALSE) || module_exists('devel')) {

Actually, this is a big chance we've completely missed for D7... Since Drupal 4.7, Administration menu module implements developer-only settings as additional fieldset on Devel module's settings page.

Which means: Only if you have Devel module installed, it's worth to expose any developer settings at all, and additionally, don't confuse regular users on the regular module settings page(s) with crazy-techy developer and debugging settings.

For D7, we could have taken the chance to implement a (eventually empty) dedicated developer settings form in core, on which (most) modules can add their stuff. Bummer.

Powered by Dreditor.

Here's the effort I've been working towards to making a better developer mode.
1. Keeps the checkbox on the settings page. The public and private key settings will be hidden whenever the development mode is enabled so it should be pretty clear that they are in development mode.
2. Added a mollom_get_keys() function that should be always used to fetch the current keys.
3. To help enforce that the site is in development/testing mode, I added a condition in the status requirements as a warning.
4. Instead of duplicating efforts by showing the current keys status via mollom_admin_settings() and mollom_requirements(), I've merged them into using just the requirements. And now we can view those messages on any major Mollom admin page (the forms page, which is the default landing page, or the blacklist page). This is the new _mollom_show_status() function in mollom.install.
5. When developer mode is enabled, it will display some helpful text near the CAPTCHA based on the type of protection on the current form. (e.g. ''Enter "spam", "ham", or "unsure" in any of the following fields (@fields) to trigger a CAPTCHA.' or 'Valid CAPTCHA values are "correct" or "incorrect".').
6. Found some broken links in the requirements message and missing hook_help for admin/config/content/mollom/blacklist.
7. node form info is incorrectly using 'body' as the field to check when it should be 'body][LANGUAGE_NONE][0][value'
8. Fixed bug with _mollom_status that would cache the wrong status for keys if we changed from using keys to having empty keys (would still show 'Your keys are working' instead of 'You have not configured keys')

TODOs:
- Needs more inline documentation around a lot of things.
- Not sure if this passes tests or not. Having a hard to getting any tests running after the hard drive crash.

I just recovered from a big hard drive crash so I'll try and get some screenshots up shortly.

+++ mollom.install	26 Feb 2010 18:03:19 -0000
@@ -13,21 +13,30 @@ function mollom_requirements($phase = 'r
+        $requirements['mollom']['description'] = t('We contacted the Mollom servers to verify your keys: the Mollom services are operating correctly. We are now blocking spam.');

I don't think that this requirements description makes sense on the status report page.

+++ mollom.install	26 Feb 2010 18:03:19 -0000
@@ -35,12 +44,39 @@ function mollom_requirements($phase = 'r
+function _mollom_show_status() {
+  static $checked = FALSE;

1) Missing PHPDoc.

2) Should be a drupal_static() to be compatible with testing.

+++ mollom.install	26 Feb 2010 18:03:19 -0000
@@ -35,12 +44,39 @@ function mollom_requirements($phase = 'r
+    $menu_item = menu_get_item();
+
+    if (strpos($menu_item['path'], 'admin/config/content/mollom') === FALSE) {
+      return;
+    }
+    elseif (!in_array($menu_item['type'], array(MENU_LOCAL_TASK, MENU_DEFAULT_LOCAL_TASK, MENU_NORMAL_ITEM))) {
+      return;
+    }

I'm not sure why this function lives in .install, while only being used outside of .install.

Additionally, the second condition badly needs docs.

If I read this correctly, then Mollom will try to verify keys on any page that is a local task or normal menu item, i.e. almost everywhere. This was purposively limited to Mollom's settings page and the status report page.

Overall, however, this logic bound to menu items looks prone to errors. I don't like how user interaction logic is contained in a supposed general helper function here.

+++ mollom.install	26 Feb 2010 18:03:19 -0000
@@ -35,12 +44,39 @@ function mollom_requirements($phase = 'r
+        REQUIREMENT_OK => 'message',

Probably obsolete when resolving the above, but anyway: s/message/status/

+++ mollom.module	26 Feb 2010 18:03:23 -0000
@@ -118,8 +133,14 @@ function mollom_help($path, $arg) {
+  // When a user visits any Mollom administration page, automatically verify the
+  // keys and output any error messages.
+  module_load_install('mollom');
+  _mollom_show_status();

This loads mollom.install on all pages.

After reverting, we can also revert the other changes to mollom_help() in this patch.

+++ mollom.module	26 Feb 2010 18:03:23 -0000
@@ -161,6 +182,7 @@ function mollom_menu() {
+    'type' => MENU_CALLBACK,

Not necessary, since menu items containing dynamic arguments will never show up in menu trees.

+++ mollom.module	26 Feb 2010 18:03:23 -0000
@@ -436,6 +480,21 @@ function mollom_form_alter(&$form, &$for
+        $form['mollom']['developer_mode'] = array(
+          '#prefix' => '<div class="messages warning">',
+          '#suffix' => '</div>',
+          '#markup' => t('Developer mode enabled.'),

Why not #type 'container' ?

Powered by Dreditor.

#536050: Keys API integration is very very relevant to this issue/patch.

Awesome. We collaborated and added an optional 'testing' parameter for all Mollom calls that will eventually be used for developer mode instead of the key-flag in the server end. That way modules can do whatever they want with developer mode, like we want to with this issue. :)

"Joepie!", as we say in Dutch. :)

Updating title to reflect the current direction.

The attached patch adds an in-module testing mode toggle that leverages the API changes. Any reviewers?

We'd still need to update the tests as we no longer need to use the server-side developer mode.

And a screenshot.

New patch attached.

1. Improved some of the language (and documentation).

2. Incorporated #813424: Tests fail because change in user registration behavior so the tests hopefully pass.

3. Incorporated #801662: Blacklist page does not explain that it requires valid keys to already be installed and adjusted the tests accordingly. It made sense to incorporate this because we wanted to show a warning when developer mode is enabled.

Committed #31 to CVS HEAD. We might want to backport this patch, or parts thereof to Drupal 6.

Fixed some remaining issues with the changed code in this patch, and also rephrased and completed the developer mode description.

I wonder whether the developer mode shouldn't be automatically enabled/triggered when running tests? (not included anywhere in these patches yet)

1) Not sure why the checkbox cannot simply state "Enable testing mode". Much more precise + clear for casual users. Allows to remove the first sentence from the description.

2) If I'm not mistaken, then we no longer write "When enabled," in descriptions? Need to look that up.

3) Single quotes could be double quotes.

4) Kill the last remaining "Mollom" in that description. The entire settings page is about Mollom, no need to repeat that all over again.

5) Do all users know what a CAPTCHA is?

6) "Make sure to disable this option in production environments." could use some love. Smells.

Will wait for further feedback before re-rolling.

#33: mollom-HEAD.developer-mode.33.patch queued for re-testing.

1) Not sure why the checkbox cannot simply state "Enable testing mode". Much more precise + clear for casual users. Allows to remove the first sentence from the description.

Works for me. We should rename the variable from developer_mode to testing_mode (potentially in the backend and API documentation too)?

4) Kill the last remaining "Mollom" in that description. The entire settings page is about Mollom, no need to repeat that all over again.

Works for me.

5) Do all users know what a CAPTCHA is?

No, they usually don't. We use "Word verification" elsewhere in the UI. Maybe we should link to http://en.wikipedia.org/wiki/CAPTCHA?

6) "Make sure to disable this option in production environments." could use some love. Smells.

It reads fine to me -- I can wordsmith at any point in time so let's not hold up this patch by one smelly sentence.

Re #33: the tests enable developer mode. See MollomWebTestCase::setKeys() or search for

    // Set the Mollom keys in developer mode.
    variable_set('mollom_developer_mode', 1);

in mollom.test. I don't think that enables any additional simplifications though. We need to provide some default keys if we want testing on drupal.org (and other test environments) to be relatively painless.

Rephrased.

Comment tests in HEAD are broken?

Hmm. As a suggestion, instead of:

+    '#description' => t('Submitting "ham", "unsure", or "spam" as post body on a protected form will trigger the corresponding behavior. All word verifications will ignore the requested characters and only distinguish between "correct" and "incorrect". Make sure this option is disabled in production environments.'),

how about:

+    '#description' => t('Submitting "ham", "unsure", or "spam" as input on a protected form will trigger the corresponding behavior, and similarly, word verifications will only respond to "correct" and "incorrect" (rather than the actual characters displayed). This option should be disabled in production environments.'),

To me "post body" sounds odd. I also have an issue about "Make sure..." .

yummy!

Submitting "ham", "unsure", or "spam" on a protected form will trigger the corresponding behavior, and similarly, word verifications will only respond to "correct" and "incorrect", instead of the actual characters asked for. This option should be disabled in production environments.

?

#42 works for me and is pretty crisp! :)

ok, re-rolled with #42. Looks RTBC to me...

...but whew, the changes for #801662: Blacklist page does not explain that it requires valid keys to already be installed that got intermixed here scare me a bit. Since those are technically unrelated to this issue, I'm going to re-open that other issue.

Also, just trying to prevent any major mistakes having consequences -- http://drupal.org/cvs?commit=373702 states that the audio captcha .swf file was also updated as part of these patches. Why is that? Are we sure that this update was correct, and, what has been changed?

I committed the patch in #44. Thanks sun.

Re #45: I don't remember changing the Flash file so that must have been a mistake. Odd because I don't see how I could have changed that file. People requested that the audio CAPTCHA player starts autoplaying (if you're blind, you don't know that you need to click 'play') so maybe we should create an issue for that, and just commit a better SWF in that issue?

Regarding the .swf, I'd highly recommend to checkout the previous revision and re-commit that. Unknown changes to binary files are pretty bad.

This still needs to be backported. On it.

I really hope this cuts it. Still kinda hard to forward- and backport patches. However, all other remaining changes belong to #801662: Blacklist page does not explain that it requires valid keys to already be installed

I suspect that these are the failures that triggered the (IMHO still bogus looking) change to assertMollomWatchdogMessages()... Attached patch should pass.

This patch looks entirely correct. Committed to DRUPAL-6--1.

Poor sdboyer. ;)