[D7] Commerce Smartpay

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.

Hi karengreen

• Check out the Drupal documentation and comment standards (https://www.drupal.org/coding-standards/docs) for examples on commenting function parameters. For example, I'd suggest adding some @param comments in the commerce_smartpay_transaction function to clarify what each parameter is used for.
• Pareview.sh http://pareview.sh/pareview/httpgitdrupalorgsandboxkarengreen2348071git looks fine, however, there are some char length warnings exceeding 80 characters.
• You may want to include a little more detail about this module in your description.

Hi Grey,

There are some coding standard issues.

1- All the constant have coding standard issue in the file commerce_smartpay_constants.inc.

Module-defined constant names should also be prefixed by an uppercase spelling of the module that defines them.

Ref: https://www.drupal.org/coding-standards#naming

Hello,

This module looks very well written, good job! I did find a few minor issues:

commerce_smartpay.admin.inc
line 20, 35, 42: There are new line characters here, but not under the other form arrays. It is inconsistent.
line 76: You are setting a lot of variables, but when the module is uninstalled they need to be cleared. Use hook_uninstall and variable_del() to clean up variables.
* These variables you are using with variable_get() should be prefixed with "commerce_" to match the module name.

commerce_smartpay_form.inc
line 36: It might be safer to cleanse the data in $_GET. Who knows what people might put in there.
line 113: You are using entity_metadata_wrapper() (Awesome btw) but do not require the entity module in your .info file

I agree with devendra.yadav that the constants should be renamed. If someone created a module called Smartpay (maybe for ubercart or just used this name for some other reason) you could run into issues with duplicate constants. It would probably never happen, but because of the name spacing you should update these.

Looks good! Yes, check_plain() is probably the best option as you are not expecting any HTML.

Other options would be:
filter_xss()
filter_xss_admin()
check_markup()

but check_plain() is the one you want here.

In hook_redirect_form: Do not put urls into translated strings because it makes translation more difficult.
Use variable replacement: t("Smartpay hosted payment is not configured for use.
Set a merchant account on the settings page.")

Hi Karengreen,

You have not reviewed any application. So you need to review some sandbox projects for bonus points.

As a developer from Commerce Guys, I reviewed this project today as part of DrupalCamp Brighton sprint, and recommend for approval.
Have spoken to David Kitchen of Commerce Guys, who will further review and update status.

Pete

This module follows good practice for Payment Gateway development.

All other coding standards etc. have already been followed.

Set to reviewed.

I absolutely agree with the previous comment, very clean code - read nice :)

Nicely done, my only comment is that in commerce_smartpay_form_redirect_form_validate(), I think $order has already been loaded via entity_metadata_wrapper(), so you can access the amount as $order->commerce_order_total->amount->value() and $order->commerce_order_total-> currency_code->value()

Thanks for your contribution, karengreen!

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:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

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.

Hi Karengreen,

I am getting the following error.

I want to state that i am using the invalid account key for Smartpay Merchant Account.

Thanks
Devendra

This project application is closed. Please post bug reports to the module's issue queue.

Hi Karengreen,

I am undestaning the same thing but is it possible that we cab handle this by redirecting some where having a proper message.

Thanks & Regard
Devendra

@bartuc @karengreen - the check_plain filtering added to the module as part of this review seems unnecessary to me. See https://www.drupal.org/node/263002 for some details on where/how filtering should be done in Drupal. Doing the filtering on input means that you have no idea what context the variables will be used in (html? sql?, email headers?) so the filtering may not be appropriate. The variables are passed off to commerce_smartpay_transaction which itself calls commerce_payment_transaction_save. I assume that commerce_payment_transaction_save does appropriate filtering of things like $transaction->status and the code here is using @placeholders on the message which will perform check_plain a second time.

Adding this feedback here on a closed application since it's not a bug in the module, but feedback about the way the module was changed as part of the review process.

Thanks @bartuc for the reviews. Congrats karengreen and thanks for contributing a module!