-
-
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?
Conversation
e07b73c
to
75624f5
Compare
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.
This is a good feature! I'm requesting some changes to make the UX easier to understand and use better naming for the field.
on_match = models.BooleanField( | ||
_('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?'), | ||
) |
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.
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.
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.
+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.
@@ -965,6 +965,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 comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this field inverted
or negated
.
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'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 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.
@@ -110,6 +110,7 @@ class Meta: | |||
model = RegexAutomationRule | |||
fields = [ | |||
'description', | |||
'on_match', |
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.
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
Ref #7653
Closes #6354