haagendazs [haagendazs]

As I mentioned in IRC this is my first review, but I did find a few things you'll want to address. Clearly you've got a solid development background, and overall this module is quite good. I did some research, and there have been quite a few requests for CCBill integration with Drupal. Ubercart, or commerce integration would be great additions in the future.

1. Performance: You should consider making uid an indexed column, as I saw it in the WHERE clause in at least one query.
2. Remove the LICENSE.txt as this will be added automatically once uploaded to drupal.org.
3. Possible Security Issue: You are storing the password unencrypted and displaying it in a plain textfield. I would think this should be encrypted and displayed in a password field.
4. Coding Standards Warnings: Very minor, but there are quite a few lines with trailing spaces
5. Requirements: You should probably emphasize more that this module is not stand alone, and provides essentially an API for accessing CCBill.

Can someone pick this up that has approval permission? I'm quite happy with what haagendazs has submitted, but I can't take it any further.

Thanks!

• 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. Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

2. Menu descriptions and titles, as well as schema descriptions, are not passed to t().

3.   watchdog('ccbill', t('CCBill call registered for User ID %uid. (Subscription ID: %sub_id)'), array('%uid' => $ccbill['uid'], '%sub_id' => $ccbill['sub_id']));
   
   

   The second argument passed to the function is not correct.

4.   // Prepare the query to insert subscription details
     $query = "INSERT INTO {ccbill} (sub_id, uid, start_date, initial_period, initial_price, rebills, recurring_period, recurring_price) VALUES ('%s', %d, %d, %d, %f, %d, %d, %f)";
   
     // Execute the query
     db_query($query,
       $ccbill['sub_id'],
       $ccbill['uid'],
       strtotime($ccbill['start_date']),
       $ccbill['initial_period'],
       $ccbill['initial_price'],
       $ccbill['rebills'],
       $ccbill['recurring_period'],
       $ccbill['recurring_price']
     );
   
   

   Why isn't the code using drupal_write_record()?

5. The module doesn't remove the Drupal variables it defines when it is uninstalled.