ZoeN [ZoeN]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

Hello. My initial glance at the code suggests that it follows Drupal coding standards pretty well and looks well coded. A couple general things.

1. I would seriously consider putting this in the MailChimp module (or having that discussion more). There are already so many modules on d.o and the goal of the MailChimp module is integrate Drupal and MailChimp. IMO, there is no reason to split up this effort into multiple module when this module is a very basic function of integration. I could understand if the MailChimp module was just an API module of if this module was a very focused use-case, but neither of these are true.
2. Consider using the "file" option in your menu item so that you can put the majority of your code in a separate file that will not get parsed on every page load, especially since the function of this module only happens rarely.

3.       if ($counter == 1) {
           $message = t('1 campaign was imported from MailChimp and saved as a') . ' ' . $nodetype . ' ' . t('node.');
         }
         else {
           $message = $counter . t(' campaigns were imported from MailChimp and saved as') . ' ' . $nodetype . ' ' . t('nodes.');
         }
   

         '#value' => t('Before you can use MailChimp Import, you must first ') . l(t('configure MailChimp module'), 'admin/settings/mailchimp') . ' with your MailChimp account information.',
   

   Use t() correctly with replacement values, as well as use t().

4. You dont delete the variables you create in the uninstall hook.

5. /**
    * Submit handler for the Import Campaigns button; imports MailChimp messages
    * according to the configured MailChimp Import settings.
    */
   function mailchimp_import_get_campaigns_submit($form, &$form_state) {
     mailchimp_import_get_campaigns($form, $form_state);
   }
   
   /**
    * Main function of this module, that does all the work of importing campaigns and saving them as nodes.
    */
   function mailchimp_import_get_campaigns($form, $form_state) {
   

   This seems unnecessary. If this function should be generic, at least change the names of the parameters.

6. In .info, you do not need "version = "6.x-0.1"
7. In .info, you do need "$Id$"
8. You menu item should be a local task of the MailChimp menu item (even if the two modules are not put together).

Overall, well done, keep up the good work.

hey zzolo -

i work with zoe, and we were discussing whether this would make a nice submodule of the main mailchimp module. is there anything special that needs to be done for this to be considered as such (other than obviously talking to the main module maintainers)?

otherwise, consider me generally vouching for zoe's awesomeness and deservingness of a cvs account. i know she'll do us proud. ;-)

a.

Hey Ariane, your kinds word are always appreciated, but we don't really do a voucher program for the CVS accounts. :)

My suggestion is to start a discussion with the module maintainer. To get a CVS account for co-maintaining a module, you need a thread in the issue queue of the given module that specifically states that the owner of the module wants the person applying to be a co-maintainer. The maintainer may want to use the code but not make you a co-maintainer, at which point, we still can't give you a CVS account without another application.

I think adding this module to the existing MailChimp module is the way to go. But I don't think it's necessary to get a CVS account and continue the review of this module for the application.

Let us know what direction you want to take, and feel free to ask more questions.

--
Alan

Yah well, still can't hurt. ;-)

Having stuck mostly to core stuff so far, I didn't even know the process for such things, so that is very helpful information... Will see what Zoe sorts out as far as which direction to take with this!

If they're into that, we'll start the appropriate thread in the MailChimp issue queue, and close this one.

If you are accepted as co-maintainer, you still need a CVS account to commit code into Drupal.org repository.

Is there any update on this?

no wonder i couldn't find this, it was closed! reopening - zoe's been away and is still working on this, so would be great if you can leave it open a bit longer. thanks kiam!

Team Chimpy! Awesome.

First off, if this is an issue somewhere else, can you link it up here so we can check out discussion.

As far as whats best, IMO, you should put this code in the MailChimp module and ZoeN should become a co-maintainer. This should be explicitly approved by the owner of the MailChimp module in that issue or this one.

6 weeks are passed from the last comment; are there any news?

Let's proceed with the review of the proposed module; if they didn't reply so far, it's useless to wait any longer.

Hi @ZoeN, can you upload a new version of the module with my comments address form #3 above.

@ZoeN, thanks for the application and patience. The following points do not necessarily encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

1. Please follow coding standards for CSS and include a $Id$ and @file block in the CSS file.
2. In install file, you need doc block for your hook_schema implementation and do not use # comments (or comments on the same line with code).

