Full featured module for Ubercart to accept Wallet one payments on your website.

Features:

  • typical installation
  • ability to browse payments in admin interface and taking actions on them(deleting and enrolling)
  • ability to allow or dissalow some wallet one payment methods
  • ability to select wallet one payment methods on your site or on wallet one site
  • ability to select widget for wallet one payment methods list
  • ability to set up (add,remove,rename) wallet one payments methods in payment_methods.ini file
  • setting up this module as other ubercart payment module /admin/store/settings/payment

Dependencies:

Ubercart
Cart module from Ubercart project
Payment module form Ubercart project

Project page
https://www.drupal.org/sandbox/moelius/2311643
Sandbox
http://cgit.drupalcode.org/sandbox-moelius-2311643
Pareview
http://pareview.sh/pareview/httpgitdrupalorgsandboxmoelius2311643git
Git
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/moelius/2311643.git
uc_w1
cd uc_w1

Files: 

Comments

moelius’s picture

Issue summary: View changes
tkuldeep17’s picture

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.

moelius’s picture

Issue summary: View changes
moelius’s picture

Issue summary: View changes
moelius’s picture

Hi tkuldeep17, thanks for your review. I added dependencies in project page & in .info file. And added menu_rebuild() in .module file. Realy, url not changed without rebuilding menu. This is very important suggestion. And fix @see uc_payment_method_uc_w1() - it was because i renamed functions for pareview and forgot about it.

moelius’s picture

Issue summary: View changes
moelius’s picture

Issue summary: View changes
moelius’s picture

Issue summary: View changes
moelius’s picture

Issue tags: +PAReview: review bonus
moelius’s picture

Issue summary: View changes
moelius’s picture

Issue summary: View changes
moelius’s picture

Issue summary: View changes
moelius’s picture

Issue summary: View changes
moelius’s picture

Issue summary: View changes
tkuldeep17’s picture

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.

moelius’s picture

Thanks for your time tkuldeep17 .
But all of this variables are useless, before administrator set up mudule for first time. Module uses API and first set up is required. I think menu_rebuild is nessesary if administrator change this url variables. I'm wrong?

tkuldeep17’s picture

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.

moelius’s picture

I think when administrator will fill wallet API details and submit them, then these menu item should be available to drupal

May be my english so bad and i don't understand what you mean, but:
My actions:

  • turn off module
  • delete module
  • testing /uc_w1/done page. result: no page.
  • then i turn on module. Fill module details. submit.
  • testing /uc_w1/done. Page created and works without flushing cache or manualy rebuilding menu.
tkuldeep17’s picture

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.

moelius’s picture

Please do a step more after disabling module, uninstall it

I did it:

  • turned off module
  • deleteuninstalled module - that's i meant
  • rebuilt menu,just in case
  • testing /uc_w1/done page. result: no page.
  • checked variables with devel. result: no variables.
  • then i turned on module. Fill module details. submit.
  • tested /uc_w1/done. Page created and works without flushing cache or manualy rebuilding menu.

Test yourself please. It works.

moelius’s picture

So IMO you should not show wallet payment gateway until administrator does not fill api details.

Good idea, thanks. Did it.

moelius’s picture

Issue summary: View changes
mpdonadio’s picture

Assigned: Unassigned » mpdonadio

Assigning to myself for next review.

mpdonadio’s picture

Assigned: mpdonadio » Unassigned
Status: Needs review » Needs work
Issue tags: +PAReview: security

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.

moelius’s picture

Thanks, mpdonadio
Great review, and it's very important for me to learn, how to make drupal modules in proper way.
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.

Added ability to use logo or no. And added description: link to wallet one brandbook.

README.txt/README.md

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

fixed

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

fixed

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).

No they don't need to be dynamic. Fixed.

All of your variable_get() calls need default values.

fixed

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

fixed

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

fixed

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.

fixed. Commented.

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

fixed.

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

fixed

(+) 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?

fixed

-----------------------------------------------------------

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.

Look at standart uc_2checkout.module (210, 231, 234), uc_cybersource.module (510), etc. Also setting form action instead of curl() / drupal_http_request().
About session, it's used only to show administrator, what wallet one payment method customer used in admin order interface. I commented this in module. But if this very big security issue i can remove this ability.

Thank you very much for your review. It was very usefull and interesting fo me.

moelius’s picture

Status: Needs work » Needs review
laceysanderson’s picture

Status: Needs review » Needs work

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?

moelius’s picture

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.

translation or requirements (brandbook):
Requirements for the placement of the logo on websites W1
partners
General requirements
Only allowed to use the logo with a vertical layout.
Depending on something logo can be placed without any signatures
or with the caption "Wallet" or "Wallet One Checkout".

And i wrote official e'mail to "wallet one" with question, how can sites use their logo.

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.

This code style from PHP example of wallet one API. I think it is not bad to follow API standarts as possible, but may be i'm not right.

foreach($fields as $key => $val)
{
    if (is_array($val))
       foreach($val as $value)
       {
   print "$key: <input type="text" name="$key" value="$value"/>
";
       }
    else     
       print "$key: <input type="text" name="$key" value="$val"/>
";
}

i'l go and work with my code, comments e.t.c.=)

Thanks for review, laceysanderson.

moelius’s picture

Status: Needs work » Needs review

I deleted logo. It seems all problems are solved.

pingwin4eg’s picture

Issue summary: View changes
Issue tags: -PAReview: review bonus

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.

moelius’s picture

ok. I'l wait some years....)

naveenvalecha’s picture

Assigned: Unassigned » naveenvalecha

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

naveenvalecha’s picture

Assigned: naveenvalecha » Unassigned
Status: Needs review » Reviewed & tested by the community

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 :)

naveenvalecha’s picture

Issue tags: +PAReview: Ubercart

Added Pareview ubercart tag.

kscheirer’s picture

Status: Reviewed & tested by the community » Fixed

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:

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.

moelius’s picture

Thank you naveenvalecha, very usefull review. Thanks for your advice kscheirer.

Status: Fixed » Closed (fixed)

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