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

Implement UI for automation rules #5996

Merged
merged 51 commits into from
Nov 12, 2019
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
51 commits
Select commit Hold shift + click to select a range
1164ba4
Add form
stsewd Jul 25, 2019
9ee90f8
Implement views
stsewd Jul 25, 2019
48e8bc9
Add templates
stsewd Jul 25, 2019
511edeb
Urls!
stsewd Jul 25, 2019
4347997
Merge branch 'master' into implement-ui-for-automation-rules
stsewd Aug 2, 2019
2e0831b
Tests
stsewd Aug 5, 2019
9db6f58
Merge branch 'master' into implement-ui-for-automation-rules
stsewd Aug 12, 2019
4ba11d3
Fix merge
stsewd Aug 12, 2019
cb72dff
Fix test
stsewd Aug 13, 2019
3c4109a
Little touch of JS
stsewd Aug 13, 2019
9048368
More js
stsewd Aug 13, 2019
d9575fe
Improve defaults
stsewd Aug 13, 2019
10bc350
Improve initial values
stsewd Aug 13, 2019
47ec2ca
Add up/down arrows to UI
stsewd Aug 13, 2019
f7d2c7d
Remove content
stsewd Aug 13, 2019
ccc02c3
Refactor
stsewd Aug 13, 2019
21fe1f0
Migration
stsewd Aug 13, 2019
e7ca032
Update help text
stsewd Aug 13, 2019
ffe2891
Merge branch 'update-automation-rules-model' into implement-ui-for-au…
stsewd Aug 13, 2019
eca2d14
Add link to python regex
stsewd Aug 13, 2019
09319c8
Update help text
stsewd Aug 14, 2019
5640dae
Merge branch 'add-move-method' into implement-ui-for-automation-rules
stsewd Aug 14, 2019
e001105
Merge branch 'add-move-method' into implement-ui-for-automation-rules
stsewd Aug 14, 2019
89ccea3
Merge branch 'add-move-method' into implement-ui-for-automation-rules
stsewd Aug 14, 2019
9c1cbfc
Rename
stsewd Aug 14, 2019
6b5c4b1
Add move view
stsewd Aug 14, 2019
5d1aa57
Tests
stsewd Aug 14, 2019
b93ef89
Add test for views
stsewd Aug 14, 2019
1779d6b
Merge branch 'add-move-method' into implement-ui-for-automation-rules
stsewd Aug 26, 2019
cdd28a3
Merge branch 'master' into implement-ui-for-automation-rules
stsewd Aug 26, 2019
156b467
Merge branch 'add-move-method' into implement-ui-for-automation-rules
stsewd Aug 26, 2019
576bde5
Merge branch 'add-move-method' into implement-ui-for-automation-rules
stsewd Aug 26, 2019
5d17ab3
Merge branch 'master' into implement-ui-for-automation-rules
stsewd Sep 6, 2019
4d82191
Merge branch 'master' into implement-ui-for-automation-rules
stsewd Nov 5, 2019
5a802a9
Update migration
stsewd Nov 5, 2019
556f676
Add predefined_match_arg field
stsewd Nov 6, 2019
28915d2
Update form
stsewd Nov 6, 2019
0dcd88b
Update UI
stsewd Nov 6, 2019
abcb822
Update migration
stsewd Nov 6, 2019
b206bc0
Fix initial values
stsewd Nov 6, 2019
d41bce7
Merge branch 'master' into implement-ui-for-automation-rules
stsewd Nov 6, 2019
43c4114
Tests
stsewd Nov 6, 2019
5d6d839
Fix tests
stsewd Nov 6, 2019
062b3fc
Fix linter
stsewd Nov 6, 2019
146c816
Apply suggestions from code review
stsewd Nov 11, 2019
9d78e74
Merge branch 'master' into implement-ui-for-automation-rules
stsewd Nov 11, 2019
5ff19bd
Improve a11y
stsewd Nov 11, 2019
ba0ccfb
Move js to a file
stsewd Nov 11, 2019
e8d356f
Fix eslint
stsewd Nov 11, 2019
3c293d6
Merge branch 'master' into implement-ui-for-automation-rules
stsewd Nov 12, 2019
ce3349a
Add comment about knockoutjs
stsewd Nov 12, 2019
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
14 changes: 8 additions & 6 deletions readthedocs/builds/constants.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
# -*- coding: utf-8 -*-

