[D7] Commerce Cielo

You haven´t any README file in repository

@FraciscoLuz, your code looks great, there are some code styles changes that needs to be made to make it be more inline with Drupal coding standards.

readme.txt should be renamed to README.txt

  //add a page for concluding the payment process after returning from cielo.

Code comments should have a space between the // and the start of the comment, and should start with a capital letter. Also, I think Cielo should always start with a capital letter in code comments since this is a name.

I personally don't indent array properties like you did, since it leads to longer, unreadable patches, and it's difficult to keep all code indented this way. E.g. we use:

$x = array(
  'property' => 'value',
  'longer_property_name' => 'value',
);

instead of:

$x = array(
  'property'            => 'value',
  'longer_property_name' => 'value',
);

Your functions names should keep a consistent namespace, e.g. they should always start with your module name and be named in lowercase, and not glue words, e.g.: generateInstallmentOptions() and credit_card_type_ajax_callback() should be named commerce_cielo_installment_options() and commerce_cielo_credit_card_type_ajax_callback() respectively, or whatever you want, but keep them lowercase, starting with module name and use underscore to separate words, not camelCase.

Another note on array indentation:

  $form['language'] = array(
    '#type'        => 'select',
    '#title'       => t('Language Interface'),
    '#description' => t('Set the language in which Cielo will provide its webservice responses as well as its user interface for when a client redirection to Cielo is required.'),
    '#options'     => array('PT' => t('Portuguese'),
                            'EN' => t('English'),
                            'ES' => t('Spanish'),
                           ),
    '#default_value' => $settings['language'],
  );

We would write this as:

  $form['language'] = array(
    '#type' => 'select',
    '#title' => t('Language Interface'),
    '#description' => t('Set the language in which Cielo will provide its webservice responses as well as its user interface for when a client redirection to Cielo is required.'),
    '#options' => array(
      'PT' => t('Portuguese'),
      'EN' => t('English'),
      'ES' => t('Spanish'),
    ),
    '#default_value' => $settings['language'],
  );

Also, it's aways good having constants for keys, and values, that aren't user translated strings, are used in more than one place and is used in comparisons, e.g. 'PT' and 'EN', in the code above, I would add constants for them like COMMERCE_CIELO_LANGUAGE_EN and COMMERCE_CIELO_LANGUAGE_PT, all constants should be added to the beginning of the file and should be commented.

See the variable bellow:

    $form['authenticate']['#options']= array(0 => t('Nope.'),
                                                  1 => t('Yes.'),
                                                 );

It would be far more readable written as:

    $form['authenticate']['#options'] = array(
      COMMERCE_CIELO_NO => t('No'),
      COMMERCE_CIELO_YES => t('Yes'),
    );

Also, your external library is released in a non commercial license, which may have an impact your module usage. Why would some use your payment library other than for a commercial purpose? I think you should consider either a. releasing your library as GPL or b. including it in your module code (also as GPL since drupal.org only accepts code licensed as GPL), or maybe just port you library and incorporate it to your module as other payment modules do.

Anyway this was what I come up with by doing a quick code review, I may test it later. Awesome work!

Please let me know if you have any questions.

@FranciscoLuz, I'm trying to get access to Cielo sandbox for testing your module. Can you tell me how I can get this?

Thanks for the instructions on how to setup a sandbox. I guess I need the credentials first? Will try to get them. I've got some integration manuals from a friend, and will figure out how to get the credentials to access the sandbox.

Here are some other issues by having a second visual inspection at your module code:

'description' => t('Integration with Cielo\'s webservice.'),

We don't escape quotes like this on translated strings as this is not allowed by gettext standard, instead, you should use double quotes, when your string contains single quotes, e.g.:

'description' => t("Integration with Cielo's webservice."),

I'd add constants for all numeric options, e.g. in the items bellow:

    'authorization_type' => 2,
    'collect_card_details' => 1,
    'authenticate' => 1,

