From da5ac00cd4684ccb21643fe1fd67ca67a895e709 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Mon, 19 Feb 2024 11:54:43 +0100 Subject: [PATCH 1/4] :sparkles: [#33] Add reset_db_save_after option to config model --- log_outgoing_requests/admin.py | 11 +++++++++ log_outgoing_requests/config_reset.py | 6 +++-- ...ngrequestslogconfig_reset_db_save_after.py | 23 +++++++++++++++++++ log_outgoing_requests/models.py | 12 +++++++++- 4 files changed, 49 insertions(+), 3 deletions(-) create mode 100644 log_outgoing_requests/migrations/0006_outgoingrequestslogconfig_reset_db_save_after.py diff --git a/log_outgoing_requests/admin.py b/log_outgoing_requests/admin.py index e9921b2..ffb33a7 100644 --- a/log_outgoing_requests/admin.py +++ b/log_outgoing_requests/admin.py @@ -9,6 +9,11 @@ from .conf import settings from .models import OutgoingRequestsLog, OutgoingRequestsLogConfig +try: + import celery +except ImportError: + celery = None + @admin.register(OutgoingRequestsLog) class OutgoingRequestsLogAdmin(admin.ModelAdmin): @@ -128,3 +133,9 @@ class Meta: @admin.register(OutgoingRequestsLogConfig) class OutgoingRequestsLogConfigAdmin(SingletonModelAdmin): form = ConfigAdminForm + + def get_fields(self, request, obj=None, *args, **kwargs): + fields = super().get_fields(request, obj=obj, *args, **kwargs) + if celery is None and (obj and not obj.reset_db_save_after): + fields.remove("reset_db_save_after") + return fields diff --git a/log_outgoing_requests/config_reset.py b/log_outgoing_requests/config_reset.py index 92ef9bb..c89c52a 100644 --- a/log_outgoing_requests/config_reset.py +++ b/log_outgoing_requests/config_reset.py @@ -4,8 +4,10 @@ from .tasks import reset_config -def schedule_config_reset(): - reset_after = settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER +def schedule_config_reset(config): + reset_after = ( + config.reset_db_save_after or settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER + ) if not reset_after: return diff --git a/log_outgoing_requests/migrations/0006_outgoingrequestslogconfig_reset_db_save_after.py b/log_outgoing_requests/migrations/0006_outgoingrequestslogconfig_reset_db_save_after.py new file mode 100644 index 0000000..e75ecae --- /dev/null +++ b/log_outgoing_requests/migrations/0006_outgoingrequestslogconfig_reset_db_save_after.py @@ -0,0 +1,23 @@ +# Generated by Django 3.2.23 on 2024-02-19 11:12 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("log_outgoing_requests", "0005_alter_outgoingrequestslog_url"), + ] + + operations = [ + migrations.AddField( + model_name="outgoingrequestslogconfig", + name="reset_db_save_after", + field=models.PositiveIntegerField( + blank=True, + help_text="If configured, after the config has been updated, reset the database logging after the specified number of minutes. Note: this overrides the LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER environment variable.", + null=True, + verbose_name="Reset saving logs in database after", + ), + ), + ] diff --git a/log_outgoing_requests/models.py b/log_outgoing_requests/models.py index 8a5bc66..e9b3a32 100644 --- a/log_outgoing_requests/models.py +++ b/log_outgoing_requests/models.py @@ -206,13 +206,23 @@ class OutgoingRequestsLogConfig(SingletonModel): "If 'Require content length' is not checked, this setting has no effect." ), ) + reset_db_save_after = models.PositiveIntegerField( + _("Reset saving logs in database after"), + null=True, + blank=True, + help_text=_( + "If configured, after the config has been updated, reset the database logging " + "after the specified number of minutes. Note: this overrides the " + "LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER environment variable." + ), + ) class Meta: verbose_name = _("Outgoing request log configuration") def save(self, *args, **kwargs): super().save(*args, **kwargs) - schedule_config_reset() + schedule_config_reset(self) @property def save_logs_enabled(self): From 4fc3883fdf5499fa68ae94245f1168a54b86c969 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Mon, 19 Feb 2024 12:16:00 +0100 Subject: [PATCH 2/4] :white_check_mark: [#33] Add/fix tests for db configurable reset_db_save_after --- tests/test_tasks.py | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/tests/test_tasks.py b/tests/test_tasks.py index 4a2dc93..5c55774 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -94,10 +94,41 @@ def test_saving_config_schedules_config_reset(mocker): @pytest.mark.skipif(not has_celery, reason="Celery is optional dependency") +@pytest.mark.django_db def test_schedule_config_schedules_celery_task(settings, mocker): + settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER = 1 + config = OutgoingRequestsLogConfig.get_solo() mock_task = mocker.patch( "log_outgoing_requests.config_reset.reset_config.apply_async" ) - settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER = 1 - schedule_config_reset() + schedule_config_reset(config) mock_task.assert_called_once_with(countdown=60) + + +@pytest.mark.skipif(not has_celery, reason="Celery is optional dependency") +@pytest.mark.django_db +def test_schedule_config_schedules_celery_task_use_db_value(settings, mocker): + settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER = 1 + config = OutgoingRequestsLogConfig.get_solo() + config.reset_db_save_after = 2 + mock_task = mocker.patch( + "log_outgoing_requests.config_reset.reset_config.apply_async" + ) + schedule_config_reset(config) + mock_task.assert_called_once_with(countdown=120) + + +@pytest.mark.skipif(not has_celery, reason="Celery is optional dependency") +@pytest.mark.django_db +def test_schedule_config_schedules_celery_task_after_save_use_db_value( + settings, mocker +): + settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER = 1 + config = OutgoingRequestsLogConfig.get_solo() + config.reset_db_save_after = 2 + mock_task = mocker.patch( + "log_outgoing_requests.config_reset.reset_config.apply_async" + ) + config.reset_db_save_after = 4 + config.save() + mock_task.assert_called_once_with(countdown=240) From bb72e566040cde5f03973835c469584f63f14e7f Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Mon, 19 Feb 2024 14:16:23 +0100 Subject: [PATCH 3/4] :memo: [#33] Update docs --- docs/quickstart.rst | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/quickstart.rst b/docs/quickstart.rst index fd65f13..1ab4c03 100644 --- a/docs/quickstart.rst +++ b/docs/quickstart.rst @@ -20,7 +20,7 @@ Installation pip install django-log-outgoing-requests -#. Add ``log_outgoing_requests`` to ``INSTALLED_APPS`` in your Django +#. Add ``log_outgoing_requests`` to ``INSTALLED_APPS`` in your Django project's ``settings.py``. #. Run ``python manage.py migrate`` to create the necessary database tables @@ -97,9 +97,10 @@ you likely want to apply the following non-default settings: From a security and privacy perspective, we advise not enabling saving to the database by default via Django settings and instead rely on runtime configuration. - If Celery is installed but not configured in your environment, ``LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER`` + If Celery is installed but not configured in your environment, ``LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER`` (which defines if/when database logging is reset after changes to the library config) should - be set to ``None``. + be set to ``None``. The duration for **Reset saving logs in database after** can also be + configured from the admin and will override the value of the environment variable if defined. The library provides a Django management command as well as a Celery task to delete logs which are older than a specified time (by default, 1 day). From 2220b2d45361f994f88c0dcb5371d83cec954544 Mon Sep 17 00:00:00 2001 From: Steven Bal Date: Tue, 27 Feb 2024 10:35:45 +0100 Subject: [PATCH 4/4] :ok_hand: [#33] PR feedback --- log_outgoing_requests/config_reset.py | 8 ++++---- .../0006_outgoingrequestslogconfig_reset_db_save_after.py | 6 ++++-- log_outgoing_requests/models.py | 8 ++++++-- tests/test_tasks.py | 5 ++--- 4 files changed, 16 insertions(+), 11 deletions(-) diff --git a/log_outgoing_requests/config_reset.py b/log_outgoing_requests/config_reset.py index c89c52a..d18b277 100644 --- a/log_outgoing_requests/config_reset.py +++ b/log_outgoing_requests/config_reset.py @@ -1,13 +1,13 @@ # NOTE: since this is imported in models.py, ensure it doesn't use functionality that # requires django to be fully initialized. +from typing import Optional + from .conf import settings from .tasks import reset_config -def schedule_config_reset(config): - reset_after = ( - config.reset_db_save_after or settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER - ) +def schedule_config_reset(reset_after: Optional[int]): + reset_after = reset_after or settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER if not reset_after: return diff --git a/log_outgoing_requests/migrations/0006_outgoingrequestslogconfig_reset_db_save_after.py b/log_outgoing_requests/migrations/0006_outgoingrequestslogconfig_reset_db_save_after.py index e75ecae..66bb090 100644 --- a/log_outgoing_requests/migrations/0006_outgoingrequestslogconfig_reset_db_save_after.py +++ b/log_outgoing_requests/migrations/0006_outgoingrequestslogconfig_reset_db_save_after.py @@ -1,5 +1,6 @@ -# Generated by Django 3.2.23 on 2024-02-19 11:12 +# Generated by Django 3.2.24 on 2024-02-27 09:32 +import django.core.validators from django.db import migrations, models @@ -15,8 +16,9 @@ class Migration(migrations.Migration): name="reset_db_save_after", field=models.PositiveIntegerField( blank=True, - help_text="If configured, after the config has been updated, reset the database logging after the specified number of minutes. Note: this overrides the LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER environment variable.", + help_text="If configured, after the config has been updated, reset the database logging after the specified number of minutes. Note: this overrides the LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER environment variable. Additionally, depending on the broker that is used, if this duration is too long the key for the reset task might have expired before that time. So make sure not to set too large a value for the reset.", null=True, + validators=[django.core.validators.MinValueValidator(1)], verbose_name="Reset saving logs in database after", ), ), diff --git a/log_outgoing_requests/models.py b/log_outgoing_requests/models.py index e9b3a32..db8f1be 100644 --- a/log_outgoing_requests/models.py +++ b/log_outgoing_requests/models.py @@ -210,10 +210,14 @@ class OutgoingRequestsLogConfig(SingletonModel): _("Reset saving logs in database after"), null=True, blank=True, + validators=[MinValueValidator(1)], help_text=_( "If configured, after the config has been updated, reset the database logging " "after the specified number of minutes. Note: this overrides the " - "LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER environment variable." + "LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER environment variable. Additionally, " + "depending on the broker that is used, if this duration is too long " + "the key for the reset task might have expired before that time. So make sure not to " + "set too large a value for the reset." ), ) @@ -222,7 +226,7 @@ class Meta: def save(self, *args, **kwargs): super().save(*args, **kwargs) - schedule_config_reset(self) + schedule_config_reset(self.reset_db_save_after) @property def save_logs_enabled(self): diff --git a/tests/test_tasks.py b/tests/test_tasks.py index 5c55774..37dbe5c 100644 --- a/tests/test_tasks.py +++ b/tests/test_tasks.py @@ -101,7 +101,7 @@ def test_schedule_config_schedules_celery_task(settings, mocker): mock_task = mocker.patch( "log_outgoing_requests.config_reset.reset_config.apply_async" ) - schedule_config_reset(config) + schedule_config_reset(config.reset_db_save_after) mock_task.assert_called_once_with(countdown=60) @@ -114,7 +114,7 @@ def test_schedule_config_schedules_celery_task_use_db_value(settings, mocker): mock_task = mocker.patch( "log_outgoing_requests.config_reset.reset_config.apply_async" ) - schedule_config_reset(config) + schedule_config_reset(config.reset_db_save_after) mock_task.assert_called_once_with(countdown=120) @@ -125,7 +125,6 @@ def test_schedule_config_schedules_celery_task_after_save_use_db_value( ): settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER = 1 config = OutgoingRequestsLogConfig.get_solo() - config.reset_db_save_after = 2 mock_task = mocker.patch( "log_outgoing_requests.config_reset.reset_config.apply_async" )