Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update pupa clean so that Jurisdiction, Division, and Post objects are not deleted #354

Merged
merged 2 commits into from
Sep 11, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pupa/cli/commands/clean.py
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,11 @@ def get_stale_objects(self, window):
ocd_apps = ["core", "legislative"]
# Check all subclasses of OCDBase
models = get_subclasses(ocd_apps, OCDBase)
# Top-level models are protected from deletion
protected_models = ("Division", "Jurisdiction", "Post")

for model in models:
# Jurisdictions are protected from deletion
if "Jurisdiction" not in model.__name__:
if model.__name__ not in protected_models:
cutoff_date = datetime.now(tz=timezone.utc) - timedelta(days=window)
yield from model.objects.filter(last_seen__lte=cutoff_date).iterator()

Expand Down
44 changes: 31 additions & 13 deletions pupa/tests/clean/test_clean.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@
from datetime import datetime, timezone, timedelta
from freezegun import freeze_time

from opencivicdata.core.models import Person, Organization, Jurisdiction, Division
from opencivicdata.core.models import Person, Organization, Jurisdiction, Division, Post

from pupa.cli.commands.clean import Command
from pupa.cli.commands.clean import Command as CleanCommand


@pytest.fixture
Expand All @@ -25,14 +25,23 @@ def subparsers():


@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")
def division():
return Division.objects.create(id="ocd-division/country:us", name="USA")


@pytest.fixture
def jurisdiction(division):
return Jurisdiction.objects.create(id="jid", division=division)


@pytest.fixture
def organization(jurisdiction):
return Organization.objects.create(name="WWE", jurisdiction_id="jid")
return Organization.objects.create(name="WWE", jurisdiction=jurisdiction)


@pytest.fixture
def post(organization):
return Post.objects.create(organization=organization, label="Some post", role="Some post")


@pytest.fixture
Expand All @@ -52,17 +61,24 @@ def build(self, **kwargs):


@pytest.mark.django_db
def test_get_stale_objects(subparsers, organization, person):
def test_get_stale_objects(subparsers, division, jurisdiction, organization, post, person):
stale_person = person.build()
membership = stale_person.memberships.create(organization=organization)

protected_objects = {division, jurisdiction, post}
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):
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

stale_objects = set(CleanCommand(subparsers).get_stale_objects(7))
assert stale_objects == expected_stale_objects

# This is implied by the above check, but it's important, so we'll check
# for it explicitly.
assert protected_objects not in stale_objects


@pytest.mark.django_db
Expand All @@ -77,7 +93,7 @@ def test_remove_stale_objects(subparsers, organization, person):
fresh_person = person.build(name="Thomas Jefferson", family_name="Jefferson")
fresh_person.memberships.create(organization=organization)

Command(subparsers).remove_stale_objects(7)
CleanCommand(subparsers).remove_stale_objects(7)
for obj in expected_stale_objects:
was_deleted = not type(obj).objects.filter(id=obj.id).exists()
assert was_deleted
Expand All @@ -97,7 +113,7 @@ def test_clean_command(subparsers, organization, person):
organization.save() # Update org's last_seen field

# Call clean command
Command(subparsers).handle(
CleanCommand(subparsers).handle(
argparse.Namespace(report=False, window=7, yes=True, max=10), []
)

Expand All @@ -118,23 +134,25 @@ def test_clean_command_failsafe(subparsers, organization, person):
for p in stale_people:
p.memberships.create(organization=organization)

cmd = CleanCommand(subparsers)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytest complained when this was instantiated multiple times.


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(
cmd.handle(
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(
cmd.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(
cmd.handle(
argparse.Namespace(report=False, window=7, max=41, yes=True), []
)
12 changes: 7 additions & 5 deletions pupa/tests/django_settings.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# django settings for tests
import os

SECRET_KEY = 'test'
INSTALLED_APPS = ('django.contrib.contenttypes',
'opencivicdata.core.apps.BaseConfig',
Expand All @@ -7,11 +9,11 @@
DATABASES = {
'default': {
'ENGINE': 'django.contrib.gis.db.backends.postgis',
'NAME': 'test',
'USER': 'test',
'PASSWORD': 'test',
'HOST': 'localhost',
'PORT': 5432,
'NAME': os.getenv('POSTGRES_DB', 'test'),
'USER': os.getenv('POSTGRES_USER', 'test'),
'PASSWORD': os.getenv('POSTGRES_PASSWORD', 'test'),
'HOST': os.getenv('POSTGRES_HOST', 'localhost'),
'PORT': os.getenv('POSTGRES_PORT', 5432),
Comment on lines +12 to +16
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allow for setting these as environment variables, useful for, e.g., if you are running Postgres in a container.

}
}
MIDDLEWARE_CLASSES = ()
2 changes: 1 addition & 1 deletion setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
entry_points='''[console_scripts]
pupa = pupa.cli.__main__:main''',
install_requires=[
'Django>=2.2',
'Django>=2.2,<5',
Copy link
Contributor Author

@hancush hancush Sep 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

index_together (used by python-opencivicdata, I guess) was removed in Django 5.

'opencivicdata>=3.3.0',
'dj_database_url>=0.3.0',
'scrapelib>=1.0',
Expand Down
Loading