From c925e7938d48935af90bfac32731c37672efa67f Mon Sep 17 00:00:00 2001 From: hannah cushman garland Date: Wed, 6 Sep 2023 13:54:35 -0500 Subject: [PATCH 1/4] Make failsafe configurable and remove redundant option --- pupa/cli/commands/clean.py | 56 ++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 33 deletions(-) diff --git a/pupa/cli/commands/clean.py b/pupa/cli/commands/clean.py index f00fe51..355f2c3 100644 --- a/pupa/cli/commands/clean.py +++ b/pupa/cli/commands/clean.py @@ -31,6 +31,12 @@ def add_args(self): "objects not seen in this many days will be deleted from the database" ), ) + self.add_argument( + "--max", + type=int, + default=10, + help="max number of objects to delete without triggering failsafe", + ) self.add_argument( "--report", action="store_true", @@ -39,11 +45,6 @@ def add_args(self): " would delete without making any changes to the database" ), ) - self.add_argument( - "--noinput", - action="store_true", - help="delete objects without getting user confirmation", - ) self.add_argument( "--yes", action="store_true", @@ -97,34 +98,23 @@ def handle(self, args, other): stale_objects = list(self.get_stale_objects(args.window)) num_stale_objects = len(stale_objects) - if args.noinput and args.yes: - self.remove_stale_objects(args.window) - sys.exit() - - if args.noinput: - # Fail-safe to avoid deleting a large amount of objects - # without explicit confimation - if num_stale_objects > 10: - print( - f"This command would delete {num_stale_objects} objects: " - f"\n{stale_objects}" - "\nIf you're sure, re-run without --noinput to provide confirmation." - "\nOr re-run with --yes to assume a yes answer to all prompts." - ) - sys.exit(1) - else: + print( + f"{num_stale_objects} objects in your database have not been seen " + f"in {args.window} days:\n{stale_objects}" + ) + + if num_stale_objects > args.max: print( - f"This will permanently delete" - f" {num_stale_objects} objects from your database" - f" that have not been scraped within the last {args.window}" - " days. Are you sure? (Y/N)" + f"WARNING: {num_stale_objects} exceeds the failsafe limit of {args.max}." ) - resp = input() - if resp != "Y": - sys.exit() - print( - "Removing objects that haven't been seen in a scrape within" - f" the last {args.window} days..." - ) - self.remove_stale_objects(args.window) + if args.yes: + print("Proceeding to deletion because you specified --yes.") + + else: + print(f"Permanently delete {num_stale_objects} objects? [Y/n]") + response = input() + + if args.yes or response == "Y": + self.remove_stale_objects(args.window) + print(f"Removed {num_stale_objects} from your database.") From 0573525cd5cd0f79ded06e6d1afa3024b874a92f Mon Sep 17 00:00:00 2001 From: hannah cushman garland Date: Wed, 6 Sep 2023 14:42:20 -0500 Subject: [PATCH 2/4] =?UTF-8?q?flake=F0=9F=8E=B1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- pupa/cli/commands/clean.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pupa/cli/commands/clean.py b/pupa/cli/commands/clean.py index 355f2c3..61e7f30 100644 --- a/pupa/cli/commands/clean.py +++ b/pupa/cli/commands/clean.py @@ -1,4 +1,3 @@ -import sys from datetime import datetime, timezone, timedelta import django From 9de55a08290b5e00f8ed289ca00dc196eee9a460 Mon Sep 17 00:00:00 2001 From: Hannah Cushman Garland Date: Wed, 17 Jul 2024 14:45:56 -0500 Subject: [PATCH 3/4] Always exit if failsafe is exceeded --- pupa/cli/commands/clean.py | 5 +- pupa/tests/clean/test_clean.py | 105 +++++++++++++++++++-------------- 2 files changed, 66 insertions(+), 44 deletions(-) diff --git a/pupa/cli/commands/clean.py b/pupa/cli/commands/clean.py index 61e7f30..f0fd2ee 100644 --- a/pupa/cli/commands/clean.py +++ b/pupa/cli/commands/clean.py @@ -1,4 +1,5 @@ from datetime import datetime, timezone, timedelta +import sys import django from django.apps import apps @@ -104,8 +105,10 @@ def handle(self, args, other): if num_stale_objects > args.max: print( - f"WARNING: {num_stale_objects} exceeds the failsafe limit of {args.max}." + f"{num_stale_objects} exceeds the failsafe limit of {args.max}. " + "Run this command with a larger --max value to proceed." ) + sys.exit() if args.yes: print("Proceeding to deletion because you specified --yes.") diff --git a/pupa/tests/clean/test_clean.py b/pupa/tests/clean/test_clean.py index d78df27..0ca9e32 100644 --- a/pupa/tests/clean/test_clean.py +++ b/pupa/tests/clean/test_clean.py @@ -24,40 +24,58 @@ def subparsers(): return parser.add_subparsers(dest="subcommand") -def create_jurisdiction(): +@pytest.fixture +def jurisdiction(): Division.objects.create(id="ocd-division/country:us", name="USA") return Jurisdiction.objects.create(id="jid", division_id="ocd-division/country:us") +@pytest.fixture +def organization(jurisdiction): + return Organization.objects.create(name="WWE", jurisdiction_id="jid") + + +@pytest.fixture +def person(): + class PersonFactory: + def build(self, **kwargs): + person_info = { + "name": "George Washington", + "family_name": "Washington", + } + + person_info.update(kwargs) + + return Person.objects.create(**person_info) + + return PersonFactory() + + @pytest.mark.django_db -def test_get_stale_objects(subparsers): - _ = create_jurisdiction() - o = Organization.objects.create(name="WWE", jurisdiction_id="jid") - p = Person.objects.create(name="George Washington", family_name="Washington") - m = p.memberships.create(organization=o) +def test_get_stale_objects(subparsers, organization, person): + stale_person = person.build() + membership = stale_person.memberships.create(organization=organization) - expected_stale_objects = {p, o, m} + expected_stale_objects = {stale_person, organization, membership} a_week_from_now = datetime.now(tz=timezone.utc) + timedelta(days=7) with freeze_time(a_week_from_now): - p = Person.objects.create(name="Thomas Jefferson", family_name="Jefferson") - p.memberships.create(organization=o) + fresh_person = person.build(name="Thomas Jefferson", family_name="Jefferson") + fresh_person.memberships.create(organization=organization) assert set(Command(subparsers).get_stale_objects(7)) == expected_stale_objects @pytest.mark.django_db -def test_remove_stale_objects(subparsers): - _ = create_jurisdiction() - o = Organization.objects.create(name="WWE", jurisdiction_id="jid") - p = Person.objects.create(name="George Washington", family_name="Washington") - m = p.memberships.create(organization=o) +def test_remove_stale_objects(subparsers, organization, person): + stale_person = person.build() + membership = stale_person.memberships.create(organization=organization) - expected_stale_objects = {p, o, m} + expected_stale_objects = {stale_person, organization, membership} a_week_from_now = datetime.now(tz=timezone.utc) + timedelta(days=7) with freeze_time(a_week_from_now): - p = Person.objects.create(name="Thomas Jefferson", family_name="Jefferson") - p.memberships.create(organization=o) + fresh_person = person.build(name="Thomas Jefferson", family_name="Jefferson") + fresh_person.memberships.create(organization=organization) Command(subparsers).remove_stale_objects(7) for obj in expected_stale_objects: @@ -66,26 +84,21 @@ def test_remove_stale_objects(subparsers): @pytest.mark.django_db -def test_clean_command(subparsers): - _ = create_jurisdiction() - o = Organization.objects.create(name="WWE", jurisdiction_id="jid") - - stale_person = Person.objects.create( - name="George Washington", family_name="Washington" - ) - stale_membership = stale_person.memberships.create(organization=o) +def test_clean_command(subparsers, organization, person): + stale_person = person.build() + stale_membership = stale_person.memberships.create(organization=organization) a_week_from_now = datetime.now(tz=timezone.utc) + timedelta(days=7) with freeze_time(a_week_from_now): - not_stale_person = Person.objects.create( - name="Thomas Jefferson", family_name="Jefferson" + fresh_person = person.build(name="Thomas Jefferson", family_name="Jefferson") + not_stale_membership = fresh_person.memberships.create( + organization=organization ) - not_stale_membership = not_stale_person.memberships.create(organization=o) - o.save() # Update org's last_seen field + organization.save() # Update org's last_seen field # Call clean command Command(subparsers).handle( - argparse.Namespace(noinput=True, report=False, window=7, yes=False), [] + argparse.Namespace(report=False, window=7, yes=True, max=10), [] ) expected_stale_objects = {stale_person, stale_membership} @@ -93,29 +106,35 @@ def test_clean_command(subparsers): was_deleted = not type(obj).objects.filter(id=obj.id).exists() assert was_deleted - expected_not_stale_objects = {o, not_stale_person, not_stale_membership} + expected_not_stale_objects = {organization, fresh_person, not_stale_membership} for obj in expected_not_stale_objects: was_not_deleted = type(obj).objects.filter(id=obj.id).exists() assert was_not_deleted @pytest.mark.django_db -def test_clean_command_failsafe(subparsers): - _ = create_jurisdiction() - o = Organization.objects.create(name="WWE", jurisdiction_id="jid") - - stale_people = [ - Person.objects.create(name="George Washington", family_name="Washington") - for i in range(20) - ] - stale_memberships = [ # noqa - p.memberships.create(organization=o) for p in stale_people - ] +def test_clean_command_failsafe(subparsers, organization, person): + stale_people = [person.build() for i in range(20)] + for p in stale_people: + p.memberships.create(organization=organization) a_week_from_now = datetime.now(tz=timezone.utc) + timedelta(days=7) with freeze_time(a_week_from_now): with pytest.raises(SystemExit): # Should trigger failsafe exist when deleting more than 10 objects Command(subparsers).handle( - argparse.Namespace(noinput=True, report=False, window=7, yes=False), [] + argparse.Namespace(report=False, window=7, yes=False, max=10), [] ) + + with pytest.raises(SystemExit): + # Should trigger failsafe exist when deleting more than 10 objects, + # even when yes is specified + Command(subparsers).handle( + argparse.Namespace(report=False, window=7, yes=True, max=10), [] + ) + + # Should proceed without error, since max is increased (1 organization, + # 20 people, 20 memberships) + Command(subparsers).handle( + argparse.Namespace(report=False, window=7, max=41, yes=True), [] + ) From 639124f817842ab775ed8c1975cc20c52c23eaf0 Mon Sep 17 00:00:00 2001 From: Hannah Cushman Garland Date: Wed, 17 Jul 2024 15:16:02 -0500 Subject: [PATCH 4/4] Don't report if a report is not requested --- pupa/cli/commands/clean.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pupa/cli/commands/clean.py b/pupa/cli/commands/clean.py index f0fd2ee..53429de 100644 --- a/pupa/cli/commands/clean.py +++ b/pupa/cli/commands/clean.py @@ -100,7 +100,7 @@ def handle(self, args, other): print( f"{num_stale_objects} objects in your database have not been seen " - f"in {args.window} days:\n{stale_objects}" + f"in {args.window} days." ) if num_stale_objects > args.max: