allow users to edit their own signup

ok, lemme rephrase:

without completely re-writing the signup module to rely on the forms API and the survey module, i think adding the ability to edit an existing signup is a good thing. ;)

i'm willing to take a look at survey and see if we can use it for the signup form, but i suspect that will require a fairly major redesign of the signup module, and i don't think we're going to want to do anything that major in 4.6. i really need this functionality for my site, and my site's going to be 4.6 for the foreseeable future.

if i have to just hack around in the dark and get it working for myself, i can, but i'd rather get some input on how to do it, instead. ;)

thanks,
-derek

also, since 4.6 signup has been so successful and many people are using it,

from a software development standpoint, the fact so many people are already using the 4.6 version and the fact that drupal has no support for module versioning other than "4.6" vs "4.7" (ugh!), it seems like a bad idea to do massive re-writing, re-organization of the 4.6 code base.

yes, lots of us want things from 4.6, but i think that a complete re-write of the module to a) make it support plugins and b) use the forms API can't happen in the "stable" version. it's going to enrage all the folks currently relying on the existing (albeit limited) functionality in 4.6 when we start radically changing how it works, how it looks, what it depends on, etc, etc.

so, i think we should make small, easy, useful enhancements and fixes to the 4.6 version, and save the heavy lifting for 4.7.

the only other choice i see is to fork this module (via a cvs branch off of DRUAPL-4-6) and make a new "4.6-form_plugin-beta" version (or something) where we can do all of this reorganization. i'm not sure how easy it'd be to get that listed on the signup project page, but that seems like the only good choice.

i'm not crazy about the idea of supporting two completely different codebases for the same module, and that's what i see happening if we start hacking in individual features to 4.6.

well, that's what we get for maintaining drupal modules at all, given that the core API radically changes with every release. i'm not passing judgement on that, i'm just saying that maintaining N code bases is a fact of life for drupal modules (at least from what i can tell). perhaps this could be made easier by making more use of cvs functionality... i.e. actually merging the DRUPAL-4-6 branch back into the cvs HEAD periodically, instead of always making changes in both places. yes, many of the changes to 4.6 will conflict when we merge them into 4.7, but again, that's the price we pay given the drupal core API changes.

this looks interesting and relevent:

Backporting Forms API to 4.6. Experimental migration feature

something to consider for signup?

we'd basically freeze 4.6 except for very trivial, tiny new features or bug fixes.

we put all our plugin and forms API work into the 4.7 version. the 4.6 forms API backport module mentioned above could allow folks to use the 4.7 signup in a 4.6 site (especially if we're careful not to use too many fancy new forms features that won't work in 4.6 compatibility mode, but it sounds like that's not what you're talking about, anyway). all the plugin-related changes could be 4.6 vs. 4.7 neutral.

existing 4.6 signup users can decide if they want to stick with the tried-and-true, stable 4.6 code, or migrate to the flexible 4.7 signup code w/o upgrading their entire site to 4.7. if we *really* want some 4.7-specific functionality, we can have it conditionally enabled via some setting, so 4.6 sites can still use the 4.7 signup in compatibility mode by disabling the feature.

thoughts?
-derek

signup 4.7/HEAD makes use of hook_form_alter, and it looked to me like that wasn't something that the backport module supported.

the guy said the module was still in beta, and was asking for help. if we can put a few hours into making that module better and that will solve 95% of our problems, i'd be thrilled. if we could basically freeze the 4.6 version, have 4.6 sites install the compat. module and our 4.7 version of signup, we're set.

i'd like to help folks out who aren't planning on migrating to 4.7 anytime soon

that's why this backport module could be so cool... we could do both.

now that a) FAPI is an established fact of life, and b) 5.x-2.* is open for new features, this issue is active again. ;) i might even work on it in the near future, but i'm not yet ready to assign it to myself.

I'm going to work on this in the near future, since it's important for #330943-10: Drupal 6.x port of signup_status.

One question that immediately comes to mind as I work on this: where should this live?

A) Another subtab at node/N/signups/edit. Might be more familiar, along the lines of the "Edit" tab on nodes. It might also get confusing for exactly the same reason. ;) Plus, this only really makes sense to edit your own signup as a subtab, not arbitrary signups.

B) A separate menu callback somewhere else, e.g. signup/edit/%sid with just a little "Edit signup" link under the user's current signup info (e.g. next to their "Cancel signup" button if they have one). This would make it easier to give an admin the ability to edit arbitrary signups via the same menu item, instead of either having to use node/%/signups/edit/%sid or something.

Any thoughts would be welcome...

Thanks,
-Derek

