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 6 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', {}))
# This was replicated from readthedocs.projects.utils.make_api_project.
# I'm not certain why these need to be deleted
for key in ['resource_uri', 'absolute_url', 'downloads']:
Copy link
Member

Choose a reason for hiding this comment

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

Because they are fake objects that aren't on the model, but created by the API.

Copy link
Member

Choose a reason for hiding this comment

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

Could also invent the logic and delete anything that isn't a field on the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah gotcha, i recognized these as model attributes without looking. makes sense.

Ultimately, I think we want to some logic that uses the serializer to model these fields modeling for API consumption, and drop Project model proxying altogether. I'm just not sure what the connection to the serializer looks like yet. I think this is probably the best way to reduce duplication of data however.

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)
11 changes: 9 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,14 @@ 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):
requirements.append('sphinx<1.7')
Copy link
Member

Choose a reason for hiding this comment

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

If it's the latest, shouldn't it just do sphinx without a version ID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was curious what we wanted to do here. I'm fine with leaving it bleeding edge, without a specifier. '1.7' specifier was a stopgap from making the version go too high.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe do semvar and <2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yeah, that's a really good idea. hopefully they follow semver as well. i'll do that instead of open ended on this and setuptools PR.

else:
requirements.append('sphinx==1.5.6')
requirements.extend([
'sphinx-rtd-theme<0.3',
'readthedocs-sphinx-ext<0.6'
])

cmd = [
'python',
Expand Down
23 changes: 21 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,26 @@ class DomainAdmin(admin.ModelAdmin):
model = Domain


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

def projects_list(self, feature):
projects = [p.name for p in feature.projects.all()[0:10]]
if feature.projects.count() > 10:
projects += [
'... and {0} more'.format(feature.projects.count() - 10)
]
Copy link
Member

Choose a reason for hiding this comment

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

Surprised this doesn't get done automatically. Does it output the whole list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This view probably isn't useful in this view anyhow. I'll just simplify and make it a count.

return ', '.join(projects)


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', 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.

]
116 changes: 113 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,55 @@ 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):
"""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=feature).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', [])
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):
return feature in self.features


@python_2_unicode_compatible
class ImportedFile(models.Model):
Expand Down Expand Up @@ -921,3 +971,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 = models.CharField(
Copy link
Member

Choose a reason for hiding this comment

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

Feature.feature is a weird name here. It should probably be id or similar.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

feature_id would be fine

_('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, self.feature)
Loading