"""Constants for the builds app."""

from django.conf import settings
Expand Down Expand Up @@ -37,16 +35,20 @@
# Manager name for External Versions or Builds.
# ie: Only pull request/merge request Versions and Builds.
EXTERNAL = 'external'
EXTERNAL_TEXT = _('External')

BRANCH = 'branch'
BRANCH_TEXT = _('Branch')
TAG = 'tag'
TAG_TEXT = _('Tag')
UNKNOWN = 'unknown'
UNKNOWN_TEXT = _('Unknown')

VERSION_TYPES = (
(BRANCH, _('Branch')),
(TAG, _('Tag')),
(EXTERNAL, _('External')),
(UNKNOWN, _('Unknown')),
(BRANCH, BRANCH_TEXT),
(TAG, TAG_TEXT),
(EXTERNAL, EXTERNAL_TEXT),
(UNKNOWN, UNKNOWN_TEXT),
)

LATEST = settings.RTD_LATEST
Expand Down
69 changes: 66 additions & 3 deletions readthedocs/builds/forms.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
# -*- coding: utf-8 -*-

"""Django forms for the builds app."""

from django import forms
from django.utils.translation import ugettext_lazy as _

from readthedocs.builds.models import Version
from readthedocs.builds.constants import BRANCH, BRANCH_TEXT, TAG, TAG_TEXT
from readthedocs.builds.models import (
RegexAutomationRule,
Version,
)
from readthedocs.core.mixins import HideProtectedLevelMixin
from readthedocs.core.utils import trigger_build

Expand Down Expand Up @@ -37,3 +39,64 @@ def save(self, commit=True):
if obj.active and not obj.built and not obj.uploaded:
trigger_build(project=obj.project, version=obj)
return obj


class RegexAutomationRuleForm(forms.ModelForm):

match = forms.ChoiceField(
label='Rule',
stsewd marked this conversation as resolved.
Show resolved Hide resolved
choices=[
(r'.*', 'All versions'),
(r'^v?(\d+\.)(\d+\.)(\d)(-.+)?$', 'Semver versions'),
(None, 'Custom'),
stsewd marked this conversation as resolved.
Show resolved Hide resolved
],
required=False,
)

match_arg = forms.CharField(
label='Custom rule',
stsewd marked this conversation as resolved.
Show resolved Hide resolved
help_text=_('A Python regular expression'),
required=False,
)

class Meta:
model = RegexAutomationRule
fields = ['description', 'match', 'match_arg', 'version_type', 'action']

def __init__(self, *args, **kwargs):
self.project = kwargs.pop('project', None)
super().__init__(*args, **kwargs)

self.fields['version_type'].choices = [
stsewd marked this conversation as resolved.
Show resolved Hide resolved
(None, '-' * 9),
(BRANCH, BRANCH_TEXT),
(TAG, TAG_TEXT),
]

def clean_match_arg(self):
match_arg = self.cleaned_data['match_arg']
match = self.cleaned_data['match']
if match is not None:
match_arg = match
if not match_arg:
raise forms.ValidationError(
_('Custom match should not be empty.'),
)
return match_arg

def clean_description(self):
description = self.cleaned_data['description']
if not description:
description = self.instance.get_description()
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to leave it blank if the user does not enter any description and make this check when rendering. This way we are not duplicating the same value over and over in the db and also if we change it later with a better message, it will change for all the users.

Copy link
Member Author

Choose a reason for hiding this comment

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

But then the user would see the description in list page and not on the form, I think that would be confusing

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with that.