This patch adds:
- 'edit own signups' permission (users that can 'administer signups' on a node can edit any signup on that node)

- new menu item at /signup/edit/%sid

- form for editing an existing signup -- it ended up being a lot easier to just have a whole new form for this instead of trying to reuse the signup_form. I included both "Update" and "Cancel signup" buttons (if the user has perms to cancel their signup, of course).

- menu object loader for loading signup objects

- access method for seeing if the current user can edit a given signup

- an 'Edit signup' link under the signup info for any user who can edit their own signup (sets a destination back to the page where you were viewing the signup once you submit).

- a 'Edit signup link' view field for generating links to edit signups in views. lets you customize the text of the link, only renders the link for users with permissions to edit that signup, and sets a destination back to the view after you update the signup.

The only things I'm not sure about:

A) Do we want a new DB field in {signup_log} to record the last time a signup was modified?

B) Should we invoke a hook when a signup is edited? If so, when/where/what should that be?

C) The UI is a little wonky when viewing a signup (see attached screenshot). Even wrapping the "Cancel signup" button and the "Edit signup" link inside <div class="container-inline">, they still show up on top of each other, instead of next to each other. :( I think something about the <form> itself doesn't like being set to display: inline. I hacked in some float/clear in the theme_signup_current_signup() function, and it sort of worked, but still looked really ugly since the form seems to have a bunch of padding/margin on the top. I'd love for this to look nicer, but I don't have much time. Any suggestions to make this look nice without totally kludgy markup would be appreciated. Since it's all in a theme function, I don't think it's the end of the world to ship it like this, but I'd rather not.

Oh, the other thing I didn't do was:

D) setup a way for anonymous to edit their own signup using a URL token, like they get with the signup cancel link.

That seemed like a fair bit of additional complication. Not sure if it's worth stuffing that in here now, or just dealing with it later in a followup patch if someone's really motivated to do so. ;)

Right, I was thinking we could reorganize all this code so that instead of it being just the "cancel" form, it should be something like the 'signup_self_operations_form' and it was smart about the perms and rendered each item as appropriate, etc, etc. Might be the only way to make this look clean, but does raise the complexity a bit. I'll ponder...

Meanwhile, any other thoughts on the UI, functionality, or code?

Thanks,
-Derek

Thanks for taking time to try the patch and comment on it. Those are all good questions/suggestions.

In terms of a confirmation form when users cancel their own signup: #361073: Confirmation form for canceling your own signup ;)

In terms of just using the signup edit form as the way to view your existing signup, that might be cleaner all the way around, and a good solution to the context problems you rightfully raised. However, here were the reasons I did it the way I did in that initial patch:

A) Core usually (always?) puts view and edit on separate pages, e.g. for nodes. I was just trying to be consistent with that practice.

B) Not everyone will have permission to edit their own signups, so it seems potentially weird to present their signup in a form without an "Update" button and/or with all the values greyed out and un-editable. Maybe that's not such a problem...

C) If we always use the form to display a user's existing signup, it means we're going to basically completely remove a series of theme functions related to displaying the signup info to the user -- sites that have customized this will have to redo all that against a form. OTOH, maybe a form will look so much better and be more usable that it won't need so much customization. ;)

D) This makes it possible (although the patch doesn't do it yet), to give anonymous users the ability to edit their own signup at signup/edit/[sid]/[token] like the cancel-your-own links I added at #325237: Allow users (even anonymous) to cancel signup via secure URL.

E) Using signup/edit/sid for this form makes it easy to reuse the identical form for admins editing another user's signup.

I also don't think it's a problem to have the cancel-your-own button show up in multiple places, but that might be a moot point.

But, I totally agree the lack of context is unacceptable and needs work. I also think there might be some nice usability (and appearance) wins from using the form to display the current signup values. And, it'd solve the layout problem with getting the two buttons to appear inline since they'd both be rendered by the same form. ;) But, we'll probably still need to keep signup/edit/[sid] for admins editing another user's signup (and the future possibility for anonymous to edit their own), and in that case, we'll need to add context anyway...

So, this clearly needs work. It really needs a little more design thought on what direction to go in. I can see benefits and problems from both approaches. Input welcome.

Ok, I took a stab at displaying your current signup info via the edit form. It wasn't as bad as I thought. The patch seems a lot bigger than it really is since this change allowed me to wipe out 3 kind of crazy theme functions, a few helper functions, and some other junk we no longer need. There are only 3 minor UI problems I can see, but I wanted to post this for folks to play with it and provide feedback on the new approach:

A) The "Update" and "Cancel signup" buttons are smooshed together when the signup edit form appears in a fieldset directly on the node (at least in garland). Not sure why, I haven't tried debugging it.

