Blacklist page does not explain that it requires valid keys to already be installed

The main Mollom settings page doesn't display such a message either. It is probably a good idea to apply this, or another pattern, across the two tabs.

The truth is that the warning message does not prevent the errors from happening. After all, you could ignore the message and still use the blacklist form. A better solution _might_ be to hide the tabs but I'd need to revisit the code to figure out if that can be implemented efficiently.

Thoughts?

You can just use an access callback to the tabs. Not sure that hiding the tabs is the best UX wise though.

Working on this. Assigning to myself.

See #415014-31: Add an in-module developer mode toggle as I incorporated these changes into the patch in #415014. This made sense to do because we also needed to explain that developer mode was enabled. The approach is identical to the mechanism required to mention that keys still need to be installed.

Committed a better version of this patch to CVS HEAD along with #415014: Add an in-module developer mode toggle. Thanks!

Re-opening, since I discovered the changes for this issue only now:
http://drupalcode.org/viewvc/drupal/contributions/modules/mollom/tests/m...

@@ -149,10 +149,9 @@
       ->condition('w.type', 'mollom')
       ->orderBy('w.timestamp', 'ASC');
     foreach ($query->execute() as $row) {
-      if ($no_fail_expected ? $row->severity >= WATCHDOG_NOTICE : $row->severity < WATCHDOG_NOTICE) {
-        $this->pass(theme_dblog_message(array('event' => $row, 'link' => FALSE)), t('Watchdog'));
-      }
-      else {
+      // All watchdog messages with a higher severity than WATCHDOG_NOTICE are
+      // considered as fails, even when no fails are expected.
+      if ($no_fail_expected && $row->severity < WATCHDOG_NOTICE) {
         $this->fail(theme_dblog_message(array('event' => $row, 'link' => FALSE)), t('Watchdog'));
       }
       $this->messages[$row->wid] = $row;

I think this change invalidated the intended logic of the watchdog message assertion. As the variable name $no_fail_expected implies, we need to assert that no severe watchdog messages were logged. And vice-versa, i.e., if we expect a failure, then there has to be one. Effectively, I think that this change broke the intention of a couple of our tests.

However, I'm also starring on these diff lines for about 20 minutes already, trying to think through all cases. Maybe I'm wrong, but it looks like if we're passing FALSE for $no_fail_expected, then we won't ever assert a failure, as the initial condition is plain FALSE already.

http://drupalcode.org/viewvc/drupal/contributions/modules/mollom/mollom....

 /**
+ * Implements hook_menu().
+ */
+function mollom_init() {
+  // When on an administration page, verify the Mollom keys.
+  if ((arg(0) == 'admin' && arg(1) == 'reports') ||
+      (arg(0) == 'admin' && arg(1) == 'config' && arg(2) == 'content' && arg(3) == 'mollom') &&
+      $_SERVER['REQUEST_METHOD'] == 'GET' &&
+      user_access('administer mollom')) {
+    // Verify that the keys have been configured.
+    $status = _mollom_status(TRUE);
+    if ($status !== TRUE) {
+      // Display the appropriate error message.
+      if (!$status['keys']) {
+        drupal_set_message(t('The Mollom API keys are not configured yet. Visit the <a href="@settings-url">Mollom settings page</a> to configure your keys.', array('@settings-url' => url('admin/config/content/mollom/settings'))), 'error');
+      }
+      elseif ($status['keys valid'] === NETWORK_ERROR) {
+        drupal_set_message(t('The Mollom servers could not be contacted. Please make sure that your web server can make outgoing HTTP requests.'), 'error');
+      }
+      elseif ($status['keys valid'] === MOLLOM_ERROR) {
+        drupal_set_message(t('The configured Mollom API keys are invalid. Visit the <a href="@settings-url">Mollom settings page</a> to configure your keys.', array('@settings-url' => url('admin/config/content/mollom/settings'))), 'error');
+      }
+    }
+    else {
+      if (variable_get('mollom_developer_mode', 0) == 1) {
+        drupal_set_message(t('Mollom developer mode is still enabled. Visit the <a href="@settings-url">Mollom settings page</a> to disable developer mode.', array('@settings-url' => url('admin/config/content/mollom/settings'))), 'warning');
+      }
+    }
+  }
+}

1) minor: Wrong PHPDoc.

2) Could use strpos() instead of that many + slow arg() invocations. Also, empty($_POST).

