fix test case

src/documents/file_handling.py

@@ -79,7 +79,19 @@ def many_to_dictionary(field):
     return mydictionary
 
 
-def generate_unique_filename(doc, archive_filename=False):
+def generate_unique_filename(doc,
+                             archive_filename=False):
+    """
+    Generates a unique filename for doc in settings.ORIGINALS_DIR.
+
+    The returned filename is guaranteed to be either the current filename
+    of the document if unchanged, or a new filename that does not correspondent
+    to any existing files. The function will append _01, _02, etc to the
+    filename before the extension to avoid conflicts.
+
+    If archive_filename is True, return a unique archive filename instead.
+
+    """
     if archive_filename:
         old_filename = doc.archive_filename
         root = settings.ARCHIVE_DIR
@@ -87,6 +99,14 @@ def generate_unique_filename(doc, archive_filename=False):
         old_filename = doc.filename
         root = settings.ORIGINALS_DIR
 
+    # If generating archive filenames, try to make a name that is similar to
+    # the original filename first.
+
+    if archive_filename and doc.filename:
+        new_filename = os.path.splitext(doc.filename)[0] + ".pdf"
+        if new_filename == old_filename or not os.path.exists(os.path.join(root, new_filename)):  # NOQA: E501
+            return new_filename
+
     counter = 0
 
     while True:

src/documents/signals/handlers.py

@@ -173,21 +173,23 @@ def cleanup_document_deletion(sender, instance, using, **kwargs):
             )
 
 
+class CannotMoveFilesException(Exception):
+    pass
+
+
 def validate_move(instance, old_path, new_path):
     if not os.path.isfile(old_path):
         # Can't do anything if the old file does not exist anymore.
         logger.fatal(
             f"Document {str(instance)}: File {old_path} has gone.")
-        return False
+        raise CannotMoveFilesException()
 
     if os.path.isfile(new_path):
         # Can't do anything if the new file already exists. Skip updating file.
         logger.warning(
             f"Document {str(instance)}: Cannot rename file "
             f"since target path {new_path} already exists.")
-        return False
+        raise CannotMoveFilesException()
-
-    return True
 
 
 @receiver(models.signals.m2m_changed, sender=Document.tags.through)
@@ -206,45 +208,40 @@ def update_filename_and_move_files(sender, instance, **kwargs):
         return
 
     with FileLock(settings.MEDIA_LOCK):
-        old_filename = instance.filename
-        new_filename = generate_unique_filename(instance)
-        move_original = old_filename != new_filename
-        old_source_path = instance.source_path
-        new_source_path = os.path.join(settings.ORIGINALS_DIR, new_filename)
-
-        if move_original and not validate_move(instance, old_source_path, new_source_path):
-            return
-
-        old_archive_filename = instance.archive_filename
-
-        if instance.has_archive_version:
-            new_archive_filename = generate_unique_filename(
-                instance, archive_filename=True
-            )
-            old_archive_path = instance.archive_path
-            new_archive_path = os.path.join(settings.ARCHIVE_DIR,
-                                            new_archive_filename)
-
-            move_archive = old_archive_filename != new_archive_filename
-            if move_archive and not validate_move(instance, old_archive_path, new_archive_path):
-                return
-        else:
-            move_archive = False
-
-        if not move_original and not move_archive:
-            # Don't do anything if filenames did not change.
-            return
-
         try:
+            old_filename = instance.filename
+            old_source_path = instance.source_path
+
+            instance.filename = generate_unique_filename(instance)
+            move_original = old_filename != instance.filename
+
+            old_archive_filename = instance.archive_filename
+            old_archive_path = instance.archive_path
+
+            if instance.has_archive_version:
+
+                instance.archive_filename = generate_unique_filename(
+                    instance, archive_filename=True
+                )
+
+                move_archive = old_archive_filename != instance.archive_filename  # NOQA: E501
+            else:
+                move_archive = False
+
+            if not move_original and not move_archive:
+                # Don't do anything if filenames did not change.
+                return
+
             if move_original:
-                instance.filename = new_filename
+                validate_move(instance, old_source_path, instance.source_path)
-                create_source_path_directory(new_source_path)
+                create_source_path_directory(instance.source_path)
-                os.rename(old_source_path, new_source_path)
+                os.rename(old_source_path, instance.source_path)
 
             if move_archive:
-                instance.archive_filename = new_archive_filename
+                validate_move(
-                create_source_path_directory(new_archive_path)
+                    instance, old_archive_path, instance.archive_path)
-                os.rename(old_archive_path, new_archive_path)
+                create_source_path_directory(instance.archive_path)
+                os.rename(old_archive_path, instance.archive_path)
 
             # Don't save() here to prevent infinite recursion.
             Document.objects.filter(pk=instance.pk).update(
@@ -252,23 +249,19 @@ def update_filename_and_move_files(sender, instance, **kwargs):
                 archive_filename=instance.archive_filename,
             )
 
-        except Exception as e:
+        except (OSError, DatabaseError, CannotMoveFilesException):
             # This happens when either:
             #  - moving the files failed due to file system errors
             #  - saving to the database failed due to database errors
             # In both cases, we need to revert to the original state.
 
-            # restore old values on the instance
-            instance.filename = old_filename
-            instance.archive_filename = old_archive_filename
-
             # Try to move files to their original location.
             try:
-                if move_original and os.path.isfile(new_source_path):
+                if move_original and os.path.isfile(instance.source_path):
-                    os.rename(new_source_path, old_source_path)
+                    os.rename(instance.source_path, old_source_path)
 
-                if move_archive and os.path.isfile(new_archive_path):
+                if move_archive and os.path.isfile(instance.archive_path):
-                    os.rename(new_archive_path, old_archive_path)
+                    os.rename(instance.archive_path, old_archive_path)
 
             except Exception as e:
                 # This is fine, since:
@@ -281,6 +274,10 @@ def update_filename_and_move_files(sender, instance, **kwargs):
                 #  anyway.
                 pass
 
+            # restore old values on the instance
+            instance.filename = old_filename
+            instance.archive_filename = old_archive_filename
+
         # finally, remove any empty sub folders. This will do nothing if
         # something has failed above.
         if not os.path.isfile(old_source_path):