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

Don't depend of enabled pdf/epub to show downloads #5502

Merged
merged 4 commits into from Mar 19, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
47 changes: 19 additions & 28 deletions readthedocs/builds/models.py
@@ -1,5 +1,3 @@
# -*- coding: utf-8 -*-

"""Models for the builds app."""

import datetime
Expand Down Expand Up @@ -258,32 +256,25 @@ def get_subdomain_url(self):
def get_downloads(self, pretty=False):
project = self.project
data = {}
if pretty:
Copy link
Member Author

Choose a reason for hiding this comment

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

This code was duplicates, just refactored it to something simpler

if project.has_pdf(self.slug):
data['PDF'] = project.get_production_media_url('pdf', self.slug)
if project.has_htmlzip(self.slug):
data['HTML'] = project.get_production_media_url(
'htmlzip',
self.slug,
)
if project.has_epub(self.slug):
data['Epub'] = project.get_production_media_url(
'epub',
self.slug,
)
else:
if project.has_pdf(self.slug):
data['pdf'] = project.get_production_media_url('pdf', self.slug)
if project.has_htmlzip(self.slug):
data['htmlzip'] = project.get_production_media_url(
'htmlzip',
self.slug,
)
if project.has_epub(self.slug):
data['epub'] = project.get_production_media_url(
'epub',
self.slug,
)

def prettify(k):
return k if pretty else k.lower()

if project.has_pdf(self.slug):
data[prettify('PDF')] = project.get_production_media_url(
'pdf',
self.slug,
)
if project.has_htmlzip(self.slug):
data[prettify('HTML')] = project.get_production_media_url(
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
data[prettify('HTML')] = project.get_production_media_url(
data[prettify('HTMLZIP')] = project.get_production_media_url(

I believe this should be HTMLZIP instead of HTML. The template that displays these links checks for dict.htmlzip.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I just realized that when prettify was set, we named this to html instead of htmlzip. I need to see if we use this in the footer api, or maybe it's safe to rename the template.

Copy link
Member Author

Choose a reason for hiding this comment

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

We currently use the name HTML in the footer api and on the theme https://github.com/rtfd/sphinx_rtd_theme/blob/a49a812c8821123091166fae1897d702cdc2d627/sphinx_rtd_theme/versions.html#L17-L21

So, it's safe to rename this in the template

Copy link
Member Author

Choose a reason for hiding this comment

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

'htmlzip',
self.slug,
)
if project.has_epub(self.slug):
data[prettify('Epub')] = project.get_production_media_url(
'epub',
self.slug,
)
return data

def get_conf_py_path(self):
Expand Down
4 changes: 0 additions & 4 deletions readthedocs/projects/models.py
Expand Up @@ -744,8 +744,6 @@ def has_aliases(self):
return self.aliases.exists()

def has_pdf(self, version_slug=LATEST):
if not self.enable_pdf_build:
return False
path = self.get_production_media_path(
type_='pdf', version_slug=version_slug
)
Expand All @@ -755,8 +753,6 @@ def has_pdf(self, version_slug=LATEST):
return os.path.exists(path) or storage.exists(storage_path)

def has_epub(self, version_slug=LATEST):
if not self.enable_epub_build:
return False
path = self.get_production_media_path(
type_='epub', version_slug=version_slug
)
Expand Down
12 changes: 4 additions & 8 deletions readthedocs/rtd_tests/tests/test_footer.py
Expand Up @@ -60,10 +60,8 @@ def test_pdf_build_mentioned_in_footer(self):
response = self.render()
self.assertIn('pdf', response.data['html'])

def test_pdf_not_mentioned_in_footer_when_build_is_disabled(self):
self.pip.enable_pdf_build = False
self.pip.save()
with fake_paths_by_regex(r'\.pdf$'):
def test_pdf_not_mentioned_in_footer_when_doesnt_exists(self):
with fake_paths_by_regex(r'\.pdf$', exists=False):
response = self.render()
self.assertNotIn('pdf', response.data['html'])

Expand All @@ -72,10 +70,8 @@ def test_epub_build_mentioned_in_footer(self):
response = self.render()
self.assertIn('epub', response.data['html'])

def test_epub_not_mentioned_in_footer_when_build_is_disabled(self):
self.pip.enable_epub_build = False
self.pip.save()
with fake_paths_by_regex(r'\.epub$'):
def test_epub_not_mentioned_in_footer_when_doesnt_exists(self):
with fake_paths_by_regex(r'\.epub$', exists=False):
response = self.render()
self.assertNotIn('epub', response.data['html'])

Expand Down
8 changes: 4 additions & 4 deletions readthedocs/rtd_tests/tests/test_project.py
Expand Up @@ -55,10 +55,10 @@ def test_has_pdf(self):
self.assertFalse(self.pip.has_pdf(LATEST))

def test_has_pdf_with_pdf_build_disabled(self):
# The project has NO pdf if pdf builds are disabled
# The project doesn't depend on `enable_pdf_build`
self.pip.enable_pdf_build = False
with fake_paths_by_regex(r'\.pdf$'):
self.assertFalse(self.pip.has_pdf(LATEST))
self.assertTrue(self.pip.has_pdf(LATEST))

def test_has_epub(self):
# The project has a epub if the PDF file exists on disk.
Expand All @@ -70,10 +70,10 @@ def test_has_epub(self):
self.assertFalse(self.pip.has_epub(LATEST))

def test_has_epub_with_epub_build_disabled(self):
# The project has NO epub if epub builds are disabled
# The project doesn't depend on `enable_epub_build`
self.pip.enable_epub_build = False
with fake_paths_by_regex(r'\.epub$'):
self.assertFalse(self.pip.has_epub(LATEST))
self.assertTrue(self.pip.has_epub(LATEST))

@patch('readthedocs.projects.models.Project.find')
def test_conf_file_found(self, find_method):
Expand Down
4 changes: 2 additions & 2 deletions readthedocs/templates/core/project_downloads.html
@@ -1,7 +1,7 @@
{% load i18n %}

{% for version, dict in version_data.items %}
{% if dict.pdf and version.project.enable_pdf_build %}
{% if dict.pdf %}
<li class="module-item col-span">
<a class="module-item-title" href="{{ dict.pdf }}">
{{ version.slug }} PDF
Expand All @@ -17,7 +17,7 @@
</li>
{% endif %}

{% if dict.epub and version.project.enable_epub_build %}
{% if dict.epub %}
<li class="module-item col-span">
<a class="module-item-title" href="{{ dict.epub }}">
{{ version.slug }} Epub
Expand Down