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

Revert "Revert "Katie/practice cookies"" #45652

Merged

Conversation

KatieShipley
Copy link
Contributor

Reverts #45604

Let's try #44979 again...

@KatieShipley KatieShipley marked this pull request as draft April 5, 2022 17:35
@KatieShipley KatieShipley marked this pull request as ready for review April 7, 2022 22:28
@KatieShipley KatieShipley force-pushed the revert-45604-revert-44979-katie/practice-cookies branch from 0ad6140 to 2141f28 Compare April 12, 2022 21:05
Copy link
Contributor

@hacodeorg hacodeorg left a comment

Choose a reason for hiding this comment

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

A couple of questions since I'm not super familiar with all the gotchas when working with cookies. Overall, the PR looks good to me, however you may want to find other people with more cookie-related experiences to review it.


%link{ :href => "/css/cookies.css", :rel => "stylesheet" }

= render inline: File.read(deploy_dir('shared/partials/cookie_notice.partial')), type: :partial
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious why you use deploy_dir here but Rails.root.join in other places?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deploy_dir finds the root path in pegasus, Rails.root finds the root path in studio. This was a weird gotcha I ran into, Rails.root works fine locally for both pegasus and dashboard because of how we run the dashboard-server locally, but in all other environments pegasus does not have access to the Rails.root.

Copy link
Contributor

Choose a reason for hiding this comment

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

Does deploy_dir work for both Pegasus and Dashboard? IMO, deploy_dir('shared/haml/onetrust_cookie_scripts.haml') is easier to read than
Rails.root.join('..', 'shared', 'haml', 'onetrust_cookie_scripts.haml').
This is just an observation, not a suggestion to change.

@@ -0,0 +1,10 @@
-# OneTrust Cookies Consent Notice scripts for code.org
- if rack_env?(:production)
%script{src: 'https://cdn.cookielaw.org/consent/27cca70a-7db3-4852-9ef0-a6660fd0977d/OtAutoBlock.js', type: 'text/javascript'}
Copy link
Contributor

Choose a reason for hiding this comment

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

This link seems to point to a very specific version of the js file. How do we know when to use a newer version?

Copy link
Contributor

Choose a reason for hiding this comment

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

And the js version in shared/haml/hoc_onetrust_cookie_scripts.haml is different from the one used here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The hash there isn't locked to a version, it just identifies our script for the *.code.org domains. When we publish a new version of the script via OneTrust, that same .js url will be updated to pull the newest version without a code change. If for some reason we do make a change on the OneTrust side that changes the link (like what happened in this commit), OneTrust notifies us that the script link has changed so we know to make the code change.

It is different from hoc_onetrust_cookie_scripts because *.code.org and hourofcode.com are separate domains, so they unfortunately need separate cookie management. The behavior configuration for both on the OneTrust side is identical though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, thanks for the explanation.

@KatieShipley KatieShipley merged commit 2f0ba38 into staging Apr 13, 2022
@KatieShipley KatieShipley deleted the revert-45604-revert-44979-katie/practice-cookies branch April 13, 2022 20:43
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.

None yet

2 participants