[D7] Novalnet Payment Module-Invoice

Multiple Applications

It appears that there have been multiple project applications opened under your username:

Project 1: http://drupal.org/node/1443928
Project 2: https://drupal.org/node/1468662
Project 3: https://drupal.org/node/1483532

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)'.

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

Hi,
as there are currently hundreds applications in queue it'll take some time until we can review your application,
so think about getting a review bonus and we will come back to your application sooner.

The problem is that you have to make a new line after beginning a {

eg

foreach($data as $value) {\\Check VALUE;  <-- this comment is not very helpful anyway
  if ($value == '') {drupal_set_message('Input Data Missing', 'error');

to

foreach($data as $value) {
  if ($value == '') {
    drupal_set_message('Input Data Missing', 'error');

Sorry for the delay, as there are currently many applications in queue this will take some time.
Note that getting a review bonus does not depend on how many bugs remain in your module. (more info)

Line 165: >= You got 2 spaces instead of one there

Also, you got to remove your Freeware-License Agreement as your software is licensed under GNU/GPL when hosting it on drupal.org repository

Please take a moment to make your project page follow tips for a great project page.

Oh no! Novalnet, I'm so sorry, actually we go through tickets that have been updated the longest ago. Updating your ticket is sort of like constantly going to the back of the line :( Think of it like calling a call center and you're on hold... if you hang up and call again, you'll have to wait a long time again because you end up at the back of the line.

I understand this is quite frustrating. We talked about this yesterday at Drupalcon, and we are trying to figure out ways to make this easier.

Sorry I have to remove the bonus tag:

You can use automated tools like pareview.sh for review but you must always do an additional manual review.

(From #1410826: [META] Review bonus)

Please do 3 manual reviews

Included Review of other projects

manual review:

1. project page is too short, see http://drupal.org/node/997024
2. commerce_novalnet_invoice_commerce_payment_method_info(): do not create link markup and image markup yourself, use l() and theme('image', ...) instead.
3. "'#description'=> t('Wählen Sie das Call Back-Methode.'),": all strings in your module should be in English. Also elsewhere.
4. commerce_novalnet_invoice_settings_form(): indentation error for the return statement at the end.
5. "'#description'=>t('Enter Your Novalnet Merchant ID'),": there should be a space before and after the "=>". Also elsewhere.
6. commerce_novalnet_invoice_submit_form(): why do you need the "dummy" element? Please add a comment.
7. commerce_novalnet_invoice_submit_form(): indentation errors in some if clauses, make sure to always use 2 spaces per level, not more.
8. "$order->commerce_order_total['und']": do not use "und", use the constant LANGUAGE_NONE instead.
9. "$query = http_build_query($data, '', '&');": indentation error, this line should not be indented so far. Also elsewhere in this function, make sure that lines that are in the same scope are on the same indentation level.
10. "drupal_set_message('Input Data Missing', 'error');": all user facing text must run through t() for translation, please check all your strings.
11. commerce_novalnet_invoice_default_param_list(): indentation errors, please check all your code.
12. "$novalnet_error = t('There was an error and your payment could not be completed') . '(' . $parsed['status'] . ')';": do not concatenate translatable strings, use placeholders instead. Also, the returned status is third party provided input and therefore untrusted. You need to sanitize it in order to avoid XSS exploits. See http://drupal.org/node/28984

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

Fixed sandbox link

Please don't remove the security tag, we keep that for statistical purposes and to show examples of security issues. Removing review bonus tag, you did not do any additional reviews of other projects.

code Updated and review of other projects

Small note: Ubantu => Ubuntu

Review of other projects

Don't forget to set the status to "needs review" if you want to get a review.

Line 484 is missing a closing parenthesis

$nn_status_error = t("There Was An Error In Your Payment. Payment Could Not Be Completed @status", array('@status' => $_POST['status']);

should be

$nn_status_error = t("There Was An Error In Your Payment. Payment Could Not Be Completed @status", array('@status' => $_POST['status']));

Also, you're setting the varaible $nn_status_error but returning $novalnet_error which makes line 484 usless.

Removing review bonus tag, you have not done any manual review, you just posted the output of an automated review tool. You must always do a manual review and take an actual look at the source code. When finishing your review comment also set the issue status either to "needs work" (you found some problems with the project) or "reviewed & tested by the community" (you found no flaws).

Project page changes

1. Please remove all license information out of your readme, also the copyright stuff. this should be fully handled by the license.txt which will be added later automatically.
2. Your readme is pretty detailed, but usually nobody cares about readmes but about the project page.
3. As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page.
4. Your readme looks really unstructured, please have a look at usual readme-styles (guidelines for in-project documentation) and remove this "#"-border (it's not correctly indented anyway and will always make you more work when you change a line of it)
5. Take care of your code documentation:
   

   * @parameter1
    *   The name-value pair array for the API request.
    * @parameter2
    *   If available, the full order object the payment request is being submitted
    *   for; otherwise NULL.
    */
   function hook_commerce_novalnet_invoice_request_alter(&$nvp, $order) {
     // No example.
   }

   should be

   * @param array $nvp
    *   The name-value pair array for the API request.
    * @param object $order
    *   If available, the full order object the payment request is being submitted
    *   for; otherwise NULL.
   

   Why is there no example ? I don't have to make any sense, just provide a trivial example.

6. There is no drupal_alter() call that actually invokes this hook_commerce_novalnet_invoice_request_alter() function.
7. line 538: your setting $headers, but your not using it

8. }
   /**

   please put a new line between functions

9. commerce_novalnet_invoice_commerce_payment_method_info(): Create the image url with file_create_url() and why are you putting so many spaces between alt="" and title="" ?
10. I don't know how this is handled in commerce but you may have to change the nn_ in your form items to the full module name to avoid name collisions. Please inform yourself about this.
11. Pin By SMS(nur Handy) make sure all translatable strings are english
12. t('Call Back Method: ') why the single space in the end? ; t('Payment period in days::') why the double :: ?

13. '1'=>'true',
    '0'=>'false'

    ),
    'true' and 'false' should be translatable here and 0 and 1 may better be TRUE and FALSE ? why your not using booleans for the testmode variables?

14. Generally, always use TRUE and FALSE instead of 1 / 0
15. If possible, don't put any HTML into t() functions, this will always make the live easier for translators
16. pareview finds some minor issues
17. if (isset($data['mobile'])||isset($data['email'])) {please always put spaces between operators ->if (isset($data['mobile']) || isset($data['email'])) {

18. /**
     * Function for Getting the Ip Address.
     **/

    should be

    /**
     * Function for Getting the Ip Address.
     */

19. function commerce_novalnet_invoice_getRealIpAddr() {
          $_check = array(
            'HTTP_X_FORWARDED_FOR',

    check your indentation, always use 2 spaces for indenting. on many places there are lines indented with 4 spaces, etc.

20. function commerce_novalnet_invoice_check_status(&$parsed) {there are 2 spaces between function and the function name

I know this is all very picky, but the whole code looks to me kind of "quick written".
Please take more care of this, try to get used to these standards and your code will look much cleaner.

Review of other projects

Generally I find your reviews not very elaborated, I recommand having a look at the following post:

Posted by patrickd on April 24, 2012 at 10:05pm

Thanks for your participation, but I'm afraid the comments you did are not very detailed and IMHO not really "reviews".
A good deep review can take up to an hour, this is not about just pointing out 2 things and set the issues to needs work.
Sorry, removing tag.
http://drupal.org/node/1540804#comment-5912808

my first point from #19 is still not fixed.

README.txt
1. The indentation is inconsistent: sometimes the block starts 2 spaces after the number and sometimes 3.

2. You should also use this rule for line length: Comments should be word-wrapped if the line length would exceed 80 characters (i.e., go past the 80th column, including any leading spaces and comment characters in the 80 character count). They should be as long as possible within the 80-character limit. This comes from Doxygen and comment formatting conventions but also applies to format the README.

3. The item from Comment #23 hasn't been fixed (Ubantu => Ubuntu)

4. Each sentence should start with a capital letter.

commerce_novalnet_invoice.api.php

There is still no example for hook_commerce_novalnet_invoice_request_alter().

commerce_novalnet_invoice.module

1. Drupal uses American spelings: e.g., Authorisation should be Authorization.

2. You should use "PIN" instead of "Pin" when the word is displayed, since PIN is an abbreviation for personal identification number and pin is a sharp piece of metal.

3. Comments should be word-wrapped if the line length would exceed 80 characters (i.e., go past the 80th column, including any leading spaces and comment characters in the 80 character count). They should be as long as possible within the 80-character limit.

4. A number of items from comment #19 haven't been fixed: items 2, 8, and 9.

5. Sections starting with
$parsed = $_SESSION['invoice_parsed'];
and
commerce_novalnet_invoice_transaction($payment_method, $parsed, $order, $charge, $pane_values['dummy'], $data);
are also indented too much.

6. Comment #31 item 6 still seems to be not fixed.

Review of other projects

You need to fix your project page which is not this issue but the page of your sandbox: http://drupal.org/sandbox/Novalnet/1481620 . This will also be the public face of your full project once it is promoted.

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:

1. commerce_novalnet_invoice_commerce_payment_method_info(): do not create image markup yourself, use theme('image', ...) instead. I just saw that I pointed this out already in my comment #19. And you don't have to call file_create_url() yourself then.
2. commerce_novalnet_invoice_getRealIpAddr(): $this does not make sense here, you are not in a class?
3. commerce_novalnet_invoice_getRealIpAddr(): please add a comment why ip_address() does not work here for you.
4. commerce_novalnet_invoice_transaction(): commerce_novalnet_invoice_check_status() returns a string that is already translated, right? Why do you translate it again in "drupal_set_message(t($success), 'error');"?
5. "drupal_set_message(t('Please note down the below Transaction details provided by Novalnet (TID)') . '' . utf8_encode($transaction->message), 'status');": You need to sanitize all components of the message before printing them to avoid XSS exploits. The components of the message stem from an untrusted third party and must be treated like user provided input. I think $transaction->message_variables should hold all the message variables, no?
6. "t('Please transfer the amount at the latest, untill') . '  ' . isset($data['due_date'])": don't concatenate varibales into translatable strings, use placeholders instead.
7. commerce_novalnet_invoice_isPublicIP(): all global functions should use only lower case letters and underscores. Camel case is reserved for method names.
8. I still don't understand the hidden "dummy" form value. It gets always set in commerce_novalnet_invoice_submit_form(), but you set it again conditionally at the end of the function, why? Then you pass the dummy to commerce_novalnet_invoice_transaction() as $name, but you only use it in the message variables? Maybe I miss something here.

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

removed link to this application issue

I've had a look through the code. On line 14 - 17 of the .module file you're using double quotes instead of single quotes. Was there a reason for this?

$image = array(
'path' => "$path/modules/payment/novalnet/novalnet.png",
'alt' => "Novalnet AG",
);

Double quotes force PHP to parse the string.

On line 53 of the .module file there's more than one space between words in the description string:

'#description'=> t('Enter Your Novalnet Authorization Code'),

On line 259 of the .module file you're using capitalisation for the warning error:

drupal_set_message(t('Please Enter the PIN Number You Have received.'), 'warning');

Seems to be the only time you're using this. Also the word "new" is capitalised on line 291:

drupal_set_message(t('Enter the New PIN You have received !'), 'warning');

Also on this line, there is a space between received and the exclamation mark. This error also occurs on line 296 and 301:

drupal_set_message(t('Please answer the email - reply email check !'), 'warning');

drupal_set_message(t('Enter the New PIN You have received !'), 'error');

Capitalisation for all words in string on line 305:

drupal_set_message(t('Oops! Maximum Time Exceeded!'), 'error');

Capitalisation of the word "correct" on line 320:

drupal_set_message(t('Oops! Enter the Correct PIN number'), 'error');

In the function function commerce_novalnet_invoice_default_param_list(&$data, $order, $billing_address) { on line 349 in the .module file, is all the data being sanitised correctly? Also, I was wondering with the XML response that you're getting and storing in your data variable, is all the response data being sanitised correctly before it's used?

Also, from line 547 to line 562 in the .module file there's some lines of code that have been commented out - maybe use a multi-line comment e.g. /* */ with a description of what these lines of code are there for. Are they part of a TO-DO list item? Or perhaps remove them all together if they're no longer needed?

I installed the module and it doesn't seem to throw any errors.

I ran the module through Ventral: http://ventral.org/pareview/httpgitdrupalorgsandboxnovalnet1481620git-7x-1x and no errors either.

Hope this helps.

manual review:

1. "t(l($image, 'http://novalnet.de', array(": do not send the link/image markup through t(), only plaintext strings should be passed to t().
2. "module file changes" is a bad commit message, please describe what you did. See http://drupal.org//node/52287
3. "t('@due_date', array(..." It does not make sense to translate a dynamic varibale only. You should not concatenate translatable strings that are tied to each other, as the dynamic placeholder value might not be at the end of the sentence in other languages. I.e. you should do something like t('Novalnet Transaction Id  @tid', array("@tid" => $parsed['tid'])). That might mean that you will include the label tags into the string passed to t() in some cases, but that's better than having t() calls with just a placeholder in them.
4. commerce_novalnet_invoice_getRealIpAddr(): that function is obsolete now?
5. commerce_novalnet_invoice_submit_form_submit(): Shouldn't you use form_set_error() instead of drupal_set_message() to indicate errors with the submission? Or does that not work with the commerce workflow? Also, any validation should be handled in a validation form callback, not in the submit function.

Although you should definitely fix those issues they are no application blockers, otherwise I think this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

No need to set this back to "needs review" at this point.

Thanks for your contribution and 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 and also for your help in the application queue! 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.

The full module name can be changed but the module shortname can't

You got full git access, surely you can change and add files

Once your first application has been successfully approved, you can promote other projects without review.

As no further reviews are required we please you not to create further project applications so the workload of reviewers won't be doubled unnecessary

Yes, this application has been approved.

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review.