and

  $form['authorization_type'] = array(
    '#type' => 'radios',
    '#title' => t('Authorization Mode'),
    '#description' => t('Define how the card holder authentication will be handled.'),
    '#options' => array(
      0 => t('Authentication only. (use this only if you know what you are doing.)'),
      1 => t('Authorize only if authenticaded'),
      2 => t('Authorize either authenticated or not'),
      3 => t('Skip authentication and go straight to authorization'),
    ),
    '#default_value' => $settings['authorization_type'],
  );

Indentation is off here:

  return array('#type' => 'ajax',
               '#commands' => array(
                  ajax_command_replace("#credit-card_code_wrapper", render($form['credit_card']['code'])),
                  ajax_command_replace("#credit-card_installments_wrapper", render($form['credit_card']['installments'])),
                ),
              );

It should be:

  return array(
    '#type' => 'ajax',
    '#commands' => array(
      ajax_command_replace("#credit-card_code_wrapper", render($form['credit_card']['code'])),
      ajax_command_replace("#credit-card_installments_wrapper", render($form['credit_card']['installments'])),
    ),
  );

I see you use cool works like Nope and Yeap, while this is fun. I think it's better keeping this consistent with other Drupal modules.

There are some functions still using camelCase e.g. commerce_cielo_getStatusState(), and others not using the module namespace e.g. cielo_redirect_back(), please fix all so they are all lowercase, starting with module name and use underscore to separate words, not camelCase.

for the admin inc files, we usually use periods to separate words, e.g. commerce_cielo.admin.inc, your router function file (cielo_redirect_back_router.inc) could be named just commerce_cielo.inc, and you can maybe move more functions there, or if it's just one function file, you can move this function to the .module file as this will not have performance impact.

I see your return callback require the user to log in, is this really necessary? There are other ways we can use to certify the authenticity of the return. But we usually, don't do that and wait for the first payment notification for creating a payment. I think this need some work so this doesn't require the user to log in as this may be bad user experience. Also I see it uses cookies, may be better to move this to session vars or avoid this when possible.

I will add Cielo integration for a project I'm starting in the next few weeks, and this will give me the opportunity to test your module.

In the long run, you may see others contributing code so your module doesn't depend on your external library, I see this as a normal evolution of the module.

Your code is looking great, keep up the good work!

I'll review your other modules, please tell me when to check them. Please apply all suggestions here to your other modules too. Once we get your modules in a good shape, I can submit patches to help with code style and bug fixes.

Thanks!

I don't agree that this will cause a bad user experience because the user is meant to be already logged in before even getting on the payment pane, so if this piece of code get executed is because something went terribly wrong and therefore, as a precaution measure, the user must login.

Are you taking into account "anonymous checkout"? It is, the checkout mode that doesn't require the user to log in?

@FranciscoLuz: please let me know if you want to discuss this further and/or need help with anything else.

@FranciscoLuz: you code is looking great!

In regards to anonymous checkout. I think it's a question for commerce core. We need to follow this pattern to met user expectations. So whenever we can, we should not require the user to have an account. Although, in standard install, an user account is created for the user when he/she completes the checkout process. So we just can't rely on users being logged in. A proper payment transaction should be created just like we do in other payment modules, like when commerce_paypal module receives an IPN. It's sure better experience, than the user getting a 403 page when returning from payment processor after successful completing a payment.

I think you module is ready to be published. I still haven't tested it. I may be interested in being the co-maintainer, lets talk more about that.

Another tip is getting more meaningful commit messages.

