Make signup confirmation, cancellation messages optional for programmatic signups

What about the signup confirmation email that gets sent, too?

I haven't thought enough about it. ;) Seems like it should probably be the same parameter, but I'm not sure. What do you think?

It does seem wrong to rely on the per-node settings for programatic signups -- what if you want signup confirmation emails when people signup via the regular UI, but want to handle the emails separately if they signup programatically via other means?

In principle, #7 is fine. Smaller functions that each do one thing are better for all sorts of reasons. However, a few problems:

A) The "forwarding email" is the email that gets sent to event admins when someone signs up. This is the "Send signups to:" address when you open the "Signup settings" fieldset when editing a signup-enabled node. The description on this form element is: "Email address where notification of new signups will be sent. Leave blank for no notifications.". The emails look something like:

Subject: Signup confirmation for Date TZ Site: date_tz_site now

The following information was submitted as a signup for date_tz_site now
Date/Time: Thu, 07/23/2009 06:34

Username: Tokyo
Profile page: http://example.com/user/15

SIGNUP INFORMATION
Name: Tokyo
Phone:

So, I guess in your case, it makes sense to hold off on sending that until the signup is confirmed/paid/etc. But, just wanted to double check you understand what this is.

B) There are a lot of code style and PHPDoc formatting bugs in #7. Please fix the PHPDoc to properly document the expected arguments, especially for the new functions you're adding. E.g., instead of this:

+/*
+* Sends the signup comfirmation mail to a user is signed up for an event.
+ * @param user: The account object of the user being signed up
+ * @param signup: The signup array
+ * @node: The node object being for which the user is signed up
+ */

We need this:

/**
 * Sends the signup comfirmation mail to a user that is signed up for an event.
 *
 * @param $account
 *   The account object of the user being signed up.
 * @param $signup
 *   The signup array.
 * @param $node
 *   The node object being for which the user is signed up.
 *
 * @return
 *   The return value from drupal_mail().
 *
 * @see drupal_mail()
 */

Also, do we really need those 3 args for this? Doesn't the $signup object contain everything we care about from $account?

C) Please document $display_message in both functions where you added that as a possible argument. And, given the scope of what that argument does, we should probably change the name. Not exactly sure what to call it -- maybe something like $notify_user?

D) Yes, we should use $account not $user everywhere possible in here.

E) Other indentation/whitespace bugs, e.g. return drupal_mail(...) with two spaces.

F) Seems like signup_confirmation_mail() should be called signup_send_confirmation_mail(). Ditto signup_send_forwarding_mail().

I'm about to cut RC4 (sometime this week), so if you'd like to get this in, time is of the essence. ;) Sorry I've been MIA -- I was traveling for a while, sick, etc. But, I'm plowing back into signup for a client, so there's going to be another flurry of activity...

Thanks for the re-roll. A few lingering problems:

A) + watchdog('ezra', 'conf got'. dprint_r($signup, TRUE)); ;)

B) It's @param $notify_user, not just $notify_user in PHPDoc

C) It's @param $signup, not @param signup

D) The description of each @param should end with a period.

E) $notify_user still isn't documented for signup_cancel_signup().

Cheers,
-Derek

p.s. Totally off topic, but interested in giving #529250: Allow users to change their profile email address while signing up a test drive? ;) Thanks!

Almost there.

10.D) still isn't fixed

F) You removed $from = variable_get('site_mail', ini_get('sendmail_from')); from signup_sign_up_user(), which is fine. However, you only re-added it to signup_send_confirmation_mail(), even though $from is used (uninitialized) in signup_send_forwarding_mail(). Honestly, since we're not doing anything special, we should just rip this out entirely and let drupal_mail() use the default $from itself in both cases.

G) signup_sign_up_user() checks $user_mail to decide if it should send the confirmation email:

+    if ($node->signup_send_confirmation && $user_mail && $notify_user) {

However, you also removed the code to initialize $user_mail. :( So, in the $notify_user == TRUE case, this isn't sending confirmation emails at all. At least you initialize $user_mail in both signup_send_confirmation_mail() and signup_send_forwarding_mail(). ;)

Seems like we shouldn't test for this at all in signup_sign_up_user() when deciding if we should invoke either helper. Instead, both helpers should probably verify they got a real value for $user_mail, and abort if not.

H) PHPDoc blocks need to start with "/**" to be valid PHPDoc comments, not just "/*"

I) The description for @return should be indented the same as for @param:

+ * @return
+ *  The return value from drupal_mail().

Should be:

+ * @return
+ *   The return value from drupal_mail().

If you don't re-roll, I'll fix these before RC4 and commit (once I've had a chance to actually test). But, just wanting to train you on proper patch technique, which will serve you well in the future. ;)

Thanks!
-Derek

I also noticed:

J) $node_type_name is used by both helper functions but not properly initialized.

Attached patch fixes all the known issues, D through J. Tested in the usual signup case, and works fine. Haven't tried testing programatic signups, yet. @ezra-g: wanna give this a spin before I commit and ship RC4?

Thanks,
-Derek

Whoops, missed 10.D on signup_send_confirmation_mail().

Based on an IRC chat, we decided to leave the test for if ($node->signup_send_confirmation) { out of signup_send_confirmation_mail() and leave that the responsibility of the call sites. It's certainly possible some callers will want to force a confirmation email for some reason, even if the node isn't configured to do so...

Also, cleans up the PHPDoc a wee bit more...

Ezra tested this and was happy. Committed to HEAD and DRUPAL-6--1.