From 5247aff5497b32602f02d45dca8e4fca63fc2e7b Mon Sep 17 00:00:00 2001 From: Richard Parke Date: Tue, 16 Jun 2026 15:15:24 +0100 Subject: [PATCH 1/2] fix bug where `created_at` is overwritten for pending files at version 0 pending files are given a version of 0 when they are created to indicate that they don't properly "belong" to a template yet. This causes an odd bug when dao methods are wrapped with the `@version_class` decorator, the files have their `created_at` attribute overwritten with the current datetime. Since we don't care about the versioning of pending files that go straight into archived anyway, we shouldn't be using version_class wrapped methods anyway for this purpose. Files that are automatically archived from pending will now just remain at version 0. --- app/dao/template_email_files_dao.py | 23 ++++++++++++++--------- tests/app/celery/test_scheduled_tasks.py | 13 ++++++++----- tests/app/db.py | 2 ++ 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/app/dao/template_email_files_dao.py b/app/dao/template_email_files_dao.py index 7dc2af53c7..177b6390a6 100644 --- a/app/dao/template_email_files_dao.py +++ b/app/dao/template_email_files_dao.py @@ -137,16 +137,11 @@ def dao_make_pending_template_email_file_live(template_email_file: TemplateEmail @version_class( VersionOptions(TemplateEmailFile, history_class=TemplateEmailFileHistory), ) -def dao_archive_template_email_file(file_to_archive, archived_by_id, template_version=None): - if not template_version: - template_version = Template.query.get(file_to_archive.template_id).version - if not file_to_archive.archived_at: - file_to_archive.archived_at = datetime.datetime.utcnow() - file_to_archive.archived_by_id = archived_by_id - file_to_archive.template_version = template_version - db.session.add(file_to_archive) +def dao_archive_template_email_file(file_to_archive, archived_by_id, template_version): + _archive_template_email_file(file_to_archive, archived_by_id, template_version) +@autocommit def dao_archive_pending_files(): files_in_pending = TemplateEmailFile.query.filter( TemplateEmailFile.pending, @@ -154,5 +149,15 @@ def dao_archive_pending_files(): > datetime.timedelta(hours=current_app.config.get("TEMPLATE_EMAIL_FILE_ARCHIVE_PERIOD_IN_HOURS")), ).all() for file in files_in_pending: - dao_archive_template_email_file(file, archived_by_id=file.created_by_id) + _archive_template_email_file(file, file.created_by_id) return len(files_in_pending) + + +def _archive_template_email_file(file_to_archive, archived_by_id, template_version=None): + if not template_version: + template_version = Template.query.get(file_to_archive.template_id).version + if not file_to_archive.archived_at: + file_to_archive.archived_at = datetime.datetime.utcnow() + file_to_archive.archived_by_id = archived_by_id + file_to_archive.template_version = template_version + db.session.add(file_to_archive) diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 477d133946..65936ad40a 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -109,27 +109,30 @@ def test_should_update_scheduled_jobs_and_put_on_queue(mock_celery_task, sample_ ) -@pytest.mark.parametrize("pending_expirary_exceeded", (True, False)) -def test_archive_pending_files_only_scopes_stale_files(sample_email_template, pending_expirary_exceeded): +@pytest.mark.parametrize("pending_expiry_exceeded", (True, False)) +def test_archive_pending_files_only_scopes_stale_files(sample_email_template, pending_expiry_exceeded): with freeze_time("2016-01-01 01:00:00.000000"): pending_file = create_template_email_file( template_id=sample_email_template.id, created_by_id=sample_email_template.created_by_id, pending=True, created_at=datetime.utcnow(), + version=0, ) live_file = create_template_email_file( template_id=sample_email_template.id, created_by_id=sample_email_template.created_by_id, pending=False, created_at=datetime.utcnow(), + version=0, ) - with freeze_time("2016-01-02 01:00:01.00000" if pending_expirary_exceeded else "2016-01-02 00:00:00.00000"): + with freeze_time("2016-01-02 01:00:01.00000" if pending_expiry_exceeded else "2016-01-02 00:00:00.00000"): archive_pending_files() expected_archived_file = dao_get_template_email_file_by_id(str(pending_file.id)) expected_live_file = dao_get_template_email_file_by_id(str(live_file.id)) - if pending_expirary_exceeded: - assert expected_archived_file.archived_at == datetime.utcnow() + if pending_expiry_exceeded: + assert expected_archived_file.archived_at == datetime.fromisoformat("2016-01-02 01:00:01.00000") + assert expected_archived_file.created_at == datetime.fromisoformat("2016-01-01 01:00:00.000000") else: assert not expected_archived_file.archived_at assert not expected_live_file.archived_at diff --git a/tests/app/db.py b/tests/app/db.py index f056dd262c..86d0d5be3b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -205,6 +205,7 @@ def create_template_email_file( retention_period=90, validate_users_email=True, pending=False, + version=0, ): data = { "filename": filename, @@ -215,6 +216,7 @@ def create_template_email_file( "created_by_id": created_by_id, "created_at": created_at if created_at is not None else datetime.utcnow(), "pending": pending, + "version": version, } template_email_file = TemplateEmailFile(**data) if pending: From 01a2368fb1e8682d99927eb33127b8dd08bea82e Mon Sep 17 00:00:00 2001 From: Richard Parke Date: Thu, 18 Jun 2026 16:30:10 +0100 Subject: [PATCH 2/2] Bug Fix: Don't attempt to archive pending that are already archived It is a waste of time looping through files that are already archived, and could throw hard to debug sqlalchemy errors --- app/celery/scheduled_tasks.py | 4 +- app/dao/template_email_files_dao.py | 1 + tests/app/celery/test_scheduled_tasks.py | 60 ++++++++++++++---------- tests/app/db.py | 2 + 4 files changed, 42 insertions(+), 25 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 8839307c01..613387dca8 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -113,9 +113,11 @@ def run_scheduled_jobs(): def archive_pending_files(): try: number_of_files_archived = dao_archive_pending_files() + base_params = {"number_of_files_archived": number_of_files_archived} current_app.logger.info( "Archived %(number_of_files_archived)s files created more than 24 hours ago that are still in pending", - extra={"number_of_files_archived": number_of_files_archived}, + base_params, + extra={**base_params}, ) except SQLAlchemyError: current_app.logger.exception("Failed to archive pending files") diff --git a/app/dao/template_email_files_dao.py b/app/dao/template_email_files_dao.py index 177b6390a6..a0b83bd433 100644 --- a/app/dao/template_email_files_dao.py +++ b/app/dao/template_email_files_dao.py @@ -145,6 +145,7 @@ def dao_archive_template_email_file(file_to_archive, archived_by_id, template_ve def dao_archive_pending_files(): files_in_pending = TemplateEmailFile.query.filter( TemplateEmailFile.pending, + TemplateEmailFile.archived_at == None, # noqa: E711 datetime.datetime.utcnow() - TemplateEmailFile.created_at > datetime.timedelta(hours=current_app.config.get("TEMPLATE_EMAIL_FILE_ARCHIVE_PERIOD_IN_HOURS")), ).all() diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 65936ad40a..aab8ada9f3 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -110,32 +110,44 @@ def test_should_update_scheduled_jobs_and_put_on_queue(mock_celery_task, sample_ @pytest.mark.parametrize("pending_expiry_exceeded", (True, False)) -def test_archive_pending_files_only_scopes_stale_files(sample_email_template, pending_expiry_exceeded): - with freeze_time("2016-01-01 01:00:00.000000"): - pending_file = create_template_email_file( - template_id=sample_email_template.id, - created_by_id=sample_email_template.created_by_id, - pending=True, - created_at=datetime.utcnow(), - version=0, - ) - live_file = create_template_email_file( - template_id=sample_email_template.id, - created_by_id=sample_email_template.created_by_id, - pending=False, - created_at=datetime.utcnow(), - version=0, - ) +def test_archive_pending_files_only_scopes_stale_files(sample_email_template, pending_expiry_exceeded, caplog): + pending_file = create_template_email_file( + template_id=sample_email_template.id, + created_by_id=sample_email_template.created_by_id, + pending=True, + created_at=datetime.fromisoformat("2016-01-01 01:00:00.000000"), + version=0, + ) + pending_file_already_archived = create_template_email_file( + template_id=sample_email_template.id, + created_by_id=sample_email_template.created_by_id, + pending=True, + created_at=datetime.fromisoformat("2016-01-01 01:00:00.000000"), + version=0, + archived_at=datetime.fromisoformat("2016-01-01 03:30:00.000000"), + ) + live_file = create_template_email_file( + template_id=sample_email_template.id, + created_by_id=sample_email_template.created_by_id, + pending=False, + created_at=datetime.fromisoformat("2016-01-01 01:00:00.000000"), + version=0, + ) with freeze_time("2016-01-02 01:00:01.00000" if pending_expiry_exceeded else "2016-01-02 00:00:00.00000"): archive_pending_files() - expected_archived_file = dao_get_template_email_file_by_id(str(pending_file.id)) - expected_live_file = dao_get_template_email_file_by_id(str(live_file.id)) - if pending_expiry_exceeded: - assert expected_archived_file.archived_at == datetime.fromisoformat("2016-01-02 01:00:01.00000") - assert expected_archived_file.created_at == datetime.fromisoformat("2016-01-01 01:00:00.000000") - else: - assert not expected_archived_file.archived_at - assert not expected_live_file.archived_at + expected_archived_file = dao_get_template_email_file_by_id(str(pending_file.id)) + expected_pending_file_already_archived = dao_get_template_email_file_by_id(str(pending_file_already_archived.id)) + expected_live_file = dao_get_template_email_file_by_id(str(live_file.id)) + assert not expected_live_file.archived_at + assert expected_pending_file_already_archived.archived_at == datetime.fromisoformat("2016-01-01 03:30:00.000000") + assert len(caplog.messages) == 1 + if pending_expiry_exceeded: + assert expected_archived_file.archived_at == datetime.fromisoformat("2016-01-02 01:00:01.00000") + assert expected_archived_file.created_at == datetime.fromisoformat("2016-01-01 01:00:00.000000") + assert "Archived 1 files created more than 24 hours ago that are still in pending" in caplog.messages + else: + assert not expected_archived_file.archived_at + assert "Archived 0 files created more than 24 hours ago that are still in pending" in caplog.messages def test_should_update_all_scheduled_jobs_and_put_on_queue(sample_template, mock_celery_task): diff --git a/tests/app/db.py b/tests/app/db.py index 86d0d5be3b..2da2015619 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -206,6 +206,7 @@ def create_template_email_file( validate_users_email=True, pending=False, version=0, + archived_at=None, ): data = { "filename": filename, @@ -217,6 +218,7 @@ def create_template_email_file( "created_at": created_at if created_at is not None else datetime.utcnow(), "pending": pending, "version": version, + "archived_at": archived_at, } template_email_file = TemplateEmailFile(**data) if pending: