Closed (fixed)
Project:
Signup
Version:
5.x-2.4
Component:
Views integration
Priority:
Normal
Category:
Feature request
Assigned:
Reporter:
Created:
5 Apr 2008 at 16:17 UTC
Updated:
28 Oct 2008 at 00:02 UTC
Jump to comment: Most recent file
Comments
Comment #1
dwwGiven how views1 works, this is really hard. You're really trying to make a view of users, not nodes, and views1 doesn't handle that. views2 will make this trivial. Meanwhile, you're pretty much out of luck. I'm not sure it'd even be possible to make this work in views1. The only way it *could* work is if you were using a users-as-nodes solution, and signup had a way to map signup users to their corresponding nodes. :(
Comment #2
dwwActually, you can do this in views1. Sorry about the confusion. Here's the view that's used on groups.drupal.org for the "Attendees" tab on event nodes:
Feel free to give that a try, and let me know if it's working for you, too. If so, I could add something like this as another default view provided by signup.module.
Cheers,
-Derek
Comment #3
socialnicheguru commentedsubscribing
Comment #4
dwwDue to a limitation in how views works, this won't be perfectly slick just from the views UI. The problem is that we only want this tab on signup-enabled nodes, and there's really no way to specify that with the views UI itself. So, I think we're going to need a wee bit of custom menu code in signup_views.inc and a few settings to control this menu item. It'll just render the view you still define with the views UI, but you don't want to specify it as a menu item via the views UI. I'll take a quick stab at this and see how far I get.
Comment #5
dwwWork-in-progress, still needs some work, but this is the basic idea. You can use it with the exported view attached with the patch. I'm convinced there's gotta be a better solution for this, but that'll require some input from the views gurus.
Comment #6
dwwYay! As hoped, merlinofchaos provided a viable solution to this via the views API: hook_views_url_tokens(). So, we can just define a $signup URL token, that will match with any signup-enabled node. Then, the URL for this view can be something like node/$signup/signup-list, and it'll all work as expected without any of the previous custom menu code. phew!
Comment #7
moshe weitzman commentedFYI, there is an issue with breadcrumbs and that technique. see http://drupal.org/node/183191
Comment #8
deviantintegral commentedI've implemented a patch which uses the views API as suggested. I've also attached a slightly modified view that includes a menu tab, and sets the view title to that of the node. It's working as expected on my test site, only showing the menu tab on signup-enabled nodes.
--Andrew
Comment #9
dwwThanks, that was a great start. I rerolled for the following:
A) Tweaked the default view for this (made both fields sortable, removed the CSV view arg since views bonus can't be assumed, capitalization of the menu item, etc) and added it to _signup_views_default_views().
B) Removed the unnecessary patch against signup.module that was a hold-over from the previous attempt.
C) Made the code that manually displays the list of users signed up for a node into a theme function, theme_signup_user_list(). Otherwise, if you turned on this view, there was no good way to disable the hard-coded display of this info.
However, I've got a few questions about (C) for consideration before I commit this:
C.1) Currently, I put the logic to query the DB into the module code and the theme function just formats/displays the results. Normally, that's good in the separation of logic and presentation sense. However, in this case, one of the most common implementations of the theme function will become
return '';. ;) So, it's dumb to do the 2 DB queries if we're not going to display any of the data, anyway. Should I move the queries into the theme function?C.2) Should I move the theme function into the signup_no_views.inc world and if views is enabled, don't invoke the theme function at all and have the "Signup list" tab appear by default? Currently, that view is disabled by default.
Any thoughts on the best approach here would be welcome. Thanks!
-Derek
Comment #10
stborchertRe C.1): I don't see where it returns an empty string. If a user doesn't have the proper permissions the function is never called. If you have the permission to view all signups it always return a table with at least one row of data.
So I think there is no need to move the queries out of
_signup_node_output.Or am I watching the wrong lines?
Comment #11
stborchertTo clarify: we are talking about these lines:
I'm some kind of confused now :-)
Comment #12
dww(I know I said I was going to bed, but I'm still wrapping up a few things and noticed this comment...) ;)
What I meant with the reference to
return '';is that, given patch #9, any site running views (the vast majority) that wants to use this view for the signup user list instead of the hard-coded stuff is going to want to put this in their copy of template.php:In other words, completely remove the table, since we've already got the signup user list on another tab (or wherever the admin puts it) thanks to views.
Make more sense? In this (IMHO very common use case) that means we're doing these two extra queries for nothing (except the feeling-good-about-ourselves-for-not-putting-SQL-in-theme-functions beauty of the code).
So, this makes me think we need something else that decides if we even want to invoke this code at all:
-
if (module_exists('views')) { ... }- a setting at admin/settings/signup
- ... ?
It's not clear if we should conditionally perform the queries but always invoke the theme function to give sites a trivial way to embed the view output at the same spot, or if we should only invoke the theme function if we already performed the queries. Maybe we want two theme functions -- one if we're doing the queries (no views) and need to render the table, another if views is present and we're not doing the queries.
No matter what, I think patch #9 can't be committed as is, since it's sort of the worst of both worlds. ;) Marking CNW.
Comment #13
stborchertAh, ok. Now I understand what you've meant.
The cleanest (and simplest) way would be one theme in signup_no_views.inc and one in signup_views.inc in my opinion.
Then the user (themer) can decide to display the list no matter if views is enabled or not. Default for views enabled is not.
Patch attached...
Comment #14
dww#13 still has the problem that in the overwhelmingly common case (site will use the view for the attendee list) we're doing those two DB queries for nothing, which is bad for performance. That's the big point I'm trying to make -- I want to turn off those queries when they're not needed, but I'd rather not "turn them off" by having them directly in the theme function. Does that make sense?
Comment #15
stborchertHey, you're still awake! :-)
I know, the DB queries would be unnecessary in this case. But what if a user has views enabled and wants to display the information additionally at the bottom of the node? How do we know when to query the data?
A simple
if (module_exists('views'))would not work because in this special case the data must be queried although. If there were a "reverse flag" saying "theme_signup_user_list() ist overwritten" it could be done but without it...The only option I see would be putting the queries into the theme function. But this is not really the way we wanted it to work, isn't it?
Comment #16
dwwOr a global (advanced) setting for the admin to specify if they want to use a view for this or not... That's what merlinofchaos suggested to me in IRC when I asked him for advice on this problem about a week ago.
Comment #17
stborchertWell, I added the option to control where to display the list to the advanced settings.
Now the admin can decide whether to show it on a tab, at the bottom of the node or both. The options are only available if views is enabled. If views isn't enabled the list is always shown at the bottom of the node (depending on
theme_signup_user_list().Comment #18
deviantintegral commentedNot tested yet, but I like the current solution. I'll run through it later today.
Comment #19
stborchertHere's a screenshot of the new setting: http://i37.tinypic.com/xnujaf.jpg
Comment #20
deviantintegral commentedI can't seem to make the options work. It looks like the actual value set is never used, and that it's only being tested a true / false variable? There is:
if (user_access('view all signups') && $display_list) {But $display_list is never checked to see if it is 1 or 2. The view is always displayed on the tab, and never at the bottom of a node. Is there something I'm missing?
Also, in the help text, I think Views is a name and should be capitalized: "If Views is enabled a view...".
--Andrew
Comment #21
stborchertThere is a new option on [admin/settings/signup] to set this.
See screenshot above.
Stefan
Comment #22
dwwThis needs some help. I'm working on it right now. Sadly, the really slick thing I was hoping to be able to do seems impossible in D5 views, so that might have to wait for the D6 port. I'll explain more once I'm done chatting w/ Earl. Just wanted to "claim" this issue so no one else spends time either reviewing/testing #17 or working on a new patch. Stay tuned...
Comment #23
dwwHuzzah! Mammoth patch for what should be an easy feature... But, I was on a learning experience binge and got carried away trying to bling the UI with jQuery... ;)
Obviously, you only get option #2 if you have views enabled.
Phew! I've tested this pretty heavily now in all the cases I could find, but more eyes and testing would be most appreciated.
Thanks,
-Derek
Comment #24
stborchertAwesome!
It looks really (really, really!) great.
I think this can be committed (and doesn't block #293005: Create a signup 5.x-2.5 release anymore).
Stefan
Comment #25
dwwCommitted to HEAD. Yay. ;)
Comment #26
deviantintegral commentedThis is quite nice, and works fine for me. I'm going to have to steal your jquery code for the conditional options - that's something that my users would really like for many of their forms.
I think the Views Page / Block option is a little confusing, but I'll file a new issue with a help text patch.
--Andrew
Comment #27
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.