-
Notifications
You must be signed in to change notification settings - Fork 20
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
Fix search referrer bug #3032
Fix search referrer bug #3032
Conversation
8b4faca
to
a71ebc0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, there's a small change needed for the CHANGELOG.
a71ebc0
to
7848754
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good, a few comments/mainly questions to help my understanding. I'd suggest merging the test commit into the code changes commit, as they're directly related (presumably the tests would fail without the code changes, and vice versa).
app/assets/javascripts/govuk_publishing_components/analytics-ga4/ga4-page-views.js
Outdated
Show resolved
Hide resolved
app/assets/javascripts/govuk_publishing_components/analytics-ga4/ga4-ecommerce-tracker.js
Outdated
Show resolved
Hide resolved
expected.page_view.dynamic = 'true' | ||
GOVUK.analyticsGa4.analyticsModules.PageViewTracker.init({ | ||
referrer: 'https://gov.uk/referrer', | ||
dynamic: 'true' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You've covered all the possibilities of these two keys existing/not existing very thoroughly here. I'm just curious though - I thought that they're intrinsically linked i.e. if there's a value for referrer
then dynamic
is always true, otherwise both are missing. Can you explain the three scenarios being tested here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is sort of linked to the first comment - if we're happy with keeping the code as it is, this means that the referrer
and dynamic
properties are being conditionally set by different values on the dynamicPageViewData
object and I'd just want to be absolutely sure that everything is working as it should in all possible scenarios
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Been wrestling with this over lunch. I think I'm coming around to the thought that this doesn't feel quite right - we're passing an object that contains two values, but only really using one of them (referrer). I'm quite happy to pass an object (it's more future proof, although I'm not aware of anything else we'd need to pass) but the code and tests currently imply that different values might be passed in that object, when in reality there are only two scenarios: nothing is passed, or referrer and dynamic (set to true) are passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks for this @andysellick. What I've done is I've changed the implementation slightly so that it's only using a single string parameter now rather than an object. Like you say, I'm not aware of anything further that would need to be passed and, thinking about it, I think passing an object when we only need a single value might not be the most intuitive. If this needs extending in the future we can make it an object but, in the interests of keeping the code more readable in its current form, perhaps just passing the string is best.
I've also made the corresponding change in finder-frontend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @JamesCGDS, this feels much tidier 👍
7848754
to
53c7039
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, good spot on the things I missed @andysellick
I'd squash the commits once Andy has seen the changes
@AshGDS - cool, will do. Just kept the commits separate for the time being so it was easier to see the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just need to squash some commits 👍
7875a99
to
90e83a4
Compare
When a results page is dynamically updated (e.g. by using filters) the referrer was not updating correctly. To fix this, the page location is grabbed in the finder-frontend JS before the the page updates and is passed to Ga4EcommerceTracker.init()
90e83a4
to
72d7400
Compare
What
This PR contains two key changes:
referrer
property on thepage_view
object was not updating correctly after dynamic page updatesdynamic
property to thepage_view
objectWhy
When a user is on a search results page and applies a filter, the URL of the page changes and PA's need to be able to track the URLs a user has visited using the
page_view.referrer
property. In the current implementation, however, thepage_view.referrer
property was not updating and remained the same whenever a dynamic update was triggered. This PR ensures that thepage_view.referrer
property is updated correctly by using a parameter that it receives fromlive_search.js
in thefinder-frontend
repository.In addition, PA's need to be able to distinguish between a fresh page load and a dynamic page update and the
dynamic
property has therefore been added to thepage_view
object. As requested by the PA's, it is a stringified boolean.Visual Changes
None.
N.B.
Related PR - alphagov/finder-frontend#2906
As this PR changes how
GOVUK.analyticsGa4.Ga4EcommerceTracker.init()
receives and handles parameters, these changes are breaking.