Fix quality flaws

diff --git a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
index 72c6a60ea333f602f31ee515e7e7fd5055fe7567..11ebef8736f553ecc8c59642aad178cae6d07cbe 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
@@ -397,8 +397,8 @@ public class InternalRubyIssueService implements ServerComponent {
   public boolean isUserAuthorized(DefaultIssueFilter issueFilter) {
     try {
       UserSession userSession = UserSession.get();
-      issueFilterService.verifyLoggedIn(userSession);
-      issueFilterService.verifyCurrentUserCanReadFilter(issueFilter, userSession);
+      String user = issueFilterService.getNotNullLogin(userSession);
+      issueFilterService.verifyCurrentUserCanReadFilter(issueFilter, user);
       return true;
     } catch (Exception e) {
       return false;

diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeQuery.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeQuery.java
index b4e0e31dabbf85279902b029a190b46eb8f505a2..6d1a0e4f9fd3e44b7d154b4e5602eef136ac919e 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeQuery.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueBulkChangeQuery.java
@@ -19,6 +19,7 @@
  */
 package org.sonar.server.issue;
 
+import com.google.common.base.Preconditions;
 import org.apache.commons.lang.builder.ReflectionToStringBuilder;
 
 import javax.annotation.CheckForNull;
@@ -34,11 +35,11 @@ import static com.google.common.collect.Lists.newArrayList;
  */
 public class IssueBulkChangeQuery {
 
-  private static final String ASSIGNEE = "ASSIGNEE";
-  private static final String PLAN = "PLAN";
-  private static final String SEVERITY = "SEVERITY";
-  private static final String TRANSITION = "TRANSITION";
-  private static final String COMMENT = "COMMENT";
+  private static final String ASSIGNEE = "assign";
+  private static final String PLAN = "plan";
+  private static final String SEVERITY = "set_severity";
+  private static final String TRANSITION = "do_transition";
+  private static final String COMMENT = "comment";
 
   private final Collection<String> actions;
   private final Collection<String> issueKeys;
@@ -165,6 +166,9 @@ public class IssueBulkChangeQuery {
     }
 
     public IssueBulkChangeQuery build() {
+      Preconditions.checkArgument(issueKeys != null && !issueKeys.isEmpty(), "Issues must not be empty");
+      Preconditions.checkArgument(!actions.isEmpty(), "At least one action must be provided");
+
       return new IssueBulkChangeQuery(this);
     }
   }

diff --git a/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java b/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java
index 9c22a237e2459630a788be3239b52a6d1fea7704..7eaae9e62baacf5ec17ad8b0a18c5e52b53cdb58 100644 (file)
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java
@@ -38,7 +38,6 @@ import org.sonar.server.user.UserSession;
 
 import javax.annotation.CheckForNull;
 
-import java.util.Collections;
 import java.util.List;
 import java.util.Map;
 
@@ -69,7 +68,7 @@ public class IssueFilterService implements ServerComponent {
   }
 
   public DefaultIssueFilter find(Long id, UserSession userSession) {
-    return findIssueFilterDto(id, userSession).toIssueFilter();
+    return findIssueFilterDto(id, getNotNullLogin(userSession)).toIssueFilter();
   }
 
   @CheckForNull
@@ -82,31 +81,30 @@ public class IssueFilterService implements ServerComponent {
   }
 
   public List<DefaultIssueFilter> findByUser(UserSession userSession) {
-    if (userSession.isLoggedIn() && userSession.login() != null) {
-      return toIssueFilters(issueFilterDao.selectByUser(userSession.login()));
-    }
-    return Collections.emptyList();
+    return toIssueFilters(issueFilterDao.selectByUser(getNotNullLogin(userSession)));
   }
 
   public DefaultIssueFilter save(DefaultIssueFilter issueFilter, UserSession userSession) {
-    verifyLoggedIn(userSession);
-    issueFilter.setUser(userSession.login());
-    validateFilter(issueFilter, userSession);
-    return insertIssueFilter(issueFilter, userSession.login());
+    String user = getNotNullLogin(userSession);
+    issueFilter.setUser(user);
+    validateFilter(issueFilter, user);
+    return insertIssueFilter(issueFilter, user);
   }
 
   public DefaultIssueFilter update(DefaultIssueFilter issueFilter, UserSession userSession) {
-    IssueFilterDto issueFilterDto = findIssueFilterDto(issueFilter.id(), userSession);
-    verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), userSession);
-    validateFilter(issueFilter, userSession);
+    String user = getNotNullLogin(userSession);
+    IssueFilterDto issueFilterDto = findIssueFilterDto(issueFilter.id(), user);
+    verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), user);
+    validateFilter(issueFilter, user);
 
     issueFilterDao.update(IssueFilterDto.toIssueFilter(issueFilter));
     return issueFilter;
   }
 
   public DefaultIssueFilter updateFilterQuery(Long issueFilterId, Map<String, Object> filterQuery, UserSession userSession) {
-    IssueFilterDto issueFilterDto = findIssueFilterDto(issueFilterId, userSession);
-    verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), userSession);
+    String user = getNotNullLogin(userSession);
+    IssueFilterDto issueFilterDto = findIssueFilterDto(issueFilterId, user);
+    verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), user);
 
     DefaultIssueFilter issueFilter = issueFilterDto.toIssueFilter();
     issueFilter.setData(serializeFilterQuery(filterQuery));
