This project integrates CheckPay PostNL into the Drupal Commerce payment and checkout systems.
PostNL (Netherlands) offers the CheckPay payment method as a retrospective payment variant. The customer pays at checkout directly to PostNL. When delivering the package, the customer receives a code by SMS which can be used to verify the delivery. The money will then be sent to the merchant.
At this moment there is no Drupal plugin for this payment service.

Extra information: Required classes to make connection to the CheckPay web service are delivered by PostNL and put in the commerce_checkpay.classes.inc file.

Project page:
https://drupal.org/sandbox/lennartvv/2088337

GIT repo:
git clone --branch 7.x-1.x git.drupal.org:sandbox/lennartvv/2088337.git commerce_checkpay

Reviews of other projects
https://drupal.org/node/2168739#comment-8366937
https://drupal.org/node/2201809#comment-8511551
https://drupal.org/node/2199965#comment-8511599
---
https://drupal.org/node/2203417#comment-8513203
https://drupal.org/node/2205497#comment-8522091
https://drupal.org/node/2204535#comment-8522175

Files: 

Comments

Hi

I had a look at the code for your module and I added some comments below

Line 155 It would be better to use the HTML or ASCII definitions of the currency signs e.g t(£30) would give you £30.

Line 460 This function has no documentation. Also you may want to consider improving the document blocks across all the functions, I found this link very useful https://drupal.org/coding-standards/docs

Your .info file could do with more information such as
description = ...
core = ...
configure = admin/commerce/config/payment-methods (this will create a link from the module list page so that users can go straight to your configuration page)
dependencies = ... (I did not test the functionality of this module but it sounds like it works with the commerce module so this module will need to be declared here).

Hope that was useful
T

Status:Needs review» Needs work

Status:Needs work» Needs review

Thanks for the remarks. I updated the module as such.
Also the info file contained more info, but apparently not in the master branch. Obviously a dependency is required here. Committed to both branches now.
All comment blocks regarding functions of the commerce payment system are according the example payment module and similar payment modules. The others I corrected.

FYI: even though you didn't mention anything about it, the checkpayentity.inc file contains classes delivered by the payment provider (PostNL). I'll add this to the main description here.

Thanks again!

Issue summary:View changes

Git link edited

Status:Needs review» Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxlennartvv2088337git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Issue summary:View changes

Extra information on the checkpayentity.inc file.

Status:Needs work» Needs review

I corrected all warnings and errors of PAReview, except for those related to the naming conventions in the CheckPay classes. I can't change them because they are linked to the CheckPay web service.

Status:Needs review» Needs work

Automated code checks for standards and practices still show warnings and some formatting errors (you can ignore the naming warnings).

Manual review, tested with Commerce Kickstarter
- Module category is "E-Commerce", it is recommended to use a general existing category, for example "Commerce (Payment)" that includes other payment modules
- Small issue in commerce_checkpay.classes.inc:656, code does not check if certificate file exists on signing function call, as it does in commerce_checkpay.module:241 or commerce_checkpay.classes.inc:684
- Before first configuring the module, there are warnings about missing "extra_charge_fieldset" which should be provided in commerce_checkpay.module:36 with a default, it will be saved after saving the form
- Security issue: if /sites/all/modules/commerce_checkpay/certs/certs.txt is readable, then your certificate files are readable by anyone allowing potential attackers to sign with or impersonate your site identity. Either put a .htaccess in the folder, use a function to check for public readability and warn the user, or suggest storing the files in a non web root folder and add textfields in the form to add the path.

Status:Needs work» Needs review

Thanks softescu.
I fixed all the remarks mentioned. I made the cert files private by adding an .htaccess file in the directory.

Greets

Issue summary:View changes

Changed file name

Priority:Normal» Major
Issue summary:View changes

More thank two weeks of no activity. Changing the status.

Priority:Major» Normal
Status:Needs review» Needs work
StatusFileSize
new1.78 KB

Thank you for your contribution. I think you are pretty close to getting this published. Here are my findings:

  1. You'll need to add commerce as a dependency to you module, as adding only commerce_payment isn't enough. This is because commerce_payment is a sub-module of commerce. So, when I try to enable the module via drush, I get the following message:
    $ drush en commerce_checkpay
    No release history was found for the requested project (commerce_payment).
    Module commerce_checkpay cannot be enabled because it depends on the following modules which could not be found: commerce_payment    [error]
  2. You've mentioned admin/commerce/config/payment-methods as the configuration path in your info file but, didn't create the menu by implementing hook_menu(). I am not sure how did you access you configuration form : commerce_checkpay_settings_form().
  3. In the configuration form above you didn't add a submit button.

