Summary

WorldPay Business Gateway integration for the Drupal Commerce payment and checkout system.

  • Project page
  • This is Drupal 7.x module. It depends on Commerce
  • Checkout: git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/magicmyth/1433370.git commerce_worldpay_bg

See the projects sandbox page for further details.

Since this is a payment module scrutinising this is a good idea, though I am quite confidant in it. A (not-launched) site using this has passed WorldPay's approval process so I think it is ready to get the wider Drupal community looking at this module.

Further info

You may want to also review some of WorldPay's documentation:

Note

Existing Commerce WorldPay module

I know there is a commerce_worldpay project but this never budged of the starting line and it does not have a suitable namespace as WorldPay offer several other online payment processing projects. One of which I have already started development on (commerce_worldpay_xml_hosted will be released at a later date) so this namespace is certainly needed.

Naming

Please note although I want the project refered to as "Commerce WorldPay Business Gateway" I do not want the modules name to be commerce_worldpay_business_gateway but instead: commerce_worldpay_bg to keep namespace typing practical. I bring this up now as I have not created a "Drupal project" before and don't know if there may be issues with this.

Thanks
Adam

Comments

magicmyth’s picture

magicmyth commented
Status: Active » Needs review
Cristian.Andrei’s picture

Cristian.Andrei commented
Status: Needs work » Needs review

You need to explicitly state both the project's git url and the project page in your issue summary.

Also, have your code reviewed by ventral.org/pareview. There is quite a substantial amount of errors that need to get fixed (nothing major, only drupal syntax specific things and trailing white spaces).

Cristian.Andrei’s picture

Cristian.Andrei commented
Status: Needs review » Needs work
magicmyth’s picture

magicmyth commented

Thanks for pointing out the ventral.org tool. I never new about that (I had been running Code Review). I've now done a lot of cleanup on the most practical errors to fix. Note that ventral.org complains about some uppercase HTML elements in the example templates. These are not HTML elements but WorldPay's proprietary tags and must be uppercase. Also it is complaining about the @see in these templates that point to their respective pre-processes. I've simply followed the same style as node/node.tpl.php there. I think I've nuked all trailing whitespaces. I'm not going to be putting CSS properties in alphabetical order at this time as it will likely shift a lot with more feedback from users/devs (it is mainly used by WorldPay itself).

The summary has been made more clear on the Project's page and a git clone statement added.

Cristian.Andrei’s picture

Cristian.Andrei commented

Thanks for making the proposed changes.

I had a look at your code and :

  • commerce_worldpay_bg.module, lines 438-488: there's some commented functionality there. Can you please elaborate on why is it there ? There is no comment explaining it and if it's not supposed to be there, please remove it.
  • commerce_worldpay_bg.module, line 689 : typo in comment
  • commerce_worldpay_bg.module, line 702 : commented code. This should be removed
  • commerce_worldpay_bg.module, commerce_worldpay_bg_txn_delete(): this function is not used anywhere, please remove it.
  • There's quite a bit of "TODO" notes, are these the subject of a next version release ? Or are they required for this module to be operational with edge cases ?
magicmyth’s picture

magicmyth commented

Just thought I'd drop a note that I have not forgoten about the module. Unfortunately work has kept me away from it longer than anticipated but I should be able to take a look at those points soon enough.

Thanks

michaellenahan’s picture

michaellenahan commented
Status: Needs review » Needs work

I have taken a look at the module as part of the Application Review sprint at Drupalcon Munich.

I just wanted to check in with you about these few remaining changes that are still needed - generally speaking it looks pretty good and there are no particular blockers - are you happy to do these last few things? I'm changing the status to needs work.

I was also wondering about a few things in the html. Did you use some kind of html generator to create it? Is this from a WorldPay example or something? These are also not blockers, just wondering about them.

Examples in http://drupalcode.org/sandbox/magicmyth/1433370.git/blob/refs/heads/7.x-...

line 54
<WPDISPLAY ITEM=banner>

line 68 etc
<span style=" font-family: ; font-size: ; color: ;">Thank you, your payment was successful</span><br>

Want to emphasise though that the module is good:
* Nice project description
* Nice links to WorldPay API
* Acknowledges Commerce Worldpay existing module and reasons for not using it
* Sandbox page has more information.
* Issues in the sandbox show the module is being used by others.

joachim’s picture

joachim commented

The logical short name for this project is already taken at http://drupal.org/project/commerce_worldpay

What I think should happen here is that when this module is ready, rather than promote the sandbox, its code should be added to the existing commerce_worldpay project. The repository for commerce_worldpay doesn't appear to have any code in it yet. The owner of the project should be contacted to ask if they want to join forces / hand the project over.

psynaptic’s picture

psynaptic commented

If this can be cleaned up to meet the coding standards, I'd be happy to grant access to commerce_worldpay.

