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

Fix subdomainless issues #3334 #3393

Merged
merged 6 commits into from
Apr 29, 2015
Merged

Conversation

rafatower
Copy link
Contributor

@Kartones please CR

@javisantana this fixes the cases 1 and 2 that you mention in the ticket. There are still some cases I know do not work (logout redirect for instance)

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@rafatower
Copy link
Contributor Author

cc @santisaez

@rafatower rafatower changed the title Ignore subdomain in subdomainless configurations #3334 [no merge] Ignore subdomain in subdomainless configurations #3334 Apr 28, 2015
@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@rafatower rafatower changed the title [no merge] Ignore subdomain in subdomainless configurations #3334 [no merge] Fix subdomainless issues #3334 Apr 28, 2015
@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@rafatower
Copy link
Contributor Author

@javisantana , @Kartones I think this is ready. Please review.

I tested it locally setting an IP address as the session_domain with different cases: login, logout, org users, shared vizs, pagination, etc. and everything looks fine to me.

I also deployed it to a staging and checked I did not break the "subdomainful cloud config" to my best knowledge.

Once it passes your review I'll tell Santi to run the on-premise regression.

# username.cartodb.com should redirect to the public user dashboard in the maps view if the user is not logged in
else
redirect_to CartoDB.url(self, 'public_maps_home')
# No current_user and no current_viewer so we resort to login page
redirect_to login_url
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't using here CartoDB.url(self, 'login') ?

using *_url and *_path is disencouraged as we cannot control Rails generation of the url, so for domain-based urls would create a /u/xxx/user/xxxx ugly (but working) url if I recall correctly, while our CartoDB.url shouldn't

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just because there's no user in that context, so it was generating stuff like mydomain.lan/user//maps

CartoDB.url(self, 'login') doesn't work either, it generates stuff like http://172.17.0.1:3000/user//login

This is clearly an exception to the rule. If you have a better suggestion...

@Kartones
Copy link
Contributor

Apart from that... looks nice, thanks for fixing the issues! 👍

@Cartofante
Copy link
Collaborator

Frontend tests were OK 👍 (details)

@rafatower
Copy link
Contributor Author

@santisaez I think you can generate a build or whatever to test that in an on-premise. I can assist you with it.

@rafatower rafatower changed the title [no merge] Fix subdomainless issues #3334 Fix subdomainless issues #3334 Apr 28, 2015
rafatower pushed a commit that referenced this pull request Apr 29, 2015
@rafatower rafatower merged commit 3b6c7e5 into master Apr 29, 2015
@rafatower rafatower deleted the 3334-fix-subdomainless-issues branch April 29, 2015 09:01
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.

3 participants