SONAR-3755 Improve creation of manual issue

diff --git a/sonar-core/src/main/java/org/sonar/core/resource/ResourceDto.java b/sonar-core/src/main/java/org/sonar/core/resource/ResourceDto.java
index 33af18cbb3ddfc699e058c68559be9a66822429a..cbf8cd69121e4b43ef4e8948a821e19d57314a8b 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/core/resource/ResourceDto.java
+++ b/sonar-core/src/main/java/org/sonar/core/resource/ResourceDto.java
@@ -21,7 +21,7 @@ package org.sonar.core.resource;
 
 import java.util.Date;
 
-public final class ResourceDto {
+public class ResourceDto {
 
   private Long id;
   private String key;

diff --git a/sonar-core/src/main/java/org/sonar/core/resource/ResourceQuery.java b/sonar-core/src/main/java/org/sonar/core/resource/ResourceQuery.java
index fff662a1690ffdf0914f0bdd48088f900b2ba051..2da1623cbb2f2dc4117c9164f6d8439368b9db53 100644 (file)
--- a/sonar-core/src/main/java/org/sonar/core/resource/ResourceQuery.java
+++ b/sonar-core/src/main/java/org/sonar/core/resource/ResourceQuery.java
@@ -22,7 +22,7 @@ package org.sonar.core.resource;
 /**
  * @since 3.0
  */
-public final class ResourceQuery {
+public class ResourceQuery {
   private String[] qualifiers = null;
   private String key = null;
   private boolean excludeDisabled = false;

diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
index 331bc0ebdc4ed60a46e945b90542fe3714a9e356..13e980527afe4b84f1312fb9ada0f10931edab49 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
@@ -19,10 +19,8 @@
  */
 package org.sonar.server.issue;
 
-import com.google.common.annotations.VisibleForTesting;
 import com.google.common.base.Strings;
 import org.sonar.api.ServerComponent;
-import org.sonar.api.component.Component;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.IssueQuery;
 import org.sonar.api.issue.IssueQueryResult;
@@ -39,6 +37,7 @@ import org.sonar.core.issue.workflow.IssueWorkflow;
 import org.sonar.core.issue.workflow.Transition;
 import org.sonar.core.resource.ResourceDao;
 import org.sonar.core.resource.ResourceDto;
+import org.sonar.core.resource.ResourceQuery;
 import org.sonar.core.user.AuthorizationDao;
 import org.sonar.server.user.UserSession;
 
@@ -109,6 +108,7 @@ public class IssueService implements ServerComponent {
   }
 
   public Issue doTransition(String issueKey, String transition, UserSession userSession) {
+    verifyLoggedIn(userSession);
     IssueQueryResult queryResult = loadIssue(issueKey);
     DefaultIssue issue = (DefaultIssue) queryResult.first();
     IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login());
@@ -120,6 +120,7 @@ public class IssueService implements ServerComponent {
   }
 
   public Issue assign(String issueKey, @Nullable String assignee, UserSession userSession) {
+    verifyLoggedIn(userSession);
     IssueQueryResult queryResult = loadIssue(issueKey);
     DefaultIssue issue = (DefaultIssue) queryResult.first();
     if (assignee != null && userFinder.findByLogin(assignee) == null) {
@@ -134,6 +135,7 @@ public class IssueService implements ServerComponent {
   }
 
   public Issue plan(String issueKey, @Nullable String actionPlanKey, UserSession userSession) {
+    verifyLoggedIn(userSession);
     if (!Strings.isNullOrEmpty(actionPlanKey) && actionPlanService.findByKey(actionPlanKey, userSession) == null) {
       throw new IllegalArgumentException("Unknown action plan: " + actionPlanKey);
     }
@@ -149,6 +151,7 @@ public class IssueService implements ServerComponent {
   }
 
   public Issue setSeverity(String issueKey, String severity, UserSession userSession) {
+    verifyLoggedIn(userSession);
     IssueQueryResult queryResult = loadIssue(issueKey);
     DefaultIssue issue = (DefaultIssue) queryResult.first();
 
@@ -161,18 +164,23 @@ public class IssueService implements ServerComponent {
   }
 
   public DefaultIssue createManualIssue(DefaultIssue issue, UserSession userSession) {
-    checkAuthorization(userSession, issue, UserRole.USER);
+    verifyLoggedIn(userSession);
+    ResourceDto resourceDto = resourceDao.getResource(ResourceQuery.create().setKey(issue.componentKey()));
+    if (resourceDto == null) {
+      throw new IllegalArgumentException("Unknown component: " + issue.componentKey());
+    }
+    if (!authorizationDao.isAuthorizedComponentId(resourceDto.getId(), userSession.userId(), UserRole.USER)) {
+      // TODO throw unauthorized
+      throw new IllegalStateException("User does not have the required role");
+    }
     if (!"manual".equals(issue.ruleKey().repository())) {
       throw new IllegalArgumentException("Issues can be created only on rules marked as 'manual': " + issue.ruleKey());
     }
+
     Rule rule = ruleFinder.findByKey(issue.ruleKey());
     if (rule == null) {
       throw new IllegalArgumentException("Unknown rule: " + issue.ruleKey());
     }
-    Component component = resourceDao.findByKey(issue.componentKey());
-    if (component == null) {
-      throw new IllegalArgumentException("Unknown component: " + issue.componentKey());
-    }
 
     Date now = new Date();
     issue.setCreationDate(now);
@@ -194,25 +202,11 @@ public class IssueService implements ServerComponent {
     return workflow.statusKeys();
   }
 
-  @VisibleForTesting
-  void checkAuthorization(UserSession userSession, Issue issue, String requiredRole) {
+  private void verifyLoggedIn(UserSession userSession) {
     if (!userSession.isLoggedIn()) {
       // must be logged
       throw new IllegalStateException("User is not logged in");
     }
-    if (!authorizationDao.isAuthorizedComponentId(findRootProject(issue.componentKey()).getId(), userSession.userId(), requiredRole)) {
-      // TODO throw unauthorized
-      throw new IllegalStateException("User does not have the required role");
-    }
   }
 
-  @VisibleForTesting
-  ResourceDto findRootProject(String componentKey) {
-    ResourceDto resourceDto = resourceDao.getRootProjectByComponentKey(componentKey);
-    if (resourceDto == null) {
-      // TODO throw 404
-      throw new IllegalArgumentException("Component '" + componentKey + "' does not exists.");
-    }
-    return resourceDto;
-  }
 }

diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceTest.java
index b529a572902623e7b860f0c11ddc726a42868973..d1c001141c96e17a55e6d444347b3ef7b52e7f58 100644 (file)
--- a/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceTest.java
@@ -23,7 +23,6 @@ package org.sonar.server.issue;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
-import org.sonar.api.component.Component;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.IssueQuery;
 import org.sonar.api.issue.IssueQueryResult;
@@ -42,6 +41,7 @@ import org.sonar.core.issue.workflow.IssueWorkflow;
 import org.sonar.core.issue.workflow.Transition;
 import org.sonar.core.resource.ResourceDao;
 import org.sonar.core.resource.ResourceDto;
+import org.sonar.core.resource.ResourceQuery;
 import org.sonar.core.user.AuthorizationDao;
 import org.sonar.core.user.DefaultUser;
 import org.sonar.server.user.UserSession;
@@ -155,7 +155,6 @@ public class IssueServiceTest {
 
   @Test
   public void should_not_do_transition() {
-    grantAccess();
     when(workflow.doTransition(eq(issue), eq(transition.key()), any(IssueChangeContext.class))).thenReturn(false);
 
     Issue result = issueService.doTransition("ABCD", transition.key(), userSession);
@@ -165,6 +164,19 @@ public class IssueServiceTest {
     verifyZeroInteractions(issueNotifications);
   }
 
+
+  @Test
+  public void should_fail_do_transition_if_not_logged() {
+    when(userSession.isLoggedIn()).thenReturn(false);
+    try {
+      issueService.doTransition("ABCD", transition.key(), userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+    }
+    verifyZeroInteractions(authorizationDao);
+  }
+
   @Test
   public void should_assign() {
     String assignee = "perceval";
@@ -303,11 +315,10 @@ public class IssueServiceTest {
 
   @Test
   public void should_create_manual_issue() {
-    grantAccess();
     RuleKey ruleKey = RuleKey.of("manual", "manualRuleKey");
     DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(RuleKey.of("manual", "manualRuleKey")).setComponentKey("org.sonar.Sample");
     when(ruleFinder.findByKey(ruleKey)).thenReturn(Rule.create("manual", "manualRuleKey"));
-    when(resourceDao.findByKey("org.sonar.Sample")).thenReturn(mock(Component.class));
+    when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(mock(ResourceDto.class));
 
     Issue result = issueService.createManualIssue(manualIssue, userSession);
     assertThat(result).isNotNull();
@@ -318,9 +329,26 @@ public class IssueServiceTest {
     verify(authorizationDao).isAuthorizedComponentId(anyLong(), anyInt(), eq(UserRole.USER));
   }
 
+  @Test
+  public void should_fail_create_manual_issue_if_not_having_required_role() {
+    RuleKey ruleKey = RuleKey.of("manual", "manualRuleKey");
+    when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(mock(ResourceDto.class));
+    when(ruleFinder.findByKey(ruleKey)).thenReturn(Rule.create("manual", "manualRuleKey"));
+    when(authorizationDao.isAuthorizedComponentId(anyLong(), eq(10), anyString())).thenReturn(false);
+
+    DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(ruleKey).setComponentKey("org.sonar.Sample");
+    try {
+      issueService.createManualIssue(manualIssue, userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User does not have the required role");
+    }
+  }
+
   @Test
   public void should_fail_create_manual_issue_if_not_manual_rule() {
-    grantAccess();
+    when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(mock(ResourceDto.class));
+
     RuleKey ruleKey = RuleKey.of("squid", "s100");
     DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(ruleKey).setComponentKey("org.sonar.Sample");
     try {
@@ -329,13 +357,13 @@ public class IssueServiceTest {
     } catch (Exception e) {
       assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Issues can be created only on rules marked as 'manual': squid:s100");
     }
-
     verifyZeroInteractions(issueStorage);
   }
 
   @Test
   public void should_fail_create_manual_issue_if_rule_not_found() {
-    grantAccess();
+    when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(mock(ResourceDto.class));
+
     RuleKey ruleKey = RuleKey.of("manual", "manualRuleKey");
     DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(RuleKey.of("manual", "manualRuleKey")).setComponentKey("org.sonar.Sample");
     when(ruleFinder.findByKey(ruleKey)).thenReturn(null);
@@ -345,72 +373,23 @@ public class IssueServiceTest {
     } catch (Exception e) {
       assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Unknown rule: manual:manualRuleKey");
     }
-
     verifyZeroInteractions(issueStorage);
   }
 
   @Test
   public void should_fail_create_manual_issue_if_component_not_found() {
-    grantAccess();
     RuleKey ruleKey = RuleKey.of("manual", "manualRuleKey");
     DefaultIssue manualIssue = new DefaultIssue().setKey("GHIJ").setRuleKey(RuleKey.of("manual", "manualRuleKey")).setComponentKey("org.sonar.Sample");
     when(ruleFinder.findByKey(ruleKey)).thenReturn(Rule.create("manual", "manualRuleKey"));
-    when(resourceDao.findByKey("org.sonar.Sample")).thenReturn(null);
+    when(resourceDao.getResource(any(ResourceQuery.class))).thenReturn(null);
     try {
       issueService.createManualIssue(manualIssue, userSession);
       fail();
     } catch (Exception e) {
       assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Unknown component: org.sonar.Sample");
     }
-
     verifyZeroInteractions(issueStorage);
   }
 
-  @Test
-  public void should_fail_if_not_logged() {
-    when(userSession.isLoggedIn()).thenReturn(false);
-    try {
-      issueService.checkAuthorization(userSession, issue, UserRole.USER);
-      fail();
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
-    }
-    verifyZeroInteractions(authorizationDao);
-  }
-
-  @Test
-  public void should_fail_if_not_having_required_role() {
-    grantAccess();
-    when(userSession.isLoggedIn()).thenReturn(true);
-    when(authorizationDao.isAuthorizedComponentId(anyLong(), anyInt(), anyString())).thenReturn(false);
-    try {
-      issueService.checkAuthorization(userSession, issue, UserRole.USER);
-      fail();
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User does not have the required role");
-    }
-  }
-
-  @Test
-  public void should_find_project() {
-    ResourceDto project = new ResourceDto().setKey("org.sonar.Sample").setId(1l);
-    when(resourceDao.getRootProjectByComponentKey(anyString())).thenReturn(project);
-    assertThat(issueService.findRootProject("org.sonar.Sample")).isEqualTo(project);
-  }
-
-  @Test
-  public void should_fail_to_find_project() {
-    when(resourceDao.getRootProjectByComponentKey(anyString())).thenReturn(null);
-    try {
-      issueService.findRootProject("org.sonar.Sample");
-      fail();
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Component 'org.sonar.Sample' does not exists.");
-    }
-  }
-
-  private void grantAccess(){
-    when(resourceDao.getRootProjectByComponentKey(anyString())).thenReturn(new ResourceDto().setKey("org.sonar.Sample").setId(1l));
-  }
 
 }