[D7] Amazon Mail Service

Doing review on

Commit f2f8e91 on 7.x-1.x

Automated Review

(*)Please clear the code sniffer issues found by pareview.sh.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

No: A quick google search gave me couple of results Amazon SES and SMTP Authentication Support. Can you please explain what unique functionality does this module provide?

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

(*)No: Please follow the guidelines for 3rd party code. Consider the use of libraries mmodule instead.

(+)README.txt/README.md

It would be preferable if you could take a reference of the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

1. (*) Major finding
2. Minor finding
3. (+) Release blocker

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Kuldeep,
Every time you commit your code, visit the above link, click on Repeat Review and click on Submit Branch button (make sure URL to your git repository is mentioned there).

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.

Assigning to myself for next review.

Duplication

This sounds like an duplicate of the existing Amazon SES project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the Amazon SES issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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

Looks like you you have not addressed the duplication question - people can just use https://www.drupal.org/project/smtp and set Amazon as SMTP server? Looks like your project page is not accurate?

The project page is wrong - you don't need to setup your own SMTP server, you can just set Amazon as your SMTP server. So you just have to set the credentials with the smtp module and you are all set, correct?

Yes, so it seems we already have a module which is working just fine with Amazon SES: the smtp module. Is there any benefit in using your module?

Review of the 7.x-1.x branch (commit 63cf8d0):

• Bad line endings were found, always use unix style terminators. See https://www.drupal.org/coding-standards#indenting
  

  ./amazon_mail_service.install:            PHP script, ASCII text, with CRLF line terminators
  amazon_mail_service.install
  

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

  FILE: /home/klausi/pareview_temp/amazon_mail_service.install
  ---------------------------------------------------------------------------
  FOUND 1 ERROR AFFECTING 1 LINE
  ---------------------------------------------------------------------------
   1 | ERROR | [x] End of line character is invalid; expected "\n" but found
     |       |     "\r\n"
  ---------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ---------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. Please add the differences to the SMTP module to the project page, so that users can make an educated decision what module fits their use case best when connecting to Amazon SES.
2. Instead of creating a new project I think we should move your code into the existing amazon_ses project namespace. It would be just even more confusing for end users if there would be 3 projects that deal with Amazon SES, so let's reduce it to 2 by moving this into amazon_ses. Please follow the abandoned project process to get access to the repository and push your code into a 7.x-2.x branch. You will also have to rename your module's functions to amazon_ses.
3. amazon_mail_service_form_alter(): if you are targeting only one form you should use hook_form_FORM_ID_alter() instead. See https://api.drupal.org/api/drupal/modules!system!system.api.php/function...
4. amazon_mail_service_form_alter(): why do you alter your own form provided in your module? You can just add that to the form building function? Please add a comment.
5. "t('Request to !action action of Amazon SES API call has failed, missing some parameter or Request is not valid.', array('!action' => "GetIdentityDkimAttributes",));": the !action placeholder is useless here since it is not a variable but always a static string? So you can just use it directly in the message?
6. "if (($form_state['clicked_button']['#value']) == 'Send Request') {": this will fail if the button label value is translated to another language, right? Also elsewhere.
7. amazon_mail_service_verify_email_identity_amazon(): that function is not called anywhere, or am I missing something?
8. amazon_mail_service_verify_email_identity_amazon(): you should use the "@" placeholder instead of "!" for user provided text in t() strings, make sure to read https://www.drupal.org/node/28984 again.
9. amazon_mail_service_verify_sender_id_form_validate(): instead of checking if the identity_email is empty you should use the #required property on the form element in amazon_mail_service_verify_sender_id_form(). See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...
10. amazon_mail_service_identity_list_form_submit(): why do you have to sleep() here for one second? Does the API otherwise give an error or what is the reason? Please add a comment.
11. amazon_mail_service_get_mail_statistics_form(): this looks vulnerable to XSS exploits. $result_quota is coming from a third party service and therefore must be treated as user provided input. You need to sanitize it with "@" or "%" placeholders in t() instead of "!". Also elsewhere, just use "@" placeholders for most cases.
12. amazon_mail_service_domain_dkim_enable_form(): The $header values are user facing text and must run through t() for translation.
13. "if (($form_state['clicked_button']['#value']) == 'Filter') {": instead of checking the clicked button you should register your submit callbacks with the desired form submit element so that it is triggered automatically when the button is clicked. See https://api.drupal.org/api/drupal/developer!topics!forms_api_reference.h...

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

