Project desciption:

A payment method for Drupal Commerce for the MIGS payment gateway.

MIGS is used by ANZ eGate, CommWebb, Commonwealth Bank of Australia, Bendigo Bank, and other banks worldwide.

This module implements the "Merchant Hosted" method, where a customer enters their credit card details and these details are sent to the payment gateway for authorisation.

The module is currently being used in 2 live sites and has been approved by ANZ bank.

There is no existing Drupal Commerce payment method module for MIGS.

Note to reviewers: The easiest way to get this module up and running for testing is to install it on a Commerce Kickstart install. http://drupal.org/project/commerce_kickstart

Project page: http://drupal.org/sandbox/tripper54/1796164

GIT repository: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/tripper54/1796164.git commerce_migs_merchant

The project is for Drupal 7 and requires Drupal Commerce.

My project application reviews:
http://drupal.org/node/1856652#comment-6939456
http://drupal.org/node/1596172#comment-6933710
http://drupal.org/node/1888186#comment-6940900

Thanks for looking!

CommentFileSizeAuthor
#8 coder-results.txt937 bytesklausi

Comments

tripper54’s picture

tripper54 commented
Issue summary: View changes

Updated issue summary.

tripper54’s picture

tripper54 commented
Issue summary: View changes

added link to project review (1 of 3 for PAR bonus)

tripper54’s picture

tripper54 commented
Issue summary: View changes

added project review link (2 of 3 for bonus)

tripper54’s picture

tripper54 commented
Issue tags: +PAreview: review bonus

adding review bonus tag.

tripper54’s picture

tripper54 commented
Issue tags: +PAReview: Commerce

adding Commerce review tag

ain’s picture

ain commented
Status: Needs review » Needs work
Issue tags: -PAReview: Commerce

The main issue currently is that it's not possible to actually test the module which is the reason why it has been pending a review for some time.

If it's possible I'd advise to get a test account so that the community members can get your module running and functionality tested. Could even be temporary.

Automated review: everything OK.

Manual review found some issues:

  1. Coding standards: mixed underscore convention with camelCase convention, e.g. commerce_migs_merchant_getResponseDescription commerce_migs_merchant.module ln 385.
  2. Undocumented (and partly not used) parameters, e.g. commerce_migs_merchant.module ln 118-121.
  3. On some lines the line is wrapped (commerce_migs_merchant.module ln 224-231) whilst on some much longer lines it is not (commerce_migs_merchant.module ln 367). I'd keep the convention seamless and have the not-so-long ones on a single line, mainly for readability. In many spots you could also push the strings to variables or even use the static variables. Otherwise it's very jumpy to read through.
  4. Quite a few unused variables (commerce_migs_merchant.module ln 216, 217, 219, 222, etc.) need clean-up. You can use IDE warnings to spot them, both Netbeans or Eclipse will deliver nicely for it.
  5. Installation help is currently only reachable from the README. It would be useful to have it available more prominently, i.e. a paragraph on project page. You could also implement hook_help() to provide more support for the end-user.

Recommendations:

  1. Comments like // ERROR! would look nicer as a standard FIXME, TODO or XXX comments.
  2. It'd be recommended to break up long functions to a series of small ones for readability purposes in points where it makes sense, e.g. in commerce_migs_merchant_submit_form_submit.
tripper54’s picture

tripper54 commented
Status: Needs work » Needs review
Issue tags: +PAReview: Commerce

Thanks very much for the review, ain.

If it's possible I'd advise to get a test account so that the community members can get your module running and functionality tested. Could even be temporary.

Agreed it does make it difficult to review the module. I'll reach out to the bank and see if they're interested in providing a test account. Considering how many forms and security checks my clients had to go through to get their test credentials, I'm not hopeful they will offer any for public consumption though. They would not deal with me at all directly while I was working this up for the client.

Coding standards: mixed underscore convention with camelCase convention, e.g. commerce_migs_merchant_getResponseDescription commerce_migs_merchant.module ln 385.

-- fixed, thanks for spotting.

Undocumented (and partly not used) parameters, e.g. commerce_migs_merchant.module ln 118-121.

-- I have documented these parameters now. Yes some of them are unused, but that's the structure of those callbacks, as defined by the commerce_payments module.

