More themeability

i'm marking this as postponed for now, since we're in the process of building the roadmap for the future of the module, i don't want to start tossing things in until we know where we're going :)

i definitely take another look at this when we have that squared away

a separate issue/patch for views integration is now here: http://drupal.org/node/106808

in terms of the themeability stuff -- yeah, it certainly needs help. however, the way it's done in this patch is evil. this patch puts a bunch of DB query logic directly into the theme functions, which is a bad idea. so, while in general, i'm in favor of making this module more theme-friendly, this particular patch needs a lot of work before i'd consider committing it. it'd be much better to do all those queries once and only once in hook_nodeapi() when we load a signup-enabled node, and stash the data into some array in the $node object. then, we'd write the theme functions to just look up the data in the $node.

Bumped from http://drupal.org/node/79331#comment-655357. See also http://drupal.org/node/78707#comment-655361

Bumping prio, too. I'd like to get this in before the 5.x-2.4 release.

Major advances in themeability are now in (or about to be committed) for 5.x-2.5:
#137609: Theme function for signup_build_signup_data
#243035: How to list all signed up users in a view??
#273072: signup_node_admin_page() needs a theme function
#290734: Cleanup, unify and fix hard-coded signup email tokens

Behold:

% grep "function theme_" *.(module|inc|theme) | wc -l
      16

;)

In 5.x-2.4, there were only 6...

At this point, the only really major part missing a theme functions is:
_signup_node_output()

The only other spots I see that could use a theme function (although not critical) are:
_signup_print_current_signup() -- mostly covered by theme('signup_custom_data_table'), but it'd still be nice to make this themeable.
signup_admin_page() -- mostly just glues a few forms together, but it'd be nice to give more flexibility here, too.

You want more themeability? Ok, here you are :-)

grep "function theme_" *.(module|inc|theme) | wc -l
21

Added:

• function theme_signup_signups_closed()
• function theme_signup_signup_anon_user()
• function theme_signup_signup_registered_user()
• function theme_signup_print_current_signup()
• function theme_signup_admin_page()

theme_signup_admin_page() is just an extraction of some lines out of signup_admin_page() but hey, now its themeable ;-).

 Stefan

@stBorchert -- thanks for taking this on -- great start. I'm going over this now. In general, I'm glad to see yet more flexibility and a lot of this is worth keeping. However, there are some problems with how this is done. ;)

A) too much business logic in the theme functions

B) generally, if a theme function is just outputting a form, there's not need for a separate theme function, since FAPI lets themers define theme_form_id() for any FAPI form.

C) some code style issues

D) some PHPdoc formatting issues
...

Anyway, I'm working on re-writing this now, stay tuned.

Oh, and the previous patch also displayed the signup form for authenticated users, even if they didn't have permission to signup. The form would fail, but it was still wrong to display it in the first place.

Anyway, try this on for size. I think it's just as flexible for themers, but involves much less business logic (basically none) in the theme functions, and generally makes for vastly simpler theme functions to override. Tested lightly, more eyes and tests would be welcome.

Thanks,
-Derek

Even more simple theme function by just using an empty string for the default 2nd arg to theme_signup_signups_closed() so we don't even have to test it with if(). ;)

Same simplification, this time for the cancel button for theme_signup_current_signup()...

Also, clarified a PHPdoc comment based on feedback from deviantintegral.

Finally, ripped yet more logic out of the theme functions by not calling drupal_get_form() from theme_signup_admin_page() but instead passing in those rendered forms as arguments.

Very much improved theme function architecture and style. +1

In more systematic testing, there are only two edge cases that seem a little weird, neither of which are actually introduced by this patch:

A) User foo has signup permissions, and signs up for node N. Admin later removes user foo from the role with perms to signup. foo then visits node N. What should foo see? Currently, the fact that they don't have perms to signup trumps all else, and they don't see anything about their current signup. That's probably what you want, although it's weird since they're still technically signed up, still get signup broadcasts, a reminder email, etc.

B) Site only allows signups for privileged roles, not just authenticated user. Anonymous user visits a signup-enabled node, and sees "Please login or register to sign up for this event." They login as a regular authenticated user, redirect to this node, and now they see nothing about signups at all. Perhaps that login text stuff for anonymous users should be conditional on if the 'authenticated user' role has the 'signup for content' permission. But, that's definitely for another issue, since this patch doesn't change this behavior at all.

Otherwise, I tried just about every possible combination of permissions and such I could think of, and everything is looking great. Pretty sure this is now RTBC, but if anyone else wants to give it a beating, please do. I gotta run to go teach a class right now, so I'm not going to commit right now, anyway.

Thanks, all!
-Derek

Point (B) from #13 moved here: #321463: "Please login or register" printed even if all auth users don't have signup perms

Patch looks great.
However,I removed param $node from theme_signup_signups_closed() because its not needed there (if a themer wants to display additional information, s/he can load the node object by using arg()). Or dou you want to leave it there for any special reason?

Re #13 A): I think it could be usefull to display the signup information to this user though he doesn't have the permission to signup.
Probably a new setting "Check if users who signed up but didn't have the permission to signup anymore should see their signup information and receive all signup related emails" could be usefull.
But then you need to specify eventually which emails to send to this users (it wouldn't be nice to don't send an email if the event is cancelled). Argh, not very simple...

 Stefan

edit: removing patch (see comment 17)

I was thinking of leaving $node in there for a few reasons:

a) the themer might want to print out stuff like "Signups closed: Limit (30) reached" which is trivial with the $node object, but more of a pain otherwise.

b) we've already got it, it could be useful, and it seems better to let themers have it directly instead of relying on arg() mojo. also, what if this theme function starts being used in a node list context, where arg() doesn't work at all?

but, if folks think it should go and make themers figure out how to get this data themselves if they need it, i'm ok with that, too. ;)

Ok, especially b) is a good argument for leaving this in.
I've hidden the patch in comment #15 so #47913-11: More themeability is the one to go with.
That leaves #47913-13: More themeability A) still open...
Do we want it to be an independent issue?

Yeah, #13-A definitely belongs in another issue, if it belongs in an issue at all. ;)

Committed #11 to HEAD. Yay! 5.x-2.5 is going to be a major leap forward in themeability for this module. I think I'm going to add an entire section to the CHANGELOG/release notes about all the new theme functions we've added. ;)