#529250: Allow users to change their profile email address while signing up only works if no one else is altering the signup form. However, if you enable signup_status, things fall apart. :( We only really properly handle a single additional submit handler (the one natively from signup.module), but we don't handle other submit handlers.

After a lengthy discussion in IRC with chx, doing this "right" would involve a lot of very complicated FAPI trickery.

I thought of a very simple alternative: forget the confirm form step, and just print out a message telling the user their profile was updated, to. So, the field still looks the same on the signup form. If they change the field, when they submit the form to signup (or edit their signup), they're presented with a drupal_set_message() about it, something like:

Updated the e-mail address in your profile to %new_value. If you want to change this in the future, you can edit your profile.

Thoughts?

CommentFileSizeAuthor
#16 534948-16.signup_confirm_email_fixes.patch12.34 KBdww
#10 signup-534948-diff.txt8.7 KBmlsamuelson
#8 signup-editables.png33.33 KBmlsamuelson
#5 534948-5.signup_confirm_email_fixes.patch12.29 KBdww
#5 534948-5.signup_confirm_email_fixes.1.png21.82 KBdww
#5 534948-5.signup_confirm_email_fixes.2.png29.93 KBdww
#5 534948-5.signup_confirm_email_fixes.3.png44.27 KBdww
#5 534948-5.signup_confirm_email_fixes.4.png39.76 KBdww

Comments

mlsamuelson’s picture

mlsamuelson commented

The alternative seems reasonable, given the constraints.

It would be great if you could make it a tad more robust by having a toggle in the admin settings so the user could choose between 1 of 2 actions:

1. The email address is updated and the message about that (with link) is displayed to the user.
2. The email address is _not_ updated, and a message is displayed noting that the email address used is not the one on the user's account, and probably they will want to change it since future communications may be delivered to that email (and link to their user page).

dww’s picture

dww
we/he/they
commented

I don't like the admin setting. If this field doesn't update your profile, it serves no purpose at all, other than generating a message that's quite easy to ignore/miss. Seems a bit weird to have a setting for that. ;)

However, there are ways we could add a confirmation "step" or sorts, without a completely separate confirm form. We could have a checkbox on the signup form that says "Update e-mail address in my profile". If the browser has JS enabled, this checkbox would be hidden by default on page load, and if the user changes the e-mail address field, the checkbox would be revealed. Without JS, the checkbox would just always be visible. There'd be a validation handler that ensured that if the e-mail address field was changed, the checkbox needs to be checked. So, that'd force the user to either not change the e-mail, or to check the checkbox to acknowledge that their profile should be updated.

Thoughts?

Thanks,
-Derek

mlsamuelson’s picture

mlsamuelson commented

I like the checkbox concept. You're basically building the confirmation step into the main form, so the full functionality returns and we don't have to rely on easy to ignore messages. :)

dww’s picture

dww
we/he/they
commented
Assigned: Unassigned » dww

Ok, cool. I'll work on that later today. I'd like to get this fixed soon since I should really roll an RC5 to fix #533970: Access denied to the 'signups' tab if the signup attendee list is not configured as a tab ASAP.

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
StatusFileSize
new 534948-5.signup_confirm_email_fixes.4.png39.76 KB
new 534948-5.signup_confirm_email_fixes.3.png44.27 KB
new 534948-5.signup_confirm_email_fixes.2.png29.93 KB
new 534948-5.signup_confirm_email_fixes.1.png21.82 KB
new 534948-5.signup_confirm_email_fixes.patch12.29 KB

Ok, this seems to do it. This implements the checkbox confirmation workflow I proposed in #2.

The only bummer this revealed is a core FAPI/theme problem where if you set a validation error on a checkbox, FAPI sets the "error" class on the form input itself, and theming checkboxes themselves is a huge pain. It'd be much better if core put the "error" class on the wrapper div around the entire form element (checkbox, title, description, etc). Better yet, it should do that on all form errors. But whatever... ;)

That aside, working on this revealed this other issue, which has a simple patch to solve: #536094: When editing a signup, if there are form validation errors, the JS shouldn't disable the form elements

So, here's the patch, and a series of screenshots demonstrating how it works when editing an existing signup:

534948-5.signup_confirm_email_fixes.1.png: Initial state of the signup edit form.

