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

Choose a reason for hiding this comment

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

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

@humitos
Copy link
Member

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 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 :)

@agjohnson agjohnson added the Needed: tests Tests are required label Jun 12, 2018
Copy link
Contributor

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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 Compare June 13, 2018 08:08
@xrmx
Copy link
Contributor Author

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

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

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

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 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
Labels
Needed: tests Tests are required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants