[D7] Webform Email Reply

Hi there, cool idea. This is very useful bit of functionality, will be a big benefit to the community. I didn't find anything too crazy, should be pretty easy for you to correct. Let me know if you need assistance with these.

One tip, make sure you're developing with error reporting turned on. There are a few obvious things that I'm sure you would have caught had you been notified.

README

The README.txt file has some weird line wrapping action happening. This isn't a big deal, mostly just style preference. I recommend simple paragraph and list type formatting.

webform_email_reply.module

1. Ventral.org is complaining about the use of the @author tag in the doc block at the top of the file. Maybe kill it.
2. webform_email_reply_help(): You've got an undefined variable $return_value. In cases where the switch statement doesn't match, is where you run into this. There are a few ways you can fix this, just make sure the variable is initialized before you reference it.
3. webform_email_reply_theme(): You've got some undefined variables here, too. I recommend setting "node" to NULL, and "submission" and "replies" to empty arrays. These are default values, so the idea is to give them default values that will keep things from breaking in case they're not set. The variables you have don't give sensible defaults and are erroring.
4. webform_email_reply_theme(): I also recommend directly returning the array instead of first assigning it to a variable, since it's simple and there's no reason to further manipulate it.
5. webform_email_reply_permission(): You can directly return the array here, too. No need to assign to a variable then return the variable.
6. webform_email_reply_form(): Not sure why you're using confirm_form() here, there's really no need. IMO it's detrimental because it sticks an empty div in your markup, #edit-description.
7. webform_email_reply_form(): You're sticking the webform submission into the #suffix of the form. This is okay, I guess, but it is going to make it difficult to alter the form later if anyone tries to do so. A better solution would be to put it in it's own element in the form. A fieldset or markup type element should be fine. Check out the FAPI for details.
8. Also, I was testing locally so I can't verify if the email went through. A brief review of your email dispatching looks like it should be working.

Nice job, and thanks for your efforts.

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hello,

This is a great contribution. I couldn't find anything major either. A few small things:

In your readme on line 22 "bee a" should be "see an". It could possibly be a little more descriptive also.

In your module, on line 56, 66, 183 I think the text should be inside of a t() if they are visible to the user so that they may be translated if need be.

Hi,
Please remove ventral.org bugs

FILE: /var/www/drupal-7-pareview/pareview_temp/webform_email_reply.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
56 | ERROR | Do not use t() in hook_menu()
66 | ERROR | Do not use t() in hook_menu()
--------------------------------------------------------------------------------

file - module
line 291

--This query allows node access control bypass, so it's a security flaw.
See https://api.drupal.org/api/drupal/modules!node!node.module/group/node_ac....
In your case just use addTag('node_access').

Thanks

..

Changing status to needs work

Hello again, there's only really one thing I think that's holding this up and it's the usage of webform_email_reply_exists(). Let me know if you have any questions or would like to discuss.

1. webform_email_reply_exists(): This gets called multiple times per request on the "view submission" page. Use drupal_static() to load it once and hang onto it. Be cognizant that it could be called with various parameters, so you'll need to handle results in some way that you can store values for unique combinations of parameters and get them back out with said parameters.
2. webform_email_reply_exists(): This function is actually loading the data, not just seeing if it exists. Maybe think about renaming it so it better describes what it does. You could split into 2 functions--one that loads the results as an array (even an empty one), and one that checks that loaded data to see if replies exist.

A few other things, not critical but worth looking into. I think we can RTBC either way:

1. webform_email_reply_access(): The first conditional that checks for $account is not needed. It will never exist and will always default to using the $user global.
2. webform_email_reply_access(): When computing $reply_own, I'm not sure why you're checking for data in $_SESSION. Everything should be computable through $submission.
3. webform_email_reply_form_submit(): Sending emails can be a time consuming task and you're iterating and sending to each recipient individually. Probably not a big concern in most cases, but worth thinking about how to handle in larger cases.
4. webform_email_reply_previous(): Think about using Drupal's format_date() instead of date(). That way the user will be able to configure it and it will be consistent with other dates on the site. Look at dblog module, it uses the "short" date format on the "Recent log messages" page.

