From 6389a2277ef6aea95087b5c01d46dd37acf1a8b2 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Wed, 22 Jul 2015 15:55:59 +0200 Subject: [PATCH] Fix closing of resovled manual issue --- .../core/issue/workflow/IssueWorkflow.java | 22 +-- .../core/issue/workflow/OrCondition.java | 42 +++++ .../issue/workflow/IssueWorkflowTest.java | 154 ++++++++++++------ .../core/issue/workflow/OrConditionTest.java | 55 +++++++ 4 files changed, 204 insertions(+), 69 deletions(-) create mode 100644 sonar-core/src/main/java/org/sonar/core/issue/workflow/OrCondition.java create mode 100644 sonar-core/src/test/java/org/sonar/core/issue/workflow/OrConditionTest.java diff --git a/sonar-core/src/main/java/org/sonar/core/issue/workflow/IssueWorkflow.java b/sonar-core/src/main/java/org/sonar/core/issue/workflow/IssueWorkflow.java index 5d5275f8502..cbd2c0661e0 100644 --- a/sonar-core/src/main/java/org/sonar/core/issue/workflow/IssueWorkflow.java +++ b/sonar-core/src/main/java/org/sonar/core/issue/workflow/IssueWorkflow.java @@ -91,7 +91,7 @@ public class IssueWorkflow implements Startable { .functions(new SetResolution(null), new SetCloseDate(false)) .build()) - // resolve as false-positive + // resolve as false-positive .transition(Transition.builder(DefaultTransitions.FALSE_POSITIVE) .from(Issue.STATUS_OPEN).to(Issue.STATUS_RESOLVED) .functions(new SetResolution(Issue.RESOLUTION_FALSE_POSITIVE), UnsetAssignee.INSTANCE) @@ -108,7 +108,7 @@ public class IssueWorkflow implements Startable { .requiredProjectPermission(UserRole.ISSUE_ADMIN) .build()) - // resolve as won't fix + // resolve as won't fix .transition(Transition.builder(DefaultTransitions.WONT_FIX) .from(Issue.STATUS_OPEN).to(Issue.STATUS_RESOLVED) .functions(new SetResolution(Issue.RESOLUTION_WONT_FIX), UnsetAssignee.INSTANCE) @@ -123,8 +123,7 @@ public class IssueWorkflow implements Startable { .from(Issue.STATUS_CONFIRMED).to(Issue.STATUS_RESOLVED) .functions(new SetResolution(Issue.RESOLUTION_WONT_FIX), UnsetAssignee.INSTANCE) .requiredProjectPermission(UserRole.ISSUE_ADMIN) - .build() - ); + .build()); } @@ -151,26 +150,19 @@ public class IssueWorkflow implements Startable { .build()) .transition(Transition.builder("automaticclose") .from(Issue.STATUS_RESOLVED).to(Issue.STATUS_CLOSED) - .conditions(IsBeingClosed.INSTANCE) - .functions(SetClosed.INSTANCE, new SetCloseDate(true)) - .automatic() - .build()) - .transition(Transition.builder("automaticclosemanual") - .from(Issue.STATUS_RESOLVED).to(Issue.STATUS_CLOSED) - .conditions(IsManual.INSTANCE) + .conditions(new OrCondition(IsBeingClosed.INSTANCE, IsManual.INSTANCE)) .functions(SetClosed.INSTANCE, new SetCloseDate(true)) .automatic() .build()) - // Reopen issues that are marked as resolved but that are still alive. - // Manual issues are kept resolved. + // Reopen issues that are marked as resolved but that are still alive. + // Manual issues are kept resolved. .transition(Transition.builder("automaticreopen") .from(Issue.STATUS_RESOLVED).to(Issue.STATUS_REOPENED) .conditions(new NotCondition(IsBeingClosed.INSTANCE), new HasResolution(Issue.RESOLUTION_FIXED), new NotCondition(IsManual.INSTANCE)) .functions(new SetResolution(null), new SetCloseDate(false)) .automatic() - .build() - ); + .build()); } @Override diff --git a/sonar-core/src/main/java/org/sonar/core/issue/workflow/OrCondition.java b/sonar-core/src/main/java/org/sonar/core/issue/workflow/OrCondition.java new file mode 100644 index 00000000000..8047892d71d --- /dev/null +++ b/sonar-core/src/main/java/org/sonar/core/issue/workflow/OrCondition.java @@ -0,0 +1,42 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2014 SonarSource + * mailto:contact AT sonarsource DOT com + * + * SonarQube is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * SonarQube is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.core.issue.workflow; + +import org.sonar.api.issue.Issue; +import org.sonar.api.issue.condition.Condition; + +public class OrCondition implements Condition { + private final Condition[] conditions; + + public OrCondition(Condition... conditions) { + this.conditions = conditions; + } + + @Override + public boolean matches(Issue issue) { + for (Condition condition : conditions) { + if (condition.matches(issue)) { + return true; + } + } + return false; + } + +} diff --git a/sonar-core/src/test/java/org/sonar/core/issue/workflow/IssueWorkflowTest.java b/sonar-core/src/test/java/org/sonar/core/issue/workflow/IssueWorkflowTest.java index 57a1d934b9b..1b59f3569b3 100644 --- a/sonar-core/src/test/java/org/sonar/core/issue/workflow/IssueWorkflowTest.java +++ b/sonar-core/src/test/java/org/sonar/core/issue/workflow/IssueWorkflowTest.java @@ -27,7 +27,6 @@ import java.util.Date; import java.util.List; import javax.annotation.Nullable; import org.apache.commons.lang.time.DateUtils; -import org.assertj.core.api.Assertions; import org.junit.Test; import org.sonar.api.issue.DefaultTransitions; import org.sonar.api.issue.Issue; @@ -39,6 +38,9 @@ import org.sonar.core.issue.IssueUpdater; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.Assert.fail; import static org.mockito.Mockito.mock; +import static org.sonar.api.issue.Issue.RESOLUTION_FIXED; +import static org.sonar.api.issue.Issue.STATUS_CLOSED; +import static org.sonar.api.rule.RuleKey.MANUAL_REPOSITORY_KEY; public class IssueWorkflowTest { @@ -52,7 +54,7 @@ public class IssueWorkflowTest { assertThat(workflow.machine()).isNotNull(); assertThat(workflow.machine().state(Issue.STATUS_OPEN)).isNotNull(); assertThat(workflow.machine().state(Issue.STATUS_CONFIRMED)).isNotNull(); - assertThat(workflow.machine().state(Issue.STATUS_CLOSED)).isNotNull(); + assertThat(workflow.machine().state(STATUS_CLOSED)).isNotNull(); assertThat(workflow.machine().state(Issue.STATUS_REOPENED)).isNotNull(); assertThat(workflow.machine().state(Issue.STATUS_RESOLVED)).isNotNull(); workflow.stop(); @@ -62,7 +64,7 @@ public class IssueWorkflowTest { public void list_statuses() { workflow.start(); // order is important for UI - assertThat(workflow.statusKeys()).containsSequence(Issue.STATUS_OPEN, Issue.STATUS_CONFIRMED, Issue.STATUS_REOPENED, Issue.STATUS_RESOLVED, Issue.STATUS_CLOSED); + assertThat(workflow.statusKeys()).containsSequence(Issue.STATUS_OPEN, Issue.STATUS_CONFIRMED, Issue.STATUS_REOPENED, Issue.STATUS_RESOLVED, STATUS_CLOSED); } @Test @@ -105,7 +107,7 @@ public class IssueWorkflowTest { public void list_no_out_transition_from_status_closed() { workflow.start(); - DefaultIssue issue = new DefaultIssue().setStatus(Issue.STATUS_CLOSED).setRuleKey(RuleKey.of("java", "R1 ")); + DefaultIssue issue = new DefaultIssue().setStatus(STATUS_CLOSED).setRuleKey(RuleKey.of("java", "R1 ")); List transitions = workflow.outTransitions(issue); assertThat(transitions).isEmpty(); } @@ -117,7 +119,7 @@ public class IssueWorkflowTest { // Manual issue because of reporter DefaultIssue issue = new DefaultIssue() .setKey("ABCDE") - .setStatus(Issue.STATUS_CLOSED) + .setStatus(STATUS_CLOSED) .setRuleKey(RuleKey.of("manual", "Performance")) .setReporter("simon"); @@ -139,22 +141,22 @@ public class IssueWorkflowTest { } @Test - public void do_automatic_transition() { + public void automatically_close_resolved_issue() { workflow.start(); DefaultIssue issue = new DefaultIssue() .setKey("ABCDE") .setRuleKey(RuleKey.of("js", "S001")) - .setResolution(Issue.RESOLUTION_FIXED) + .setResolution(RESOLUTION_FIXED) .setStatus(Issue.STATUS_RESOLVED) .setNew(false) .setBeingClosed(true); Date now = new Date(); workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now)); - Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FIXED); - Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_CLOSED); - Assertions.assertThat(issue.closeDate()).isNotNull(); - Assertions.assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND)); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); + assertThat(issue.closeDate()).isNotNull(); + assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND)); } @Test @@ -169,10 +171,10 @@ public class IssueWorkflowTest { .setBeingClosed(true); Date now = new Date(); workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now)); - Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FIXED); - Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_CLOSED); - Assertions.assertThat(issue.closeDate()).isNotNull(); - Assertions.assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND)); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); + assertThat(issue.closeDate()).isNotNull(); + assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND)); } @Test @@ -187,10 +189,10 @@ public class IssueWorkflowTest { .setBeingClosed(true); Date now = new Date(); workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now)); - Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FIXED); - Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_CLOSED); - Assertions.assertThat(issue.closeDate()).isNotNull(); - Assertions.assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND)); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); + assertThat(issue.closeDate()).isNotNull(); + assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND)); } @Test @@ -205,10 +207,10 @@ public class IssueWorkflowTest { .setBeingClosed(true); Date now = new Date(); workflow.doAutomaticTransition(issue, IssueChangeContext.createScan(now)); - Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FIXED); - Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_CLOSED); - Assertions.assertThat(issue.closeDate()).isNotNull(); - Assertions.assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND)); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); + assertThat(issue.closeDate()).isNotNull(); + assertThat(issue.updateDate()).isEqualTo(DateUtils.truncate(now, Calendar.SECOND)); } @Test @@ -217,7 +219,7 @@ public class IssueWorkflowTest { DefaultIssue issue = new DefaultIssue() .setKey("ABCDE") - .setResolution(Issue.RESOLUTION_FIXED) + .setResolution(RESOLUTION_FIXED) .setStatus("xxx") .setNew(false) .setBeingClosed(true); @@ -240,11 +242,11 @@ public class IssueWorkflowTest { workflow.start(); workflow.doTransition(issue, DefaultTransitions.FALSE_POSITIVE, IssueChangeContext.createScan(new Date())); - Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FALSE_POSITIVE); - Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_RESOLVED); + assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_FALSE_POSITIVE); + assertThat(issue.status()).isEqualTo(Issue.STATUS_RESOLVED); // should remove assignee - Assertions.assertThat(issue.assignee()).isNull(); + assertThat(issue.assignee()).isNull(); } @Test @@ -258,19 +260,23 @@ public class IssueWorkflowTest { workflow.start(); workflow.doTransition(issue, DefaultTransitions.WONT_FIX, IssueChangeContext.createScan(new Date())); - Assertions.assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_WONT_FIX); - Assertions.assertThat(issue.status()).isEqualTo(Issue.STATUS_RESOLVED); + assertThat(issue.resolution()).isEqualTo(Issue.RESOLUTION_WONT_FIX); + assertThat(issue.status()).isEqualTo(Issue.STATUS_RESOLVED); // should remove assignee - Assertions.assertThat(issue.assignee()).isNull(); + assertThat(issue.assignee()).isNull(); } + /** + * User marks the manual issue as resolved -> issue is automatically + * closed. + */ @Test - public void manual_issues_be_resolved_then_closed() { + public void automatically_close_resolved_manual_issue() { DefaultIssue issue = new DefaultIssue() .setKey("ABCDE") .setStatus(Issue.STATUS_OPEN) - .setRuleKey(RuleKey.of(RuleKey.MANUAL_REPOSITORY_KEY, "Performance")); + .setRuleKey(RuleKey.of(MANUAL_REPOSITORY_KEY, "Performance")); workflow.start(); @@ -278,20 +284,62 @@ public class IssueWorkflowTest { Transition.create("confirm", "OPEN", "CONFIRMED"), Transition.create("resolve", "OPEN", "RESOLVED"), Transition.create("falsepositive", "OPEN", "RESOLVED"), - Transition.create("wontfix", "OPEN", "RESOLVED") - ); + Transition.create("wontfix", "OPEN", "RESOLVED")); workflow.doTransition(issue, "resolve", mock(IssueChangeContext.class)); - Assertions.assertThat(issue.resolution()).isEqualTo("FIXED"); - Assertions.assertThat(issue.status()).isEqualTo("RESOLVED"); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo("RESOLVED"); assertThat(workflow.outTransitions(issue)).containsOnly( - Transition.create("reopen", "RESOLVED", "REOPENED") - ); + Transition.create("reopen", "RESOLVED", "REOPENED")); workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class)); - Assertions.assertThat(issue.resolution()).isEqualTo("FIXED"); - Assertions.assertThat(issue.status()).isEqualTo("CLOSED"); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); + } + + /** + * Manual issue is fixed because the file does not exist anymore + * or the tracking engine did not find the associated code + * -> the issue is closed + */ + @Test + public void automatically_close_manual_issue_on_deleted_code() { + DefaultIssue issue = new DefaultIssue() + .setKey("ABCDE") + .setStatus(Issue.STATUS_OPEN) + .setRuleKey(RuleKey.of(MANUAL_REPOSITORY_KEY, "Performance")) + .setBeingClosed(true); + + workflow.start(); + + workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class)); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); + } + + /** + * Corner-case : the manual issue was marked as resolved by user but at the same + * time the file or the associated line was deleted. + */ + @Test + public void automatically_close_resolved_manual_issue_on_deleted_code() { + DefaultIssue issue = new DefaultIssue() + .setKey("ABCDE") + .setRuleKey(RuleKey.of(MANUAL_REPOSITORY_KEY, "Performance")) + + // resolved by user + .setResolution(RESOLUTION_FIXED) + .setStatus(Issue.STATUS_RESOLVED) + + // but unmatched by tracking engine + .setBeingClosed(true); + + workflow.start(); + + workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class)); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); } @Test @@ -309,29 +357,27 @@ public class IssueWorkflowTest { Transition.create("confirm", "OPEN", "CONFIRMED"), Transition.create("resolve", "OPEN", "RESOLVED"), Transition.create("falsepositive", "OPEN", "RESOLVED"), - Transition.create("wontfix", "OPEN", "RESOLVED") - ); + Transition.create("wontfix", "OPEN", "RESOLVED")); workflow.doTransition(issue, "confirm", mock(IssueChangeContext.class)); - Assertions.assertThat(issue.resolution()).isNull(); - Assertions.assertThat(issue.status()).isEqualTo("CONFIRMED"); + assertThat(issue.resolution()).isNull(); + assertThat(issue.status()).isEqualTo("CONFIRMED"); assertThat(workflow.outTransitions(issue)).containsOnly( Transition.create("unconfirm", "CONFIRMED", "REOPENED"), Transition.create("resolve", "CONFIRMED", "RESOLVED"), Transition.create("falsepositive", "CONFIRMED", "RESOLVED"), - Transition.create("wontfix", "CONFIRMED", "RESOLVED") - ); + Transition.create("wontfix", "CONFIRMED", "RESOLVED")); // keep confirmed and unresolved workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class)); - Assertions.assertThat(issue.resolution()).isNull(); - Assertions.assertThat(issue.status()).isEqualTo("CONFIRMED"); + assertThat(issue.resolution()).isNull(); + assertThat(issue.status()).isEqualTo("CONFIRMED"); // unconfirm workflow.doTransition(issue, "unconfirm", mock(IssueChangeContext.class)); - Assertions.assertThat(issue.resolution()).isNull(); - Assertions.assertThat(issue.status()).isEqualTo("REOPENED"); + assertThat(issue.resolution()).isNull(); + assertThat(issue.status()).isEqualTo("REOPENED"); } @Test @@ -348,8 +394,8 @@ public class IssueWorkflowTest { workflow.start(); workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class)); - Assertions.assertThat(issue.resolution()).isEqualTo("REMOVED"); - Assertions.assertThat(issue.status()).isEqualTo("CLOSED"); + assertThat(issue.resolution()).isEqualTo("REMOVED"); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); } @Test @@ -366,8 +412,8 @@ public class IssueWorkflowTest { workflow.start(); workflow.doAutomaticTransition(issue, mock(IssueChangeContext.class)); - Assertions.assertThat(issue.resolution()).isEqualTo("FIXED"); - Assertions.assertThat(issue.status()).isEqualTo("CLOSED"); + assertThat(issue.resolution()).isEqualTo(RESOLUTION_FIXED); + assertThat(issue.status()).isEqualTo(STATUS_CLOSED); } private Collection keys(List transitions) { diff --git a/sonar-core/src/test/java/org/sonar/core/issue/workflow/OrConditionTest.java b/sonar-core/src/test/java/org/sonar/core/issue/workflow/OrConditionTest.java new file mode 100644 index 00000000000..83763cbb098 --- /dev/null +++ b/sonar-core/src/test/java/org/sonar/core/issue/workflow/OrConditionTest.java @@ -0,0 +1,55 @@ +/* + * SonarQube, open source software quality management tool. + * Copyright (C) 2008-2014 SonarSource + * mailto:contact AT sonarsource DOT com + * + * SonarQube is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * SonarQube is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.core.issue.workflow; + +import org.junit.Test; +import org.sonar.api.issue.Issue; +import org.sonar.api.issue.condition.Condition; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +public class OrConditionTest { + + private static final Condition TRUE_CONDITION = new BooleanCondition(true); + private static final Condition FALSE_CONDITION = new BooleanCondition(false); + Issue issue = mock(Issue.class); + + @Test + public void match() { + assertThat(new OrCondition(TRUE_CONDITION).matches(issue)).isTrue(); + assertThat(new OrCondition(FALSE_CONDITION).matches(issue)).isFalse(); + assertThat(new OrCondition(FALSE_CONDITION, TRUE_CONDITION).matches(issue)).isTrue(); + assertThat(new OrCondition(FALSE_CONDITION, FALSE_CONDITION).matches(issue)).isFalse(); + } + + private static class BooleanCondition implements Condition { + private final boolean b; + + public BooleanCondition(boolean b) { + this.b = b; + } + + @Override + public boolean matches(Issue issue) { + return b; + } + } +} -- 2.39.5