-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Analytics: move page views to its own endpoint #7739
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
"""Analytics views that are served from the same domain as the docs.""" | ||
|
||
from django.db.models import F | ||
from django.shortcuts import get_object_or_404 | ||
from django.utils import timezone | ||
from rest_framework.response import Response | ||
from rest_framework.views import APIView | ||
|
||
from readthedocs.analytics.models import PageView | ||
from readthedocs.api.v2.permissions import IsAuthorizedToViewVersion | ||
from readthedocs.core.unresolver import unresolve_from_request | ||
from readthedocs.core.utils.extend import SettingsOverrideObject | ||
from readthedocs.projects.models import Feature, Project | ||
|
||
|
||
class BaseAnalyticsView(APIView): | ||
|
||
""" | ||
Track page views. | ||
|
||
Query parameters: | ||
|
||
- project | ||
- version | ||
- absolute_uri: Full path with domain. | ||
""" | ||
|
||
http_method_names = ['get'] | ||
permission_classes = [IsAuthorizedToViewVersion] | ||
|
||
def _get_project(self): | ||
cache_key = '__cached_project' | ||
project = getattr(self, cache_key, None) | ||
|
||
if not project: | ||
project_slug = self.request.GET.get('project') | ||
project = get_object_or_404(Project, slug=project_slug) | ||
setattr(self, cache_key, project) | ||
|
||
return project | ||
|
||
def _get_version(self): | ||
cache_key = '__cached_version' | ||
version = getattr(self, cache_key, None) | ||
humitos marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
if not version: | ||
version_slug = self.request.GET.get('version') | ||
project = self._get_project() | ||
version = get_object_or_404( | ||
project.versions.all(), | ||
slug=version_slug, | ||
) | ||
setattr(self, cache_key, version) | ||
|
||
return version | ||
|
||
# pylint: disable=unused-argument | ||
def get(self, request, *args, **kwargs): | ||
project = self._get_project() | ||
version = self._get_version() | ||
absolute_uri = self.request.GET.get('absolute_uri') | ||
self.increase_page_view_count( | ||
request=request, | ||
project=project, | ||
version=version, | ||
absolute_uri=absolute_uri, | ||
) | ||
return Response(status=200) | ||
|
||
# pylint: disable=no-self-use | ||
def increase_page_view_count(self, request, project, version, absolute_uri): | ||
"""Increase the page view count for the given project.""" | ||
if not absolute_uri or not project.has_feature(Feature.STORE_PAGEVIEWS): | ||
return | ||
|
||
unresolved = unresolve_from_request(request, absolute_uri) | ||
if not unresolved: | ||
return | ||
|
||
path = unresolved.filename | ||
|
||
fields = dict( | ||
project=project, | ||
version=version, | ||
path=path, | ||
date=timezone.now().date(), | ||
) | ||
page_view = PageView.objects.filter(**fields).first() | ||
if page_view: | ||
page_view.view_count = F('view_count') + 1 | ||
page_view.save(update_fields=['view_count']) | ||
else: | ||
PageView.objects.create(**fields, view_count=1) | ||
|
||
|
||
class AnalyticsView(SettingsOverrideObject): | ||
|
||
_default_class = BaseAnalyticsView |
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,12 +11,11 @@ | |
from rest_framework_jsonp.renderers import JSONPRenderer | ||
|
||
from readthedocs.api.v2.permissions import IsAuthorizedToViewVersion | ||
from readthedocs.api.v2.signals import footer_response | ||
from readthedocs.builds.constants import LATEST, TAG | ||
from readthedocs.builds.models import Version | ||
from readthedocs.core.utils.extend import SettingsOverrideObject | ||
from readthedocs.projects.constants import MKDOCS, SPHINX_HTMLDIR | ||
from readthedocs.projects.models import Feature, Project | ||
from readthedocs.projects.models import Project | ||
from readthedocs.projects.version_handling import ( | ||
highest_version, | ||
parse_version_failsafe, | ||
|
@@ -243,16 +242,6 @@ def get(self, request, format=None): | |
'version_supported': version.supported, | ||
} | ||
|
||
# Allow folks to hook onto the footer response for various information | ||
# collection, or to modify the resp_data. | ||
footer_response.send( | ||
sender=None, | ||
request=request, | ||
context=context, | ||
response_data=resp_data, | ||
absolute_uri=self.request.GET.get('absolute_uri'), | ||
) | ||
|
||
Comment on lines
-246
to
-255
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we keep sending this signal here? We were only using it for pageviews here, but it could be useful for other things as well. Besides, it may have an impact downstream. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't think this is useful, it also hides additional queries. The class can be extended already with @readthedocs/core thoughts? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My only worry is about custom installs that might be using it, but I don't expect it to be heavily used there. We plan to cache it in prod, but not all users will necessarily do that. I think I'm OK removing it, and we can re-add it if someone yells :) |
||
return Response(resp_data) | ||
|
||
|
||
|
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -427,7 +427,7 @@ def test_highest_version_without_tags(self): | |
class TestFooterPerformance(TestCase): | ||
# The expected number of queries for generating the footer | ||
# This shouldn't increase unless we modify the footer API | ||
EXPECTED_QUERIES = 18 | ||
EXPECTED_QUERIES = 14 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❤️ |
||
|
||
def setUp(self): | ||
self.pip = get( | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have this code in a few places now, we should probably move it to a mixin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do another PR with that