Skip to content

Fix/473 quarantine noupdate - #479

Open
LunarCapsule127 wants to merge 4 commits into
OpenSPP:19.0from
LunarCapsule127:fix/473-quarantine-noupdate
Open

Fix/473 quarantine noupdate#479
LunarCapsule127 wants to merge 4 commits into
OpenSPP:19.0from
LunarCapsule127:fix/473-quarantine-noupdate

Conversation

@LunarCapsule127

Copy link
Copy Markdown

Why is this change needed?

data/quarantine_cron.xml declared two crons and two config params in a plain <odoo> block with no noupdate, so every module upgrade rewrote all four to the shipped defaults. An admin who tuned a retention window, changed a cron interval, or deliberately disabled a cron got silently reset on the next upgrade.

Split out of the #470 review, which fixed the same defect in scan_sweep_cron.xml.

Fixes #473.

How was the change implemented?

  1. Wrapped quarantine_cron.xml in <odoo noupdate="1">.
  2. Added migrations/19.0.2.2.0/post-migrate.py, since the loader only honors noupdate at record-creation time; on databases that already installed the module, the four ir.model.data rows exist with noupdate = False and the XML change alone would not flip them. The migration sets noupdate = TRUE on those rows and leaves the stored values untouched, so tuned values survive and untouched defaults stay as shipped.
  3. Bumped the manifest to 19.0.2.2.0 so the migration runs.

New unit tests

None added; extended the existing regression test.

Unit tests executed by the author

No local environment; relying on repository CI to run spp_attachment_av_scan tests on this PR.

How to test manually

  1. Install the module, then change one of the quarantine params (e.g. set spp_attachment_av_scan.quarantine_retention_days to 30) or disable one of the crons.
  2. Upgrade the module.
  3. Confirm your change survives instead of snapping back to the shipped default.

Related links

"name": "OpenSPP Attachment Antivirus Scan",
"category": "OpenSPP",
"version": "19.0.2.1.0",
"version": "19.0.2.2.0",

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.

"""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

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.

"config_param_pending_sweep_min_age_minutes",
"config_param_pending_sweep_batch_size",
"config_param_pending_sweep_max_attempts",
"ir_cron_purge_quarantined_files",

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.

@codecov

codecov Bot commented Sep 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.26%. Comparing base (0341801) to head (0242d54).
⚠️ Report is 45 commits behind head on 19.0.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             19.0     #479   +/-   ##
=======================================
  Coverage   76.26%   76.26%           
=======================================
  Files         662      662           
  Lines       44225    44225           
=======================================
  Hits        33729    33729           
  Misses      10496    10496           
Flag Coverage Δ
spp_attachment_av_scan 86.72% <ø> (ø)
spp_base_common 91.07% <ø> (ø)
spp_programs 67.58% <ø> (ø)
spp_registry 87.79% <ø> (ø)
spp_security 69.56% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spp_attachment_av_scan: quarantine_cron.xml crons/params are not noupdate — upgrades silently reset admin-tuned values

2 participants