Skip to content

Fix/473 quarantine noupdate #479

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.

Sign up for GitHub

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

Open
LunarCapsule127 wants to merge 4 commits into
base: 19.0
Choose a base branch
from
Open

Fix/473 quarantine noupdate #479

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
516d67a
Update __manifest__.py
LunarCapsule127 Aug 29, 2026
c4db10d
Update quarantine_cron.xml
LunarCapsule127 Aug 29, 2026
d999884
Create post-migrate.py
LunarCapsule127 Aug 29, 2026
0242d54
Update test_pending_scan_sweep.py
LunarCapsule127 Aug 29, 2026
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
2 changes: 1 addition & 1 deletion spp_attachment_av_scan/__manifest__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{ # pylint: disable=pointless-statement
"name": "OpenSPP Attachment Antivirus Scan",
"category": "OpenSPP",
"version": "19.0.2.1.0",
"version": "19.0.2.2.0",

@kneckinator kneckinator Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The version goes to 19.0.2.2.0 but readme/HISTORY.md still ends at ### 19.0.2.1.0, and no fragment is added. Every other released version of this module has a matching ### <version> section (19.0.2.0.1, 19.0.2.0.2, 19.0.2.1.0), and README.rst/README.md render their changelog from that file, so the shipped module will advertise 19.0.2.2.0 while its published changelog stops at the previous version -- an admin reading the README has no record that the quarantine crons/params became noupdate and that a migration touched ir_model_data. oca-gen-addon-readme runs --if-source-changed, so pre-commit stays green and the gap ships silently.

"sequence": 1,
"author": "OpenSPP.org",
"website": "https://github.com/OpenSPP/OpenSPP2",
Expand Down
2 changes: 1 addition & 1 deletion spp_attachment_av_scan/data/quarantine_cron.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<?xml version="1.0" encoding="utf-8" ?>
<odoo>
<odoo noupdate="1">
<!-- Scheduled action to purge old quarantined files -->
<record id="ir_cron_purge_quarantined_files" model="ir.cron">
<field name="name">Purge Old Quarantined Files</field>
Expand Down
36 changes: 36 additions & 0 deletions spp_attachment_av_scan/migrations/19.0.2.2.0/post-migrate.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import logging

_logger = logging.getLogger(__name__)

_RECORDS = (
"ir_cron_purge_quarantined_files",
"ir_cron_cleanup_forensic_downloads",
"config_param_quarantine_retention_days",
"config_param_forensic_download_retention_hours",
)


def migrate(cr, version):
"""Protect admin-tuned quarantine crons/params from upgrade resets.

The records in ``data/quarantine_cron.xml`` are now declared
``noupdate="1"``, but that flag is only honored when a record is first

@kneckinator kneckinator Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The stated rationale is wrong for Odoo 19, and it is wrong in a direction that matters. In odoo/tools/convert.py, xml_import._tag_record short-circuits on the file-level attribute before it ever reaches _load_records:

# in update mode, the record won't be updated if the data node explicitly
# opt-out using @noupdate="1". A second check will be performed in
# model._load_records() using the record's ir.model.data `noupdate` field.
if self.noupdate and self.mode != 'init':
    ...
    if record := env['ir.model.data']._load_xmlid(xid):
        self.idref[xid] = record.id
        return None

self.noupdate comes from <odoo noupdate="1"> via _tag_root, not from the ir_model_data row, so on an already-installed database the XML change alone already stops the reset -- the flag is not "only honored when a record is first created." What the migration actually does is reconcile the ir_model_data.noupdate column, which is what ir.model.data._process_end and the new test read. That is still worth doing, but the comment should say so.

Why it is worth fixing rather than leaving: as written, this reasoning implies (a) a pre-migrate would be required for correctness here, and (b) the XML-only fix merged for scan_sweep_cron.xml in #470 is broken. Neither is true, and the next person to copy this docstring will act on both.

created. On any database that installed this module before the flag was
added, the ``ir.model.data`` rows already exist with ``noupdate = False``,
so every upgrade keeps rewriting them to the shipped defaults. Flip the
flag on the existing rows; leave the stored values untouched so an admin's
tuning survives and untouched defaults stay as shipped.
"""
cr.execute(
"""
UPDATE ir_model_data
SET noupdate = TRUE
WHERE module = 'spp_attachment_av_scan'
AND name IN %s
""",
(_RECORDS,),
)
_logger.info(
"spp_attachment_av_scan: set noupdate on %s quarantine cron/param records",
cr.rowcount,
)
4 changes: 4 additions & 0 deletions spp_attachment_av_scan/tests/test_pending_scan_sweep.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,6 +401,10 @@ def test_the_cron_and_config_defaults_are_not_reset_by_a_module_upgrade(self):
"config_param_pending_sweep_min_age_minutes",
"config_param_pending_sweep_batch_size",
"config_param_pending_sweep_max_attempts",
"ir_cron_purge_quarantined_files",

@kneckinator kneckinator Sep 3, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Minor: these four records belong to the quarantine/forensic feature, not the pending-scan sweep, and the test's docstring ("The comments on these records invite the admin to tune them") does not hold for them -- data/quarantine_cron.xml carries no tuning comments at all, unlike scan_sweep_cron.xml. Someone auditing quarantine behaviour will look in tests/test_ir_attachment.py, where _cron_purge_old_quarantined_files and _cron_cleanup_forensic_downloads are already covered, and will not find this guard. Either move the four names into a sibling test there, or generalize this test's name/docstring to cover the module's data records as a whole.

"ir_cron_cleanup_forensic_downloads",
"config_param_quarantine_retention_days",
"config_param_forensic_download_retention_hours",
):
with self.subTest(record=name):
imd = self.env["ir.model.data"].search([("module", "=", "spp_attachment_av_scan"), ("name", "=", name)])
Expand Down
Loading