[19.0][FIX] upgrade_analysis: don't reset foreign-module fields on moved noupdate records

upgrade_analysis/models/upgrade_analysis.py

@@ -314,9 +314,27 @@ def _get_node_value(element):
             return element.text
         return etree.tostring(element)
 
+    def _get_available_modules(self, module_name):
+        """Return the set of module names a given module may reference in its
+        data files: the module itself plus its (transitive) dependencies.
+        """
+        module = self.env["ir.module.module"].search([("name", "=", module_name)])
+        if not module:
+            return set()
+        upstream = module.upstream_dependencies(
+            exclude_states=("uninstalled", "uninstallable", "to remove")
+        )
+        return {module_name} | set(upstream.mapped("name"))
+
     def _get_xml_diff(
-        self, remote_update, remote_noupdate, local_update, local_noupdate
+        self,
+        remote_update,
+        remote_noupdate,
+        local_update,
+        local_noupdate,
+        local_module,
     ):
+        available_modules = self._get_available_modules(local_module)
         odoo = etree.Element("odoo")
         for xml_id in sorted(local_noupdate.keys()):
             local_record = local_noupdate[xml_id]
@@ -355,12 +373,21 @@ def _get_xml_diff(
                     # Does the field still exist?
                     if record_remote_dict[key].tag == "field":
                         field_name = remote_record.xpath(key)[0].attrib.get("name")
+                        model_name = local_record.attrib["model"]
                         if (
-                            local_record.attrib["model"] not in self.env
-                            or field_name
-                            not in self.env[local_record.attrib["model"]]._fields.keys()
+                            model_name not in self.env
+                            or field_name not in self.env[model_name]._fields
                         ):
                             continue
+                        # When the record was moved to another module, its
+                        # previous definition may set a field that is declared
+                        # in a module the current one does not depend on (e.g.
+                        # the field lives in a module that depends on this one).
+                        # That field is still managed by its own module, so it
+                        # must not be reset from here.
+                        field = self.env[model_name]._fields[field_name]
+                        if field._module and field._module not in available_modules:
+                            continue
                     # Overwrite an existing value with an empty one.
                     attribs = deepcopy(record_remote_dict[key]).attrib
                     for attr in ["eval", "ref"]:
@@ -389,7 +416,23 @@ def _get_xml_diff(
                             eval_constant = ast.unparse(ast.Constant(field.falsy_value))
                         if eval_constant != "''":
                             attribs["eval"] = eval_constant
-                    element.append(etree.Element(record_remote_dict[key].tag, attribs))
+                        new_element = etree.Element(
+                            record_remote_dict[key].tag, attribs
+                        )
+                        # Only emit the reset when the value a fresh install
+                        # would give the field actually differs from what the
+                        # previous version set. If they are equal, the
+                        # (noupdate) record already holds the right value and
+                        # needs no change.
+                        if self._get_node_value(new_element) == self._get_node_value(
+                            record_remote_dict[key]
+                        ):
+                            continue
+                        element.append(new_element)
+                    else:
+                        element.append(
+                            etree.Element(record_remote_dict[key].tag, attribs)
+                        )
                 else:
                     oldrepr = self._get_node_value(record_remote_dict[key])
                     newrepr = self._get_node_value(record_local_dict[key])
@@ -575,7 +618,11 @@ def generate_noupdate_changes(
             local_files = local_record_obj.get_xml_records(local_module)
             local_update, local_noupdate = self._parse_files(local_files, local_module)
             diff = self._get_xml_diff(
-                remote_update, remote_noupdate, local_update, local_noupdate
+                remote_update,
+                remote_noupdate,
+                local_update,
+                local_noupdate,
+                local_module,
             )
             if diff:
                 module = self.env["ir.module.module"].search(

upgrade_analysis/tests/test_module.py

@@ -150,14 +150,27 @@ def test_xml_comparison(self):
                     """
                 ),
             },
+            "upgrade_analysis",
         )
         self.assertIn('<field name="module_ids" eval="None"/>', diff)
         self.assertIn('<field name="display_name"/>', diff)
 
     def test_analyze(self):
         """
         Test a full analysis run.
-        For the time being, only xmlid related functionality is tested
+        For the time being, only xmlid related functionality is tested.
+
+        The record is detected as renamed from ``other_module`` to
+        ``upgrade_analysis`` and the generated ``noupdate_changes.xml``
+        exercises the discarded-field handling:
+
+        - ``name``: value changed, so it is written;
+        - ``port``: dropped, and the value a fresh install gives (its default)
+          differs from the previous one, so a reset is written;
+        - ``username``: dropped, but the default equals the previous value, so
+          nothing is written;
+        - ``database``: dropped, but its field is (patched to be) declared in a
+          module the current one does not depend on, so it is left untouched.
         """
         analysis = self.env["upgrade.analysis"].create(
             {
@@ -235,7 +248,9 @@ def get_xml_records(self, module):
                                 <field
                                     name="name"
                                 >Noupdate xmlid from other_module</field>
-                                <field name="server">some_server</field>
+                                <field name="port" eval="7000"/>
+                                <field name="username" eval="'admin'"/>
+                                <field name="database">some_database</field>
                             </record>
                         </odoo>
                         """
@@ -253,7 +268,6 @@ def local_get_xml_records(module):
                             <field
                                 name="name"
                             >Noupdate xmlid from upgrade_analysis</field>
-                            <field name="server">some_server</field>
                         </record>
                     </odoo>
                     """
@@ -264,6 +278,10 @@ def local_get_xml_records(module):
         def write_file(module_name, version, content, filename="upgrade_analysis.txt"):
             written_files[f"{module_name}-{version}-{filename}"] = content
 
+        # Pretend the "database" field is declared in a module that depends on
+        # (and is therefore not reachable from) upgrade_analysis.
+        database_field = self.env["upgrade.comparison.config"]._fields["database"]
+
         with (
             patch.object(analysis.config_id.__class__, "get_connection"),
             patch.object(
@@ -273,6 +291,7 @@ def write_file(module_name, version, content, filename="upgrade_analysis.txt"):
             patch.object(
                 self.env["upgrade.record"].__class__, "get_xml_records"
             ) as patched_get_xml_records,
+            patch.object(database_field, "_module", "some_dependent_module"),
         ):
             patched_get_remote_model.side_effect = lambda *args: RemoteUpgradeRecord()
             patched_get_xml_records.side_effect = local_get_xml_records
@@ -283,6 +302,7 @@ def write_file(module_name, version, content, filename="upgrade_analysis.txt"):
 <odoo>
   <record id="test_noupdate_xmlid" model="upgrade.comparison.config">
     <field name="name">Noupdate xmlid from upgrade_analysis</field>
+    <field name="port" eval="8069"/>
   </record>
 </odoo>
 """