3.   $items['admin/settings/mailchimp/import'] = array(
       'title' => t('MailChimp Import settings'),
       'description' => t('Manage MailChimp Import settings.'),
       'page callback' => 'drupal_get_form',
       'page arguments' => array('mailchimp_import_admin_settings'),
       'access arguments' => array('administer site configuration'),
       'type' => MENU_LOCAL_TASK,
       'file' => 'mailchimp_import.inc',
     );
   

   It is unnecessary to run title and description through t() as this is done by default.

4.     drupal_add_css(drupal_get_path('module', 'mailchimp_import') .'/mailchimp_import.css');
   

       require_once(dirname(__FILE__) . '/mailchimp_import.inc');
   

   Consistency on using the dot operator correctly (space on both sides).

5.     $form['mailchimp_import_lists'] = array(
         '#type'           => 'select',
         '#multiple'       => 1,
         '#title'          => t('Lists'),
         '#options'        => $options,
         '#default_value'  => is_array($field['mailchimp_import_lists']) ? $field['mailchimp_import_lists'] : 
                                variable_get('mailchimp_import_lists', array()),
         '#description'    => t('Select the lists you want to import campaigns from.'),
       );
       
       $status_options = array(
         'save' => 'Saved',
         'paused' => 'Paused',
         'schedule' => 'Scheduled',
         'sending' => 'Sending',
         'sent' => 'Sent'
       );
   

   Consistency with array structures. Either one is fine (though I personally prefer the latter :)

6.     $form['mailchimp_import_lists'] = array(
         '#type'           => 'select',
         '#multiple'       => 1,
         '#title'          => t('Lists'),
         '#options'        => $options,
         '#default_value'  => is_array($field['mailchimp_import_lists']) ? $field['mailchimp_import_lists'] : 
                                variable_get('mailchimp_import_lists', array()),
         '#description'    => t('Select the lists you want to import campaigns from.'),
       );
   

   What is $field for or from?

7.     // MailChimp account info has not yet been configured.  Provide instructions and a config link with a 
       // destination query which will bring us back here.
       $form['message'] = array(
         '#value' => t('Before you can use MailChimp Import, you must first <a href="@config_path">configure 
                       MailChimp module</a> with your MailChimp API key.', 
                       array('@config_path' => url('admin/settings/mailchimp', array('query' => array(
                         'destination' => 'admin/settings/mailchimp/import'))))),
       );
   

   Your spacing is way off (in other places as well). Also, I would consider using hook_requirements to ensure that the module cannot be installed without an API key int eh MailChimp module. Oh, and this should just be a drupal message, not a form item.

8. 
         '#submit' => array('mailchimp_import_get_campaigns'),
       );
   
       return system_settings_form($form);
   

   Your use of system_settings_form seems a bit off. That function will save all form items as variables. This means there is no reason to reference your variables and array indexes in them. It also means that you are probably not deleting all the variables you are creating, specifically your new button.

9. 
       // Process variables.  Try to use the values from the form first; if not set, use the saved variables.
       $lists        = count($form['mailchimp_import_lists']['#value']) ? $form['mailchimp_import_lists']['#value'] : 
                         variable_get('mailchimp_import_lists', array());
   

   You should be using $form_state['values']. Also, since this is the submit function, you will have the values form the form, so there is no need to look into the variables.

10.             // Only import campaigns that have the right status.
                if (array_key_exists($campaign['status'], variable_get('mailchimp_import_campaign_status', array('sent' => 'Sent')))) {
    
                  // Check if this campaign has already been imported.
                  if (!in_array($campaign['id'], $already_imported_campaigns)) {
    

    These could be combined into one if..else. Also, the usual coding Drupal paradigm is to check for the opposite and return, thus creating a structure that is not full of nested if..else statements.

Overall, this is looking good. These are mostly minor things; just needs some cleaning up.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

Please read the following links as this is very important information about CVS applications.

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for these applications. Please read Migrating from CVS Applications to (Git) Full Project Applications and Applying for permission to opt into security advisory coverage on how this affects and benefits you and the application process. In short, every user has now the permissions necessary to create new projects, but they need to apply for opt into security advisory coverage. Without applying, the projects will have a warning on projects that says:

This project is not covered by Drupal’s security advisory policy.