I went ahead and did those changes. Attaching a patch for the same. Also, added a permission to access the configuration menu item.

Hi AjitS, thanks for the review.
I added the commerce dependency to resolve the drush error.

Concerning point 2 and 3:
The path admin/commerce/config/payment-methods is provided by the commerce_payment module, so it should not be necessary to define this path again. You should be able to configure the payment method on this page by clicking on the Edit link next to the name of the payment method. Didn't this path work for you?

The module uses the payment method callback 'settings form', provided by the Commerce Payment module. CheckPay basically plugs into this module and uses existing functionality. The submit button is already provided in the rendered form thus not needed to be defined in the form array.

If I'm not mistaken, I won't be needing the patch. But thanks for the help!

Status:Needs work» Needs review

Change status.

Any other tips to refine the module and make it release ready?

Status:Needs review» Needs work
  • There are 2 unnecessary check_plains in commerce_checkpay.module
  • Don't use escaped quotes when passing strings through t(), instead use double-quotes on the outside.
  • I'm not sure about your handling of the extra charge settings. Wouldn't the charge be a fixed amount, and then converted to the transaction currency when needed? I'm not sure why the admin would have to set the extra charge currency symbol manually, and they definitely should not have to type in html codes.
  • I'm not sure how reading the certs in commerce_checkpay_submit_form_validate() and SecurityXML::GetPublicKey() is working. Using $base_url gets you a web path doesn't it, like http://example.com/, but the certs directory has an htaccess to deny from all. I guess PHP is deciding that the url is local and reading from disk anyway? I think it would be nicer to specify a server file path directly. Also, since users must place files here, add a check in hook_requirements() to make sure they are readable by the apache user.
  • 'The phone number must start with "06".' - I assume this is because only someone in Holland can use this service. They could never have a phone number from another country?
  • class SecurityXML extends SoapClient - where is SoapClient coming from? If you're relying on built in PHP classes, that should be documented and checked for in hook_requirements() as well. Or you can depend on a soap module that provides a class implementation for you. You're also using open_ssl functions that are not guaranteed to be installed - check for those as well.

Seems like a very useful module!

----
Top Shelf Modules - Crafted, Curated, Contributed.

Status:Needs work» Needs review

Hi kscheirer,

Thanks for the review. Learned some new stuff here!
I made all changes as you suggested.

The extra charge is not a fixed amount because the merchant has to be able to choose how many he wants to charge extra from a commercial point of view. I removed the prefix and suffix fields. The amount is now formatted by Commerce.
The phone number must start with 06 indeed, because it's a Netherlands only service. PostNL does the check on their side (the payment side) whatsoever, so I built it in to avoid unnecessary moving forth and back between the web shop and the actual payment service.

Greets!

Status:Needs review» Needs work

I just ran it through an automated review tools and got some results back. Mostly concerning coding standards:
http://pareview.sh/pareview/httpgitdrupalorgsandboxlennartvv2088337git

Status:Needs work» Needs review

"needs work" is for when you find a blocking issue. Just running a report on pareview.sh isn't enough.

However... I fixed the new PAreview "errors" and warnings which I was able to get rid of, just to make it complete. See #5 as reason for the remaining PAreview errors.

Anyone any more tips to promote this project to a full project?

Status:Needs review» Needs work
Issue tags:-commerce, -commerce payment gateway, -commerce payment

// Generate a payment redirect key.
$order->data['payment_redirect_key'] = drupal_hash_base64(time());

I don't know what this is supposed to be, but if this should be a random string, I would suggest using drupal_random_bytes() instead of time. If this needs to be random this can be a security issue (needs work status).

491   $transaction->payload[REQUEST_TIME] = $order->data;
Will cause a PHP notice.

Other than that I think the module looks fine. Just fix or tell me why you don't need to fix the payment_redirect_key and we can set it to RTBC.

Status:Needs work» Needs review

Hi xqus, thanks for your advise. I fixed the payment_redirect_key as suggested. It's indeed a random string.
I didn't change the payload yet. Could you tell me what exactly is causing a PHP notice? I'm not sure what to change here. Thanks!

Any more advise?

Issue summary:View changes

Issue summary:View changes

Issue summary:View changes

Status:Needs review» Needs work

PAReview is still reporting issue on your GIT repo. Please fix them as well.

Status:Needs work» Needs review

