]> source.dussan.org Git - sonarqube.git/commitdiff
SONAR-8899 Move assign code from IssueService to AssignAction
authorJulien Lancelot <julien.lancelot@sonarsource.com>
Tue, 14 Mar 2017 10:20:51 +0000 (11:20 +0100)
committerJulien Lancelot <julien.lancelot@sonarsource.com>
Tue, 21 Mar 2017 12:05:50 +0000 (13:05 +0100)
server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueTesting.java
server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueDbTester.java
server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java
server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java
server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java

index 748d3bb8b02af6ad4aae9937c8af00fca410d65d..33ff23722c438370166a499b39a676222fe9aa28 100644 (file)
@@ -43,7 +43,7 @@ public class IssueTesting {
    */
   public static IssueDto newDto(RuleDto rule, ComponentDto file, ComponentDto project) {
     return new IssueDto()
-      .setKee(Uuids.create())
+      .setKee(Uuids.createFast())
       .setRule(rule)
       .setType(RuleType.CODE_SMELL)
       .setComponent(file)
index a21cb1b04dc8c0421a0a79cc37c5a82d08d8b907..9ef1a22cf346222952d70d96c715e3970c8eae65 100644 (file)
@@ -20,6 +20,7 @@
 package org.sonar.db.issue;
 
 import java.util.Arrays;
+import java.util.function.Consumer;
 import javax.annotation.Nullable;
 import org.sonar.core.issue.DefaultIssueComment;
 import org.sonar.core.issue.FieldDiffs;
@@ -46,10 +47,17 @@ public class IssueDbTester {
   }
 
   public IssueDto insertIssue() {
+    return insertIssue(issueDto -> {
+    });
+  }
+
+  public IssueDto insertIssue(Consumer<IssueDto> populateIssueDto) {
     RuleDto rule = db.rules().insertRule(newRuleDto());
     ComponentDto project = db.components().insertProject();
     ComponentDto file = db.components().insertComponent(newFileDto(project));
-    return insertIssue(newDto(rule, file, project));
+    IssueDto issueDto = newDto(rule, file, project);
+    populateIssueDto.accept(issueDto);
+    return insertIssue(issueDto);
   }
 
   public IssueChangeDto insertChange(IssueChangeDto issueChangeDto) {
index 09e56f45b3a37c9854a7183d537a5ce6d52677f4..a72d2322f75c4dcbda0972e5a61d288ea67362e0 100644 (file)
@@ -19,7 +19,6 @@
  */
 package org.sonar.server.issue;
 
-import com.google.common.base.Strings;
 import java.util.Collection;
 import java.util.Date;
 import java.util.List;
@@ -31,12 +30,9 @@ import org.sonar.core.issue.DefaultIssue;
 import org.sonar.core.issue.IssueChangeContext;
 import org.sonar.db.DbClient;
 import org.sonar.db.DbSession;
-import org.sonar.db.user.UserDto;
 import org.sonar.server.issue.index.IssueIndex;
 import org.sonar.server.user.UserSession;
 
-import static org.sonar.server.ws.WsUtils.checkRequest;
-
 @ServerSide
 @ComputeEngineSide
 public class IssueService {
@@ -58,27 +54,6 @@ public class IssueService {
     this.userSession = userSession;
   }
 
-  public void assign(String issueKey, @Nullable String assignee) {
-    userSession.checkLoggedIn();
-
-    DbSession session = dbClient.openSession(false);
-    try {
-      DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue();
-      UserDto user = null;
-      if (!Strings.isNullOrEmpty(assignee)) {
-        user = dbClient.userDao().selectByLogin(session, assignee);
-        checkRequest(user != null && user.isActive(), "Unknown user: %s", assignee);
-      }
-      IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.getLogin());
-      if (issueFieldsSetter.assign(issue, user, context)) {
-        issueUpdater.saveIssue(session, issue, context, null);
-      }
-
-    } finally {
-      session.close();
-    }
-  }
-
   /**
    * Search for all tags, whatever issue resolution or user access rights
    */
index abf15d50d860a9a9b893fc6253eba9785af4994a..33b6b611a8819640902fec6e938b1871f5495b71 100644 (file)
  */
 package org.sonar.server.issue.ws;
 
+import com.google.common.base.Strings;
 import com.google.common.io.Resources;
+import java.util.Date;
+import javax.annotation.CheckForNull;
+import javax.annotation.Nullable;
 import org.apache.commons.lang.BooleanUtils;
 import org.sonar.api.server.ws.Request;
 import org.sonar.api.server.ws.Response;
 import org.sonar.api.server.ws.WebService;
+import org.sonar.api.utils.System2;
+import org.sonar.core.issue.DefaultIssue;
+import org.sonar.core.issue.IssueChangeContext;
 import org.sonar.core.util.Uuids;
-import org.sonar.server.issue.IssueService;
+import org.sonar.db.DbClient;
+import org.sonar.db.DbSession;
+import org.sonar.db.user.UserDto;
+import org.sonar.server.issue.IssueFieldsSetter;
+import org.sonar.server.issue.IssueFinder;
+import org.sonar.server.issue.IssueUpdater;
 import org.sonar.server.user.UserSession;
 
 import static com.google.common.base.Strings.emptyToNull;
+import static org.sonar.server.ws.WsUtils.checkFound;
 import static org.sonarqube.ws.client.issue.IssuesWsParameters.ACTION_ASSIGN;
 import static org.sonarqube.ws.client.issue.IssuesWsParameters.PARAM_ASSIGNEE;
 import static org.sonarqube.ws.client.issue.IssuesWsParameters.PARAM_ISSUE;
@@ -36,14 +49,24 @@ import static org.sonarqube.ws.client.issue.IssuesWsParameters.PARAM_ISSUE;
 public class AssignAction implements IssuesWsAction {
 
   private static final String DEPRECATED_PARAM_ME = "me";
+  private static final String ASSIGN_TO_ME_VALUE = "_me";
 
+  private final System2 system2;
   private final UserSession userSession;
-  private final IssueService issueService;
+  private final DbClient dbClient;
+  private final IssueFinder issueFinder;
+  private final IssueFieldsSetter issueFieldsSetter;
+  private final IssueUpdater issueUpdater;
   private final OperationResponseWriter responseWriter;
 
-  public AssignAction(UserSession userSession, IssueService issueService, OperationResponseWriter responseWriter) {
+  public AssignAction(System2 system2, UserSession userSession, DbClient dbClient, IssueFinder issueFinder, IssueFieldsSetter issueFieldsSetter, IssueUpdater issueUpdater,
+    OperationResponseWriter responseWriter) {
+    this.system2 = system2;
     this.userSession = userSession;
-    this.issueService = issueService;
+    this.dbClient = dbClient;
+    this.issueFinder = issueFinder;
+    this.issueFieldsSetter = issueFieldsSetter;
+    this.issueUpdater = issueUpdater;
     this.responseWriter = responseWriter;
   }
 
@@ -61,8 +84,7 @@ public class AssignAction implements IssuesWsAction {
       .setRequired(true)
       .setExampleValue(Uuids.UUID_EXAMPLE_01);
     action.createParam(PARAM_ASSIGNEE)
-      // TODO document absent value for unassign, and "_me" for assigning to me
-      .setDescription("Login of the assignee")
+      .setDescription("Login of the assignee. When not set, it will unassign the issue. Use '%s' to assign to current user", ASSIGN_TO_ME_VALUE)
       .setExampleValue("admin");
     action.createParam(DEPRECATED_PARAM_ME)
       .setDescription("(deprecated) Assign the issue to the logged-in user. Replaced by the parameter assignee=_me")
@@ -72,16 +94,39 @@ public class AssignAction implements IssuesWsAction {
 
   @Override
   public void handle(Request request, Response response) throws Exception {
+    userSession.checkLoggedIn();
+    String assignee = getAssignee(request);
+    String key = request.mandatoryParam(PARAM_ISSUE);
+    assign(key, assignee);
+    responseWriter.write(key, request, response);
+  }
+
+  private void assign(String issueKey, @Nullable String assignee) {
+    try (DbSession dbSession = dbClient.openSession(false)) {
+      DefaultIssue issue = issueFinder.getByKey(dbSession, issueKey).toDefaultIssue();
+      UserDto user = getUser(dbSession, assignee);
+      IssueChangeContext context = IssueChangeContext.createUser(new Date(system2.now()), userSession.getLogin());
+      if (issueFieldsSetter.assign(issue, user, context)) {
+        issueUpdater.saveIssue(dbSession, issue, context, null);
+      }
+    }
+  }
+
+  @CheckForNull
+  private String getAssignee(Request request) {
     String assignee = emptyToNull(request.param(PARAM_ASSIGNEE));
-    if ("_me".equals(assignee) || BooleanUtils.isTrue(request.paramAsBoolean(DEPRECATED_PARAM_ME))) {
-      // Permission is currently checked by IssueService. We still
-      // check that user is authenticated in order to get his login.
-      userSession.checkLoggedIn();
-      assignee = userSession.getLogin();
+    if (ASSIGN_TO_ME_VALUE.equals(assignee) || BooleanUtils.isTrue(request.paramAsBoolean(DEPRECATED_PARAM_ME))) {
+      return userSession.getLogin();
     }
-    String key = request.mandatoryParam(PARAM_ISSUE);
-    issueService.assign(key, assignee);
+    return assignee;
+  }
 
-    responseWriter.write(key, request, response);
+  @CheckForNull
+  private UserDto getUser(DbSession dbSession, @Nullable String assignee) {
+    if (Strings.isNullOrEmpty(assignee)) {
+      return null;
+    }
+    UserDto user = dbClient.userDao().selectByLogin(dbSession, assignee);
+    return checkFound(user != null && user.isActive() ? user : null, "Unknown user: %s", assignee);
   }
 }
index f4b8c5f5f1e7d026be839f8215136c90b56d14fd..e3d516d96c3713f7cd3a662d04324b69affe8370 100644 (file)
@@ -42,10 +42,8 @@ import org.sonar.db.organization.OrganizationTesting;
 import org.sonar.db.rule.RuleDao;
 import org.sonar.db.rule.RuleDto;
 import org.sonar.db.rule.RuleTesting;
-import org.sonar.db.user.UserDto;
 import org.sonar.server.es.SearchOptions;
 import org.sonar.server.es.SearchResult;
-import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.issue.index.IssueDoc;
 import org.sonar.server.issue.index.IssueIndex;
 import org.sonar.server.issue.index.IssueIndexer;
@@ -62,7 +60,6 @@ import static com.google.common.collect.Lists.newArrayList;
 import static java.util.Arrays.asList;
 import static org.assertj.core.api.Assertions.assertThat;
 import static org.assertj.core.api.Assertions.entry;
-import static org.junit.Assert.fail;
 
 public class IssueServiceMediumTest {
 
@@ -91,70 +88,6 @@ public class IssueServiceMediumTest {
     session.close();
   }
 
-  private void index() {
-    IssueIndexer r = tester.get(IssueIndexer.class);
-    r.indexOnStartup(r.getIndexTypes());
-  }
-
-  @Test
-  public void assign() {
-    RuleDto rule = newRule();
-    ComponentDto project = newProject();
-    ComponentDto file = newFile(project);
-    userSessionRule.logIn("john").addProjectUuidPermissions(UserRole.USER, project.uuid());
-
-    IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project));
-
-    UserDto user = new UserDto().setLogin("perceval").setName("Perceval");
-    db.userDao().insert(session, user);
-    session.commit();
-    index();
-
-    assertThat(getIssueByKey(issue.getKey()).assignee()).isNull();
-
-    service.assign(issue.getKey(), user.getLogin());
-
-    assertThat(getIssueByKey(issue.getKey()).assignee()).isEqualTo("perceval");
-  }
-
-  @Test
-  public void unassign() {
-    RuleDto rule = newRule();
-    ComponentDto project = newProject();
-    ComponentDto file = newFile(project);
-    userSessionRule.logIn("john").addProjectUuidPermissions(UserRole.USER, project.uuid());
-
-    IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project).setAssignee("perceval"));
-
-    UserDto user = new UserDto().setLogin("perceval").setName("Perceval");
-    db.userDao().insert(session, user);
-    session.commit();
-    index();
-
-    assertThat(getIssueByKey(issue.getKey()).assignee()).isEqualTo("perceval");
-
-    service.assign(issue.getKey(), "");
-
-    assertThat(getIssueByKey(issue.getKey()).assignee()).isNull();
-  }
-
-  @Test
-  public void fail_to_assign_on_unknown_user() {
-    RuleDto rule = newRule();
-    ComponentDto project = newProject();
-    ComponentDto file = newFile(project);
-    userSessionRule.logIn("john").addProjectUuidPermissions(UserRole.USER, project.uuid());
-
-    IssueDto issue = saveIssue(IssueTesting.newDto(rule, file, project));
-
-    try {
-      service.assign(issue.getKey(), "unknown");
-      fail();
-    } catch (Exception e) {
-      assertThat(e).isInstanceOf(BadRequestException.class).hasMessage("Unknown user: unknown");
-    }
-  }
-
   @Test
   public void list_tags() {
     RuleDto rule = newRule();
index c45d04496cc00c969ffc331348e78667d04255fa..4e0dc49ddf8e29f312e9f045c61736e9b10da859 100644 (file)
  */
 package org.sonar.server.issue.ws;
 
+import javax.annotation.Nullable;
+import org.junit.Before;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.sonar.api.config.MapSettings;
 import org.sonar.api.server.ws.Request;
 import org.sonar.api.server.ws.Response;
+import org.sonar.api.utils.internal.TestSystem2;
+import org.sonar.db.DbTester;
+import org.sonar.db.issue.IssueDto;
+import org.sonar.server.es.EsTester;
+import org.sonar.server.exceptions.ForbiddenException;
+import org.sonar.server.exceptions.NotFoundException;
 import org.sonar.server.exceptions.UnauthorizedException;
-import org.sonar.server.issue.IssueService;
+import org.sonar.server.issue.IssueFieldsSetter;
+import org.sonar.server.issue.IssueFinder;
+import org.sonar.server.issue.IssueUpdater;
+import org.sonar.server.issue.ServerIssueStorage;
+import org.sonar.server.issue.index.IssueIndexDefinition;
+import org.sonar.server.issue.index.IssueIndexer;
+import org.sonar.server.issue.index.IssueIteratorFactory;
+import org.sonar.server.notification.NotificationManager;
+import org.sonar.server.rule.DefaultRuleFinder;
 import org.sonar.server.tester.UserSessionRule;
 import org.sonar.server.ws.WsActionTester;
 
+import static org.assertj.core.api.Assertions.assertThat;
 import static org.mockito.Matchers.any;
 import static org.mockito.Matchers.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
+import static org.sonar.api.web.UserRole.CODEVIEWER;
+import static org.sonar.api.web.UserRole.USER;
 
 public class AssignActionTest {
 
+  private static final String PREVIOUS_ASSIGNEE = "previous";
+  private static final String CURRENT_USER_LOGIN = "john";
+
+  private static final long PAST = 10_000_000_000L;
+  private static final long NOW = 50_000_000_000L;
+
+  private TestSystem2 system2 = new TestSystem2().setNow(NOW);
+
   @Rule
   public ExpectedException expectedException = ExpectedException.none();
 
   @Rule
   public UserSessionRule userSession = UserSessionRule.standalone();
 
-  IssueService issueService = mock(IssueService.class);
-  OperationResponseWriter responseWriter = mock(OperationResponseWriter.class);
-  AssignAction underTest = new AssignAction(userSession, issueService, responseWriter);
-  WsActionTester tester = new WsActionTester(underTest);
+  @Rule
+  public EsTester es = new EsTester(new IssueIndexDefinition(new MapSettings()));
+
+  @Rule
+  public DbTester db = DbTester.create(system2);
+
+  private IssueIndexer issueIndexer = new IssueIndexer(es.client(), new IssueIteratorFactory(db.getDbClient()));
+  private OperationResponseWriter responseWriter = mock(OperationResponseWriter.class);
+  private AssignAction underTest = new AssignAction(system2, userSession, db.getDbClient(), new IssueFinder(db.getDbClient(), userSession), new IssueFieldsSetter(),
+    new IssueUpdater(db.getDbClient(),
+      new ServerIssueStorage(system2, new DefaultRuleFinder(db.getDbClient()), db.getDbClient(), issueIndexer), mock(NotificationManager.class)),
+    responseWriter);
+  private WsActionTester tester = new WsActionTester(underTest);
+
+  @Before
+  public void setUp() throws Exception {
+    db.users().insertUser(PREVIOUS_ASSIGNEE);
+  }
+
+  @Test
+  public void assign_to_someone() throws Exception {
+    IssueDto issue = newIssueWithBrowsePermission();
+    db.users().insertUser("arthur");
+
+    tester.newRequest()
+      .setParam("issue", issue.getKey())
+      .setParam("assignee", "arthur")
+      .execute();
+
+    checkIssueAssignee(issue.getKey(), "arthur");
+    verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class));
+  }
 
   @Test
   public void assign_to_me() throws Exception {
-    userSession.logIn("perceval");
+    IssueDto issue = newIssueWithBrowsePermission();
 
     tester.newRequest()
-      .setParam("issue", "ABC")
+      .setParam("issue", issue.getKey())
       .setParam("assignee", "_me")
       .execute();
 
-    verify(issueService).assign("ABC", "perceval");
-    verify(responseWriter).write(eq("ABC"), any(Request.class), any(Response.class));
+    checkIssueAssignee(issue.getKey(), CURRENT_USER_LOGIN);
+    verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class));
   }
 
   @Test
-  public void assign_to_me_with_deprecated_param() throws Exception {
-    userSession.logIn("perceval");
+  public void assign_to_me_using_deprecated_me_param() throws Exception {
+    IssueDto issue = newIssueWithBrowsePermission();
 
     tester.newRequest()
-      .setParam("issue", "ABC")
+      .setParam("issue", issue.getKey())
       .setParam("me", "true")
       .execute();
 
-    verify(issueService).assign("ABC", "perceval");
-    verify(responseWriter).write(eq("ABC"), any(Request.class), any(Response.class));
+    checkIssueAssignee(issue.getKey(), CURRENT_USER_LOGIN);
+    verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class));
   }
 
   @Test
-  public void assign_to_someone() throws Exception {
-    userSession.logIn("perceval");
+  public void unassign() throws Exception {
+    IssueDto issue = newIssueWithBrowsePermission();
 
     tester.newRequest()
-      .setParam("issue", "ABC")
-      .setParam("assignee", "arthur")
+      .setParam("issue", issue.getKey())
+      .execute();
+
+    checkIssueAssignee(issue.getKey(), null);
+    verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class));
+  }
+
+  @Test
+  public void unassign_with_empty_assignee_param() throws Exception {
+    IssueDto issue = newIssueWithBrowsePermission();
+
+    tester.newRequest()
+      .setParam("issue", issue.getKey())
+      .setParam("assignee", "")
+      .execute();
+
+    checkIssueAssignee(issue.getKey(), null);
+    verify(responseWriter).write(eq(issue.getKey()), any(Request.class), any(Response.class));
+  }
+
+  @Test
+  public void nothing_to_do_when_new_assignee_is_same_as_old_one() throws Exception {
+    db.users().insertUser("arthur");
+    IssueDto issue = newIssueWithBrowsePermission();
+
+    tester.newRequest()
+      .setParam("issue", issue.getKey())
+      .setParam("assignee", PREVIOUS_ASSIGNEE)
       .execute();
 
-    verify(issueService).assign("ABC", "arthur");
-    verify(responseWriter).write(eq("ABC"), any(Request.class), any(Response.class));
+    IssueDto issueReloaded = db.getDbClient().issueDao().selectByKey(db.getSession(), issue.getKey()).get();
+    assertThat(issueReloaded.getAssignee()).isEqualTo(PREVIOUS_ASSIGNEE);
+    assertThat(issueReloaded.getUpdatedAt()).isEqualTo(PAST);
+    assertThat(issueReloaded.getIssueUpdateTime()).isEqualTo(PAST);
   }
 
   @Test
-  public void must_be_authenticated_to_assign_to_me() throws Exception {
+  public void fail_when_assignee_does_not_exist() throws Exception {
+    IssueDto issue = newIssueWithBrowsePermission();
+
+    expectedException.expect(NotFoundException.class);
+
+    tester.newRequest()
+      .setParam("issue", issue.getKey())
+      .setParam("assignee", "unknown")
+      .execute();
+  }
+
+  @Test
+  public void fail_when_assignee_is_disabled() throws Exception {
+    IssueDto issue = newIssueWithBrowsePermission();
+    db.users().insertUser(user -> user.setActive(false));
+
+    expectedException.expect(NotFoundException.class);
+
+    tester.newRequest()
+      .setParam("issue", issue.getKey())
+      .setParam("assignee", "unknown")
+      .execute();
+  }
+
+  @Test
+  public void fail_when_not_authenticated() throws Exception {
+    IssueDto issue = newIssue();
+    userSession.anonymous();
+
     expectedException.expect(UnauthorizedException.class);
 
     tester.newRequest()
-      .setParam("issue", "ABC")
+      .setParam("issue", issue.getKey())
       .setParam("assignee", "_me")
       .execute();
   }
 
   @Test
-  public void unassign() throws Exception {
-    userSession.logIn("perceval");
+  public void fail_when_missing_browse_permission() throws Exception {
+    IssueDto issue = newIssue();
+    db.users().insertUser(CURRENT_USER_LOGIN);
+    userSession.logIn(CURRENT_USER_LOGIN).addProjectUuidPermissions(CODEVIEWER, issue.getProjectUuid());
+
+    expectedException.expect(ForbiddenException.class);
 
     tester.newRequest()
-      .setParam("issue", "ABC")
+      .setParam("issue", issue.getKey())
+      .setParam("assignee", "_me")
       .execute();
+  }
+
+  private IssueDto newIssue() {
+    return db.issues().insertIssue(
+      issueDto -> issueDto
+        .setAssignee(PREVIOUS_ASSIGNEE)
+        .setCreatedAt(PAST).setIssueCreationTime(PAST)
+        .setUpdatedAt(PAST).setIssueUpdateTime(PAST));
+  }
+
+  private void setUserWithBrowsePermission(IssueDto issue) {
+    db.users().insertUser(CURRENT_USER_LOGIN);
+    userSession.logIn(CURRENT_USER_LOGIN).addProjectUuidPermissions(USER, issue.getProjectUuid());
+  }
+
+  private IssueDto newIssueWithBrowsePermission() {
+    IssueDto issue = newIssue();
+    setUserWithBrowsePermission(issue);
+    return issue;
+  }
 
-    verify(issueService).assign("ABC", null);
-    verify(responseWriter).write(eq("ABC"), any(Request.class), any(Response.class));
+  private void checkIssueAssignee(String issueKey, @Nullable String expectedAssignee) {
+    IssueDto issueReloaded = db.getDbClient().issueDao().selectByKey(db.getSession(), issueKey).get();
+    assertThat(issueReloaded.getAssignee()).isEqualTo(expectedAssignee);
+    assertThat(issueReloaded.getIssueUpdateTime()).isEqualTo(NOW);
+    assertThat(issueReloaded.getUpdatedAt()).isEqualTo(NOW);
   }
 }