From 1add3d4854718db3f3c663b6492179b4f4dbbe4a Mon Sep 17 00:00:00 2001 From: Xavier Morel Date: Mon, 7 Feb 2022 13:23:41 +0100 Subject: [PATCH] [FIX] runbot_merge: freeze being triggered upon reopening the wizard The freeze wizard was implemented using a single action to open and validate the dialog. This was a mistake, as it means if there are no errors left (e.g. all the PRs being waited for are now validated) trying to view the freeze wizard will immediately validate it and commit the freeze, which is unexpected, surprising, and unsafe e.g. - open wizard - add freeze prs - add a required pr or two - close and go do something else - be told that more PRs need to be waited for - reopen wizard - oops freeze is done So split the "open action" part of `action_freeze` into opening the action and performing the freeze. The "freeze" / "view freeze" button on the project only activates the latter, and the actual freeze operation is only triggered from the wizard's "Freeze" button. Part of #559. --- runbot_merge/models/project.py | 2 +- .../models/project_freeze/__init__.py | 21 +++++++++++-------- runbot_merge/tests/test_multirepo.py | 9 ++++++++ 3 files changed, 22 insertions(+), 10 deletions(-) diff --git a/runbot_merge/models/project.py b/runbot_merge/models/project.py index dc83b8f4..77c778e2 100644 --- a/runbot_merge/models/project.py +++ b/runbot_merge/models/project.py @@ -117,4 +117,4 @@ class Project(models.Model): 'branch_name': self._next_freeze(), 'release_pr_ids': [(0, 0, {'repository_id': repo.id}) for repo in self.repo_ids] }) - return w.action_freeze() + return w.action_open() diff --git a/runbot_merge/models/project_freeze/__init__.py b/runbot_merge/models/project_freeze/__init__.py index f795261d..92bc6c65 100644 --- a/runbot_merge/models/project_freeze/__init__.py +++ b/runbot_merge/models/project_freeze/__init__.py @@ -56,21 +56,24 @@ class FreezeWizard(models.Model): return {'type': 'ir.actions.act_window_close'} + def action_open(self): + return { + 'type': 'ir.actions.act_window', + 'target': 'new', + 'name': f'Freeze project {self.project_id.name}', + 'view_mode': 'form', + 'res_model': self._name, + 'res_id': self.id, + } + def action_freeze(self): """ Attempts to perform the freeze. """ - project_id = self.project_id # if there are still errors, reopen the wizard if self.errors: - return { - 'type': 'ir.actions.act_window', - 'target': 'new', - 'name': f'Freeze project {project_id.name}', - 'view_mode': 'form', - 'res_model': self._name, - 'res_id': self.id, - } + return self.action_open() + project_id = self.project_id # need to create the new branch, but at the same time resequence # everything so the new branch is the second one, just after the branch # it "forks" diff --git a/runbot_merge/tests/test_multirepo.py b/runbot_merge/tests/test_multirepo.py index f2aaf471..bfb4f316 100644 --- a/runbot_merge/tests/test_multirepo.py +++ b/runbot_merge/tests/test_multirepo.py @@ -1152,6 +1152,7 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): w = project.action_prepare_freeze() w2 = project.action_prepare_freeze() assert w == w2, "each project should only have one freeze wizard active at a time" + assert w['res_model'] == 'runbot_merge.project.freeze' w_id = env[w['res_model']].browse([w['res_id']]) assert w_id.branch_name == '1.1', "check that the forking incremented the minor by 1" @@ -1188,6 +1189,14 @@ def test_freeze_complete(env, project, repo_a, repo_b, repo_c, users, config): assert not w_id.errors + # assume the wizard is closed, re-open it + w = project.action_prepare_freeze() + assert w['res_model'] == 'runbot_merge.project.freeze' + assert w['res_id'] == w_id.id, "check that we're still getting the old wizard" + w_id = env[w['res_model']].browse([w['res_id']]) + assert w_id.exists() + + # actually perform the freeze r = w_id.action_freeze() # check that the wizard was deleted assert not w_id.exists()