artis [artis]

Hello, and thanks 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.

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site using the proposed theme; for modules, it should include also a comparison with the existing solutions.

The procedure is done by volunteers, as everything on Drupal.org; there isn't a way to estimate how long a CVS application can take.

• This is a partial review.
• 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. Menu descriptions and titles, as well as schema descriptions, are not passed to t().
2. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

3. function uc_qbms_install() {
     drupal_install_schema('uc_qbms');
   
     drupal_set_message(st("QBMS CC Gateway is installed and can be turned on and congifured under !link",
       array( '!link' => l('Administer > Store Administration > Configuration > Payment settings > Payment Gateways ',  'admin/store/settings/payment/edit/gateways' ) )
     ));
   }
   
   

   t() is available when hook_install(), or hook_uninstall() are invoked. See the code executed from php_install() as example.

   l() should not be used together with t(). See the documentation for t(), where this code is reported to be wrong:

   $output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));
   

   The correct code to use is the following:

   $output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';
   

   There is a typo (congifured).

4. Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules.
5. 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 a module should be named; how the code should be formatted.

I am attaching a major clean-up of this module. Reviews appreciated. I would be happy to co-maintain this module with artis if necessary to get it committed. Here is a summary:

1. I found and fixed the schema descriptions that were not passed through t(), but I could not where any menu descriptions and titles were not passed through t() because there don't appear to be any menu elements.

2. All hook comments have been fixed.

3. Corrected the use of t() and l() to t() and url() in hook_install().

4. Fixed the deletion of variables in hook_uninstall.

5. Performed some major clean-up of the module to ad-hear to the coding standards.

Please let me know if you find anything else.

Cheers,

Antoine

Missed a bug in the install file.

Cheers,

Antoine

Remember to change status, when you upload a new version of the code.

kiamlaluno,

Thanks for updating the status... was in a rush and missed that.

artis,

Thanks for reviewing the module so quickly and catching couple of mistakes. I did create a test account following your instructions in the README.txt file and was able to successfully process transactions. I have not been able to try this on a live site yet.

I think this code is ready for a 6.x-1.x-dev release so we can get feedback from others through an issue que.

Cheers,

Antoine

1. Menu descriptions and titles, as well as schema descriptions, should not be passed to t() (in my previous point it seemed I was saying the opposite).
2. 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.

3.   if (!is_dir($qb_php_dk_path)) {
       drupal_set_message(t('You need to download the <a href="!qb_php_dk">Quickbooks PHP DevKit</a> and extract the entire contents of the archive into the !path folder of your server. Then rename the top folder of the library to quickbooks_php_devkit so that the main library file is located here: !path/quickbooks_php_devkit/QuickBooks.php. ', array('!qb_php_dk' => url('https://code.intuit.com/sf/frs/do/viewSummary/projects.php_devkit/frs'), '!path' => 'sites/all/libraries')), 'error');
     }
   
   

   The placeholder is not the one that should be used for URLs.

4.   $form['uc_qbms_connection_ticket'] = array(
       '#type' => 'textfield',
       '#title' => t('Connection Ticket'),
       '#default_value' => $login_data['connection_ticket'],
       '#description' => t('Enter the Contection Ticket provided by Intuit.<br />Get your connection ticket from Intuit by connecting your QBMS account with this module at: <a href="!qb_appr">Quickbooks Merchant Service Center</a>', array('!qb_appr' => url('https://merchantaccount.quickbooks.com/j/sdkconnection?appid=179399124&appdata=mydata'))),
     );
   
   

   url() should not be used with external URLs.

5. /**
    * Implements hook_form_alter().
    */
   function uc_qbms_form_alter(&$form, $form_state, $form_id) {
     if ($form_id == 'uc_payment_gateways_form') {
       $form['#submit'][] = 'uc_qbms_payment_gateway_settings_submit';
     }
   }
   

   The module could implement hook_form_FORM_ID_alter() in such cases.

6. Remove any debug code.

The Quickbooks payment gateway is widely used, and this feature is often requested, so this module will be a major asset to Drupal/Ubercart. I recommend approving both artis and AntoineSolutions as co-maintainers as soon as the Coder issues are worked out.

Kiamlaluno,

Thanks for your quick review and great input. Here is an update.

1. Removed t() function from schema descriptions.

2. Drupal variables all seem ok. Please let me know if there is something specific I am missing.
Renamed only defined static variable from $data to $_uc_qbms_data.
Reformatted comments of constants to use blocks.
Renamed constants prefixing with the module name upper-cased.
Renamed all functions beginning with an underscore to begin with the module name.

3. Fixed the place-holder to use '@' so it will be passed through check_plain.

4. I couldn't find much on how to process external URLs with t, but sinceyou says l() is not supposed to be used with t() and url() is incorrect for external URLs, I removed all processing. Please let me know if this is correct or not.

5. Replaced hook_form_alter() with hook_form_FORM_ID_alter().

6. Removed all debug code.

Additonal Changes:
- Moved TODO: comments into blocks using @todo doxygen directives.

I have tested this with a test account.

Cheers,

Antoine

  if (!is_dir($UC_QBMS_PHP_DK_PATH)) {
    drupal_set_message(t('You need to download the <a href="!qb_php_dk">Quickbooks PHP DevKit</a> and extract the entire contents of the archive into the !path folder of your server. Then rename the top folder of the library to quickbooks_php_devkit so that the main library file is located here: !path/quickbooks_php_devkit/QuickBooks.php. ', array('!qb_php_dk' => url('https://code.intuit.com/sf/frs/do/viewSummary/projects.php_devkit/frs'), '!path' => 'sites/all/libraries')), 'error');
  }

The code needs to simply be changed to

  if (!is_dir($UC_QBMS_PHP_DK_PATH)) {
    drupal_set_message(t('You need to download the <a href="@qb_php_dk">Quickbooks PHP DevKit</a> and extract the entire contents of the archive into the !path folder of your server. Then rename the top folder of the library to quickbooks_php_devkit so that the main library file is located here: @path. ', array('@qb_php_dk' => 'https://code.intuit.com/sf/frs/do/viewSummary/projects.php_devkit/frs', '@path' => 'sites/all/libraries/quickbooks_php_devkit/QuickBooks.php')), 'error');
  }

I can only approve the CVS account of who apply for an account; if AntoineSolutions doesn't have a CVS account, he needs to apply, and report the link to this CVS application in the motivation of his CVS application.

Thank you for your contribution! I am going to update your account.

These 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 more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.

Hello,

We are trying to use this module, and have so far had only one successful transaction, and many other that have failed. We have not installed the latest version (listed here) as when we started working with it this thread was not linked to from ubercart.org.

We are running into issues at the payment processing end. Other gateways are working, so we cannot identify the problem. We are going to try generating a new connection ticket, following more closely the readme.txt instructions for this.

Do you think that not having the latest release (here on the forum) could be contributing to our issues?

rerooting: I suggest you open an issue for this on the project issue queue: http://drupal.org/project/issues/uc_qbms

nice! very useful that you guys now have a project page! i will commit an issue once I have tested against the latest CVS release