[D7] Commerce Est

Hi,

first of all there are quite a few issues to sort out such as indentation, whitespace.

You can find them all here:
http://ventral.org/pareview/httpgitdrupalorgsandboxbekirdag1905430git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Do not use ?> delimiter in end of file, it done by drupal's 'Prepend.inc' file itself.

Also change your status to Need review, If you want to review your module by community reviewer.

You should utilise t() where applicable, thus

drupal_set_message('We encountered an error processing your transaction. Please contact us to resolve the issue.', 'error')

should be

drupal_set_message(t('We encountered an error processing your transaction. Please contact us to resolve the issue.'), 'error').

Regards,
K.

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 :-)

Priority is normal, see http://drupal.org/node/539608

Don't forget to add the "PAReview: review bonus" tag as indicated in #1410826: [META] Review bonus, otherwise you won't show up on my high priority list.

Hi there,

Please, DO NOT activate by default a payment method. As soon as the module will be enabled, the payment method will be available for customers without any configuration set.
'active' => FALSE,

You should put in the description of your payment method that's a ON-Site solution.

Why didn't you used the normal setting form provided by Commerce_payment and Rules? This avoid the creation of a hook_menu and the storing variables in the database.

You should add more descriptions to your setting fields to help merchants to fill out them (what's needed, where they can find the info...)

I tried to pay with your module without filled out any settings and got an error. Customers shouldn't be in front of error pages like that.

Exception: Geçersiz bir slug seçtiniz. Seçenekler: garanti, akbank, isbank, finansbank, halkbank, anadolubank. in EST->__construct() (line 117 of /home/vagrant/public_html/drupal-commerce/sites/all/modules/contrib/commerce_est/est.inc). Backtrace:

functions commerce_est_transaction() and commerce_est_cvv_response() are not used. New or old code ?

You use a php class to process the payment, but this file is not declared as a file in the info file.
files[] = est.inc

Also, when you create a form in order to save things in the variables table, use system_settings_form (@see http://drupal.org/node/222158) it's easier and you don't have to save all the variables one by one. And when you use variables, think to remove them during the uninstall process.

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

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

Changing status to Active.

@Kartagis Why?

@bekirdag Copy links of your reviews to the issue summary.

Reviews from your comment #6. You should edit this issue and add them to the description. Because it is not obvious that you reviewed other project applications.

Review of the 7.x-1.x branch (commit 6946071):

• Remove all .settings files from your repository.
• No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

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. commerce_est_menu(): why do you need your own hook_menu()? Shouldn't you configure the payment settings with the payment rule and some commerce_payment hook? See http://cgit.drupalcode.org/commerce_stripe/tree/commerce_stripe.module for example and commerce_stripe_settings_form().
2. commerce_est_request(): this is vulnerable to XSS exploits. $response is directly coming from a third party source and I'm not sure if all content in it has been sanitized or not. You should wrap the print_r() statement with a check_plain(). And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
3. commerce_est_request(): the watchdog() call is wrong, WATCHDOG_DEBUG should be the next parameter of the function, not part of the variables array.

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

Automated Review

Best practice issues identified by pareview.sh seems to be clean.

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No duplication

Yes: Does not cause module duplication and fragmentation.

Master Branch

Yes: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt/README.md

No: Follows the guidelines for in-project documentation and the README Template. I have found that your README.txt is not following the README Template, please go through the mentioned link and that will surely help you to bring out a well detailed README.txt file.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes.

Coding style & Drupal API usage

1. Why did you left this attribute blank on line 37? 'code' => ''
2. I've also found x_amount is commented on line 90 I think you have used it above, if you really don't needed it then please remove.

This review uses the Project Application Review Template.

Looks like the issues you found are minor and not application blockers, anything else or should this be RTBC instead?

As the issues found are minor and not application blockers, moving this project to RTBC.

Thanks for your contribution!

Getting review bonus would help speed up the process and make sure it gets on the review admins radar.

Sorry for the delay, looks like you forgot to add the review bonus tag again?

manual review:

1. est.inc and xml_builder.inc appear to be 3rd party code from https://github.com/ozgur/php-est . 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.
2. commerce_est_request(): this is still vulnerable to XSS exploits. $response goes unsanitized to watchdog() with the "!" placeholder. You should do it like this: watchdog('commerce_est', 'Est response: <pre>@response</pre>', array('@response' => print_r($response, TRUE)), WATCHDOG_INFO);.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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