[D7] MercadoPago Payment

Change the git clone repository to the anonymus url. From vmunozp@git.drupal.org:sandbox/vmunozp/1643648.git to http://git.drupal.org/sandbox/vmunozp/1643648.git

Thank you rlmumford,

I decided to apply the version 7.x-1.x for a full project because it is the stable one. I would love both version to be reviewed. But version 7.x-2.x needs a lot much more work.

About your review, i did every thing that you said.

• Remove ", development" from tha .info file
• Change docblock form mercado_pago_payment_menu
• I translate in to English all strings wrapped in t() and also the ones in $transaction->message. And move them to a es.po file
• I passed the link to l() function, just like it is suggested in http://drupal.org/node/322774

And finally, I hope i can review your module soon in return.

I forgot to change the status to needs review

Changing priority. I have been awaiting for reviewers for more then 2 weeks.

Sorry for not attending your review, a have been busy this past weeks.

Well, despite that your review is short, it is making me work a lot.

1. About your comment on translation files. I don't know how to add my translation file to translate.drupal.org. I have been reading the documentation and I didn't find out. I suppose, I can simply remove the translation file from the module, but i don't know where to put it after that.
2. About the possibility of fake IPN's, your are right. It is possible, in fact, I did my own IPN's for testing. This is because at the time of the developing, MercadoPago did not have testing options. Now a days, it is possible to make some testing.
   In order to verify the authenticity of the IPN, I should compare the information sent by the IPN with the one on the servers of MercadoPago; meaning, made a POST with the account information to their servers asking for the actual information. But in terms of Drupal Commerce, there are some complications to do that. I should extract from the IPN the payment method instance id used to create the payment. I checked PayPal module to understand how they did it and it is a nice solution.

So, it will take me a couple of weeks to do the work needed. I just want to keep you posted.

And, thanks for the review.

Finally it is done.

I will answer one by one your suggestions.

Suggestion 1

Line 91: I think it would be better to use l() here.

rlmumford at comment-7272690 also suggested the same. But I can't find that suggestion on any part of the Drupal documentation. In fact, in Dynamic or static links and HTML in translatable strings you can find an opposite suggestion, and also on the core modules you can found that link's are been handled just like I did. For example, on search.module line 91:

<?php
 $output .= '<p>' . t('The Search module provides the ability to index and search for content by exact keywords, and for users by username or e-mail. For more information, see the online handbook entry for <a href="@search-module">Search module</a>.', array('@search-module' => 'http://drupal.org/documentation/modules/search/', '@search' => url('search'))) . '</p>';
?>

On the other hand, I notice some complains about HTML on translation strings. But I don't remember where.

Suggestion 2

Lines 189-192: The last closing paren should be on a new line, and the rest of this section should be indented by two more spaces.

Done, just like you suggested.

Suggestion 3

I think it would be possible to fake a payment by sending a POST request to the IPN URL.

This gave me a lot of trouble but I finally nailed it. It wasn't so simple as just make a POST to the MercadoPago Server to verify the information, because it was needed to recall the Payment Method Instance Id. I solve this by using the extra parameter accepted by Mercado Pago Server and in case of missing that parameter, I use the default Instance Id.

Suggestion 4

Translation files are no longer included with the module; they are handled on translate.drupal.org.

Ok, I get it. But I don't like too much the idea because, in my understanding, translate.drupal.org does not handle translations for sand projects. So, while my project still on review for promotion, there will be no translation file easily available. And my problem with that, is that all the user of this module are from Latin America, and translation is absolutely required. But, I did remove it as you suggested, in the hope that it will be promoted.

Thank you for taking your time to review my module and to read this long comment.

Hi kscheirer,

I corrected the typos that you mention. I used your comment as an excuse to correct the big list of ventral issues for the 7.x-2.x branch.

For the security question, let me explain you first the difference between 7.x-1.x and 7.x-2.x. There are two version of MercadoPago API, lets call them 1.0 and 2.0. Version 7.x-1.x of my module use 1.0 version, and version 7.x-2.x can use both versions. So, after reading your comment, I realized that it got no sense to keep both versions on 7.x-2.x of the module, it is more natural and simple to work with only one version per module branch.

So, i remove the compatibility for Mercado API 1.0 on the module branch 7.x-2.x; after this, the security implementation on #9 no longer applies for 7.x-2.x branch. This is because 7.x-2.x works in a different way; Mercado Pago IPN 2.0 notifications does not post the payment status, and a post back to Mercado Server's is always required.

Thank you for reviewing the module.