From e9fc57816b4eb883a9d0a6a3fcbb7205cbbd3dc6 Mon Sep 17 00:00:00 2001 From: Christophe Monniez Date: Mon, 29 Jan 2024 10:16:11 +0100 Subject: [PATCH] [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 | 12 +++++++----- runbot/security/ir.model.access.csv | 2 +- runbot/tests/test_build_error.py | 12 ++++++++++++ 3 files changed, 20 insertions(+), 6 deletions(-) diff --git a/runbot/models/build_error.py b/runbot/models/build_error.py index a7909333..3bec114b 100644 --- a/runbot/models/build_error.py +++ b/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): diff --git a/runbot/security/ir.model.access.csv b/runbot/security/ir.model.access.csv index d47dd647..c79b6e97 100644 --- a/runbot/security/ir.model.access.csv +++ b/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 diff --git a/runbot/tests/test_build_error.py b/runbot/tests/test_build_error.py index 009462fa..a58bbc48 100644 --- a/runbot/tests/test_build_error.py +++ b/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})