Although, if you want to show it in the form as well, you can add it in the Form.__init__ method.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can't do that bc we don't know what option is going to be selected yet

Copy link
Member

Choose a reason for hiding this comment

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

The thing is that you want to populate that field with the default text only when editing the rule. When creating the rule, the field should be empty so the user can write anything they want.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is empty by default, if the form is saved without a description, the rule is saved with the default description, otherwise use what the user wrote

Copy link
Member

Choose a reason for hiding this comment

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

Right. Why do we want to save the same value over and over again in the database if it's the default when it's None? That's my point.

return description

def save(self, commit=True):
if self.instance.pk:
return super().save(commit=commit)
return RegexAutomationRule.objects.append_rule(
project=self.project,
description=self.cleaned_data['description'],
match_arg=self.cleaned_data['match_arg'],
version_type=self.cleaned_data['version_type'],
action=self.cleaned_data['action'],
)
44 changes: 39 additions & 5 deletions readthedocs/builds/managers.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,10 @@
# -*- coding: utf-8 -*-

"""Build and Version class model Managers."""

import logging

from django.db import models
from django.core.exceptions import ObjectDoesNotExist
from django.db import models
from polymorphic.managers import PolymorphicManager

from readthedocs.core.utils.extend import (
SettingsOverrideObject,
Expand All @@ -14,14 +13,15 @@

from .constants import (
BRANCH,
EXTERNAL,
LATEST,
LATEST_VERBOSE_NAME,
STABLE,
STABLE_VERBOSE_NAME,
TAG,
EXTERNAL,
)
from .querysets import VersionQuerySet, BuildQuerySet
from .querysets import BuildQuerySet, VersionQuerySet


log = logging.getLogger(__name__)

Expand Down Expand Up @@ -179,3 +179,37 @@ class InternalBuildManager(SettingsOverrideObject):

class ExternalBuildManager(SettingsOverrideObject):
_default_class = ExternalBuildManagerBase


class VersionAutomationRuleManager(PolymorphicManager):

def append_rule(
stsewd marked this conversation as resolved.
Show resolved Hide resolved
self, *, project, description, match_arg, version_type,
action, action_arg=None,
):
"""
Append an automation rule to `project`.

The rule is created with a priority higher than the last rule
stsewd marked this conversation as resolved.
Show resolved Hide resolved
in `project`.
"""
last_priority = (
project.automation_rules
.values_list('priority', flat=True)
.last()
stsewd marked this conversation as resolved.
Show resolved Hide resolved
)
if last_priority is None:
priority = 0
else:
priority = last_priority + 1

rule = self.create(
project=project,
priority=priority,
description=description,
match_arg=match_arg,
version_type=version_type,
action=action,
action_arg=action_arg,
)
return rule
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# -*- coding: utf-8 -*-
# Generated by Django 1.11.22 on 2019-07-25 17:24
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('builds', '0009_added_external_version_type'),
]

operations = [
migrations.AddField(
model_name='versionautomationrule',
name='description',
field=models.CharField(blank=True, max_length=255, null=True, verbose_name='Description'),
),
]
73 changes: 48 additions & 25 deletions readthedocs/builds/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,48 +17,30 @@
from polymorphic.models import PolymorphicModel

import readthedocs.builds.automation_actions as actions
from readthedocs.config import LATEST_CONFIGURATION_VERSION
from readthedocs.core.utils import broadcast
from readthedocs.projects.constants import (
BITBUCKET_COMMIT_URL,
BITBUCKET_URL,
GITHUB_BRAND,
GITHUB_COMMIT_URL,
GITHUB_URL,
GITHUB_PULL_REQUEST_URL,
GITHUB_PULL_REQUEST_COMMIT_URL,
GITLAB_COMMIT_URL,
GITLAB_URL,
PRIVACY_CHOICES,
PRIVATE,
MEDIA_TYPES,
)
from readthedocs.projects.models import APIProject, Project
from readthedocs.projects.version_handling import determine_stable_version

