From f766f2235a90fdc2e033ec9964215e223ca7e003 Mon Sep 17 00:00:00 2001 From: Julien Lancelot Date: Tue, 14 Mar 2017 09:28:20 +0100 Subject: [PATCH] SONAR-8899 Replace User by UserDto in Function --- .../org/sonar/server/issue/AssignAction.java | 42 +++---- .../sonar/server/issue/IssueFieldsSetter.java | 8 +- .../org/sonar/server/issue/IssueService.java | 14 +-- .../sonar/server/issue/workflow/Function.java | 4 +- .../issue/workflow/FunctionExecutor.java | 4 +- .../sonar/server/issue/AssignActionTest.java | 116 ++++++++---------- .../server/issue/IssueFieldsSetterTest.java | 12 +- .../server/issue/ws/BulkChangeActionTest.java | 3 +- 8 files changed, 89 insertions(+), 114 deletions(-) diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java b/server/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java index d80ccbacf44..bbc5e6e87aa 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java @@ -19,16 +19,20 @@ */ package org.sonar.server.issue; -import com.google.common.base.Strings; import java.util.Collection; import java.util.Map; import org.sonar.api.issue.condition.IsUnResolved; import org.sonar.api.server.ServerSide; -import org.sonar.api.user.User; -import org.sonar.api.user.UserFinder; import org.sonar.core.issue.DefaultIssue; +import org.sonar.db.DbClient; +import org.sonar.db.DbSession; +import org.sonar.db.user.UserDto; import org.sonar.server.user.UserSession; +import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.base.Strings.isNullOrEmpty; +import static org.sonar.server.ws.WsUtils.checkFound; + @ServerSide public class AssignAction extends Action { @@ -36,45 +40,37 @@ public class AssignAction extends Action { public static final String ASSIGNEE_PARAMETER = "assignee"; public static final String VERIFIED_ASSIGNEE = "verifiedAssignee"; - private final UserFinder userFinder; + private final DbClient dbClient; private final IssueFieldsSetter issueUpdater; - public AssignAction(UserFinder userFinder, IssueFieldsSetter issueUpdater) { + public AssignAction(DbClient dbClient, IssueFieldsSetter issueUpdater) { super(ASSIGN_KEY); - this.userFinder = userFinder; + this.dbClient = dbClient; this.issueUpdater = issueUpdater; super.setConditions(new IsUnResolved()); } @Override public boolean verify(Map properties, Collection issues, UserSession userSession) { - String assignee = assigneeValue(properties); - if (!Strings.isNullOrEmpty(assignee)) { - User user = selectUser(assignee); - if (user == null) { - throw new IllegalArgumentException("Unknown user: " + assignee); - } - properties.put(VERIFIED_ASSIGNEE, user); - } else { - properties.put(VERIFIED_ASSIGNEE, null); - } + String assignee = getAssigneeValue(properties); + properties.put(VERIFIED_ASSIGNEE, isNullOrEmpty(assignee) ? null : getUser(assignee)); return true; } @Override public boolean execute(Map properties, Context context) { - if (!properties.containsKey(VERIFIED_ASSIGNEE)) { - throw new IllegalArgumentException("Assignee is missing from the execution parameters"); - } - User assignee = (User) properties.get(VERIFIED_ASSIGNEE); + checkArgument(properties.containsKey(VERIFIED_ASSIGNEE), "Assignee is missing from the execution parameters"); + UserDto assignee = (UserDto) properties.get(VERIFIED_ASSIGNEE); return issueUpdater.assign(context.issue(), assignee, context.issueChangeContext()); } - private String assigneeValue(Map properties) { + private static String getAssigneeValue(Map properties) { return (String) properties.get(ASSIGNEE_PARAMETER); } - private User selectUser(String assigneeKey) { - return userFinder.findByLogin(assigneeKey); + private UserDto getUser(String assigneeKey) { + try (DbSession dbSession = dbClient.openSession(false)) { + return checkFound(dbClient.userDao().selectByLogin(dbSession, assigneeKey), "Unknown user: %s", assigneeKey); + } } } diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java index 6b01d22dfd8..2a2b2434c47 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/IssueFieldsSetter.java @@ -33,12 +33,12 @@ import org.sonar.api.ce.ComputeEngineSide; import org.sonar.api.rules.RuleType; import org.sonar.api.server.ServerSide; import org.sonar.api.server.rule.RuleTagFormat; -import org.sonar.api.user.User; import org.sonar.api.utils.Duration; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.DefaultIssueComment; import org.sonar.core.issue.IssueChangeContext; import org.sonar.core.util.stream.Collectors; +import org.sonar.db.user.UserDto; import static com.google.common.base.Preconditions.checkState; import static com.google.common.base.Strings.isNullOrEmpty; @@ -109,13 +109,13 @@ public class IssueFieldsSetter { return false; } - public boolean assign(DefaultIssue issue, @Nullable User user, IssueChangeContext context) { + public boolean assign(DefaultIssue issue, @Nullable UserDto user, IssueChangeContext context) { String sanitizedAssignee = null; if (user != null) { - sanitizedAssignee = StringUtils.defaultIfBlank(user.login(), null); + sanitizedAssignee = StringUtils.defaultIfBlank(user.getLogin(), null); } if (!Objects.equals(sanitizedAssignee, issue.assignee())) { - String newAssigneeName = user != null ? user.name() : null; + String newAssigneeName = user != null ? user.getName() : null; issue.setFieldChange(context, ASSIGNEE, UNUSED, newAssigneeName); issue.setAssignee(sanitizedAssignee); issue.setUpdateDate(context.date()); 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 61d65a33449..09e56f45b3a 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 @@ -27,12 +27,11 @@ import java.util.Map; import javax.annotation.Nullable; import org.sonar.api.ce.ComputeEngineSide; import org.sonar.api.server.ServerSide; -import org.sonar.api.user.User; -import org.sonar.api.user.UserFinder; 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; @@ -48,17 +47,14 @@ public class IssueService { private final IssueFinder issueFinder; private final IssueFieldsSetter issueFieldsSetter; private final IssueUpdater issueUpdater; - private final UserFinder userFinder; private final UserSession userSession; - public IssueService(DbClient dbClient, IssueIndex issueIndex, IssueFinder issueFinder, IssueFieldsSetter issueFieldsSetter, IssueUpdater issueUpdater, - UserFinder userFinder, UserSession userSession) { + public IssueService(DbClient dbClient, IssueIndex issueIndex, IssueFinder issueFinder, IssueFieldsSetter issueFieldsSetter, IssueUpdater issueUpdater, UserSession userSession) { this.dbClient = dbClient; this.issueIndex = issueIndex; this.issueFinder = issueFinder; this.issueFieldsSetter = issueFieldsSetter; this.issueUpdater = issueUpdater; - this.userFinder = userFinder; this.userSession = userSession; } @@ -68,10 +64,10 @@ public class IssueService { DbSession session = dbClient.openSession(false); try { DefaultIssue issue = issueFinder.getByKey(session, issueKey).toDefaultIssue(); - User user = null; + UserDto user = null; if (!Strings.isNullOrEmpty(assignee)) { - user = userFinder.findByLogin(assignee); - checkRequest(user != null, "Unknown user: %s", 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)) { diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/workflow/Function.java b/server/sonar-server/src/main/java/org/sonar/server/issue/workflow/Function.java index 8dec0e27cf3..fe805d55170 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/workflow/Function.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/workflow/Function.java @@ -21,13 +21,13 @@ package org.sonar.server.issue.workflow; import javax.annotation.Nullable; import org.sonar.api.issue.Issue; -import org.sonar.api.user.User; +import org.sonar.db.user.UserDto; interface Function { interface Context { Issue issue(); - Context setAssignee(@Nullable User user); + Context setAssignee(@Nullable UserDto user); Context setResolution(@Nullable String s); diff --git a/server/sonar-server/src/main/java/org/sonar/server/issue/workflow/FunctionExecutor.java b/server/sonar-server/src/main/java/org/sonar/server/issue/workflow/FunctionExecutor.java index 363cfa4fccb..5d7fc02a47c 100644 --- a/server/sonar-server/src/main/java/org/sonar/server/issue/workflow/FunctionExecutor.java +++ b/server/sonar-server/src/main/java/org/sonar/server/issue/workflow/FunctionExecutor.java @@ -23,9 +23,9 @@ import javax.annotation.Nullable; import org.sonar.api.ce.ComputeEngineSide; import org.sonar.api.issue.Issue; import org.sonar.api.server.ServerSide; -import org.sonar.api.user.User; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.IssueChangeContext; +import org.sonar.db.user.UserDto; import org.sonar.server.issue.IssueFieldsSetter; @ServerSide @@ -64,7 +64,7 @@ public class FunctionExecutor { } @Override - public Function.Context setAssignee(@Nullable User user) { + public Function.Context setAssignee(@Nullable UserDto user) { updater.assign(issue, user, changeContext); return this; } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java index 52e5341cf44..8ab7dbe7b2f 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java @@ -19,108 +19,92 @@ */ package org.sonar.server.issue; -import java.util.List; +import com.google.common.collect.ImmutableMap; +import java.util.Date; +import java.util.HashMap; import java.util.Map; -import org.junit.Before; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; import org.sonar.api.issue.Issue; -import org.sonar.api.user.User; -import org.sonar.api.user.UserFinder; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.IssueChangeContext; -import org.sonar.core.user.DefaultUser; -import org.sonar.server.user.ThreadLocalUserSession; - -import static com.google.common.collect.Lists.newArrayList; -import static com.google.common.collect.Maps.newHashMap; +import org.sonar.db.DbTester; +import org.sonar.db.user.UserDto; +import org.sonar.server.exceptions.NotFoundException; +import org.sonar.server.issue.ws.BulkChangeAction; +import org.sonar.server.tester.UserSessionRule; + +import static java.util.Collections.emptyMap; +import static java.util.Collections.singletonList; import static org.assertj.core.api.Assertions.assertThat; -import static org.mockito.Mockito.any; -import static org.mockito.Mockito.eq; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.when; +import static org.sonar.db.user.UserTesting.newUserDto; public class AssignActionTest { - private AssignAction action; - - private final UserFinder userFinder = mock(UserFinder.class); - private IssueFieldsSetter issueUpdater = mock(IssueFieldsSetter.class); + @Rule + public ExpectedException expectedException = ExpectedException.none(); @Rule - public ExpectedException throwable = ExpectedException.none(); + public UserSessionRule userSession = UserSessionRule.standalone(); - @Before - public void before() { - action = new AssignAction(userFinder, issueUpdater); - } + @Rule + public DbTester dbTester = DbTester.create(); - @Test - public void should_execute() { - User assignee = new DefaultUser(); + private IssueFieldsSetter issueUpdater = new IssueFieldsSetter(); - Map properties = newHashMap(); - properties.put(AssignAction.VERIFIED_ASSIGNEE, assignee); - DefaultIssue issue = mock(DefaultIssue.class); + private IssueChangeContext issueChangeContext = IssueChangeContext.createUser(new Date(), "emmerik"); - Action.Context context = mock(Action.Context.class); - when(context.issue()).thenReturn(issue); + private DefaultIssue issue = new DefaultIssue().setKey("ABC"); + private Action.Context context = new BulkChangeAction.ActionContext(issue, issueChangeContext); - action.execute(properties, context); - verify(issueUpdater).assign(eq(issue), eq(assignee), any(IssueChangeContext.class)); - } + private AssignAction action = new AssignAction(dbTester.getDbClient(), issueUpdater); @Test - public void should_fail_if_assignee_is_not_verified() { - throwable.expect(IllegalArgumentException.class); - throwable.expectMessage("Assignee is missing from the execution parameters"); + public void assign_issue() { + UserDto assignee = newUserDto(); - Map properties = newHashMap(); + action.execute(ImmutableMap.of("verifiedAssignee", assignee), context); - Action.Context context = mock(Action.Context.class); + assertThat(issue.assignee()).isEqualTo(assignee.getLogin()); + } + + @Test + public void fail_if_assignee_is_not_verified() { + expectedException.expect(IllegalArgumentException.class); + expectedException.expectMessage("Assignee is missing from the execution parameters"); - action.execute(properties, context); + action.execute(emptyMap(), context); } @Test - public void should_verify_assignee_exists() { + public void verify_that_assignee_exists() { String assignee = "arthur"; - Map properties = newHashMap(); - properties.put("assignee", assignee); + Map properties = new HashMap<>(ImmutableMap.of("assignee", assignee)); + UserDto user = dbTester.users().insertUser(assignee); - User user = new DefaultUser().setLogin(assignee); + boolean verify = action.verify(properties, singletonList(issue), userSession); - List issues = newArrayList(new DefaultIssue().setKey("ABC")); - when(userFinder.findByLogin(assignee)).thenReturn(user); - assertThat(action.verify(properties, issues, mock(ThreadLocalUserSession.class))).isTrue(); - assertThat(properties.get(AssignAction.VERIFIED_ASSIGNEE)).isEqualTo(user); + assertThat(verify).isTrue(); + UserDto verifiedUser = (UserDto) properties.get(AssignAction.VERIFIED_ASSIGNEE); + assertThat(verifiedUser.getLogin()).isEqualTo(user.getLogin()); } @Test - public void should_fail_if_assignee_does_not_exists() { - throwable.expect(IllegalArgumentException.class); - throwable.expectMessage("Unknown user: arthur"); + public void fail_if_assignee_does_not_exists() { + expectedException.expect(NotFoundException.class); + expectedException.expectMessage("Unknown user: arthur"); - String assignee = "arthur"; - Map properties = newHashMap(); - properties.put("assignee", assignee); - - List issues = newArrayList(new DefaultIssue().setKey("ABC")); - when(userFinder.findByLogin(assignee)).thenReturn(null); - action.verify(properties, issues, mock(ThreadLocalUserSession.class)); + action.verify(ImmutableMap.of("assignee", "arthur"), singletonList(issue), userSession); } @Test - public void should_unassign_if_assignee_is_empty() { - String assignee = ""; - Map properties = newHashMap(); - properties.put("assignee", assignee); - - List issues = newArrayList(new DefaultIssue().setKey("ABC")); - action.verify(properties, issues, mock(ThreadLocalUserSession.class)); - assertThat(action.verify(properties, issues, mock(ThreadLocalUserSession.class))).isTrue(); + public void unassign_issue_if_assignee_is_empty() { + Map properties = new HashMap<>(ImmutableMap.of("assignee", "")); + + boolean verify = action.verify(properties, singletonList(issue), userSession); + + assertThat(verify).isTrue(); assertThat(properties.containsKey(AssignAction.VERIFIED_ASSIGNEE)).isTrue(); assertThat(properties.get(AssignAction.VERIFIED_ASSIGNEE)).isNull(); } diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java index a7d4ca5baf0..bff828755ad 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/IssueFieldsSetterTest.java @@ -26,14 +26,14 @@ import org.apache.commons.lang.time.DateUtils; import org.junit.Rule; import org.junit.Test; import org.junit.rules.ExpectedException; -import org.sonar.api.user.User; import org.sonar.api.utils.Duration; import org.sonar.core.issue.DefaultIssue; import org.sonar.core.issue.FieldDiffs; import org.sonar.core.issue.IssueChangeContext; -import org.sonar.core.user.DefaultUser; +import org.sonar.db.user.UserDto; import static org.assertj.core.api.Assertions.assertThat; +import static org.sonar.db.user.UserTesting.newUserDto; import static org.sonar.server.issue.IssueFieldsSetter.ASSIGNEE; import static org.sonar.server.issue.IssueFieldsSetter.RESOLUTION; import static org.sonar.server.issue.IssueFieldsSetter.SEVERITY; @@ -53,7 +53,7 @@ public class IssueFieldsSetterTest { @Test public void assign() { - User user = new DefaultUser().setLogin("emmerik").setName("Emmerik"); + UserDto user = newUserDto().setLogin("emmerik").setName("Emmerik"); boolean updated = updater.assign(issue, user, context); assertThat(updated).isTrue(); @@ -67,7 +67,7 @@ public class IssueFieldsSetterTest { @Test public void unassign() { issue.setAssignee("morgan"); - boolean updated = updater.assign(issue, null, context); + boolean updated = updater.assign(issue, (UserDto) null, context); assertThat(updated).isTrue(); assertThat(issue.assignee()).isNull(); assertThat(issue.mustSendNotifications()).isTrue(); @@ -78,7 +78,7 @@ public class IssueFieldsSetterTest { @Test public void change_assignee() { - User user = new DefaultUser().setLogin("emmerik").setName("Emmerik"); + UserDto user = newUserDto().setLogin("emmerik").setName("Emmerik"); issue.setAssignee("morgan"); boolean updated = updater.assign(issue, user, context); @@ -92,7 +92,7 @@ public class IssueFieldsSetterTest { @Test public void not_change_assignee() { - User user = new DefaultUser().setLogin("morgan").setName("Morgan"); + UserDto user = newUserDto().setLogin("morgan").setName("Morgan"); issue.setAssignee("morgan"); boolean updated = updater.assign(issue, user, context); diff --git a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/BulkChangeActionTest.java b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/BulkChangeActionTest.java index c287b05d303..d811d6cfc12 100644 --- a/server/sonar-server/src/test/java/org/sonar/server/issue/ws/BulkChangeActionTest.java +++ b/server/sonar-server/src/test/java/org/sonar/server/issue/ws/BulkChangeActionTest.java @@ -58,7 +58,6 @@ import org.sonar.server.issue.workflow.IssueWorkflow; import org.sonar.server.notification.NotificationManager; import org.sonar.server.rule.DefaultRuleFinder; import org.sonar.server.tester.UserSessionRule; -import org.sonar.server.user.DefaultUserFinder; import org.sonar.server.ws.TestRequest; import org.sonar.server.ws.WsActionTester; import org.sonarqube.ws.Issues.BulkChangeWsResponse; @@ -517,7 +516,7 @@ public class BulkChangeActionTest { } private void addActions() { - actions.add(new org.sonar.server.issue.AssignAction(new DefaultUserFinder(db.getDbClient()), issueFieldsSetter)); + actions.add(new org.sonar.server.issue.AssignAction(db.getDbClient(), issueFieldsSetter)); actions.add(new org.sonar.server.issue.SetSeverityAction(issueFieldsSetter, userSession)); actions.add(new org.sonar.server.issue.SetTypeAction(issueFieldsSetter, userSession)); actions.add(new org.sonar.server.issue.TransitionAction(new TransitionService(userSession, issueWorkflow))); -- 2.39.5