Fix/473 quarantine noupdate - #479
Conversation
| "name": "OpenSPP Attachment Antivirus Scan", | ||
| "category": "OpenSPP", | ||
| "version": "19.0.2.1.0", | ||
| "version": "19.0.2.2.0", |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 Noneself.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", |
There was a problem hiding this comment.
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 Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #479 +/- ##
=======================================
Coverage 76.26% 76.26%
=======================================
Files 662 662
Lines 44225 44225
=======================================
Hits 33729 33729
Misses 10496 10496
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Why is this change needed?
data/quarantine_cron.xmldeclared two crons and two config params in a plain<odoo>block with nonoupdate, 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?
quarantine_cron.xmlin<odoo noupdate="1">.migrations/19.0.2.2.0/post-migrate.py, since the loader only honorsnoupdateat record-creation time; on databases that already installed the module, the fourir.model.datarows exist withnoupdate = Falseand the XML change alone would not flip them. The migration setsnoupdate = TRUEon those rows and leaves the stored values untouched, so tuned values survive and untouched defaults stay as shipped.19.0.2.2.0so 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_scantests on this PR.How to test manually
spp_attachment_av_scan.quarantine_retention_daysto 30) or disable one of the crons.Related links