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

Rails cookie writing settings - domains/subdomains, URL-encoding #565

Closed
anselmbradford opened this issue Aug 28, 2014 · 2 comments · Fixed by #593
Closed

Rails cookie writing settings - domains/subdomains, URL-encoding #565

anselmbradford opened this issue Aug 28, 2014 · 2 comments · Fixed by #593

Comments

@anselmbradford
Copy link
Member

A few questions:

  • Can and should the domain in https://github.com/codeforamerica/ohana-web-search/blob/master/app/controllers/concerns/google_translator.rb#L13 use :all like is done in the cookie delete method? I thought I remember reading somewhere that the domain on the cookie creation and deletion should be the same. It appears to work the way it is, but using :all would eliminate the environment variable call.

  • Google Translate adds two cookies. Running the app locally these are on the domains .lvh.me and lvh.me. The Rails code in https://github.com/codeforamerica/ohana-web-search/blob/master/app/controllers/concerns/google_translator.rb#L11-L14 only overwrites one of these. The current method:

    cookies[:googtrans] = {
      value: "/en/#{params[:translate]}",
      domain: "#{ENV['DOMAIN_NAME']}"
    }

    ...can be interchanged with...

    cookies[:googtrans] = "/en/#{params[:translate]}"

    ...to selectively overwrite the cookie at lvh.me and .lvh.me. However, I can't figure out how to use both these methods of writing cookies at the same time. When they are both present, one gets ignored. We're overwriting the cookies on the client, but it would be preferable if they were overwritten on the server.

  • Google Translate's cookies don't have URL-encoded values, whereas the ones Rails writes are, so /en/es becomes %2Fen%2Fes. This seems okay, because once the page reloads the Google Translate Widget overwrites these with the non-URL-encoded values, but if it is easy to prevent Rails from URL-encoding these values it may be worth doing so that the output values are the same as Google's.

@monfresh
Copy link
Member

I don't know the answers off the top of my head, but you could answer them by testing the app with different settings for domain, and with only using server-based cookie deletion, or only client-based, or both. Did you try setting domain to :all to see if it still works?

@anselmbradford
Copy link
Member Author

My preliminary test on lvh.me had :all working the same as "#{ENV['DOMAIN_NAME']}", but both don't hit both .lvh.me and lvh.me, which Google does with their script. It would still need to be tested on a heroku deploy as well.

anselmbradford added a commit that referenced this issue Sep 8, 2014
- Removes unused util/util methods.
- Moves cookie code to its own CRUD module.
- Fixes #565 - Removes server-side handling of Google Translate cookie
and consolidates cookie handling in google-translate-manager.
- Moves isTranslated method to google-translate-manager.
- Updates “close” button method to “clear” to go with new naming.
- Pads header popup title text so (x) button doesn’t risk overlapping.
- Gives alert close button min height/width so that size doesn’t change
as (x) icon is loading.
- Creates IE8-specific override stylesheet and pulls :before and :after
selectors out of main stylesheet into this one. Also provides overrides
to elements that have box-shadows but no border by providing a border
instead (alert and popup).
anselmbradford added a commit that referenced this issue Sep 8, 2014
- Removes unused util/util methods.
- Moves cookie code to its own CRUD module.
- Fixes #565 - Removes server-side handling of Google Translate cookie
and consolidates cookie handling in google-translate-manager.
- Moves isTranslated method to google-translate-manager.
- Updates “close” button method to “clear” to go with new naming.
- Pads header popup title text so (x) button doesn’t risk overlapping.
- Gives alert close button min height/width so that size doesn’t change
as (x) icon is loading.
- Implements addEventListener on alert so events can be attached to it
being dismissed.
- Creates IE8-specific override stylesheet and pulls :before and :after
selectors out of main stylesheet into this one. Also provides overrides
to elements that have box-shadows but no border by providing a border
instead (alert and popup).
-
anselmbradford added a commit that referenced this issue Sep 8, 2014
- Removes unused util/util methods.
- Moves cookie code to its own CRUD module.
- Fixes #565 - Removes server-side handling of Google Translate cookie
and consolidates cookie handling in google-translate-manager.
- Moves isTranslated method to google-translate-manager.
- Updates “close” button method to “clear” to go with new naming.
- Pads header popup title text so (x) button doesn’t risk overlapping.
- Gives alert close button min height/width so that size doesn’t change
as (x) icon is loading.
- Implements addEventListener on alert so events can be attached to it
being dismissed.
- Creates IE8-specific override stylesheet and pulls :before and :after
selectors out of main stylesheet into this one. Also provides overrides
to elements that have box-shadows but no border by providing a border
instead (alert and popup).
-
anselmbradford added a commit that referenced this issue Sep 8, 2014
- Removes unused util/util methods.
- Moves cookie code to its own CRUD module.
- Fixes #565 - Removes server-side handling of Google Translate cookie
and consolidates cookie handling in google-translate-manager.
- Moves isTranslated method to google-translate-manager.
- Updates “close” button method to “clear” to go with new naming.
- Pads header popup title text so (x) button doesn’t risk overlapping.
- Gives alert close button min height/width so that size doesn’t change
as (x) icon is loading.
- Implements addEventListener on alert so events can be attached to it
being dismissed.
- Creates IE8-specific override stylesheet and pulls :before and :after
selectors out of main stylesheet into this one. Also provides overrides
to elements that have box-shadows but no border by providing a border
instead (alert and popup).
-
anselmbradford added a commit that referenced this issue Sep 8, 2014
- Removes unused util/util methods.
- Moves cookie code to its own CRUD module.
- Fixes #565 - Removes server-side handling of Google Translate cookie
and consolidates cookie handling in google-translate-manager.
- Moves isTranslated method to google-translate-manager.
- Updates “close” button method to “clear” to go with new naming.
- Pads header popup title text so (x) button doesn’t risk overlapping.
- Gives alert close button min height/width so that size doesn’t change
as (x) icon is loading.
- Implements addEventListener on alert so events can be attached to it
being dismissed.
- Fixes #573 - Creates IE8-specific override stylesheet and pulls :before and :after
selectors out of main stylesheet into this one. Also provides overrides
to elements that have box-shadows but no border by providing a border
instead (alert and popup).
- Updates modernizr custom build to include html5-shiv and include all core features used on the site.
- Removes gsub from requirejs include tag.
anselmbradford added a commit that referenced this issue Sep 8, 2014
- Removes unused util/util methods.
- Moves cookie code to its own CRUD module.
- Fixes #565 - Removes server-side handling of Google Translate cookie
and consolidates cookie handling in google-translate-manager.
- Moves isTranslated method to google-translate-manager.
- Updates “close” button method to “clear” to go with new naming.
- Pads header popup title text so (x) button doesn’t risk overlapping.
- Gives alert close button min height/width so that size doesn’t change
as (x) icon is loading.
- Implements addEventListener on alert so events can be attached to it
being dismissed.
- Fixes #573 - Creates IE8-specific override stylesheet and pulls :before and :after
selectors out of main stylesheet into this one. Also provides overrides
to elements that have box-shadows but no border by providing a border
instead (alert and popup).
- Updates modernizr custom build to include html5-shiv and include all core features used on the site.
- Removes gsub from requirejs include tag.
anselmbradford added a commit that referenced this issue Sep 9, 2014
Fixes #565 - consolidates translation code.
anselmbradford added a commit that referenced this issue Sep 9, 2014
Fixes #565 - consolidates translation code.
anselmbradford added a commit that referenced this issue Sep 9, 2014
Fixes #565 - consolidates translation code.
anselmbradford added a commit that referenced this issue Sep 9, 2014
Fixes #565 - consolidates translation code.
anselmbradford added a commit that referenced this issue Sep 9, 2014
Fixes #565 - consolidates translation code.
anselmbradford added a commit that referenced this issue Sep 9, 2014
Fixes #565 - consolidates translation code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants