-
Notifications
You must be signed in to change notification settings - Fork 9
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
PP-12559 Migrate from google analytics to gtag #966
Conversation
9e2f015
to
3b20ce0
Compare
@@ -22,39 +22,39 @@ describe("Application JS", () => { | |||
}); | |||
|
|||
it("should always call the showBannerIfConsentNotSet", () => { | |||
require("./application") | |||
require("./application.min.js") |
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.
Just wondering if the original tests were still working?
As I was expecting this wouldn't need to be changed.
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.
They didn't work for me locally, but Github doesn't seem to mind whatever I put so I'm guessing they're not being run as part of the static build process?
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 would probably leave it as /application
.
So this way, we are only getting your change in the PR.
I suspect that in github actions, it does not have the same issue.
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. Will do!
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 got a minor question
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.
Just left a comment
@@ -22,39 +22,39 @@ describe("Application JS", () => { | |||
}); | |||
|
|||
it("should always call the showBannerIfConsentNotSet", () => { | |||
require("./application") | |||
require("./application.min.js") |
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 would probably leave it as /application
.
So this way, we are only getting your change in the PR.
I suspect that in github actions, it does not have the same issue.
3b20ce0
to
ca82d00
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.
LGTM - good work
Context
Google is sunsetting Universal Analytics (GA3) at the end of June 2024.
The new version of analytics, GA4, recommends the use of Google Tag Manager. This is a functionality provided by Google, whereby arbitrary code may be entered into the Google Analytics console and then injected into a script on the application page.
The Google Tag Manager process bypasses Pay's secure development requirements (prioritisation, code written by trained software developers, code review and security testing) and is therefore not acceptable for use in any applications in the card data environment (CDE) or any applications that affect the CDE, as our PCI compliance would be at risk.
Google tag ('gtag') may be used as a static alternative to Google Tag Manager (see Google's documentation for more information about the difference between the two). This method does not include any code modifiable in the console and as such is suitable for use by GOV.UK Pay.
Changes proposed in this pull request
Implementation of gtag and removal of GA code.
Guidance to review
See Jira ticket PP-12559 . If you want to see this working locally you will need to rename the
application.min.js
fileapplication.js