From 29d243b3c54cccee8a222a8b76f48583f4a25215 Mon Sep 17 00:00:00 2001 From: Yohan Boniface Date: Tue, 10 Dec 2024 16:26:58 +0100 Subject: [PATCH] feat: move map to trash on delete This also: - adds a `umap empty_trash` command - remove the previous purgatory concept --- docs/config/settings.md | 8 ----- umap/management/commands/empty_trash.py | 32 +++++++++++++++++ umap/management/commands/purge_purgatory.py | 28 --------------- umap/models.py | 13 +++---- umap/settings/base.py | 1 - .../integration/test_anonymous_owned_map.py | 2 +- umap/tests/integration/test_dashboard.py | 2 +- umap/tests/integration/test_owned_map.py | 2 +- umap/tests/test_datalayer.py | 2 -- umap/tests/test_empty_trash.py | 34 +++++++++++++++++++ umap/tests/test_map.py | 7 ++++ umap/tests/test_map_views.py | 10 ++++-- umap/tests/test_purge_purgatory.py | 25 -------------- umap/views.py | 2 +- 14 files changed, 88 insertions(+), 80 deletions(-) create mode 100644 umap/management/commands/empty_trash.py delete mode 100644 umap/management/commands/purge_purgatory.py create mode 100644 umap/tests/test_empty_trash.py delete mode 100644 umap/tests/test_purge_purgatory.py diff --git a/docs/config/settings.md b/docs/config/settings.md index 88ed527d..4cea33fe 100644 --- a/docs/config/settings.md +++ b/docs/config/settings.md @@ -287,14 +287,6 @@ How many total maps to return in the search. How many maps to show in the user "my maps" page. -#### UMAP_PURGATORY_ROOT - -Path where files are moved when a datalayer is deleted. They will stay there until -`umap purge_purgatory` is run. May be useful in case a user deletes by mistake -a datalayer, or even a map. -Default is `/tmp/umappurgatory/`, so beware that this folder will be deleted on -each server restart. - #### UMAP_SEARCH_CONFIGURATION Use it if you take control over the search configuration. diff --git a/umap/management/commands/empty_trash.py b/umap/management/commands/empty_trash.py new file mode 100644 index 00000000..de35ab37 --- /dev/null +++ b/umap/management/commands/empty_trash.py @@ -0,0 +1,32 @@ +from datetime import UTC, datetime, timedelta + +from django.core.management.base import BaseCommand + +from umap.models import Map + + +class Command(BaseCommand): + help = "Remove maps in trash. Eg.: umap purge_deleted --days 7" + + def add_arguments(self, parser): + parser.add_argument( + "--days", + help="Number of days to consider maps for removal", + default=30, + type=int, + ) + parser.add_argument( + "--dry-run", + help="Pretend to delete but just report", + action="store_true", + ) + + def handle(self, *args, **options): + days = options["days"] + since = datetime.now(UTC) - timedelta(days=days) + print(f"Deleting map in trash since {since}") + maps = Map.objects.filter(share_status=Map.DELETED, modified_at__lt=since) + for map in maps: + if not options["dry_run"]: + map.delete() + print(f"Deleted map {map.name} ({map.id}), trashed on {map.modified_at}") diff --git a/umap/management/commands/purge_purgatory.py b/umap/management/commands/purge_purgatory.py deleted file mode 100644 index ee4be29d..00000000 --- a/umap/management/commands/purge_purgatory.py +++ /dev/null @@ -1,28 +0,0 @@ -import time -from pathlib import Path - -from django.conf import settings -from django.core.management.base import BaseCommand - - -class Command(BaseCommand): - help = "Remove old files from purgatory. Eg.: umap purge_purgatory --days 7" - - def add_arguments(self, parser): - parser.add_argument( - "--days", - help="Number of days to consider files for removal", - default=30, - type=int, - ) - - def handle(self, *args, **options): - days = options["days"] - root = Path(settings.UMAP_PURGATORY_ROOT) - threshold = time.time() - days * 86400 - for path in root.iterdir(): - stats = path.stat() - filestamp = stats.st_mtime - if filestamp < threshold: - path.unlink() - print(f"Removed old file {path}") diff --git a/umap/models.py b/umap/models.py index 93c315d8..912d6ea6 100644 --- a/umap/models.py +++ b/umap/models.py @@ -276,6 +276,10 @@ class Map(NamedModel): ) return map_settings + def move_to_trash(self): + self.share_status = Map.DELETED + self.save() + def delete(self, **kwargs): # Explicitely call datalayers.delete, so we can deal with removing files # (the cascade delete would not call the model delete method) @@ -513,17 +517,8 @@ class DataLayer(NamedModel): def delete(self, **kwargs): self.purge_gzip() - self.to_purgatory() return super().delete(**kwargs) - def to_purgatory(self): - dest = Path(settings.UMAP_PURGATORY_ROOT) - dest.mkdir(parents=True, exist_ok=True) - src = Path(self.geojson.storage.location) / self.storage_root() - for version in self.versions: - name = version["name"] - shutil.move(src / name, dest / f"{self.map.pk}_{name}") - def upload_to(self): root = self.storage_root() name = "%s_%s.geojson" % (self.pk, int(time.time() * 1000)) diff --git a/umap/settings/base.py b/umap/settings/base.py index 8c61146c..ceea26af 100644 --- a/umap/settings/base.py +++ b/umap/settings/base.py @@ -272,7 +272,6 @@ UMAP_DEFAULT_FEATURES_HAVE_OWNERS = False UMAP_HOME_FEED = "latest" UMAP_IMPORTERS = {} UMAP_HOST_INFOS = {} -UMAP_PURGATORY_ROOT = "/tmp/umappurgatory" UMAP_LABEL_KEYS = ["name", "title"] UMAP_READONLY = env("UMAP_READONLY", default=False) diff --git a/umap/tests/integration/test_anonymous_owned_map.py b/umap/tests/integration/test_anonymous_owned_map.py index 5b964883..85d71903 100644 --- a/umap/tests/integration/test_anonymous_owned_map.py +++ b/umap/tests/integration/test_anonymous_owned_map.py @@ -240,7 +240,7 @@ def test_anonymous_owner_can_delete_the_map(anonymap, live_server, owner_session owner_session.get_by_role("button", name="Delete").click() with owner_session.expect_response(re.compile(r".*/update/delete/.*")): owner_session.get_by_role("button", name="OK").click() - assert not Map.objects.count() + assert Map.objects.get(pk=anonymap.pk).share_status == Map.DELETED def test_non_owner_cannot_see_delete_button(anonymap, live_server, page): diff --git a/umap/tests/integration/test_dashboard.py b/umap/tests/integration/test_dashboard.py index 1d0d003f..b2cff1fc 100644 --- a/umap/tests/integration/test_dashboard.py +++ b/umap/tests/integration/test_dashboard.py @@ -22,7 +22,7 @@ def test_owner_can_delete_map_after_confirmation(map, live_server, login): with page.expect_navigation(): delete_button.click() assert dialog_shown - assert Map.objects.all().count() == 0 + assert Map.objects.get(pk=map.pk).share_status == Map.DELETED def test_dashboard_map_preview(map, live_server, datalayer, login): diff --git a/umap/tests/integration/test_owned_map.py b/umap/tests/integration/test_owned_map.py index 2e87dda0..2eaa9e6a 100644 --- a/umap/tests/integration/test_owned_map.py +++ b/umap/tests/integration/test_owned_map.py @@ -141,7 +141,7 @@ def test_owner_has_delete_map_button(map, live_server, login): delete.click() with page.expect_navigation(): page.get_by_role("button", name="OK").click() - assert Map.objects.all().count() == 0 + assert Map.objects.get(pk=map.pk).share_status == Map.DELETED def test_editor_do_not_have_delete_map_button(map, live_server, login, user): diff --git a/umap/tests/test_datalayer.py b/umap/tests/test_datalayer.py index 1019ff58..9ef18d20 100644 --- a/umap/tests/test_datalayer.py +++ b/umap/tests/test_datalayer.py @@ -273,7 +273,6 @@ def test_anonymous_can_edit_in_inherit_mode_and_map_in_public_mode( def test_should_remove_all_versions_on_delete(map, settings): - settings.UMAP_PURGATORY_ROOT = tempfile.mkdtemp() datalayer = DataLayerFactory(uuid="0f1161c0-c07f-4ba4-86c5-8d8981d8a813", old_id=17) root = Path(datalayer.storage_root()) before = len(datalayer.geojson.storage.listdir(root)[1]) @@ -292,4 +291,3 @@ def test_should_remove_all_versions_on_delete(map, settings): datalayer.delete() found = datalayer.geojson.storage.listdir(root)[1] assert found == [other, f"{other}.gz"] - assert len(list(Path(settings.UMAP_PURGATORY_ROOT).iterdir())) == 4 + before diff --git a/umap/tests/test_empty_trash.py b/umap/tests/test_empty_trash.py new file mode 100644 index 00000000..457b1e55 --- /dev/null +++ b/umap/tests/test_empty_trash.py @@ -0,0 +1,34 @@ +from datetime import UTC, datetime, timedelta +from unittest import mock + +import pytest +from django.core.management import call_command + +from umap.models import Map + +from .base import MapFactory + +pytestmark = pytest.mark.django_db + + +def test_purge_purgatory(user): + recent = MapFactory(owner=user) + recent_deleted = MapFactory(owner=user) + recent_deleted.move_to_trash() + recent_deleted.save() + with mock.patch("django.utils.timezone.now") as mocked: + mocked.return_value = datetime.now(UTC) - timedelta(days=8) + old_deleted = MapFactory(owner=user) + old_deleted.move_to_trash() + old_deleted.save() + old = MapFactory(owner=user) + assert Map.objects.count() == 4 + call_command("empty_trash", "--days=7", "--dry-run") + assert Map.objects.count() == 4 + call_command("empty_trash", "--days=9") + assert Map.objects.count() == 4 + call_command("empty_trash", "--days=7") + assert not Map.objects.filter(pk=old_deleted.pk) + assert Map.objects.filter(pk=old.pk) + assert Map.objects.filter(pk=recent.pk) + assert Map.objects.filter(pk=recent_deleted.pk) diff --git a/umap/tests/test_map.py b/umap/tests/test_map.py index d9c60dac..e02095b3 100644 --- a/umap/tests/test_map.py +++ b/umap/tests/test_map.py @@ -167,3 +167,10 @@ def test_can_change_default_share_status(user, settings): map = Map.objects.create(owner=user, center=DEFAULT_CENTER) map = MapFactory(owner=user) assert map.share_status == Map.PUBLIC + + +def test_move_to_trash(user, map): + map.move_to_trash() + map.save() + reloaded = Map.objects.get(pk=map.pk) + assert reloaded.share_status == Map.DELETED diff --git a/umap/tests/test_map_views.py b/umap/tests/test_map_views.py index c1b0e7b4..90b70d44 100644 --- a/umap/tests/test_map_views.py +++ b/umap/tests/test_map_views.py @@ -114,8 +114,10 @@ def test_delete(client, map, datalayer): url, headers={"X-Requested-With": "XMLHttpRequest"}, follow=True ) assert response.status_code == 200 - assert not Map.objects.filter(pk=map.pk).exists() - assert not DataLayer.objects.filter(pk=datalayer.pk).exists() + assert Map.objects.filter(pk=map.pk).exists() + assert DataLayer.objects.filter(pk=datalayer.pk).exists() + reloaded = Map.objects.get(pk=map.pk) + assert reloaded.share_status == Map.DELETED # Check that user has not been impacted assert User.objects.filter(pk=map.owner.pk).exists() # Test response is a json @@ -406,7 +408,9 @@ def test_anonymous_delete(cookieclient, anonymap): url, headers={"X-Requested-With": "XMLHttpRequest"}, follow=True ) assert response.status_code == 200 - assert not Map.objects.filter(pk=anonymap.pk).count() + assert Map.objects.filter(pk=anonymap.pk).exists() + reloaded = Map.objects.get(pk=anonymap.pk) + assert reloaded.share_status == Map.DELETED # Test response is a json j = json.loads(response.content.decode()) assert "redirect" in j diff --git a/umap/tests/test_purge_purgatory.py b/umap/tests/test_purge_purgatory.py deleted file mode 100644 index d4d29e43..00000000 --- a/umap/tests/test_purge_purgatory.py +++ /dev/null @@ -1,25 +0,0 @@ -import os -import tempfile -from pathlib import Path - -from django.core.management import call_command - - -def test_purge_purgatory(settings): - settings.UMAP_PURGATORY_ROOT = tempfile.mkdtemp() - root = Path(settings.UMAP_PURGATORY_ROOT) - old = root / "old.json" - old.write_text("{}") - stat = old.stat() - os.utime(old, times=(stat.st_mtime - 31 * 86400, stat.st_mtime - 31 * 86400)) - recent = root / "recent.json" - recent.write_text("{}") - stat = recent.stat() - os.utime(recent, times=(stat.st_mtime - 8 * 86400, stat.st_mtime - 8 * 86400)) - now = root / "now.json" - now.write_text("{}") - assert {f.name for f in root.iterdir()} == {"old.json", "recent.json", "now.json"} - call_command("purge_purgatory") - assert {f.name for f in root.iterdir()} == {"recent.json", "now.json"} - call_command("purge_purgatory", "--days=7") - assert {f.name for f in root.iterdir()} == {"now.json"} diff --git a/umap/views.py b/umap/views.py index fcc1db33..e2c166ef 100644 --- a/umap/views.py +++ b/umap/views.py @@ -1022,7 +1022,7 @@ class MapDelete(DeleteView): self.object = self.get_object() if not self.object.can_delete(self.request): return HttpResponseForbidden(_("Only its owner can delete the map.")) - self.object.delete() + self.object.move_to_trash() home_url = reverse("home") messages.info(self.request, _("Map successfully deleted.")) if is_ajax(self.request):