Add GA Support for Submit and update service#532
Conversation
rosado
left a comment
rosado
left a comment
There was a problem hiding this comment.
Looks good, but can you also update
- the config schema in
config/util.jswith an optional GA entry? It's a decent form of documentation - add item to the list in README under API Keys/Secrets
DilwoarH
force-pushed
the
519-add-google-analytics
branch
from
974145d to
a26fee9
Compare
Done |
|
Will there be an associated cookie banner and update to the cookie & privacy notice? (Apologies if that’s on another issue but important they’re released at the same time.) |
Hi Steve - Yes, we have been talking about the content for this and the things we need to add in. Will keep you posted and only merge this once we have the banner up first. |
DilwoarH
changed the title
|
This is blocked until we have the cookie banner in |
DilwoarH
force-pushed
the
519-add-google-analytics
branch
from
a26fee9 to
095f370
Compare
WalkthroughThe pull request introduces Google Analytics integration across multiple configuration files and templates. It adds a Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (10)
src/views/common/google-analytics.js (1)
1-7: Add performance optimisation and documentationConsider these improvements:
- Load GA script asynchronously to prevent blocking page load
- Add JSDoc comments explaining the purpose and usage
- Consider implementing a queue mechanism for events before consent is granted
Here's a suggested implementation:
/** * Google Analytics integration module * @description Handles GA tracking with respect to cookie consent * @requires Cookie consent to be implemented */ // Queue for storing events before consent window.gaEventQueue = []; // Initialize GA only if consent is granted function initializeGA() { if (document.cookie.includes('analytics_cookies_accepted=true')) { window.dataLayer = window.dataLayer || []; processEventQueue(); } } function gtag() { if (document.cookie.includes('analytics_cookies_accepted=true') && window.dataLayer) { window.dataLayer.push(arguments); } else { window.gaEventQueue.push(arguments); } } function processEventQueue() { while (window.gaEventQueue.length > 0) { const args = window.gaEventQueue.shift(); window.dataLayer.push(args); } } // Initialize initializeGA(); gtag('js', new Date()); gtag('config', '{{ googleAnalyticsMeasurementId }}');config/test.yaml (2)
12-13: Fix indentation to maintain consistencyThe
googleAnalyticssection should use 4 spaces for indentation to match the rest of the file.Apply this diff:
} -googleAnalytics: { - measurementId: 'G-TEST-CODE' +googleAnalytics: { + measurementId: 'G-TEST-CODE' }🧰 Tools
🪛 yamllint
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
12-13: Consider adding cookie consent configurationGiven that GA tracking should only occur when users accept cookies (as per PR objectives), consider extending the configuration to include cookie consent settings. This would help maintain a clear connection between consent and tracking functionality.
Would you like me to propose an extended configuration structure that includes cookie consent settings?
🧰 Tools
🪛 yamllint
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
test/unit/landingPage.test.js (1)
1-1: Address the TODO comment at the top of the fileThe comment indicates that this test needs to be duplicated for the submit start page. This should be addressed before merging.
Would you like me to help create the duplicate test suite for the submit start page?
src/serverSetup/nunjucks.js (1)
61-63: Consider adding type validation for the Google Analytics configuration.The implementation follows the established pattern for optional features, but could benefit from validation to ensure the measurement ID is properly defined.
Consider this enhancement:
if ('googleAnalytics' in config) { + if (!config.googleAnalytics?.measurementId) { + logger.warn('Google Analytics measurement ID is not properly configured') + return + } globalValues.googleAnalyticsMeasurementId = config.googleAnalytics.measurementId }config/util.js (2)
53-63: Consider enhancing schema validation and documentationThe new configuration schema entries are well-structured, but could benefit from:
- JSDoc comments explaining the purpose of each configuration object
- Stricter validation for the Google Analytics measurement ID (should follow the format 'G-XXXXXXXXXX')
Consider applying this enhancement:
+ /** + * Optional Smartlook configuration for user session recording + * @property {string} key - Smartlook project key + * @property {string} region - Smartlook data centre region + */ smartlook: v.optional( v.object({ key: v.string(), region: v.string() }) ), + /** + * Optional Google Analytics configuration + * @property {string} measurementId - GA4 measurement ID (format: G-XXXXXXXXXX) + */ googleAnalytics: v.optional( v.object({ - measurementId: v.string() + measurementId: v.pipe( + v.string(), + v.regex(/^G-[A-Z0-9]{10}$/) + ) }) )
59-63: Consider adding cookie consent configurationThe PR objectives specify that GA tracking should only occur after cookie consent. However, the current schema doesn't include configuration for managing this requirement.
Consider extending the schema to include cookie consent configuration:
googleAnalytics: v.optional( v.object({ - measurementId: v.string() + measurementId: v.string(), + cookieConsent: v.object({ + cookieName: v.string(), + cookieExpiry: v.number(), + privacyPolicyUrl: v.string() + }) }) )This would help ensure that the GA implementation consistently respects user consent across all environments.
readme.md (3)
32-32: Fix indentation for consistencyThe indentation should be 2 spaces to match other entries in the list, rather than 4 spaces.
- - **Used for**: Processing user submitted data + - **Used for**: Processing user submitted data🧰 Tools
🪛 Markdownlint
32-32: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
49-52: Fix indentation for consistencyThe indentation should be 2 spaces to match other entries in the list, rather than 4 spaces.
- - **Description**: Web Analytics - - **Used for**: Collecting data on website usage - - **Contact**: Providers team @ MHCLG + - **Description**: Web Analytics + - **Used for**: Collecting data on website usage + - **Contact**: Providers team @ MHCLG🧰 Tools
🪛 Markdownlint
50-50: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
51-51: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
52-52: Expected: 2; Actual: 4
Unordered list indentation(MD007, ul-indent)
85-85: Fix additional typos in the textThere are several typographical errors in this section that need correction.
-depeding on `NODE_ENV` environment variable (set to 'production' by default). +depending on `NODE_ENV` environment variable (set to 'production' by default). -You can start the appliction in one fo the following ways (see "scripts" section +You can start the application in one of the following ways (see "scripts" section
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
config/production.yaml(1 hunks)config/test.yaml(1 hunks)config/util.js(1 hunks)readme.md(4 hunks)src/serverSetup/nunjucks.js(1 hunks)src/views/common/google-analytics.js(1 hunks)src/views/layouts/main.html(1 hunks)src/views/privacy-notice.html(4 hunks)test/unit/landingPage.test.js(2 hunks)test/unit/views/common/google-analytics.test.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/views/privacy-notice.html
🧰 Additional context used
🪛 yamllint
config/test.yaml
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
🪛 Markdownlint
readme.md
32-32: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
50-50: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
51-51: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
52-52: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (6)
config/production.yaml (1)
18-20: Please verify the Google Analytics measurement ID and add documentation.
The measurement ID follows the correct GA4 format. However, to ensure maintainability:
- Verify this is the correct measurement ID for your property
- Consider adding a comment explaining which GA property this ID belongs to
✅ Verification successful
Google Analytics measurement ID is properly configured and documented
The measurement ID G-F2H5SW06PZ is correctly integrated across the codebase:
- Properly configured in
config/production.yamlwith validation inconfig/util.js - Correctly implemented in templates (
src/views/layouts/main.htmlandsrc/views/common/google-analytics.js) - Test configuration exists with a test measurement ID
- Integration is documented in the readme.md with a reference to Google Analytics
No documentation concerns identified as the implementation follows a clear pattern with proper validation and test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the measurement ID is documented in README or similar files
echo "Searching for measurement ID documentation..."
rg -i "measurement.*id|google.*analytics.*id|G-F2H5SW06PZ" README.md docs/
Length of output: 227
Script:
#!/bin/bash
# Let's check for any documentation files in the repository
echo "Searching for documentation files..."
fd -e md -e mdx -e txt
# Search for any Google Analytics related configuration or documentation
echo -e "\nSearching for Google Analytics references..."
rg -i "google.*analytics|measurement.*id|tracking"
# Check if there are any existing configuration files that might contain GA documentation
echo -e "\nSearching for configuration files..."
fd "config|settings" -e yaml -e yml -e json
Length of output: 2927
test/unit/views/common/google-analytics.test.js (1)
1-4: LGTM: Imports and setup look correct.
The necessary testing utilities and Nunjucks setup are properly imported and initialised.
test/unit/landingPage.test.js (1)
3-3: LGTM: Clean import statement
The addition of expect and it to the imports is appropriate for the new test case.
config/util.js (1)
59-63:
Verify GA measurement ID handling
To prevent accidental mixing of test and production analytics:
Consider adding environment-specific validation:
googleAnalytics: v.optional(
v.object({
- measurementId: v.string()
+ measurementId: v.pipe(
+ v.string(),
+ (input) => {
+ if (process.env.NODE_ENV === 'production' && input.includes('TEST')) {
+ throw new Error('Test measurement ID detected in production');
+ }
+ return input;
+ }
+ )
})
)src/views/layouts/main.html (2)
102-107: LGTM: Proper script organisation and loading sequence.
The script loading sequence is well-structured, with proper module type declarations and component initialisation.
111-116:
Critical: Add cookie consent check before loading analytics.
The current implementation only checks for the presence of googleAnalyticsMeasurementId, but according to the PR objectives, analytics should only load when users have explicitly accepted cookies.
Consider modifying the condition to include a cookie consent check:
- {% if googleAnalyticsMeasurementId %}
+ {% if googleAnalyticsMeasurementId and cookieConsent %}
<script async src="https://www.googletagmanager.com/gtag/js?id={{ googleAnalyticsMeasurementId }}"></script>
<script type='text/javascript'>
{% include "common/google-analytics.js" %}
</script>
{% endif %}Additionally, consider these security and performance improvements:
- Add
nonceattributes to inline scripts for CSP compliance - Consider using the
deferattribute instead ofasyncif analytics.js depends on other scripts
Let's verify the cookie consent implementation:
DilwoarH
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
src/views/cookies.html (1)
173-194: Consider separate controls for different analytics servicesCurrently, a single toggle controls both Google Analytics and Smartlook cookies. Consider providing separate controls to allow users to choose which analytics service they want to enable.
{{ govukRadios({ - name: "accept_cookies", - idPrefix: "cookies-analytics", + name: "analytics_preferences", + idPrefix: "cookies-analytics-preferences", fieldset: { legend: { - text: "Do you want to accept analytics cookies?", + text: "Which analytics services would you like to enable?", classes: "govuk-fieldset__legend--s" } }, items: [ { - value: "true", - text: "Yes", - checked: cookiePreferences == "true" + value: "both", + text: "Enable both Google Analytics and Smartlook", + checked: cookiePreferences == "both" }, { - value: "false", - text: "No", - checked: cookiePreferences == "false" + value: "ga_only", + text: "Enable Google Analytics only", + checked: cookiePreferences == "ga_only" + }, + { + value: "smartlook_only", + text: "Enable Smartlook only", + checked: cookiePreferences == "smartlook_only" + }, + { + value: "none", + text: "Disable all analytics", + checked: cookiePreferences == "none" } ] }) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/serverSetup/nunjucks.js(1 hunks)src/views/common/google-analytics.js(1 hunks)src/views/cookies.html(1 hunks)src/views/layouts/main.html(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/views/common/google-analytics.js
- src/views/layouts/main.html
🧰 Additional context used
🪛 GitHub Check: test
src/serverSetup/nunjucks.js
[failure] 64-64: test/unit/check-answers.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/check-answers.test.js:20:20
[failure] 64-64: test/unit/choose-datasetPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/choose-datasetPage.test.js:6:18
[failure] 64-64: test/unit/dataset-details.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/dataset-details.test.js:14:18
[failure] 64-64: test/unit/datasetTaskListPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/datasetTaskListPage.test.js:8:18
[failure] 64-64: test/unit/default-http-errors.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/default-http-errors.test.js:4:18
[failure] 64-64: test/unit/findPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/findPage.test.js:9:18
[failure] 64-64: test/unit/get-startedPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/get-startedPage.test.js:10:18
[failure] 64-64: test/unit/http-errorPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/http-errorPage.test.js:8:18
[failure] 64-64: test/unit/issueDetailsPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/issueDetailsPage.test.js:9:18
[failure] 64-64: test/unit/landingPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/landingPage.test.js:7:18
🔇 Additional comments (3)
src/serverSetup/nunjucks.js (2)
61-63: LGTM: GA measurement ID setup looks correct
The conditional check for Google Analytics configuration and the addition of the measurement ID to global values is implemented correctly.
61-68: Verify: Cookie consent implementation needs review
The current implementation sets up Google Analytics tracking before the cookie banner is ready (as mentioned in PR objectives). This could lead to premature tracking.
Let's verify the cookie consent implementation:
#!/bin/bash
# Check cookie banner and consent implementation
echo "Checking for cookie banner implementation..."
fd -g '*cookie*.{js,jsx,ts,tsx,html}'
echo "\nChecking how cookie consent is used..."
rg -g '!node_modules' -g '!dist' "cookies_preferences_set"
echo "\nChecking GA script loading conditions..."
rg -g '!node_modules' -g '!dist' "googleAnalyticsMeasurementId"🧰 Tools
🪛 GitHub Check: test
[failure] 64-64: test/unit/check-answers.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/check-answers.test.js:20:20
[failure] 64-64: test/unit/choose-datasetPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/choose-datasetPage.test.js:6:18
[failure] 64-64: test/unit/dataset-details.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/dataset-details.test.js:14:18
[failure] 64-64: test/unit/datasetTaskListPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/datasetTaskListPage.test.js:8:18
[failure] 64-64: test/unit/default-http-errors.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/default-http-errors.test.js:4:18
[failure] 64-64: test/unit/findPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/findPage.test.js:9:18
[failure] 64-64: test/unit/get-startedPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/get-startedPage.test.js:10:18
[failure] 64-64: test/unit/http-errorPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/http-errorPage.test.js:8:18
[failure] 64-64: test/unit/issueDetailsPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/issueDetailsPage.test.js:9:18
[failure] 64-64: test/unit/landingPage.test.js
TypeError: Cannot read properties of undefined (reading 'use')
❯ Module.setupNunjucks src/serverSetup/nunjucks.js:64:9
❯ test/unit/landingPage.test.js:7:18
src/views/cookies.html (1)
98-144: Verify analytics strategy regarding dual tracking
The page now includes both Google Analytics and Smartlook tracking. Consider:
- Is dual tracking necessary?
- Could this impact page performance?
- Should users be able to opt-out of each service independently?
✅ Verification successful
Dual tracking implementation is properly managed
The implementation shows appropriate separation of concerns:
- Google Analytics loads only when cookies are accepted (
cookiesAccepted == true) - Smartlook is conditionally loaded based on presence of
smartlookKey - Both trackers serve different purposes: GA for basic analytics, Smartlook for user behaviour insights
- Performance impact is minimised through async loading
- Users can control tracking through cookie preferences
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if both tracking scripts are loaded simultaneously
rg -l "smartlook" "src/views/layouts/"
rg -l "google-analytics" "src/views/layouts/"
Length of output: 136
Script:
#!/bin/bash
# Check the actual implementation of both tracking scripts
cat src/views/layouts/main.html
Length of output: 3494
Script:
#!/bin/bash
# Check the implementation of both analytics scripts and cookie consent logic
cat src/views/common/smartlook.js
cat src/views/common/google-analytics.js
Length of output: 854
DilwoarH
force-pushed
the
519-add-google-analytics
branch
from
63b1e7e to
c26a0a4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/unit/views/layouts/main.test.js (2)
1-4: Consider extracting test configurationThe test setup could be more maintainable. Consider moving the test configuration, including the GA measurement ID, to a dedicated test configuration file.
import { describe, expect, it } from 'vitest' import { setupNunjucks } from '../../../../src/serverSetup/nunjucks.js' +import { testConfig } from '../../../test-config.js' -const nunjucks = setupNunjucks({ datasetNameMapping: new Map() }) +const nunjucks = setupNunjucks({ + datasetNameMapping: new Map(), + googleAnalytics: testConfig.googleAnalytics +})
10-10: Strengthen test assertionsThe current assertions only check for the script URL. Consider using more specific assertions to validate:
- Script attributes
- Measurement ID format
- Script placement in the HTML head
- expect(html).toContain('<script async src="https://www.googletagmanager.com/gtag/js?id=G-TEST-CODE"></script>') + expect(html).toMatch(/<head>.*<script[^>]*async[^>]*src="https:\/\/www\.googletagmanager\.com\/gtag\/js\?id=G-[A-Z0-9-]+".*>.*<\/head>/s)Also applies to: 16-16
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
test/unit/views/common/google-analytics.test.js(1 hunks)test/unit/views/layouts/main.test.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unit/views/common/google-analytics.test.js
🔇 Additional comments (1)
test/unit/views/layouts/main.test.js (1)
6-18: 🛠️ Refactor suggestion
Enhance test coverage with additional scenarios
Whilst the basic acceptance/rejection scenarios are covered, consider adding tests for:
- Undefined/null cookie acceptance values
- Missing or invalid GA measurement ID
- Complete GA script implementation including the
gtagfunction setup
Add these test cases:
it('should handle undefined cookie acceptance gracefully', () => {
const html = nunjucks.render('layouts/main.html', {})
expect(html).not.toContain('googletagmanager.com')
})
it('should verify complete GA script implementation', () => {
const html = nunjucks.render('layouts/main.html', { cookiesAccepted: true })
expect(html).toContain('gtag(\'config\'')
expect(html).toContain('gtag(\'js\'')
})Let's verify the GA script implementation across the codebase:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
vite.config.js (1)
19-19: Consider adding analytics-specific exclusionsThe current exclusion patterns are sensible for general coverage reporting. However, since this PR introduces Google Analytics, consider whether any analytics-specific files (e.g., tracking scripts, analytics configuration) should also be excluded from coverage reporting.
- exclude: ['**/node_modules/**', '**/test/**', '**/public/**', '**/coverage/**', '**/vite.config.js'] + exclude: [ + '**/node_modules/**', + '**/test/**', + '**/public/**', + '**/coverage/**', + '**/vite.config.js', + '**/analytics/**' // Analytics-specific files + ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
vite.config.js(1 hunks)
🔇 Additional comments (1)
vite.config.js (1)
17-17: Excellent practice: Generating coverage reports even on test failures
The reportOnFailure setting ensures visibility into code coverage regardless of test outcomes, which is particularly valuable when debugging failing tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
config/test.yaml (1)
12-13: Fix indentation to maintain consistencyThe
googleAnalyticssection uses 2-space indentation, whilst 4 spaces are expected. Please update to match the file's indentation style.Apply this diff to fix the indentation:
googleAnalytics: { - measurementId: 'G-TEST-CODE' + measurementId: 'G-TEST-CODE' }🧰 Tools
🪛 yamllint
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
readme.md (1)
85-85: Fix typos in the running instructionsThere are two typographical errors in this section:
- "depeding" should be "depending"
- "appliction" should be "application"
Apply this diff to fix the typos:
-depeding on `NODE_ENV` environment variable (set to 'production' by default). +depending on `NODE_ENV` environment variable (set to 'production' by default). -You can start the appliction in one fo the following ways (see "scripts" section +You can start the application in one of the following ways (see "scripts" section
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
config/production.yaml(1 hunks)config/test.yaml(1 hunks)config/util.js(1 hunks)readme.md(4 hunks)src/serverSetup/nunjucks.js(1 hunks)src/views/common/google-analytics.js(1 hunks)src/views/cookies.html(1 hunks)src/views/layouts/main.html(1 hunks)src/views/privacy-notice.html(4 hunks)test/unit/views/common/google-analytics.test.js(1 hunks)test/unit/views/layouts/main.test.js(1 hunks)vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- config/production.yaml
- config/util.js
- src/serverSetup/nunjucks.js
- src/views/common/google-analytics.js
- src/views/cookies.html
- src/views/layouts/main.html
- src/views/privacy-notice.html
- test/unit/views/common/google-analytics.test.js
- test/unit/views/layouts/main.test.js
- vite.config.js
🧰 Additional context used
🪛 yamllint
config/test.yaml
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
🪛 Markdownlint
readme.md
32-32: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
50-50: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
51-51: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
52-52: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (3)
config/test.yaml (1)
12-13: Verify cookie consent integration
The PR objectives specify that GA should only track users who accept cookies. Please ensure this configuration supports conditional GA loading based on cookie consent.
Consider adding a configuration option to control GA activation based on cookie consent, perhaps something like:
googleAnalytics: {
measurementId: 'G-TEST-CODE',
requiresCookieConsent: true
}Let's verify the cookie consent integration:
✅ Verification successful
Cookie consent integration is properly implemented
The codebase already has robust cookie consent integration with Google Analytics:
- GA script is only loaded when
cookiesAcceptedis true (src/views/layouts/main.html) - Cookie consent check is implemented in multiple layers:
- Server-side via
cookiesAcceptedglobal variable (src/serverSetup/nunjucks.js) - Client-side double-check in GA initialisation script (
src/views/common/google-analytics.js)
- Server-side via
- Complete cookie management system is in place with:
- Cookie banner component (
src/assets/js/components/cookie-banner.js) - Cookie preferences page (
src/views/cookies.html) - Cookie preference update endpoint (
src/routes/cookies.js)
- Cookie banner component (
The suggested configuration option is unnecessary as the cookie consent integration is already properly implemented at the application level.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for cookie consent integration with GA
# Expect: Files showing cookie consent checks before GA initialisation
echo "Checking for cookie consent integration..."
# Look for cookie consent checks near GA initialization
rg -B 5 -A 5 "googleAnalytics.*measurementId" src/
# Look for cookie-related functions/variables near GA code
rg -l "cookie.*consent" src/
Length of output: 889
Script:
#!/bin/bash
# Let's check for cookie-related templates and scripts
echo "Checking for cookie-related templates..."
rg -B 5 -A 5 "cookies_preferences_set" src/
# Check for GA script inclusion in templates
echo "Checking GA script inclusion..."
fd -e html -e njk | xargs rg -l "googleAnalytics"
# Look for cookie banner implementation
echo "Checking cookie banner implementation..."
fd -e js -e ts | xargs rg -l "cookie.*banner|cookie.*consent"
Length of output: 4546
Script:
#!/bin/bash
# Check the actual GA script implementation
echo "Checking GA implementation..."
rg -B 5 -A 10 "googleAnalyticsMeasurementId" src/
Length of output: 2367
🧰 Tools
🪛 yamllint
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
readme.md (2)
32-32: LGTM!
The formatting and content changes for the Local Authorities API section look good.
🧰 Tools
🪛 Markdownlint
32-32: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
49-52:
Add privacy and cookie consent documentation for Google Analytics
Based on the PR objectives, Google Analytics tracking should only be enabled for users who accept cookies. This crucial privacy information needs to be documented.
Apply this diff to add the necessary privacy information:
- [Google Analytics](https://analytics.google.com/analytics)
- **Description**: Web Analytics
- - **Used for**: Collecting data on website usage
+ - **Used for**: Collecting data on website usage (only enabled for users who accept cookies)
- **Contact**: Providers team @ MHCLG
+ - **Privacy**: Requires explicit cookie consent via banner before tracking
+ - **Data Collection**: No data is collected if cookies are rejected🧰 Tools
🪛 Markdownlint
50-50: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
51-51: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
52-52: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
DilwoarH
force-pushed
the
519-add-google-analytics
branch
from
f8e17ce to
c1985ac
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
config/test.yaml (1)
12-13: Fix YAML indentationThe
googleAnalyticssection has incorrect indentation. Please use 4 spaces to maintain consistency with the rest of the file.Apply this diff to fix the indentation:
googleAnalytics: { - measurementId: 'G-TEST-CODE' + measurementId: 'G-TEST-CODE' }🧰 Tools
🪛 yamllint
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
readme.md (1)
Line range hint
85-89: Fix remaining typos in the documentationThere are two remaining typos in this section:
- "depeding" should be "depending"
- "appliction" should be "application"
Apply this diff to fix the typos:
-depeding on `NODE_ENV` environment variable (set to 'production' by default). +depending on `NODE_ENV` environment variable (set to 'production' by default). -You can start the appliction in one fo the following ways (see "scripts" section +You can start the application in one of the following ways (see "scripts" section
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
config/production.yaml(1 hunks)config/test.yaml(1 hunks)config/util.js(1 hunks)readme.md(4 hunks)src/serverSetup/nunjucks.js(1 hunks)src/views/common/google-analytics.js(1 hunks)src/views/cookies.html(1 hunks)src/views/layouts/main.html(1 hunks)src/views/privacy-notice.html(4 hunks)test/unit/views/common/google-analytics.test.js(1 hunks)test/unit/views/layouts/main.test.js(1 hunks)vite.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- config/production.yaml
- config/util.js
- src/serverSetup/nunjucks.js
- src/views/common/google-analytics.js
- src/views/cookies.html
- src/views/layouts/main.html
- src/views/privacy-notice.html
- test/unit/views/common/google-analytics.test.js
- test/unit/views/layouts/main.test.js
- vite.config.js
🧰 Additional context used
🪛 yamllint
config/test.yaml
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
🪛 Markdownlint
readme.md
32-32: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
50-50: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
51-51: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
52-52: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
🔇 Additional comments (4)
config/test.yaml (1)
13-13: Measurement ID format issue already flagged
Skipping comment about the measurement ID format as it's already covered in past reviews.
🧰 Tools
🪛 yamllint
[warning] 13-13: wrong indentation: expected 4 but found 2
(indentation)
readme.md (3)
32-32: LGTM!
The indentation is consistent with other sections in the document.
🧰 Tools
🪛 Markdownlint
32-32: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
49-52: Previous review comment about privacy documentation is still applicable
The documentation needs to include privacy and cookie consent information as previously suggested.
🧰 Tools
🪛 Markdownlint
50-50: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
51-51: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
52-52: Expected: 2; Actual: 4
Unordered list indentation
(MD007, ul-indent)
73-73: LGTM!
The code block formatting is now correct.
5 participants
What type of PR is this? (check all applicable)
Description
Add GA Support for Submit and update service. This allows us to track user behaviour.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Please replace this line with instructions on how to test your changes, a note
on the devices and browsers this has been tested on, as well as any relevant
images for UI changes.
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
have not been included
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
Release Notes
New Features
Documentation
Bug Fixes
Tests