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

projects: serve badge with same protocol as site #4228

Merged
merged 1 commit into from Jun 18, 2018

Conversation

@xrmx
Copy link
Contributor

@xrmx xrmx commented Jun 12, 2018

Instead of request.is_secure use settings.PUBLIC_DOMAIN_USES_HTTPS
to find out if we should serve the badge under https or not.
That matches what the resolver is doing and works here.

Copy link
Contributor

@davidfischer davidfischer left a comment

I like this change. Essentially if the public domain supports HTTPS then we use it.

@humitos
Copy link
Member

@humitos humitos commented Jun 12, 2018

I like this change. Thanks!

Could you write a test for this, please? There are other similar tests in the test_resolver.py file.

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 12, 2018

@humitos let's make a deal, you hook some coverage to the CI and I'll write tests if the code i am touching is not tested :)

Copy link
Contributor

@agjohnson agjohnson left a comment

Looks like a helpful change! Public and production domain shouldn't be conflated though.

Echoing the need for tests also.

@@ -90,7 +90,8 @@ def get_context_data(self, **kwargs):
user=self.request.user, project=project)

protocol = 'http'
if self.request.is_secure():
use_https = getattr(settings, 'PUBLIC_DOMAIN_USES_HTTPS', False)
Copy link
Contributor

@agjohnson agjohnson Jun 12, 2018

For this purpose, public domain != production domain. Badges are served from our production domain, docs are served from our public domain.

Copy link
Contributor Author

@xrmx xrmx Jun 13, 2018

So what about removing the scheme altogether?

Instead of request.is_secure use a schemaless url for the badge
url in project detail view.
@xrmx xrmx force-pushed the httpsbadgeupstream branch from cdb1372 to 8e85110 Jun 13, 2018
@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jun 13, 2018

I've pushed the schema-less version

@xrmx xrmx changed the title projects: serve badge with same protocol as resolver projects: serve badge with same protocol as site Jun 14, 2018
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jun 18, 2018

This looks like a much better fix. I like protocol relative URL's 👍

@ericholscher ericholscher merged commit 08beab1 into readthedocs:master Jun 18, 2018
1 check passed
@stsewd
Copy link
Member

@stsewd stsewd commented Jul 6, 2018

Without a protocol users can't copy/paste the badge generated code anymore #4337

@xrmx
Copy link
Contributor Author

@xrmx xrmx commented Jul 10, 2018

Feel free to revert then until we found a solution that work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants