Hello,
I created a module which integrates Drupal Commerce transactions to Google Tag Manager. Popularity of this combination is growing but there is no module for that so far.
Code is based on well tested Commerce Google Analytics module. Module was developed for commerce site Expomat.cz (please do not test it there :-)
To ensure the module is working properly and complies with Drupal code standards I checked it via
Motivation
My name is Petr Parimucha and I've been working with Drupal since 2009. My speciality is Ubercart / Drupal Commerce. I'm developing many small modules that might be useful to the Drupal community. I'm eager to share them.
Links
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/Parisek/2263083.git
commerce_google_tag_manager
Link to project page: https://drupal.org/sandbox/parisek/2263083
Reviews of other projects
https://drupal.org/node/2261421#comment-8777063
https://drupal.org/node/2263319#comment-8777165
https://drupal.org/node/2257809#comment-8777779
Comments
Comment #1
parisekComment #2
scotthooker CreditAttribution: scotthooker commentedHi apologies but this functionality already exists in https://drupal.org/project/google_tag
Comment #3
PA robot CreditAttribution: PA robot commentedWe are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then 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.
Comment #4
parisekscotthooker: Thanks for comment, but how to achieve this functionality in https://drupal.org/project/google_tag ? I checked whole code and couldn't find it! :-)
Comment #5
parisekComment #6
scotthooker CreditAttribution: scotthooker commentedAh I see. Ok then this could be a good addition! You need to update the git link as thats your personal git clone link .
Comment #7
parisekCorrected git link
Comment #8
parisekComment #9
mpdonadioManual Review; I can't install as I don't have a working Commerce site at the moment.
1. You mention footer in the comment on line 18, but you are adding it to the header scope on line 125.
2. The label in commerce_google_tag_manager_default_rules_configuration() is untranslated, but
I am not 100% sure if this needs to be. I have also never see this directly built up this way; I have only seen
exported rules as strings that then get run through rules_import().
3. There is some extra space in commerce_google_tag_manager_enable() on the chained database methods.
4. Is there a better hook that you can use than commerce_google_tag_manager_init()? This gets called on every
non-cached page that Drupal handles, which includes image styles, AJAX, etc. That could wipe out the session info. Also,
as drupal_add_js() is gone
from Drupal 8 (https://drupal.org/node/2169605), you should move everything to #attached. Perhaps using hook_page_build,
and adding it as #attached there would be best.
5. The unfiltered data going into the inline Javascript gave me pause, but drupal_json_encode() will escape everything, so
there are no XSS concerns. May want a comment here.
I am leaving this as Needs Review, as I don't know whether #4 constitutes a blocking issue. The rest is minor.
Comment #10
heddncommerce_google_tag_manager_enable
Rather than adjusting module weights, using hook_module_implements_alter() is a better solution.
A dependency on entity module should probably be listed. I know that commerce depends on it too, but what if it stopped using it (not that it is a risk)? As a general rule, a module should list all its direct dependencies.
No need to revert to old school method to obtain the entity id. ex.
$line_item->value()->commerce_product[LANGUAGE_NONE][0]['product_id']
Replace with:
$line_item->commerce_product->getIdentifier()
Replace the hook_init() with hook_page_build()
The un-translated string should be translated in commerce_google_tag_manager.rules_defaults.inc.
Moving to needs work for these small fixes. This module is almost ready for RTBC.
Comment #11
heddnComment #12
parisekmpdonadio: Thank you for your review.
1. Fixed
2. Label is now translatable
3. Actually when I was thinking how to make it better via hook_module_implements_alter() I found out, that this step is not neccessary and deleted this file
4. Rewrited to hook_page_build() function
5. Added comment
heddn Thanks for review, I added dependency and remove "old school" method to access ID :-) Other was fixed based on mpdonadio review
Comment #13
parisekComment #14
mpdonadioAutomated Review
Super minor warning that can be ignored.
Manual Review
In commerce_google_tag_manager_ecommerce_js:73, the default value that core uses is 'Drupal'
Not seeing any module duplication, third-party code, security, or major API issues. I think this this is RBTC.
Comment #15
klausimanual review:
Thanks for your contribution, parisek!
I updated your account so you can 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:
You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay 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.
Comment #16
parisekThank you guys for reviews and promoting me
Issue closed