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

✨ [#33] Add reset_db_save_after option to config model #37

Merged
merged 4 commits into from
Jun 13, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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
7 changes: 4 additions & 3 deletions docs/quickstart.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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).
Expand Down
11 changes: 11 additions & 0 deletions log_outgoing_requests/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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
6 changes: 4 additions & 2 deletions log_outgoing_requests/config_reset.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure how I can add the OutgoingRequestsLogConfig typehint here without the docs check failing, I tried it with if TYPE_CHECKING but that didn't work

Copy link
Member

Choose a reason for hiding this comment

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

from __future__ import annotations and then a conditional import tends to work usually, but instead, why not pass the self.reset_db_save_after so you can provide a typehint reset_after: int | None so you're only working in primitives :)

reset_after = (
config.reset_db_save_after or settings.LOG_OUTGOING_REQUESTS_RESET_DB_SAVE_AFTER
)
if not reset_after:
return

Expand Down
Original file line number Diff line number Diff line change
@@ -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",
),
),
]
12 changes: 11 additions & 1 deletion log_outgoing_requests/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."
),
)
Copy link
Member

Choose a reason for hiding this comment

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

I'd also add a min value validator to set at least 1 minute, 0 minutes would immediately reset it and makes it unclear in the code if the value is set to 0 or None with the soft-boolean check if config.reset_db_save_after

Adding the validator removes ambiguity


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):
Expand Down
35 changes: 33 additions & 2 deletions tests/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -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()
stevenbal marked this conversation as resolved.
Show resolved Hide resolved
mock_task.assert_called_once_with(countdown=240)
Loading