from readthedocs.builds.constants import (
BRANCH,
BUILD_STATE,
BUILD_STATE_FINISHED,
BUILD_STATE_TRIGGERED,
BUILD_TYPES,
EXTERNAL,
GENERIC_EXTERNAL_VERSION_NAME,
GITHUB_EXTERNAL_VERSION_NAME,
INTERNAL,
LATEST,
NON_REPOSITORY_VERSIONS,
EXTERNAL,
STABLE,
TAG,
VERSION_TYPES,
)
from readthedocs.builds.managers import (
VersionManager,
InternalVersionManager,
ExternalVersionManager,
BuildManager,
InternalBuildManager,
ExternalBuildManager,
ExternalVersionManager,
InternalBuildManager,
InternalVersionManager,
VersionAutomationRuleManager,
VersionManager,
)
from readthedocs.builds.querysets import (
BuildQuerySet,
Expand All @@ -71,7 +53,24 @@
get_gitlab_username_repo,
)
from readthedocs.builds.version_slug import VersionSlugField
from readthedocs.oauth.models import RemoteRepository
from readthedocs.config import LATEST_CONFIGURATION_VERSION
from readthedocs.core.utils import broadcast
from readthedocs.projects.constants import (
BITBUCKET_COMMIT_URL,
BITBUCKET_URL,
GITHUB_BRAND,
GITHUB_COMMIT_URL,
GITHUB_PULL_REQUEST_COMMIT_URL,
GITHUB_PULL_REQUEST_URL,
GITHUB_URL,
GITLAB_COMMIT_URL,
GITLAB_URL,
MEDIA_TYPES,
PRIVACY_CHOICES,
PRIVATE,
)
from readthedocs.projects.models import APIProject, Project
from readthedocs.projects.version_handling import determine_stable_version


log = logging.getLogger(__name__)
Expand Down Expand Up @@ -936,6 +935,12 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel):
_('Rule priority'),
help_text=_('A lower number (0) means a higher priority'),
)
description = models.CharField(
_('Description'),
max_length=255,
null=True,
blank=True,
)
match_arg = models.CharField(
_('Match argument'),
help_text=_('Value used for the rule to match the version'),
Expand All @@ -959,6 +964,8 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel):
choices=VERSION_TYPES,
)

objects = VersionAutomationRuleManager()

class Meta:
unique_together = (('project', 'priority'),)
ordering = ('priority', '-modified', '-created')
Expand Down Expand Up @@ -1002,6 +1009,15 @@ def apply_action(self, version, match_result):
raise NotImplementedError
action(version, match_result, self.action_arg)

def get_description(self):
if self.description:
return self.description
return f'{self.get_action_display()}'

@property
def edit_url(self):
raise NotImplementedError

def __str__(self):
class_name = self.__class__.__name__
return (
Expand Down Expand Up @@ -1030,3 +1046,10 @@ def match(self, version, match_arg):
except Exception as e:
log.info('Error parsing regex: %s', e)
return False, None

@property
def edit_url(self):
stsewd marked this conversation as resolved.
Show resolved Hide resolved
return reverse(
'projects_automation_rule_regex_edit',
args=[self.project.slug, self.pk],
)
6 changes: 2 additions & 4 deletions readthedocs/builds/views.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
# -*- coding: utf-8 -*-

"""Views for builds app."""

import logging
import textwrap
from urllib.parse import urlparse

from django.contrib import messages
from django.contrib.auth.decorators import login_required
Expand All @@ -17,12 +16,11 @@
from django.utils.decorators import method_decorator
from django.views.generic import DetailView, ListView
from requests.utils import quote
from urllib.parse import urlparse

from readthedocs.doc_builder.exceptions import BuildEnvironmentError
from readthedocs.builds.models import Build, Version
from readthedocs.core.permissions import AdminPermission
from readthedocs.core.utils import trigger_build
from readthedocs.doc_builder.exceptions import BuildEnvironmentError
from readthedocs.projects.models import Project


Expand Down
Loading