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
Comment | File | Size | Author |
---|---|---|---|
#9 | 2088391_commerce_checkpay.patch | 1.78 KB | AjitS |
checkpay_screenshot.png | 47.77 KB | Fernly |
Comments
Comment #1
t14 CreditAttribution: t14 commentedHi
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
Comment #2
t14 CreditAttribution: t14 commentedComment #3
Fernly CreditAttribution: Fernly commentedThanks 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!
Comment #3.0
Fernly CreditAttribution: Fernly commentedGit link edited
Comment #4
PA robot CreditAttribution: PA robot commentedThere 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.
Comment #4.0
PA robot CreditAttribution: PA robot commentedExtra information on the checkpayentity.inc file.
Comment #5
Fernly CreditAttribution: Fernly commentedI 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.
Comment #6
softescu CreditAttribution: softescu commentedAutomated 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.
Comment #7
Fernly CreditAttribution: Fernly commentedThanks softescu.
I fixed all the remarks mentioned. I made the cert files private by adding an .htaccess file in the directory.
Greets
Comment #7.0
Fernly CreditAttribution: Fernly commentedChanged file name
Comment #8
AjitSMore thank two weeks of no activity. Changing the status.
Comment #9
AjitSThank you for your contribution. I think you are pretty close to getting this published. Here are my findings:
admin/commerce/config/payment-methods
as the configuration path in your info file but, didn't create the menu by implementinghook_menu()
. I am not sure how did you access you configuration form :commerce_checkpay_settings_form()
.I went ahead and did those changes. Attaching a patch for the same. Also, added a permission to access the configuration menu item.
Comment #10
Fernly CreditAttribution: Fernly commentedHi 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!
Comment #11
Fernly CreditAttribution: Fernly commentedChange status.
Comment #12
Fernly CreditAttribution: Fernly commentedAny other tips to refine the module and make it release ready?
Comment #13
kscheirer'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.
Comment #14
Fernly CreditAttribution: Fernly commentedHi 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!
Comment #15
dutchyodaI just ran it through an automated review tools and got some results back. Mostly concerning coding standards:
http://pareview.sh/pareview/httpgitdrupalorgsandboxlennartvv2088337git
Comment #16
kscheirer"needs work" is for when you find a blocking issue. Just running a report on pareview.sh isn't enough.
Comment #17
Fernly CreditAttribution: Fernly commentedHowever... 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.
Comment #18
Fernly CreditAttribution: Fernly commentedAnyone any more tips to promote this project to a full project?
Comment #19
xqus CreditAttribution: xqus commentedI 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.
Comment #20
Fernly CreditAttribution: Fernly commentedHi 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!
Comment #21
Fernly CreditAttribution: Fernly commentedAny more advise?
Comment #22
Fernly CreditAttribution: Fernly commentedComment #23
Fernly CreditAttribution: Fernly commentedComment #24
Fernly CreditAttribution: Fernly commentedComment #25
gobinathmPAReview is still reporting issue on your GIT repo. Please fix them as well.
Comment #26
Fernly CreditAttribution: Fernly commentedI 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.
Comment #27
klausiIf 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.
Comment #28
Fernly CreditAttribution: Fernly commentedThanks 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?
Comment #29
Fernly CreditAttribution: Fernly commentedReviews of other projects added.
Comment #30
Fernly CreditAttribution: Fernly commentedI 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:
Any advise on how to proceed is very appreciated. Thanks in advance.
Comment #31
klausimanual review:
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.Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.
Comment #32
Fernly CreditAttribution: Fernly commentedNice review, klausi.
Thanks again and if I make it to Drupalcon Amsterdam, maybe see you there!
Comment #33
Fernly CreditAttribution: Fernly commentedNew reviews added.
PAReview: review bonus added
Comment #34
klausimanual review:
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!
Comment #35
Fernly CreditAttribution: Fernly commentedThanks 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.
Comment #36
patrickd CreditAttribution: patrickd commentedThe 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.
Comment #37
Fernly CreditAttribution: Fernly commentedThanks to everyone involved!