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

Conversation

stsewd
Copy link
Member

@stsewd stsewd commented Nov 10, 2020

foo

Ref #7653
Closes #6354

@stsewd stsewd force-pushed the invert-automation-rules branch from e07b73c to 75624f5 Compare November 10, 2020 16:54
@stsewd stsewd requested a review from a team November 10, 2020 17:22
Copy link
Member

@humitos humitos left a 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.

Comment on lines +968 to +974
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?'),
)
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.

@@ -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.

@@ -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.

@stale
Copy link

stale bot commented Dec 29, 2020

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.

@stale stale bot added the Status: stale Issue will be considered inactive soon label Dec 29, 2020
@ericholscher ericholscher added the Accepted Accepted issue on our roadmap label Dec 29, 2020
@stale stale bot removed the Status: stale Issue will be considered inactive soon label Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accepted Accepted issue on our roadmap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support option to invert match on automation rules
3 participants