signup_node_admin_page() needs a theme function

Yup.

Hi.
Here's a very simple patch that moves all the code from signup_node_admin_page() to a new function theme_signup_node_admin_page().
I first tried to leave the main logic in signup_node_admin_page() but ended up in total confusion and unnecessary loops.

Stefan

Heh. Yeah, I looked at this function once and came to a similar conclusion, but felt so dirty about it that I didn't want to post such a patch as an admission of defeat... ;) Maybe this is the best we can do for now. But, I want to give it a little more thought before I subject the themers of the world to something like this.

Joking (and not so joking) aside, thanks for the patch. Always nice to see people who put code where their mouths are and genuinely try to help -- greatly appreciated.

Anyway, I'll give this another look before the 5.x-2.5 release -- which should hopefully be coming this week if all goes according to plan.

On further thought, I don't think this is a good idea as it currently stands.

I think I'd rather that signup_node_admin_page() did the SQL query itself, created an array of info about each signup for the given node, and then handed that array off to the theme function for display. It's not ideal, but I'm ok with the theme function being responsible for rendering 'signup_admin_node_form' and the 'signup_form' itself.

However, I'm also wondering about these related issues:
#275298: Order Signup Administration List
#243035: How to list all signed up users in a view??

Just like for #243035, it seems like it'd be great to allow the table of attendees to just be a view, instead of doing the query manually and then ignoring the results. However, using a view for this table is going to be really complicated since there's no good way to have form elements (those "Cancel signup" buttons) in a view. So, probably I should just forget about that part entirely, and leave this a hard-coded table. But, we should definitely fix #275298 by using tablesort_sql() and make both the username and signup times sortable columns. If we do that, then it becomes *VERY* clumsy and weird to have the tablesort_sql() in one function and the table rendering in another. :(

p.s. While I was waiting to submit this, I decided to commit a fix for #275298: Order Signup Administration List so that is indeed using tablesort_sql() now...

Ok, here's another approach, now that #275298: Order Signup Administration List is done. It's still a little wonky, and doesn't address having a view for the signup details table, but this still seems better.

a) The signup details table is entirely constructed and rendered in signup_node_admin_page() and passed as a string to the theme function.
b) Each part of that page is now rendered in a fieldset. Mostly this is done in form theme functions that already exist, so if sites don't like this, they can undo it. However, I think it makes the whole page more visually consistent, and I could see wanting to collapse the signup details table, especially if that starts getting huge.

See attached patch and screenshot.

Thoughts?

Hm, I was previewing my comment with a new patch as your last patch arrived.
Yours is looking much better :-) (especially the fiedsets and the way you are building the table).

I built the table in the theme-function but this requires signup_node_admin_page() to hand over an array with each $signed_up_user to theme_signup_node_admin_page().

As said above: yours looks fine and could be commited.

 Stefan

As mentioned in IRC, it would be good if the $signup_details_table could be a keyed array passed into the theme function, which itself calls theme('table'). Then the function can modify or reorder columns.

Other than that, looks good!

--Andrew

Thanks for the feedback. I agree that passing in the $header and (keyed) $rows arrays makes more sense, and provides even better flexibility. I tested reordering the columns in that table via a theme override, and it worked great. Therefore, I committed the attached patch to HEAD.