eqock [eqock]

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.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.

We just review a module per applicant; the description does not report a comparison with existing solutions.

Please let us know which module you want reviewed.

I will review the code within 6 days; for sure, I will not review it in the next 2 days.

1. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.

2.   $cpiresultscode = array(
       'The transaction was approved.',
       'The user cancelled the transaction.',
       'The processor declined the transaction for an unknown reason.',
       'The transaction was declined because of a problem with the card. For example, an invalid card number or expiration date was specified.',
       'The processor did not return a response.',
       'The amount specified in the transaction was either too high or too low for the processor.',
       'The specified currency is not supported by either the processor or the card.',
       'The order is invalid because the order ID is a duplicate.',
       'The transaction was rejected by FraudShield.',
       'The transaction was placed in Review state by FraudShield.',
       'The transaction failed because of invalid input data.',
       'The transaction failed because the CPI was configured incorrectly.',
       'The transaction failed because the Storefront was configured incorrectly.',
       'The connection timed out.',
       'The transaction failed because the cardholder\'s browser refused a cookie.',
       'The customer\'s browser does not support 128-bit encryption.',
       'The CPI cannot communicate with the Payment Engine.',
     );
     if (isset($_POST['OrderHash']) && ($_POST['OrderHash'] == uc_hsbc_orderhash_generator($hashdata, $cpihashkey)) && ($_POST['StorefrontId'] == variable_get('uc_hsbc_payment_method_callback_settings_storefrontid', ''))) {
       foreach ($cpiresultscode as $key => $value) {
         if ($_POST['CpiResultsCode'] == $key) {
           watchdog('uc_hsbc', t('HSBC Result code #' . $key . ': ' . $value . $watchdog_message_details), $watchdog_message_details_array , WATCHDOG_INFO, NULL);
         }
   

   The call to watchdog() is not correct. The second argument is a not translated string (the translation is done from watchdog()).

3. Strings used in the user interface should be in sentence case. Acronyms should be written in upper case (URL, in example).

4.         '#description' => t("Don't forget to configure your") . ' <strong>' . t('Currency format') . '</strong> ' . t('in') . ' <strong>' . l(t('Store format settings'), 'admin/store/settings/store/edit/format') . '</strong>.',
   

   Rather than concatenating translated strings, it is better to translate a single string; in this way, translator users have more context to translate the sentence correctly. In example, in some languages the preposition in could be translated in different ways, basing on the context.
   If you use a single string, remember to not use l() for the link. The correct code is something similar to the following (taken from the documentation for t()):

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

5.   $cpiresultscode = array(
       'The transaction was approved.',
       'The user cancelled the transaction.',
       'The processor declined the transaction for an unknown reason.',
       'The transaction was declined because of a problem with the card. For example, an invalid card number or expiration date was specified.',
       'The processor did not return a response.',
       'The amount specified in the transaction was either too high or too low for the processor.',
       'The specified currency is not supported by either the processor or the card.',
       'The order is invalid because the order ID is a duplicate.',
       'The transaction was rejected by FraudShield.',
       'The transaction was placed in Review state by FraudShield.',
       'The transaction failed because of invalid input data.',
       'The transaction failed because the CPI was configured incorrectly.',
       'The transaction failed because the Storefront was configured incorrectly.',
       'The connection timed out.',
       'The transaction failed because the cardholder\'s browser refused a cookie.',
       'The customer\'s browser does not support 128-bit encryption.',
       'The CPI cannot communicate with the Payment Engine.',
     );
     if (isset($_POST['OrderHash']) && ($_POST['OrderHash'] == uc_hsbc_orderhash_generator($hashdata, $cpihashkey)) && ($_POST['StorefrontId'] == variable_get('uc_hsbc_payment_method_callback_settings_storefrontid', ''))) {
       foreach ($cpiresultscode as $key => $value) {
         if ($_POST['CpiResultsCode'] == $key) {
           watchdog('uc_hsbc', t('HSBC Result code #' . $key . ': ' . $value . $watchdog_message_details), $watchdog_message_details_array , WATCHDOG_INFO, NULL);
         }
   

   It is better to use t()-placeholders, rather than concatenating strings. The strings provided in $cpiresultscode are not translated.

6. function uc_hsbc_uninstall() {
     db_query("DELETE FROM {variable} WHERE name LIKE 'uc_hsbc_%%'");
   }
   

   That code would delete variables of other modules with a name starting with uc_hsbc_; it is better to delete each single Drupal variable (the name of each variable is well known, when the code is complete).

1. The code should use the Drupal functions, when available; in particular, it should use the Drupal Unicode functions (as reported from http://drupal.org/coding-standards).

2. function theme_uc_hsbc_cpireturnurl_complete($orderid, $userid) {
     $output = '<p>Your order is complete! Your order number is ' . $orderid . '.</p>';
     if (user_is_logged_in()) {
       $output .= '<p>Thank you for shopping at Our store. While logged in, you may continue shopping or ' . l('view your current order status', 'user/' . $userid . '/order/' . $orderid) . ' and order history.</p>';
     } else {
       $output .= '<p>Thank you for shopping at Our store. Your current order has been attached to the account we found matching your e-mail address.</p>';
       $output .= '<p>' . l('Login', 'user') . ' to view your current order status and order history. Remember to login when you make your next purchase for a faster checkout experience!</p>';
     }
     $output .= '<p>' . l('Return to the front page.', '<front>') . '</p>';
     return $output;
   }
   

   It doesn't seem the message is translated.

3.     foreach ($cpiresultscode as $key => $value) {
         if ($_POST['CpiResultsCode'] == $key) {
           watchdog('uc_hsbc', 'HSBC Result code #' . $key . ': ' . $value . $watchdog_message_details, $watchdog_message_details_array , WATCHDOG_INFO, NULL);
         }
       }
   

   See what I reported in my previous comment. See also the point about the code formatting.

4. The version line in the .info file should be removed as it is already added by the packaging script, and duplicating that line causes problems with the updates manager.

The class orderCrypto doesn't follow what the coding standards report about how to write a class. What is then the purpose of using a class that has only a public method, and that is only used once? In such case, it would be better to use a plain functions.

Thank you for a great contribution! I was about to dig into how to make the HSBC integration when I found your work. Great!

I have installed the latest version and tried to run transactions, but I get the "Possible hacking attempt" error. The return code from HSBC is 12. Any suggestions where I should start to dig to track down the problem?

I started working on my own uc_paylater until my colleague found this. I am now testing uc_paylater from comment #5

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.

Hey eqock,

For the first part, I would contact the module developer for uc_hsbc directly and point him to this post and the two of you can work out getting your features in his module, or his features in your module. If you find that they can be worked together, then I'm sure a drupal admin can give you git access to commit to those repos.

As far as uc_paylater, I did test out your module but it didn't quite accomplish what I was going for so I started over with a different approach. I created a sandbox, but I don't think I reserved a name or anything. I'm sorry I didn't post back her my findings as I should have, but the contractors life is a distracting one. I will schedule time next week to review my paylater and get all the code in the sandbox and I will review your paylater again to see why I didn't want to use it. Also please contact me via my contact page and we can start working together on it. Maybe we can promote a good module soon.

Also if you haven't already, go ahead and create two sandboxes for your two modules (per http://drupal.org/node/1075406). This will make it easier for other people to log in, see the code, and post and issue to it.

Hey eqock,

Contacting the authors of both modules and working with them directly would be the preferred approach ... both longwave and mrconnerton should have the ability to grant you commit rights on their respective modules without having to undergo a formal Drupal code review process (other than convincing the maintainer themselves). Even if they weren't willing to grant direct commit access, you can still contribute patches and bugfixes to those projects via their issue queues ... both of these approaches would be preferred over the introduction of a new module with similar functionality.

Regarding your application itself, I fully understand that you initiated it long before the Git migration. However, if you were to choose to continue with the application process, you need to follow the instructions in the 'Migrating from CVS Applications to (Git) Full Project Applications' link I provided; as folks are no longer actively performing reviews in this (the 'CVS Applications') queue.

Hi,

Where did you post your module for:

"Web Hosting (contribution module for Ubercart, enabling monthly and pro-rata billing and cPanel/WHM account creation using Ubercart API)"

Thanks!

This application has not gotten back a reply in almost 2 years. If you still need to apply for being a vetted user, open an issue in the new queue we are now using for this purpose.