Automation Rules: simplify ordering

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

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

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):

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",
+            ),
+        ),
+    ]

readthedocs/builds/models.py

@@ -43,7 +43,6 @@
     ExternalVersionManager,
     InternalBuildManager,
     InternalVersionManager,
-    VersionAutomationRuleManager,
     VersionManager,
 )
 from readthedocs.builds.querysets import (
@@ -1239,6 +1238,7 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel):
     priority = models.IntegerField(
         _('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')
@@ -1359,51 +1357,99 @@ def move(self, steps):
         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 +1467,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:

readthedocs/rtd_tests/tests/test_automation_rule_views.py

@@ -69,7 +69,7 @@ def test_create_and_update_regex_rule(self):
         assert r["Location"] == self.list_rules_url
 
         rule = self.project.automation_rules.get(description="Another rule")
-        assert rule.priority == 1
+        assert rule.priority == 0
 
         r = self.client.post(
             reverse(
@@ -88,7 +88,7 @@ def test_create_and_update_regex_rule(self):
 
         rule.refresh_from_db()
         assert rule.description == "Edit rule"
-        assert rule.priority == 1
+        assert rule.priority == 0
 
     def test_create_regex_rule_default_description(self):
         r = self.client.post(
@@ -194,7 +194,7 @@ def test_delete_rule(self):
                 args=[self.project.slug],
             ),
             {
-                "description": "rule-0",
+                "description": "rule-2",
                 "predefined_match_arg": ALL_VERSIONS,
                 "version_type": TAG,
                 "action": VersionAutomationRule.ACTIVATE_VERSION_ACTION,
@@ -218,7 +218,7 @@ def test_delete_rule(self):
                 args=[self.project.slug],
             ),
             {
-                "description": "rule-2",
+                "description": "rule-0",
                 "predefined_match_arg": ALL_VERSIONS,
                 "version_type": BRANCH,
                 "action": VersionAutomationRule.ACTIVATE_VERSION_ACTION,
@@ -259,7 +259,7 @@ def test_move_rule_up(self):
                 args=[self.project.slug],
             ),
             {
-                "description": "rule-0",
+                "description": "rule-2",
                 "predefined_match_arg": ALL_VERSIONS,
                 "version_type": TAG,
                 "action": VersionAutomationRule.ACTIVATE_VERSION_ACTION,
@@ -283,7 +283,7 @@ def test_move_rule_up(self):
                 args=[self.project.slug],
             ),
             {
-                "description": "rule-2",
+                "description": "rule-0",
                 "predefined_match_arg": ALL_VERSIONS,
                 "version_type": BRANCH,
                 "action": VersionAutomationRule.ACTIVATE_VERSION_ACTION,
@@ -318,7 +318,7 @@ def test_move_rule_down(self):
                 args=[self.project.slug],
             ),
             {
-                "description": "rule-0",
+                "description": "rule-2",
                 "predefined_match_arg": ALL_VERSIONS,
                 "version_type": TAG,
                 "action": VersionAutomationRule.ACTIVATE_VERSION_ACTION,
@@ -342,7 +342,7 @@ def test_move_rule_down(self):
                 args=[self.project.slug],
             ),
             {
-                "description": "rule-2",
+                "description": "rule-0",
                 "predefined_match_arg": ALL_VERSIONS,
                 "version_type": BRANCH,
                 "action": VersionAutomationRule.ACTIVATE_VERSION_ACTION,