Overall it's looking pretty good. Almost there. Thanks.

Paul,

For drupal_static(), either give it a unique name/key for each combination or use a multi-dimensional array to store the values.

Unique name

Just creating a string that will be unique for this combination of parameters.

  $previous_replies  = &drupal_static(__FUNCTION__ . $nid . "_" . $sid);

Multi-dimensional Array

  $previous_replies  = &drupal_static(__FUNCTION__, array());

  if (!isset($previous_replies[$nid][$sid])) {
    // Load from db...
    $previous_replies[$nid][$sid] = $result;
  }

  return $previous_replies[$nid][$sid];

Return Value

Also, I mentioned in #10 but it was kind of a passing thought but it may be best to have the webform_email_reply_get_replies() function always return the array of results--even if it's an empty array. I think it's going to suit you best to let an empty array indicate no results instead of a sentinel (NULL), then let the calling function deal with counting results to determine if empty. In the case of this function, you're going to have trouble determining if the value in drupal_static() has yet to be loaded or if the value has been loaded and is NULL.

Let me know if you have any other questions. I think you're really close, so get it buttoned-up and I'll check it one last time to get it RTBC'd so the project review team can get to it.

added final sentance

Hi Paul. I'm going to recommend one more change but it's minor. IMO this is ready to go.

webform_email_reply_exists(): Lose the conditional at the bottom and let the function return $previous_replies. If the result set is empty, it will return an empty array so no need for a check. No functional difference, just a couple of unnecessary lines.

If you haven't already, I would recommend getting the review bonus by reviewing some of the other projects that are pending review. This will expedite the approval process so you can get this thing out to the world.

Thanks for your contribution and good luck!

manual review:

1. webform_email_reply_previous(): should use theme('user_name', ...)?
2. Notice: Undefined variable: from in webform_email_reply_form_submit() (line 229 of webform_email_reply.module).
3. webform_email_reply_previous(): this is vulnerable to XSS exploits. If I enter a reply message with the content <script>alert('XSS');>/script> I get a nasty javascript popup on the Reply to submission page. You need to sanitize user provided text before printing, make sure to read https://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
4. webform_email_reply_form(): do not document $form and $form_state, see https://drupal.org/node/1354#forms
5. "'#value' => 'Send',": all user facing text must run through t() for translation.
6. project page is a bit short, see https://drupal.org/node/997024

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Looks RTBC to me now! Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to stBorchert as he might have time to take a final look at this.

In the Installation section of the README, instead of "3.Enable the required permissions." can you tell the user what those permissions are?

In webform_email_reply_access(), isn't $submission->uid the id of the user who submitted the form? I think you need to check the $node->uid to see if it's the same as the webform node author.

In webform_email_reply_form() you're using 'full_html' without checking to make sure the user has access. Normally admins have this permission, but checking is still good.

Setting to needs work for security issues, but otherwise this module looks nice.

----
Top Shelf Modules - Crafted, Curated, Contributed.

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
  

  
  FILE: /home/klausi/pareview_temp/README.txt
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   25 | WARNING | Line exceeds 80 characters; contains 83 characters
  --------------------------------------------------------------------------------
  
  
  FILE: /home/klausi/pareview_temp/webform_email_reply.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   144 | ERROR | No space before comment text; expected "// check text format
       |       | access for the current user" but found "//check text format
       |       | access for the current user"
   144 | ERROR | Inline comments must start with a capital letter
   144 | ERROR | Inline comments must end in full-stops, exclamation marks, or
       |       | question marks
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

• webform_email_reply_previous(): so instead of check_plain() you should use check_markup() here with the message? But you are not saving the format, so you cannot do that. Why do you even offer a format selection in the form if you don't use it anyway?

But otherwise looks RTBC.

I trialled this on simplytest.me, and did a visual review.

I was a bit surprised that the email 'to:' field wasn't auto-filled by anything. I'd expect that a webform that collected an email address would be able to use that in this utility when replying. Maybe I didn't find that option...

Code looks nice. Security review looks pretty thorough.
the RTBCs above look good and I agree.

Thanks for your contribution, Paul Rowell !

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.