Mailjet

Hello Mailjet,

Please follow the instructions here to apply for full project access - http://drupal.org/node/1011698
Also, currently I saw everything is on the master branch.
Please follow instructions here - http://drupal.org/node/1127732

Updating status to needs work

Go to your project page, then to the version control tab, and you will find the something that says - git clone ....
you will have to provide that link here.
The module is already there in the drupal repository, you have to provide a link here.

Also, please move your code from master to version specific branch.

Mailjet,

Please follow naming conventions explained here - http://drupal.org/node/1127732
For a drupal 7 module, branch name would be 7.x-1.x or similar. Check on the link for more. You will have to rename your branch

Updating git clone command.

A few quick things while going over the code.

- It is recommended that the project name is more or less the same thing as the module name. E.g. mailjet in your case. It is currently named mj_customsmtp. Then you could also save yourself the additional subdirectly and place the module files directly in the top level directory of your project.

configure = admin/config/system/mj_customsmtp

I know that many existing mail modules are below system, just wondering if your module would make more sense below services.

files[] = mj_customsmtp.module
files[] = mj_customsmtp.admin.inc
files[] = mj_customsmtp.mail.inc
files[] = mj_customsmtp.phpmailer.inc
files[] = mj_customsmtp.transport.inc

You should only specify files which contain classes and the .module file is always included so you don't need to specify that.

version = 1.0

You don't need to specify the version manually. You will, once you're project has been approved, make releases by tagging your code (e.g. 7.x-1.0alpha1, or 7.x-1.1) and then the package build will automatically add the version and other information to your .info file.

That said, I *strongly* recommend to not start with 1.0 but an alpha or beta version. As soon as you make a stable version, people will assume that the UI/API will be stable and if someone finds a security issue in your code, you will have to work together with the security team to make a security release including Security Anounncements and so on.

- Make sure you follow the coding standards, e.g. install the Coder module and let it inspect your module. One obvious thing I noticed are missing docblocks in your .module file and probably other places. Hooks e.g. should have a Implements hook_something(). docblock. I believe there is also a page where you can have your module reviewed.

class SmtpMailSystem implements MailSystemInterface {

Classes in your module show be namespaces, so it should be called e.g. MailJetSmtpMailSystem.

- I am also not sure how the exact rules are about including external libraries within your module code. For legal/licencing reasons, this is usually not allowed. http://drupal.org/project/phpmailer for example doesn't include the phpmailer library.

More later...

Changes look good.

When you do a git clone right now like this:
git clone .... mailjet (which will be the default once you're project has been approved, assuming your project shortname will be mailject)

You end up with the following strucute:
.../mailjet/mailjet/mailjet.module
Aka, the level is unecessary within two mailjet folders. So you can simply move all files

Something like that is usually only done if a project contains multiple modules (and then the main module is still in the top level directory).

Also, you don't need the LICENSE.txt, that is another thing that is automatically added by the package script (one reason for this is that all modules have the same license file and that there aren't like 8000 duplicated license files on d.o ;))

Coder unfortunately does not verify code documenation, have a look at http://drupal.org/node/1354 to see how it should look like. The new Drupal Coding Standard module, which is used by http://drupal.org/project/pareviewsh does a better job at that I think.

You can of course use any version you want (within the naming schemes enforced by d.o), I'm just recommending that you start with e.g. a 7.x-1.0beta1 or 7.x-1.0rc1 instead of going straight for a 7.x-1.0. Then you can give your users a few weeks to try and test your module. I mean, you just did a complete rename of your module, it's easy to miss a variable or something like that :) And there will be a few more changes until your project will be approved I think.

One more thing I noticed while looking at the commit is that you're doing a watchdog() call for every sent mail. That is a rather slow operation that results in INSERT queries, you might want to make that conditional by adding a debug variable. And if it's turned on then you could maybe even print a bit more, like the mail subject. Especially since for example Simplenews (A newsletter module that I maintain) already has similar debugging built in, see http://api.worldempire.ch/api/simplenews/includes--simplenews.mail.inc/f....

Ok, actually trying out the module now.

After some trying around, I managed to successfully send HTML mails through Simplenews with the help of http://drupal.org/project/mimemail and http://drupal.org/project/mailsystem. The latter allows to create combined format/send mail system classes on the fly, so I created one that uses mimemail's formatting and mailjet's sending.

Here's some feedback, in no particular order:

- Various options aren't configurable in the UI, i.e. the debug or the allow html setting (Some probably don't need to be, but I think at least these two would be useful as checkboxes)
- No idea what the Enabled/Disabled setting is for exactly? Defaults to No and says that I need to disable it first before I can uninstall the module... ?
- Maybe move the test send feature (which is helpful) into a separate fieldset, with a separate button that only triggers that? (you can add #submit callbacks on specific buttons) Then you don't need the hack you currently have to do that and it simplifies the UI (no radios necessary, just mail textfield + button in a collapsed fieldset)
- You might want to look into mailsystem to integrate with that instead of overriding the mail system settings completely when the module is installed.
- I don't think you need to check the debug variable for logging errors, just for actual debug messages :)
- What is the mimemail check in the admin settings form function exactly supposed to do?
- Sending with Simplenews works fine with the combined class but sending a test mail gives me this error when trying to send a test mail: "Notice: Undefined index: charset in MailjetSmtpMailSystem->mail() (line 155 of .../mailjet.mail.inc).
- I've played around with some mass sending and quickly reached the 200 mails per day limit. What's unfortunate is that there doesn't seem to be an error. So these mails are actually lost without any indication that it failed. E.g. Simplenews does support tracking send success and will automatically re-try later if it failed. I am not sure if this is a bug or the expected behavior, because if mailjet would return an error, the module should report that I think.

As a general and kinda off-topic question, would you be interested in working together integrating Simplenews with Mailjet? For example to track bouncing and disable bounced subscribers. Please contact us through our issue queue if so :)

