[D7] Ubercart Wallet one

Hi Moelius,

I could not run this module with wallet, as wallet has not provided me settings which are required to use wallet. But I reviewed your code. Here are my some suggestion.

• Please instruct to developer download ubercart module, as uc_payment and uc_cart and other required modules are sub module of ubercart.
• You have to add also uc_cart with uc_payment depedency to your info file.
• Please follow coding standard in hook_menu function.
• I am not able to find out this uc_payment_method_uc_w1. You have written here
  

  /**
   * Implements hook_uc_payment_method().
   *
   * @see uc_payment_method_uc_w1()
   */
  function uc_w1_uc_payment_method() {

• In hook_menu you are defining path by variable_get('uc_w1_done_url'), variable_get('uc_w1_success_url') and variable_get('uc_w1_fail_url').
  and you are setting these values in function uc_w1_payment_method. But hook_menu function is called when you install module. So for recongzing these new menu items, you have to force to developer clear the menu cache or clear your self by calling function `menu_rebuild()` after submition of this form uc_payment_method_settings_form.

  You can do this by adding custom submit handler to this form uc_payment_method_settings_form, there you can call this function menu_rebuild(); And add also some condition, so for each payment method, your custom submit handler does not get called.

PS: Nice Module, Good work.

You have invoked function menu_rebuild in itself form. But on first time module installation the variable uc_w1_done_url and so on are empty. Also their value will change after form submission. So it will not save the new menu items, as value would be empty or contain old values. :-) :-)
I told you to add in submit handler.

I am attaching patch. Use it, if it is appropriate for you.

I think when administrator will fill wallet API details and submit them, then these menu item should be available to drupal, as these menu items would be used in checkout completion. So for this we need menu_rebuild function to be called just after form submission else administrator has to clear the menu cache.

And also When I tried to use this module, I could see wallet payment gateway after checkout. So I had redirected to wallet site without API details and I saw error page. So IMO you should not show wallet payment gateway until administrator does not fill api details.

Please do a step more after disabling module, uninstall it. Then do all the steps you have done. And then without clearing the menu cache test the url.

Assigning to myself for next review.

Automated Review

Review of the 7.x-1.x branch (commit c1f315d):

• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes/No: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

(*) Maybe: Follows the guidelines for 3rd party code, but the origin of the logo needs to be investivated. If it is copyrighted, it needs to be removed.

README.txt/README.md

No: Doesn't follows the guidelines for in-project documentation and the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

No. If "no", list security issues identified.

While I am not familiar the UC, I think you need to plain the variables in uc_w1_payment_method('cart-details') if that is really output that goes right to the screen. See https://www.drupal.org/node/28984

Coding style & Drupal API usage

(*) Using variable_get() for the hook_menu path isn't the best idea. Do these really need to be dynamic? At the very least, you need a default value (that is the reason for the blocker).

(+) Your URL checks in uc_w1_uc_payment_method() won't work if Drupal is in a subdomain.

(+) Why the need for session in uc_w1_uc_payment_method() and elsewhere? Refactor or comment on why it is really needed. I suspect these really need to be variables.

All of your variable_get() calls need default values.

(+) You have a handful of untranslated strings (uc_w1.module lines 178, 196, etc). Double check the module and wrap these in t().

(+) iconv() isn't always available in PHP installations. I think you can just use drupal_convert_to_utf8().

There is no hook_form_modify. Do you mean hook_form_alter? And why would you need to unset all of those anyway? Comment would be needed.

uc_w1_print_answer() isn't used anywhere in the module.

Your interactions with the W1 API shouldbe documented with some @see comments to their docs.

(*) print_answer() is called but not defined anywhere?

(+) In uc_w1_payment_method('cart-details'), the menu rebuild should really be part of a submit handler and not for form definition itself.

(+) In uc_w1_print_answer, drupal_exit() should be instead instead of exit() to allow hooks to run. Does this also need a proper Content-Type set?

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

To be honest, I stopped with my review after some of these findings. The security issue need to be addressed. However, even without the security issues and the
menu path problem, I would have sent this back to needs work. The use of session combined with how the payment actually gets sent (setting form action instead of curl() / drupal_http_request()) look like major problems
with the form API. My suggestion is to look at how other UC payment methods work. Typically, the Paypal integration is used as the example module of how to do things. I
think this will be a good addtion to the module space (non-US centric ones always are), but this needs a good amount of work before it is ready for widespread use.