@tkuldeep17 has requested maintainer access to the amazon_ses module, which I discontinued development on due to there being better support for SMTP email sending (this is probably still the case, IMO). Using Amazon SES via SMTP does not require "your own SMTP server", but this point might be lost in translation. If he can resolve the issues outstanding with his submission, I'll be happy to grant it.

Review of the 7.x-1.x branch (commit d2791c2):

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

  FILE: /home/klausi/pareview_temp/README.txt
  ------------------------------------------------------------------------
  FOUND 1 ERROR AND 1 WARNING AFFECTING 1 LINE
  ------------------------------------------------------------------------
   41 | WARNING | [ ] Line exceeds 80 characters; contains 107 characters
   41 | ERROR   | [x] Expected 1 newline at end of file; 0 found
  ------------------------------------------------------------------------
  PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
  ------------------------------------------------------------------------
  

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. project page: "SMTP requires an SMTP server to be configured." as repeated multiple time this is imply wrong. No SMTP server needs to be configured, because Amazon will be the SMTP server. Please change that to "SMTP module: sends mails via SMTP to Amazon SES" or similar.
2. amazon_ses_form_amazon_ses_domain_dkim_enable_form_alter(): doc block is wrong, this is hook_form_FORM_ID_alter().
3. amazon_ses_get_identity_list(): the $header entries are user facing text and must run through t() for translation.
4. amazon_ses_domain_dkim_enable_form(): HTML tags such as the br tags at the should be outside of t() where possible.
5. AmazonSesSystem::mail(): the amazon_ses_send_request() call here has a third parameter which does not exist on amazon_ses_send_request()?
6. amazon_ses_form_amazon_ses_domain_dkim_enable_form_alter(): the $result from amazon_ses_send_request() is directly printed to HTML here and it is not sanitized in amazonses.class.php either. Content received from a third party service must be treated as untrusted user supplied text and therefore must be sanitized before printing to HTML. Make sure to read https://www.drupal.org/node/28984 again. Please check all data that you receive and how it is printed. Since it is quite difficult for an attacker to exploit this vulnerability (hack Amazon or hack network traffic) I'm not adding the security tag here, but this should be fixed in any case.

Otherwise I think this is close to ready.

@Ryan Palmer: can you grant tkuldeep17 access in the meantime so that he can push the code to the amazon_ses project?

Cool, please move the code now into the amazon_ses repository.

can we transfer ownership of Ryan's module to @tkuldeep17 #2432593: Offering to maintain Amazon SES. We have added him as co-maintainer to move this process along.

Code is in the amazon_ses repository, so let's continue here.

Yes, you should delete the old sandbox now to avoid confusion.

Git errors:

• There is still a master branch, make sure to set the correct default branch: https://www.drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in https://www.drupal.org/node/1127732

But otherwise looks good to me now!

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

Read efbd3ad...

1. amazon_ses.install : Please add a comment on the project page that after enabling this module it will override then it will override the default mail system and start using AmazonSesSystem
2. There is still a master branch exists.Please make a liasie with prevoius maintainer(Ryan) and then remove the master branch.
3. amazon_ses_get_identity_list :
   

     $form['amazon_list_identities_filter']['select_indentity'] = array(
       '#type' => 'select',
       '#title' => '',
       '#options' => array(
         'EmailAddress' => t('Email Address'),
         'Domain' => t('Domain'),
       ),
       '#default_value' => array($identity_type),
     );

   directly pass the default value instead of passing through array because we don't have multiples enabled in select list.

No Blocker..

Thanks for your contribution, tkuldeep17!

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.