[D7] Marketo MA

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxjyokum1949342git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

Manual Review:
Well written and commented code. Nicely done. Installed without a hitch. I found a few things worth noting:

• Should variable_set('marketo_ma_webform_fields', $value) need to be included in marketo_ma_webform_install()? The variable_del('marketo_ma_webform_fields') function is called in marketo_ma_webform_uninstall() and marketo_ma_uninstall().
• Consider changing hash type on line 198 in marketo_ma.module to 'sha256' versus 'sha1' as recommended here - http://drupal.org/node/845876.
• When clicking Retrieve from Marketo button on admin/config/search/marketo_ma with all default form values set, the system returns the following javascript dialog stating an AJAX HTTP error code 200 occurred.

@jyokum - thanks for removing the hook_uninstall() and disabling the form button. I tested and works as expected. The 'sha1' hash type requirement makes sense. I ran the code through ventral once more and it looks like there are still formatting errors (not including the marketo_ma.soap.inc as what you stated makes sense): http://ventral.org/pareview/httpgitdrupalorgsandboxjyokum1949342git

This looks quite good. Clear demonstration of your knowledge of Drupal APIs and coding standards. Below are some recommendations, some of which may have aready been addressed. I'd be ready to mark the application as RTBC once the first issue is dealt with.

File: marketo_ma.soap.inc

Please make sure that this file can be released as GPL. If not it will need to be removed from the repository and hosted elsewhere.

File: marketo_ma.info

You only need to include a JS file using the scripts[] directive of an info file if you want the script loaded on every page of a site.

scripts[] = js/marketo_ma.js

file: marketo_ma_webform.module

Does this allow the use of the email key when it may not be set?

* Check to see if an Email field has been provided. If not, we will try
* to use the current logged in user info
*/
if (!isset($data['Email']) || $data['Email'] == '') {
global $user;
if (isset($user->mail)) {
$data['Email'] = $user->mail;
}
}

marketo_ma_add_lead($data['Email'], $data);
}

File: marketo_ma.module

Comments should be complete sentences, including a period. Recurring in the different files.

/**
* @file
* Drupal Module: Marketo MA
*/

If there are no help text strings ready you should consider omitting this function.

function marketo_ma_help($path, $arg) {

Consider some additional description in the docblock to help others understand the purpose of this function. Also, please remove the commented line of code.

/**
* Implements hook_page_alter().
*/
function marketo_ma_page_alter(&$page) {
$marketo_ma_data = marketo_ma_get_queue();
// drupal_alter('marketo_ma_data', $marketo_ma_data);

The first key should be the module name, not marketo, this helps avoid namespace collision (happens elsewhere as well.

drupal_add_js(array(
'marketo' => array(

Are you sure you want to log each lead capture? This appears in a few places.

watchdog('marketo', 'Associating lead !email [@method]

@data

', array('!email' => $lead['email'], '@method' => 'munchkin', '@data' => print_r($lead['data'], TRUE)));

File: marketo_ma.inc

If these constants were defined in the marketo_ma.module you wouldn't need to explicitly include this file.

File: marketo_ma.js

yYou can reduce namespace collisions by using the module name as the behaviors namespace.

(function($) {
Drupal.behaviors.marketo = {

You should include this function in your module's behavior namespace for better encapsulation, and to keep the global namespace clean.

function marketoMunchkinFunction(leadType, data, hash) {

Good work. This module looks like it will be useful to the community, and you have demonstrated clear understanding of Drupal APIs and practices, including accessing and understanding community documentation.

This is still RTBC after changes mentioned above.

This issue has been in RTBC for over 2 months, raising priority based on https://drupal.org/node/539608

This sounds like a feature that should live in the existing marketo_munchkin project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the marketo_munchkin 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 RTBC.

The issue description explains pretty well why it is a different project from Marketo Munchkin: both providing different features and using a different approach. Seems they would remain separate modules, and unless someone might use both approaches at the same time, these are not good candidates for living in the same project package.

As jyokum has demonstrated solid maintainer responsibility, i've granted him vetted git user access and getting this out of the new maintainer project review queue.

Thanks greatly to chason and Everett Zufelt for the code reviews.

Congratulations, jyokum, on demonstrating sufficient quality of code and proficiency with Git to join the ranks of vetted Git users, the project maintainers.

The Git instruction tab on your project is one of your new best friends, and please mind your issue queue. Thanks for contributing!

@mlncn: please use the promotion template from https://groups.drupal.org/node/184389 :

Thanks for your contribution!

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.