Hi moelius,

When mpdonadio mentioned

(*) Maybe: Follows the guidelines for 3rd party code, but the origin of the logo needs to be investivated. If it is copyrighted, it needs to be removed.

, he meant: Do you have permission to use this logo? I found the following document on the Wallet1 website but am unable to read it -http://www.walletone.com/shared/service/page/380/brandbook-w1.pdf. If it says that the logo can be used in third-party software or that it can used anywhere as long as the correct spacing/colouring is used then you should be ok but if it's restricted then you will have to remove it from your module. This is to ensure you don't infringe upon copyright. Simply allowing the administrators to choose not to use it is not enough since those who do choose to use it would be infringing copyright laws if you don't have permission. Is this more clear?

Your README.txt looks much better :)

Your code could do with a lot more in-line comments. The recommendation is for a well-balanced number of in-line comments. I would recommend having a comment above each if/else statement to explain the condition you're testing in plain english (ie: not pseudo-code). You need @param and @return comments in your function headers.

I felt the code in your uc_w1_form() form was particularly hard to read and barely recognizable as a form. I think this would be much better written as you did your settings form rather than programatically creating the array in the foreach's on line 311 & 319.

The .module file is loaded on every page request. It would be better, performance-wise, to move all the code in your .module file except the hook_menu() definition into an include file. This way you can include that file only on the pages needed (ie: during the checkout process & in ubercart administration).

I suggest you change the name of uc_w1_form_modify() since it looks too much like hook_form_alter. It would be less confusing to call it something like uc_w1_form_remove_id()

I'm moving this back to "Needs Work" since the logo licensing needs to be resolved. Can you better explain the copyright?

Removing review bonus tag, you have not done all manual reviews. One of the links you provided is not a project application review at all, and in other comments you just repeated the output of an automated review tool.

Make sure to read through the source code of the other projects, as requested on the review bonus page.

Assigning to myself for next review that may be tomorrow tonight.

First Thanks for your contribution.Not found any blocker,So setting this to RTBC.

Automated Review

Best practice issues identified by pareview.sh / drupalcs / coder.Please see http://pareview.sh/pareview/httpgitdrupalorgsandboxmoelius2311643git

Review of the 7.x-1.x branch (commit c557d0b):

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and/or fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements.

3rd party assets/code

Yes: Follows the guidelines for 3rd party assets/code.

README.txt/README.md

Yes: Follows the guidelines for in-project documentation and/or the README Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes: Meets the security requirements.

Coding style & Drupal API usage

1. hook_help() is missing in this module.It would be nice if you add the hook_help in the module.
2. uc_w1_form : $form['#action'] = 'https://www.walletone.com/checkout/default.aspx'; Keep the action in constant because it can be changed later if sandbox support will be added.
3. uc_w1_payment_method : All of your variable_get() calls need default values.
4. uc_w1_form : date_default_timezone_set('UTC'); Please add a comment the need of setting the default timezone.
5. uc_w1_payment_check : Use drupal_strtoupper is prefferred over strtoupper.Similarly in uc_w1_print_answer
6. uc_w1_payment_end :I have not seen the usage of $output variable.Please add a comment.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

I would recommend you, please help to review other project applications to get a review bonus back. This will put you on the high priority list, then git administrators will take a look at your project right away :)

Added Pareview ubercart tag.

Non-blocking issues:

• Awesome for putting the translation right on the project page! I wish d.o could allow multilingual project pages
• In uc_w1_payment_check() Drupal code standards use && and || instead of and/or for logic
• Consider using drupal_strtoupper() instead of strtoupper()
• Try to avoid concatenating strings in t(). For example, t('Bad order state') . ' ' . check_plain($_POST['WMI_ORDER_STATE'])) is better as t('Bad order state @state', array('@state' => $_POST['WMI_ORDER_STATE'])). This makes the string easier for translators since the context is preserved. Also by using the @ you get a check_plain for free.
• It's strange to set the timezone directly, usually you want to leave that up to the site admins

Thanks for your contribution, moelius!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

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.

Thanks to the dedicated reviewer(s) as well.