Merge 20204d1 into c7efd6c

spirit/category/admin/templates/spirit/category/admin/index.html

@@ -18,8 +18,8 @@ <h1 class="headline">{% trans "Category list" %}</h1>
         {% for c in categories %}
             <div class="row">
                 {% if st_settings.ST_ORDERED_CATEGORIES %}
-                    <a href="{% url "spirit:admin:category:move_up" category_id=c.pk %}"><i class="fa fa-arrow-up"></i></a>
-                    <a href="{% url "spirit:admin:category:move_dn" category_id=c.pk %}"><i class="fa fa-arrow-down"></i></a>
+                    <a class="js-post" href="{% url "spirit:admin:category:move_up" category_id=c.pk %}"><i class="fa fa-arrow-up"></i></a>
+                    <a class="js-post" href="{% url "spirit:admin:category:move_dn" category_id=c.pk %}"><i class="fa fa-arrow-down"></i></a>
                 {% endif %}
 
                 <a {% if c.color %}style="color: {{ c.color }}"{% endif %} href="{{ c.get_absolute_url }}">{{ c.title }}</a>
@@ -39,8 +39,8 @@ <h1 class="headline">{% trans "Category list" %}</h1>
                 <div class="row">
                     ---
                     {% if st_settings.ST_ORDERED_CATEGORIES %}
-                        <a href="{% url "spirit:admin:category:move_up" category_id=subc.pk %}"><i class="fa fa-arrow-up"></i></a>
-                        <a href="{% url "spirit:admin:category:move_dn" category_id=subc.pk %}"><i class="fa fa-arrow-down"></i></a>
+                        <a class="js-post" href="{% url "spirit:admin:category:move_up" category_id=subc.pk %}"><i class="fa fa-arrow-up"></i></a>
+                        <a class="js-post" href="{% url "spirit:admin:category:move_dn" category_id=subc.pk %}"><i class="fa fa-arrow-down"></i></a>
                     {% endif %}
 
                     <a {% if subc.color %}style="color: {{ subc.color }}"{% endif %} href="{{ subc.get_absolute_url }}">{{ subc.title }}</a>

spirit/category/admin/tests.py

@@ -89,24 +89,31 @@ def test_category_form_color(self):
 
         self.assertFalse(form.is_valid())
 
-    def test_category_move(self):
+    def test_category_move_up_down(self):
+        """Should order the category when moving up/down"""
         utils.login(self)
         self.another_category = utils.create_category()
-        response = self.client.get(
-            reverse('spirit:admin:category:move_dn', kwargs={"category_id": self.category.pk, }))
+        response = self.client.post(
+            reverse(
+                'spirit:admin:category:move_dn',
+                kwargs={"category_id": self.category.pk, }))
         expected_url = reverse("spirit:admin:category:index")
         self.assertRedirects(response, expected_url, status_code=302)
         self.category.refresh_from_db()
         self.another_category.refresh_from_db()        
         self.assertTrue(self.category.sort > self.another_category.sort)
-        response = self.client.get(
-            reverse('spirit:admin:category:move_up', kwargs={"category_id": self.category.pk, }))
+
+        response = self.client.post(
+            reverse(
+                'spirit:admin:category:move_up',
+                kwargs={"category_id": self.category.pk, }))
         expected_url = reverse("spirit:admin:category:index")
         self.assertRedirects(response, expected_url, status_code=302)
         self.category.refresh_from_db()
         self.another_category.refresh_from_db()        
         self.assertTrue(self.category.sort < self.another_category.sort)
 
+
 class AdminFormTest(TestCase):
 
     def setUp(self):

spirit/category/admin/urls.py

@@ -17,6 +17,6 @@
 
 if settings.ST_ORDERED_CATEGORIES:
     urlpatterns.extend([
-        url(r'^move_up/(?P<category_id>[0-9]+)/$', views.move_up, name='move_up'),
-        url(r'^move_dn/(?P<category_id>[0-9]+)/$', views.move_dn, name='move_dn')
+        url(r'^move-up/(?P<category_id>[0-9]+)/$', views.move_up, name='move_up'),
+        url(r'^move-dn/(?P<category_id>[0-9]+)/$', views.move_dn, name='move_dn')
     ])

spirit/category/admin/views.py

@@ -7,8 +7,8 @@
 from django.urls import reverse
 from django.contrib import messages
 from django.utils.translation import ugettext as _
+from django.views.decorators.http import require_POST
 
-from ...core.conf import settings
 from ...core.utils.decorators import administrator_required
 from ..models import Category
 from .forms import CategoryForm
