Skip to content

Add feature flipping models to Projects, plus example feature flip #3194

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

Merged
merged 8 commits into from
Oct 31, 2017
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions readthedocs/builds/migrations/0004_add-apiversion-proxy-model.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.9.12 on 2017-10-27 00:17
from __future__ import unicode_literals

from django.db import migrations


class Migration(migrations.Migration):

dependencies = [
('builds', '0003_add-cold-storage'),
]

operations = [
migrations.CreateModel(
name='APIVersion',
fields=[
],
options={
'proxy': True,
},
bases=('builds.version',),
),
]
36 changes: 35 additions & 1 deletion readthedocs/builds/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
from readthedocs.projects.constants import (PRIVACY_CHOICES, GITHUB_URL,
GITHUB_REGEXS, BITBUCKET_URL,
BITBUCKET_REGEXS, PRIVATE)
from readthedocs.projects.models import Project
from readthedocs.projects.models import Project, APIProject


DEFAULT_VERSION_PRIVACY_LEVEL = getattr(settings, 'DEFAULT_VERSION_PRIVACY_LEVEL', 'public')
Expand Down Expand Up @@ -301,6 +301,40 @@ def get_bitbucket_url(self, docroot, filename, source_suffix='.rst'):
)


class APIVersion(Version):

"""Version proxy model for API data deserialization

This replaces the pattern where API data was deserialized into a mocked
:py:cls:`Version` object. This pattern was confusing, as it was not explicit
as to what form of object you were working with -- API backed or database
backed.

This model preserves the Version model methods, allowing for overrides on
model field differences. This model pattern will generally only be used on
builder instances, where we are interacting solely with API data.
"""

project = None

class Meta:
proxy = True

def __init__(self, *args, **kwargs):
self.project = APIProject(**kwargs.pop('project', {}))
# These fields only exist on the API return, not on the model, so we'll
# remove them to avoid throwing exceptions due to unexpected fields
for key in ['resource_uri', 'absolute_url', 'downloads']:
try:
del kwargs[key]
except KeyError:
pass
super(APIVersion, self).__init__(*args, **kwargs)

def save(self, *args, **kwargs):
return 0


@python_2_unicode_compatible
class VersionAlias(models.Model):

Expand Down
6 changes: 4 additions & 2 deletions readthedocs/core/management/commands/update_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,10 @@
import logging

from django.core.management.base import BaseCommand
from readthedocs.projects import tasks

from readthedocs.api.client import api
from readthedocs.projects import tasks
from readthedocs.projects.models import APIProject


log = logging.getLogger(__name__)
Expand All @@ -29,6 +31,6 @@ def handle(self, *args, **options):
docker = options.get('docker', False)
for slug in options['projects']:
project_data = api.project(slug).get()
p = tasks.make_api_project(project_data)
p = APIProject(**project_data)
log.info("Building %s", p)
tasks.update_docs.run(pk=p.pk, docker=docker)
13 changes: 11 additions & 2 deletions readthedocs/doc_builder/python_environments.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
from readthedocs.doc_builder.config import ConfigWrapper
from readthedocs.doc_builder.loader import get_builder_class
from readthedocs.projects.constants import LOG_TEMPLATE
from readthedocs.projects.models import Feature

log = logging.getLogger(__name__)

Expand Down Expand Up @@ -128,8 +129,16 @@ def install_core_requirements(self):
if self.project.documentation_type == 'mkdocs':
requirements.append('mkdocs==0.15.0')
else:
requirements.extend(['sphinx==1.5.3', 'sphinx-rtd-theme<0.3',
'readthedocs-sphinx-ext<0.6'])
if self.project.has_feature(Feature.USE_SPHINX_LATEST):
# We will assume semver here and only automate up to the next
# backward incompatible release: 2.x
requirements.append('sphinx<2')
else:
requirements.append('sphinx==1.5.6')
requirements.extend([
'sphinx-rtd-theme<0.3',
'readthedocs-sphinx-ext<0.6'
])