534948-5.signup_confirm_email_fixes.2.png: After clicking the "Edit" button (so form elements are enabled, and the "Edit" button turns into "Save" -- already exists), and starting to edit the E-mail address field (so that the "Update e-mail address in user profile" checkbox is revealed).

534948-5.signup_confirm_email_fixes.3.png: Immediately try clicking "Save", without confirming via the checkbox: validation error. Notice that the checkbox is immediately visible whenever it's the cause of a form validation error, even if you don't edit the "E-mail address" field again.

534948-5.signup_confirm_email_fixes.4.png: Select the checkbox and "Save" again: the edit worked, and you get a message telling you your user profile was updated.

Please give it a spin and let me know if you find anything.

Thanks,
-Derek

mlsamuelson’s picture

mlsamuelson commented
Status: Needs review » Needs work

I tested the patch in #5 and it works great. The only issue I'm seeing is the "Attendance" drop down is the only field that is disabled before the edit button is clicked. I can edit "Name," "Phone," and "Email address" prior to clicking the edit button...

I mentioned on #536094: When editing a signup, if there are form validation errors, the JS shouldn't disable the form elements having custom submit and validation handlers for the form on the install I tested with, but actually those are only applied to the signup_form, and this is the edit form, so I think we can rule those out.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review

@mlsamuelson: Are you sure you have JS enabled, and that you cleared your browser's JS cache and you cleared your site's JS aggregation? Works just fine for me in Safari and FF.

mlsamuelson’s picture

mlsamuelson commented
StatusFileSize
new signup-editables.png33.33 KB

Yes, cache issues shouldn't be a problem. I've cleared the cache and just tested with a brand new Drupal 6.13 install.

For Signup I'm using a patched version of DRUPAL-6--1-0-RC4.

I've attached a screenshot showing where I can edit.

dww’s picture

dww
we/he/they
commented

"For Signup I'm using a patched version of DRUPAL-6--1-0-RC4."

That sounds scary. ;) Any chance you can generate a diff between your signup tree and 6.x-1.0-rc4 official and post that here? (Or email it to me privately if it's got secret sauce you don't want publicly visible).

Works fine for me (and others), so I'm assuming it's something special to your setup.

Also, I'm pretty sure this is unrelated to this patch. Can you confirm if it works at all on a totally clean test site with nothing but 6.x-1.0-rc4 signup installed?

mlsamuelson’s picture

mlsamuelson commented
StatusFileSize
new signup-534948-diff.txt8.7 KB

Can you confirm if it works at all on a totally clean test site with nothing but 6.x-1.0-rc4 signup installed?

Actually, I can't. In #8 when I mentioned using a patched version of Signup - the patch I was referring to was the patch at hand. ;)

I've tested again with a brand new install of Drupal and a brand new DRUPAL-6--1-0-RC4 of Signup. The patch seems to apply fine.

Diff attached.

dww’s picture

dww
we/he/they
commented

Heh... I was wondering about that. ;) Ok, does the JS stuff work without *this* patch at all? I.e. clean D6 and RC4 -- if you just try to edit an existing signup, what happens?

mlsamuelson’s picture

mlsamuelson commented

Actually, without applying the patch I'm getting the exact same behavior. :D

Bad patch?

dww’s picture

dww
we/he/they
commented

If by "same behavior" you mean "I can edit those fields even without pressing the 'Edit' button", then no, the patch here is fine. It's some other problem, unrelated to this patch.

So, does the checkbox stuff here work as expected? If so, this should be committed, and we should open a new issue to figure out why you can edit those fields without hitting 'Edit' first...

Thanks for testing!
-Derek

mlsamuelson’s picture

mlsamuelson commented
Status: Needs review » Needs work

My apologies for getting turned around and losing sight of the original issue. :P

The checkbox behavior works as expected with the patch.

On applying the patch, however, the location of the Email field moves from the top of the signup form to below the Edit/Save button... Do you want to handle that here, or on another issue?

mlsamuelson

dww’s picture

dww
we/he/they
commented

Ahh, sure. Trivial fix for that. Here is fine. One sec.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 534948-16.signup_confirm_email_fixes.patch12.34 KB
mlsamuelson’s picture

mlsamuelson commented
Status: Needs review » Reviewed & tested by the community

Works great. Thx, dww!

dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

Great, thanks. Committed to HEAD and DRUPAL-6--1.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.