fix: exceptional max attribute length and page_referrer flag addition #59

mmustafa-tse · 2024-04-30T16:57:49Z

Summary

This PR covers a bug fix and a feature request:

fix: Google has a couple of exceptions for some attributes/params max lengths such as page_title (300 chars), page_location (1000 chars), page_referrer (420 chars) (doc here) while we automatically truncate every attribute and custom flag to 100 chars, this resulted in an issue where the customer reported missing attribution on the Google Ads side due to this issue.
feature request: GA4 has a special param page_referrer that is similar to page_title and page_location and the request here is to include it as a custom flag to map into it. I included the new flag GA4.Referrer for the mapping. This flag has no legacy backwards compatibility as it wasn't available for GA Universal as you can see in the screenshot below taken from this GA support doc here

Also, just like the flags GA4.Location (page_location) and GA4.Title (page_title) I did include it's default value in case it was not provided by the customer which in this case is the document.referrer as mentioned here in this google support doc

Testing Plan

Included new and modified some unit tests and it passed as expected.
tested E2E using local overrides with the new code implemented and it worked as expected

Master Issue

Closes https://mparticle-eng.atlassian.net/browse/PRODRDMP-6731 and https://mparticle.lightning.force.com/lightning/r/Gap_Opportunity_Connector__c/a0uRg000001RWHlIAO/view

alexs-mparticle

Looks good so far. Most of my feedback is related to questions and some readability suggestions.

alexs-mparticle · 2024-05-01T20:58:03Z

packages/GA4Client/src/common.js

+            var val;
+            switch (key) {
+                case 'page_title':
+                    val = truncateString(
+                        attributes[attribute],
+                        PAGE_TITLE_MAX_LENGTH
+                    );
+                    break;
+                case 'page_referrer':
+                    val = truncateString(
+                        attributes[attribute],
+                        PAGE_REFERRER_MAX_LENGTH
+                    );
+                    break;
+                case 'page_location':
+                    val = truncateString(
+                        attributes[attribute],
+                        PAGE_LOCATION_MAX_LENGTH
+                    );
+                    break;
+                default:
+                    val = truncateString(attributes[attribute], valueLimit);
+            }


From a readability standpoint, I think it makes things more clear that you're overriding the value limit by just changing valueLimit inside of the switch chase.

Then it becomes:

Suggested change

var val;

switch (key) {

case 'page_title':

val = truncateString(

attributes[attribute],

PAGE_TITLE_MAX_LENGTH

);

break;

case 'page_referrer':

val = truncateString(

attributes[attribute],

PAGE_REFERRER_MAX_LENGTH

);

break;

case 'page_location':

val = truncateString(

attributes[attribute],

PAGE_LOCATION_MAX_LENGTH

);

break;

default:

val = truncateString(attributes[attribute], valueLimit);

}

var valueLimitOverride;

switch (key) {

case 'page_title':

valueLimitOverride = PAGE_TITLE_MAX_LENGTH;

break;

case 'page_referrer':

valueLimitOverride = PAGE_REFERRER_MAX_LENGTH

break;

case 'page_location':

valueLimitOverride = PAGE_LOCATION_MAX_LENGTH

break;

default:

valueLimitOverride = valueLimit;

}

var val = truncateString(attributes[attribute], valueLimitOverride);

alexs-mparticle · 2024-05-01T20:58:54Z

packages/GA4Client/src/event-handler.js

+        pageTitle = document.title,
+        pageReferrer = document.referrer;


Suggested change

pageTitle = document.title,

pageReferrer = document.referrer;

var pageTitle = document.title;

var pageReferrer = document.referrer;

if this is accepted as a suggestion, update the , on line 78 to a ; as well

packages/GA4Client/src/event-handler.js

alexs-mparticle · 2024-05-01T21:01:32Z

packages/GA4Client/test/src/tests.js

