-
Notifications
You must be signed in to change notification settings - Fork 102
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
Add events to Google Analytics with Node #245
base: main
Are you sure you want to change the base?
Add events to Google Analytics with Node #245
Conversation
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.
Hey, great start, we need to refactor the code a bit to fit our code standards and make things easier to read. Thank you so much!
93b4c47
to
df0b4ef
Compare
Hi @gewenyu99 I have changed the code and pushed the changes Thanks for the review comments |
node/events-to-ga/package.json
Outdated
"license": "ISC", | ||
"dependencies": { | ||
"node-appwrite": "^11.0.0", | ||
"node-fetch": "^3.3.2", |
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.
Can we use fetch from undici module?
You can see examples in other templates
node/events-to-ga/src/main.js
Outdated
const ga4MeasurementId = process.env.GA4_MEASUREMENT_ID; | ||
const ga4secret = process.env.GA4_API_SECRET; |
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.
Can we just use these process.env inline? Just a style thing
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.
Great PR! 🤯 We left some comments during the review, please check them out.
Hi @HrithikSampson can you please work on the suggested changes and request a re-review by tomorrow? We can add the hacktoberfest-accepted label then |
Ok I will do it by tomorrow |
692a880
to
2310fbb
Compare
node/events-to-ga/README.md
Outdated
@@ -0,0 +1,124 @@ | |||
<!-- Name your function --> |
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.
Can you remove the markdown comments like this?
node/events-to-ga/src/main.js
Outdated
import { fetch } from "undici"; | ||
import { verifyHeaders, formatIntoGoogleAnalyticsEvent } from "./utils.js"; | ||
|
||
/* Appwrite function */ |
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.
Can you remove this comment?
node/events-to-ga/src/main.js
Outdated
/* Appwrite function */ | ||
|
||
export default async ({ res, req, log, error }) => { | ||
// Listen for Appwrite events |
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.
Can you remove this comment?
node/events-to-ga/src/utils.js
Outdated
*/ | ||
export function verifyHeaders(req) { | ||
if (req.headers["x-appwrite-user-id"] == "") { | ||
throw `x-appwrite-user-id value in req.headers is not there`; |
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.
Can you do throw new Error("...")
here instead? and below
node/events-to-ga/src/utils.js
Outdated
throw `x-appwrite-user-id value in req.headers is not there`; | ||
} | ||
if (req.headers["x-appwrite-trigger"] != "event") { | ||
throw `Not triggered by event but by ${req.headers["x-appwrite-trigger"]}`; |
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.
Can you do throw new Error("...") here instead?
node/events-to-ga/package.json
Outdated
"undici": "^5.27.0" | ||
}, | ||
"devDependencies": { | ||
"prettier": "^3.0.3" |
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.
Can we also add a .pretterrc.json
with the same settings as other template?
node/events-to-ga/src/utils.js
Outdated
if (i % 2 == 0) oddElemArray.push(splitArray[i]); | ||
else | ||
wildCardArray.push([ | ||
`${oldElemArray[oldElemArray.length - 1]}Id`, | ||
splitArray[i], | ||
]); |
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.
Please add curly braces here, we prefer to use common syntax so the template is accessible
node/events-to-ga/src/main.js
Outdated
user_id: `${req.headers["x-appwrite-user-id"]}`, | ||
events: [ | ||
{ | ||
// Event names must have all characters as alphanumeric. |
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.
Is this comment necessary?
2310fbb
to
79551ff
Compare
de93988
to
55edded
Compare
7ea30ce
to
4098807
Compare
Testing Video: https://drive.google.com/file/d/1Guw74H-xbZm1Xl2mlIlNBnMYVUBuhrAr/view?usp=sharing Hi @Haimantika, I have requested for rereview |
node/events-to-ga/src/utils.js
Outdated
if (i % 2 == 0) oddElemArray.push(splitArray[i]); | ||
else | ||
wildCardArray.push([ | ||
`${oddElemArray[oddElemArray.length - 1]}Id`, |
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.
Hmm this will result in something like databasesId
, right? It would be better if it was singular to match how we typically have it.
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.
Also, this algorithm won't work for events like teams.*.memberships.*.update.status
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.
is it because of no 's' in update because that I can resolve but I cant find out a way to get [status,prefs,name,email,password] unless I consider them that these will not go to the wildcard array
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.
can I do that
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.
Can I assume the length of the words in wildCard array is more than 12 or somethiing to distinguish it
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.
is it because of no 's' in update because that I can resolve but I cant find out a way to get [status,prefs,name,email,password] unless I consider them that these will not go to the wildcard array
Because not every odd element is an ID. Although...I guess it's fine that status
ends up in the params...
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.
Can I assume the length of the words in wildCard array is more than 12 or somethiing to distinguish it
No, an ID isn't necessarily more than 12 chars.
4098807
to
887c231
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.
Please see #245 (comment)
887c231
to
a549fcd
Compare
a549fcd
to
d3d4895
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.
See #245 (comment)
Hey there! There were a lot of big PRs during this Hacktoberfest, and we wanted to give everyone ample time to collaborate with our engineering team. If you were able to merge your PRs during October, amazing. If it’s still not merged, don’t worry about it either. Either way, we’ve got your Hacktoberfest swag minted and ready to ship. Please comment with your Discord username here so we can contact you about your shipping information to deliver your Hacktoberfest swag. |
Hi, |
Be in touch soon! |
What does this PR do?
The function created will be triggered by configured Appwrite events and report these events to Google Analytics
Test Plan
I have checked the functionality by creating a dummy webapp and triggering event in Google Analytics by login and logout
Video Link: https://drive.google.com/file/d/1CONrh9BpSRaEv3JcdshtexRKQDRCUOU5/view?usp=sharing
Related PRs and Issues
Have you read the Contributing Guidelines on issues?
Yes