Ubercart Chinapay

Hello, and thank you 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; for modules, it should include also a comparison with the existing solutions.

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.

Hi hibersh,

We only review one project as part of this application process. Which of your sandbox projects would you like us to review?

Hi hibersh,

Great, thanks. Ubercart modules often benefit from review by someone more familiar with Ubercart. I'm updating the title and adding a tag to make it a little easier for such people to find. As you've already seen, this can sometimes be a very slow process. Thanks for your patience.

You can't review your own application., but if you're willing, you can use your own Ubercart knowledge to help review other Ubercart applications, after reading the review guide.

Updating issue priority, as per the new application priority guidelines.

Before adding tags read the issue tag guidelines. http://drupal.org/node/1023102

subscribe, hope to see the code in the sandbox version control.

I've avoided reviewing this because I think it would be better for someone with Ubercart expertise to review. But as no one with Ubercart expertise has come along, I went ahead and did a less-informed review. I found one minor issue. I think this is probably as close to a full review as we're going to get, so I'm marking this RTBC.

The file netpayclient.inc is full of non-namespaced functions, and looks like it could be a third party file. Please explain if you yourself wrote this file, and prefix all of the function names with uc_chinapay_.

As I've noted in a @todo, the t() function isn't meant to take variables. You'll need to resolve this another way.

Also, attached is a patch with some coding standards fixes.

Do you have two accounts? http://drupal.org/node/1175188/committers

http://drupal.org/user/1341534
http://drupal.org/user/900646

You cannot include external libraries. You will have to either namespace those functions, or remove the file and provide adequate download instructions in the README.txt.

# "netpayclient.inc" is from Chinapay, was obfuscated before(not free-open).

Can you elaborate a bit more on your comment regarding netpayclient.inc file? I'm afraid I don't fully understand the licensing situation. Can end users download this file (or an obfuscated original that this was derived from) from ChinaPay? If so, what are ChinaPay's license terms? And if not, is this your code, or a 3rd party?

Because contributed modules are hosted on drupal.org's infrastructure, there can be legal implications assiciated with licensing of 3rd party code ... so as reviewers, we need to ensure we (and you) fully understand the licensing details associated with any given project before we can mark it RTBC.

Thanks in advance!

hibersh,

Thanks for the response ... can the same be said of the ChinaPay logo? (i.e. Do you have rights to use the ChinaPay logo, and include it in a GPLv2 project?) I'm assuming that this would be a copyrighted image / registered trademark, and would likely require a release from ChinaPay before it could be included in the project bundle.

I'm not familiar with ubercart, so I may be wrong ... but I wonder if it may not be more appropriate to use '#type' => 'value' instead of 'hidden' on line 153 of uc_chinapay.module, to prevent the fields from being output to the client browser.

foreach ($data as $name => $value) {
  $form[$name] = array('#type' => 'hidden', '#value' => $value);
}

1. Please add a README.txt file, with a module overview and installation instructions. See http://drupal.org/node/161085 for more information.
2. Also add a @file document block for the netpayclient.inc file, explaining the file's purpose. Function documentation on the rest of the netpayclient.inc functions would be an added bonus.
3. uc_chinapay_pages.inc
   • Line 12:
     Using the @ prefix will cause watchdog variables to be wrapped in a check_plain() automatically, so that
     watchdog('chinapay', 'Receiving new order notification for order !order_id.', array('!order_id' => check_plain($_POST['Priv1'])));
     can be simplified as
     watchdog('chinapay', 'Receiving new order notification for order @order_id.', array('@order_id' => $_POST['Priv1']));
   • The same applies for t() functions (such as in line 33 of the same file).
4. I'll admit that I'm nervous about the number of times that $_POST appears in the code, but the couple of instances that I decided to trace through the ubercart code are never output to display.
5. I've seen other RSA implementations in php which require certain PHP extensions to be enabled, in order to effectively work with the large numbers which can be involved in the calculations ... does your implementation depend on any php extensions (such as gmp or bcmath)? If so, they should be documented in the README.txt file.

The README.txt file, netpayclient.inc @file docblock, and an answer to the Chinapay logo question in #26 are likely the only current showstoppers that I can see. That said, I don't know ubercart ... so the standard disclaimer applies.

On another note ... thanks for your patience and persistence with this module, and the review process in general!

Looks good ... key items in #28 answered or addressed, so let's push this through!

Please take a moment to make your project page follow tips for a great project page.

Thanks for your contribution, hibersh! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in minde: Commit messages - providing history and credit and Release naming conventions.

ugh, I keep forgetting this :(