post "signup Broadcast" email message as new comment

this also needs a setting to enable/disable the functionality. i'd never want signup to automatically post all emails i send to people who signed up as public comments on my site. if you want this, it's gotta be off by default, with a way to enable it.

also, that's not a patch. please see http://drupal.org/patch and attach a real patch.

thanks,
-derek

that's closer to a real patch, thanks. ;) FYI: you should have set the status to "patch (node needs review)" when you posted it...

however, after review, it needs work:

0) the patch doesn't apply at all:
patch: **** malformed patch at line 13: + '#default_value' => true
so please try again.

1) please see http://drupal.org/coding-standards
- spaces vs. tabs, and indentation
- how to properly format arrays (trailing comma on every attribute, even the last one)
- TRUE not true
...

2) module_exists('comment') will tell you if comment.module is enabled. we already node_load() into $event, so you can just inspect $event->comment to see if comments are enabled on this particular event. should probably test for this and not even add this checkbox to the broadcast form for nodes that don't have comments at all (not just testing when we save the form).

3) + '#default_value' => true should be TRUE to match coding standards, but should really be FALSE to match my request above. ;) If you really wanted, you could add a checkbox to the admin/settings/signup page to control the default value of this checkbox, but i think that's overkill. again, this checkbox shouldn't be there at all if comments are disabled on the site or node.

4) there's a more fundamental, architectural problem with this patch. you're currently appending another comment after sending the email to *each* user who signed up. :( you really just want to add a single comment, i assume, otherwise this feature is insane. however, if you're only posting 1 comment, what do you do about all the tokens that can be put in the message like %username and %useremail? you probably want to setup a separate $trans array to *just* replace %event and only do that substitution on the message body before posting a single comment for the entire broadcast. a) this is private info and you don't want to publish it and b) it's only valid for each user, so it doesn't make sense to try to put in values to a global comment.

hope that all makes sense. let me know if you have other questions...

cheers,
-derek

much better, thanks. almost there. ;) 2 minor code style issues remain:

this:

   $form['savebroadcast'] = array(
   '#type' => 'checkbox',
   '#title' => t('Also save this message and display it as a comment.'),
   '#default_value' => FALSE,
   );

should be this:

   $form['savebroadcast'] = array(
     '#type' => 'checkbox',
     '#title' => t('Also save this message and display it as a comment.'),
     '#default_value' => FALSE,
   );

ditto with the $trans array in your patch.

also, this:

   $edit['subject'] = '[E-mail] - ' . $subject;

should be:

   $edit['subject'] = '[E-mail] - '. $subject;

(no space between the . and string literals).

i could just fix these myself, but if i tell you then you're more likely to get these things right in the future, saving time in the long run (especially if you submit patches for core).

thanks,
-derek

oh, and the comments you added need a little help, too:

  //jetstop add check box

i don't know what "jetstop" means. we can see we're adding a checkbox, too, so i'd just leave this out entirely.

  //add the sent mail as a new comment on the node 

should have a space after the //, and it should be a full sentence. (drat, i don't see this in the handbook, but i know we recently standardized the formatting of comments -- not sure what's up with that). something like:

  // Conditionally add the message as a new comment on the node.

thanks,
-derek

Not quite. ;) Still minor code style issues (I'd just fix them myself, but you have to learn somewhere, especially if you're trying to deal with core, so it's worth taking more time and telling you what you're missing so you do it right in the future)...

A) This needs more spaces:
if($form_values['savebroadcast']){

Should be:

if ($form_values['savebroadcast']) {

B) That whole block of code isn't indented properly (2 spaces, no tabs).

Thanks for being persistent,
-Derek

Just so you don't roll another patch that's not actually ready..

C) The array indentation is wrong, too:

    $trans = array(
    '%event' => $event->title,
    ...
    );

Should be:

    $trans = array(
      '%event' => $event->title,
      ...,
    );

D) Patch itself is invalid:

(Stripping trailing CRs from patch.)
patching file signup.module
patch: **** malformed patch at line 19: @@ -1702,6 +1710,26 @@

This is worthy. Rerolled for 5.x-2.5 with the requested fixes.

I prefer "email" but core uses "e-mail" everywhere so I changed it to that.

Thanks for taking another stab at this. However, some problems remain:

A) There are tabs or other formatting bugs in that patch.

B) Although signup's internal tokens are replaced, if the site has token.module enabled, those tokens won't be replaced. See #285626: Integration with the Token module for more.

C) You're hard-coding event.module support here by using _event_date(). :( You need to use signup_format_date() to get the right value for %time.

D) You should really test that $node->comment == COMMENT_NODE_READ_WRITE so that people can't post comments via signup broadcast if writing comments are otherwise prohibited on that node.

E) You should also test user_access('post comments').

F) We shouldn't hard-code this: $edit['format'] = 1;. We need variable_get('filter_default_format') in there.

Thanks,
-Derek

p.s. In the future, please don't set your own patches to "RTBC". ;)

Sorry about that. Setting it to RTBC was accidental. Your notes are great and following the steps you laid out is like taking an open book test. I hope somebody beats me to updating the patch.

This version of Signup is not supported anymore. The issue is closed for this reason.
Please upgrade to a supported version and feel free to reopen the issue on the new version if applicable.

This issue has been automagically closed by a script.

Reverting recent closing.