Re-add alphabetical sorting as default (fixes #1415) (#1416)

* fix(folderadmin): Implement .sort() on file_qs when no order_by query param is set

* fix(folderadmin): conditionally convert file_qs to list and sort if no query params are set

* fix(folderadmin): use file_list when no query params are set, else use file_qs

* Revert "fix(folderadmin): use file_list when no query params are set, else use file_qs"

This reverts commit f34c560.

* chore: revert style changes made by black

* test: add tests for folderadmin sorting

* refactor: use .annotate() and Coalesce to create a temporary sort field instead of using Python sort

* chore: clean up flake8 errors

* chore: clean up flake8 errors

---------

Co-authored-by: Filip Weidemann <[email protected]>

Author: filipweidemann

filer/admin/folderadmin.py

@@ -13,7 +13,8 @@
 from django.core.exceptions import PermissionDenied, ValidationError
 from django.core.paginator import EmptyPage, PageNotAnInteger, Paginator
 from django.db import models, router
-from django.db.models import F, OuterRef, Subquery
+from django.db.models import F, OuterRef, Subquery, Case, When
+from django.db.models.functions import Coalesce, Lower
 from django.http import HttpResponse, HttpResponseRedirect
 from django.shortcuts import get_object_or_404, render
 from django.urls import path, reverse
@@ -317,13 +318,24 @@ def directory_listing(self, request, folder_id=None, viewtype=None):
 
         folder_qs = folder_qs.order_by('name').select_related("owner")
         order_by = request.GET.get('order_by', None)
+        order_by_annotation = None
         if order_by is None:
-            order_by = "file"
-        order_by = order_by.split(',')
+            file_qs = file_qs.annotate(coalesce_sort_field=Coalesce(
+                Case(
+                    When(name__exact='', then=None),
+                    When(name__isnull=False, then='name')
+                ),
+                'original_filename'
+            ))
+            order_by_annotation = Lower('coalesce_sort_field')
+
+        order_by = order_by.split(',') if order_by else []
         order_by = [field for field in order_by
                     if re.sub(r'^-', '', field) in self.order_by_file_fields]
         if len(order_by) > 0:
             file_qs = file_qs.order_by(*order_by)
+        elif order_by_annotation:
+            file_qs = file_qs.order_by(order_by_annotation)
 
         if folder.is_root and not search_mode:
             virtual_items = folder.virtual_folders

tests/test_admin.py

@@ -748,6 +748,94 @@ def create_file(self, folder, filename=None):
         return file_obj
 
 
+class FolderAndFileSortingMixin(BulkOperationsMixin):
+    def setUp(self):
+        self.superuser = create_superuser()
+        self.client.login(username='admin', password='secret')
+        self.img = create_image()
+        self.folder_1 = Folder(name='Pictures', parent=None)
+        self.folder_1.save()
+        self.nested_folder_2 = Folder(name='Nested 2', parent=self.folder_1)
+        self.nested_folder_1 = Folder(name='Nested 1', parent=self.folder_1)
+        self.nested_folder_1.save()
+        self.nested_folder_2.save()
+        self.create_file(folder=self.folder_1, filename='background.jpg')
+        self.create_file(folder=self.folder_1, filename='A_Testfile.jpg')
+        self.create_file(folder=self.folder_1, filename='Another_Test.jpg')
+        newspaper_file = self.create_file(folder=self.folder_1, filename='Newspaper.pdf')
+        newspaper_file.name = 'Zeitung'
+        newspaper_file.save()
+        renamed_file = self.create_file(folder=self.folder_1, filename='last_when_sorting_by_filename.jpg')
+        renamed_file.name = 'A cute dog'
+        renamed_file.save()
+
+    def tearDown(self):
+        self.client.logout()
+        for f in File.objects.all():
+            f.delete()
+        for folder in Folder.objects.all():
+            folder.delete()
+
+
+class FilerFolderAndFileSortingTests(FolderAndFileSortingMixin, TestCase):
+    # Assert that the folders are correctly sorted
+    def test_filer_folder_sorting(self):
+        response = self.client.get(reverse('admin:filer-directory_listing', kwargs={
+            'folder_id': self.folder_1.pk
+        }))
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.context['folder_children'].count(), 2)
+        self.assertEqual(response.context['folder_children'][0].name, 'Nested 1')
+        self.assertEqual(response.context['folder_children'][1].name, 'Nested 2')
+
+    # Default sorting should be alphabetically
+    def test_filer_directory_listing_default_sorting(self):
+        response = self.client.get(reverse('admin:filer-directory_listing', kwargs={
+            'folder_id': self.folder_1.pk
+        }))
+        self.assertEqual(response.status_code, 200)
+        # when using the default sort, the folder_files are of type `list`,
+        # so we assert the length.
+        self.assertEqual(len(response.context['folder_files']), 5)
+
+        expected_filenames = ['A cute dog', 'A_Testfile.jpg', 'Another_Test.jpg', 'background.jpg', 'Zeitung']
+        for index, expected_filename in enumerate(expected_filenames):
+            self.assertEqual(str(response.context['folder_files'][index]), expected_filename)
+
+    # Now, all columns with empty name should be alphabetically sorted by their filename,
+    # after that, at the end of the list, all files with and explicit name should appear;
+    # however, since we ONLY sort by name, the order of items without name is not defined
+    # by their filename but rather by their creation date.
+    # So, the order is expected to be ordered as they are created in the setUp method.
+    def test_filer_directory_listing_sorting_with_order_by_param(self):
+        response = self.client.get(reverse('admin:filer-directory_listing', kwargs={
+            'folder_id': self.folder_1.pk
+        }), {'order_by': 'name'})
+        self.assertEqual(response.status_code, 200)
+        # when using the default sort, the folder_files are of type `list`,
+        # so we assert the length.
+        self.assertEqual(len(response.context['folder_files']), 5)
+
+        expected_filenames = ['background.jpg', 'A_Testfile.jpg', 'Another_Test.jpg', 'A cute dog', 'Zeitung']
+        for index, expected_filename in enumerate(expected_filenames):
+            self.assertEqual(str(response.context['folder_files'][index]), expected_filename)
+
+    # Finally, we can define a fallback column to pass into `order_by` so that files without
+    # any name are still sorted by something (in this case, their original_filename).
+    # This should yield the expected order as well, but NOT the exact same order as the default sorting,
+    # since we sort by name FIRST and all items with the same value again by original_filename.
+    def test_filer_directory_listing_sorting_with_multiple_order_by_params(self):
+        response = self.client.get(reverse('admin:filer-directory_listing', kwargs={
+            'folder_id': self.folder_1.pk
+        }), {'order_by': 'name,original_filename'})
+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(len(response.context['folder_files']), 5)
+
+        expected_filenames = ['A_Testfile.jpg', 'Another_Test.jpg', 'background.jpg', 'A cute dog', 'Zeitung']
+        for index, expected_filename in enumerate(expected_filenames):
+            self.assertEqual(str(response.context['folder_files'][index]), expected_filename)
+
+
 class FilerBulkOperationsTests(BulkOperationsMixin, TestCase):
     def test_move_files_and_folders_action(self):
         # TODO: Test recursive (files and folders tree) move