3) Why do we invoke this status check and message on all admin/reports* pages?

4) I'd prefer to display the testing mode message only on pages containing a (protected) form. And, more importantly, I'd prefer to display this message regardless of user permissions. If your site's users are complaining about not being able to submit forms, and at the same time, mentioning a warning message about a "Mollom testing mode", you immediately know what's going on.

5) hook_requirements() had the benefit of Drupal actually reporting a configuration error, automatically. There are contributed modules that periodically check requirements and automatically send an e-mail if something happens to be wrong. Properly debugging Drupal is a problem space on its own, and the status report page is at least the first address users should be looking for warnings or errors if something does not work. From a more distant perspective, it is counter-productive to replace the requirement checks there with arbitrary error messages elsewhere.

This also happily translates into: "I don't like code in hook_init()."

--

Effectively, I'd suggest to roll back the removal of hook_requirements(), and rather execute mollom_requirements() on those particular page callbacks as well as mollom_form_alter() [for protected forms].

2) Could use strpos() instead of that many + slow arg() invocations. Also, empty($_POST).

Sure.

3) Why do we invoke this status check and message on all admin/reports* pages?

Probably an oversight.

4) I'd prefer to display the testing mode message only on pages containing a (protected) form.

Works for me.

5) hook_requirements() had the benefit of Drupal actually reporting a configuration error, automatically.

Works for me.

Properly extracting and reverting the changes from the merged commit is quite time-consuming. Attached patch is contains the reversely applied changes, mixed into the current code.

Before continuing here, I think we need to clarify the goal. Let me explain my strategic point of view:

• Keys are required to be configured, keys are required to be valid, and the module/site is required to be able to communicate to mollom.com. Therefore, we are talking about requirements and requirement errors.
• The status report page is where modules should note missing or incomplete requirements to use a functionality. Optionally including elaborate guidance on how to fix the particular issue. Contrary to error messages and other tools, requirement items/errors intentionally leave space for that.
• Users are (or should be) trained to not ignore requirement errors, and, to check the site's status report from time to time.
• Drupal core already ensures that system requirements are checked on certain locations. If additionally needed, the module is (manually) able to invoke all or just its own requirement check, where appropriate.

  It may be true that Drupal core's behavior for checking and re-ensuring requirements is a bit weak and/or memory intensive. However, the proper place to improve those checks would be Drupal core then.

• Technically, Drupal core only checks requirements on /admin (no sub-pages) and /admin/reports/status. Not on any other admin page.

  Since we are required to check our requirements on most of our admin pages, we are forced to

  1. either just check our requirements, and display just our requirement error as error message
  2. or check system requirements as usual, which outputs an error message including link to the status report page automatically

  Due to aforementioned weak implementation and performance reasons, I guess we're going to do 1.

Effectively, this would mean:

1. mollom_requirements() was based on mollom_status(). To only check if requirements are met, mollom_status() can be invoked.
2. mollom_requirements() generates UI error messages only. It can be restored and kept in mollom.install.
3. We add a private helper function, _mollom_requirements(), which basically does the following:
   

     $status = mollom_status(TRUE); // TRUE == Force re-check.
     // Only load mollom.install, if we need requirements
     if (!$status) {
       module_load_include('install', 'mollom');
       $errors = mollom_requirements(FALSE);
       drupal_set_message($errors[0]['description'], 'error');
     }
   

   This function will be invoked manually on our administrative pages. Roughly, it simply checks the status first, and if that is not ok, we load mollom_requirements() to use it as dedicated UI error message generator.

While we could just move mollom_requirements() from mollom.install into mollom.module at this point in time, we would definitely face this problem space again, once we need to add a pre-installation requirement.