@@ -58,11 +58,29 @@ def update(request, category_id):
     return render(request, 'spirit/category/admin/update.html', context)
 
 
+# XXX fix race conditions
+@require_POST
 @administrator_required
-def move_up(request, category_id):
-    category = get_object_or_404(Category, pk=category_id)
-    sibling = Category.objects.filter(parent=category.parent, sort__lt=category.sort)\
-        .exclude(pk=settings.ST_TOPIC_PRIVATE_CATEGORY_PK).order_by('-sort').first()
+def _move(request, category_id, direction):
+    if direction == 'up':
+        sort_filter = 'sort__lt'
+        order_by = '-sort'
+    else:
+        assert direction == 'down'
+        sort_filter = 'sort__gt'
+        order_by = 'sort'
+
+    category = get_object_or_404(
+        Category.objects.select_related('parent'),
+        pk=category_id)
+    sibling = (
+        Category.objects
+        .public()
+        .filter(
+            parent=category.parent,
+            **{sort_filter: category.sort})
+        .order_by(order_by)
+        .first())
     if sibling:
         sort = category.sort
         category.sort = sibling.sort
@@ -72,15 +90,11 @@ def move_up(request, category_id):
     return redirect(reverse("spirit:admin:category:index"))
 
 
+@administrator_required
+def move_up(request, category_id):
+    return _move(request, category_id, 'up')
+
+
 @administrator_required
 def move_dn(request, category_id):
-    category = get_object_or_404(Category, pk=category_id)
-    sibling = Category.objects.filter(parent=category.parent, sort__gt=category.sort)\
-        .exclude(pk=settings.ST_TOPIC_PRIVATE_CATEGORY_PK).order_by('sort').first()
-    if sibling:
-        sort = category.sort
-        category.sort = sibling.sort
-        sibling.sort = sort
-        category.save()
-        sibling.save()
-    return redirect(reverse("spirit:admin:category:index"))
+    return _move(request, category_id, 'down')

spirit/category/migrations/0006_category_sort.py → spirit/category/migrations/0006_auto_20190120_0406.py

@@ -1,6 +1,4 @@
-# -*- coding: utf-8 -*-
-# Generated by Django 1.11 on 2018-12-26 08:48
-from __future__ import unicode_literals
+# Generated by Django 2.1.5 on 2019-01-20 04:06
 
 from django.db import migrations, models
 
@@ -24,7 +22,7 @@ class Migration(migrations.Migration):
         migrations.AddField(
             model_name='category',
             name='sort',
-            field=models.PositiveSmallIntegerField(db_index=True, default=0, verbose_name='sorting order'),
+            field=models.PositiveIntegerField(default=0, verbose_name='sorting order'),
         ),
-        migrations.RunPython(reorder, migrations.RunPython.noop)
+        migrations.RunPython(reorder),
     ]

spirit/category/models.py

@@ -34,6 +34,7 @@ class Category(models.Model):
     color = models.CharField(
         _("color"), max_length=7, blank=True,
         help_text=_("Title color in hex format (i.e: #1aafd0)."))
+    sort = models.PositiveIntegerField(_("sorting order"), default=0)
     reindex_at = models.DateTimeField(_("modified at"), default=timezone.now)
 
     is_global = models.BooleanField(
@@ -45,8 +46,6 @@ class Category(models.Model):
     is_removed = models.BooleanField(_("removed"), default=False)
     is_private = models.BooleanField(_("private"), default=False)
 
-    sort = models.PositiveSmallIntegerField(_("sorting order"), default=0, blank=False, null=False, db_index=True)
-
     objects = CategoryQuerySet.as_manager()
 
     class Meta:

spirit/category/tests.py

@@ -18,7 +18,8 @@
 from ..comment.bookmark.models import CommentBookmark
 from .models import Category
 
-data_migration_0006 = importlib.import_module('spirit.category.migrations.0006_category_sort')
+data_migration_0006 = importlib.import_module(
+    'spirit.category.migrations.0006_auto_20190120_0406')
 
 
 class CategoryViewTest(TestCase):
@@ -204,6 +205,11 @@ def test_categories(self):
         self.assertEqual(len(Category.objects.all()), 2)
 
     def test_migration_0006(self):
+        utils.create_category(sort=0)
+        utils.create_category(sort=0)
+        utils.create_category(sort=0)
         data_migration_0006.reorder(apps, None)
+        last = 0
         for category in Category.objects.all():
-            self.assertTrue(category.sort > 0)
+            self.assertTrue(category.sort > last)
+            last = category.sort