Drupal Pushover

Very interesting module, very well coded... just add a couple of comments between your lines and I think it could be "ready to rock and roll" :-).

Nothing more to add, you delete the variables in your install, using files to separate different functionalities, a readme explains how to install and use it, ... very good job anderersg :-)

No errors in the automatic review: http://ventral.org/pareview/httpgitdrupalorgsandboxandeersg1902816git

Anyone confirm it, but i think it could be ready to mark as tested by the community :-)

The module seem to do exactly what it says. The rules integration works like a charm.

Some minor things:

• Sinse Pushover is a third party app, it would be nice if you linked to the site (pushover.net) from the help section.
• You have an "example" heading in your help section without any content.
• When I wrote bogus values in the API key and Client ID fields and pressed "Test notification" I got a status message saying "Message sent". There should be some errorchecking to see if the API actually sent the message.
• The "Test notification" link should be made into a button and renamed to something along the lines of; "Send test notification" which returns a status with success or failure.
• Sinse you use rules to send to roles/users on the event of rule-triggering, there should be some informative text in the help-section as well as the configuration panel explaining exactly that.
• In the main config panel you name the user-key field "Client ID" while in the user-edit page it's named "User key". Nitpicking, I know, but uniform naming is awesome :P
• You could also link to the rules-admin section from your admin page. Just a thought.

Code review:

• Your comment in the pushover.rules_default.inc needs an update ;) I'm guessing you fetched it from Commerce Stock...
• Same goes for pushover.rules.inc => pushover_rules_action_info(). There is a todo there that's outdated.
• The function _pushover_send_notification() in the pushover_send_to_all_rule() function is commented out, rendering the entire purpose of the function useless. And it's a watchdog in its place, for debugging I'm guessing.
• The $userobj = user_load_by_name($user) might fail as you explode on '\n'. '\r' might be left at the end/start of each item, corrupting the name.
• In the _pushover_send_notification() you only check for a 200 OK response. You should trap the other codes to, especially 4xx and 5xx as they indicate something seriously fishy is going on.
• If the message is not sent, the status code will still be 200, but the text-status will return -1 or something like that, which you should also check.
• This might cause an endless loop if you have a rule to trigger on watchdog, but it would be nice if the function logged to watchdog on failure of any kind.
• You also only include the title, message, and priority as arguments. The API supports message, device, title, url, url_title, priority, timestamp, and sound. The main function doesn't need all those, but a secondary function could take all arguments. The primary function with only title, message, and priority could just call the secondary function will NULL values for the other arguments so you don't code two function with basically the same code.

Just a few relatively minor things:

In _pushover_send_notification() it would be good to return TRUE or FALSE depending on successful return code. Functions which call _pushover_send_notification() can then choose to display messages based on success or failure.

In pushover.rules.inc, $title is passed into _pushover_send_notification() but is undefined (lines 159 and 183) so you will get PHP warnings from this. Looks like you could set this to NULL or to the value of $subject

pushover_send_to_user_rule() the comment for this function says "send notification to single user" but from the code it looks like this will send to multiple users.

Otherwise looking good, looks a pretty cool module.

Duplicate. Deleting.

Looks good - the only further thing I'd do on the return TRUE/FALSE is that you currently check:

if ($response->code !== '200' || $return_data->status !== 1) {

but don't do anything if that doesnt match. What I'd maybe do (which reflects what PDNagilum said in #4 is:

if ($response->code !== '200' || $return_data->status !== 1) {
  $language = language_default();
  $to = $userkey . '@api.pushover.net';
  $params = array(
    'title' => $title,
    'message' => $message,
  );
  drupal_mail('pushover', 'pushover', $to, $language, $params);

  return TRUE;
}
else {
  watchdog('pushover', 'Pushover API call failed !response', array('!response' => $response));
  return FALSE;
}

(check my code actually works of course as I've not tested it!). This way you're then telling your calling applications/rules that something unexpected happened, and you're logging it for investigation later.

No, you're right - I think I mixed up = and != in the logic :-)

In that case, the return FALSE should come just after the drupal_mail, and return TRUE in the else.

I still think it would be worth logging the failure specifically in your own watchdog category (just before you return FALSE) so you can easily filter from all the other drupal_mail log entries, but its not a show stopper.

Nice one, that looks good. Based on code review and that other fixes from other posters look like they've been incorporated I'm going to mark as RTBC and see if anyone disagrees...

Good stuff, and a good module for Drupal.

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/pushover.module
  --------------------------------------------------------------------------------
  FOUND 3 ERROR(S) AND 1 WARNING(S) AFFECTING 4 LINE(S)
  --------------------------------------------------------------------------------
    58 | WARNING | Line exceeds 80 characters; contains 85 characters
   124 | ERROR   | The second argument to watchdog() should not be enclosed with
       |         | t()
   128 | ERROR   | The second argument to watchdog() should not be enclosed with
       |         | t()
   208 | ERROR   | Functions must not contain multiple empty lines in a row;
       |         | found 2 empty lines
  --------------------------------------------------------------------------------
  

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:

1. pushover_mail(): no need to use check_plain() for mails, see http://groups.drupal.org/node/280368
2. "drupal_set_message('No test user defined!', 'error');": all user facing text must run through t() for translation. Also elsewhere.
3. pushover_menu(): wrong docblock, see http://drupal.org/node/1354#hookimpl . Same for the other hook implementations.
4. "$form['field_pushover_user_sound']['und']['#options']": do not use 'und', use LANGUAGE_NONE instead.
5. pushover_rules_action_info(): I think the url should be of type uri, no?
6. pushover_rules_action_info(): I think the user should be of type user, no?
7. pushover_rules_action_info(): I think the timestamp should be of type date, no?
8. pushover_send_to_all_rule(): Do not use db_select() for simple static queries, use db_query() instead. See http://drupal.org/node/310075

But that are not critical application blockers, so ...

Thanks for your contribution, andeersg!

I updated your account to let you 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 get 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.