Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

3901: Webtrekk MWA traking #1283

Merged
merged 44 commits into from Dec 18, 2018

Conversation

Projects
None yet
2 participants
@holt83
Copy link
Contributor

commented Dec 3, 2018

Link to issue

https://platform.dandigbib.org/issues/3901

Description

In this PR the tracking capabilities of ding_webtrekk is extended to include several new Most Wanted Actions (MWAs) like reservation, rating, carousel click etc. The implementation follows the guidelines for parameter names and values from following spreadsheet, which also lists all the tracking that is added in this PR (those marked with "Skal implementeres"):

Webtrekk 20171128 - MWA list

With Webtrekk we have three methods of tracking:

Page parameters
These are used for information about the page that are known at load time and are only related to that page. For example what is the type of the material being shown or what sort was used on the current search result.
These are added as properties on the global _ti object. Example:

window._ti = window._ti || {};
window._ti['OSS'] = 'Krimi';
window._ti['OSSr'] = '5099';
window._ti['p_s_Page'] = '1';
window._ti['p_s_Sort'] = 'date_descending';
window._ti['p_OSSprofile'] = 'boeger'

URL parameters
These are used when we want track relations between pages. For example, did the material view originate from a carousel and if so which one? On the ting_search_carousel the "Read more"-button and Reserve-button will have following parameter appended to their URL:
/ting/object/870970-basis%3A29997829?u_navigatedby=Unge
Where "Unge" is the title of the carousel.

Events
For user interactions that doesn't result in a page load Webtrekk's event functionality is used. Examples is the carousel prev/next buttons and the reserve button which uses AJAX to make the reservation. It's implemented by calling the push() method on the wts Webtrekk-object and passing JSON-formatted event data to the function. Example:

wts.push(['send', 'click', {
  linkId:'Karousel, click på næste knappen',
  customClickParameter:{ 59:'Biblioteket foreslår materialer til dig'}
}]);

I have tried to handle as much on the server as possible, since there's much more reliable and convienient access to data. Events can be completely setup and attached on the server using HTML data-attribute, and a single catch-all click-handler will take care of pushing the events in the client.

Some things like autocomplete events are dependant on dynamic data created when the user is interacting, and therefore needs to be handled in client JS. Other stuff proved very problematic to handle on the server and is also handled in the client. For example, the ding_carousel theme often doesn't even know the name of it's carousel and it would require a rework or a hacky approach, to get the needed value for tracking parameter.

Checklist

  • My complies with our coding guidelines.
  • My code passes our static analysis suite. If not then I have added a comment explaining why this change should be exempt from the code standards and process.
  • My code passes our continuous integration process. If not then I have added a comment explaining why this change should be exempt from the code standards and process.

Additional comments or questions

The PR also includes some fixes for some critical stuff:

  1. The Webtrekk loading script was incorrectly placed in the footer of the page. I have moved it to the
    top of the head together with the page parameters as mentioned in the implementation guidelines: Webtrekk-Implementation-Guidelines. Also mentioned in this PR: #1274
  2. The internal node search was never meant to be tracking with separate parameters, resulting in these parameters never being actually send to webtrekk. The same parameters that is used for ting search should be used (OSS and OSSr).
  3. The code that was adding the page-parameter-script filterede out values zero-values and as a result important tracking of zero-hit searches has been lost and never tracked in Webtrekk. I have updated the code to still ensure no empty values, but make exceptions for the values 0 and '0'.

holt83 added some commits Nov 20, 2018

3901: Remove tracking of page title
It's decided by the business that the page title has no real traking
value in Webtrekk, since the important information is contained within
the URLs that is tracked. This also frees us from the concern of
provider names appearing in page title in the statistics. We could try
to prevent this, but if for example som third party modules is installed
it could add some pages under /user wich has usernames in page titles
and this would be difficult to control.

@holt83 holt83 force-pushed the vejlebib:3901-webtrekk-mwa branch from 8adb2b0 to b396899 Dec 4, 2018

@holt83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

It should be ready for renew now.

Scrutinizer complains about wts is not defined, but it's a global variable added by the Webtrekk script. Could maybe check if it exists in top of attach function and return early if it's not defined? Don't know if that would fix scrutinizer also.

@holt83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 4, 2018

Ok, the scrutinizer errors magically dissapeared. I just love when that happens! :D

@holt83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 5, 2018

I don't know what to do about that array indentation problem Scrutinizer keeps complaining about.

@holt83 holt83 force-pushed the vejlebib:3901-webtrekk-mwa branch from 64523c7 to 2132ecc Dec 5, 2018

