[D7] CiviCRM Contact Match

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxstandrag1910058git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors. And then change your status again.

Hi stan.d,

Your project application seems to succesfully pass Pareview automatic test :-)

Ideas :

Because CiviCRM is a third party app, it could be interesting to link to the site (civicrm.org) from the help section.

Following a bit of manual code review, here are few comments :

• README.txt file could contain information about the installation and configuration.
• it would be nice to complete comment sections with @param & @ return informations like
  
  /**
  * Modify the association between a drupal user and a civicrm contact.
  *
  * @param ...
  * Description ...
  *
  * @return ...
  * Description ...
  */

Please address them and change stask status again for a final review

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Hi Stan,

I'm a co-maintainer for commerce_civicrm and I've done a bit of work on CiviCRM core so I think this would be a good module for me to review.

It all looks fairly good and I can see how it would be useful but there are a few improvements I can see.

1. Split admin callback to a separate file.

You could move the admin callback to a separate file, this helps with performance since less code has to be loaded. CiviCRM is quite a beast anyway so I think this would be worth doing to help lighten the load.

  $items['admin/config/civicrm_match'] = array(
    'title' => 'CiviCRM Contact Match Settings',
    'description' => 'Configuration settings for the Modify CiviCRM uf match module.',
    'page callback' => 'drupal_get_form',
    'page arguments' => array('civicrm_contact_match_admin_form'),
    'file' => 'civicrm_match_admin.inc',
    'access arguments' => array('administer users'),
    'type' => MENU_NORMAL_ITEM,
  );

2. Return value consistency.

The $user check in civicrm_contact_match_edit_form could probably do with returning an empty array so the returns match, and also should display an error message saying no user found.

3. Use CiviCRM API.

I'm seeing a few problems with the way you're accessing and updating the CiviCRM db. Civi settings normally live in civicrm.settings.php and there's no mention in the README that these need to be duplicated into the drupal settings.php

You could avoid developers having to duplicate this setting by using the CiviCRM API to access and update the Civi db instead. I've added some example code below but I haven't tested it.

If you can get this to work then you can also get rid of a bunch of code from the admin page.

http://book.civicrm.org/developer/current/techniques/api/

    civicrm_initialize();
    // Get the CiviCRM contact id for the Drupal uid if there is a match
    $params = array('uf_id' => $uid, 'version' => 3);
    $uf_match = civicrm_api('uf_match', 'get', $params);
    if ($uf_match['id'] && count($uf_match['values'] == 1)) {
      $contact_id = $uf_match['contact_id'];
      // Now get the contact data
      $contact_params = array('contact_id' => $contact_id, 'version' => 3);
      $contact = civicrm_api('contact', 'get', $contact_params);
      $first_name = $result['values'][$cid]['first_name'];
      $last_name = $result['values'][$cid]['last_name'];
      // email is not used, is it necessary to set the $email variable?
    }
    // Add an error check here if there is no match (or more than one match?)

    civicrm_initialize();
    // Update the CiviCRM contact id for the Drupal uid
    $contact_id = $form_state['values']['civicrm_contact_match_civiid'];
    $params = array('uf_id' => $uid, 'contact_id' => contact_id, 'version' => 3);
    $result = civicrm_api('uf_match', 'update', $params);
    // Add an error check here in case there was a problem updating the record.

4. Error Checks.

I think you need to include error checks on whether you get multiple results back from CiviCRM for the uid match (shouldn't do but not impossible), and also need to check that the update has occured succesfully.

Hi Stan,

that sounds fair enough re the uf_match issue. Would be good to put the CiviCRM compatibility in the README and on the project page.

If you could you fix the automated code check errors and I'll review it again for you.

http://ventral.org/pareview/httpgitdrupalorgsandboxstandrag1910058git

Thanks,

Mark

Looks good.

----
Top Shelf Modules - Enterprise modules from the community for the community.

The project page could use more detail.

In civicrm_contact_match_edit_form() there are some style improvements you could make - using <<< is usually only for large blocks of text. You could simply write $info = "<h2>Modifying user <i>{$user->name}</i> ({$user->mail})</h2><br /><br />" instead. Instead of constructing the bare href string, you can use l(). Use single quotes when calling civicrm_api("UFMatch", "get",... - it would be nice if both the calls were formatted the same way. Why is more than 1 contact such a big problem that you immediately return? Is there a way for the admin to fix this issue if it happens somehow?

Since you're using civicrm_api() in several functions and some of the parameters are always the same, consider writing a small wrapper function that you can reuse. With all the api and db calls you could be more defensive - check that values are defined before using them.

Those are fairly minor issues though and it's been over a month now.

Thanks for your contribution, stan.d!

I updated your account to let you 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 get 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.

----
Top Shelf Modules - Crafted, Curated, Contributed.