[D7] Commerce Mobilpay

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

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,

Please check your module on ventral.org. There are so many error in your module. Please resolve it. Checkout this link :

http://ventral.org/pareview/httpgitdrupalorgsandboxbogdanru1905678

Regards
Lexicon

You don't include third party code in your module, use instead files[] in .info and you can use the Libraries API module.

Removing review bonus tag, you have not done all manual reviews, you just posted the output of an automated review tool. Make sure to read through the source code of the other projects, as requested on the review bonus page.

I removed the automated review comment links from the issue summary.

removed automated reviews

manual review:

1. commerce_mobilpay_uninstall(): that select query is bad, because you would also delete variables of possible submodules. Example: if I create a commerce_mobilpay_ajaxfoo module you would also delete variables of that module. Better use an explicit list of your own variables.
2. commerce_mobilpay_completion_access(): this function can be removed, just use 'access callback' => TRUE in hook_menu().
3. commerce_mobilpay_order_form(): so the billing address is hardcoded to romania? Why? This should also be mentioned in README.txt and the project page?
4. "$transaction->message = t('Plata este in curs de procesare la MobilPay.');": all user facing text in code must be in English and should be translated with t().
5. The Libraries API module is a recommended method for adding 3rd party dependencies without having them in the module folder (easier to upgrade later).
6. Be careful where you place your keys! By default they should be in the private files directory, which is protected from unauthorized downloads by just accessing the URL. "sites/default/keys" is not a safe default and allows anonymous users to download the keys, if you don't make additional webserver protection rules.

So the last point looks like a security blocker, can you elaborate a bit on that? Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual reviews

• commerce_mobilpay.module
• line 70 : offsite_autoredirect should be boolean
• line 170 : you don't need to check if $user variable is defined. Maybe you should send the order owner's id here instead of the current user ?
• line 198 & 199 : you must build your url using url() function. eg:
  

  url('checkout/mobilpay/confirm', array('absolute' => TRUE));
  

• commerce_mobilpay.pages.inc
• line 12 : unused variable $base_url
• line 25 to 145 : all your comments are in Romanian. Comments must be in English.

Private key file

Handling your private key file is a critical security point. You should take advantage of the Drupal File API to achieve it, here's some of the advantages :

• File is securely stored in Drupal's private folder.
• no need to manually upload the file on the server. Just upload it using a form.
• no restriction on filename

There's a simple example here.
Learn more about File API.

libraries support

manual review:

1. commerce_mobilpay_settings_form(): why is the post_url a text field and not a select box?
2. commerce_mobilpay_settings_form(): do not use variable_set() in a form building function! Use it in a submit callback, if you must. Why do you need it anyway here?
3. commerce_mobilpay_settings_form(): $default_currency is unused.
4. commerce_mobilpay_settings_form(): why do you use system_settings_form() here? I guess that will pollute your variable table with all the form elements? It could even overwrite the setting of another module?

Although you should definitely fix those issues they don't seem to be application blockers, so I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

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

I am usually invoked to nitpick. Well, here it goes -- none of this blocks:

line 59, 'type' => MENU_NORMAL_ITEM is not necessary, it's the default and can be ommitted.

line 69 if (isset($items['admin/commerce/config/advanced-settings'])) { could use a comment. Why this path?

line 119, there's a drupal_map_assoc function to save you from typing the urls twice and to better indicate the key and the value are the same.

line 166 number_format(floatval($total['amount'] / 100), 2, '.', ''); there is no need for floatval.

line 173 $x509_file_path = drupal_realpath($public_key->uri); drupal_realpath is rarely necessary. It says so "The use of drupal_realpath() is discouraged" Did you try without? Same in pages.inc line 87.

line 194 isset($customer_address['first_name']) ? $billing_address->firstName = $customer_address['first_name'] : ''; this is either a superb clever trick here or it's not what you want -- this will run $billing_address->firstName = $customer_address['first_name'] when the first name is set and do nothing when it is not. If this is the desired behaviour, if (isset($customer_address['first_name'])) { $billing_address->firstName = $customer_address['first_name'];} is way more readable than the abuse of an assignment being an expression in and itself.

line 259 echo "{$error_message}"; let's admit, this is not pretty. so many escapes and {}. I recommend printf('%s', $error_type, $error_code, $error_message'); same perhaps for line 256

line 267 although this code does use the id_comanda variable correctly in the following SQL query (great!), is there a way to validate this and die immediately if it is invalid?

Great, since chx confirmed no blocking issues ...

Thanks for your contribution, bogdanru!

I updated your account to let you 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 get 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.