-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Unify feature check for organization/project #8920
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 18 commits
f29401f
8e339c0
b58e282
274a377
277a513
5ad812e
60f0016
7dae9b4
248b567
8911ded
330e1f6
cfa0c66
bf4199b
33c4a30
d011326
6d9f144
9d23f79
444c65e
ff12df7
bff4ab4
652c62c
39581af
21473bc
65b33df
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 |
---|---|---|
@@ -1,5 +1,4 @@ | ||
"""Project model QuerySet classes.""" | ||
from django.conf import settings | ||
from django.db import models | ||
from django.db.models import Count, OuterRef, Prefetch, Q, Subquery | ||
|
||
|
@@ -96,6 +95,7 @@ def max_concurrent_builds(self, project): | |
|
||
- project | ||
- organization | ||
- plan | ||
- default setting | ||
|
||
:param project: project to be checked | ||
|
@@ -104,15 +104,21 @@ def max_concurrent_builds(self, project): | |
:returns: number of max concurrent builds for the project | ||
:rtype: int | ||
""" | ||
from readthedocs.subscriptions.constants import TYPE_CONCURRENT_BUILDS | ||
from readthedocs.subscriptions.models import PlanFeature | ||
|
||
max_concurrent_organization = None | ||
organization = project.organizations.first() | ||
if organization: | ||
max_concurrent_organization = organization.max_concurrent_builds | ||
|
||
return ( | ||
project.max_concurrent_builds or | ||
max_concurrent_organization or | ||
settings.RTD_MAX_CONCURRENT_BUILDS | ||
project.max_concurrent_builds | ||
or max_concurrent_organization | ||
or PlanFeature.objects.get_feature_value( | ||
project, | ||
type=TYPE_CONCURRENT_BUILDS, | ||
) | ||
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. we had an override just to check for the value from the plan on .com. 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 this method contain all the logic? I mean, also check for Otherwise, it's only being called for the default value, but it won't affect the project's value. 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. It seems my suggestion is already applied, right? |
||
) | ||
|
||
def prefetch_latest_build(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,7 +39,6 @@ | |
) | ||
from readthedocs.core.history import UpdateChangeReasonPostView | ||
from readthedocs.core.mixins import ListViewWithForm, PrivateViewMixin | ||
from readthedocs.core.utils.extend import SettingsOverrideObject | ||
from readthedocs.integrations.models import HttpExchange, Integration | ||
from readthedocs.invitations.models import Invitation | ||
from readthedocs.oauth.services import registry | ||
|
@@ -80,6 +79,12 @@ | |
ProjectRelationListMixin, | ||
) | ||
from readthedocs.search.models import SearchQuery | ||
from readthedocs.subscriptions.constants import ( | ||
TYPE_CNAME, | ||
TYPE_PAGEVIEW_ANALYTICS, | ||
TYPE_SEARCH_ANALYTICS, | ||
) | ||
from readthedocs.subscriptions.models import PlanFeature | ||
|
||
log = structlog.get_logger(__name__) | ||
|
||
|
@@ -752,6 +757,7 @@ class DomainMixin(ProjectAdminMixin, PrivateViewMixin): | |
model = Domain | ||
form_class = DomainForm | ||
lookup_url_kwarg = 'domain_pk' | ||
feature_type = TYPE_CNAME | ||
|
||
def get_success_url(self): | ||
return reverse('projects_domains', args=[self.get_project().slug]) | ||
|
@@ -763,11 +769,13 @@ def get_context_data(self, **kwargs): | |
return context | ||
|
||
def _is_enabled(self, project): | ||
"""Should we allow custom domains for this project?""" | ||
return True | ||
return PlanFeature.objects.has_feature( | ||
project, | ||
type=self.feature_type, | ||
) | ||
|
||
|
||
class DomainListBase(DomainMixin, ListViewWithForm): | ||
class DomainList(DomainMixin, ListViewWithForm): | ||
|
||
def get_context_data(self, **kwargs): | ||
ctx = super().get_context_data(**kwargs) | ||
|
@@ -782,12 +790,7 @@ def get_context_data(self, **kwargs): | |
return ctx | ||
|
||
|
||
class DomainList(SettingsOverrideObject): | ||
|
||
_default_class = DomainListBase | ||
|
||
|
||
class DomainCreateBase(DomainMixin, CreateView): | ||
class DomainCreate(DomainMixin, CreateView): | ||
|
||
def post(self, request, *args, **kwargs): | ||
project = self.get_project() | ||
|
@@ -806,12 +809,7 @@ def get_success_url(self): | |
) | ||
|
||
|
||
class DomainCreate(SettingsOverrideObject): | ||
|
||
_default_class = DomainCreateBase | ||
|
||
|
||
class DomainUpdateBase(DomainMixin, UpdateView): | ||
class DomainUpdate(DomainMixin, UpdateView): | ||
|
||
def form_valid(self, form): | ||
response = super().form_valid(form) | ||
|
@@ -825,11 +823,6 @@ def post(self, request, *args, **kwargs): | |
return HttpResponse('Action not allowed', status=401) | ||
|
||
|
||
class DomainUpdate(SettingsOverrideObject): | ||
|
||
_default_class = DomainUpdateBase | ||
|
||
|
||
class DomainDelete(DomainMixin, DeleteView): | ||
|
||
pass | ||
|
@@ -1072,10 +1065,11 @@ class RegexAutomationRuleUpdate(RegexAutomationRuleMixin, UpdateView): | |
pass | ||
|
||
|
||
class SearchAnalyticsBase(ProjectAdminMixin, PrivateViewMixin, TemplateView): | ||
class SearchAnalytics(ProjectAdminMixin, PrivateViewMixin, TemplateView): | ||
|
||
template_name = 'projects/projects_search_analytics.html' | ||
http_method_names = ['get'] | ||
feature_type = TYPE_SEARCH_ANALYTICS | ||
|
||
def get(self, request, *args, **kwargs): | ||
download_data = request.GET.get('download', False) | ||
|
@@ -1159,21 +1153,24 @@ def _get_csv_data(self): | |
|
||
def _get_retention_days_limit(self, project): | ||
"""From how many days we need to show data for this project?""" | ||
return settings.RTD_ANALYTICS_DEFAULT_RETENTION_DAYS | ||
return PlanFeature.objects.get_feature_value( | ||
project, | ||
type=self.feature_type, | ||
) | ||
|
||
def _is_enabled(self, project): | ||
"""Should we show search analytics for this project?""" | ||
return True | ||
|
||
|
||
class SearchAnalytics(SettingsOverrideObject): | ||
_default_class = SearchAnalyticsBase | ||
return PlanFeature.objects.has_feature( | ||
project, | ||
type=self.feature_type, | ||
) | ||
Comment on lines
1161
to
+1166
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. This method should go in a mixin since it's shared among multiple classes. |
||
|
||
|
||
class TrafficAnalyticsViewBase(ProjectAdminMixin, PrivateViewMixin, TemplateView): | ||
class TrafficAnalyticsView(ProjectAdminMixin, PrivateViewMixin, TemplateView): | ||
|
||
template_name = 'projects/project_traffic_analytics.html' | ||
http_method_names = ['get'] | ||
feature_type = TYPE_PAGEVIEW_ANALYTICS | ||
|
||
def get(self, request, *args, **kwargs): | ||
download_data = request.GET.get('download', False) | ||
|
@@ -1259,12 +1256,14 @@ def _get_csv_data(self): | |
|
||
def _get_retention_days_limit(self, project): | ||
"""From how many days we need to show data for this project?""" | ||
return settings.RTD_ANALYTICS_DEFAULT_RETENTION_DAYS | ||
return PlanFeature.objects.get_feature_value( | ||
project, | ||
type=self.feature_type, | ||
) | ||
|
||
def _is_enabled(self, project): | ||
"""Should we show traffic analytics for this project?""" | ||
return True | ||
|
||
|
||
class TrafficAnalyticsView(SettingsOverrideObject): | ||
_default_class = TrafficAnalyticsViewBase | ||
return PlanFeature.objects.has_feature( | ||
project, | ||
type=self.feature_type, | ||
) |
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.
This is the same method as
_is_enabled
. However, it usesorganization=
instead ofproject=
and it's named differently. We should decide whether naming it_is_feature_enabled
or_is_enabled
to keep consistency.