Enable/disable signups by content type

I'm not exactly sure what to title this issue... when I first skimmed the queue, I thought this was a feature request about the signup limits functionality (given event only has N seats, etc). You're really talking about enabling/disabling signups entirely on a per-type basis, instead of giving admins with enough permission the ability to decide to signup-enable any kind of content on the site, right? That seems potentially confusing, but also perhaps worth supporting. No one's working on this as far as I know. If you can come up with some good mockups of how it would look in the UI, drafts of the descriptions and help text, etc, then it should be relatively easy to implement this, since it's really just a question of hiding functionality that's already there.

dww's description sounds a lot like comments on a node. they are defaulted by content type but changeable on a per node basis.

@moshe: that's exactly how signup.module works now. I think this feature request is to make it so that there's an admin override of some kind that prevents 1-time signup-enabling on certain node types. For example, some site that never wants to signup-enable Page or Book page nodes, so even admins don't see the "Signup settings" fieldset when creating or editing those two node types.

Thanks for the patch, this is a good start. I haven't tested this, but based a quick skim of the code, I see the following problems:

A) Multiple checkboxes seems confusing. What if you select "Allow signups by default" but not "Enable signups for this content type"? Honestly, I'm guessing most site admins wouldn't understand the subtle difference between these two options, either. I think this should be a set of 3 radio buttons: Completely disabled, enabled but off by default, enabled and on by default. I don't know exactly what to call each option, suggestions welcome. ;)

B) I really don't like having to duplicate this block of code in multiple locations:

+  $signup_options = variable_get('signup_options_'. $node->type, array());
+  $signup_available = in_array('available', $signup_options) ? TRUE : FALSE;
+  $signup_enabled = in_array('enabled', $signup_options) ? TRUE : FALSE;

I'd rather we had a single variable with some PHP constants. If we really need 2 independent settings for some reason, let's make them separate checkbox elements entirely, instead of this checkboxes set, and then we can variable_get() each one directly.

C) There's no upgrade path for existing sites. You need a DB update to convert the existing settings into the new settings, and remove the stale settings from the DB.

D) signup_uninstall() needs to know about removing the new settings instead of the current ones.

If you address these problems, I'll take a closer look and actually apply the patch and test it.

Thanks!
-Derek

(BTW, please change the status back to "code needs review" when you post another patch that addresses the previous concerns...)

Thanks, this is indeed looking much better. My gripes with your latest patch... ;) (hence, I'm leaving this "needs work"):

A) $signup_status is a misleading name for the variable. In most of the signup codebase, we use "signup status" to refer to if signups are open or closed for a given node. This is really more like "$signup_type_setting" or some such... In fact, the best thing would be for the variable names and setting name to match...

B) "signup_options_[type]" is also a little wonky for the setting/variable name. Maybe that's the best choice, but it doesn't seem ideal to me. Can you try to come up with a better alternative? I'm exhausted and need to go to sleep now (1:50am here), and I'm not thinking clearly enough to have a good suggestion myself. ;) "signup options" makes me think of options for your reply when you signup or something.

C) $form['workflow']['signup_options'] needs a #description to explain what the choices really mean, and how they interact with the 'administer all signups' permission.

D) $findme is also a little bit unfortunate. ;) How about "$old_prefix" instead?

E) Pretty sure this hunk at the very top of signup_nodeapi introduces bugs:

+  // Skip all this if signups aren't possible for the content type
+  if ($signup_status == 'disabled') {
+    return;
+  }
+

Say you have a node type that doesn't allow signups by default. An admin signup-enabled a given node of that type, anyway. After they upgrade to this patch, this node type will have signups 'disabled'. Now, the admin goes back to edit this node, to disable signups on that particular node. But, your bail-early code there causes signup_nodeapi() to abort before the logic in the 'update' case has a chance to run. The admin tries to save the change, but when the page reloads, the node is still signup-enabled. :( I haven't actually tried this myself, but I'd bet $20 that's what would happen.

That hunk should be removed, and each case in signup_nodeapi() should test the setting and act/bail accordingly.

There might be more, but that's all I can see in my current state from just looking at the patch. ;)

Thanks,
-Derek

Cool thanks. I can't spot any more errors just from looking, so now I'll need to apply the patch, test it, see how it really looks in the UI, etc. Today is totally insane at my day job, but hopefully in the next few days I'll get to this.

I just tried testing this out and it's mostly working great. A few lingering problems:

A) Looking at it more closely, I'm not thrilled with the names of these settings, nor the description. It's close, but it still seems potentially awkward and confusing. I took the following stab at a better description, but I still don't really like it:

    '#description' => t('If %disabled is selected, signups will not be possible for this content type. If %enabled_off is selected, signups will be off by default, but users with the %admin_all_signups permission will be able to allow signups for specific posts of this content type. If %enabled_on is selected, users will be allowed to signup for this content type unless an administrator disallows signups on specific posts.', array('%disabled' => t('Disabled'), '%enabled_off' => t('Enabled, but off by default'), '%enabled_on' => t('Enabled, and on by default'), '%admin_all_signups' => t('administer all signups'))),

And, the two "Enabled, ..." names still don't quite sit right. I'd like to get some other ideas on what to call these for maximum clarity.