B) There's still support for signup/edit/[sid]. If you visit that page as the user of the signup you're editing, it just says "Your signup information" instead of providing context (e.g. what node you signed up for). I don't want it to always contain the node title since that'll be redundant for end-users viewing their own signup info directly on the node or at the signups/signup tab.

C) There's no page title for signup/edit/[sid].

And, there's still no support for anonymous to edit their own via signup/edit/[sid]/[token]. I think I want to just handle that via a separate issue once this lands.

Even though this has a few minor known problems, I'm going to call this CNR. ;)

This fixes (A), (B) and (C) from #21, along with some other cleanup and a few other problems in the previous patch:

D) Documents the new permission in INSTALL.txt.

E) Allows admins to edit the email address used to identify anon signups.

F) If you navigate directly to a signup/edit/[sid] page (without a destination set) and cancel the signup, you are now redirected back to the node the signup was about automatically, instead of staying on the "page not found".

G) Reorganized a bit of code to be more sharable.

I'm not thrilled about the solution to (A), but I don't know what else to do with it.

"no need of altering the signup details display table"

see my new comment at #209954-7: Allow other modules to alter signup data before it's displayed.

"I wander if there is a way to display the signup fieldset expanded if it is referenced in the url with the anchor tag 'node/NID/#signup'?"

This should go in a new issue: #362133: Expand the signup fieldset when viewing nodes if the #signup anchor is used? and not hold up this patch.

Any final UI or code objections before I commit this?

Thanks,
-Derek

At yoroy's request in IRC, here are some screenshots to demo the patch. All of these assume you have the signup form showing up embedded on the node itself, as opposed to a separate tab. Things mostly look the same, although when it's on a separate tab there's no fieldset and the fieldset title shows up as a <h4> heading over the form, instead.

51226_edit_signup.before.0.png: Before the patch, before you signup for a node (same before/after)
51226_edit_signup.before.1.png: Before the patch, once you've signed up
51226_edit_signup.after.1.png: After the patch, once you've signed up (if you have perms to edit)
51226_edit_signup.after.1-no-edit-perm.png: After the patch, once you've signed up (no perms to edit)

Changes since #22:

A) Fixed the text for the drupal_set_message() when you update a signup to be "Signup information updated."

B) Changed the button text to read "Update signup" instead of just "Update" to help improve clarity.

C) Now with jQuery bling! ;) (thanks to ksenzee for the suggestion, and quicksketch for the help implementing it):

When you first load the page to view an existing signup, all the signup form elements are disabled and the "Update signup" button is renamed to "Change values". If you click "Change values", the form elements are enabled and the button changes back to "Update signup". The cancel button works as it always did (though see #361073: Confirmation form for canceling your own signup for a nice patch about that). If there's no JS, this just degrades to basically what we had before.

Attached is the patch, plus some screenies to demo the new JS functionality. I think this is pretty close to RTBC now. ;)

Ok, based on further discussion in IRC, we decided just "Edit" vs. "Save" would be best.

BTW, here's a screenshot of the edit form when an admin edits another user's signup. There's no JS loaded here, since someone already clicked on an "Edit signup" link and knows what they're doing.

Committed #32 to HEAD and DRUPAL-6--1. Yay. ;)

Minor UI follow-up patch: Adds an "Operations" column to the default admin views for node/N/signups/admin with the "Edit signup" link for each signup. Patch and screenshot attached. Thoughts?

Oh, and another one -- if an admin is editing an existing signup, allow them to record/edit attendance, too.

Committed #35 and #36 to HEAD and DRUPAL-6--1.

Also committed a change to reorder the options when editing attendance so that "Attended" comes before "Did not attend", and change the other option to "- Not recorded -" as a visual cue that it's different from the "real" ones. Thanks, greggles, for the UI feedback.

FYI: Just committed http://drupal.org/cvs?commit=166685 to fix one other minor UI bug from the earlier patches.

At #363238-2: Release signup 6.x-1.0-rc4, Miglius points out a bug in this code: before granting access to edit a signup, _signup_edit_menu_access() should enforce that the user has access to view the node. Should be an easy patch once I've had a chance to sleep. ;)

Cleaned up the menu access callbacks for both edit and cancel so they share a single function (_signup_menu_signup_access($signup, $op)) and added all the appropriate logic there to check node_access('view'), etc. Works great in all my testing. Anyone else wanna take a look before I commit?

Thanks
-Derek

Whoops, missed a call to _signup_edit_menu_access() in the view handler for the signup edit link. This is better...

After a bit more testing and review, committed #42 to HEAD and D6--1.