We may also need to set up a 7.x-1.x branch for your module (I'm not used to new git sandboxes yet).

The 'admin/commerce/orders/%commerce_order/payment/%commerce_payment_transaction/cielo-void' menu item is pointing to the old include file.

Just tagging.

Hmm. FranciscoLuz appears to have two applications going at once: this one and http://drupal.org/node/1259898

FranciscoLuz, we only need to review one of your projects. When you get full project permissions after one review, you can promote any project you choose to a full project. I'm going to mark both projects as "postponed" for now. Please decide which application you want to complete, change the status to "needs review" and change the other to "closed (duplicate)." If you'd still like to have someone review the other project, you can do that in the Peer Review group. But since we have such a large backlog here, we can't afford to review more than one project for each contributor.

@FranciscoLuz has been contacted to ask if the application is abandoned.

After ten weeks with a status of needs work: the applicant may be contacted by a reviewer to determine whether the application was indeed abandoned. The action taken by the reviewer should be documented in the project application issue.

http://drupal.org/node/894256

@MiSc, it's not abandoned. Actually we are just getting back to it again, doing some polishing to get the module done.

@MiSc
It really is not abandoned, I'm even applying and testing the module on a website (amxaluminio.com.br). And all I'm reporting the Francisco and Recidive, both are very attentive.

If you want review of the module you need to set it to 'Need review'.

Hi,
any update?

Let me preface this review with my opinion...

I can agree with your reason to keep it as a library so that it could be uitilzed by other platforms, however the mission of Commerce Guys and Drupal Commerce seems to be quite the opposite of what the goal of your module is. Everyone else that is contribuiting to Drupal Commerce is doing so for the "greater good" (not to be cliche), but I've yet to see anyone trying to make money of a contributed piece of code.

commerce_cielo.module:

After installing the module I get the message:

Cielo library is not properly installed. The file @file does not exist.

This is to be expected since the library is missing, but the t() function should be:

drupal_set_message(t('Cielo library is not properly installed. The file @file does not exist.', array('@file' => $cielo_class_file)), 'error');

instead of:

drupal_set_message(t('Cielo library is not properly installed. The file @file does not exist.', array('@file', $cielo_class_file)), 'error');

Also, I wouldn't reccomend doing propritary dependancy checks during hook_init() since these messages will be displayed to all visitors to the website even on pages that aren't specific to commerce. This message should only be displayed to the admin when configuring commerce cielo and in watchdog.

commerce_cielo.admin.inc:

I would reccomend following a more professional format for settings form. For example:

Default credit card transaction type
Authorization and capture
Authorization only (requires manual or automated capture after checkout)

Instead of:

Auto Capture
No, just request authorization. (requires manual or automated capture after checkout).
Yes, take every penny.

In your setting for collect_card_details you should use "Yes/No" instead of "Yes./Nope." for options. So that all instances of Yes and No are translated without having to add another translation.

Authenticated checkout should be determined by the store owner and permissions, not by a payment module, regardless of your opinion.

Required Library:

Several functions of your library do not properly check for dependanciecs. For example curl_init caused WSOD because I didn't have php-curl installed on my testing server.

README.txt:

Line 21: enviroment should be spelled environment.

Recap:

I am all about extending the functionality of Drupal Commerce and I've spent as much free time as I can working on modules and patches helping to get DC into the mainstream. I feel like your module has great potential and can tell that you've spent a lot of time making it. I will admit that I am ignorant when it comes to international payment gateways and even international commerce. However, I feel that this module is going against the Drupal Commerce grain by trying to make a direct profit off of a contrib module. I've yet to pay a license for a Drupal module much less any contrib add on. Please pardon me if I've misunderstood your intent.

I'm not trying to shoot your module down, just expressing MHO. Thanks!

There are still some issues thrown by automated reviews you should fix:

• Remove the translations folder, translations are done on http://localize.drupal.org
• Remove all old CVS $Id tags, they are not needed anymore since drupal.org is running with GIT.

There are also some coding standard errors left: http://ventral.org/pareview/httpgitdrupalorgsandboxdrupalista-br1240210git
As coding standards make sure projects are coded in a consistent style we please you to have a look at the report and try to fix them. Anyway, note that issues found are possibly false positives and fixing all issues is not a requirement for getting through the application process.

dependencies[] = commerce_ui
Surely your module can't be configured without commerce_ui, but if it is ready configured, it should work without ui - or not?

It's not a recommended way to warn a user about a missing library on hook_init(). Rather implement hook_requirement() in your .install so the requirements are checked properly before your module is enabled. This way such performance heavy (because it's called on any page) implementations are not necessary.

Rather use REQUEST_TIME than time()

Please never use such generic variable names as $i

Always try to avoid HTML within t() strings

$message .= '<br />' . check_plain(t("$process: @message", array('@message' => check_plain($values['mensagem']))));
Here are a lot of issues regarding filtering and right usage of t()

1. Never ever put a variable directly into a t('') string USE placeholders
2. The placeholders with @ and % are already check_plain()ed automatically - don't do it extra there
3. The only placeholder that is not filtered and will be replaced within t() without any filtering is !
4. There is no need to check_plain() the output of t() if you made correct usage of t()'s functionality
5. If you want to use placeholders (because they are so nice for readability) but there are actually no translatable words you can use format_string() instead, it works the same like t() but without translation. In your case: format_string('@process: @message' array(....))

In commerce_cielo_credit_card_type_ajax_callback()
Don't craft your own forms! when you start creating your own inputs, selects ect. you get 1. really ugly code and 2. you waive all the security checks drupal provides you with its forms API. Seriously! use form elements!

Rather use hide() than unset() to remove form elements

watchdog('library_brazilcards', $error . ': ' . $error_delta_message, array(), WATCHDOG_DEBUG);
watchdog is similar to t(). Translatable string is second parameter, replaceable variables is the third one.
also here: don't put variables into the second parameter! use the third one with placeholders!
in this case: watchdog('library_brazilcards', '!error: !error_message', array('!error' => $error, '!error_message' => $error_delta_message), WATCHDOG_DEBUG); Also note that anything put into the second parameter is not filtered! make sure that it's not possible that any unfiltered user input goes into watchdog()

Don't just return FALSE when checking the access in your config form, this will result in giving anyone not having the right permission to get a whitescreen (in such cases use drupal_access_denied())!
BUT whenever possible, use the access callbacks of menu items properly! (in the case of your config this is probably not possible as it is commerce specific)

// TODO: Validate input to make sure a numeric value has been entered.
This can be accomplished easily by adding '#element_validate' => 'element_validate_number' to a form element

Sorry, have to set needs work as here are some major (bold) issues you have to resolve first

Not critical, since the last review has been a view days ago. You have to get a review bonus to get a review from me.

Multiple Applications

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

Project 1: http://drupal.org/node/1678380
Project 2: http://drupal.org/node/1246104

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)', with only one application left open (chosen at random).

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 guys,
FranciscoLuz has some great modules(Commerce Boleto and this one, Commerce Cielo) for the e-commerce ecosystem in Brazil, that would help a lot to Drupal and Drupal Commerce to gain traction in the region. It's true that they need polishing, but me, @recidive and other developers are engaged in a lot of Drupal Commerce projects, so will be a natural step to came and collaborate with FranciscoLuz in his modules.

The module is in a better shape that many modules out there and could be in a -dev release for sure, and is sad that such an useful module isn't available to the brazilian Drupal community for over an year of his first attempt to create a full project. Its like the "chicken or the egg" problem, because the module needs to be polished to be promoted, but need to be promoted to raise awareness and get more collaboration from others and becomes polished.

Promoting his user to be able to create full projects will make the ecosystem do the rest of the job: more users and developers will be aware of the module, will test, help, patch, and he will be polished enough to have an alpha release.

My 2 cents.

#46 doesn't seem like a review, so I'm moving this back to needs review. pedrorocha, the main reason this has been waiting so long is a shortage of volunteer reviewers. Anyone can do reviews, so you can help solve this problem by reviewing the project yourself. Guidelines are here: http://drupal.org/node/894256

@sreynen, this is a review, from my point of view:

"The module is in a better shape that many modules out there and could be in a -dev release for sure, and is sad that such an useful module isn't available to the brazilian Drupal community for over an year of his first attempt to create a full project. Its like the "chicken or the egg" problem, because the module needs to be polished to be promoted, but need to be promoted to raise awareness and get more collaboration from others and becomes polished."

The code is in a quality level that could be promoted to a full project, because the minor tweaks said here could be made after, with patches, like any other modules. The time spent here going on and back, to say to FranciscoLuz the to do the tweaks, would be much more productive making patches towards concrete issues in the issue queue from the module.

In shorter words: "yes, the module is good enough to be promoted, used, tested, patched and evolve with the community"

p.s: sorry to put on "needs work" before.. was an erroneous click that i didn't saw and saved

I've tested it as well. The only problems I found are those that are currently in the Issue Tracker for the module, and 1 additional which I will address hereafter.
I apologize for writing my review after the status has already been updated to "reviewed & tested by community", as I had business to attend earlier today when I began reviewing it. However I thought that the following 2 issues really should be looked at.

One of the two in the issue tracker is of utmost priority, and is as follows:

1. IssueWhen user is @ checkout/#/review (reviewing their order), if user performs the following, card type("flag") becomes corrupted, and must be re-initialized by either reloading the page or clicking an alternate payment method (and returning) to forcibly reload the ajax form data.... Reproducible steps:
   • Click "By Credit or Debit Card." radio button
   • Select "Debit" (as opposed to "Credit" - the default) radio button
   • Notice the combobox now is empty, and will not work until form is reloaded.

Aside from that, I personally was unable to install the module without modifying the .install file. The error is: Fatal error: Class 'CommerceCieloLib' not found.
This occurs because the CommerceCieloLib class that is included by the .info file:
files[] = includes/commerce_cielo.external.class.inc
is not instantiated during the hook_requirements "install" $phase. Therefore I had to replace the following @ line #30:

$cielo_class_file = drupal_realpath(libraries_get_path('cielo-php-lib')) . DIRECTORY_SEPARATOR . 'Cielo.class.php';
        // Check if Cielo class is installed.
        if (file_exists($cielo_class_file)) {
          $requirements['cielo_php_lib'] = array(
            'value' => $t("Cielo Developer's Manual: %manual_ver", array('%manual_ver' => CommerceCieloLib::DEV_MANUAL)),
            'severity' => REQUIREMENT_OK,
          );
        }

with:

 $cielo_class_file = drupal_realpath(libraries_get_path('cielo-php-lib')) . DIRECTORY_SEPARATOR . 'Cielo.class.php';
        // Check if Cielo class is installed.
        if (file_exists($cielo_class_file)) {


          // Not present during 'install' phase.
          // http://drupal-br.org/geral/anuncios/modulo-cielo-para-drupal-commerce
          if ($phase=='install') {
            include_once $cielo_class_file;
            $sz_manual=Cielo::DEV_MANUAL;
          } else $sz_manual=CommerceCieloLib::DEV_MANUAL;


          $requirements['cielo_php_lib'] = array(
            'value' => $t("Cielo Developer's Manual: %manual_ver", array('%manual_ver' => $sz_manual)),
            'severity' => REQUIREMENT_OK,
          );
        }

I assume this is something only a few people encounter (as I am the 1st to mention it), and I am unsure why that is.

-------------------------------------------------------------------------------------------------
Everything else worked exactly as expected for me. With modules like these my main concern would be security, and I am honest enough to admit I'm incapable of testing the security of transferring credit/debit card information from the module's non-SSL form page --> to the Drupal module back-end --> to the SSL payment processor Cielo page.
But from what I have seen (I can't speak Portuguese), it looks like you are MD5'ing and conforming to the Ceilo standards for authenticated requests. And you conform to the Drupal standards. So in short, looks good to me.
* Coder came back with a couple of formatting issues, but nothing serious. Just an fyi.

I did not change the status, as the Issue Tracker contains the former issue above, and the latter issue appears to occur only with certain installments(very few have encountered it apparently - url provided in source above). Therefore I have added this new Issue to the module's Issue Tracker, for your review and potential implementation if deemed appropriate.
Hope this helps.

I think there are some work to be done on the project, but that is nothing that is going to stop for you getting the git vetted user role, so thanks for your contribution, FranciscoLuz! 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.

Welcome aboard FranciscoLuz!