Also, please note that for better or for worse (IMHO, worse), the Drupal coding style says to never use 2 spaces after a period. Go figure. :(

B) In trying to break your patch, I found a case that didn't work how I expected. I'd consider this a bug, but you could argue otherwise and you might convince me. ;)
- Enable signups on a given node type, default to on.
- Create a node of that type, with signups enabled.
- Go back to admin/content/types/[type] and set signups to "disabled".
- Try to edit your signup-enabled node of this type, even as uid 1.
At this point, the node is still signup-enabled, there's a "Signups" tab, a signup form on the node, etc, etc. However, the admin doesn't have the "Signup settings" fieldset anymore, since the global killswitch is set to disabled for this node type.

So, I'd say that this if() statement needs help:

  if ($signup_type_default != 'disabled' &&
      (user_access('administer all signups') ||
      (!empty($node) && $signup_type_default == 'enabled_on' && $node->uid == $user->uid &&
       user_access('administer signups for own content')))) {

That also needs to handle the case where the node already exists and is already signup-enabled. If those are true, it needs to include the signup fieldset, even if the type is now configured with signups 'disabled'.

C) minor gripe, but i'm not convinced "signup_default_event" is a good name for the variable that holds if signups should be enabled or disabled for nodes of type 'event'. "signup_default_event" seems like it'd be pointing to an event node or something, the default event you signup for, etc. I'd rather a slightly longer name that was more self-documenting and clear. Again, it's too late for me to have a great suggestion, but I wanted to raise it. I won't refuse the patch if this is the only remaining problem, but if we can improve it, I'd be happier. ;)

Thanks!
-Derek

It seems to me that "signup_options_[type]" should become "signups_allowed_[numeric], so that you could easily store and retrieve the numeric value of those radio buttons. It'd be less confusing too.

I'll try to come up with some ideas for dww's #12.

@Senpai: sorry, #13 confuses me. ;) I don't see what [numeric] has to do with anything, since there are no numeric values of the radios. And we need these settings per type. Finally, the whole point of the setting is for allowed vs. disabled, so i don't see how having "allowed" in the name makes it less confusing...

Ok, let me rephrase. If you're using a var that holds the recorded value of whether a content type is signup-enabled, using $signups_allowed = TRUE, FALSE, or even doing a $signups_allowed = 1, 2, or 3 is the best way to go, IMO.

What I was getting at is this. Store the value as :
0 == disallowed
1 == allowed(defaulted off)
2 == allowed (defaulted on)

@Senpai, for enumerated setting variables like this, i prefer to store them as self-documenting strings, just as the previous patch does, instead of storing the values as integers and using PHP constants throughout the code. The variable table in the DB is already storing a serialize array as a string, so it's not like there's any DB performance gain to be had by using ints. The PHP performance gain of int == vs. string equality isn't enough of a big deal to me to justify obscuring the code via ints.

@dww: I see. Good points all around. I guess I've got nothing else to offer as far as ideas go, so I'll bow out and watch this patch come to fruition. Ping me again if you want a hand testing stuff out, ok?

@jrbeeman: Disabled, Allowed (off), and Enabled (on) seems good to me. My only suggestion would be to then use 'allowed_off' in the patch instead of 'enabled_off'. Otherwise, I think this is very close.

@senpai: I appreciate the help and it'd be great to have you test this as soon as the next patch shows up.

thanks all

Wonderful, thanks. Good timing, since I was already in the middle of doing some other signup patch reviews and development. ;) So, I had a chance to test this and look more closely. There were a few minor problems with your latest patch:

• Added a period at the end of the comment introducing signup_update_5203().
• In signup_update_5203(), I found the code was easier to read by introducing a $old_name variable.
• Code style: no space between the . and string literals (also in update 5203)
• In that crazy if() clause to figure out if we need the "Signup settings" fieldset, your patch was testing $node->signup but $node might be NULL, so I added a check for !empty($node) there, and fixed the comment in front of the if() to explain the new logic.
• Added an entry for the CHANGELOG.txt

Otherwise, this is great, so I committed my modified version to HEAD (http://drupal.org/cvs?commit=90979 if you want to see the exact diff, etc).

Thanks again!
-Derek

Drat, whoops. I forgot about signup_views.inc. This patch just broke the "Signup: Node: Enabled/Disabled'' views filter. :(

@jrbeeman: any chance you've got time to work on this?

Sorry, we're both confused. ;)

A) This filter is just checking if there's a record in the {signup} table or not, which is how you can tell if a node is enabled for signups or not. The new feature has nothing to do with this -- it only provides settings for the default UI regarding if you can enable signups. Fundamentally, everything still works the same -- if there's a record in the table, the node is signup enabled. I just tried this out on a test site and the filter is working just fine with the latest code. Sorry for the false alarm!

B) The dev load appears to be confused, or at least isn't exactly telling you the right thing. Here's how the filter is actually defined:

      'signup_disabled' => array(
        'name' => t('Signup: Node: Enabled/Disabled'),
        'field' => 'nid',
        'operator' => array('IS' => t('are')),
        'value' => array(
          '#type' => 'select',
          '#options' => array('NULL' => t('Disabled'), 'NOT NULL' => t('Enabled'
)),
        ),
        'handler' => 'views_handler_filter_is_null',
        'help' => t('Filter on if signups are enabled or disabled.'),
      ),

Since it defines 'field', it's using a valid field from the DB, not the "signup_disabled" that devel is telling you about. That's just a unique queue for the filters array, so that views doesn't clobber a filter that's actually using 'nid' as the field name.

Anyway, everything's cool here. Again, sorry about sending you off on a bogus mission to fix something that already worked...

Cheers,
-Derek