cmd = [
'python',
Expand Down
18 changes: 16 additions & 2 deletions readthedocs/projects/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@
from readthedocs.redirects.models import Redirect
from readthedocs.notifications.views import SendNotificationView

from .notifications import ResourceUsageNotification
from .models import (Project, ImportedFile,
from .forms import FeatureForm
from .models import (Project, ImportedFile, Feature,
ProjectRelationship, EmailHook, WebHook, Domain)
from .notifications import ResourceUsageNotification
from .tasks import remove_dir


Expand Down Expand Up @@ -180,8 +181,21 @@ class DomainAdmin(admin.ModelAdmin):
model = Domain


class FeatureAdmin(admin.ModelAdmin):
model = Feature
form = FeatureForm
list_display = ('feature_id', 'project_count', 'default_true')
search_fields = ('feature_id',)
filter_horizontal = ('projects',)
readonly_fields = ('add_date',)

def project_count(self, feature):
return feature.projects.count()


admin.site.register(Project, ProjectAdmin)
admin.site.register(ImportedFile, ImportedFileAdmin)
admin.site.register(Domain, DomainAdmin)
admin.site.register(Feature, FeatureAdmin)
admin.site.register(EmailHook)
admin.site.register(WebHook)
22 changes: 21 additions & 1 deletion readthedocs/projects/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
from readthedocs.projects import constants
from readthedocs.projects.exceptions import ProjectSpamError
from readthedocs.projects.models import (
Project, ProjectRelationship, EmailHook, WebHook, Domain)
Project, ProjectRelationship, EmailHook, WebHook, Domain, Feature)
from readthedocs.redirects.models import Redirect


Expand Down Expand Up @@ -595,3 +595,23 @@ class Meta(object):
def __init__(self, *args, **kwargs):
self.project = kwargs.pop('project', None)
super(ProjectAdvertisingForm, self).__init__(*args, **kwargs)


class FeatureForm(forms.ModelForm):

"""Project feature form for dynamic admin choices

This form converts the CharField into a ChoiceField on display. The
underlying driver won't attempt to do validation on the choices, and so we
can dynamically populate this list.
"""

feature = forms.ChoiceField()

class Meta(object):
model = Feature
fields = ['projects', 'feature', 'default_true']

def __init__(self, *args, **kwargs):
super(FeatureForm, self).__init__(*args, **kwargs)
self.fields['feature'].choices = Feature.FEATURES
29 changes: 29 additions & 0 deletions readthedocs/projects/migrations/0019_add-features.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.9.12 on 2017-10-27 12:55
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('projects', '0018_fix-translation-model'),
]

operations = [
migrations.CreateModel(
name='Feature',
fields=[
('id', models.AutoField(auto_created=True, primary_key=True, serialize=False, verbose_name='ID')),
('feature_id', models.CharField(max_length=32, unique=True, verbose_name='Feature identifier')),
('add_date', models.DateTimeField(auto_now_add=True, verbose_name='Date feature was added')),
('default_true', models.BooleanField(default=False, verbose_name='Historical default is True')),
],
),
migrations.AddField(
model_name='feature',
name='projects',
field=models.ManyToManyField(blank=True, to='projects.Project'),
),
]
24 changes: 24 additions & 0 deletions readthedocs/projects/migrations/0020_add-api-project-proxy.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.9.12 on 2017-10-27 12:56
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('projects', '0019_add-features'),
]

operations = [
migrations.CreateModel(
name='APIProject',
fields=[
],
options={
'proxy': True,
},
bases=('projects.project',),
),
Copy link
Member

Choose a reason for hiding this comment

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

Curious that Django makes a migration for a proxy model. I assume it doesn't do anything to the DB?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, i was surprised by this too at first, i didn't realize this when doing the integrations proxy models.

]
118 changes: 115 additions & 3 deletions readthedocs/projects/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@
from readthedocs.projects.querysets import (
ProjectQuerySet,
RelatedProjectQuerySet,
ChildRelatedProjectQuerySet
ChildRelatedProjectQuerySet,
FeatureQuerySet,
)
from readthedocs.projects.templatetags.projects_tags import sort_version_aware
from readthedocs.projects.utils import make_api_version
from readthedocs.projects.version_handling import determine_stable_version, version_windows
from readthedocs.restapi.client import api
from readthedocs.vcs_support.backends import backend_cls
Expand Down Expand Up @@ -639,9 +639,10 @@ def get_latest_build(self, finished=True):
return self.builds.filter(**kwargs).first()

