Filter settings are shown for disabled modules

The settings page doesn't show the filter settings if the module is disabled. It does show the filter settings if you have the filter turned off in the filter settings page. Since the filter module isn't itself disabled, I'm not sure this isn't just expected behavior.

But -- when editing filter settings for a filter that's currently turned off, the page should probably warn you that it's off.

This can be done by implementing that in each filter module and requiring that same code for all future filter modules, or it can (better) be done in spam.module by having it, instead of the filter modules, implement the filter settings pages -- the filter modules will just add its settings to its spam.module generated filter page. Then we can set all module settings in that page, including weight and such (which will come from the standard "template" provided in spam.module, along with the warning message).

I just feel like asking the question here again... Why is it that we need an enable/disable flag that's a duplicate of the Module?

Do we foresee a module that would offer 3 or 4 filters and that way users could choose which ones they want to use in details?

> Do we foresee a module that would offer 3 or 4 filters and that way users
> could choose which ones they want to use in details?

That was not the intention as far as I remember, though I suppose that's a possibility. In reality, I believe the goal was to allow complete control over the filters from this screen. Reviewing this issue, however, it seems it's really just adding confusion. I think it would be acceptable to remove the enable/disable flag from this page to simplify the overall UI, and if perhaps use use hook_help() to include a link to the module page for enabling/disabling filters (though that's likely not even necessary). The underlying code that checks this flag will also need to be refactored, but easy enough.

Yes, removing this redundant way to enable/disable filters has my vote.

Makes sense to me.

Though, on a separate note, I think it may still be nice to have spam.module "own" the admin forms for the submodules, and give them a hook to add their specific customizations in. (Not really directly related to this issue anymore, though.)

Okay, let me supply a first patch for the removal of the Enable column. 8-)

There is a first patch.

I have one main very important point to make:

--->>> All the administration forms need to move the a .admin.inc file.

At this point, this is not a patch to move the code that way, but my next patch could very well be that. It will help us in seeing what's what. There should also be some other sub-modules. That was making me think we could have a Spam block to show the admins what's going from anywhere on their site. Just a thought at this point. 8-)

------ On to the patch ------

* spam_help()

The path was wrong and pointed to the main settings page instead of the overview. I fixed that and obviously removed the reference to enabling/disabling filters from here.

* spam_admin_filters()

I removed the 'status' from the query. At this point I did not change all the code, but I guess this one patch should include the removal of the 'status' column from all the code, do you agree?

Marking the form as a tree generally works better when you have more than one level. So I added that.

Removed the Enable column.

Switch the $form['name'][$key] the other way around and removed the extra stuff added to the key (i.e. $form['weight']['weight'.$key]). This works better and is more in line with Form 2.0.

Increased the delta of the weight to 20 (instead of the default of 10). Just in case.

Removed the $counter since we do not need it (again, that's more Form 2.0 compliant).

* spam_admin_filters_submit()

Update the submit to work with the new scheme. (new layout and no status column)

* theme_spam_admin_filters()

Redesign this function to work with a more Form 2.0 scheme. It makes the table rows draggable on the weight (with the little +). And again, removal of the Enable (status) column.

Fixed the default message (When no filters are defined) so it gives a link to the Modules administration page where people can enable/disable extra filters. Unfortunately, there are no good anchor to send people in the Spam vicinity.

Let me know if you have any question about the changes.

Thank you.
Alexis

The biggest issue I see with this current patch is that disabling a filter module does not disable it in the filter listing page.

> but I guess this one patch should include the removal of the 'status'
> column from all the code

It needs to remove it from the code and the database both. (I'm fine with committing it in pieces if that helps, however, as it's a big step forward in improving usability.

> +    case 'admin/settings/spam/filters':
> +      return t('Control each filter gain and the order in which they run.');

This administration page generates a lot of confusion for people using the spam module for the first time. We should provide useful help here instead of this terse sentence... Perhaps we can pull some useful information from this handbook page:
http://drupal.org/node/498092

> +    //$form[$fid]['#spam_filter'] = $filter; -- not currently required, avoid at this point

What is this? Why the commented out line?

The to drag and drop filters around to order them is a great usability improvement!

> The biggest issue I see with this current patch is that disabling a filter module does not disable it in the filter listing page.

There is another proposed patch for that, but the issue is to have code in the hook_[un]install() to update the list of filters. See #1009448: Filters remain in list on uninstall

I think it is better to keep that problem separate since it is not a problem with the UI which is what we're fixing here.

---

I'll come back with a fix to the database as well.

Thank you.
Alexis

There is a patch with the following additions:

1) Much better documentation (hook_help()) -- see below

2) No more status, spam_update_6101() to remove it from existing tables

3) One fix around line 1280 where the $output variable was set for the first time with .= (i.e. E_NOTICE!)

I have another comment in that regard: setting the Gain to 0 has the same effect as Disabling a filter. Shouldn't we prevent users from doing so?! What would be the point of doing that?

In regard to the documentation, I have a picture (see attached) representing the computation. We could add it to the module so end-users (spam admins) would see it on that page. I suggest we create a doc sub-folder in the project and stick that image in there. What do you think?

As much as I like to keep separate problems separate, I think Jeremy has a point in #8. If the install/uninstall hooks properly enable and disable modules, the problem in this issue goes away entirely. (Since the module wouldn't be in the database any longer anyway.)

Gnassar,

Note that I mistakenly said [un]install, but it should actually in the enable/disable hooks.

Now fixing those enable/disable hooks will solve the visual problem in the UI and again we won't need the Enable column that this current patch removes.

Oh! A note... #8 was by me... 8-)

So... to make sure I understand, are you saying that we should have a patch here that also fixes the enable and disable hooks? If yes, I'll proceed with that (although there is another issue for that problem as I also mentioned somewhere.)

Thank you.
Alexis

Sorry -- I meant he had a point in #7. Thank you. What I was saying was, fixing the problem in the other issue makes this issue unnecessary -- it solves both problems. So maybe we should be focusing on #1009448: Filters remain in list on uninstall.

And of course, we actually mean the en/disable *and* the un/install hooks.