Skip to content

Dont link to dashboard from footer #6353

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Nov 7, 2019
Merged

Dont link to dashboard from footer #6353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cabe0c9
Never link to the dashboard from the footer
stsewd Nov 6, 2019
2b4fddb
Fix test
stsewd Nov 6, 2019
e90ea84
Fix tests
stsewd Nov 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion readthedocs/api/v2/views/footer_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,9 @@ def get_version_compare_data(project, base_version=None):
'is_highest': True,
}
if highest_version_obj:
ret_val['url'] = highest_version_obj.get_absolute_url()
# Never link to the dashboard,
# users reading the docs may don't have access to the dashboard.
ret_val['url'] = highest_version_obj.get_absolute_url(link_to_dashboard=False)
ret_val['slug'] = highest_version_obj.slug
if base_version and base_version.slug != LATEST:
try:
Expand Down
11 changes: 8 additions & 3 deletions readthedocs/builds/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,6 @@
from readthedocs.projects.models import APIProject, Project
from readthedocs.projects.version_handling import determine_stable_version


log = logging.getLogger(__name__)


Expand Down Expand Up @@ -289,7 +288,13 @@ def commit_name(self):
)
return self.identifier

def get_absolute_url(self):
def get_absolute_url(self, link_to_dashboard=True):
"""
Get absolute url to the docs of the version.

:param link_to_dashboard: If `False` we never try to link to the dashboard,
we link to the docs even if they result in a 404.
"""
# Hack external versions for now.
# TODO: We can integrate them into the resolver
# but this is much simpler to handle since we only link them a couple places for now
Expand All @@ -299,7 +304,7 @@ def get_absolute_url(self):
f'{self.project.slug}/{self.slug}/index.html'
return url

if not self.built and not self.uploaded:
if not self.built and not self.uploaded and link_to_dashboard:
return reverse(
'project_version_detail',
kwargs={
Expand Down
22 changes: 14 additions & 8 deletions readthedocs/rtd_tests/tests/test_footer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import mock
from django.contrib.sessions.backends.base import SessionBase
from django.test import TestCase
from django.test.utils import override_settings
from rest_framework.test import APIRequestFactory, APITestCase

from readthedocs.api.v2.views.footer_views import (
Expand Down Expand Up @@ -114,6 +115,10 @@ def test_not_show_edit_on_github(self):
self.assertNotIn('Edit', response.data['html'])


@override_settings(
USE_SUBDOMAIN=True,
PUBLIC_DOMAIN='readthedocs.io',
)
class TestVersionCompareFooter(TestCase):
fixtures = ['test_data']

Expand All @@ -124,7 +129,7 @@ def test_highest_version_from_stable(self):
base_version = self.pip.get_stable_version()
valid_data = {
'project': 'Version 0.8.1 of Pip (19)',
'url': '/dashboard/pip/version/0.8.1/',
'url': 'http://pip.readthedocs.io/en/0.8.1/',
'slug': '0.8.1',
'version': '0.8.1',
'is_highest': True,
Expand All @@ -136,7 +141,7 @@ def test_highest_version_from_lower(self):
base_version = self.pip.versions.get(slug='0.8')
valid_data = {
'project': 'Version 0.8.1 of Pip (19)',
'url': '/dashboard/pip/version/0.8.1/',
'url': 'http://pip.readthedocs.io/en/0.8.1/',
'slug': '0.8.1',
'version': '0.8.1',
'is_highest': False,
Expand All @@ -149,7 +154,7 @@ def test_highest_version_from_latest(self):
base_version = self.pip.versions.get(slug=LATEST)
valid_data = {
'project': 'Version 0.8.1 of Pip (19)',
'url': '/dashboard/pip/version/0.8.1/',
'url': 'http://pip.readthedocs.io/en/0.8.1/',
'slug': '0.8.1',
'version': '0.8.1',
'is_highest': True,
Expand Down Expand Up @@ -177,7 +182,7 @@ def test_highest_version_over_branches(self):
base_version = self.pip.versions.get(slug='0.8.1')
valid_data = {
'project': 'Version 1.0.0 of Pip ({})'.format(version.pk),
'url': '/dashboard/pip/version/1.0.0/',
'url': 'http://pip.readthedocs.io/en/1.0.0/',
'slug': '1.0.0',
'version': '1.0.0',
'is_highest': False,
Expand All @@ -191,7 +196,7 @@ def test_highest_version_without_tags(self):
base_version = self.pip.versions.get(slug='0.8.1')
valid_data = {
'project': 'Version 0.8.1 of Pip (19)',
'url': '/dashboard/pip/version/0.8.1/',
'url': 'http://pip.readthedocs.io/en/0.8.1/',
'slug': '0.8.1',
'version': '0.8.1',
'is_highest': True,
Expand All @@ -202,7 +207,7 @@ def test_highest_version_without_tags(self):
base_version = self.pip.versions.get(slug='0.8')
valid_data = {
'project': 'Version 0.8.1 of Pip (19)',
'url': '/dashboard/pip/version/0.8.1/',
'url': 'http://pip.readthedocs.io/en/0.8.1/',
'slug': '0.8.1',
'version': '0.8.1',
'is_highest': False,
Expand All @@ -219,7 +224,7 @@ def test_highest_version_without_tags(self):
)
valid_data = {
'project': 'Version 2.0.0 of Pip ({})'.format(version.pk),
'url': '/dashboard/pip/version/2.0.0/',
'url': 'http://pip.readthedocs.io/en/2.0.0/',
'slug': '2.0.0',
'version': '2.0.0',
'is_highest': False,
Expand All @@ -235,11 +240,12 @@ class TestFooterPerformance(APITestCase):

# The expected number of queries for generating the footer
# This shouldn't increase unless we modify the footer API
EXPECTED_QUERIES = 9
EXPECTED_QUERIES = 13
Copy link
Member

@ericholscher ericholscher Nov 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm, this is less than ideal. Is this some number of additional queries for each version?

Copy link
Member Author

Choose a reason for hiding this comment

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

The explanation is the commit

We were testing with non-build versions.
But in production is more common to hit this with build versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

That number is the "normal" number of queries, we were testing for a no common case where the version wasn't built

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see, it's in the version_compare data. I wonder if we should be caching this, seems simple enough to cache if it's a lot of our footer queries.

Copy link
Member

Choose a reason for hiding this comment

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

I will never look at commit messages in PR's, so it's probably better to put it in the PR description.


def setUp(self):
self.pip = Project.objects.get(slug='pip')
self.pip.versions.create_latest()
self.pip.versions.update(built=True)

def render(self):
request = self.factory.get(self.url)
Expand Down