[D7] Commerce Yandex.Metrics

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

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and 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.

FILE: /var/www/drupal-7-pareview/pareview_temp/commerce_yandex_metrics.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
45 | ERROR | Using the e flag in preg_match is a possible security risk. For
| | details see http://drupal.org/node/750148
--------------------------------------------------------------------------------

Very glad to see the module on the basis of Yandex.Metrics. Module provides useful stuff for ecommerce sites.

Here is my manual review.

1. Module weight (for discussion)

   I can't see why module weight is changed. You are not implementing any hooks which yandex_metrics implement, so .install file can be omitted. Maybe I miss something or maybe it is not needed?

2. $shipping, $tax_sum, $address calculation (needs work)

   I see you are calculating variables $shipping, $tax_sum and etc, but it seems they are not used anywhere.

3. Get counter_id with Yandex.Metrics module (removed because of performance)

   Try to use yandex_metrics_get_counter_for_current_site() instead of preg_match().

4. Use currency code from order, not default (needs work)

   Problem in this code:
   'currency' => variable_get('commerce_default_currency', 'RUR'),
   I completed order in USD, but got RUR in yaParams. You can always find proper currency code in order/commerce_order_total object.

5. Add information about dependencies (needs work)

   Provide info about module dependencies (with links) on module page and in README.txt

6. Repository
   1. Git name and email (future recommendation)
      Now you are using 3 different name/email pairs in your commits.
      Try to use one configuration for all commits on Drupal.org. If you are commiting from over computer, you can configure git identities per project. See http://drupal.org/node/1022156, http://drupal.org/node/1053134
   2. Commit messages (future recommendation)
      Make commit messages more informative. See http://drupal.org/node/52287
   3. Commit frequency(future recommendation)
      Try to use "one commit per one problem" concept. It will keep git history clean and useful.

Now your JS is added on complete page regardless of page specific and role specific Yandex.Metrics tracking settings. I opened an issue to fix it on Yandex.Metrics side: #1978382: Provide API for managing Yandex.Metrika goals You are welcome to discussion.

@kalabro, thanks for the good review!

3. I'm not sure it's good idea to use yandex_metrics_get_counter_for_current_site() in that context because it's heavy method which makes HTTP request to Yandex.Metrika, transforms internationalized domain name, looks for mirrors.
User will have to wait ThankYouForYourOrder page loading because of yandex_metrics_get_counter_for_current_site().
The preg_match approach is much faster.

What do you think, guys?

Thanks, Konstantin!
3. Agreed. Let's leave preg_match.
About #4: it's known issue #1779020: Invalid match for "Using the e flag in preg_match"

Looks pretty good! You may want to consider making the goal "ORDER_COMPLETE" a user-configurable setting in the future.

No major issues found.

----
Top Shelf Modules - Enterprise modules from the community for the community.

I confirm RTBC.

I'm new bee in git, tryed to make last commit according to yours recommendations.

This commit message can be "by holdmann | kalabro: Added ACBR-check before inject goal-code to checkout complete page." or just "by holdmann: Added ACBR-check before inject goal-code to checkout complete page." because there is no my code.

Good luck with your project!

Thanks for your contribution, holdmann!

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.

----
Top Shelf Modules - Crafted, Curated, Contributed.