Also, have a look at http://ventral.org/pareview/httpgitdrupalorgsandboxmailjet1319774git-7x-1x, which is a more extensive check than what coder.module does.

Note that there might be false positives :)

Also, about phpmailer, have a look at the last chapter of http://drupal.org/licensing/faq#q10 and http://drupal.org/project/libraries. The problem is not the license, but for example that you essentially fork the other project and people are stuck with the version you're delivering and it might result in conflicts if another module also depends on PHPMailer (Drupal 7 supports multiple mail system libraries, so that you could e.g. send newsletters wit mailjet and simple password recovery links directly). As mentioned, have a look at how http://drupal.org/project/phpmailer handles it.

Looks like you can forget about my last point in #15, the additional mails have been delivered today so they *have* been sent. Is there a way to see that I have queued mails on mailjet.com?

And yet another point :)

- The from mail address is not marked as required but the validation fails if it's empty. Suggestion: Mark it as #required => TRUE and use the sitewide default als default value: variable_get('mailjet_mail', variable_get('site_mail', ''))

Posted a patch for the charset issue here #1448172: Notice when charset is not defined explicitly

Please don't assign the issue to your self, only the current reviewer should do this.

as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

Mailjet,

I'm current using your module to try out Mailjet for free (the 200 emails a day). I was able to successfully send through Drupal, using the contact form on a user's profile, but I need to know, is there a way with this module to see it's status? Is there a report that gets data from the Mailjet API and displays it? All I see is the logs to the database that the email was sent, and Drupal does that already without your module. Is there any statistics and log review?

Rich

Sorry for the delay, but you have not listed any reviews of other project applications in your issue summary as strongly recommended in the application documentation.

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

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

manual review:

1. mailjet_install(): no need to set variables here as you can use default values in variable_get() anyway.
2. mailjet_uninstall(): why do you need to call the disable hook here, it will be called anyway if your module is disabled?
3. mailjet.phpmailer.inc appears to be 3rd party code. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms. The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.
4. Please got through the automated code review results and fix them.

Hi,

Could you clarify reasons why you set default mail system, if you clearly intend to have it disabled at first.

It will be set in your admin configuration form depending user selection anyway.

/**
 * Implements hook_enable().
 */
function mailjet_enable() {
  variable_set('mail_system', array('default-system' => 'MailjetSmtpMailSystem'));
}

/**
 * Implements hook_update().
 */
function mailjet_update_7000() {
  if (variable_get('mailjet_on', 0) != 0) {
    variable_set('mail_system', array('default-system' => 'MailjetSmtpMailSystem'));
  }
}

Also check is update or enable hook run first if you intend to set mail system during installation/enabling the module.

What comes to setting variables in hook_enable (in your previous comment #1), i think you still shouldn't do that. One of those variables you set anyway in admin configuration form, leaving only two variable.

If you think that my suggestions doens't apply / are incorrect, feel free to change status back to needs review.

Other than that, i think it starts to be ready.

One more issue witch i found is that if you intend to have feature disabling mailjet email sending in your admin configuration form, you don't react on that change.

Once your module has been saved and if mailjet has been disabled, you should set defaultmailsystem back.

variable_set('mail_system', array('default-system' => 'DefaultMailSystem'));

And ofc set mailjetmailsystem as mail_system if mailjet gets enabled again.

I still don't understand why in your .install set mailjet_on disabled + replace current DefaultMailSystem.

Without that code in install file procedure follows
1. User installs & enables modules, mailjet stays disabled and DefaultMailSystem continues sending email
2. User goes config page, enables mailjet -> your code replaces DefaulMailSystem with MailjetMailSystem.

I think you should either remove those codes, or in your configuration form mailjet_on default value in variable_get should be 1, because at that point DefaultMailSystem has allready been replaced but your code *thinks* it is not.

  $form['onoff']['mailjet_on'] = array(
    '#type'          => 'radios',
    '#title'         => t('Send emails through Mailjet'),
    '#default_value' => variable_get('mailjet_on', 0),
    '#options'       => array(1 => t('Yes'), 0 => t('No')),
  );

Respond / fix this and i am considering this reviewed and tested.

Yes,

Thank you for your patience.

1. Always use TRUE and FALSE instead of 1 / 0 so it's clear that these variables are booleans
2. It would be better if you add another submit button called "Send testmail" and putting the testmail code in its submit handler instead of putting all that stuff into the form building function.
3. Your also saving the mailsystem variable within the form building function, instead just add another submit handler and do it in there.
4. Your checking whether values are mandatory by the validation function with empty() - this is not necessary as you already use '#required' => TRUE
5. Don't use hook_install() for initializing variables - always use default values instead, there are several reasons for this, please follow best practice here
6. fSockOpen() don't use camelcase when the definition of the function is also not camelcase (see php.net)
7. sPrintF(t('Please contact Mailjet support to sort this out.<br /><br />Error %d - %s'), $errno, $errstr) use t() function replacement variables see t()-documentation

Leaving RTBC please fix those points first

I can understand your desperation with the process, sorry about that.

Sorry I can't find the messages where you answered the points 2 and 3 ?
The problem i see in that is that whenever the form is build the variables are get and set everytime no matter whether it was submitted

Exactly these "make it equal to other cms" behaviours are really annoying, for other drupal developers they make it very hard to understand and contribute to such modules. That is the reason why I persist on telling people to please do it the drupal way for a drupal module.

But your right it's not important, if you really want to do it that way, go for it...

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.