RBK Money

Incorrect usage of the t() function. The t() function expects English to translate into the target language.

Your project page is in Russian. You can google translate that in English

Please fix the reported issues. Don't forget to set status to needs review after committed to your sandbox project.

Powered by Dreditor (triage sandbox) and Triage transitions

Tagging for easier finding.

Your repository is structured wrong. You have branches named "6", "7", and "master". In Drupal, the branches must be named like 6.x-1.x, 7.x-1.x, not "6" and "7". See http://drupal.org/node/1015226

It's also recommended that your "master" branch be empty except for a README.txt explaining that the actual code is in 6.x-1.x or 7.x-1.x.

In addition to the branches having non-standard names, it looks like you have a non-standard file structure. Your .module file should be directly within the repository, not in a folder within the repository.

there is still a 7.x-1.x folder in the 7.x-1.x branch. This is not needed as said in #6. Same for 6.x.

1. I'm assuming the RBK Money logo is a copyrighted image / registered trademark. Do you have a release allowing for the inclusion of this logo in a GPLv2 project?
2. Not a critical issue, but the Drupal coding standard is to use two spaces for indentation, as opposed to tabs or four spaces.
3. To make life easier for translators, I'd recommend restructuring your t() calls to use a !link placeholder, rather than including the full html link inside the text to be translated. Drupal 7 Example:
   t("Log responses in the !dblog", array('!dblog' => l(t('system log'), '/admin/reports/dblog')))
4. You should include a uc_rbkmoney.install file, containing a function which implements hook_uninstall() and removes any variables set by the module.
5. No need to include "$type =" in drupal_set_message(t('Settings have been saved'), $type = 'status'); on line 143
6. I'd suggest spelling out 'Number' or 'No' instead of using the symbol in $desc = t('Order №') . $order->order_id; on line 205.

Also appear to have some non-printable characters on line 205: $desc = t('Order №') . $order->order_id;

7. I believe you could use '#type' => 'value' instead of 'hidden' for the hidden uc_rbkmoney_submit_form elements, which would prevent them from being passed to the user's browser.
8. I suspect the $value->title value needs to be passed through a sanitization function such as check_plain before being output to the screen or included in the hidden form element: $desc .= $value->qty . '× ' . $value->title; on line 201

Bulinat,

Thanks for the quick turnaround!

It appears that including the payment gateway logo is a common practice for commerce bridge modules ... I'll leave the decision up to you, as long as you're aware that we generally can not allow other copyrighted/trademarked material in contrib modules - anything in a contrib module is assumed to be licensed GPLv2, and I'm sure that companies generally don't consider their logos as 'free to use or modify'. ;)

My understanding was that '#type' => 'value' items would still be POSTed with the form, but would not be displayed on the screen. If you're posting the form directly, then maybe this isn't the case. Either way, it was only a suggestion, not a requirement for approval.

Marking RTBC, as the rest of the issues appear to have been addressed.

Reviewing the 7.x-1.x branch, there are chunks of code that are definitely written for D6.

hook_perm => hook_permission

Several doc blocks have "Implementation of hook_blah" where others have the correct "Implements hook_blah".

Comments should end with a full stop.

Remove these lines from your .info file:

files[] = uc_rbkmoney.module
files[] = uc_rbkmoney.install
files[] = images/logo.gif

files[] is only for files that contain PHP classes.

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/uc_rbkmoney.module:
   +55: [normal] The $string argument to t() should not begin or end with a space.
   +219: [minor] in most cases, replace the string function with the drupal_ equivalent string functions
   +318: [normal] else statements should begin on a new line
   +321: [normal] The $message argument to watchdog() should NOT be enclosed within t(), so that it can be properly translated at display time.
  
  sites/all/modules/pareview_temp/test_candidate/uc_rbkmoney.install:
   +-1: [normal] @file block missing
   +4: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 2 files, 4 normal warnings, 2 minor warnings, 0 warnings were flagged to be ignored
  

• Lines in README.txt should not exceed 80 characters, see the guidelines for in-project documentation.
• Remove "project" from the info file, it will be added by drupal.org packaging automatically.
• @file doc block is missing in the module file, see http://drupal.org/node/1354#files .
• @file doc block is missing in the install file, see http://drupal.org/node/1354#files .

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

Review of the 7.x-1.x branch:

• Remove all old CVS $Id tags, they are not needed anymore.
  

  README.txt:1:$Id:$
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• "// Валидные IP адреса системы": please use English for comments
• "'#suffix' => '<div class="description">' . t('For insertion into "Payment notification URL" field on <a target="_blank" href="!site">RBK Money</a> settings page', array('!site' => url('http://www.rbkmoney.ru'))) . '</div>',": you should create links with the l() function.
• uc_rbkmoney_setup(): you can pass your $form to system_settings_form() which will automatically save the form elements to variables. Then you can remove uc_rbkmoney_setup_submit(). You need to rename the form keys to fit the variable names.
• "$title = t('RBK Money') . '<br /><img src="' . $path . '/images/logo.gif" style="position: relative; left: 2.5em;">';": use theme('image', ...) to generate image markup.
• uc_payment_method_rbkmoney(): that function does not do anything, remove it.
• "$desc = t('Order number') . ' ' . $order->order_id;": you should use place holders for t(), because the order id might not be at end in all languages.

Review of the 7.x-1.x branch:

