From 0f3ad18d1e3a3fa91447c0b23c892137062fd2e8 Mon Sep 17 00:00:00 2001
From: Gert-Jan Braas <braas@steckelfisch.nl>
Date: Tue, 31 Mar 2020 14:02:21 +0200
Subject: [PATCH] Bugfix/fix delete image (#1034)

* intermediate commit

* extended test for purging image with multiple revisions.

https://github.com/django-wiki/django-wiki/issues/936

* made same implentation as in attachments.model

* cleanup, and a little comment

* cleanup

* Update src/wiki/plugins/images/models.py

Co-Authored-By: Benjamin Balder Bach <benjaoming@gmail.com>

* implemented review request changes

* as per review request

* updated releasenotes for 0.6

* one assignment is enough

Co-authored-by: Benjamin Balder Bach <benjaoming@gmail.com>
---
 docs/release_notes.rst             |  2 +-
 src/wiki/plugins/images/models.py  | 18 ++++++++++++++----
 testproject/testproject/urls.py    | 12 ++++++++++++
 tests/plugins/images/test_views.py | 30 ++++++++++++++++++++++++++++++
 4 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/docs/release_notes.rst b/docs/release_notes.rst
index 21641637..94d213c6 100644
--- a/docs/release_notes.rst
+++ b/docs/release_notes.rst
@@ -27,7 +27,7 @@ Fixed
 * Python 3.7 issue with notifications plugin main view ``/_plugin/notifications/`` :url-issue:`1000` (Mads Jensen)
 * Broken Delete and Deleted pages :url-issue:`976` (Benjamin Bach)
 * Can't delete article with ``USE_THOUSAND_SEPARATOR = True`` :url-issue:`1014` (tim3towers)
-
+* Deleting images fails :url-issue:'936' (Gert-Jan Braas, Steckelfisch)
 
 Changed
 ~~~~~~~
diff --git a/src/wiki/plugins/images/models.py b/src/wiki/plugins/images/models.py
index b2ddd152..04b01fae 100644
--- a/src/wiki/plugins/images/models.py
+++ b/src/wiki/plugins/images/models.py
@@ -119,13 +119,23 @@ def on_image_revision_delete(instance, *args, **kwargs):
     if not instance.image:
         return
 
-    # Remove image file
-    instance.image.delete(save=False)
-
+    path = None
     try:
-        path = instance.image.path.split("/")[:-1]
+        path = os.path.dirname(instance.image.path)
     except NotImplementedError:
         # This backend storage doesn't implement 'path' so there is no path to delete
+        pass
+    except ValueError:
+        # in case of Value error
+        # https://github.com/django-wiki/django-wiki/issues/936
+        pass
+    finally:
+        # Remove image file
+        instance.image.delete(save=False)
+
+    if path is None:
+        # This backend storage doesn't implement 'path' so there is no path to delete
+        # or some other error (ValueError)
         return
 
     # Clean up empty directories
diff --git a/testproject/testproject/urls.py b/testproject/testproject/urls.py
index fca7d43c..e1601da9 100644
--- a/testproject/testproject/urls.py
+++ b/testproject/testproject/urls.py
@@ -22,6 +22,18 @@ if settings.DEBUG:
         ),
     ]
 
+if settings.DEBUG:
+    try:
+        import debug_toolbar
+        urlpatterns = [
+            re_path('__debug__/', include(debug_toolbar.urls)),
+
+            # For django versions before 2.0:
+            # url(r'^__debug__/', include(debug_toolbar.urls)),
+
+        ] + urlpatterns
+    except ImportError as ie:
+        pass
 
 urlpatterns += [
     re_path(r"^notify/", include("django_nyt.urls")),
diff --git a/tests/plugins/images/test_views.py b/tests/plugins/images/test_views.py
index 25441a32..c7f990ca 100644
--- a/tests/plugins/images/test_views.py
+++ b/tests/plugins/images/test_views.py
@@ -242,6 +242,36 @@ class ImageTests(RequireRootArticleMixin, ArticleWebTestUtils, DjangoClientTestB
         self.assertEqual(models.Image.objects.count(), 0)
         self.assertIs(os.path.exists(f_path), False)
 
+    def test_add_revision_purge_image(self):
+        """
+        Tests that an image with more than one revision is really purged
+        """
+        # use another test to stage this one
+        self.test_add_revision()
+
+        image = models.Image.objects.get()
+        image_revision = image.current_revision.imagerevision
+        f_path = image_revision.image.file.name
+
+        self.assertIs(os.path.exists(f_path), True)
+
+        response = self.client.post(
+            reverse(
+                "wiki:images_purge",
+                kwargs={
+                    "article_id": self.root_article,
+                    "image_id": image.pk,
+                    "path": "",
+                },
+            ),
+            data={"confirm": True},
+        )
+        self.assertRedirects(
+            response, reverse("wiki:images_index", kwargs={"path": ""})
+        )
+        self.assertEqual(models.Image.objects.count(), 0)
+        self.assertIs(os.path.exists(f_path), False)
+
     @wiki_override_settings(ACCOUNT_HANDLING=True)
     def test_login_on_revision_add(self):
         self._create_test_image(path="")
-- 
2.45.2