def api_versions(self):
from readthedocs.builds.models import APIVersion
ret = []
for version_data in api.project(self.pk).active_versions.get()['versions']:
version = make_api_version(version_data)
version = APIVersion(**version_data)
ret.append(version)
return sort_version_aware(ret)

Expand Down Expand Up @@ -821,6 +822,57 @@ def add_comment(self, version_slug, page, content_hash, commit, user, text):
hash=content_hash, commit=commit)
return node.comments.create(user=user, text=text)

@property
def features(self):
return Feature.objects.for_project(self)

def has_feature(self, feature_id):
"""Does project have existing feature flag

If the feature has a historical True value before the feature was added,
we consider the project to have the flag. This is used for deprecating a
feature or changing behavior for new projects
"""
return self.features.filter(feature_id=feature_id).exists()


class APIProject(Project):

"""Project proxy model for API data deserialization

This replaces the pattern where API data was deserialized into a mocked
:py:cls:`Project` object. This pattern was confusing, as it was not explicit
as to what form of object you were working with -- API backed or database
backed.

This model preserves the Project model methods, allowing for overrides on
model field differences. This model pattern will generally only be used on
builder instances, where we are interacting solely with API data.
"""

features = []

class Meta:
proxy = True

def __init__(self, *args, **kwargs):
self.features = kwargs.pop('features', [])
# These fields only exist on the API return, not on the model, so we'll
# remove them to avoid throwing exceptions due to unexpected fields
for key in ['users', 'resource_uri', 'absolute_url', 'downloads',
'main_language_project', 'related_projects']:
try:
del kwargs[key]
except KeyError:
pass
super(APIProject, self).__init__(*args, **kwargs)

def save(self, *args, **kwargs):
return 0

def has_feature(self, feature_id):
return feature_id in self.features


@python_2_unicode_compatible
class ImportedFile(models.Model):
Expand Down Expand Up @@ -921,3 +973,63 @@ def delete(self, *args, **kwargs): # pylint: disable=arguments-differ
from readthedocs.projects import tasks
broadcast(type='app', task=tasks.symlink_domain, args=[self.project.pk, self.pk, True])
super(Domain, self).delete(*args, **kwargs)


@python_2_unicode_compatible
class Feature(models.Model):

"""Project feature flags

Features should generally be added here as choices, however features may
also be added dynamically from a signal in other packages. Features can be
added by external packages with the use of signals::

@receiver(pre_init, sender=Feature)
def add_features(sender, **kwargs):
sender.FEATURES += (('blah', 'BLAH'),)

The FeatureForm will grab the updated list on instantiation.
"""

# Feature constants - this is not a exhaustive list of features, features
# may be added by other packages
USE_SPHINX_LATEST = 'use_sphinx_latest'

FEATURES = (
(USE_SPHINX_LATEST, _('Use latest version of Sphinx')),
)
Copy link
Member

Choose a reason for hiding this comment

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

Any reason we store these on the model? We usually have a constants file, so it seems odd to break that convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


projects = models.ManyToManyField(
Project,
blank=True,
)
# Feature is not implemented as a ChoiceField, as we don't want validation
# at the database level on this field. Arbitrary values are allowed here.
feature_id = models.CharField(
_('Feature identifier'),
max_length=32,
unique=True,
)
add_date = models.DateTimeField(
_('Date feature was added'),
auto_now_add=True,
)
default_true = models.BooleanField(
_('Historical default is True'),
default=False,
)

objects = FeatureQuerySet.as_manager()

def __str__(self):
return "{0} feature".format(
self.get_feature_display(),
)

def get_feature_display(self):
"""Implement display name field for fake ChoiceField

Because the field is not a ChoiceField here, we need to manually
implement this behavior.
"""
return dict(self.FEATURES).get(self.feature_id, self.feature_id)
Loading