[D7] SendGrid Integration [#1548328]

Project page: http://drupal.org/sandbox/exlin/1545920
Git: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/exlin/1545920.git sendgrid_integration
Drupal version: 7.x

This project allows you to send emails through SendGrid's email delivery services.
We use Web API and SMTPAPI for adding unique arguments (args) + category for email instead of sending that email throught SMTP.

Using SMTPAPI allows us to:
Categories
If you have several Drupal sites using the same service you can more easily identify the source of email as statistical or debugging functionality.
Unique_args
Later when there comes more features this allows site to implement example following functionalities
* User has clicked spam button, allow your Drupal site to react for that (e.g.: stops sending emails)
* Email address doesn't exist/function anymore (e.g.: Drupal suspends the account)

Differences with other modules

(mainly SMTP Authentication Support)
SMTP module is using general smtp, with authentication.
SendGrid Authentication uses WebAPI, allowing you to get response if email is accepted for delivery right away + we use SMTPAPI to add Drupal sites name as category and message + uid as unique_args witch will be referred later if SendGrid contacts sites directly or as filter throught SendGrid's admin interface.

Review of other modules

Profile visit
- http://drupal.org/node/1505706#comment-5947094
Blizzard Community Platform API
- http://drupal.org/node/1351312#comment-5931906
- http://drupal.org/node/1351312#comment-5931944
Mailjet
- http://drupal.org/node/1439308#comment-5936554
- http://drupal.org/node/1439308#comment-5936590
- http://drupal.org/node/1439308#comment-5937152

Comments

Comment #1

operinko commented 26 April 2012 at 10:14

Passes pareview, couldn't find any outstanding issues with a manual review, local testing went well.
Btw, cool project :)

Comment #2

operinko commented 26 April 2012 at 10:14

Status:

Needs review

» Reviewed & tested by the community

Changing status to reviewed.

Comment #3

exlin commented 26 April 2012 at 11:37

Thanks ;)

Comment #3.0

exlin commented 26 April 2012 at 11:37

Issue summary:

View changes

Added full git clone command instead of only git-url

Comment #4

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 28 April 2012 at 13:37

Status:

Reviewed & tested by the community

» Needs work

You have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

manual review:

sendgrid_integration_permissions(): wrong function name, remove the "s" from the end.
sendgrid_integration_permissions(): the returned array needs to have the permission names as key. See http://api.drupal.org/api/drupal/modules!system!system.api.php/function/...
"'access arguments' => array('Administer SendGrid settings'),": permission names should be all lower case.
If you are setting your own variables you should remove them on hook_uninstall().
"json_decode($result->data);": use drupal_json_decode() instead. Same for json_encode().
"'from' => variable_get('site_mail'),": I think you should check here if there is a different from address already set in $message, no?
"module_invoke_all('sendgrid_integration_sent'..." If you are calling your own hooks you should document htem in a *.api.php file. See http://drupal.org/node/161085#api_php

Comment #5

exlin commented 28 April 2012 at 15:20

Status:

Needs work

» Needs review

Thanks,

#3 I added key for hook_permission, but i believe access argument *calls* permission names not title.

Pareview complains about use of st() in hook_install, tho it should be fine.
From http://api.drupal.org/api/drupal/includes!bootstrap.inc/function/get_t/7:

Use t() if your code will never run during the Drupal installation phase. Use st() if your code will only run during installation and never any other time. Use get_t() if your code could run in either circumstance.

Comment #6

exlin commented 29 April 2012 at 11:02

Also created new feature, witch adds message to queue for later delivery if sending fails with code -110.
I need to figure out also other response codes cases where i want to add message to queue.
edit: more response codes were added

Comment #6.0

exlin commented 29 April 2012 at 11:02

Issue summary:

View changes

added implement to "allows site to implement example"

Comment #6.1

exlin commented 2 May 2012 at 18:12

Issue summary:

View changes

Added another modules i have reviewed section

Comment #7

exlin commented 2 May 2012 at 18:15

Issue tags:

+PAreview: review bonus

Added list of modules i have reviewed.

Comment #8

klausi

he/him||they/them

German

🇦🇹 Vienna

commented 4 May 2012 at 15:04

Status:	Needs review	» Reviewed & tested by the community
Issue tags:	-PAreview: review bonus

Regarding t() in hook_install(): if you enable your module in an existing Drupal installation, then t() should be called. If your module comes with an installation profile or a drupal distribution, then st() should be used during the Drupal core profile installation phase. You cannot be sure, so you should use get_t(). That is also described on the API doc page of get_t() that you quoted.

Regarding permissions: Form the hook_permission() doc page: "return value: An array whose keys are permission names".

manual review:

I think the *.api.php file should live in the root folder of the repository, as this is the first thing developers are looking for.
"$paramn $result_data": should be "@param $result_data"
Is it possible that mails circle forever in the queue if they are not accepted by sendgrid for some reason?

Anyway, that are just minor issues, otherwise I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Comment #9

exlin commented 4 May 2012 at 17:24

Status:

Reviewed & tested by the community

» Needs review

Thank you for review,

I switched from st() to get_t() in hook_install().

I don't think there risk of mail getting into infinite loop is significant.
According to sendgrids API any return code starting with 5x is good one, but you should try again later.
404 and 408 is quite standards i think, even if not guided to retry later on.

Greates risk of email staying forever in queue is if api request paths changes and module isn't upgraded accordingly either by module maintainer or user.

Comment #10

exlin commented 4 May 2012 at 17:25

Status:

Needs review

» Reviewed & tested by the community

Changed status accidently.

Other issues you mentioned has been fixed also.

Comment #11

patrickd commented 4 May 2012 at 17:57

Status:

Reviewed & tested by the community

» Fixed

You got some grammatical errors in your comments (mostly in api.php)

Please add a little example usage of the hook into api.php

Beside that everything looks pretty good.

Thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.