Infusionsoft Integration Module

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

Reviewing the code myself, I have three changes planned to the initial upload

If you plan to change the code, it would be better if you upload here the new version; it would be better for who review the code to review the latest developed code, to avoid to point out changes that you already did.

• The points reported in this review are not in order of importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.

2.     return t($txt, $replace);
   
   

   The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable), or t(function()); this means that if the argument of the function is not a literal string, it will not appear in the translation template.

3. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

   As reported in Documenting hook implementations:

   If the implementation of a hook is rather standard and does not require more explanation than the hook reference provides, a shorthand documentation form may be used in place of the full function documentation block described above:

   /**
    * Implements hook_help().
    */
   function blog_help($section) {
     // ...
   }
   

   This generates a link to the hook reference, reminds the developer that this is a hook implementation, and avoids having to document parameters and return values that are the same for every implementation of the hook. Optionally, you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

   In the case of hooks that have variables in the names, such as hook_form_FORM_ID_alter(), a slightly expanded syntax should be used:

   /**
    * Implements hook_form_FORM_ID_alter() for node_type_form().
    */
   function mymodule_form_node_type_form_alter(&$form, &$form_state) {
     // ...
   }
   

   This generates a link to the hook reference, as well as to the particular form that is being altered. Again, optionally you can add more information in a separate paragraph to describe the particular quirks of your hook implementation.

4. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how Drupal variables, global variables, constants, and functions defined from the module should be named; how the code should be formatted (in particular, the control structures).
5. The module doesn't implement hook_uninstall(), or doesn't implement it to remove the Drupal variables it defines.

6. /**
    * Implementation of infusionlink_admin_settings().
    * Store the Infusionsoft application name, API key, IP address,
    * and profile category name in the Drupal variables table.
    */
   

   The first part of the comment is used only for hook implementations.

7.   $form['actionId'] = array(
       '#type' => 'textfield',
       '#title' => t('Action ID'),
       '#description' => t('Enter numeric Action Set ID from Infusionsoft Setup-Action Sets'),
       '#required' => TRUE,
       );
     $form['nodeId'] = array(
       '#type' => 'textfield',
       '#title' => t('Node ID'),
       '#description' => t('Enter numeric Node ID of page on your Drupal site'),
       '#required' => TRUE,
       );
     return $form;
   

   The default values are missing.

