Automation Rules: simplify ordering (#10896)

stsewd · web-flow · commit 9cecf04f9aa4 · 2023-11-30T16:16:45.000-05:00
I'm trying to re-use this for redirects, and found a couple of things that we can improve.

- New items would be inserted first instead of at the end, if a user is adding a new item, they may want to have it priority over existing ones. Inserting the item at the end was mostly done because it was "easier" to implement.
- Instead of explicitly using the manager to add a new rule, just override the save() method. This allows us to:
  - Insert a new rule with any priority
  - Change the priority of a rule by just saving it with the new priority.

  With this, we no longer need to override forms, serializers, etc, saving the model will take care of putting the rule in the right place.
diff --git a/docs/user/guides/automation-rules.rst b/docs/user/guides/automation-rules.rst
@@ -81,4 +81,5 @@ You can change the order using the up and down arrow buttons.
 
 .. note::
 
-   New rules are added at the end (lower priority).
+   New rules are added at the start of the list
+   (i.e. they have the highest priority).
diff --git a/readthedocs/builds/forms.py b/readthedocs/builds/forms.py
@@ -93,6 +93,7 @@ def save(self, commit=True):
 
 class RegexAutomationRuleForm(forms.ModelForm):
 
+    project = forms.CharField(widget=forms.HiddenInput(), required=False)
     match_arg = forms.CharField(
         label='Custom match',
         help_text=_(textwrap.dedent(
@@ -109,11 +110,12 @@ class RegexAutomationRuleForm(forms.ModelForm):
     class Meta:
         model = RegexAutomationRule
         fields = [
-            'description',
-            'predefined_match_arg',
-            'match_arg',
-            'version_type',
-            'action',
+            "project",
+            "description",
+            "predefined_match_arg",
+            "match_arg",
+            "version_type",
+            "action",
         ]
         # Don't pollute the UI with help texts
         help_texts = {
@@ -174,19 +176,5 @@ def clean_match_arg(self):
             )
         return match_arg
 
-    def save(self, commit=True):
-        if self.instance.pk:
-            rule = super().save(commit=commit)
-        else:
-            rule = RegexAutomationRule.objects.add_rule(
-                project=self.project,
-                description=self.cleaned_data['description'],
-                match_arg=self.cleaned_data['match_arg'],
-                predefined_match_arg=self.cleaned_data['predefined_match_arg'],
-                version_type=self.cleaned_data['version_type'],
-                action=self.cleaned_data['action'],
-            )
-        if not rule.description:
-            rule.description = rule.get_description()
-            rule.save()
-        return rule
+    def clean_project(self):
+        return self.project
diff --git a/readthedocs/builds/managers.py b/readthedocs/builds/managers.py
@@ -3,7 +3,6 @@
 import structlog
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import models
-from polymorphic.managers import PolymorphicManager
 
 from readthedocs.builds.constants import (
     BRANCH,
@@ -39,7 +38,7 @@ def from_queryset(cls, queryset_class, class_name=None):
         # no direct members.
         queryset_class = get_override_class(
             VersionQuerySet,
-            VersionQuerySet._default_class,  # pylint: disable=protected-access
+            VersionQuerySet._default_class,
         )
         return super().from_queryset(queryset_class, class_name)
 
@@ -135,52 +134,6 @@ def get_queryset(self):
         return super().get_queryset().filter(version__type=EXTERNAL)
 
 
-class VersionAutomationRuleManager(PolymorphicManager):
-
-    """
-    Manager for VersionAutomationRule.
-
-    .. note::
-
-       This manager needs to inherit from PolymorphicManager,
-       since the model is a PolymorphicModel.
-       See https://django-polymorphic.readthedocs.io/page/managers.html
-    """
-
-    def add_rule(
-        self, *, project, description, match_arg, version_type,
-        action, action_arg=None, predefined_match_arg=None,
-    ):
-        """
-        Append an automation rule to `project`.
-
-        The rule is created with a priority lower than the last rule
-        in `project`.
-        """
-        last_priority = (
-            project.automation_rules
-            .values_list('priority', flat=True)
-            .order_by('priority')
-            .last()
-        )
-        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,
-            predefined_match_arg=predefined_match_arg,
-            version_type=version_type,
-            action=action,
-            action_arg=action_arg,
-        )
-        return rule
-
-
 class AutomationRuleMatchManager(models.Manager):
 
     def register_match(self, rule, version, max_registers=15):
diff --git a/readthedocs/builds/migrations/0055_alter_versionautomationrule_priority.py b/readthedocs/builds/migrations/0055_alter_versionautomationrule_priority.py
@@ -0,0 +1,21 @@
+# Generated by Django 4.2.5 on 2023-11-08 22:18
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("builds", "0054_add_builds_index_for_addons"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="versionautomationrule",
+            name="priority",
+            field=models.IntegerField(
+                default=0,
+                help_text="A lower number (0) means a higher priority",
+                verbose_name="Rule priority",
+            ),
+        ),
+    ]
diff --git a/readthedocs/builds/migrations/0056_alter_versionautomationrule_priority.py b/readthedocs/builds/migrations/0056_alter_versionautomationrule_priority.py
@@ -0,0 +1,21 @@
+# Generated by Django 4.2.5 on 2023-11-22 16:50
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+    dependencies = [
+        ("builds", "0055_alter_versionautomationrule_priority"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="versionautomationrule",
+            name="priority",
+            field=models.PositiveIntegerField(
+                default=0,
+                help_text="A lower number (0) means a higher priority",
+                verbose_name="Rule priority",
+            ),
+        ),
+    ]
diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py
@@ -43,7 +43,6 @@
     ExternalVersionManager,
     InternalBuildManager,
     InternalVersionManager,
-    VersionAutomationRuleManager,
     VersionManager,
 )
 from readthedocs.builds.querysets import (
@@ -1236,9 +1235,10 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel):
         related_name='automation_rules',
         on_delete=models.CASCADE,
     )
-    priority = models.IntegerField(
+    priority = models.PositiveIntegerField(
         _('Rule priority'),
         help_text=_('A lower number (0) means a higher priority'),
+        default=0,
     )
     description = models.CharField(
         _('Description'),
@@ -1283,8 +1283,6 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel):
         choices=VERSION_TYPES,
     )
 
-    objects = VersionAutomationRuleManager()
-
     class Meta:
         unique_together = (('project', 'priority'),)
         ordering = ('priority', '-modified', '-created')
@@ -1354,56 +1352,103 @@ def move(self, steps):
 
         :param steps: Number of steps to be moved
                       (it can be negative)
-        :returns: True if the priority was changed
         """
         total = self.project.automation_rules.count()
         current_priority = self.priority
         new_priority = (current_priority + steps) % total
+        self.priority = new_priority
+        self.save()
 
-        if current_priority == new_priority:
-            return False
+    def _change_priority(self):
+        """
+        Re-order the priorities of the other rules when the priority of this rule changes.
+
+        If the rule is new, we just need to move all other rules down,
+        so there is space for the new rule.
+
+        If the rule already exists, we need to move the other rules up or down,
+        depending on the new priority, so we can insert the rule at the new priority.
+
+        The save() method needs to be called after this method.
+        """
+        total = self.project.automation_rules.count()
+
+        # If the rule was just created, we just need to insert it at the given priority.
+        # We do this by moving the other rules down before saving.
+        if not self.pk:
+            # A new rule can be created at the end as max.
+            self.priority = min(self.priority, total)
+
+            # A new rule can't be created with a negative priority. All rules start at 0.
+            self.priority = max(self.priority, 0)
 
-        # Move other's priority
-        if new_priority > current_priority:
-            # It was moved down
-            rules = (
-                self.project.automation_rules
-                .filter(priority__gt=current_priority, priority__lte=new_priority)
-                # We sort the queryset in asc order
-                # to be updated in that order
-                # to avoid hitting the unique constraint (project, priority).
-                .order_by('priority')
-            )
-            expression = F('priority') - 1
-        else:
-            # It was moved up
             rules = (
-                self.project.automation_rules
-                .filter(priority__lt=current_priority, priority__gte=new_priority)
-                .exclude(pk=self.pk)
+                self.project.automation_rules.filter(priority__gte=self.priority)
                 # We sort the queryset in desc order
                 # to be updated in that order
                 # to avoid hitting the unique constraint (project, priority).
                 .order_by('-priority')
             )
             expression = F('priority') + 1
+        else:
+            current_priority = self.project.automation_rules.values_list(
+                "priority",
+                flat=True,
+            ).get(pk=self.pk)
+
+            # An existing rule can't be moved past the end.
+            self.priority = min(self.priority, total - 1)
+
+            # A new rule can't be created with a negative priority. all rules start at 0.
+            self.priority = max(self.priority, 0)
+
+            # The rule wasn't moved, so we don't need to do anything.
+            if self.priority == current_priority:
+                return
+
+            if self.priority > current_priority:
+                # It was moved down, so we need to move the other rules up.
+                rules = (
+                    self.project.automation_rules.filter(
+                        priority__gt=current_priority, priority__lte=self.priority
+                    )
+                    # We sort the queryset in asc order
+                    # to be updated in that order
+                    # to avoid hitting the unique constraint (project, priority).
+                    .order_by("priority")
+                )
+                expression = F("priority") - 1
+            else:
+                # It was moved up, so we need to move the other rules down.
+                rules = (
+                    self.project.automation_rules.filter(
+                        priority__lt=current_priority, priority__gte=self.priority
+                    )
+                    # We sort the queryset in desc order
+                    # to be updated in that order
+                    # to avoid hitting the unique constraint (project, priority).
+                    .order_by("-priority")
+                )
+                expression = F("priority") + 1
 
         # Put an impossible priority to avoid
-        # the unique constraint (project, priority)
-        # while updating.
-        self.priority = total + 99
-        self.save()
-
-        # We update each object one by one to
-        # avoid hitting the unique constraint (project, priority).
+        # the unique constraint (project, priority) while updating.
+        # We use update() instead of save() to avoid calling the save() method again.
+        if self.pk:
+            self._meta.model.objects.filter(pk=self.pk).update(priority=total + 99)
+
+        # NOTE: we can't use rules.update(priority=expression), because SQLite is used
+        # in tests and hits a UNIQUE constraint error. PostgreSQL doesn't have this issue.
+        # We use update() instead of save() to avoid calling the save() method.
         for rule in rules:
-            rule.priority = expression
-            rule.save()
+            self._meta.model.objects.filter(pk=rule.pk).update(priority=expression)
 
-        # Put back new priority
-        self.priority = new_priority
-        self.save()
-        return True
+    def save(self, *args, **kwargs):
+        """Override method to update the other priorities before save."""
+        self._change_priority()
+        if not self.description:
+            self.description = self.get_description()
+        super().save(*args, **kwargs)
 
     def delete(self, *args, **kwargs):
         """Override method to update the other priorities after delete."""
@@ -1421,9 +1466,11 @@ def delete(self, *args, **kwargs):
         )
         # We update each object one by one to
         # avoid hitting the unique constraint (project, priority).
+        # We use update() instead of save() to avoid calling the save() method.
         for rule in rules:
-            rule.priority = F('priority') - 1
-            rule.save()
+            self._meta.model.objects.filter(pk=rule.pk).update(
+                priority=F("priority") - 1,
+            )
 
     def get_description(self):
         if self.description:
diff --git a/readthedocs/rtd_tests/tests/test_automation_rule_views.py b/readthedocs/rtd_tests/tests/test_automation_rule_views.py
diff --git a/readthedocs/rtd_tests/tests/test_automation_rules.py b/readthedocs/rtd_tests/tests/test_automation_rules.py
