From 020bdeb0585eb65624677951a97392091ce40ac7 Mon Sep 17 00:00:00 2001 From: Jennifer Richards Date: Thu, 30 May 2024 10:23:49 -0300 Subject: [PATCH] feat: purge_personal_api_key_events() task (#7485) * feat: purge_personal_api_key_events() task * feat: log number of events purged * test: test new task * fix: name task properly * chore: create daily PeriodicTask * chore: remove old management command * chore: remove tests of old command * test: finish removing now-empty tests.py --- bin/daily | 3 - .../purge_old_personal_api_key_events.py | 64 --------- ietf/person/management/commands/tests.py | 122 ------------------ ietf/person/tasks.py | 20 +++ ietf/person/tests.py | 19 ++- .../management/commands/periodic_tasks.py | 11 ++ 6 files changed, 48 insertions(+), 191 deletions(-) delete mode 100644 ietf/person/management/commands/purge_old_personal_api_key_events.py delete mode 100644 ietf/person/management/commands/tests.py create mode 100644 ietf/person/tasks.py diff --git a/bin/daily b/bin/daily index 6adb16794..b4ea33995 100755 --- a/bin/daily +++ b/bin/daily @@ -36,6 +36,3 @@ $DTDIR/ietf/manage.py populate_yang_model_dirs -v0 # Re-run yang checks on active documents $DTDIR/ietf/manage.py run_yang_model_checks -v0 - -# Purge older PersonApiKeyEvents -$DTDIR/ietf/manage.py purge_old_personal_api_key_events 14 diff --git a/ietf/person/management/commands/purge_old_personal_api_key_events.py b/ietf/person/management/commands/purge_old_personal_api_key_events.py deleted file mode 100644 index 66b9d2c33..000000000 --- a/ietf/person/management/commands/purge_old_personal_api_key_events.py +++ /dev/null @@ -1,64 +0,0 @@ -# Copyright The IETF Trust 2021, All Rights Reserved -# -*- coding: utf-8 -*- - -from datetime import timedelta -from django.core.management.base import BaseCommand, CommandError -from django.db.models import Max, Min -from django.utils import timezone - -from ietf.person.models import PersonApiKeyEvent - - -class Command(BaseCommand): - help = 'Purge PersonApiKeyEvent instances older than KEEP_DAYS days' - - def add_arguments(self, parser): - parser.add_argument('keep_days', type=int, - help='Delete events older than this many days') - parser.add_argument('-n', '--dry-run', action='store_true', default=False, - help="Don't delete events, just show what would be done") - - - def handle(self, *args, **options): - keep_days = options['keep_days'] - dry_run = options['dry_run'] - verbosity = options.get("verbosity", 1) - - def _format_count(count, unit='day'): - return '{} {}{}'.format(count, unit, ('' if count == 1 else 's')) - - if keep_days < 0: - raise CommandError('Negative keep_days not allowed ({} was specified)'.format(keep_days)) - - if verbosity > 1: - self.stdout.write('purge_old_personal_api_key_events: Finding events older than {}\n'.format(_format_count(keep_days))) - if dry_run: - self.stdout.write('Dry run requested, records will not be deleted\n') - self.stdout.flush() - - now = timezone.now() - old_events = PersonApiKeyEvent.objects.filter( - time__lt=now - timedelta(days=keep_days) - ) - - stats = old_events.aggregate(Min('time'), Max('time')) - old_count = old_events.count() - if old_count == 0: - if verbosity > 1: - self.stdout.write('No events older than {} found\n'.format(_format_count(keep_days))) - return - - oldest_date = stats['time__min'] - oldest_ago = now - oldest_date - newest_date = stats['time__max'] - newest_ago = now - newest_date - - action_fmt = 'Would delete {}\n' if dry_run else 'Deleting {}\n' - if verbosity > 1: - self.stdout.write(action_fmt.format(_format_count(old_count, 'event'))) - self.stdout.write(' Oldest at {} ({} ago)\n'.format(oldest_date, _format_count(oldest_ago.days))) - self.stdout.write(' Most recent at {} ({} ago)\n'.format(newest_date, _format_count(newest_ago.days))) - self.stdout.flush() - - if not dry_run: - old_events.delete() diff --git a/ietf/person/management/commands/tests.py b/ietf/person/management/commands/tests.py deleted file mode 100644 index 38d770a58..000000000 --- a/ietf/person/management/commands/tests.py +++ /dev/null @@ -1,122 +0,0 @@ -# Copyright The IETF Trust 2021, All Rights Reserved -# -*- coding: utf-8 -*- - -import datetime -from io import StringIO - -from django.core.management import call_command, CommandError -from django.utils import timezone - -from ietf.person.factories import PersonApiKeyEventFactory -from ietf.person.models import PersonApiKeyEvent, PersonEvent -from ietf.utils.test_utils import TestCase - - -class CommandTests(TestCase): - @staticmethod - def _call_command(command_name, *args, **options): - out = StringIO() - options['stdout'] = out - call_command(command_name, *args, **options) - return out.getvalue() - - def _assert_purge_results(self, cmd_output, expected_delete_count, expected_kept_events): - self.assertNotIn('Dry run requested', cmd_output) - if expected_delete_count == 0: - delete_text = 'No events older than' - else: - delete_text = 'Deleting {} event'.format(expected_delete_count) - self.assertIn(delete_text, cmd_output) - self.assertCountEqual( - PersonApiKeyEvent.objects.all(), - expected_kept_events, - 'Wrong events were deleted' - ) - - def _assert_purge_dry_run_results(self, cmd_output, expected_delete_count, expected_kept_events): - self.assertIn('Dry run requested', cmd_output) - if expected_delete_count == 0: - delete_text = 'No events older than' - else: - delete_text = 'Would delete {} event'.format(expected_delete_count) - self.assertIn(delete_text, cmd_output) - self.assertCountEqual( - PersonApiKeyEvent.objects.all(), - expected_kept_events, - 'Events were deleted when dry-run option was used' - ) - - def test_purge_old_personal_api_key_events(self): - keep_days = 10 - - # Remember how many PersonEvents were present so we can verify they're cleaned up properly. - personevents_before = PersonEvent.objects.count() - - now = timezone.now() - # The first of these events will be timestamped a fraction of a second more than keep_days - # days ago by the time we call the management command, so will just barely chosen for purge. - old_events = [ - PersonApiKeyEventFactory(time=now - datetime.timedelta(days=n)) - for n in range(keep_days, 2 * keep_days + 1) - ] - num_old_events = len(old_events) - - recent_events = [ - PersonApiKeyEventFactory(time=now - datetime.timedelta(days=n)) - for n in range(0, keep_days) - ] - # We did not create recent_event timestamped exactly keep_days ago because it would - # be treated as an old_event by the management command. Create an event a few seconds - # on the "recent" side of keep_days old to test the threshold. - recent_events.append( - PersonApiKeyEventFactory( - time=now + datetime.timedelta(seconds=3) - datetime.timedelta(days=keep_days) - ) - ) - num_recent_events = len(recent_events) - - # call with dry run - output = self._call_command('purge_old_personal_api_key_events', str(keep_days), '--dry-run', '-v2') - self._assert_purge_dry_run_results(output, num_old_events, old_events + recent_events) - - # call for real - output = self._call_command('purge_old_personal_api_key_events', str(keep_days), '-v2') - self._assert_purge_results(output, num_old_events, recent_events) - self.assertEqual(PersonEvent.objects.count(), personevents_before + num_recent_events, - 'PersonEvents were not cleaned up properly') - - # repeat - there should be nothing left to delete - output = self._call_command('purge_old_personal_api_key_events', '--dry-run', str(keep_days), '-v2') - self._assert_purge_dry_run_results(output, 0, recent_events) - - output = self._call_command('purge_old_personal_api_key_events', str(keep_days), '-v2') - self._assert_purge_results(output, 0, recent_events) - self.assertEqual(PersonEvent.objects.count(), personevents_before + num_recent_events, - 'PersonEvents were not cleaned up properly') - - # and now delete the remaining events - output = self._call_command('purge_old_personal_api_key_events', '0', '-v2') - self._assert_purge_results(output, num_recent_events, []) - self.assertEqual(PersonEvent.objects.count(), personevents_before, - 'PersonEvents were not cleaned up properly') - - def test_purge_old_personal_api_key_events_rejects_invalid_arguments(self): - """The purge_old_personal_api_key_events command should reject invalid arguments""" - event = PersonApiKeyEventFactory(time=timezone.now() - datetime.timedelta(days=30)) - - with self.assertRaises(CommandError): - self._call_command('purge_old_personal_api_key_events') - - with self.assertRaises(CommandError): - self._call_command('purge_old_personal_api_key_events', '-15') - - with self.assertRaises(CommandError): - self._call_command('purge_old_personal_api_key_events', '15.3') - - with self.assertRaises(CommandError): - self._call_command('purge_old_personal_api_key_events', '15', '15') - - with self.assertRaises(CommandError): - self._call_command('purge_old_personal_api_key_events', 'abc', '15') - - self.assertCountEqual(PersonApiKeyEvent.objects.all(), [event]) diff --git a/ietf/person/tasks.py b/ietf/person/tasks.py new file mode 100644 index 000000000..221b718f2 --- /dev/null +++ b/ietf/person/tasks.py @@ -0,0 +1,20 @@ +# Copyright The IETF Trust 2024, All Rights Reserved +# +# Celery task definitions +# +import datetime + +from celery import shared_task +from django.utils import timezone + +from ietf.utils import log +from .models import PersonApiKeyEvent + + +@shared_task +def purge_personal_api_key_events_task(keep_days): + keep_since = timezone.now() - datetime.timedelta(days=keep_days) + old_events = PersonApiKeyEvent.objects.filter(time__lt=keep_since) + count = len(old_events) + old_events.delete() + log.log(f"Deleted {count} PersonApiKeyEvents older than {keep_since}") diff --git a/ietf/person/tests.py b/ietf/person/tests.py index e5bc855a2..9da201b70 100644 --- a/ietf/person/tests.py +++ b/ietf/person/tests.py @@ -4,6 +4,7 @@ import datetime import json +import mock from io import StringIO, BytesIO from PIL import Image @@ -25,8 +26,9 @@ from ietf.group.models import Group from ietf.nomcom.models import NomCom from ietf.nomcom.test_data import nomcom_test_data from ietf.nomcom.factories import NomComFactory, NomineeFactory, NominationFactory, FeedbackFactory, PositionFactory -from ietf.person.factories import EmailFactory, PersonFactory -from ietf.person.models import Person, Alias +from ietf.person.factories import EmailFactory, PersonFactory, PersonApiKeyEventFactory +from ietf.person.models import Person, Alias, PersonApiKeyEvent +from ietf.person.tasks import purge_personal_api_key_events_task from ietf.person.utils import (merge_persons, determine_merge_order, send_merge_notification, handle_users, get_extra_primary, dedupe_aliases, move_related_objects, merge_nominees, handle_reviewer_settings, get_dots) @@ -450,3 +452,16 @@ class PersonUtilsTests(TestCase): self.assertEqual(get_dots(ncmember),['nomcom']) ncchair = RoleFactory(group__acronym='nomcom2020',group__type_id='nomcom',name_id='chair').person self.assertEqual(get_dots(ncchair),['nomcom']) + + +class TaskTests(TestCase): + @mock.patch("ietf.person.tasks.log.log") + def test_purge_personal_api_key_events_task(self, mock_log): + now = timezone.now() + old_event = PersonApiKeyEventFactory(time=now - datetime.timedelta(days=1, minutes=1)) + young_event = PersonApiKeyEventFactory(time=now - datetime.timedelta(days=1, minutes=-1)) + purge_personal_api_key_events_task(keep_days=1) + self.assertFalse(PersonApiKeyEvent.objects.filter(pk=old_event.pk).exists()) + self.assertTrue(PersonApiKeyEvent.objects.filter(pk=young_event.pk).exists()) + self.assertTrue(mock_log.called) + self.assertIn("Deleted 1", mock_log.call_args[0][0]) diff --git a/ietf/utils/management/commands/periodic_tasks.py b/ietf/utils/management/commands/periodic_tasks.py index 792eb0068..5595bbcbf 100644 --- a/ietf/utils/management/commands/periodic_tasks.py +++ b/ietf/utils/management/commands/periodic_tasks.py @@ -241,6 +241,17 @@ class Command(BaseCommand): ), ) + PeriodicTask.objects.get_or_create( + name="Purge old personal API key events", + task="ietf.person.tasks.purge_personal_api_key_events_task", + kwargs=json.dumps(dict(keep_days=14)), + defaults=dict( + enabled=False, + crontab=self.crontabs["daily"], + description="Purge PersonApiKeyEvent instances older than 14 days", + ), + ) + def show_tasks(self): for label, crontab in self.crontabs.items(): tasks = PeriodicTask.objects.filter(crontab=crontab).order_by(