The requirements handling in Drupal did not suffice and we still got a fair amount of people that did not know what to do after installing the Mollom module. The Mollom settings page is hidden and it is not natural to navigate to it after someone installed Mollom -- we needed to better guide new users to the Mollom settings page.

The problem is not unique to Mollom, many other modules have the same problem. The requirements page might be the best we have today, but let's recognize that it is not a good solution relative to the post installation experience. You don't present people an error after enabling a module; you need to guide them in a wizard-style way. Error messages make for a poor and scary experience. ;-)

Anyway, that is a bit of a rant but it might provide some additional context and background. ;-)

I only skimmed the patch in #11 and it doesn't look like it reverts the UX improvements we've made to guide people to the Mollom settings page when necessary. Let's wait for the test bot -- is it still on?

Additional thoughts? Additional ideas to improve the new user experience?

Note: We're definitely working around a shortcoming of Drupal core here. A very important task for D8 should definitely be to add an admin_guide module to core. I might as well invent that + add it to my package elsewhere. Anyway:

Didn't update the tests accordingly, so tests will likely fail, but I'm relatively happy with attached patch. We're reverting to hook_requirements(), and we're using _mollom_status() to programmatically and quickly check the configuration status on administration pages. We only link to the settings page, if the current user is able to access it. Regardless of user permissions, all users will see the testing mode warning. But only admins will get a link to the settings page. Details are documented in-code.

Additionally, I stumbled over some more hard-to-grok behaviors and expectations that are contained in the module's code for a long time already. Added docs for those bits as well.

Lastly, some of the added @todos are debatable. You may want to use Dreditor to comment on them.

Worked on those tests.

s/developer mode/testing mode/

Re-rolled against HEAD.

+++ mollom.admin.inc	4 Aug 2010 10:22:56 -0000
@@ -476,57 +476,73 @@ function mollom_admin_blacklist_delete_s
 function mollom_admin_settings($form, &$form_state) {
...
+  // Output a positive status message, since users keep on asking whether
+  // Mollom should work or not. Also do this on every regular visit of this
+  // form to verify ... @todo Broken expectations! :)

+++ mollom.install	4 Aug 2010 10:22:57 -0000
@@ -110,6 +172,15 @@ function mollom_schema() {
+function mollom_install() {
+  // Point the user to Mollom's settings page after installation.

These two expectations are not covered by tests currently.

Additionally, I'm not sure whether we want to re-verify the keys/configuration upon every visit of the settings page, but it might make sense.

Powered by Dreditor.

Additionally, I'm not sure whether we want to re-verify the keys/configuration upon every visit of the settings page, but it might make sense.

We want to. It gives us an opportunity to update the site's server list, and in the future, to send module information to Mollom.

Reviewed the patch and it looks good. I've a couple of minor thoughts about some of the @todos but they are very minor.

In general, the patch has a fair amount of @todo's so I'm wondering: would you want me to commit this now or are you still working on those @todos?

+++ mollom.admin.inc	4 Aug 2010 10:22:56 -0000
@@ -476,57 +476,73 @@ function mollom_admin_blacklist_delete_s
+  // Output a positive status message, since users keep on asking whether
+  // Mollom should work or not. Also do this on every regular visit of this
+  // form to verify ... @todo Broken expectations! :)

Fixed + resolved in this patch.

+++ mollom.admin.inc	4 Aug 2010 10:22:56 -0000
@@ -476,57 +476,73 @@ function mollom_admin_blacklist_delete_s
+  // Clear the stored configuration status upon form submission. Needs to be
+  // manually set, as this is a system_settings_form().
+  $form['#submit'][] = 'mollom_admin_settings_submit';

@@ -552,6 +568,14 @@ function mollom_admin_settings($form, &$
 /**
+ * Form submit handler for mollom_admin_settings().
+ */
+function mollom_admin_settings_submit() {
+  // mollom_init() will re-check the status upon redirection of this form.
+  variable_del('mollom_status');
+}

Those lines are gone, as we are re-checking the module's configuration status upon every visit of the settings form now.

+++ mollom.admin.inc	4 Aug 2010 10:22:56 -0000
@@ -476,57 +476,73 @@ function mollom_admin_blacklist_delete_s
+  // @todo Is installing without configuring still possible after all?

I think that this entire issue + patch more or less destroys the option of being able to install + prepare/configure without entering API keys. I've removed this @todo for now; separate and entirely different issue.

+++ mollom.module	4 Aug 2010 10:22:58 -0000
@@ -578,6 +567,25 @@ function mollom_form_alter(&$form, &$for
+  // @todo Ideally, this message would show up on protected forms only, though
+  //   still regardless of user permissions. That would require to populate
+  //   $protected_forms really fast, however.

Just shortened this @todo for now. We'll implement a fast cache for this over in #849340: Cache mollom_form_load() and query in mollom_form_alter()

+++ mollom.module	4 Aug 2010 10:22:58 -0000
@@ -578,6 +567,25 @@ function mollom_form_alter(&$form, &$for
+    // @todo "still" does not make sense for innocent users. They probably have
+    //   no clue what this message means at all. Which might as well be what we
+    //   want. ;)
+    $message = t('Mollom testing mode is still enabled. !admin-message', array(

Removed this @todo for now. I guess it makes sense this way in the end. :)

+++ mollom.module	4 Aug 2010 10:22:58 -0000
@@ -1050,6 +1058,8 @@ function _mollom_status($reset = FALSE) 
 function _mollom_fallback() {
   $fallback = variable_get('mollom_fallback', MOLLOM_FALLBACK_BLOCK);
+  // @todo Add !empty($_POST) to this condition + manually invoke this function
+  //   from mollom_process_form() on GET requests?

Heavily clarified this comment as well. Thinking more and more about this single @todo, I almost guess that resolving this single one would have resolved many other issues related to installation and status handling... :)

Please wait for patch coming back green.

Powered by Dreditor.

LOL

This one should pass.

Committed to CVS HEAD. Thanks.

Do we want to backport this?

That second patch is a tiny follow-up fix for D7. Should be re-tested after committing the D6 patch.

However, note that we have 0 (zero) passes again, which means that no tests were executed. Requesting a re-test.

#28: mollom-DRUPAL-6--1.requirements.28.patch queued for re-testing.

#28: mollom-DRUPAL-6--1.requirements.28.patch queued for re-testing.

#28: mollom-DRUPAL-6--1.requirements.28.patch queued for re-testing.

#28: mollom-DRUPAL-6--1.requirements.28.patch queued for re-testing.

#28: mollom-DRUPAL-6--1.requirements.28.patch queued for re-testing.

Bogus "0 pass(es)".

Is it perhaps caused by the patch filename...?

Still 0 passes. Odd.

Running tests locally revealed an exception, but I doubt that an exception is able to trigger this odd behavior.

#39: mollom-DRUPAL-6--1.requirements.39.patch queued for re-testing.

Looking more closely at the failing test results, it seems like tests are returning early due to a fatal error. Drupal 6 does not load .module files during installation. When manually running tests, this seems to work somehow. So PIFR seems to install modules differently... (?) Let's give this a try.

#42: mollom-DRUPAL-6--1.requirements.42.patch queued for re-testing.

#42: mollom-DRUPAL-6--1.requirements.42.patch queued for re-testing.

The bot is still unhappy. :/

Let's try another variant then. Moved the drupal_load('module', 'mollom') into mollom_requirements().

#46: mollom-DRUPAL-6--1.requirements.46.patch queued for re-testing.

So what happens if we remove that entire code from hook_install()...?

Eliminating major parts of the patch... just to see.

Back to #39.

Now that the testbot seems to work again, let's finally port and commit this one. Please wait for the bot to come back green (and >0 passes).

Don't halloo till you're out of the wood! Had to re-test 3 times..., but finally passes now.

Committed to CVS HEAD. Thanks.