Posted by scottrigby on December 17, 2008 at 5:41pm
5 followers
| Project: | Signup |
| Version: | 6.x-1.x-dev |
| Component: | Views integration |
| Category: | feature request |
| Priority: | normal |
| Assigned: | dww |
| Status: | closed (fixed) |
Issue Summary
I added a views handler so we can 'manage signups' from a link. Patch against HEAD attached.
This is my first CVS patch that adds addtl files, so please let me know if it should be done differently :)
| Attachment | Size |
|---|---|
| signup-adminlink-handler.patch | 2.25 KB |
Comments
#1
Hi.
Patch looks good.
One minor note:
+ $node->nid = $values->{$this->aliases['nid']};+ $node->status = 1;
+ $node->status = 1; // unpublished nodes ignore access control
+ if (!node_access('update', $node)) {
There's one
$node->statustoo much :-)#2
This is a great first attempt, thanks! A few concerns:
A) The way you're handling node_access() is wrong. You're pretending the node is always published (twice), which is going to give faulty results if the node really is unpublished. Furthermore, you're not including some fields in the $node object that core's node_access() is using -- in particular, $node->format and $node->type. Plus, node_access() invokes a hook and is supposed to pass in a fully loaded $node object. So, any module that's implementing hook_access() which is expecting a field not present will get confused. Check out how views_handler_field_node_link_edit works from views/modules/node/views_handler_field_node_link_edit.inc
B) However, we actually don't care if the user can edit the node in question at all. ;) What really matters is if they can access the "signups" tab, and that requires either 'administer signups' permission, or 'administer signups for own content' permission if the node is owned by them. So, now that I think about it, (A) doesn't really matter in this case (though it's useful for your education), you really need to solve (B) instead and remove that call to node_access() entirely. The best way to do this would be for your handler class to implement the construct() method which adds the 'uid' from the {node} table as an additional field so you can use that in your render() method. If this doesn't make sense, just ask and I can post code demonstrating what I mean.
C)
signup_handler_field_signup_adminlink-- I'd prefersignup_handler_field_signup_admin_linksince there's no reason to arbitrarily smash those two words together into a non-word. For example, views usesviews_handler_field_node_link_editD) No need to capitalize "Signups" in the label and help text.
Thanks for getting this started, this will be a nice addition to the views support.
#3
Ok, here's a patch with option
B).#4
Better, but the logic is not quite right. This will incorrectly deny access if the user is an admin with 'administer signups' but not 'administer signups to own content' to all the nodes they own. The basic logic needs to be:
<?phpif (!(user_access('administer signups') || (user_access('administer signups for own content') && $node->uid == $user->uid)) {
return '';
}
?>
(although there's not really a $node object, obviously)...
#5
Like this.
#6
Great! Thanks dww & stBorchert ... will check this out asap - but dww, yep, this makes sense :)
#7
Ah, yes. Much better.
Good to know, you're so attentive :-)
#8
Rerolled so it still applies cleanly. Finally had a chance to test this. Mostly it all seems fine. The only thing I'm slightly worried about is the destination on the URL. That means that as soon as you submit any form at node/N/signups, you return to the view you were just at. That seems potentially confusing, especially since there are 3 different forms at node/N/signups you might be using. Not sure if this is a feature or a bug of the new handler. ;) I guess we could even have a handler configuration option to decide if you want the link to set the destination or not, although that might be hard to explain in the UI for average site admins setting up a view...
#9
I've read through the code and it seems OK. I haven't actually tried it, but my guess is that it's trying to model admin/content/node, where saving a node brings you back to the content list. I'd agree with it if there was only one form, but with multiple forms it's less usable.
I'm going to suggest removing the drupal_get_destination(), but what I think this really points out is that there shouldn't be three forms on one page. Is there a reason why subtasks aren't being used? I would probably do it like this:
Signups
View (content of Summary fieldset and user list fieldset, default subtask) | Add (Sign up another user form) | Broadcast
If this sounds good I'll open another issue about it. It would also mean that you could link from the view to any of the tasks with an appropriate destination set.
#10
Hrm -- interesting thoughts. However, there are some issues here:
A) "content of Summary fieldset and user list fieldset" -- that's already 2 forms right there.
B) Broadcast shouldn't be a subtask since there are many use-cases where users have permission to send broadcasts but not administer signups.
That said, I like the idea of moving "Add" to a subtask, and that might help part of the problem here. It should be fairly easy to add multiple views field handlers for those subtasks. Now that you mention it, we should have a view handler for the link to the broadcast page, too.
Do folks think it'd be too confusing if there was an option for the main admin page link that controlled if the destination was set or not?
#11
Forgot to mention, this will go in DRUPAL-6--1 when it's done, so roll patches against that. Thanks.
#12
I asked merlinofchaos for input on this basic problem, and he thought it wasn't insane to add a checkbox to the field handler admin UI to select if you want the destination behavior or not. Something like:
[x] Set view as return destination
(After submitting any of the forms on the signup management page, do you want to be redirected back to this view, or stay at the signup management page?)
...
#13
This needs to wait until the dust settles over at #349292: Reorganize signup node tabs.
#14
I just committed #349292 so this is active again... It definitely needs work, since the paths are all different now. ;)
#15
Here's 1 approach. A shared base class for most of the tabs, and child classes that implement the minor differences.
#16
Even better... merlin suggested I should just use a single field for this, with a selector for which tab you're aiming for. ;) Much nicer.
#17
Fixed the comment at the top of the views/handlers/signup_handler_field_signup_node_link.inc file, did some more testing, and committed to HEAD and DRUPAL-6--1.
#18
Automatically closed -- issue fixed for two weeks with no activity.