@@ -115,41 +113,38 @@ public class IssueFilterService implements ServerComponent {
   }
 
   public void delete(Long issueFilterId, UserSession userSession) {
-    IssueFilterDto issueFilterDto = findIssueFilterDto(issueFilterId, userSession);
-    verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), userSession);
+    String user = getNotNullLogin(userSession);
+    IssueFilterDto issueFilterDto = findIssueFilterDto(issueFilterId, user);
+    verifyCurrentUserCanModifyFilter(issueFilterDto.toIssueFilter(), user);
 
     deleteFavouriteIssueFilters(issueFilterDto);
     issueFilterDao.delete(issueFilterId);
   }
 
   public DefaultIssueFilter copy(Long issueFilterIdToCopy, DefaultIssueFilter issueFilter, UserSession userSession) {
-    IssueFilterDto issueFilterDtoToCopy = findIssueFilterDto(issueFilterIdToCopy, userSession);
-    issueFilter.setUser(userSession.login());
+    String user = getNotNullLogin(userSession);
+    IssueFilterDto issueFilterDtoToCopy = findIssueFilterDto(issueFilterIdToCopy, user);
+    issueFilter.setUser(user);
     issueFilter.setData(issueFilterDtoToCopy.getData());
-    validateFilter(issueFilter, userSession);
+    validateFilter(issueFilter, user);
 
-    return insertIssueFilter(issueFilter, userSession.login());
+    return insertIssueFilter(issueFilter, user);
   }
 
   public List<DefaultIssueFilter> findSharedFilters(UserSession userSession) {
-    if (userSession.isLoggedIn() && userSession.login() != null) {
-      return toIssueFilters(issueFilterDao.selectSharedWithoutUserFilters(userSession.login()));
-    }
-    return Collections.emptyList();
+    return toIssueFilters(issueFilterDao.selectSharedWithoutUserFilters(getNotNullLogin(userSession)));
   }
 
   public List<DefaultIssueFilter> findFavoriteFilters(UserSession userSession) {
-    if (userSession.isLoggedIn() && userSession.login() != null) {
-      return toIssueFilters(issueFilterDao.selectByUserWithOnlyFavoriteFilters(userSession.login()));
-    }
-    return Collections.emptyList();
+    return toIssueFilters(issueFilterDao.selectByUserWithOnlyFavoriteFilters(getNotNullLogin(userSession)));
   }
 
   public void toggleFavouriteIssueFilter(Long issueFilterId, UserSession userSession) {
-    findIssueFilterDto(issueFilterId, userSession);
-    IssueFilterFavouriteDto issueFilterFavouriteDto = findFavouriteIssueFilter(userSession.login(), issueFilterId);
+    String user = getNotNullLogin(userSession);
+    findIssueFilterDto(issueFilterId, user);
+    IssueFilterFavouriteDto issueFilterFavouriteDto = findFavouriteIssueFilter(user, issueFilterId);
     if (issueFilterFavouriteDto == null) {
-      addFavouriteIssueFilter(issueFilterId, userSession.login());
+      addFavouriteIssueFilter(issueFilterId, user);
     } else {
       deleteFavouriteIssueFilter(issueFilterFavouriteDto);
     }
@@ -163,42 +158,43 @@ public class IssueFilterService implements ServerComponent {
     return issueFilterSerializer.deserialize(issueFilter.data());
   }
 
-  private IssueFilterDto findIssueFilterDto(Long id, UserSession userSession) {
-    verifyLoggedIn(userSession);
+  private IssueFilterDto findIssueFilterDto(Long id, String user) {
     IssueFilterDto issueFilterDto = issueFilterDao.selectById(id);
     if (issueFilterDto == null) {
       // TODO throw 404
       throw new IllegalArgumentException("Filter not found: " + id);
     }
-    verifyCurrentUserCanReadFilter(issueFilterDto.toIssueFilter(), userSession);
+    verifyCurrentUserCanReadFilter(issueFilterDto.toIssueFilter(), user);
     return issueFilterDto;
   }
 
-  void verifyLoggedIn(UserSession userSession) {
-    if (!userSession.isLoggedIn() && userSession.login() != null) {
+  String getNotNullLogin(UserSession userSession) {
+    String user = userSession.login();
+    if (!userSession.isLoggedIn() && user != null) {
       throw new IllegalStateException("User is not logged in");
     }
+    return user;
   }
 
-  void verifyCurrentUserCanReadFilter(DefaultIssueFilter issueFilter, UserSession userSession) {
-    if (!issueFilter.user().equals(userSession.login()) && !issueFilter.shared()) {
+  void verifyCurrentUserCanReadFilter(DefaultIssueFilter issueFilter, String user) {
+    if (!issueFilter.user().equals(user) && !issueFilter.shared()) {
       // TODO throw unauthorized
       throw new IllegalStateException("User is not authorized to read this filter");
     }
   }
 
-  private void verifyCurrentUserCanModifyFilter(DefaultIssueFilter issueFilter, UserSession userSession) {
-    if (!issueFilter.user().equals(userSession.login()) && !isAdmin(userSession.login())) {
+  private void verifyCurrentUserCanModifyFilter(DefaultIssueFilter issueFilter, String user) {
+    if (!issueFilter.user().equals(user) && !isAdmin(user)) {
       // TODO throw unauthorized
       throw new IllegalStateException("User is not authorized to modify this filter");
     }
   }
 
-  private void validateFilter(DefaultIssueFilter issueFilter, UserSession userSession) {
-    if (issueFilterDao.selectByNameAndUser(issueFilter.name(), userSession.login(), issueFilter.id()) != null) {
+  private void validateFilter(DefaultIssueFilter issueFilter, String user) {
+    if (issueFilterDao.selectByNameAndUser(issueFilter.name(), user, issueFilter.id()) != null) {
       throw new IllegalArgumentException("Name already exists");
     }
-    if (issueFilter.shared() && issueFilterDao.selectSharedWithoutUserFiltersByName(issueFilter.name(), userSession.login(), issueFilter.id()) != null) {
+    if (issueFilter.shared() && issueFilterDao.selectSharedWithoutUserFiltersByName(issueFilter.name(), user, issueFilter.id()) != null) {
       throw new IllegalArgumentException("Other users already share filters with the same name");
     }
   }

diff --git a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java
index 47c70fb907684b57dcf0b0e730c33a90180bc256..2363f0c016234bf8bbbf1d996b585caf17b6707c 100644 (file)
--- a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java
@@ -38,6 +38,7 @@ import org.sonar.server.user.UserSession;
 import java.util.Collections;
 import java.util.Map;
 
+import static com.google.common.collect.Lists.newArrayList;
 import static com.google.common.collect.Maps.newHashMap;
 import static org.fest.assertions.Assertions.assertThat;
 import static org.mockito.Matchers.eq;
@@ -528,13 +529,16 @@ public class InternalRubyIssueServiceTest {
   public void should_check_is_user_is_authorized_to_see_issue_filter() {
     DefaultIssueFilter issueFilter = new DefaultIssueFilter();
     service.isUserAuthorized(issueFilter);
-    verify(issueFilterService).verifyLoggedIn(any(UserSession.class));
-    verify(issueFilterService).verifyCurrentUserCanReadFilter(eq(issueFilter), any(UserSession.class));
+    verify(issueFilterService).getNotNullLogin(any(UserSession.class));
+    verify(issueFilterService).verifyCurrentUserCanReadFilter(eq(issueFilter), anyString());
   }
 
   @Test
   public void should_execute_bulk_change() {
-    service.executeBulkChange(Maps.<String, Object>newHashMap());
+    Map<String, Object> params = newHashMap();
+    params.put("issues", newArrayList("ABCD", "EFGH"));
+    params.put("assignee", "arthur");
+    service.executeBulkChange(params);
     verify(issueBulkChangeService).execute(any(IssueBulkChangeQuery.class), any(UserSession.class));
   }
 
@@ -542,7 +546,10 @@ public class InternalRubyIssueServiceTest {
   public void should_no_execute_bulk_change_if_unexpected_error() {
     doThrow(new RuntimeException("Error")).when(issueBulkChangeService).execute(any(IssueBulkChangeQuery.class), any(UserSession.class));
 
-    Result result = service.executeBulkChange(Maps.<String, Object>newHashMap());
+    Map<String, Object> params = newHashMap();
+    params.put("issues", newArrayList("ABCD", "EFGH"));
+    params.put("assignee", "arthur");
+    Result result = service.executeBulkChange(params);
     assertThat(result.ok()).isFalse();
     assertThat(((Result.Message) result.errors().get(0)).text()).contains("Error");
   }

diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeQueryTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeQueryTest.java
new file mode 100644 (file)
index 0000000..55de437
--- /dev/null
+++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueBulkChangeQueryTest.java
@@ -0,0 +1,54 @@
+/*
+ * SonarQube, open source software quality management tool.
+ * Copyright (C) 2008-2013 SonarSource
+ * mailto:contact AT sonarsource DOT com
+ *
+ * SonarQube is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 3 of the License, or (at your option) any later version.
+ *
+ * SonarQube is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+package org.sonar.server.issue;
+
+import org.junit.Test;
+
+import java.util.Collections;
+
+import static com.google.common.collect.Lists.newArrayList;
+import static org.fest.assertions.Assertions.assertThat;
+
+public class IssueBulkChangeQueryTest {
+
+  @Test
+  public void test_build(){
+    IssueBulkChangeQuery issueBulkChangeQuery = IssueBulkChangeQuery.builder().issueKeys(newArrayList("ABCD", "EFGH")).assignee("perceval").build();
+    assertThat(issueBulkChangeQuery.issueKeys()).isNotNull();
+    assertThat(issueBulkChangeQuery.assignee()).isNotNull();
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void fail_to_build_if_no_issue(){
+    IssueBulkChangeQuery.builder().assignee("perceval").build();
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void fail_to_build_if_issues_are_empty(){
+    IssueBulkChangeQuery.builder().issueKeys(Collections.<String>emptyList()).assignee("perceval").build();
+  }
+
+  @Test(expected = IllegalArgumentException.class)
+  public void fail_to_build_if_no_action(){
+    IssueBulkChangeQuery.builder().issueKeys(newArrayList("ABCD", "EFGH")).build();
+  }
+
+}

diff --git a/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java b/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java
index 0dd94324cbbd56a155a8cb208befaf75b848cddb..cd6c87a504c364a5354d6d8239b4222a19cb1e58 100644 (file)
--- a/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java
@@ -149,10 +149,14 @@ public class IssueFilterServiceTest {
   }
 
   @Test
-  public void should_find_by_user_return_empty_result_for_not_logged_user() {
+  public void should_not_find_by_user_if_not_logged() {
     when(userSession.isLoggedIn()).thenReturn(false);
-
-    assertThat(service.findByUser(userSession)).isEmpty();
+    try {
+      service.findByUser(userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+    }
   }
 
   @Test
@@ -426,8 +430,12 @@ public class IssueFilterServiceTest {
   public void should_not_find_favourite_issue_filter_if_not_logged() {
     when(userSession.isLoggedIn()).thenReturn(false);
 
-    List<DefaultIssueFilter> results = service.findFavoriteFilters(userSession);
-    assertThat(results).isEmpty();
+    try {
+      service.findFavoriteFilters(userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+    }
   }
 
   @Test