[FIX] runbot: properly move builds to the merged error

When build errors are merged together, the builds of the merged errors
should be moved to the only error that will be kept.
It 's not the case because the merge method is assigning a compute field
and moreover it was hidden in the tests because the compute was not
triggered.

With this commit, the build_error_link is updated to point to the new
error. The test is modified to properly check the case and also to add a
case when the link already exists.

The access rights are updated to allow admin to unlink the
build_error_link records. Otherwise the action could fail when the link
already exists.

runbot/models/build_error.py

@@ -298,7 +298,6 @@ class BuildError(models.Model):
         _logger.debug('Merging errors %s', self)
         base_error = self[0]
         base_linked = self[0].parent_id or self[0]
-        old_error_ids = []
         for error in self[1:]:
             assert base_error.fingerprint == error.fingerprint, f'Errors {base_error.id} and {error.id} have a different fingerprint'
             if error.test_tags and not base_linked.test_tags:
@@ -311,8 +310,12 @@ class BuildError(models.Model):
                 error.message_post(body=f'⚠ trying to merge errors with different test-tags from {base_error._get_form_link()} tag: "{base_error.test_tags}"')
                 continue
 
-            base_error.build_ids += error.build_ids
-            error.build_ids = False
+            for build_error_link in error.build_error_link_ids:
+                if build_error_link.build_id not in base_error.build_error_link_ids.build_id:
+                    build_error_link.build_error_id = base_error
+                else:
+                    # as the relation already exists and was not transferred we can remove the old one
+                    build_error_link.unlink()
 
             if error.responsible and not base_linked.responsible:
                 base_error.responsible = error.responsible
@@ -326,7 +329,6 @@ class BuildError(models.Model):
             error.message_post(body=f'Error was merged into {base_linked._get_form_link()}')
             error.child_ids.parent_id = base_error
             error.active = False
-            old_error_ids.append(error.id)
 
     ####################
     #   Actions
@@ -357,7 +359,7 @@ class BuildError(models.Model):
         errors_by_fingerprint = self.env['runbot.build.error'].search([('fingerprint', 'in', list(changed_fingerprints))])
         for fingerprint in changed_fingerprints:
             errors_to_merge = errors_by_fingerprint.filtered(lambda r: r.fingerprint == fingerprint)
-            rec_id = errors_to_merge._merge()
+            errors_to_merge._merge()
 
     def action_assign(self):
         if not any((not record.responsible and not record.team_id and record.file_path and not record.parent_id) for record in self):

runbot/security/ir.model.access.csv

@@ -23,7 +23,7 @@ access_runbot_build_error_user,runbot_build_error_user,runbot.model_runbot_build
 access_runbot_build_error_admin,runbot_build_error_admin,runbot.model_runbot_build_error,runbot.group_runbot_admin,1,1,1,1
 access_runbot_build_error_manager,runbot_build_error_manager,runbot.model_runbot_build_error,runbot.group_runbot_error_manager,1,1,1,1
 access_runbot_build_error_link_user,runbot_runbot_build_error_link_user,runbot.model_runbot_build_error_link,group_user,1,0,0,0
-access_runbot_build_error_link_admin,runbot_runbot_build_error_link_admin,runbot.model_runbot_build_error_link,runbot.group_runbot_admin,1,1,1,0
+access_runbot_build_error_link_admin,runbot_runbot_build_error_link_admin,runbot.model_runbot_build_error_link,runbot.group_runbot_admin,1,1,1,1
 access_runbot_build_error_link_manager,runbot_runbot_build_error_link_manager,runbot.model_runbot_build_error_link,runbot.group_runbot_error_manager,1,1,1,0
 access_runbot_build_error_tag_user,runbot_build_error_tag_user,runbot.model_runbot_build_error_tag,group_user,1,0,0,0
 access_runbot_build_error_tag_admin,runbot_build_error_tag_admin,runbot.model_runbot_build_error_tag,runbot.group_runbot_admin,1,1,1,1

runbot/tests/test_build_error.py

@@ -75,8 +75,11 @@ class TestBuildError(RunbotCase):
         self.assertEqual(len(self.BuildError.search([('fingerprint', '=', error_a.fingerprint)])), 1)
         self.assertTrue(error_a.active, 'The first merged error should stay active')
         self.assertFalse(error_b.active, 'The second merged error should have stay deactivated')
+        self.assertIn(build_a, error_a.build_error_link_ids.build_id)
         self.assertIn(build_a, error_a.build_ids)
+        self.assertIn(build_b, error_a.build_error_link_ids.build_id)
         self.assertIn(build_b, error_a.build_ids)
+        self.assertFalse(error_b.build_error_link_ids)
         self.assertFalse(error_b.build_ids)
 
         error_c = self.BuildError.create({'content': 'foo foo'})
@@ -85,6 +88,15 @@ class TestBuildError(RunbotCase):
         with self.assertRaises(AssertionError):
             (error_a | error_c)._merge()
 
+        # merge two build errors while the build <--> build_error relation already exists
+        error_d = self.BuildError.create({'content': 'foo bar'})
+        self.BuildErrorLink.create({'build_id': build_a.id, 'build_error_id': error_d.id})
+        (error_a | error_d)._merge()
+        self.assertIn(build_a, error_a.build_error_link_ids.build_id)
+        self.assertIn(build_a, error_a.build_ids)
+        self.assertFalse(error_d.build_error_link_ids)
+        self.assertFalse(error_d.build_ids)
+
     def test_merge_linked(self):
         top_error = self.BuildError.create({'content': 'foo foo', 'active': False})