-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
base: main
Are you sure you want to change the base?
Changes from all commits
75624f5
da3f8dc
bf22f0d
7ea04d9
11a2c8b
668ceb6
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 |
---|---|---|
@@ -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', '0033_dont_cascade_delete_builds'), | ||
] | ||
|
||
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?'), | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1014,6 +1014,13 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel): | |
related_name='automation_rules', | ||
on_delete=models.CASCADE, | ||
) | ||
on_match = models.BooleanField( | ||
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. I'd call this field 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. I'm not convinced that name is better, as only the match is inverted. 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 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 |
||
_('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
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 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:
Then, by default, "Invert matching" is unchecked. 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. +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'), | ||
|
@@ -1085,7 +1092,8 @@ def run(self, version, **kwargs): | |
return False | ||
|
||
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) | ||
AutomationRuleMatch.objects.register_match( | ||
rule=self, | ||
|
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.
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.
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.
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.