From 0b1335a4b2297830f5b40fc4271caad3c2d74c24 Mon Sep 17 00:00:00 2001
From: Julien Lancelot <julien.lancelot@gmail.com>
Date: Thu, 4 Jul 2013 14:28:18 +0200
Subject: [PATCH] SONAR-4421 Improve issues bulk change errors handling

---
 .../server/issue/IssueBulkChangeService.java      | 12 +++---------
 .../server/issue/IssueBulkChangeServiceTest.java  | 15 +++++++--------
 2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
index cc23710a2c6..71c72db08a1 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeService.java
@@ -32,6 +32,7 @@ import org.sonar.api.issue.internal.IssueChangeContext;
 import org.sonar.api.web.UserRole;
 import org.sonar.core.issue.IssueNotifications;
 import org.sonar.core.issue.db.IssueStorage;
+import org.sonar.server.exceptions.BadRequestException;
 import org.sonar.server.user.UserSession;
 
 import javax.annotation.CheckForNull;
@@ -59,7 +60,7 @@ public class IssueBulkChangeService {
 
   public IssueBulkChangeResult execute(IssueBulkChangeQuery issueBulkChangeQuery, UserSession userSession) {
     LOG.debug("BulkChangeQuery : {}", issueBulkChangeQuery);
-    verifyLoggedIn(userSession);
+    userSession.checkLoggedIn();
 
     IssueBulkChangeResult result = new IssueBulkChangeResult();
     IssueQueryResult issueQueryResult = issueFinder.find(IssueQuery.builder().issueKeys(issueBulkChangeQuery.issues()).pageSize(-1).requiredRole(UserRole.USER).build());
@@ -68,7 +69,7 @@ public class IssueBulkChangeService {
     for (String actionName : issueBulkChangeQuery.actions()) {
       Action action = getAction(actionName);
       if (action == null) {
-        throw new IllegalArgumentException("The action : '"+ actionName + "' is unknown");
+        throw new BadRequestException("The action : '"+ actionName + "' is unknown");
       }
       action.verify(issueBulkChangeQuery.properties(actionName), issues, userSession);
       bulkActions.add(action);
@@ -107,13 +108,6 @@ public class IssueBulkChangeService {
     }, null);
   }
 
-  private void verifyLoggedIn(UserSession userSession) {
-    if (!userSession.isLoggedIn()) {
-      // must be logged
-      throw new IllegalStateException("User is not logged in");
-    }
-  }
-
   static class ActionContext implements Action.Context {
     private final Issue issue;
     private final IssueChangeContext changeContext;
diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceTest.java
index 0d42dc13c8c..09bc9e1fd26 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeServiceTest.java
@@ -31,6 +31,9 @@ import org.sonar.api.issue.internal.IssueChangeContext;
 import org.sonar.api.web.UserRole;
 import org.sonar.core.issue.IssueNotifications;
 import org.sonar.core.issue.db.IssueStorage;
+import org.sonar.server.exceptions.BadRequestException;
+import org.sonar.server.exceptions.UnauthorizedException;
+import org.sonar.server.user.MockUserSession;
 import org.sonar.server.user.UserSession;
 
 import java.util.List;
@@ -51,7 +54,7 @@ public class IssueBulkChangeServiceTest {
   private IssueNotifications issueNotifications = mock(IssueNotifications.class);
 
   private IssueQueryResult issueQueryResult = mock(IssueQueryResult.class);
-  private UserSession userSession = mock(UserSession.class);
+  private UserSession userSession = MockUserSession.create().setLogin("john").setUserId(10);
   private DefaultIssue issue = new DefaultIssue().setKey("ABCD");
 
   private IssueBulkChangeService service;
@@ -61,10 +64,6 @@ public class IssueBulkChangeServiceTest {
 
   @Before
   public void before() {
-    when(userSession.isLoggedIn()).thenReturn(true);
-    when(userSession.userId()).thenReturn(10);
-    when(userSession.login()).thenReturn("fred");
-
     when(finder.find(any(IssueQuery.class))).thenReturn(issueQueryResult);
     when(issueQueryResult.issues()).thenReturn(newArrayList((Issue) issue));
 
@@ -208,7 +207,7 @@ public class IssueBulkChangeServiceTest {
 
   @Test
   public void should_fail_if_user_not_logged() {
-    when(userSession.isLoggedIn()).thenReturn(false);
+    userSession = MockUserSession.create().setLogin(null);
 
     Map<String, Object> properties = newHashMap();
     properties.put("issues", "ABCD");
@@ -219,7 +218,7 @@ public class IssueBulkChangeServiceTest {
       service.execute(issueBulkChangeQuery, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+      assertThat(e).isInstanceOf(UnauthorizedException.class);
     }
     verifyZeroInteractions(issueStorage);
     verifyZeroInteractions(issueNotifications);
@@ -236,7 +235,7 @@ public class IssueBulkChangeServiceTest {
       service.execute(issueBulkChangeQuery, userSession);
       fail();
     } catch (Exception e) {
-      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("The action : 'unknown' is unknown");
+      assertThat(e).isInstanceOf(BadRequestException.class).hasMessage("The action : 'unknown' is unknown");
     }
     verifyZeroInteractions(issueStorage);
     verifyZeroInteractions(issueNotifications);
-- 
2.39.5