Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jrbeeman’s picture

Version: 5.x-1.x-dev » 6.x-1.x-dev
kentr’s picture

What's the path to get this interface? I'm not finding it.

I'm using 6.x-1.x-dev. I have defined statuses, but see nothing for defining limits per status - even on new nodes.

pelicani’s picture

sorry, this is no longer available in the core signup module.
you may have already seen this post ... http://drupal.org/node/359412#comment-1207160
the signup module had this function removed.
there is a plan to create a sub module to reincorporate it in the future.
help make that future now.

michellezeedru’s picture

Subscribing

dww’s picture

Assigned: jrbeeman » dww
dww’s picture

Here's a preliminary patch based on the spec I posted at #359412-3: Move code for signup limits per status per node into a submodule. This applies to HEAD of signup_status, and requires the latest code in DRUPAL-6--1 of signup since it builds on #578502: Move signup-related settings from node/N/edit to node/N/signups/settings. Attached are some screenshots of the bottom of the per-node signup settings form for the 3 different values of the radio with JS enabled, and a shot of the UI with JS disabled.

I noticed a slightly evil problem with the 'none' option when JS is disabled: so long as the radio is set to none, whatever you put in the "Limit on total signups" box is ignored, and you have to both move the Signup limit type to "Limit on total signups" *and* change the actual limit. Not sure if I care. 'None' does seem like a nice simplification to the UI when JS is enabled, so I think it's worth keeping.

dww’s picture

Based on some IRC feedback from greggles and ezra-g, we all agreed that "None" is more trouble than it's worth, so that's now gone. Also, at greggles's request, I added some help text to the radios (enclosed in a js-hide so it's only visible for non-JS users) to explain WTF is going on. ;) New screenies attached. Any other thoughts before I commit?

dww’s picture

Status: Needs review » Fixed

The UI is now committed to HEAD. That's about the only thing working now, but I'm going to call this fixed and move back to #359412: Move code for signup limits per status per node into a submodule for getting the rest of the spec implemented and working.

dww’s picture

Thinking about this UI some more, and based on the experience of #551862: singup_status_mailer settings UI cleanup, it occurred to me it might be nice to have a 3rd radio option after all for "Use the current site-wide defaults". In that case, we don't write anything to {signup_status_limit_node_limit}, and we stash 'default' in {signup_status_limit_node_setting}.limit_type. Whenever we load a node, if it's configured to use 'default', we load the current site-wide default limit settings.

This does make the non-JS UI pretty painful again, but I think overall, the price is worth it, so we don't have to store lots of duplicates on sites that mostly want to use the same defaults on all nodes...

However, it does complicate the site-wide settings page, since after this patch, changes there can have retroactive impact on existing nodes (which is both good and bad) not just something saved for future new nodes.

Thoughts?

p.s. in case it's not clear 241869-9.signup_limit_settings.default-status.no-perm.png is what 241869-9.signup_limit_settings.default-status.png looks like for a user that doesn't have permission to edit the site-wide defaults...

mlsamuelson’s picture

I haven't tested the patch, but the "Use sitewide default option" is a good one to have. Like stated, the ability for changes to have retroactive effect is both good and bad, but as evident in the no-perm screenshot, it is possible to keep the sitewide default locked up behind a permission, so that seems a good safeguard.

dww’s picture

but as evident in the no-perm screenshot, it is possible to keep the sitewide default locked up behind a permission, so that seems a good safeguard.

Not entirely sure what you mean by this. Anyone who can admin signups on a node can change the settings for that node. I guess you just mean that changing the site-wide defaults can be a more restricted operation? That's true, so the potential damage of retroactively changing defaults is only possible with elevated perms...

Another big problem with this patch is that there's no way currently to "opt-out" of it. With this patch, all new nodes start life with the "Use the site-wide defaults" -- there's no way to say "all new nodes should have their own limit of 10, unless I go out of my way to configure them to point to the retroactive/dynamic site-wide settings".

Plus, we'd have to do some pretty heavy lifting when you change the site-wide defaults (via batch API) to go through and update all the existing nodes pointing to the defaults, check the new limits, potentially open/close signups, etc, etc. Ugh.

I'm inclined to leave this out at this point. It's just a lot of additional complication for something of only dubious value to the UI (since in some cases, it's nice, in others, it's exactly what people don't want). Especially since there's now a working "Reset to defaults" button on each node/N/signups/settings page...

kentr’s picture

@#9

This does make the non-JS UI pretty painful again, but I think overall, the price is worth it, so we don't have to store lots of duplicates on sites that mostly want to use the same defaults on all nodes...

Agree that noscript gracefulness can be a lower priority.

@#11

Another big problem with this patch is that there's no way currently to "opt-out" of it. With this patch, all new nodes start life with the "Use the site-wide defaults" -- there's no way to say "all new nodes should have their own limit of 10, unless I go out of my way to configure them to point to the retroactive/dynamic site-wide settings".

Plus, we'd have to do some pretty heavy lifting when you change the site-wide defaults (via batch API) to go through and update all the existing nodes pointing to the defaults, check the new limits, potentially open/close signups, etc, etc. Ugh.

I'm inclined to leave this out at this point. It's just a lot of additional complication for something of only dubious value to the UI (since in some cases, it's nice, in others, it's exactly what people don't want). Especially since there's now a working "Reset to defaults" button on each node/N/signups/settings page...

Thanks for all your work. When someone needs additional functionality, they can sponsor a patch :-)

ln282’s picture

I'm excited about this, but I have a question-- given that the status options are sitewide, and setting the limit for any status to 0 makes it unlimited, I don't see any way to disallow a status for a particular node, and that would be key for my site.

Is the plan to conquer #227643: Allow each node to specify which status values should appear on its signup form with this too? Otherwise, could a number other than 0 could be used to mean "unlimited"?

Thanks for all your work!

dww’s picture

Good question. However, #227643: Allow each node to specify which status values should appear on its signup form doesn't necessarily have anything to do with *limits* per se, so I'd rather not try to fix both of these at the same time with the exact same UI, since it'd be nice if #227643 worked even without the signup_status_limit module enabled. But, that's definitely a very good thing to consider. Thanks for the reminder!

dww’s picture

Yeah, the more I look at these, they really need a completely separate UI (and totally different code). #227643-6: Allow each node to specify which status values should appear on its signup form still seems accurate. Furthermore, even if you've got limits enforced, if you have a node configured to use a global limit instead of per-status limits, you might still want to alter which status values are visible on that node's signup form. So, #227643 has to be completely independent of this feature, in code, schema, and UI. So, I have no reservations about plowing ahead on this without trying to fix #227643 at the same time. But, that was an excellent question, ln282 -- THANKS!

mlsamuelson’s picture

After considering the issues around the "use sitewide defaults" listed in #11 I think it would better be left out at this point.

dww’s picture

Status: Needs review » Fixed

Great, then I'm calling this issue fixed again. ;)

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.