From d44011c65cad156e8882be7e50b253a355ff9d4a Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 10:10:40 +0300 Subject: [PATCH 1/9] Add patch for scheduling and revokation of autoapprove into test for reject --- .../tests/test_reject_moderation_request.py | 48 ++++++++++++++----- 1 file changed, 37 insertions(+), 11 deletions(-) diff --git a/BackEnd/profiles/tests/test_reject_moderation_request.py b/BackEnd/profiles/tests/test_reject_moderation_request.py index 6742ac234..85f654160 100644 --- a/BackEnd/profiles/tests/test_reject_moderation_request.py +++ b/BackEnd/profiles/tests/test_reject_moderation_request.py @@ -1,3 +1,5 @@ +from unittest.mock import patch, call, MagicMock + from rest_framework import status from rest_framework.test import APITestCase, APIClient @@ -11,6 +13,8 @@ from utils.dump_response import dump # noqa +@patch("profiles.views.ModerationManager.schedule_autoapprove") +@patch("profiles.views.ModerationManager.revoke_deprecated_autoapprove") class TestProfileModeration(APITestCase): def setUp(self) -> None: @@ -28,7 +32,7 @@ def setUp(self) -> None: self.unregistered_user_client = APIClient() - def test_reject_banner_and_logo(self): + def test_reject_banner_and_logo(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -69,8 +73,10 @@ def test_reject_banner_and_logo(self): self.assertEqual(self.profile.BLOCKED, self.profile.status) self.assertTrue(self.profile.is_deleted) self.assertFalse(self.user.is_active) + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_reject_banner(self): + def test_reject_banner(self, mock_revoke, mock_schedule): # user updates only banner self.user_client.patch( @@ -107,8 +113,10 @@ def test_reject_banner(self): self.assertEqual(self.profile.BLOCKED, self.profile.status) self.assertTrue(self.profile.is_deleted) self.assertFalse(self.user.is_active) + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_reject_logo(self): + def test_reject_logo(self, mock_revoke, mock_schedule): # user updates logo self.user_client.patch( @@ -145,8 +153,10 @@ def test_reject_logo(self): self.assertEqual(self.profile.BLOCKED, self.profile.status) self.assertTrue(self.profile.is_deleted) self.assertFalse(self.user.is_active) + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_reject_banner_and_logo_processed_request(self): + def test_reject_banner_and_logo_processed_request(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -193,8 +203,10 @@ def test_reject_banner_and_logo_processed_request(self): }, response.json(), ) + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_reject_banner_and_logo_outdated_request(self): + def test_reject_banner_and_logo_outdated_request(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -246,8 +258,10 @@ def test_reject_banner_and_logo_outdated_request(self): self.assertNotEqual(self.profile.banner, first_banner) self.assertNotEqual(self.profile.logo, first_logo) self.assertEqual(self.profile.PENDING, self.profile.status) + mock_schedule.assert_has_calls([call(), call()]) + mock_revoke.assert_not_called() - def test_reject_banner_and_logo_wrong_action(self): + def test_reject_banner_and_logo_wrong_action(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -277,8 +291,10 @@ def test_reject_banner_and_logo_wrong_action(self): self.assertEqual( {"action": ["Action is not allowed"]}, response.json() ) + mock_schedule.assert_called_once() + mock_revoke.assert_not_called() - def test_reject_banner_and_logo_error_in_signed_id(self): + def test_reject_banner_and_logo_error_in_signed_id(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -306,8 +322,10 @@ def test_reject_banner_and_logo_error_in_signed_id(self): self.assertEqual(status.HTTP_404_NOT_FOUND, response.status_code) self.assertEqual({"detail": "Not found."}, response.json()) + mock_schedule.assert_called_once() + mock_revoke.assert_not_called() - def test_reject_banner_and_logo_non_existing_profile(self): + def test_reject_banner_and_logo_non_existing_profile(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -335,8 +353,10 @@ def test_reject_banner_and_logo_non_existing_profile(self): self.assertEqual(status.HTTP_404_NOT_FOUND, response.status_code) self.assertEqual({"detail": "Not found."}, response.json()) + mock_schedule.assert_called_once() + mock_revoke.assert_not_called() - def test_reject_banner_and_logo_empty_image_fields(self): + def test_reject_banner_and_logo_empty_image_fields(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -368,8 +388,10 @@ def test_reject_banner_and_logo_empty_image_fields(self): }, response.json(), ) + mock_schedule.assert_called_once() + mock_revoke.assert_not_called() - def test_login_blocked_user_due_to_rejected_request(self): + def test_login_blocked_user_due_to_rejected_request(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -411,8 +433,10 @@ def test_login_blocked_user_due_to_rejected_request(self): {"non_field_errors": ["Profile has been blocked."]}, response.json(), ) + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_register_blocked_user_due_to_rejected_request(self): + def test_register_blocked_user_due_to_rejected_request(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( @@ -461,3 +485,5 @@ def test_register_blocked_user_due_to_rejected_request(self): {"email": ["Email is already registered"]}, response.json(), ) + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() From 1c6a842b5b27d8d5565902dd226f68431fc85922 Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 10:25:03 +0300 Subject: [PATCH 2/9] Add patch for revokation of autoapprove into test for approve --- .../tests/test_approve_moderation_request.py | 48 +++++++++++-------- 1 file changed, 29 insertions(+), 19 deletions(-) diff --git a/BackEnd/profiles/tests/test_approve_moderation_request.py b/BackEnd/profiles/tests/test_approve_moderation_request.py index e4feda0b7..204924198 100644 --- a/BackEnd/profiles/tests/test_approve_moderation_request.py +++ b/BackEnd/profiles/tests/test_approve_moderation_request.py @@ -1,4 +1,4 @@ -from unittest.mock import patch +from unittest.mock import patch, call, MagicMock from rest_framework import status from rest_framework.test import APITestCase, APIClient @@ -14,6 +14,7 @@ @patch("profiles.views.ModerationManager.schedule_autoapprove") +@patch("profiles.views.ModerationManager.revoke_deprecated_autoapprove") class TestProfileModeration(APITestCase): def setUp(self) -> None: self.banner = ProfileimageFactory(image_type="banner") @@ -28,7 +29,7 @@ def setUp(self) -> None: self.moderator_client = APIClient() - def test_approve_banner_and_logo(self, mock_manager): + def test_approve_banner_and_logo(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -67,9 +68,10 @@ def test_approve_banner_and_logo(self, mock_manager): self.assertEqual(self.profile.banner_approved, self.profile.banner) self.assertEqual(self.profile.logo_approved, self.profile.logo) self.assertEqual(self.profile.APPROVED, self.profile.status) - mock_manager.assert_called_once() + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_approve_banner(self, mock_manager): + def test_approve_banner(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates only banner self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -103,9 +105,10 @@ def test_approve_banner(self, mock_manager): self.assertTrue(self.banner.is_approved) self.assertEqual(self.profile.banner_approved, self.profile.banner) self.assertEqual(self.profile.APPROVED, self.profile.status) - mock_manager.assert_called_once() + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_approve_logo(self, mock_manager): + def test_approve_logo(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -139,9 +142,10 @@ def test_approve_logo(self, mock_manager): self.assertTrue(self.logo.is_approved) self.assertEqual(self.profile.logo_approved, self.profile.logo) self.assertEqual(self.profile.APPROVED, self.profile.status) - mock_manager.assert_called_once() + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_approve_banner_and_logo_processed_request(self, mock_manager): + def test_approve_banner_and_logo_processed_request(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -187,9 +191,10 @@ def test_approve_banner_and_logo_processed_request(self, mock_manager): }, response.json(), ) - mock_manager.assert_called_once() + mock_schedule.assert_called_once() + mock_revoke.assert_called_once() - def test_approve_banner_and_logo_outdated_request(self, mock_manager): + def test_approve_banner_and_logo_outdated_request(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -242,9 +247,10 @@ def test_approve_banner_and_logo_outdated_request(self, mock_manager): self.assertNotEqual(self.profile.banner, first_banner) self.assertNotEqual(self.profile.logo, first_logo) self.assertEqual(self.profile.PENDING, self.profile.status) - mock_manager.assert_called() + mock_schedule.assert_has_calls([call(), call()]) + mock_revoke.assert_not_called() - def test_approve_banner_and_logo_wrong_action(self, mock_manager): + def test_approve_banner_and_logo_wrong_action(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -273,9 +279,10 @@ def test_approve_banner_and_logo_wrong_action(self, mock_manager): self.assertEqual( {"action": ["Action is not allowed"]}, response.json() ) - mock_manager.assert_called_once() + mock_schedule.assert_called_once() + mock_revoke.assert_not_called() - def test_approve_banner_and_logo_error_in_signed_id(self, mock_manager): + def test_approve_banner_and_logo_error_in_signed_id(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -302,9 +309,10 @@ def test_approve_banner_and_logo_error_in_signed_id(self, mock_manager): self.assertEqual(status.HTTP_404_NOT_FOUND, response.status_code) self.assertEqual({"detail": "Not found."}, response.json()) - mock_manager.assert_called_once() + mock_schedule.assert_called_once() + mock_revoke.assert_not_called() - def test_approve_banner_and_logo_non_existing_profile(self, mock_manager): + def test_approve_banner_and_logo_non_existing_profile(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -331,9 +339,10 @@ def test_approve_banner_and_logo_non_existing_profile(self, mock_manager): self.assertEqual(status.HTTP_404_NOT_FOUND, response.status_code) self.assertEqual({"detail": "Not found."}, response.json()) - mock_manager.assert_called_once() + mock_schedule.assert_called_once() + mock_revoke.assert_not_called() - def test_approve_banner_and_logo_empty_image_fields(self, mock_manager): + def test_approve_banner_and_logo_empty_image_fields(self, mock_revoke: MagicMock, mock_schedule: MagicMock): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -365,4 +374,5 @@ def test_approve_banner_and_logo_empty_image_fields(self, mock_manager): }, response.json(), ) - mock_manager.assert_called_once() + mock_schedule.assert_called_once() + mock_revoke.assert_not_called() From 53844c7b5b1a63e15ca6cface8447a7190866b46 Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 10:29:19 +0300 Subject: [PATCH 3/9] Removed redundant type hints and imports from tests --- .../tests/test_approve_moderation_request.py | 20 +++++++++---------- .../tests/test_reject_moderation_request.py | 2 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/BackEnd/profiles/tests/test_approve_moderation_request.py b/BackEnd/profiles/tests/test_approve_moderation_request.py index 204924198..c18c194dc 100644 --- a/BackEnd/profiles/tests/test_approve_moderation_request.py +++ b/BackEnd/profiles/tests/test_approve_moderation_request.py @@ -1,4 +1,4 @@ -from unittest.mock import patch, call, MagicMock +from unittest.mock import patch, call from rest_framework import status from rest_framework.test import APITestCase, APIClient @@ -29,7 +29,7 @@ def setUp(self) -> None: self.moderator_client = APIClient() - def test_approve_banner_and_logo(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_banner_and_logo(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -71,7 +71,7 @@ def test_approve_banner_and_logo(self, mock_revoke: MagicMock, mock_schedule: Ma mock_schedule.assert_called_once() mock_revoke.assert_called_once() - def test_approve_banner(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_banner(self, mock_revoke, mock_schedule): # user updates only banner self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -108,7 +108,7 @@ def test_approve_banner(self, mock_revoke: MagicMock, mock_schedule: MagicMock): mock_schedule.assert_called_once() mock_revoke.assert_called_once() - def test_approve_logo(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_logo(self, mock_revoke, mock_schedule): # user updates logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -145,7 +145,7 @@ def test_approve_logo(self, mock_revoke: MagicMock, mock_schedule: MagicMock): mock_schedule.assert_called_once() mock_revoke.assert_called_once() - def test_approve_banner_and_logo_processed_request(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_banner_and_logo_processed_request(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -194,7 +194,7 @@ def test_approve_banner_and_logo_processed_request(self, mock_revoke: MagicMock, mock_schedule.assert_called_once() mock_revoke.assert_called_once() - def test_approve_banner_and_logo_outdated_request(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_banner_and_logo_outdated_request(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -250,7 +250,7 @@ def test_approve_banner_and_logo_outdated_request(self, mock_revoke: MagicMock, mock_schedule.assert_has_calls([call(), call()]) mock_revoke.assert_not_called() - def test_approve_banner_and_logo_wrong_action(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_banner_and_logo_wrong_action(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -282,7 +282,7 @@ def test_approve_banner_and_logo_wrong_action(self, mock_revoke: MagicMock, mock mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_approve_banner_and_logo_error_in_signed_id(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_banner_and_logo_error_in_signed_id(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -312,7 +312,7 @@ def test_approve_banner_and_logo_error_in_signed_id(self, mock_revoke: MagicMock mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_approve_banner_and_logo_non_existing_profile(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_banner_and_logo_non_existing_profile(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -342,7 +342,7 @@ def test_approve_banner_and_logo_non_existing_profile(self, mock_revoke: MagicMo mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_approve_banner_and_logo_empty_image_fields(self, mock_revoke: MagicMock, mock_schedule: MagicMock): + def test_approve_banner_and_logo_empty_image_fields(self, mock_revoke, mock_schedule): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( diff --git a/BackEnd/profiles/tests/test_reject_moderation_request.py b/BackEnd/profiles/tests/test_reject_moderation_request.py index 85f654160..b9c670632 100644 --- a/BackEnd/profiles/tests/test_reject_moderation_request.py +++ b/BackEnd/profiles/tests/test_reject_moderation_request.py @@ -1,4 +1,4 @@ -from unittest.mock import patch, call, MagicMock +from unittest.mock import patch, call from rest_framework import status from rest_framework.test import APITestCase, APIClient From c957239b570f9f35964750682575ba90dc244122 Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 11:16:46 +0300 Subject: [PATCH 4/9] Correct control flow in ModerationManager.schedule_autoapprove(), add db task record deletion after task completion --- BackEnd/profiles/tasks.py | 4 ++ .../tests/test_approve_moderation_request.py | 24 ++++++-- .../tests/test_reject_moderation_request.py | 56 ++++++++++--------- BackEnd/utils/moderation/image_moderation.py | 3 +- 4 files changed, 54 insertions(+), 33 deletions(-) diff --git a/BackEnd/profiles/tasks.py b/BackEnd/profiles/tasks.py index 26ae6f508..719bd31ed 100644 --- a/BackEnd/profiles/tasks.py +++ b/BackEnd/profiles/tasks.py @@ -1,5 +1,6 @@ from celery import shared_task +from administration.models import AutoapproveTask from .models import Profile from images.models import ProfileImage from utils.completeness_counter import completeness_count @@ -23,3 +24,6 @@ def celery_autoapprove(profile_id, banner_uuid, logo_uuid): profile.status = profile.AUTOAPPROVED profile.save() completeness_count(profile) + deprecated_record = AutoapproveTask.objects.filter(profile=profile).first() + if deprecated_record: + deprecated_record.delete() diff --git a/BackEnd/profiles/tests/test_approve_moderation_request.py b/BackEnd/profiles/tests/test_approve_moderation_request.py index c18c194dc..1ef7226d6 100644 --- a/BackEnd/profiles/tests/test_approve_moderation_request.py +++ b/BackEnd/profiles/tests/test_approve_moderation_request.py @@ -145,7 +145,9 @@ def test_approve_logo(self, mock_revoke, mock_schedule): mock_schedule.assert_called_once() mock_revoke.assert_called_once() - def test_approve_banner_and_logo_processed_request(self, mock_revoke, mock_schedule): + def test_approve_banner_and_logo_processed_request( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -194,7 +196,9 @@ def test_approve_banner_and_logo_processed_request(self, mock_revoke, mock_sched mock_schedule.assert_called_once() mock_revoke.assert_called_once() - def test_approve_banner_and_logo_outdated_request(self, mock_revoke, mock_schedule): + def test_approve_banner_and_logo_outdated_request( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -250,7 +254,9 @@ def test_approve_banner_and_logo_outdated_request(self, mock_revoke, mock_schedu mock_schedule.assert_has_calls([call(), call()]) mock_revoke.assert_not_called() - def test_approve_banner_and_logo_wrong_action(self, mock_revoke, mock_schedule): + def test_approve_banner_and_logo_wrong_action( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -282,7 +288,9 @@ def test_approve_banner_and_logo_wrong_action(self, mock_revoke, mock_schedule): mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_approve_banner_and_logo_error_in_signed_id(self, mock_revoke, mock_schedule): + def test_approve_banner_and_logo_error_in_signed_id( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -312,7 +320,9 @@ def test_approve_banner_and_logo_error_in_signed_id(self, mock_revoke, mock_sche mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_approve_banner_and_logo_non_existing_profile(self, mock_revoke, mock_schedule): + def test_approve_banner_and_logo_non_existing_profile( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -342,7 +352,9 @@ def test_approve_banner_and_logo_non_existing_profile(self, mock_revoke, mock_sc mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_approve_banner_and_logo_empty_image_fields(self, mock_revoke, mock_schedule): + def test_approve_banner_and_logo_empty_image_fields( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( diff --git a/BackEnd/profiles/tests/test_reject_moderation_request.py b/BackEnd/profiles/tests/test_reject_moderation_request.py index b9c670632..bd820fbbd 100644 --- a/BackEnd/profiles/tests/test_reject_moderation_request.py +++ b/BackEnd/profiles/tests/test_reject_moderation_request.py @@ -17,7 +17,6 @@ @patch("profiles.views.ModerationManager.revoke_deprecated_autoapprove") class TestProfileModeration(APITestCase): def setUp(self) -> None: - self.banner = ProfileimageFactory(image_type="banner") self.logo = ProfileimageFactory(image_type="logo") self.second_banner = ProfileimageFactory(image_type="banner") @@ -33,7 +32,6 @@ def setUp(self) -> None: self.unregistered_user_client = APIClient() def test_reject_banner_and_logo(self, mock_revoke, mock_schedule): - # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -77,7 +75,6 @@ def test_reject_banner_and_logo(self, mock_revoke, mock_schedule): mock_revoke.assert_called_once() def test_reject_banner(self, mock_revoke, mock_schedule): - # user updates only banner self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -114,10 +111,9 @@ def test_reject_banner(self, mock_revoke, mock_schedule): self.assertTrue(self.profile.is_deleted) self.assertFalse(self.user.is_active) mock_schedule.assert_called_once() - mock_revoke.assert_called_once() + mock_revoke.assert_called_once() def test_reject_logo(self, mock_revoke, mock_schedule): - # user updates logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -154,10 +150,11 @@ def test_reject_logo(self, mock_revoke, mock_schedule): self.assertTrue(self.profile.is_deleted) self.assertFalse(self.user.is_active) mock_schedule.assert_called_once() - mock_revoke.assert_called_once() - - def test_reject_banner_and_logo_processed_request(self, mock_revoke, mock_schedule): + mock_revoke.assert_called_once() + def test_reject_banner_and_logo_processed_request( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -204,10 +201,11 @@ def test_reject_banner_and_logo_processed_request(self, mock_revoke, mock_schedu response.json(), ) mock_schedule.assert_called_once() - mock_revoke.assert_called_once() - - def test_reject_banner_and_logo_outdated_request(self, mock_revoke, mock_schedule): + mock_revoke.assert_called_once() + def test_reject_banner_and_logo_outdated_request( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -259,10 +257,11 @@ def test_reject_banner_and_logo_outdated_request(self, mock_revoke, mock_schedul self.assertNotEqual(self.profile.logo, first_logo) self.assertEqual(self.profile.PENDING, self.profile.status) mock_schedule.assert_has_calls([call(), call()]) - mock_revoke.assert_not_called() - - def test_reject_banner_and_logo_wrong_action(self, mock_revoke, mock_schedule): + mock_revoke.assert_not_called() + def test_reject_banner_and_logo_wrong_action( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -294,8 +293,9 @@ def test_reject_banner_and_logo_wrong_action(self, mock_revoke, mock_schedule): mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_reject_banner_and_logo_error_in_signed_id(self, mock_revoke, mock_schedule): - + def test_reject_banner_and_logo_error_in_signed_id( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -325,8 +325,9 @@ def test_reject_banner_and_logo_error_in_signed_id(self, mock_revoke, mock_sched mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_reject_banner_and_logo_non_existing_profile(self, mock_revoke, mock_schedule): - + def test_reject_banner_and_logo_non_existing_profile( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -356,8 +357,9 @@ def test_reject_banner_and_logo_non_existing_profile(self, mock_revoke, mock_sch mock_schedule.assert_called_once() mock_revoke.assert_not_called() - def test_reject_banner_and_logo_empty_image_fields(self, mock_revoke, mock_schedule): - + def test_reject_banner_and_logo_empty_image_fields( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -389,10 +391,11 @@ def test_reject_banner_and_logo_empty_image_fields(self, mock_revoke, mock_sched response.json(), ) mock_schedule.assert_called_once() - mock_revoke.assert_not_called() - - def test_login_blocked_user_due_to_rejected_request(self, mock_revoke, mock_schedule): + mock_revoke.assert_not_called() + def test_login_blocked_user_due_to_rejected_request( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -436,8 +439,9 @@ def test_login_blocked_user_due_to_rejected_request(self, mock_revoke, mock_sche mock_schedule.assert_called_once() mock_revoke.assert_called_once() - def test_register_blocked_user_due_to_rejected_request(self, mock_revoke, mock_schedule): - + def test_register_blocked_user_due_to_rejected_request( + self, mock_revoke, mock_schedule + ): # user updates both banner and logo self.user_client.patch( path="/api/profiles/{profile_id}".format( @@ -486,4 +490,4 @@ def test_register_blocked_user_due_to_rejected_request(self, mock_revoke, mock_s response.json(), ) mock_schedule.assert_called_once() - mock_revoke.assert_called_once() + mock_revoke.assert_called_once() diff --git a/BackEnd/utils/moderation/image_moderation.py b/BackEnd/utils/moderation/image_moderation.py index ad7abf531..ab92261f8 100644 --- a/BackEnd/utils/moderation/image_moderation.py +++ b/BackEnd/utils/moderation/image_moderation.py @@ -66,9 +66,10 @@ def check_for_moderation(self): return self.moderation_is_needed def schedule_autoapprove(self): + """Make shure that you called check_for_moderation on your ModerationManager instance, before scheduling autoapprove.""" try: self.revoke_deprecated_autoapprove() - if self.needs_moderation and not self.content_deleted: + if self.moderation_is_needed and not self.content_deleted: banner = self.images.get("banner") logo = self.images.get("logo") banner_uuid = str(banner.uuid) if banner else None From c603171864cefc0fd393a042625e8aa5b44d7deb Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 11:51:08 +0300 Subject: [PATCH 5/9] Add tests for celery task and arguments passed into it, changed tag for redis image from latest to 7 --- .../profiles/tests/test_celery_autoapprove.py | 76 +++++++++++++++++++ 1 file changed, 76 insertions(+) create mode 100644 BackEnd/profiles/tests/test_celery_autoapprove.py diff --git a/BackEnd/profiles/tests/test_celery_autoapprove.py b/BackEnd/profiles/tests/test_celery_autoapprove.py new file mode 100644 index 000000000..913a57010 --- /dev/null +++ b/BackEnd/profiles/tests/test_celery_autoapprove.py @@ -0,0 +1,76 @@ +from unittest.mock import patch + +from rest_framework.test import APITestCase + +from authentication.factories import UserFactory +from profiles.factories import ( + ProfileCompanyFactory, +) +from administration.models import AutoModeration +from images.factories import ProfileimageFactory +from utils.unittest_helper import AnyInt + +@patch("utils.moderation.image_moderation.celery_autoapprove.apply_async") +class TestCelery(APITestCase): + def setUp(self) -> None: + self.banner = ProfileimageFactory(image_type="banner") + self.logo = ProfileimageFactory(image_type="logo") + self.user = UserFactory() + self.profile = ProfileCompanyFactory.create(person=self.user) + self.client.force_authenticate(self.user) + + + def test_autoapprove_banner_and_logo(self, mock_task): + mock_task.return_value.id = "sometaskid" + self.client.patch( + path="/api/profiles/{profile_id}".format( + profile_id=self.profile.id + ), + data={ + "banner": self.banner.uuid, + "logo": self.logo.uuid, + }, + ) + mock_task.assert_called_with((self.profile.id, self.banner.uuid, self.logo.uuid), countdown=AnyInt()) + + def test_autoapprove_only_banner(self, mock_task): + mock_task.return_value.id = "sometaskid" + self.client.patch( + path="/api/profiles/{profile_id}".format( + profile_id=self.profile.id + ), + data={ + "banner": self.banner.uuid, + }, + ) + mock_task.assert_called_with((self.profile.id, self.banner.uuid, None), countdown=AnyInt()) + + def test_autoapprove_only_logo(self, mock_task): + mock_task.return_value.id = "sometaskid" + self.client.patch( + path="/api/profiles/{profile_id}".format( + profile_id=self.profile.id + ), + data={ + "logo": self.logo.uuid, + }, + ) + mock_task.assert_called_with((self.profile.id, None, self.logo.uuid), countdown=AnyInt()) + + def test_change_autoapprove_time(self, mock_task): + test_value = 5 + autapprove_delay = AutoModeration.get_auto_moderation_hours() + autapprove_delay.auto_moderation_hours = test_value + autapprove_delay.save() + + mock_task.return_value.id = "sometaskid" + self.client.patch( + path="/api/profiles/{profile_id}".format( + profile_id=self.profile.id + ), + data={ + "banner": self.banner.uuid, + "logo": self.logo.uuid, + }, + ) + mock_task.assert_called_with((self.profile.id, self.banner.uuid, self.logo.uuid), countdown=test_value*60*60) \ No newline at end of file From 18aceae44190f4e8ec289f4e45b2eb4f5fdd85d6 Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 11:55:40 +0300 Subject: [PATCH 6/9] formatted with Black --- .../profiles/tests/test_celery_autoapprove.py | 36 ++++++++++++------- docker-compose.dev.yml | 2 +- 2 files changed, 24 insertions(+), 14 deletions(-) diff --git a/BackEnd/profiles/tests/test_celery_autoapprove.py b/BackEnd/profiles/tests/test_celery_autoapprove.py index 913a57010..83924e0ac 100644 --- a/BackEnd/profiles/tests/test_celery_autoapprove.py +++ b/BackEnd/profiles/tests/test_celery_autoapprove.py @@ -10,18 +10,18 @@ from images.factories import ProfileimageFactory from utils.unittest_helper import AnyInt + @patch("utils.moderation.image_moderation.celery_autoapprove.apply_async") class TestCelery(APITestCase): def setUp(self) -> None: self.banner = ProfileimageFactory(image_type="banner") self.logo = ProfileimageFactory(image_type="logo") self.user = UserFactory() - self.profile = ProfileCompanyFactory.create(person=self.user) + self.profile = ProfileCompanyFactory.create(person=self.user) self.client.force_authenticate(self.user) - - - def test_autoapprove_banner_and_logo(self, mock_task): - mock_task.return_value.id = "sometaskid" + + def test_autoapprove_banner_and_logo(self, mock_task): + mock_task.return_value.id = "sometaskid" self.client.patch( path="/api/profiles/{profile_id}".format( profile_id=self.profile.id @@ -31,10 +31,13 @@ def test_autoapprove_banner_and_logo(self, mock_task): "logo": self.logo.uuid, }, ) - mock_task.assert_called_with((self.profile.id, self.banner.uuid, self.logo.uuid), countdown=AnyInt()) + mock_task.assert_called_with( + (self.profile.id, self.banner.uuid, self.logo.uuid), + countdown=AnyInt(), + ) def test_autoapprove_only_banner(self, mock_task): - mock_task.return_value.id = "sometaskid" + mock_task.return_value.id = "sometaskid" self.client.patch( path="/api/profiles/{profile_id}".format( profile_id=self.profile.id @@ -43,10 +46,12 @@ def test_autoapprove_only_banner(self, mock_task): "banner": self.banner.uuid, }, ) - mock_task.assert_called_with((self.profile.id, self.banner.uuid, None), countdown=AnyInt()) + mock_task.assert_called_with( + (self.profile.id, self.banner.uuid, None), countdown=AnyInt() + ) def test_autoapprove_only_logo(self, mock_task): - mock_task.return_value.id = "sometaskid" + mock_task.return_value.id = "sometaskid" self.client.patch( path="/api/profiles/{profile_id}".format( profile_id=self.profile.id @@ -55,15 +60,17 @@ def test_autoapprove_only_logo(self, mock_task): "logo": self.logo.uuid, }, ) - mock_task.assert_called_with((self.profile.id, None, self.logo.uuid), countdown=AnyInt()) + mock_task.assert_called_with( + (self.profile.id, None, self.logo.uuid), countdown=AnyInt() + ) def test_change_autoapprove_time(self, mock_task): test_value = 5 - autapprove_delay = AutoModeration.get_auto_moderation_hours() + autapprove_delay = AutoModeration.get_auto_moderation_hours() autapprove_delay.auto_moderation_hours = test_value autapprove_delay.save() - mock_task.return_value.id = "sometaskid" + mock_task.return_value.id = "sometaskid" self.client.patch( path="/api/profiles/{profile_id}".format( profile_id=self.profile.id @@ -73,4 +80,7 @@ def test_change_autoapprove_time(self, mock_task): "logo": self.logo.uuid, }, ) - mock_task.assert_called_with((self.profile.id, self.banner.uuid, self.logo.uuid), countdown=test_value*60*60) \ No newline at end of file + mock_task.assert_called_with( + (self.profile.id, self.banner.uuid, self.logo.uuid), + countdown=test_value * 60 * 60, + ) diff --git a/docker-compose.dev.yml b/docker-compose.dev.yml index 61ce28e05..0541c6f43 100644 --- a/docker-compose.dev.yml +++ b/docker-compose.dev.yml @@ -80,7 +80,7 @@ services: networks: - forum_network redis: - image: redis:latest + image: redis:7 container_name: redis ports: - 6379:6379 From 1d3721c0cc83248959d9ecb03d076db730071a09 Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 12:50:25 +0300 Subject: [PATCH 7/9] Add transaction.atomic context manager for celery task --- BackEnd/profiles/tasks.py | 38 +++++++++++--------- BackEnd/utils/moderation/image_moderation.py | 2 +- 2 files changed, 22 insertions(+), 18 deletions(-) diff --git a/BackEnd/profiles/tasks.py b/BackEnd/profiles/tasks.py index 719bd31ed..78fe533e8 100644 --- a/BackEnd/profiles/tasks.py +++ b/BackEnd/profiles/tasks.py @@ -1,4 +1,5 @@ from celery import shared_task +from django.db import transaction from administration.models import AutoapproveTask from .models import Profile @@ -8,22 +9,25 @@ @shared_task def celery_autoapprove(profile_id, banner_uuid, logo_uuid): - profile = Profile.objects.get(pk=profile_id) - if banner_uuid: - banner = ProfileImage.objects.get(pk=banner_uuid) - banner.is_approved = True - profile.banner_approved = banner - banner.save() + with transaction.atomic(): + profile = Profile.objects.get(pk=profile_id) + if banner_uuid: + banner = ProfileImage.objects.get(pk=banner_uuid) + banner.is_approved = True + profile.banner_approved = banner + banner.save() - if logo_uuid: - logo = ProfileImage.objects.get(pk=logo_uuid) - logo.is_approved = True - profile.logo_approved = logo - logo.save() + if logo_uuid: + logo = ProfileImage.objects.get(pk=logo_uuid) + logo.is_approved = True + profile.logo_approved = logo + logo.save() - profile.status = profile.AUTOAPPROVED - profile.save() - completeness_count(profile) - deprecated_record = AutoapproveTask.objects.filter(profile=profile).first() - if deprecated_record: - deprecated_record.delete() + profile.status = profile.AUTOAPPROVED + profile.save() + completeness_count(profile) + deprecated_record = AutoapproveTask.objects.filter( + profile=profile + ).first() + if deprecated_record: + deprecated_record.delete() diff --git a/BackEnd/utils/moderation/image_moderation.py b/BackEnd/utils/moderation/image_moderation.py index ab92261f8..7f5292f38 100644 --- a/BackEnd/utils/moderation/image_moderation.py +++ b/BackEnd/utils/moderation/image_moderation.py @@ -79,7 +79,7 @@ def schedule_autoapprove(self): AutoModeration.get_auto_moderation_hours().auto_moderation_hours ) * 60 - * 60 + # * 60 ) result = celery_autoapprove.apply_async( (self.profile.id, banner_uuid, logo_uuid), countdown=delay From 8bf8d8142ef5975271bc4e9def6d2c06ea2f3dea Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 12:51:11 +0300 Subject: [PATCH 8/9] Fix autoapprove delay --- BackEnd/utils/moderation/image_moderation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/BackEnd/utils/moderation/image_moderation.py b/BackEnd/utils/moderation/image_moderation.py index 7f5292f38..ab92261f8 100644 --- a/BackEnd/utils/moderation/image_moderation.py +++ b/BackEnd/utils/moderation/image_moderation.py @@ -79,7 +79,7 @@ def schedule_autoapprove(self): AutoModeration.get_auto_moderation_hours().auto_moderation_hours ) * 60 - # * 60 + * 60 ) result = celery_autoapprove.apply_async( (self.profile.id, banner_uuid, logo_uuid), countdown=delay From 4950677684081af8be8918da4da5adab5cc58136 Mon Sep 17 00:00:00 2001 From: Yan Zhylavy Date: Mon, 2 Sep 2024 19:32:12 +0300 Subject: [PATCH 9/9] Add prevention of email sending and automoderation restart when updating non-image profile fields while profile is 'pending' --- BackEnd/profiles/views.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/BackEnd/profiles/views.py b/BackEnd/profiles/views.py index 4236a5f77..2c1830e38 100644 --- a/BackEnd/profiles/views.py +++ b/BackEnd/profiles/views.py @@ -25,6 +25,7 @@ from utils.moderation.handle_approved_images import ApprovedImagesDeleter from forum.pagination import ForumPagination +from images.models import ProfileImage from .models import SavedCompany, Profile, Category, Activity, Region from .permissions import ( UserIsProfileOwnerOrReadOnly, @@ -219,18 +220,22 @@ def perform_update(self, serializer): profile = serializer.save() SavedCompany.objects.filter(company=profile).update(is_updated=True) completeness_count(profile) - deletion_checker = ApprovedImagesDeleter(profile) - deletion_checker.handle_potential_deletion() - moderation_manager = ModerationManager(profile) if ( - moderation_manager.check_for_moderation() - or moderation_manager.content_deleted + ProfileImage.BANNER in serializer.validated_data.keys() + or ProfileImage.LOGO in serializer.validated_data.keys() ): - banner = moderation_manager.images["banner"] - logo = moderation_manager.images["logo"] - is_deleted = moderation_manager.content_deleted - send_moderation_email(profile, banner, logo, is_deleted) - moderation_manager.schedule_autoapprove() + deletion_checker = ApprovedImagesDeleter(profile) + deletion_checker.handle_potential_deletion() + moderation_manager = ModerationManager(profile) + if ( + moderation_manager.check_for_moderation() + or moderation_manager.content_deleted + ): + banner = moderation_manager.images["banner"] + logo = moderation_manager.images["logo"] + is_deleted = moderation_manager.content_deleted + send_moderation_email(profile, banner, logo, is_deleted) + moderation_manager.schedule_autoapprove() class ProfileViewCreate(CreateAPIView):