Fix/473 quarantine noupdate

spp_attachment_av_scan/README.rst

@@ -134,6 +134,19 @@ External: ``pyclamd`` (Python library for ClamAV integration)
 Changelog
 =========
 
+19.0.2.2.0
+~~~~~~~~~~
+
+- fix: stop module upgrades from resetting the quarantine crons and
+  config parameters. ``data/quarantine_cron.xml`` shipped in a plain
+  ``<odoo>`` block, so every upgrade rewrote all four records to the
+  shipped defaults: a tuned ``quarantine_retention_days``, a changed
+  cron interval, or a deliberately disabled cron was silently reverted.
+  The file is now ``noupdate="1"``. A post-migration reconciles the
+  ``noupdate`` column on the ``ir_model_data`` rows of databases that
+  installed the module earlier, leaving the stored values untouched, so
+  tuned values survive and untouched defaults stay as shipped
+
 19.0.2.1.0
 ~~~~~~~~~~
 

spp_attachment_av_scan/__manifest__.py

@@ -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",
     "sequence": 1,
     "author": "OpenSPP.org",
     "website": "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/OpenSPP/OpenSPP2",

spp_attachment_av_scan/data/quarantine_cron.xml

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

spp_attachment_av_scan/migrations/19.0.2.2.0/post-migrate.py

@@ -0,0 +1,47 @@
+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):
+    """Reconcile ``ir_model_data.noupdate`` for the quarantine crons/params.
+
+    ``data/quarantine_cron.xml`` is declared ``noupdate="1"`` now, and on an
+    already-installed database that file-level flag is by itself enough to stop
+    the reset: ``xml_import._tag_record`` tests ``self.noupdate``, which
+    ``_tag_root`` reads off the ``<odoo>`` element rather than off the stored
+    row, and returns before ``_load_records`` whenever the mode is not ``init``.
+    So this migration is not what fixes the upgrade rewrite, and no pre-migrate
+    is needed; the XML-only fix applied to ``scan_sweep_cron.xml`` in #470 is
+    correct as it stands.
+
+    What the XML cannot do is correct the stored rows.
+    ``_build_update_xmlids_query`` upserts ``ON CONFLICT ... DO UPDATE SET
+    (model, res_id, write_date)`` and never writes ``noupdate``, so every
+    database that already carries these xml_ids keeps ``noupdate = false`` in
+    ``ir_model_data``. That column is the one ``ir.model.data._process_end``
+    consults, and the one an admin -- or the regression test for these records
+    -- reads to confirm they are protected. Flip it once here, and leave the
+    stored values alone: tuned values survive 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,
+    )

spp_attachment_av_scan/readme/HISTORY.md

@@ -1,3 +1,14 @@
+### 19.0.2.2.0
+
+- fix: stop module upgrades from resetting the quarantine crons and config
+  parameters. `data/quarantine_cron.xml` shipped in a plain `<odoo>` block, so
+  every upgrade rewrote all four records to the shipped defaults: a tuned
+  `quarantine_retention_days`, a changed cron interval, or a deliberately
+  disabled cron was silently reverted. The file is now `noupdate="1"`. A
+  post-migration reconciles the `noupdate` column on the `ir_model_data` rows of
+  databases that installed the module earlier, leaving the stored values
+  untouched, so tuned values survive and untouched defaults stay as shipped
+
 ### 19.0.2.1.0
 
 - feat: sweep attachments stranded at `scan_status = pending`. Queueing a scan is

spp_attachment_av_scan/static/description/index.html

@@ -515,6 +515,20 @@ <h2><a class="toc-backref" href="#toc-entry-1">Changelog</a></h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.2.0</h1>
+<ul class="simple">
+<li>fix: stop module upgrades from resetting the quarantine crons and
+config parameters. <tt class="docutils literal">data/quarantine_cron.xml</tt> shipped in a plain
+<tt class="docutils literal">&lt;odoo&gt;</tt> block, so every upgrade rewrote all four records to the
+shipped defaults: a tuned <tt class="docutils literal">quarantine_retention_days</tt>, a changed
+cron interval, or a deliberately disabled cron was silently reverted.
+The file is now <tt class="docutils literal"><span class="pre">noupdate=&quot;1&quot;</span></tt>. A post-migration reconciles the
+<tt class="docutils literal">noupdate</tt> column on the <tt class="docutils literal">ir_model_data</tt> rows of databases that
+installed the module earlier, leaving the stored values untouched, so
+tuned values survive and untouched defaults stay as shipped</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.1.0</h1>
 <ul class="simple">
 <li>feat: sweep attachments stranded at <tt class="docutils literal">scan_status = pending</tt>.
@@ -532,7 +546,7 @@ <h1>19.0.2.1.0</h1>
 are out of scope</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.2</h1>
 <ul class="simple">
 <li>fix: never enqueue a malware scan while the registry is still loading.
@@ -544,7 +558,7 @@ <h1>19.0.2.0.2</h1>
 the scan status, so changed bytes never keep a stale <tt class="docutils literal">clean</tt> verdict</li>
 </ul>
 </div>
-<div class="section" id="section-3">
+<div class="section" id="section-4">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>fix: re-raise database errors (<tt class="docutils literal">psycopg2.Error</tt>,
@@ -554,7 +568,7 @@ <h1>19.0.2.0.1</h1>
 unrelated downstream code</li>
 </ul>
 </div>
-<div class="section" id="section-4">
+<div class="section" id="section-5">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

spp_attachment_av_scan/tests/test_ir_attachment.py

@@ -618,6 +618,26 @@ def test_cron_cleanup_keeps_recent_forensic_downloads(self):
         # Attachment should still exist (it's recent)
         self.assertTrue(self.attachment_model.browse(attachment_id).exists())
 
+    def test_the_quarantine_crons_and_params_are_not_reset_by_a_module_upgrade(self):
+        """An upgrade must not overwrite an admin's tuning of these records.
+
+        ``data/quarantine_cron.xml`` shipped in a plain ``<odoo>`` block, so every
+        upgrade rewrote all four records to the shipped defaults: a changed retention
+        window, a retimed cron, or a deliberately disabled cron was silently reverted.
+        The file is ``noupdate="1"`` now, and 19.0.2.2.0's post-migration flips the
+        stored flag on databases that predate it.
+        """
+        for name in (
+            "ir_cron_purge_quarantined_files",
+            "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)])
+                self.assertTrue(imd, "the record must exist")
+                self.assertTrue(imd.noupdate, "an upgrade must keep admin-tuned values")
+
     @patch("odoo.addons.spp_attachment_av_scan.models.av_scanner_backend.pyclamd")
     def test_large_file_skipped_during_scan(self, mock_pyclamd):
         """Test that files exceeding max_file_size_mb are skipped."""