From 0815c99740e22a8316ccab8d29606a79d985c026 Mon Sep 17 00:00:00 2001 From: ReAnzu Date: Sat, 24 Feb 2018 01:57:31 -0600 Subject: [PATCH 1/5] Delete thumbnails and post source immediately on post delete --- server/szurubooru/func/posts.py | 14 +++++++++----- server/szurubooru/tests/api/test_post_deleting.py | 6 ++++-- server/szurubooru/tests/func/test_posts.py | 8 +++++--- server/szurubooru/tests/model/test_post.py | 8 ++++++++ server/szurubooru/tests/model/test_tag.py | 8 ++++++++ 5 files changed, 34 insertions(+), 10 deletions(-) diff --git a/server/szurubooru/func/posts.py b/server/szurubooru/func/posts.py index 406f6e18..25cd73a9 100644 --- a/server/szurubooru/func/posts.py +++ b/server/szurubooru/func/posts.py @@ -399,7 +399,9 @@ def _after_post_update( def _before_post_delete( _mapper: Any, _connection: Any, post: model.Post) -> None: if post.post_id: - image_hash.delete_image(post.post_id) + image_hash.delete_image(str(post.post_id)) + files.delete(get_post_content_path(post)) + files.delete(get_post_thumbnail_path(post)) def _sync_post_content(post: model.Post) -> None: @@ -686,12 +688,14 @@ def merge_posts( merge_favorites(source_post.post_id, target_post.post_id) merge_relations(source_post.post_id, target_post.post_id) - delete(source_post) - - db.session.flush() - + content = None if replace_content: content = files.get(get_post_content_path(source_post)) + + delete(source_post) + db.session.flush() + + if content is not None: update_post_content(target_post, content) diff --git a/server/szurubooru/tests/api/test_post_deleting.py b/server/szurubooru/tests/api/test_post_deleting.py index e35a4488..85744ad1 100644 --- a/server/szurubooru/tests/api/test_post_deleting.py +++ b/server/szurubooru/tests/api/test_post_deleting.py @@ -1,12 +1,14 @@ from unittest.mock import patch + import pytest + from szurubooru import api, db, model, errors -from szurubooru.func import posts, tags, snapshots +from szurubooru.func import posts, snapshots @pytest.fixture(autouse=True) def inject_config(config_injector): - config_injector({'privileges': {'posts:delete': model.User.RANK_REGULAR}}) + config_injector({'secret': 'secret', 'data_dir': '', 'privileges': {'posts:delete': model.User.RANK_REGULAR}}) def test_deleting(user_factory, post_factory, context_factory): diff --git a/server/szurubooru/tests/func/test_posts.py b/server/szurubooru/tests/func/test_posts.py index e296ec0a..c5570f0e 100644 --- a/server/szurubooru/tests/func/test_posts.py +++ b/server/szurubooru/tests/func/test_posts.py @@ -1,7 +1,9 @@ -import os -from unittest.mock import patch from datetime import datetime +from unittest.mock import patch + +import os import pytest + from szurubooru import db, model from szurubooru.func import ( posts, users, comments, tags, images, files, util, image_hash) @@ -936,6 +938,6 @@ def test_merge_posts_replaces_content( assert posts.try_get_post_by_id(source_post.post_id) is None post = posts.get_post_by_id(target_post.post_id) assert post is not None - assert os.path.exists(source_path) + assert not os.path.exists(source_path) assert os.path.exists(target_path1) assert not os.path.exists(target_path2) diff --git a/server/szurubooru/tests/model/test_post.py b/server/szurubooru/tests/model/test_post.py index f35e2751..2fffd87b 100644 --- a/server/szurubooru/tests/model/test_post.py +++ b/server/szurubooru/tests/model/test_post.py @@ -1,7 +1,15 @@ from datetime import datetime + +import pytest + from szurubooru import db, model +@pytest.fixture(autouse=True) +def inject_config(config_injector): + config_injector({'secret': 'secret', 'data_dir': ''}) + + def test_saving_post(post_factory, user_factory, tag_factory): user = user_factory() tag1 = tag_factory() diff --git a/server/szurubooru/tests/model/test_tag.py b/server/szurubooru/tests/model/test_tag.py index 07bbc0e5..9121f91c 100644 --- a/server/szurubooru/tests/model/test_tag.py +++ b/server/szurubooru/tests/model/test_tag.py @@ -1,7 +1,15 @@ from datetime import datetime + +import pytest + from szurubooru import db, model +@pytest.fixture(autouse=True) +def inject_config(config_injector): + config_injector({'secret': 'secret', 'data_dir': ''}) + + def test_saving_tag(tag_factory): sug1 = tag_factory(names=['sug1']) sug2 = tag_factory(names=['sug2']) From addd2afdaa9357d7d8471d1f37b5e48791ddb059 Mon Sep 17 00:00:00 2001 From: ReAnzu Date: Fri, 2 Mar 2018 23:13:32 -0600 Subject: [PATCH 2/5] Configuration gate for feature * Put source and thumbnail deletion functionality behind configuration * Updated tests with new configuration parameter --- config.yaml.dist | 6 ++ server/szurubooru/func/posts.py | 5 +- .../tests/api/test_post_deleting.py | 5 +- server/szurubooru/tests/func/test_posts.py | 59 +++++++++++++------ server/szurubooru/tests/model/test_post.py | 4 +- server/szurubooru/tests/model/test_tag.py | 3 +- 6 files changed, 60 insertions(+), 22 deletions(-) diff --git a/config.yaml.dist b/config.yaml.dist index 42267452..6193505d 100644 --- a/config.yaml.dist +++ b/config.yaml.dist @@ -20,6 +20,12 @@ database: test_database: 'sqlite:///:memory:' # required for running the test suite +# Delete thumbnails and source files on post delete +# Original functionality is false, to mitigate the impacts of admins going +# on unchecked post purges. +delete_source_files: no + + thumbnails: avatar_width: 300 avatar_height: 300 diff --git a/server/szurubooru/func/posts.py b/server/szurubooru/func/posts.py index 25cd73a9..e35e2e12 100644 --- a/server/szurubooru/func/posts.py +++ b/server/szurubooru/func/posts.py @@ -400,8 +400,9 @@ def _before_post_delete( _mapper: Any, _connection: Any, post: model.Post) -> None: if post.post_id: image_hash.delete_image(str(post.post_id)) - files.delete(get_post_content_path(post)) - files.delete(get_post_thumbnail_path(post)) + if config.config['delete_source_files']: + files.delete(get_post_content_path(post)) + files.delete(get_post_thumbnail_path(post)) def _sync_post_content(post: model.Post) -> None: diff --git a/server/szurubooru/tests/api/test_post_deleting.py b/server/szurubooru/tests/api/test_post_deleting.py index 85744ad1..7e588472 100644 --- a/server/szurubooru/tests/api/test_post_deleting.py +++ b/server/szurubooru/tests/api/test_post_deleting.py @@ -8,7 +8,10 @@ from szurubooru.func import posts, snapshots @pytest.fixture(autouse=True) def inject_config(config_injector): - config_injector({'secret': 'secret', 'data_dir': '', 'privileges': {'posts:delete': model.User.RANK_REGULAR}}) + config_injector({'secret': 'secret', + 'data_dir': '', + 'delete_source_files': False, + 'privileges': {'posts:delete': model.User.RANK_REGULAR}}) def test_deleting(user_factory, post_factory, context_factory): diff --git a/server/szurubooru/tests/func/test_posts.py b/server/szurubooru/tests/func/test_posts.py index c5570f0e..b4c41ade 100644 --- a/server/szurubooru/tests/func/test_posts.py +++ b/server/szurubooru/tests/func/test_posts.py @@ -677,7 +677,8 @@ def test_feature_post(post_factory, user_factory): assert new_featured_post == post -def test_delete(post_factory): +def test_delete(post_factory, config_injector): + config_injector({'delete_source_files': False}) post = post_factory() db.session.add(post) db.session.flush() @@ -687,7 +688,8 @@ def test_delete(post_factory): assert posts.get_post_count() == 0 -def test_merge_posts_deletes_source_post(post_factory): +def test_merge_posts_deletes_source_post(post_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() db.session.add_all([source_post, target_post]) @@ -699,7 +701,8 @@ def test_merge_posts_deletes_source_post(post_factory): assert post is not None -def test_merge_posts_with_itself(post_factory): +def test_merge_posts_with_itself(post_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() db.session.add(source_post) db.session.flush() @@ -707,7 +710,8 @@ def test_merge_posts_with_itself(post_factory): posts.merge_posts(source_post, source_post, False) -def test_merge_posts_moves_tags(post_factory, tag_factory): +def test_merge_posts_moves_tags(post_factory, tag_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() tag = tag_factory() @@ -722,7 +726,9 @@ def test_merge_posts_moves_tags(post_factory, tag_factory): assert posts.get_post_by_id(target_post.post_id).tag_count == 1 -def test_merge_posts_doesnt_duplicate_tags(post_factory, tag_factory): +def test_merge_posts_doesnt_duplicate_tags( + post_factory, tag_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() tag = tag_factory() @@ -737,7 +743,9 @@ def test_merge_posts_doesnt_duplicate_tags(post_factory, tag_factory): assert posts.get_post_by_id(target_post.post_id).tag_count == 1 -def test_merge_posts_moves_comments(post_factory, comment_factory): +def test_merge_posts_moves_comments( + post_factory, comment_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() comment = comment_factory(post=source_post) @@ -751,7 +759,9 @@ def test_merge_posts_moves_comments(post_factory, comment_factory): assert posts.get_post_by_id(target_post.post_id).comment_count == 1 -def test_merge_posts_moves_scores(post_factory, post_score_factory): +def test_merge_posts_moves_scores( + post_factory, post_score_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() score = post_score_factory(post=source_post, score=1) @@ -766,7 +776,8 @@ def test_merge_posts_moves_scores(post_factory, post_score_factory): def test_merge_posts_doesnt_duplicate_scores( - post_factory, user_factory, post_score_factory): + post_factory, user_factory, post_score_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() user = user_factory() @@ -782,7 +793,9 @@ def test_merge_posts_doesnt_duplicate_scores( assert posts.get_post_by_id(target_post.post_id).score == 1 -def test_merge_posts_moves_favorites(post_factory, post_favorite_factory): +def test_merge_posts_moves_favorites( + post_factory, post_favorite_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() favorite = post_favorite_factory(post=source_post) @@ -797,7 +810,8 @@ def test_merge_posts_moves_favorites(post_factory, post_favorite_factory): def test_merge_posts_doesnt_duplicate_favorites( - post_factory, user_factory, post_favorite_factory): + post_factory, user_factory, post_favorite_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() user = user_factory() @@ -813,7 +827,8 @@ def test_merge_posts_doesnt_duplicate_favorites( assert posts.get_post_by_id(target_post.post_id).favorite_count == 1 -def test_merge_posts_moves_child_relations(post_factory): +def test_merge_posts_moves_child_relations(post_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() related_post = post_factory() @@ -828,7 +843,9 @@ def test_merge_posts_moves_child_relations(post_factory): assert posts.get_post_by_id(target_post.post_id).relation_count == 1 -def test_merge_posts_doesnt_duplicate_child_relations(post_factory): +def test_merge_posts_doesnt_duplicate_child_relations(post_factory, + config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() related_post = post_factory() @@ -844,7 +861,8 @@ def test_merge_posts_doesnt_duplicate_child_relations(post_factory): assert posts.get_post_by_id(target_post.post_id).relation_count == 1 -def test_merge_posts_moves_parent_relations(post_factory): +def test_merge_posts_moves_parent_relations(post_factory, config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() related_post = post_factory() @@ -861,7 +879,9 @@ def test_merge_posts_moves_parent_relations(post_factory): assert posts.get_post_by_id(related_post.post_id).relation_count == 1 -def test_merge_posts_doesnt_duplicate_parent_relations(post_factory): +def test_merge_posts_doesnt_duplicate_parent_relations(post_factory, + config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() related_post = post_factory() @@ -878,7 +898,9 @@ def test_merge_posts_doesnt_duplicate_parent_relations(post_factory): assert posts.get_post_by_id(related_post.post_id).relation_count == 1 -def test_merge_posts_doesnt_create_relation_loop_for_children(post_factory): +def test_merge_posts_doesnt_create_relation_loop_for_children(post_factory, + config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() source_post.relations = [target_post] @@ -892,7 +914,9 @@ def test_merge_posts_doesnt_create_relation_loop_for_children(post_factory): assert posts.get_post_by_id(target_post.post_id).relation_count == 0 -def test_merge_posts_doesnt_create_relation_loop_for_parents(post_factory): +def test_merge_posts_doesnt_create_relation_loop_for_parents(post_factory, + config_injector): + config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() target_post.relations = [source_post] @@ -911,6 +935,7 @@ def test_merge_posts_replaces_content( config_injector({ 'data_dir': str(tmpdir.mkdir('data')), 'data_url': 'example.com', + 'delete_source_files': False, 'thumbnails': { 'post_width': 300, 'post_height': 300, @@ -938,6 +963,6 @@ def test_merge_posts_replaces_content( assert posts.try_get_post_by_id(source_post.post_id) is None post = posts.get_post_by_id(target_post.post_id) assert post is not None - assert not os.path.exists(source_path) + assert os.path.exists(source_path) assert os.path.exists(target_path1) assert not os.path.exists(target_path2) diff --git a/server/szurubooru/tests/model/test_post.py b/server/szurubooru/tests/model/test_post.py index 2fffd87b..704c5f1c 100644 --- a/server/szurubooru/tests/model/test_post.py +++ b/server/szurubooru/tests/model/test_post.py @@ -7,7 +7,9 @@ from szurubooru import db, model @pytest.fixture(autouse=True) def inject_config(config_injector): - config_injector({'secret': 'secret', 'data_dir': ''}) + config_injector({'secret': 'secret', + 'data_dir': '', + 'delete_source_files': False}) def test_saving_post(post_factory, user_factory, tag_factory): diff --git a/server/szurubooru/tests/model/test_tag.py b/server/szurubooru/tests/model/test_tag.py index 9121f91c..c22a6ac2 100644 --- a/server/szurubooru/tests/model/test_tag.py +++ b/server/szurubooru/tests/model/test_tag.py @@ -7,7 +7,8 @@ from szurubooru import db, model @pytest.fixture(autouse=True) def inject_config(config_injector): - config_injector({'secret': 'secret', 'data_dir': ''}) + config_injector({'delete_source_files': False, + 'secret': 'secret', 'data_dir': ''}) def test_saving_tag(tag_factory): From 4dcbfbdc33501d709db0bbfcaf4b6b34d6f65350 Mon Sep 17 00:00:00 2001 From: ReAnzu Date: Wed, 7 Mar 2018 19:00:44 -0600 Subject: [PATCH 3/5] Resolved code formatting change requests --- config.yaml.dist | 2 +- server/szurubooru/func/posts.py | 2 +- server/szurubooru/tests/api/test_post_deleting.py | 12 ++++++++---- server/szurubooru/tests/func/test_posts.py | 14 ++++++-------- server/szurubooru/tests/model/test_post.py | 10 +++++----- server/szurubooru/tests/model/test_tag.py | 9 +++++---- 6 files changed, 26 insertions(+), 23 deletions(-) diff --git a/config.yaml.dist b/config.yaml.dist index 6193505d..d2b4e9c4 100644 --- a/config.yaml.dist +++ b/config.yaml.dist @@ -21,7 +21,7 @@ test_database: 'sqlite:///:memory:' # required for running the test suite # Delete thumbnails and source files on post delete -# Original functionality is false, to mitigate the impacts of admins going +# Original functionality is no, to mitigate the impacts of admins going # on unchecked post purges. delete_source_files: no diff --git a/server/szurubooru/func/posts.py b/server/szurubooru/func/posts.py index e35e2e12..86274616 100644 --- a/server/szurubooru/func/posts.py +++ b/server/szurubooru/func/posts.py @@ -399,7 +399,7 @@ def _after_post_update( def _before_post_delete( _mapper: Any, _connection: Any, post: model.Post) -> None: if post.post_id: - image_hash.delete_image(str(post.post_id)) + image_hash.delete_image(post.post_id) if config.config['delete_source_files']: files.delete(get_post_content_path(post)) files.delete(get_post_thumbnail_path(post)) diff --git a/server/szurubooru/tests/api/test_post_deleting.py b/server/szurubooru/tests/api/test_post_deleting.py index 7e588472..b5ca8319 100644 --- a/server/szurubooru/tests/api/test_post_deleting.py +++ b/server/szurubooru/tests/api/test_post_deleting.py @@ -8,10 +8,14 @@ from szurubooru.func import posts, snapshots @pytest.fixture(autouse=True) def inject_config(config_injector): - config_injector({'secret': 'secret', - 'data_dir': '', - 'delete_source_files': False, - 'privileges': {'posts:delete': model.User.RANK_REGULAR}}) + config_injector({ + 'secret': 'secret', + 'data_dir': '', + 'delete_source_files': False, + 'privileges': { + 'posts:delete': model.User.RANK_REGULAR + } + }) def test_deleting(user_factory, post_factory, context_factory): diff --git a/server/szurubooru/tests/func/test_posts.py b/server/szurubooru/tests/func/test_posts.py index b4c41ade..dd256574 100644 --- a/server/szurubooru/tests/func/test_posts.py +++ b/server/szurubooru/tests/func/test_posts.py @@ -1,9 +1,7 @@ from datetime import datetime from unittest.mock import patch - import os import pytest - from szurubooru import db, model from szurubooru.func import ( posts, users, comments, tags, images, files, util, image_hash) @@ -843,8 +841,8 @@ def test_merge_posts_moves_child_relations(post_factory, config_injector): assert posts.get_post_by_id(target_post.post_id).relation_count == 1 -def test_merge_posts_doesnt_duplicate_child_relations(post_factory, - config_injector): +def test_merge_posts_doesnt_duplicate_child_relations( + post_factory, config_injector): config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() @@ -879,8 +877,8 @@ def test_merge_posts_moves_parent_relations(post_factory, config_injector): assert posts.get_post_by_id(related_post.post_id).relation_count == 1 -def test_merge_posts_doesnt_duplicate_parent_relations(post_factory, - config_injector): +def test_merge_posts_doesnt_duplicate_parent_relations( + post_factory, config_injector): config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() @@ -898,8 +896,8 @@ def test_merge_posts_doesnt_duplicate_parent_relations(post_factory, assert posts.get_post_by_id(related_post.post_id).relation_count == 1 -def test_merge_posts_doesnt_create_relation_loop_for_children(post_factory, - config_injector): +def test_merge_posts_doesnt_create_relation_loop_for_children( + post_factory, config_injector): config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() diff --git a/server/szurubooru/tests/model/test_post.py b/server/szurubooru/tests/model/test_post.py index 704c5f1c..ee691460 100644 --- a/server/szurubooru/tests/model/test_post.py +++ b/server/szurubooru/tests/model/test_post.py @@ -1,15 +1,15 @@ from datetime import datetime - import pytest - from szurubooru import db, model @pytest.fixture(autouse=True) def inject_config(config_injector): - config_injector({'secret': 'secret', - 'data_dir': '', - 'delete_source_files': False}) + config_injector({ + 'secret': 'secret', + 'data_dir': '', + 'delete_source_files': False + }) def test_saving_post(post_factory, user_factory, tag_factory): diff --git a/server/szurubooru/tests/model/test_tag.py b/server/szurubooru/tests/model/test_tag.py index c22a6ac2..b677eeff 100644 --- a/server/szurubooru/tests/model/test_tag.py +++ b/server/szurubooru/tests/model/test_tag.py @@ -1,14 +1,15 @@ from datetime import datetime - import pytest - from szurubooru import db, model @pytest.fixture(autouse=True) def inject_config(config_injector): - config_injector({'delete_source_files': False, - 'secret': 'secret', 'data_dir': ''}) + config_injector({ + 'delete_source_files': False, + 'secret': 'secret', + 'data_dir': '' + }) def test_saving_tag(tag_factory): From 87140cadba1673e7650d53b05152c5e695f98f63 Mon Sep 17 00:00:00 2001 From: ReAnzu Date: Wed, 7 Mar 2018 19:53:15 -0600 Subject: [PATCH 4/5] Missed an indentation change. --- server/szurubooru/tests/func/test_posts.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/server/szurubooru/tests/func/test_posts.py b/server/szurubooru/tests/func/test_posts.py index dd256574..d0c27ba6 100644 --- a/server/szurubooru/tests/func/test_posts.py +++ b/server/szurubooru/tests/func/test_posts.py @@ -912,8 +912,8 @@ def test_merge_posts_doesnt_create_relation_loop_for_children( assert posts.get_post_by_id(target_post.post_id).relation_count == 0 -def test_merge_posts_doesnt_create_relation_loop_for_parents(post_factory, - config_injector): +def test_merge_posts_doesnt_create_relation_loop_for_parents( + post_factory, config_injector): config_injector({'delete_source_files': False}) source_post = post_factory() target_post = post_factory() From 19b41c20f254974d6fb4f0be86f6467efd29b302 Mon Sep 17 00:00:00 2001 From: ReAnzu Date: Thu, 8 Mar 2018 16:34:42 -0600 Subject: [PATCH 5/5] server/tests: Address code review comments --- server/szurubooru/tests/api/test_post_deleting.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/server/szurubooru/tests/api/test_post_deleting.py b/server/szurubooru/tests/api/test_post_deleting.py index b5ca8319..bb5f9ced 100644 --- a/server/szurubooru/tests/api/test_post_deleting.py +++ b/server/szurubooru/tests/api/test_post_deleting.py @@ -1,7 +1,5 @@ from unittest.mock import patch - import pytest - from szurubooru import api, db, model, errors from szurubooru.func import posts, snapshots