-
Notifications
You must be signed in to change notification settings - Fork 2
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
Cookie consent#45 #47
base: main
Are you sure you want to change the base?
Conversation
…and query based on who keys.
Hi @ddfridley: This is what I have on the consent model's schema so far:
For this task I see the Enciv Home branch changes already includes the Google Analytics code.
I was trying to understand what's left to be done - I read in the docs onConsent() runs on every subsequent page load, so does this already functionally do what's done in react-server-render, and would the change there be to remove the hardcoded push of the Google Analytics code? civil-server/app/components/app.jsx Lines 46 to 58 in bb663b0
|
The code for google analytics above should only run if the user has given the appropriate consent. The cookie consent code has some hooks for executing code based on what the user has agreed to. See https://cookieconsent.orestbida.com/advanced/manage-scripts.html#using-callbacks-events
|
…ling service runs, and make cookieconsent CSS work.
I found documentation in the CookieConsent repo that I couldn't find on the site to display individual service toggles. It seems to work without using onConsent to loop through the changes - the GA code is running on every load only when I accept in the modal.
I was wondering if we could do something similar to how iotas.json works to update the services. I'll mark this for review in the meantime. |
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 am very impressed by all the stuff you figured out to do in this server, and all the validation in the consent model.
One problem I see is that when I run dev, and then go to the localhost:3011/join and accept the cookies, nothing shows up on the database. The consent collection isn't there.
I use MongoDB Compass to manually look at what's in the DB.
The other consideration is mentioned in the code comments, we need to make a React Component out of this, that can be included in App in the other repos.
Looking good. Thanks.
@@ -129,6 +117,7 @@ function serverReactRender(App, req, res, next) { | |||
})() | |||
)})</script> | |||
${helmet.script.toString()} | |||
<link rel="stylesheet" href="https://cdn.jsdelivr.net/gh/orestbida/[email protected]/dist/cookieconsent.css"> |
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.
This can be taken out of here done using ReactHelmet in the EnCivCookies component.
I'll get the callbacks connected to the DB logging function next! I did have a question about identifying users - where do we actually get the ID data from like userId, IP address, etc to save? I looked around but had difficulty determining where to retrieve them from within app.jsx's (and soon-to-be encivcookies.js's) scope. |
Its not well documented, but if the user is logged in App will receive the user prop,, with email, id, and tempId
tempId is like an id but the users hasn't left an email address yet. But for consent you could use id || tempId. For the IP address, that's a bit more of a challenge. seems it's hard to get on the client side. But on the server side, this is probably going to make a socket-api call to tell the server to log the consent. For the IP address, app/server/routes/iotas.js creates a browserConfig props that is also passed to App, that has the ip address. It gets it from express In the socketApi function, |
I formatted and tried to save the data but am a bit confused with the actual API call - I was wondering if I'm missing something? I created another file under socket-apis - however the "this" variable only had synuser, whereas socketlogger's "this" had everything including the IP. It also seems I can't use any of the mongo/self-defined functions like findOne() as Consent was just equal to {}. I saw in client-side-model.js there's an explanation and export - Consent had data again once I added to it, but the functions still weren't defined. I also thought calling the save consent API would be performed on the server side but I saw the comment mentions "for code that runs on the client side" - so I'm a bit unclear on that as well. |
The Consent model will not work on the browser side. It will only run on the server side -- in the socket-api directory. clicent-side-models is there to make webpack happy when building the client side code. So there will have to be a an api call from the client side code to the server side. To call an api from the browser side do window.socket.emit('socket-api-file', arg1, arg2, ..., callback) Then in the socket-apis directory, create a file with the corresponding name, with a function to accept the call. I can't see why Let me know if you want a more details. |
I recall trying window.socket.emit() but I couldn't access window, so I switched to saveConsent.call() at one point. I can use emit() now though - maybe it was a "turn it off and on" type situation. The callback I pass doesn't show anything though, nor do the logs, it seems like saveConsent isn't running? Not sure if this warning is related too.
Before I checked the this variable for both APIs with npm run dev, with socketlogger triggered on run and saveconsent triggered when I Accept/Reject All. I noticed socketlogger printed to the VSCode console, while saveConsent logged to the browser's console. I defined saveConsent in the same socket-apis/ folder, so that's why I was confused why it couldn't access the Consent model when I did saveConsent.call(). I thought it was almost as if the code I expected to run server-side was being treated as if it were client-side. I switched back to socket.emit() and am trying to resolve the present not-called issue - but I'd definitely be curious to learn why these occurred! |
If you can't access emit('saveConsent' would need to be 'save-consent' and the file name would need to be save-consent.js different file systems do different things with mixed case names - so we are always using lower case file names, and the emit handle needs to match the first part of the file name. the name of the function inside the file can be whatever. I hope this helps. This is all interesting stuff - server vs client side and in this project some of the code runs on both. |
This diagram might be helpful. I go though it on dev calls sometimes, but probably not recently. https://www.figma.com/design/dYNc6vBmKXY1mZWiSV8qrI/civil-server-render-and-api?node-id=10-73&t=hSwc10R4SUpISqpD-4 I should find a way to get linked to from github. |
Thanks for the insight! This makes sense to me now - I forgot about the server rendering the code first. I finished separating the cookies as a component and added a Helmet with the link, I've just been figuring why helmet.link seems to be empty even after this to finish up. |
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.
This is great. AFter you fix the react-helmet thing I still have more studying to do, and probably rewording some text and stuff. But this is really great!
app/components/enciv-cookies.js
Outdated
@@ -0,0 +1,172 @@ | |||
import React, { useEffect, useState, useRef } from 'react' | |||
import Helmet from 'helmet' |
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.
This should be from 'react-helmet'. there is also a helmet - which is security for node. I bet this is why helmet.link is empty.
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.
Yeah that fixed it. Thanks for catching 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.
This look great. I made one fix - see the inline comment. I'm going to approve this, but not merge the PR yet. I added an issue for myself to review the text in enciv-cookies, and another issue to suppress the popup if funning within an iframe.
I feel the need to proceed carefully with this change because because it crosses a lot of places.
Thanks!
{ | ||
label: 'Google Analytics', | ||
onAccept: () => { | ||
if (window.process.env.GOOGLE_ANALYTICS) { |
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 changed this from process.env to window.process.env because it was getting a different value. I added a an issue to civil-client to fix the code in main to use process.env rather then global.env - but this fixes the immediate problem.
This change adds a mongo-collections model for storing user consent data, handles displaying service options to consent to, and running the code when consented.