I initially created the project as a client wanted it but then decided to go with ubercart so we ported uc_worldpay to d7 instead. A couple of people have submitted code to me for the module but none of them followed the coding standards so I kicked them back with a full review. Unfortunately they didn't fix the issues and the project remains empty.

magicmyth’s picture

magicmyth commented

Hi all

Sorry for the really late reply. Life and work and all :(

Really appreciate the feedback. I will try to resolve the issues mentioned as soon as I can but if anyone wants to contribute I'd welcome the help.

As for the naming of the project. Please keep in mind Worldpay does not offer just one online payment gateway product and I have actually written a Drupal module for both. One is this one which is based on simple HTML posts and the other is done via XML. I recently gained permission to release the later so I will push that into a sandbox soon.

If both could be consolidated to be based on a single module with a separate sub-module for each I would agree with the name change but the way they work is very different and code re-use is quite impractical between them.

Thanks

Adam

psynaptic’s picture

psynaptic commented

I think commerce_worldpay/business_gateway/business_gateway.module makes the most sense.

magicmyth’s picture

magicmyth commented

Would the module hook prefix not be business_gateway_ then? Is this not too geniric for its purpose? Are you suggest merging the XML version I mentioned into the same repoitory? Is commerce_worldpay_bg confusing for a module hook prefix? Sorry for all the questions. I really appreciated the advice people have put in here.

psynaptic’s picture

psynaptic commented

It's been a convention to have a single module for the provider with sub-modules for the different services. Please see http://drupalcode.org/project/commerce_paypoint.git/tree/refs/heads/7.x-1.x for an example of how I would recommend structuring it. I would be happy to assign the commerce_worldpay project to you since you have done so much work on it.

psynaptic’s picture

psynaptic commented

I have granted you full access to the commerce_worldpay project. Please open an application in the webmasters queue to have the ownership transferred and I will be happy to hand it over.

magicmyth’s picture

magicmyth commented

Thanks psynaptic. Really appreciated. I'm looking to spend time finishing up the module in December.

Are you referring to http://drupal.org/project/issues/webmasters when you mention the webmasters queue?

It makes sense to keep services of a provider together. Guess I'll have to merge together the two services into one repository. If I went this route though how would one go about handling a new service developed by someone else? Would I have to give them full access to the projects repository or can it be limited to a branch? I would be reluctant to give someone else access to the whole thing without first proving themselves a bit but would also hate to remove commit privileges of there own service just to be able to keep everything under one umbrella. Can we simply have git submodules for each of the project's sub-modules allowing each to have its own repository?

psynaptic’s picture

psynaptic commented

Yep, I think that is the right queue to request transfer of ownership for a project, although I could be wrong.

I have never used submodule with a drupal.org project so I don't know if that works with the packaging system, I somehow doubt it. Commit permissions are not granular at all on drupal.org, or github for that matter - you can't set access per branch.

The drupal.org way of collaborating is via patches. I personally like github's pull request feature and you can certainly mirror the drupal.org project on github and ask the aforementioned contributor to create a pull request to that, merge it in when it's ready, and push to drupal.org. You could just use the drupal.org bare metal approach and request a patch for the new module (in a sub directory of the main module). A third approach would be if the contributor doesn't use git but has the module already coded, they could just post an archive of the module in the issue queue and if you thought the code was of good enough quality you could commit it yourself, giving the contributor attribution for their efforts in the commit message.

If you feel the submitted code is of a high enough quality to build trust in the contributor, you could grant them access to commit directly to the drupal.org project. We don't want to put too many barriers up to stifle contributions but the code should be of good enough quality.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Closed (won't fix)

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented

We are working on this integration currently and making sure the result is pushed into the public worldpay_commerce project.
Contacted psynaptic for this and requested maintenance access there.

magicmyth’s picture

magicmyth commented

@miro_dietiker Thanks. Wish I had the time to properly see this through at the moment but real world keeps getting in the way. Please try to keep me in the loop on this.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

We tested the module and made some coding standard improvements and pushed it including the original commits from @magicmyth to https://drupal.org/project/commerce_worldpay

Will create a release there soon!

JaneL’s picture

JaneL commented
Title: Commerce WorldPay Business Gateway » Commerce WorldPay for XML hosted redirect

Hello, I'm developing a website that will use the WorldPay Global Gateway rather than the Business Gateway. Therefore the orders will be submitted in XML rather than HTML. Also, I'll be using the hosted redirect option, meaning that the order will be sent to the WorldPay site to be processed and then WorldPay sends a callback to my site to confirm whether the payment has been successful. I can see that @magicmyth has already said in comment #10 above that a combined WorldPay module (to cover both xml and html orders) would perhaps not be a good idea as they work so differently. The reason for using the xml method is that WorldPay offers a much bigger range of international payment options to shoppers with this method. In comment #20 above @berdir mentions that the commerce_worldpay module has been improved - do you know if this now works for the xml redirect service? I'm new to Drupal, sorry if my question is a bit long!

JaneL’s picture

JaneL commented
Issue summary: View changes

Amending summary based on review comment