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

Feature - Added Optional Google Analytics tag for Status Page. #2567

Merged
merged 14 commits into from
Feb 4, 2023

Conversation

clcninja
Copy link

@clcninja clcninja commented Jan 8, 2023

  • I have read and understand the pull request rules.

Description

This PR adds in a Google Analytics tag field for a Status Page (allowing different tags for different pages/domains).

Fixes #2180 (issue)

Type of change

  • User interface (UI)
  • New feature (non-breaking change which adds functionality)

Checklist

  • My code follows the style guidelines of this project
  • I ran ESLint and other linters for modified files
  • I have performed a self-review of my own code and tested it
  • I have commented my code, particularly in hard-to-understand areas
    (including JSDoc for methods)
  • My changes generate no new warnings
  • My code needed automated testing. I have added them (this is optional task)

Screenshots (if any)

image

src/pages/StatusPage.vue Outdated Show resolved Hide resolved
@louislam
Copy link
Owner

louislam commented Jan 9, 2023

I am thinking, maybe we should use sever side rendering instead.

As the code have to put at beginning of <head>:
https://support.google.com/analytics/answer/1008080?hl=en#zippy=%2Cin-this-article

@clcninja
Copy link
Author

clcninja commented Jan 9, 2023

Sounds good, I'll refactor it into server/status_page.js

Copy link
Contributor

@Saibamen Saibamen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing new i18n keys

server/model/status_page.js Outdated Show resolved Hide resolved
server/database.js Outdated Show resolved Hide resolved
server/model/status_page.js Outdated Show resolved Hide resolved
@clcninja clcninja force-pushed the feature-google-analytics branch from 3194f48 to 7b36bfc Compare January 10, 2023 20:29
@clcninja clcninja requested review from Saibamen and louislam and removed request for Saibamen and louislam January 10, 2023 20:30
server/modules/google-analytics.js Outdated Show resolved Hide resolved
server/modules/google-analytics.js Outdated Show resolved Hide resolved
@louislam louislam added the question Further information is requested label Jan 12, 2023
@clcninja clcninja force-pushed the feature-google-analytics branch from 63e3ea7 to 01456bc Compare January 13, 2023 19:34
@clcninja clcninja requested a review from louislam January 15, 2023 12:23
@louislam louislam added this to the 1.20.0 milestone Jan 15, 2023
@clcninja clcninja requested a review from louislam January 22, 2023 16:59
@clcninja clcninja force-pushed the feature-google-analytics branch 3 times, most recently from 60a48cd to 760a61b Compare January 24, 2023 20:45
@louislam louislam removed the question Further information is requested label Jan 25, 2023
@louislam louislam added the question Further information is requested label Jan 31, 2023
@@ -0,0 +1,4 @@
-- You should not modify if this have pushed to Github, unless it does serious wrong with the db.
BEGIN TRANSACTION;
ALTER TABLE status_page ADD google_analytics_tag_id TEXT;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be VARCHAR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed ✔️

@louislam
Copy link
Owner

My codes (GA3 and GA4) do not seem to be working, it is still empty after save.

  • UA-36889556-1
  • G-8Y5FKTE8S4

@clcninja
Copy link
Author

My codes (GA3 and GA4) do not seem to be working, it is still empty after save.

  • UA-36889556-1
  • G-8Y5FKTE8S4

Ah that's getting blocked by the regex, apparently that looks different than what the google ads site specifies, I'll make it a bit less restrictive, obviously they have used different formats at different points.

@louislam
Copy link
Owner

louislam commented Jan 31, 2023

My codes (GA3 and GA4) do not seem to be working, it is still empty after save.

  • UA-36889556-1
  • G-8Y5FKTE8S4

Ah that's getting blocked by the regex, apparently that looks different than what the google ads site specifies, I'll make it a bit less restrictive, obviously they have used different formats at different points.

Maybe remove this regex? Because Google will keep adding new code format which will be hard to handle in the future.

@clcninja
Copy link
Author

My codes (GA3 and GA4) do not seem to be working, it is still empty after save.

  • UA-36889556-1
  • G-8Y5FKTE8S4

Ah that's getting blocked by the regex, apparently that looks different than what the google ads site specifies, I'll make it a bit less restrictive, obviously they have used different formats at different points.

Maybe remove this regex? Because Google will keep adding new code format which will be hard to handle in the future.

A good shout I think. I've removed it and updated the database field type.

@clcninja clcninja force-pushed the feature-google-analytics branch from 7209db5 to f153082 Compare February 2, 2023 21:52
@clcninja clcninja requested a review from Saibamen February 3, 2023 11:49
@louislam louislam removed the question Further information is requested label Feb 4, 2023
@@ -53,6 +54,13 @@ class StatusPage extends BeanModel {

const head = $("head");

if (statusPage.googleAnalyticsTagId) {
let escapedGoogleAnalyticsScript = jsesc(googleAnalytics.getGoogleAnalyticsScript(statusPage.googleAnalyticsTagId), {
Copy link
Owner

@louislam louislam Feb 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think jsesc is wrongly used, as getGoogleAnalyticsScript() returns with <script> tag which is not purely a javascript. I will fix it

@louislam louislam merged commit 06dc61b into louislam:master Feb 4, 2023
@stbc stbc mentioned this pull request Feb 17, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding Google Analytics
4 participants