8.       watchdog('infusionlink','Infusionlink_webform: Error saving user account.',
   
   

   The message should simply be Error saving the user account.

9. function infusionlink_error($error) {
     if ($error->code <= 0) {
       $error->message = t('TCP/IP Socket could not be opened');
     }
     drupal_set_message(t('XML-RPC error: %message (@code)', array(
       '%message' => $error->message, '@code' => $error->code)));
     watchdog('infusionlink', 'XML-RPC error: %message (@code)', array(
       '%message' => $error->message, '@code' => $error->code));
   }
   
   

   The messages should end with a period.

10.       watchdog('infusionlink', 'XML-RPC added Person: Id=!contactId', 
            array('!contactId' => $contactId), WATCHDOG_DEBUG);
    
    

    The placeholder is not if the right type, if the contact ID is not a number.

11. The presence of commented out code makes me think the code is not complete, contrary to what required from the CVS application requirements.

12.   drupal_set_message(t('XML-RPC error: %message (@code)', array(
        '%message' => $error->message, '@code' => $error->code)));
      watchdog('infusionlink', 'XML-RPC error: %message (@code)', array(
        '%message' => $error->message, '@code' => $error->code));
    
    

    Both the function calls miss the parameter for the severity; without that, the message is a notice or status message (which is not an error message).

1.  variable_get('infusionlink_ipAddress', '208.76.24.x')
   

   The coding standards report that Drupal variables, PHP variables, and PHP functions should use lowercase characters, numbers or the underscore character. This means also that the PHP variable $IPaddress should have a different name.
   The code sometimes call the Drupal variable infusionlink_ipAddress, and sometimes infusionlink_ipaddress; AFAIK, Drupal variable names are not case sensitive, but I could be wrong (or it could be changed in the future).

2.   if (!empty($_POST['ContactType']))
       $edit['profile_persontype'] = $_POST['ContactType'];
     if (!empty($_POST['FirstName']))
       $edit['profile_firstname'] = $_POST['FirstName'];
     if (!empty($_POST['LastName']))
       $edit['profile_lastname'] = $_POST['LastName'];
     if (!empty($_POST['Company']))
       $edit['profile_company'] = $_POST['Company'];
     if (!empty($_POST['JobTitle']))
       $edit['profile_jobtitle'] = $_POST['JobTitle'];
     if (!empty($_POST['Phone1']))
       $edit['profile_phone1'] = $_POST['Phone1'];
     if (!empty($_POST['StreetAddress1']))
       $edit['profile_address1'] = $_POST['StreetAddress1'];
     if (!empty($_POST['StreetAddress2']))
       $edit['profile_address2'] = $_POST['StreetAddress2'];
     if (!empty($_POST['City']))
       $edit['profile_city'] = $_POST['City'];
     if (!empty($_POST['State']))
       $edit['profile_state'] = $_POST['State'];
     if (!empty($_POST['PostalCode']))
       $edit['profile_postalcode'] = $_POST['PostalCode'];
     if (!empty($_POST['Country']))
       $edit['profile_country'] = $_POST['Country'];
   

   As per Drupal coding standards (the ones I always make reference) the curly parentheses are not optional, in the control structures. This means that an IF-statement is always written as

     if ($test) {
       echo $test;
     }
   

   Following the format suggested from the coding standards help the security team, which has also the task to verify any security issues reported for the modules committed in Drupal.org repository; writing 12 IF-statements like that doesn't help the security team in finding out any possible security issues.

   The same format is used for the following code (it is also followed in other code lines I don't report here):

     if (isset($user->profile_firstname))
       $queryfields['FirstName'] = $user->profile_firstname;
     if (isset($user->profile_lastname))
       $queryfields['LastName'] = $user->profile_lastname;
     // Set contact fields...
     $contactfields = array( 'Email' => $user->init );
     if (isset($user->profile_firstname))
       $contactfields['FirstName'] = $user->profile_firstname;
     if (isset($user->profile_lastname))
       $contactfields['LastName'] = $user->profile_lastname;
     if (isset($user->profile_persontype))
       $contactfields['ContactType'] = $user->profile_persontype;
     if (isset($user->profile_company))
       $contactfields['Company'] = $user->profile_company;
     if (isset($user->profile_jobtitle))
       $contactfields['JobTitle'] = $user->profile_jobtitle;
     if (isset($user->profile_phone1))
       $contactfields['Phone1'] = $user->profile_phone1;
     if (isset($user->profile_address1))
       $contactfields['StreetAddress1'] = $user->profile_address1;
     if (isset($user->profile_address2))
       $contactfields['StreetAddress2'] = $user->profile_address2;
     if (isset($user->profile_city))
       $contactfields['City'] = $user->profile_city;
     if (isset($user->profile_state))
       $contactfields['State'] = $user->profile_state;
     if (isset($user->profile_postalcode))
       $contactfields['PostalCode'] = $user->profile_postalcode;
     if (isset($user->profile_country))
       $contactfields['Country'] = $user->profile_country;
   

3.   $message = sprintf('XML-RPC error: %s (%d).', $error->message, $error->code);
     drupal_set_message($message, 'error');
   

   Strings used in the user interface should be translated; see the documentation for t() how strings are translated.

4. As reported by the application requirements, any debugging code (even the commented out code) should be removed.

5.     $txt = '<p>This module links a Drupal site with an Infusionsoft '
         .'application. It synchronizes Users in your Drupal site '
         .'with Persons in Infusionsoft, in both directions, using '
         .'contact information drawn from Profiles in Drupal. It '
         .'allows you to run Infusionsoft Action Sets that can tag a '
         .'Person, or add them to a follow-up sequence in Infusionsoft '
         .'when they perform certain actions on your site, recognized '
         .'by the Triggers module in Drupal.</p>'
         .'<p>This module uses Drupal XML-RPC facilities and the '
         .'Infusionsoft XML-RPC API. To use this module, your '
         .'Infusionsoft application must be API-enabled and you must '
         .'save your Infusionsoft application name and API key in '
         .'Site configuration - Infusionlink Settings.</p>'
         .'<p>This module will synchronize certain standard Person '
         .'fields in Infusionsoft with User Profile fields in Drupal. '
         .'To enable this, choose User Management - Profiles to define '
         .'the fields. The field names must match the list below:</p>'
         .'<p>profile_firstname, profile_lastname, profile_jobtitle, '
         .'profile_company, profile_phone1, profile_address1, '
         .'profile_address2, profile_city, profile_state, '
         .'profile_postalcode, profile_country, profile_persontype. '
         .'If you use profile_persontype, your list of choices must '
         .'match between Infusionsoft and the Profile field. To '
         .'add fields or change these names, you will need to modify '
         .'the module PHP source code (not very difficult).</p>'
         .'<p>If you use Infusionsoft Webforms for opt-ins, and you '
         .'want these people added to your Drupal site, you must add '
         .'an action to your Webform to send an HTTP POST to your '
         .'Drupal site at http://yoursite.com/infusionlink_webform. '
         .'Usernames for the Drupal site are created from the first '
         .'letter of the first name concatenated with the last name '
         .'as entered on the webform.</p>'
         .'<p>For your security, this module checks that the HTTP POST '
         .'comes from an Infusionsoft server IP address, that you '
         .'specify in Infusionlink settings (the last quad in the '
         .'IP address is ignored). The default IP address 208.76.24.x '
         .'is OK as of September 2010, but to make sure, leave this '
         .'setting blank, fill out and submit your Webform, then use '
         .'Reports - Recent log entries to see the IP address.</p>'
         .'<p>To define Action Sets to run, use Site configuration - '
         .'Actions, choose an Advanced Action, enter an Infusionsoft '
         .'Action Set ID and Drupal Node ID, then associate that '
         .'Action with a Trigger on Content, Comment or User activity. '
         .'To determine Action Set IDs, choose Setup - Action Sets in '
         .'Infusionsoft, find your action and check the Id column.</p>';
       $replace = array();
       return $txt;
   

   Strings used in the user interface should be translated.
   When possible, HTML tags should not be included in the translated strings; that doesn't mean to split a sentence or a phrase just to avoid to include an HTML tag in the string, though. In this case, this means the string can be split before <p> and after </p> (as those tags are placed before or after a sentence).

6.   $form['infusionlink_apikey'] = array(
       '#type' => 'textfield',
       '#title' => t('API Key'),
       '#default_value' => variable_get('infusionlink_apikey', 'Your API Key'),
       '#description' => t('Copy and paste your API key here -- request this from Infusionsoft'),
       '#required' => TRUE,
       );
   

   The description strings should always end with a period.

7.   $name = strtolower(substr($firstname,0,1) . $lastname);
   

   There is a Drupal function that should be used instead of strtolower() (see the Unicode Drupal functions reported in the coding standards).

1.   $message = sprintf('XML-RPC error: %s (%d).', $error->message, $error->code);
     drupal_set_message($message, 'error');
   

   On #3, since this string is created dynamically, it cannot go inside the t() function as you pointed out earlier.

   The string 'XML-RPC error:' is not dynamically created, but it's a static string. The error code could be only in English, but that is not translated, if passed through a place-holder.
   As such, the string still need to be passed to t() because it's a static string used in the user interface.
   There is then another reason to use t() and its placeholders: the strings used as placeholder are passed to check_plain(), which is necessary to avoid security issues.

2. Following the Drupal coding standards is not optional; if that would be true, then a module could not suffix its function names with the module name (and have function names like node_test()).

   I do not believe that adding curly braces, in this code, will add to readability or help the security team.

   Believe me, the code as it is now is not much readable, especially from who is used to read code that follows the Drupal coding standards (as the security team is).

3. On #5, it appears that you've changed a simple string "http://yoursite.com/infusionlink_webform", illustrating what must be entered in the Infusionsoft system, into a long href tag which would be invalid if entered in the Infusionsoft system. This is not a good idea. I haven't changed the code here.

   In point #5, I am reporting that you are using a not translated string in the user interface, when the strings used in the user interface should be translated. The implementation of hook_help() is only relevant on the Drupal-powered site, and it doesn't involve any external sites.

4. Just FYI, I'm extremely pressed for time and I probably cannot spend much more time on this contribution which is an optional activity for me, especially over the next couple of months. I do appreciate your time spent reviewing the code.

   I take that means you would not have too much time to maintain the module, which is what you are supposed to do. What would happen if a user reports a bug report? Will you have the time to port the code to Drupal 7?

Setting to needs review.

I really like the idea and will try testing it if I can.
It's a shame that no one is testing this as this opens up a huge avenue for drupal over the wordpress projects. I'm currently fighting to keep drupal over having to switch to wordpress here in the office using infusionwp.com. So please, I'd love to see this project come to fruition!

I am also doing a third party integration with infusionsoft crm and drupal; more specifically just for webform 3.x and infusionsoft data integration. I've glance over this page, installed the infusionlink module and went through reviewing code with the help of the Coder module while it's settings admin/settings/coder are on strictest.

It yielded me the following:

Coder found 1 projects, 2 files, 1 critical warnings, 51 normal warnings, 19 minor warnings, 0 warnings were flagged to be ignored

I think it might help if the infusionlink module gets updated so there won't be any warnings.

regards,
Chris

I am changing status as per previous comment.

For anyone interested on an update about the webform 3.x and infusionsoft crm data integration; my company did not get the work order to have this functionality :( Trying to achieve an integration between webform and infusionsoft crm might take quite a bit of time and doing this during my free time is low in my priority list.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

With iSoft growing in popularity and the need for a Drupal Integration this module seems the best start to build a full integration plugin. If you are not interested in maintaining it would you be willing to allow someone to take over the development? My firm has a client who is looking for this exact solution, i'd hate to reinvent the wheel you've so nicely created.

I'll take the non-response as an abandonment from the author and will resume this project under a new name. When I have it up I will post back here. I am moving forward with a full membership management plugin with integration via iSoft's API.

zjoia,

I would be interested in any new module you are developing. I am currently working with a client who needs Infustionsoft integrated with Drupal membership site.

Rick

Hi @dfylstra. Not sure what happened when I updated this issue. I was doing a few hundred that night. Must have just made a mistake. You can keep this active, but you will have to move this a Git sandbox and then change the Project type. Please read: http://drupal.org/node/1075406

Great to see some progress on this thread... I'm also very interested in Infusionsoft integration with Drupal.

--Ben

Also glad to see some progress on this thread. dfylstra, appreciate you work, comments and suggestions on Drupal integration with Infusionsoft.

Rick

Still no code in the sandbox ...

One of the requirements of a project application is that you provide code for review. This issue is being closed because there is still no code available, thus no review or approval is possible.

This change is not permanent and the application may be re-opened after you have posted code in your sandbox for review.

Would love to see this be kept alive....considering moving to infusionsoft and would love a module that would help me integrate my user data, email campaigns and store.

Thanks!
Patrick