SONAR-4375 Improve issue changelog display by returning names instead of keys for users and action plans

8 files changed, 55 insertions, 40 deletions

diff --git a/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java b/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java
index 61995806499..735cfed12e7 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/AssignAction.java
@@ -25,6 +25,7 @@ import org.sonar.api.ServerComponent;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.condition.IsUnResolved;
 import org.sonar.api.issue.internal.DefaultIssue;
+import org.sonar.api.user.User;
 import org.sonar.api.user.UserFinder;
 import org.sonar.core.issue.IssueUpdater;
 import org.sonar.server.user.UserSession;
@@ -58,7 +59,8 @@ public class AssignAction extends Action implements ServerComponent {
 
   @Override
   public boolean execute(Map<String, Object> properties, Context context) {
-    return issueUpdater.assign((DefaultIssue) context.issue(), assignee(properties), context.issueChangeContext());
+    User user = userFinder.findByLogin(assignee(properties));
+    return issueUpdater.assign((DefaultIssue) context.issue(), user, context.issueChangeContext());
   }
 
   private String assignee(Map<String, Object> properties){
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueChangelogService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueChangelogService.java
index 809538a6f57..fa6d08b1fce 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueChangelogService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueChangelogService.java
@@ -57,6 +57,5 @@ public class IssueChangelogService implements ServerComponent {
     Collection<User> users = userFinder.findByLogins(logins);
 
     return new IssueChangelog(changes, users);
-
   }
 }
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 13e980527af..c14ff25cf1e 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueService.java
@@ -21,6 +21,7 @@ package org.sonar.server.issue;
 
 import com.google.common.base.Strings;
 import org.sonar.api.ServerComponent;
+import org.sonar.api.issue.ActionPlan;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.IssueQuery;
 import org.sonar.api.issue.IssueQueryResult;
@@ -28,6 +29,7 @@ import org.sonar.api.issue.internal.DefaultIssue;
 import org.sonar.api.issue.internal.IssueChangeContext;
 import org.sonar.api.rules.Rule;
 import org.sonar.api.rules.RuleFinder;
+import org.sonar.api.user.User;
 import org.sonar.api.user.UserFinder;
 import org.sonar.api.web.UserRole;
 import org.sonar.core.issue.IssueNotifications;
@@ -42,7 +44,6 @@ import org.sonar.core.user.AuthorizationDao;
 import org.sonar.server.user.UserSession;
 
 import javax.annotation.Nullable;
-
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Date;
@@ -123,11 +124,15 @@ public class IssueService implements ServerComponent {
     verifyLoggedIn(userSession);
     IssueQueryResult queryResult = loadIssue(issueKey);
     DefaultIssue issue = (DefaultIssue) queryResult.first();
-    if (assignee != null && userFinder.findByLogin(assignee) == null) {
-      throw new IllegalArgumentException("Unknown user: " + assignee);
+    User user = null;
+    if (!Strings.isNullOrEmpty(assignee)) {
+      user = userFinder.findByLogin(assignee);
+      if(user == null) {
+        throw new IllegalArgumentException("Unknown user: " + assignee);
+      }
     }
     IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login());
-    if (issueUpdater.assign(issue, assignee, context)) {
+    if (issueUpdater.assign(issue, user, context)) {
       issueStorage.save(issue);
       issueNotifications.sendChanges(issue, context, queryResult);
     }
@@ -136,14 +141,18 @@ 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);
+    ActionPlan actionPlan = null;
+    if (!Strings.isNullOrEmpty(actionPlanKey)) {
+      actionPlan = actionPlanService.findByKey(actionPlanKey, userSession);
+      if(actionPlan == null) {
+        throw new IllegalArgumentException("Unknown action plan: " + actionPlanKey);
+      }
     }
     IssueQueryResult queryResult = loadIssue(issueKey);
     DefaultIssue issue = (DefaultIssue) queryResult.first();
 
     IssueChangeContext context = IssueChangeContext.createUser(new Date(), userSession.login());
-    if (issueUpdater.plan(issue, actionPlanKey, context)) {
+    if (issueUpdater.plan(issue, actionPlan, context)) {
       issueStorage.save(issue);
       issueNotifications.sendChanges(issue, context, queryResult);
     }
diff --git a/sonar-server/src/main/java/org/sonar/server/issue/PlanAction.java b/sonar-server/src/main/java/org/sonar/server/issue/PlanAction.java
index 27fc90746ab..b357474775e 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/PlanAction.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/PlanAction.java
@@ -62,7 +62,8 @@ public class PlanAction extends Action implements ServerComponent {
 
   @Override
   public boolean execute(Map<String, Object> properties, Context context) {
-    return issueUpdater.plan((DefaultIssue) context.issue(), planKey(properties), context.issueChangeContext());
+    ActionPlan actionPlan = actionPlanService.findByKey(planKey(properties), UserSession.get());
+    return issueUpdater.plan((DefaultIssue) context.issue(), actionPlan, context.issueChangeContext());
   }
 
   private String planKey(Map<String, Object> properties) {
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/ActionPlanServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/ActionPlanServiceTest.java
index f27f8155f62..d49ba739016 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/ActionPlanServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/ActionPlanServiceTest.java
@@ -134,13 +134,13 @@ public class ActionPlanServiceTest {
 
     IssueDto issueDto = new IssueDto().setId(100L).setStatus(Issue.STATUS_OPEN).setRuleKey_unit_test_only("squid", "s100");
     when(issueDao.selectIssues(any(IssueQuery.class))).thenReturn(newArrayList(issueDto));
-    when(issueUpdater.plan(any(DefaultIssue.class), eq((String) null), any(IssueChangeContext.class))).thenReturn(true);
+    when(issueUpdater.plan(any(DefaultIssue.class), eq((ActionPlan) null), any(IssueChangeContext.class))).thenReturn(true);
 
     ArgumentCaptor<DefaultIssue> captor = ArgumentCaptor.forClass(DefaultIssue.class);
     actionPlanService.delete("ABCD", userSession);
     verify(actionPlanDao).delete("ABCD");
     verify(authorizationDao).isAuthorizedComponentId(anyLong(), anyInt(), eq(UserRole.ADMIN));
-    verify(issueUpdater).plan(captor.capture(), eq((String) null), any(IssueChangeContext.class));
+    verify(issueUpdater).plan(captor.capture(), eq((ActionPlan) null), any(IssueChangeContext.class));
     verify(issueStorage).save(newArrayList(captor.getAllValues()));
   }
 
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java b/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java
index 3a9cec4ac86..7c5d7d43bb0 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/AssignActionTest.java
@@ -25,6 +25,7 @@ import org.junit.Test;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.internal.DefaultIssue;
 import org.sonar.api.issue.internal.IssueChangeContext;
+import org.sonar.api.user.User;
 import org.sonar.api.user.UserFinder;
 import org.sonar.core.issue.IssueUpdater;
 import org.sonar.core.user.DefaultUser;
@@ -63,8 +64,11 @@ public class AssignActionTest {
     Action.Context context = mock(Action.Context.class);
     when(context.issue()).thenReturn(issue);
 
+    User user = new DefaultUser();
+    when(userFinder.findByLogin(assignee)).thenReturn(user);
+
     action.execute(properties, context);
-    verify(issueUpdater).assign(eq(issue), eq(assignee), any(IssueChangeContext.class));
+    verify(issueUpdater).assign(eq(issue), eq(user), any(IssueChangeContext.class));
   }
 
   @Test
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 d1c001141c9..dce2dd5eeac 100644
--- 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,6 +23,7 @@ package org.sonar.server.issue;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
+import org.sonar.api.issue.ActionPlan;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.IssueQuery;
 import org.sonar.api.issue.IssueQueryResult;
@@ -31,6 +32,7 @@ import org.sonar.api.issue.internal.IssueChangeContext;
 import org.sonar.api.rule.RuleKey;
 import org.sonar.api.rules.Rule;
 import org.sonar.api.rules.RuleFinder;
+import org.sonar.api.user.User;
 import org.sonar.api.user.UserFinder;
 import org.sonar.api.web.UserRole;
 import org.sonar.core.issue.DefaultActionPlan;
@@ -180,15 +182,16 @@ public class IssueServiceTest {
   @Test
   public void should_assign() {
     String assignee = "perceval";
+    User user = new DefaultUser();
 
-    when(userFinder.findByLogin(assignee)).thenReturn(new DefaultUser());
-    when(issueUpdater.assign(eq(issue), eq(assignee), any(IssueChangeContext.class))).thenReturn(true);
+    when(userFinder.findByLogin(assignee)).thenReturn(user);
+    when(issueUpdater.assign(eq(issue), eq(user), any(IssueChangeContext.class))).thenReturn(true);
 
     Issue result = issueService.assign("ABCD", assignee, userSession);
     assertThat(result).isNotNull();
 
     ArgumentCaptor<IssueChangeContext> measureCaptor = ArgumentCaptor.forClass(IssueChangeContext.class);
-    verify(issueUpdater).assign(eq(issue), eq(assignee), measureCaptor.capture());
+    verify(issueUpdater).assign(eq(issue), eq(user), measureCaptor.capture());
     verify(issueStorage).save(issue);
 
     IssueChangeContext issueChangeContext = measureCaptor.getValue();
@@ -201,13 +204,15 @@ public class IssueServiceTest {
   @Test
   public void should_not_assign() {
     String assignee = "perceval";
+    User user = new DefaultUser();
 
-    when(userFinder.findByLogin(assignee)).thenReturn(new DefaultUser());
-    when(issueUpdater.assign(eq(issue), eq(assignee), any(IssueChangeContext.class))).thenReturn(false);
+    when(userFinder.findByLogin(assignee)).thenReturn(user);
+    when(issueUpdater.assign(eq(issue), eq(user), any(IssueChangeContext.class))).thenReturn(false);
 
     Issue result = issueService.assign("ABCD", assignee, userSession);
     assertThat(result).isNotNull();
-    verify(issueUpdater).assign(eq(issue), eq(assignee),any(IssueChangeContext.class));
+
+    verify(issueUpdater).assign(eq(issue), eq(user), any(IssueChangeContext.class));
     verifyZeroInteractions(issueStorage);
     verifyZeroInteractions(issueNotifications);
   }
@@ -234,14 +239,16 @@ public class IssueServiceTest {
   public void should_plan() {
     String actionPlanKey = "EFGH";
 
-    when(actionPlanService.findByKey(actionPlanKey, userSession)).thenReturn(new DefaultActionPlan());
-    when(issueUpdater.plan(eq(issue), eq(actionPlanKey), any(IssueChangeContext.class))).thenReturn(true);
+    ActionPlan actionPlan = new DefaultActionPlan();
+
+    when(actionPlanService.findByKey(actionPlanKey, userSession)).thenReturn(actionPlan);
+    when(issueUpdater.plan(eq(issue), eq(actionPlan), any(IssueChangeContext.class))).thenReturn(true);
 
     Issue result = issueService.plan("ABCD", actionPlanKey, userSession);
     assertThat(result).isNotNull();
 
     ArgumentCaptor<IssueChangeContext> measureCaptor = ArgumentCaptor.forClass(IssueChangeContext.class);
-    verify(issueUpdater).plan(eq(issue), eq(actionPlanKey), measureCaptor.capture());
+    verify(issueUpdater).plan(eq(issue), eq(actionPlan), measureCaptor.capture());
     verify(issueStorage).save(issue);
 
     IssueChangeContext issueChangeContext = measureCaptor.getValue();
@@ -255,12 +262,14 @@ public class IssueServiceTest {
   public void should_not_plan() {
     String actionPlanKey = "EFGH";
 
-    when(actionPlanService.findByKey(actionPlanKey, userSession)).thenReturn(new DefaultActionPlan());
-    when(issueUpdater.plan(eq(issue), eq(actionPlanKey), any(IssueChangeContext.class))).thenReturn(false);
+    ActionPlan actionPlan = new DefaultActionPlan();
+
+    when(actionPlanService.findByKey(actionPlanKey, userSession)).thenReturn(actionPlan);
+    when(issueUpdater.plan(eq(issue), eq(actionPlan), any(IssueChangeContext.class))).thenReturn(false);
 
     Issue result = issueService.plan("ABCD", actionPlanKey, userSession);
     assertThat(result).isNotNull();
-    verify(issueUpdater).plan(eq(issue), eq(actionPlanKey), any(IssueChangeContext.class));
+    verify(issueUpdater).plan(eq(issue), eq(actionPlan), any(IssueChangeContext.class));
     verifyZeroInteractions(issueStorage);
     verifyZeroInteractions(issueNotifications);
   }
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/PlanActionTest.java b/sonar-server/src/test/java/org/sonar/server/issue/PlanActionTest.java
index f524b7b12e6..a1a5fcded8d 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/PlanActionTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/PlanActionTest.java
@@ -22,6 +22,7 @@ package org.sonar.server.issue;
 
 import org.junit.Before;
 import org.junit.Test;
+import org.sonar.api.issue.ActionPlan;
 import org.sonar.api.issue.Issue;
 import org.sonar.api.issue.internal.DefaultIssue;
 import org.sonar.api.issue.internal.IssueChangeContext;
@@ -61,33 +62,23 @@ public class PlanActionTest {
     Action.Context context = mock(Action.Context.class);
     when(context.issue()).thenReturn(issue);
 
-    action.execute(properties, context);
-    verify(issueUpdater).plan(eq(issue), eq(planKey), any(IssueChangeContext.class));
-  }
-
-  @Test
-  public void should_execute_on_null_action_plan(){
-    Map<String, Object> properties = newHashMap();
-    DefaultIssue issue = mock(DefaultIssue.class);
-
-    Action.Context context = mock(Action.Context.class);
-    when(context.issue()).thenReturn(issue);
+    ActionPlan actionPlan = new DefaultActionPlan();
+    when(actionPlanService.findByKey(eq(planKey), any(UserSession.class))).thenReturn(actionPlan);
 
     action.execute(properties, context);
-    verify(issueUpdater).plan(eq(issue), eq((String) null), any(IssueChangeContext.class));
+    verify(issueUpdater).plan(eq(issue), eq(actionPlan), any(IssueChangeContext.class));
   }
 
   @Test
-  public void should_execute_on_empty_action_plan(){
+  public void should_execute_on_null_action_plan(){
     Map<String, Object> properties = newHashMap();
-    properties.put("plan", "");
     DefaultIssue issue = mock(DefaultIssue.class);
 
     Action.Context context = mock(Action.Context.class);
     when(context.issue()).thenReturn(issue);
 
     action.execute(properties, context);
-    verify(issueUpdater).plan(eq(issue), eq(""), any(IssueChangeContext.class));
+    verify(issueUpdater).plan(eq(issue), eq((ActionPlan) null), any(IssueChangeContext.class));
   }
 
   @Test