CVS edit link for vikas_salyan

I am planning to create modules.
I am developing websites in Drupal for more than 1.5 year.

Recently we developed the website OPEN(http://openthemagazine.com), where we have integrated ICICI bank payment gateway for the magazine subscription (http://openthemagazine.com/subscribe) functionality.

I found it very useful and worth making it publicly available, specially for the India based users. So, I decided to develop a module which will add ICICI payment gateway as an option for the payment with Ubercart module(http://drupal.org/project/ubercart).

Please create my CVS account, so that i can submit my module for review.

CommentFileSizeAuthor
#10 uc_icici.zip14.81 KBvikas_salyan
#6 uc_icici.zip14.99 KBvikas_salyan
#4 uc_icici.zip11.49 KBvikas_salyan
#1 uc_icici.zip11.49 KBvikas_salyan

Comments

vikas_salyan’s picture

vikas_salyan commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new uc_icici.zip11.49 KB

Please review the module attached with this comment.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work
Issue tags: +Module review

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.

vikas_salyan’s picture

vikas_salyan commented
Status: Needs work » Needs review

Hello, and thanks for adding the review tags.

Here i am writing some more detail about the features of the module:

The Payseal - ICICI Payment Gateway module implements the Payseal i.e. ICICI bank's payment processing service (www.icicibank.com) in Ubercart. This will come as a boon to Ubercart users in India looking to use an Indian payment processing service.

What services are integrated?

* Website Payments Standard - Customers are redirected to payseal after reviewing their order to complete payment, they return to your Ubercart site once payment is completed.

What does it take to get started?

* In order to use Payseal - ICICI payment gateway as your payment solution, you must have a payseal account.

To use this module, do the following:

1. Get a merchant account from ICICI bank.
2. Unzip the contents of the module (or upload the unzipped folder named uc_icici)at ../sites/all/modules/ubercart/payment/
3. Enable the module at ../admin/build/modules
4. Enable payseal - ICICI payment Gateway as a payment method at ../admin/store/settings/payment/edit/methods by ticking the check box for EBS
5. Enter your ICICI account secret key.
6. Upload the jars files (provided by the ICICI bank as PHP SFA library) to the server. For more detail about how to configure Payseal, please read the SFA PHP Merchant Integration Guide provided by the Payseal and INSTALL.txt file inside the module.

vikas_salyan’s picture

vikas_salyan commented
StatusFileSize
new uc_icici.zip11.49 KB

HI kiamlaluno,

Waiting for the review of the attached module.
In the above comment (http://drupal.org/node/739392#comment-2725622), i have also written the detail motivation message as suggested by you.

Thanks

zzolo’s picture

zzolo commented
Status: Needs review » Needs work

@vikas_salyan, thanks for the application and patience. The following points are just a start and do not encompass all of the changes that may be necessary for your approval. Also, a specific point may just be an example and may apply in other places.

  1. What are the license and origin on the images files? Overall the policy is not to allow third-party resources.
  2. You should have basic README.txt to help explain the module. You can just change the INSTALL.txt to this probably.
  3. In the INSTALL.txt you mention PHP 4.3 as a requirement, but you set it as PHP 5.0 in the .info file.
  4. Place these files in modules/ubercart/payment/uc_icici:

    You suggest to users to put your module inside the ubercart module, but this is very bad as it is likely that this module will be erased during upgrading the ubercart module.

  5. Copy the ICICI payment gateway generated key file i.e. XXXXXX.key where XXXXXX is the merchant ID in modules/ubercart/payment/uc_icici/key.

    Depending on how sensitive this key is, having it stored in the file system of the site is insecure. Either have it stored outside the webroot of the site, or input it into the database.

  6. Please make sure you are following all the Drupal coding standards: http://drupal.org/coding-standards
  7.     'title'           => t('Process ICICI PG response'),

    Menu titles do not need to be wrapped in t().

  8.             $form['#prefix'] = '<table style="display: inline; padding-top: 1em;"><tr><td>';
            $form['#suffix'] = '</td><td>'. drupal_get_form('uc_icici_ssl_form', $order) .'</td></tr></table>';

        $title1   = '<span><img src ="' . $path . '/images/icicilogo3.gif" /><img src ="' . $path . '/images/icicilogo2.gif" />';
    $cc_types = array('visa', 'mastercard');

    $title2       =  '<br />' . t('Includes:');
    $cardtype_path = base_path() . drupal_get_path('module', 'uc_credit');
    foreach ($cc_types as $type) {
        $title2 .= ' <img src="' . $cardtype_path . '/images/' . $type . '.gif" style="position: relative; top: 5px;">';
    }

    You should be utilizing themes.

  9. You should utilize hook_requirements() in an .install file to ensure that your module cannot be installed unless it meets its dpendencies.
  10. You create variables, but do not remove then in hook_uninstall(). http://zzolo.org/thoughts/tip-managing-variables-drupal-module
  11.      ?>
     <html>
       <head>
         <script language="javascript" type="text/javascript">
         window.self.location='<?php print($url);?>';
         </script>
       </head>
     </html>
     <?php
     exit;

    You should utilizing print() here, and not using JS for redirection as some people may not have it enabled.

  12.         $mesg .= t('Error Occured.') . '<br />';
        $mesg .= t('Error Message: ') . java_values($opg_resp->getRespMessage()) . '.';
        drupal_set_message($mesg, 'error');
        uc_icici_redirect($base_url . '/cart/checkout');

    Use t() correctly.

  13. All function names need to be prefixed with module name.

--
Note: Please be patient with the CVS application process. It is all done by volunteers. Our goal is not to be arbitrarily slow or meticulous. Our goal is to get you CVS access and ensure that you are and will become a more responsible Drupal contributor. For a quick reference on what I look for in a code review, please see this article.

vikas_salyan’s picture

vikas_salyan commented
Status: Needs work » Needs review
StatusFileSize
new uc_icici.zip14.99 KB

@zzolo: Thanks for the detailed and very helpful feedback.

I have incorporated the all above changed suggested by you, Here i am attaching the latest version of the code.

Please review the module.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: Unassigned » avpaderno

I will review the code between 4 days.

vikas_salyan’s picture

vikas_salyan commented

@kiamlaluno: thanks.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Needs work 
  1.     for ($i = 0; $i < strlen($order->{$address .'_phone'}); $i++) {
        if (is_numeric($order->{$address .'_phone'}[$i])) {
            $phone .= $order->{$address .'_phone'}[$i];
        }
    }

    Code should use Drupal Unicode functions, when available. They allow to handle Unicode characters, as the charset set by Drupal is Unicode. 

  2.   $requirements = array();
  
  $file_exists = file_exists(dirname(__FILE__) . '/Sfa/java/Java.inc');
  $sfa_exists  = file_exists(dirname(__FILE__) . '/Sfa/PostLibPHP.php');

  // Ensure translations don't break at install time
  $t = get_t();
  if ($phase == 'runtime') {
      $error = FALSE;
      if (version_compare(PHP_VERSION, '4.3', '<')) {
        $error = TRUE;
        $severity = REQUIREMENT_ERROR;
        $value = $t('The uc_icici module requires the  PHP versions greater than 4.3.');
      }

    The correct way to get the path where a module is installed is to use drupal_get_path().
    Drupal 6 requires at least PHP 4.4; the code to verify is PHP is at least 4.3 is then superfluous. 

  3. /**
 * Implementation of hook_uninstall().
 */
function uc_icici_uninstall() {
  //Get global variable array
  global $conf;
  foreach (array_keys($conf) as $key) {
    // Find variables that have the module prefix
    if (strpos($key, 'uc_icici') === 0) {
      variable_del($key);
    }
  }
}

    The code could delete the variables of any module with a name starting with uc_icici (somebody could create a module named uc_icici_extension). The suggested way to remove Drupal variables is to rename them by their complete name. 

  4. php = 4.3.0

    As Drupal 6 requires at least PHP 4.4, that line is superfluous too.

  5. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted. 
  6.     $modulepath = drupal_get_path('module', 'uc_icici');
    // ICICI payment gateway code starts from here
    
    // Includes the required files
    require_once $modulepath . '/Sfa/BillToAddress.php';
    require_once $modulepath . '/Sfa/ShipToAddress.php';
    require_once $modulepath . '/Sfa/Merchant.php';
    require_once $modulepath . '/Sfa/PGResponse.php';
    require_once $modulepath . '/Sfa/PostLibPHP.php';
    require_once $modulepath . '/Sfa/PGReserveData.php';
    require_once $modulepath . '/Sfa/SessionDetail.php';

    In such cases, the function module_load_include() should be used. 

  7.     'access callback' => 'uc_icici_response_access',

    As the access callback always return TRUE, 'access callback' => TRUE is the faster way to define the callback (faster also for Drupal, that would not check if the function exists, call it, and check the return value). 

  8.             'req.Sale'             => t('Complete Sale'),

    Strings used in the user interface should be in sentence case.

vikas_salyan’s picture

vikas_salyan commented
Status: Needs work » Needs review
StatusFileSize
new uc_icici.zip14.81 KB

@kiamlaluno: Thanks for the review.

Please find the attached newer version of the module.

phantomvish’s picture

phantomvish commented

subscribing. When will this module be available for testing? I'm just a user and not a coder so can't help in review.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

I will make my next review within 6 days from July 8.

vikas_salyan’s picture

vikas_salyan commented

Hi kiamlaluno,

I truly appreciate your reviews you have done in past for this module.... But i desperately wants to see this module on drupal.org. Please, can u help me in that?

Thanks,
Vikas

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Needs review » Fixed

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

These are some recommended readings to help with excellent maintainership:

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.

Status: Fixed » Closed (fixed)
Issue tags: -Module review

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue tags: +Module review

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Component: Miscellaneous » new project application
Issue summary: View changes