On some lines the line is wrapped (commerce_migs_merchant.module ln 224-231) whilst on some much longer lines it is not (commerce_migs_merchant.module ln 367). I'd keep the convention seamless and have the not-so-long ones on a single line, mainly for readability. In many spots you could also push the strings to variables or even use the static variables. Otherwise it's very jumpy to read through.

Yeah, it's kind of tricky. I'm trying to strictly address everything the automated review throws up in an effort to get through the review process more quickly, although this sometimes comes at the price of readability (but not if you're using an 80 col display I guess). I've kept the wrapping there but made it more consistent.

Quite a few unused variables (commerce_migs_merchant.module ln 216, 217, 219, 222, etc.) need clean-up. You can use IDE warnings to spot them, both Netbeans or Eclipse will deliver nicely for it.

-- fixed, again thanks for spotting these. I think I just collected everything the bank threw back thinking I would use them later, then forgot about them.. I've added a comment noting there's more information in the full response string (which is logged in the transaction object) if people need it.

Installation help is currently only reachable from the README. It would be useful to have it available more prominently, i.e. a paragraph on project page. You could also implement hook_help() to provide more support for the end-user.

- -I've put some installation instructions on the project page, also implemented hook_help().

It'd be recommended to break up long functions to a series of small ones for readability purposes in points where it makes sense, e.g. in commerce_migs_merchant_submit_form_submit.

I've actually stripped a lot of code out of this function. it's shorter and easier to follow now.

jthorson’s picture

jthorson commented

A few suggestions for improvement (but not show-stoppers):

Indentation and line wraps
I don't really have a preference on whether long code lines are wrapped or not, as long as it's consistent within the module. You should definitely try to limit comments to under 80 characters (and wrap if they are going to exceed), but sometimes wrapping code can make it less readable. In any case, whichever approach you use, make it consistent throughout the module. (I'm looking at commerce_migs_merchant_submit_form() versus commerce_migs_merchant_submit_form_validate(), for example.)
Line 215: $message = sprintf(
I'd find this a little easier to read if the query string was defined as an array, and then built using drupal_http_build_query().
Line 242: if (strstr($response, "<html>"))
You may not want to print out the entire contents of the response in this case, as this message could be displayed in the watchdog error logs ... things like a second tag on a given page tends to offend the html validation fanatics (though technically, I guess it would be stripped out in the sanitization function). In any case, I'd suggest parsing the response to extract the actual $message content, and only sending the message text on to the watchdog message.

Otherwise, I didn't attempt to install the code ... but a quick visual scan looks pretty good.

tripper54’s picture

tripper54 commented

Thanks jthorson for the review - some excellent points made!

Indentation and wraps - I've done another pass - hopefully things are pretty consistent now.

drupal_http_buid_query - thanks, great suggestion. I knew there had to be a better way! That's implemented now.

$response logged to watchdog: I see your point, but I think I'm going to have to leave that one as it is. Reason being I don't actually know what would lie within the tags in this message. This check was taken from the bank's sample code without any documentation as to what to expect there. Usually a failed response will include response failure codes, I'm guessing this is for if their server is completely wonky and unable to process the request, or perhaps if the user enters the wrong URL for the gateway in the config. So I guess its 1) difficult to know what to expect and 2) worth capturing as much of the response as possible to help troubleshoot the problem. As you mention the html would be sanitised, so I think it's fine to leave it as it is.

Again, thanks for the review! I'm learning a lot going through this process.

ain’s picture

ain commented

Nice work.

I've done eCom work myself from ground-up for direct debit style payments that interact directly with the bank account. All banks I've worked with have a standard test framework for testing before you go live and it is accessible to all, connected to dummy accounts. I hope you can fetch a temporary test account.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus
StatusFileSize
new coder-results.txt937 bytes

Note that code lines are allowed to be longer than 80 characters, only comments must be shorter.

Review of the 7.x-1.x branch:

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:

  1. "variable_get('uc_migs_merchant_po_prefix', 'test')": wrong variable name, and is this still needed? Also elsewhere.
  2. commerce_migs_merchant_submit_form_submit(): why do you need check_plain() on the $message_args, it is not printed to the user anywhere. Make sure to read http://drupal.org/node/28984 again.

Although you should definitively fix those issues they are not critical application blockers, so I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

jthorson’s picture

jthorson commented
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, tripper54!

I updated your account to let you 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 get 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.

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

added review link for PAR bonus (3 of 3)