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

Dont link to dashboard from footer #6353

Merged

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 6, 2019

We use the get_absolute_url in other parts of the code
where it makes sense to link to the dashboard.
So I'm adding an additional parameter.

Closes #6137

stsewd added 2 commits Nov 6, 2019
Closes readthedocs#6137

We use the `get_absolute_url` in other parts of the code
where it makes sense to link to the dashboard.
So I'm adding an additional parameter.
@stsewd stsewd requested a review from Nov 6, 2019
We were testing with non-build versions.
But in production is more common to hit this with build versions.
@@ -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

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

@stsewd stsewd Nov 7, 2019

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

@stsewd stsewd Nov 7, 2019

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

@ericholscher ericholscher Nov 7, 2019

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

@ericholscher ericholscher Nov 7, 2019

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.

Copy link
Member

@ericholscher ericholscher left a comment

Looks good 👍

@ericholscher ericholscher merged commit 2f97d8b into readthedocs:master Nov 7, 2019
2 checks passed
stsewd added a commit to stsewd/readthedocs.org that referenced this issue Nov 7, 2019
@stsewd stsewd deleted the dont-link-to-dashboard-from-footer branch Nov 11, 2019
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.

2 participants