@kasperg
Copy link
Member

left a comment

👍 I think this is really nicely done. Logical structure, well executed, well commented.

I have some questions and comments here and there.

Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.js Outdated
Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.js
Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.js Outdated
Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.js Outdated
linkId:'Materiale rating',
customClickParameter: {
58: rating,
57: contentId

This comment has been minimized.

Copy link
@kasperg

kasperg Dec 6, 2018

Member

57 and 58 are odd values. Is there any documentation for what they mean?

This comment has been minimized.

Copy link
@holt83

holt83 Dec 6, 2018

Author Contributor

These are just some arbitrary numbers, that the business has decided to use for the different parameters. They are listed in the spreadsheet under the column WTK.

This comment has been minimized.

Copy link
@holt83

holt83 Dec 6, 2018

Author Contributor

Perhaps they should be documented in code or maybe a link to the spreadsheet?

This comment has been minimized.

Copy link
@kasperg

kasperg Dec 17, 2018

Member

I would prefer a set of constants than a link to a spreadsheet but then again the value would also be 100% tied to the link id. It may not be worth it to implement something more advanced.

This comment has been minimized.

Copy link
@holt83

holt83 Dec 17, 2018

Author Contributor

I'll add some constants

Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.module Outdated
Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.module
Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.module
Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.module
Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.module

@holt83 holt83 force-pushed the vejlebib:3901-webtrekk-mwa branch from 675a691 to 7e61465 Dec 7, 2018

@holt83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 7, 2018

@kasperg I updated the PR with the suggested changes. Still have a few questions/comments to some of your comments.

I also discovered a small error, which i've added a fix for. The renew-all button is implemented by checking all renewable loans and then trigger a click on the renew-selected button. In the event that a user clicked on renew-all, this would trigger two events both renew-all and -selected. Fortunately, we can use jQuery's isTrigger to filter those clicks out.

Regarding scrutinizer: I don't know what to do about the remaining issues. They keep disappearing/popping up again everytime I update the PR.

@holt83 holt83 closed this Dec 7, 2018

@holt83 holt83 reopened this Dec 7, 2018

@kasperg
Copy link
Member

left a comment

I have taken another look. The primary thing preventing me from approving this is the use of isTriggered().

// this jQuere marker to ensure we don't send two events in this case.
// See: https://stackoverflow.com/a/10704256
if (e.isTrigger) {
return;

This comment has been minimized.

Copy link
@kasperg

kasperg Dec 17, 2018

Member

I appreciate the documentation but I am not a fan of this. As mentioned on the same StackOverflow thread e.isTrigger is internal API and entirely undocumented.

I suggest testing [event.originalEvent instead which is referenced in the jQuery event documentation]. This will be undefined for programmatically triggered events.

I tested this on the site using the following code as test:

jQuery('a.topbar-link-user').click(function(event) {
    console.log(event.originalEvent);
});                                  
jQuery('a.topbar-link-user').trigger('click');
// ... then manually click the login link

This comment has been minimized.

Copy link
@holt83

holt83 Dec 17, 2018

Author Contributor

Agree. I also found some additional information about it:

jquery/api.jquery.com#319
http://newcodeandroll.blogspot.com/2012/01/how-to-detect-if-event-is-triggered.html

The last one above also follows your suggestion. I'll update the code to use this approach

Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.module
Show resolved Hide resolved modules/ding_webtrekk/ding_webtrekk.module
linkId:'Materiale rating',
customClickParameter: {
58: rating,
57: contentId

This comment has been minimized.

Copy link
@kasperg

kasperg Dec 17, 2018

Member

I would prefer a set of constants than a link to a spreadsheet but then again the value would also be 100% tied to the link id. It may not be worth it to implement something more advanced.

@holt83

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2018

PR is updated!

@holt83 holt83 force-pushed the vejlebib:3901-webtrekk-mwa branch from b366e11 to 7189f0f Dec 17, 2018

@kasperg kasperg merged commit 1836145 into ding2:master Dec 18, 2018

8 of 9 checks passed

Scrutinizer Failure condition met
Details
ci/circleci: behat_tests Your tests passed on CircleCI!
Details
ci/circleci: drush_make Your tests passed on CircleCI!
Details
ci/circleci: gulp Your tests passed on CircleCI!
Details
ci/circleci: package Your tests passed on CircleCI!
Details
ci/circleci: php_lint Your tests passed on CircleCI!
Details
ci/circleci: site_install Your tests passed on CircleCI!
Details
ci/circleci: unit_tests Your tests passed on CircleCI!
Details
commit-message-check/gitcop All commit messages are valid
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.