From: Julien Lancelot Date: Tue, 14 Mar 2017 10:20:51 +0000 (+0100) Subject: SONAR-8899 Move assign code from IssueService to AssignAction X-Git-Tag: 6.4-RC1~720 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=992d2c44503eabebcdba9a3837c38d5341b39782;p=sonarqube.git SONAR-8899 Move assign code from IssueService to AssignAction --- diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueTesting.java b/server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueTesting.java index 748d3bb8b02..33ff23722c4 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueTesting.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/issue/IssueTesting.java @@ -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) diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueDbTester.java b/server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueDbTester.java index a21cb1b04dc..9ef1a22cf34 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueDbTester.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/issue/IssueDbTester.java @@ -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 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) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java index 09e56f45b3a..a72d2322f75 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java @@ -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 */ diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java index abf15d50d86..33b6b611a88 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/ws/AssignAction.java @@ -19,16 +19,29 @@ */ 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); } } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java index f4b8c5f5f1e..e3d516d96c3 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueServiceMediumTest.java @@ -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(); diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java index c45d04496cc..4e0dc49ddf8 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/AssignActionTest.java @@ -19,92 +19,232 @@ */ 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); } }