• Run coder to check your style, some issues were found (please check the Drupal coding standards):
  

  Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards
  
  sites/all/modules/pareview_temp/test_candidate/uc_rbkmoney.module:
   +61: [normal] Arrays should be formatted with a space separating each element and assignment operator
   +89: [normal] Arrays should be formatted with a space separating each element and assignment operator
   +137: [normal] missing space after comma
   +164: [minor] indent secondary line of comment one space 
  
  Status Messages:
   Coder found 1 projects, 2 files, 3 normal warnings, 1 minor warnings, 0 warnings were flagged to be ignored
  

• Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
  

  ./uc_rbkmoney.module:202:  if (drupal_strlen($desc) > 255) { // Prevent more than 255 symbols of order description.
  ./uc_rbkmoney.module:260:    $allowed_ip = array('89.111.188.128', '46.38.182.208', '46.38.182.209', '46.38.182.210'); // List of allowed IP
  

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

• "$desc = t('Order number !num', array('!num' => $order->order_id));": order id looks like a number or plain string, so use the "@" placeholder in t().
• uc_rbkmoney_submit_form(): you should use the same default variables in variable_get() as on the settings page.
• same in uc_rbkmoney_done_payment()

Looks good to me.

On line 255 of the 6.x version I see that you are taking $_POST variables and logging them to watchdog. These variables may include malicious code including javascript.

You do an IP address check but it is possible to forge the server remote_addr variable and bypass that check.

I suggest two things:

1) Before doing a watchdog be sure to filter the variables.
2) You have a check for the hash later in the function - move that up to the top of the function and then watchdog and exit if the hash doesn't validate.

I didn't install the module and test this out, but can't someone just visit uc_rbkmoney/success and magically move their order into a complete state without actually paying any money?

I'm not sure that the uc_order_comment_save() function calls are correct at 6.x.

I haven't tried but as far as I remember after just visiting uc_rbkmoney/success the order will become 'pending', because there was no uc_payment_enter() call and balance isn't zero. But that still has to be reworked I think.

Some problems in 6.x version (may be 7.x has same problems):

You define permission
array('administer uc_rbkmoney');
but it's never used.

Callbacks for uc_rbkmoney/success and uc_rbkmoney/fail use default 'access content' permission, but it's not flexible, better to use own permissions for that.

In uc_rbkmoney_setup() you wrote
t('Response URL: ') . $base_url . '/uc_rbkmoney/response'
better is (and no need to define global $base_url...)
t('Response URL: !link', array('!link' => 'uc_rbkmoney/response'))

In several lines you get key of $_SESSION array without isset() check, it may cause notices.

In uc_rbkmoney_done_payment() present hardcoded IP addresses, better make them configurable via variables_get(...)

Also, why you use $GLOBALS['_POST'] construction instead simple $_POST?

Available statuses from $response['paymentStatus'] should be defined via constants, it will improve readabilty and agilty of code.

Errors through Ventral.

Lines too long in README.txt file. Split onto multiple lines (80 character limit).
uc.rbkmoney.info - Missing blank line at end of file.

uc_rbkmoney.install - Errors in function doc comment, missing punctuation makes for comment, whitespace at end of line, missing black line at end of file.

uc_rbkmoney.module - Blank line needed after file doc comment, lines exceeding 80 characters, translation problems (use t() for text arguments), comma missing for multiline array, spacing issues, inline comment capitalisation, use function ip_address() not $_SERVER, blank lines after break statements, missing blank line at end of file.

I find it helpful to use a text editor that can show you invisible characters (e.g. spaces).

Please find attached Ventral output.

Review bonus.

// edit
unrelated

Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure. Same with your readme, you may have a look at the common styles for readme's (see guidelines for in-project documentation.)

7.x-1.x

1. .module, 66: t('Response URL:') . ' ' . $base_url . '/uc_rbkmoney/response',why not using t('Response URL: %url', array('%url' => $base_url . '/uc_rbkmoney/response')),
2. 100: t('Save RBK Money\'s server responses in !dblog') rather use "money's" instead of 'money\'s'
3. 173: $form['#prefix'] = '<table><tr><td>'; rather use a proper <div> with some css here instead of using table's (using tables for formatting is not very professional these days)
4. different places: $base_url . '/uc_rbkmoney/success' please use url() here instead if possible.
5. 265: watchdog("uc_rbkmoney", "Possible fraud. Error with REMOTE IP ADDRESS = " . ip_address() . ". The remote address of the script posting to this notify script does not match a valid RBK Money ip address"); Please use the variables parameter of watchdog() here for the ip_address to keep the message translatable

- also, use the proper severity - if this is an error - mark it like an error

6.x-1.x

1. Here are some issues same as in 7.x-1.x
2. .info: Remove ; $Id$ this is deprecated since we are using GIT on drupal.org
3. uc_payment_method_rbkmoney(): why your implementing this callback if it does not do anything?

If possible, break lines after 80 characters because eg. line 287 is very hard to read, and about 500 characters long.

Beside this I can't find a critical issue, therefore RTBC for me

Review of the 7.x-1.x branch:

• Drupal Code Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

manual review:

• "define('STATUS_PROCESS', 3);": all constants defined by your module need to prefixed with your module's name to avoid name collisions.

Although you should definitively fix those issues they are not application blockers, so ...

Thanks for your contribution! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.