I have to refer to #5. The remaining warnings and errors are all about commerce_checkpay.classes.inc, which contains classes provided by PostNL. I can't change that.

Status:Needs review» Needs work

If the classes are third-party provided you must not include them on drupal.org. 3rd party code is not generally allowed on Drupal.org and should be deleted. This policy is described in the getting involved handbook. It also appears in the terms and conditions you agreed to when you signed up for Git access, which you may want to re-read, to be sure you're not violating other terms.

The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Thanks for the information, klausi.
The classes are third-party provided, yet not downloadable on a public space. The classes are not commercially licensed though. How should I approach this? Is there a recommended place to put the library on?

Edit: I'm talking to PostNL for their proposition on the library. But in the meanwhile: any other ideas for publishing the library?

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

Reviews of other projects added.

Status:Needs work» Needs review

I took another look at the classes and it's going to be hard to treat them as a library. It's just an extension on the PHP SoapClient and at some point a Drupal function is called. I won't be able to publish it as a standalone library.
I'm not sure we have to look at it as a library in the first place, but correct me if I'm wrong. It was a head start PostNL sent me to implement their web service.

I reduced the PAReview errors but the very last ones I can't get rid of:

  • __doRequest is not a PHP magical function but it sure is used in SoapClient. How to get around this?
  • I can't change the class names (by prepending the module name) because they correspond with the PostNL web service. Changing any of them results in failure of the call.

Any advise on how to proceed is very appreciated. Thanks in advance.

Status:Needs review» Needs work
Issue tags:-PAReview: review bonus+PAReview: security

manual review:

  1. commerce_checkpay.classes.inc: under what license did you obtain the code? Are you allowed to publish it as GPLv2+ on drupal.org? Please add that to the @file comment.
  2. commerce_checkpay_requirements(): the check for the php extensions should not be done on runtime, but rather on install time, because your module will not work without them, right?
  3. commerce_checkpay.info: "files[]" is missing for the class containing file, then you don't need to module_load_include() it from your code. The class registry will autoload them for you.
  4. "watchdog('commerce_checkpay', 'Transaction information after returning from CheckPay: !data', array('!data' => '<pre>' . print_r($transaction_info, TRUE) . '</pre>'": that looks suspicious to me. Since $transaction_info is directly coming from a third party source you should sanitize it when sending into watchdog to avoid XSS exploits. Use a check_plain() around the print_r(), then your pre tags will still work. Same in commerce_checkpay_redirect_form_validate() where you print $_GET unsanitized to watchdog(). And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
  5. commerce_checkpay_redirect_form_validate(): you are getting the $transaction_nr from $_GET here, meaning an attacker could supply any value. How do you make sure that the transaction actually belongs to the current order? Shouldn't you save the transaction ID somewhere on the order and then compare it? Or does commerce somehow do that for you? Please add a comment. Could an attacker get an order completed by simply pointing to another previously successful transaction?

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Status:Needs work» Needs review

Nice review, klausi.

  1. I checked with PostNL, the code is released under the license. Added as such in the file.
  2. Modified as such
  3. Modified as such
  4. Modified as such
  5. Good point here and fixed: Commerce doesn't do it automagically. I compare the order IDs: the order ID CheckPay returns and the active order ID. In case the IDs don't match, the order fails.

Thanks again and if I make it to Drupalcon Amsterdam, maybe see you there!

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

New reviews added.
PAReview: review bonus added

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

manual review:

  1. Your module does not include any test file, did you consider writing automated tests with simpletest? See https://drupal.org/simpletest
  2. "module_load_include('inc', 'commerce_checkpay', 'commerce_checkpay.classes');": you can remove that now, classes will be autoloaded by the registry.

But otherwise looks RTBC to me now.

Assigning to patrickd as he might have time to take a final look at this.

P.S.: Yes I will be at Drupalcon Amsterdam, looking forward to talk to you in person!

  1. I'm digging into the simpletest matter right now. Interesting material. I will try to make time to implement it.
  2. Overlooked one of 2 'includes'. Removed the 2nd one too.

Thanks again for your time.

PS and off topic: Since you're living in Austria, just got back from a week snowboarding at Austria, Ischgl :) First time Austria, great experience but the beer was expensive enough though. The pizza's great.

Status:Reviewed & tested by the community» Fixed

The classes could be documented a little better and maybe you shouldn't put all of them into the same file.. but beside that, reading your code gives the impression that you are experienced enough with PHP and the Drupal API's and that's all I need to know.

Thanks for your contribution!

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.

Thanks to everyone involved!

Status:Fixed» Closed (fixed)

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