@@ -1522,6 +1522,7 @@ describe('Google Analytics 4 Event', function () {
                    {
                        page_title: 'Mocha Tests',
                        page_location: location.href,
+                        page_referrer: document.referrer,


I see that you added some tests that include page_referrer but are there any tests cases that reflect the behavior when it isn't included?

so as mentioned the default for page_referrer per google docs is document.referrer and I included it just how we default page_title and page_location

rmi22186 · 2024-05-01T21:23:24Z

packages/GA4Client/src/common.js

-            var val = truncateString(attributes[attribute], valueLimit);
+            var val;
+            switch (key) {
+                case 'page_title':


I would make each case a variable at the top of the page below the variables for max lengths

rmi22186 · 2024-05-01T21:25:42Z

packages/GA4Client/src/event-handler.js

+        pageTitle = document.title,
+        pageReferrer = document.referrer;


if this is accepted as a suggestion, update the , on line 78 to a ; as well

packages/GA4Client/src/event-handler.js

rmi22186 · 2024-05-01T21:29:26Z

packages/GA4Client/test/src/tests.js

@@ -1644,6 +1647,7 @@ describe('Google Analytics 4 Event', function () {
                    {
                        page_title: 'Foo Page Title',
                        page_location: '/foo',
+                        page_referrer: document.referrer,


any reason this is a reference to document.referrer an dnot a string like the others?

rmi22186 · 2024-05-02T01:02:27Z

also changed the branch we are pr-ing into from master to development

rmi22186 · 2024-05-03T16:35:02Z

I had to do some syncing of master and development, and had to update the branch for some reason to remove some commits that were in development already.

rmi22186

LGTM - once @alexs-mparticle has another look, we can merge

## [1.4.3](v1.4.2...v1.4.3) (2024-05-07) ### Bug Fixes * exceptional max attribute length and page_referrer flag addition ([#59](#59)) ([d81fff3](d81fff3))

fix: exceptional max attribute length and page_referrer flag

24a0955

mmustafa-tse requested review from rmi22186 and alexs-mparticle April 30, 2024 16:57

alexs-mparticle requested changes May 1, 2024

View reviewed changes

rmi22186 requested changes May 1, 2024

View reviewed changes

rmi22186 changed the base branch from master to development May 2, 2024 01:02

fix: exceptional max attribute length and page_referrer flag

825aaed

mmustafa-tse requested review from rmi22186 and alexs-mparticle May 2, 2024 20:42

rmi22186 changed the base branch from development to master May 3, 2024 16:34

rmi22186 changed the base branch from master to development May 3, 2024 16:34

rmi22186 approved these changes May 3, 2024

View reviewed changes

alexs-mparticle approved these changes May 3, 2024

View reviewed changes

rmi22186 merged commit d81fff3 into mparticle-integrations:development May 3, 2024
4 checks passed

github-actions bot pushed a commit that referenced this pull request May 7, 2024

chore(release): 1.4.3 [skip ci]

0cc9a35

## [1.4.3](v1.4.2...v1.4.3) (2024-05-07) ### Bug Fixes * exceptional max attribute length and page_referrer flag addition ([#59](#59)) ([d81fff3](d81fff3))

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: exceptional max attribute length and page_referrer flag addition #59

fix: exceptional max attribute length and page_referrer flag addition #59

mmustafa-tse commented Apr 30, 2024 •

edited

Loading

alexs-mparticle left a comment

alexs-mparticle May 1, 2024

alexs-mparticle May 1, 2024

rmi22186 May 1, 2024

alexs-mparticle May 1, 2024

mmustafa-tse May 2, 2024

rmi22186 May 1, 2024

rmi22186 May 1, 2024

rmi22186 May 1, 2024

rmi22186 commented May 2, 2024

rmi22186 commented May 3, 2024

rmi22186 left a comment

		pageTitle = document.title,
		pageReferrer = document.referrer;

fix: exceptional max attribute length and page_referrer flag addition #59

fix: exceptional max attribute length and page_referrer flag addition #59

Conversation

mmustafa-tse commented Apr 30, 2024 • edited Loading

Summary

Testing Plan

Master Issue

alexs-mparticle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmi22186 commented May 2, 2024

rmi22186 commented May 3, 2024

rmi22186 left a comment

Choose a reason for hiding this comment

mmustafa-tse commented Apr 30, 2024 •

edited

Loading