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

Automation Rules: allow to invert a match #7657

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
12 changes: 8 additions & 4 deletions docs/automation-rules.rst
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,12 @@ You can change the order using the up and down arrow buttons.

New rules are added at the end (lower priority).

Perform action on match
-----------------------

If this attribute is un-check, the rule will be inverted.
This means the action will be performed over versions that don't match the pattern.

Examples
--------

Expand Down Expand Up @@ -127,9 +133,7 @@ Activate all new tags and branches that start with ``v`` or ``V``
Activate all new tags that don't contain the ``-nightly`` suffix
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

.. TODO: update example if https://github.com/readthedocs/readthedocs.org/issues/6354 is approved.


- Custom match: ``.*(?<!-nightly)$``
- Perform action on match: ``False``
- Custom match: ``-nightly$``
- Version type: ``Tag``
- Action: ``Activate version``
7 changes: 7 additions & 0 deletions readthedocs/builds/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@ class Meta:
model = RegexAutomationRule
fields = [
'description',
'on_match',
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this field (in the form) below to "Custom match:". I feel it's more natural to firstly write the regex and then make it negative/invert it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't agree, I think moving it down will make the user think the action or the tab/branch are inverted instead of th match.

'predefined_match_arg',
'match_arg',
'version_type',
Expand All @@ -128,6 +129,11 @@ def __init__(self, *args, **kwargs):
self.project = kwargs.pop('project', None)
super().__init__(*args, **kwargs)

# Remove the nullable option from the form.
# TODO: remove after migration.
self.fields['on_match'].widget = forms.CheckboxInput()
self.fields['on_match'].empty_value = False

# Only list supported types
self.fields['version_type'].choices = [
(None, '-' * 9),
Expand Down Expand Up @@ -181,6 +187,7 @@ def save(self, commit=True):
rule = RegexAutomationRule.objects.add_rule(
project=self.project,
description=self.cleaned_data['description'],
on_match=self.cleaned_data['on_match'],
match_arg=self.cleaned_data['match_arg'],
predefined_match_arg=self.cleaned_data['predefined_match_arg'],
version_type=self.cleaned_data['version_type'],
Expand Down
6 changes: 2 additions & 4 deletions readthedocs/builds/managers.py
Original file line number Diff line number Diff line change
Expand Up @@ -194,8 +194,7 @@ class VersionAutomationRuleManager(PolymorphicManager):
"""

def add_rule(
self, *, project, description, match_arg, version_type,
action, action_arg=None, predefined_match_arg=None,
self, *, project, description, match_arg, version_type, action, **kwargs,
):
"""
Append an automation rule to `project`.
Expand All @@ -219,9 +218,8 @@ def add_rule(
priority=priority,
description=description,
match_arg=match_arg,
predefined_match_arg=predefined_match_arg,
version_type=version_type,
action=action,
action_arg=action_arg,
**kwargs,
)
return rule
18 changes: 18 additions & 0 deletions readthedocs/builds/migrations/0028_add_on_match_field.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
# Generated by Django 2.2.16 on 2020-11-10 15:45

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('builds', '0027_add_privacy_level_automation_rules'),
]

operations = [
migrations.AddField(
model_name='versionautomationrule',
name='on_match',
field=models.BooleanField(default=True, help_text="Perform this action when the pattern matches or when it doesn't match?", null=True, verbose_name='Perform action on match?'),
),
]
10 changes: 9 additions & 1 deletion readthedocs/builds/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -965,6 +965,13 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel):
related_name='automation_rules',
on_delete=models.CASCADE,
)
on_match = models.BooleanField(
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this field inverted or negated.

Copy link
Member Author

@stsewd stsewd Nov 11, 2020

Choose a reason for hiding this comment

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

I'm not convinced that name is better, as only the match is inverted.

Copy link
Member

Choose a reason for hiding this comment

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

It feels natural to me to call it inverted (eg. you have been using inverted to refer to it in different places already: PR description and this comment as well). We can add the match prefix or suffix to convey only that the match is inverted if needed.

_('Perform action on match?'),
# TODO: remove after migration.
null=True,
default=True,
help_text=_('Perform this action when the pattern matches or when it doesn\'t match?'),
)
Comment on lines +1017 to +1023
Copy link
Member

Choose a reason for hiding this comment

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

This message is confusing for a checkbox to me. Also, I think we should invert the message to make it "negative" when it's checked. So, I'd suggest something like:

  • Label: Invert matching
  • Description: Apply this rule to all versions that don't match the regex

Then, by default, "Invert matching" is unchecked.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1 on a shorted label/description, but I still think having it true as default is better. Is easy to associate positive action = check, negative action = uncheck than the other way around.

priority = models.IntegerField(
_('Rule priority'),
help_text=_('A lower number (0) means a higher priority'),
Expand Down Expand Up @@ -1034,7 +1041,8 @@ def run(self, version, *args, **kwargs):
"""
if version.type == self.version_type:
match, result = self.match(version, self.get_match_arg())
if match:
# TODO: remove the is None check after migration.
if bool(match) is (self.on_match is None or self.on_match):
self.apply_action(version, result)
return True
return False
Expand Down
26 changes: 26 additions & 0 deletions readthedocs/rtd_tests/tests/test_automation_rules.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,32 @@ def test_match(
)
assert rule.run(version) is result

@pytest.mark.parametrize('type', [TAG, BRANCH])
def test_not_match(self, type):
version = get(
Version,
verbose_name='dont-match',
project=self.project,
active=False,
type=type,
built=False,
)
rule = get(
RegexAutomationRule,
project=self.project,
on_match=False,
priority=0,
match_arg='^match$',
action=VersionAutomationRule.ACTIVATE_VERSION_ACTION,
version_type=type,
)

assert rule.run(version) is True

version.verbose_name = 'match'
version.save()
assert rule.run(version) is False

@pytest.mark.parametrize(
'version_name,result',
[
Expand Down