From ec362118f5482b294fc657c00be6288142106428 Mon Sep 17 00:00:00 2001
From: Julien Lancelot <julien.lancelot@gmail.com>
Date: Mon, 17 Jun 2013 18:36:38 +0200
Subject: [PATCH] SONAR-4392 List and edit existing issues filter

---
 .../resources/org/sonar/l10n/core.properties  |   6 +
 .../sonar/core/issue/DefaultIssueFilter.java  |  10 +-
 .../sonar/core/issue/db/IssueFilterDao.java   |  12 +
 .../core/issue/db/IssueFilterMapper.java      |   6 +
 .../sonar/core/issue/db/IssueFilterMapper.xml |  13 +-
 .../core/issue/db/IssueFilterDaoTest.java     |  12 +-
 .../should_update-result.xml                  |   2 +-
 .../issue/InternalRubyIssueService.java       |  93 ++++++--
 .../server/issue/IssueFilterService.java      |  93 +++++---
 .../app/controllers/issues_controller.rb      |  82 ++++---
 .../app/views/issues/_display_errors.html.erb |   7 +
 .../app/views/issues/_edit_form.html.erb      |  33 +++
 .../app/views/issues/_favourites.html.erb     |   7 +
 .../app/views/issues/_save_as_form.html.erb   |  11 +-
 .../app/views/issues/_sidebar.html.erb        |   4 +-
 .../WEB-INF/app/views/issues/manage.html.erb  |  53 +++++
 .../WEB-INF/app/views/issues/search.html.erb  |   2 +
 .../issue/InternalRubyIssueServiceTest.java   | 100 +++++++-
 .../server/issue/IssueFilterServiceTest.java  | 225 +++++++++++++++++-
 19 files changed, 654 insertions(+), 117 deletions(-)
 create mode 100644 sonar-server/src/main/webapp/WEB-INF/app/views/issues/_display_errors.html.erb
 create mode 100644 sonar-server/src/main/webapp/WEB-INF/app/views/issues/_edit_form.html.erb
 create mode 100644 sonar-server/src/main/webapp/WEB-INF/app/views/issues/_favourites.html.erb
 create mode 100644 sonar-server/src/main/webapp/WEB-INF/app/views/issues/manage.html.erb

diff --git a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties
index 6caab7f672a..4c36dc7c676 100644
--- a/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties
+++ b/plugins/sonar-core-plugin/src/main/resources/org/sonar/l10n/core.properties
@@ -531,9 +531,15 @@ issue_filter.criteria.status=Status
 issue_filter.max_results_reached=Only the first {0} issues matching the search criteria have been retrieved. Add some additional criteria to get fewer results to be able to sort this list.
 issue_filter.no_result=No matching issues found.
 issue_filter.save_filter=Save Filter
+issue_filter.edit_filter=Edit Filter
 issue_filter.form.name=Name
 issue_filter.form.description=Description
 issue_filter.form.share=Shared with all users
+issue_filter.favourite_filters=Favourite Filters
+issue_filter.manage.my_filters=My Filters
+issue_filter.no_filters=No filters
+issue_filter.delete_confirm_title=Delete Filter
+issue_filter.are_you_sure_want_delete_filter_x=Are you sure that you want to delete the filter "{0}"?
 
 
 #------------------------------------------------------------------------------
diff --git a/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueFilter.java b/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueFilter.java
index 03ec5dac42b..db2b2b9fbf7 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueFilter.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/DefaultIssueFilter.java
@@ -25,8 +25,6 @@ import com.google.common.base.Splitter;
 import com.google.common.collect.Lists;
 import org.apache.commons.lang.StringUtils;
 
-import javax.annotation.CheckForNull;
-
 import java.util.Date;
 import java.util.Iterator;
 import java.util.List;
@@ -63,7 +61,7 @@ public class DefaultIssueFilter {
   }
 
   public DefaultIssueFilter(Map<String, Object> mapData) {
-    this.data = mapAsdata(mapData);
+    setData(mapData);
   }
 
   public Long id() {
@@ -84,7 +82,6 @@ public class DefaultIssueFilter {
     return this;
   }
 
-  @CheckForNull
   public String user() {
     return user;
   }
@@ -139,6 +136,11 @@ public class DefaultIssueFilter {
     return this;
   }
 
+  public final DefaultIssueFilter setData(Map<String, Object> mapData) {
+    this.data = mapAsdata(mapData);
+    return this;
+  }
+
   /**
    * Used by ui
    */
diff --git a/sonar-core/src/main/java/org/sonar/core/issue/db/IssueFilterDao.java b/sonar-core/src/main/java/org/sonar/core/issue/db/IssueFilterDao.java
index 2ac8a80265c..679b02e7915 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/db/IssueFilterDao.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/db/IssueFilterDao.java
@@ -26,6 +26,7 @@ import org.sonar.api.ServerComponent;
 import org.sonar.core.persistence.MyBatis;
 
 import javax.annotation.CheckForNull;
+import javax.annotation.Nullable;
 
 import java.util.List;
 
@@ -51,6 +52,17 @@ public class IssueFilterDao implements BatchComponent, ServerComponent {
     }
   }
 
+  @CheckForNull
+  public IssueFilterDto selectByNameAndUser(String name, String user, @Nullable Long existingId) {
+    SqlSession session = mybatis.openSession();
+    try {
+      session.getMapper(IssueFilterMapper.class);
+      return getMapper(session).selectByNameAndUser(name, user, existingId);
+    } finally {
+      MyBatis.closeQuietly(session);
+    }
+  }
+
   public List<IssueFilterDto> selectByUser(String user) {
     SqlSession session = mybatis.openSession();
     try {
diff --git a/sonar-core/src/main/java/org/sonar/core/issue/db/IssueFilterMapper.java b/sonar-core/src/main/java/org/sonar/core/issue/db/IssueFilterMapper.java
index c4e8520febb..86f10e01c7b 100644
--- a/sonar-core/src/main/java/org/sonar/core/issue/db/IssueFilterMapper.java
+++ b/sonar-core/src/main/java/org/sonar/core/issue/db/IssueFilterMapper.java
@@ -19,7 +19,10 @@
  */
 package org.sonar.core.issue.db;
 
+import org.apache.ibatis.annotations.Param;
+
 import javax.annotation.CheckForNull;
+import javax.annotation.Nullable;
 
 import java.util.List;
 
@@ -31,6 +34,9 @@ public interface IssueFilterMapper {
   @CheckForNull
   IssueFilterDto selectById(Long id);
 
+  @CheckForNull
+  IssueFilterDto selectByNameAndUser(@Param("name") String name, @Param("userLogin") String userLogin, @Nullable @Param("existingId") Long existingId);
+
   List<IssueFilterDto> selectByUser(String user);
 
   void insert(IssueFilterDto filter);
diff --git a/sonar-core/src/main/resources/org/sonar/core/issue/db/IssueFilterMapper.xml b/sonar-core/src/main/resources/org/sonar/core/issue/db/IssueFilterMapper.xml
index 8a5ed2fc971..291f90db47f 100644
--- a/sonar-core/src/main/resources/org/sonar/core/issue/db/IssueFilterMapper.xml
+++ b/sonar-core/src/main/resources/org/sonar/core/issue/db/IssueFilterMapper.xml
@@ -23,6 +23,18 @@
     </where>
   </select>
 
+  <select id="selectByNameAndUser" parameterType="map" resultType="IssueFilter">
+    select <include refid="issueFilterColumns"/>
+    from issue_filters filters
+    <where>
+      and filters.name=#{name}
+      and filters.user_login=#{userLogin}
+      <if test="existingId != null">
+        and filters.id&lt;&gt;#{existingId}
+      </if>
+    </where>
+  </select>
+
   <select id="selectByUser" parameterType="String" resultType="IssueFilter">
     select <include refid="issueFilterColumns"/>
     from issue_filters filters
@@ -48,7 +60,6 @@
   <update id="update" parameterType="IssueFilter">
     update issue_filters set
     name=#{name},
-    user_login=#{userLogin},
     shared=#{shared},
     description=#{description},
     data=#{data},
diff --git a/sonar-core/src/test/java/org/sonar/core/issue/db/IssueFilterDaoTest.java b/sonar-core/src/test/java/org/sonar/core/issue/db/IssueFilterDaoTest.java
index de841318b98..4f6b9001a0d 100644
--- a/sonar-core/src/test/java/org/sonar/core/issue/db/IssueFilterDaoTest.java
+++ b/sonar-core/src/test/java/org/sonar/core/issue/db/IssueFilterDaoTest.java
@@ -50,6 +50,17 @@ public class IssueFilterDaoTest extends AbstractDaoTestCase {
     assertThat(dao.selectById(123L)).isNull();
   }
 
+  @Test
+  public void should_select_by_name_and_user() {
+    setupData("shared");
+
+    IssueFilterDto filter = dao.selectByNameAndUser("Sonar Issues", "stephane", null);
+    assertThat(filter.getId()).isEqualTo(1L);
+
+    filter = dao.selectByNameAndUser("Sonar Issues", "stephane", 1L);
+    assertThat(filter).isNull();
+  }
+
   @Test
   public void should_select_by_user() {
     setupData("should_select_by_user");
@@ -84,7 +95,6 @@ public class IssueFilterDaoTest extends AbstractDaoTestCase {
     IssueFilterDto filterDto = new IssueFilterDto();
     filterDto.setId(2L);
     filterDto.setName("Closed issues");
-    filterDto.setUserLogin("henry");
     filterDto.setShared(false);
     filterDto.setDescription("All closed issues");
     filterDto.setData("statuses=CLOSED");
diff --git a/sonar-core/src/test/resources/org/sonar/core/issue/db/IssueFilterDaoTest/should_update-result.xml b/sonar-core/src/test/resources/org/sonar/core/issue/db/IssueFilterDaoTest/should_update-result.xml
index a3e7af9bda8..daf43474984 100644
--- a/sonar-core/src/test/resources/org/sonar/core/issue/db/IssueFilterDaoTest/should_update-result.xml
+++ b/sonar-core/src/test/resources/org/sonar/core/issue/db/IssueFilterDaoTest/should_update-result.xml
@@ -13,7 +13,7 @@
   <issue_filters
       id="2"
       name="Closed issues"
-      user_login="henry"
+      user_login="michael"
       shared="[false]"
       description="All closed issues"
       data="statuses=CLOSED"
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 c8c2967d2f5..533566f8ade 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/InternalRubyIssueService.java
@@ -278,7 +278,6 @@ public class InternalRubyIssueService implements ServerComponent {
     String description = parameters.get("description");
     String deadLineParam = parameters.get("deadLine");
     String projectParam = parameters.get("project");
-    Date deadLine = null;
 
     checkMandatorySizeParameter(name, "name", 200, result);
     checkOptionnalSizeParameter(description, "description",  1000, result);
@@ -287,7 +286,7 @@ public class InternalRubyIssueService implements ServerComponent {
     if (existingActionPlan == null) {
       checkProject(projectParam, result);
     }
-    deadLine = checkAndReturnDeadline(deadLineParam, result);
+    Date deadLine = checkAndReturnDeadline(deadLineParam, result);
 
     if (!Strings.isNullOrEmpty(projectParam) && !Strings.isNullOrEmpty(name) && (existingActionPlan == null || !name.equals(existingActionPlan.name()))
       && actionPlanService.isNameAlreadyUsedForProject(name, projectParam)) {
@@ -373,38 +372,72 @@ public class InternalRubyIssueService implements ServerComponent {
     return issueFilterService.findById(id, UserSession.get());
   }
 
+  public List<DefaultIssueFilter> findUserIssueFilters() {
+    return issueFilterService.findByUser(UserSession.get());
+  }
+
   public DefaultIssueFilter createFilterFromMap(Map<String, Object> mapData) {
-    return issueFilterService.createEmptyFilter(mapData);
+    return new DefaultIssueFilter(mapData);
   }
 
   /**
-   * Execute issue filter
+   * Execute issue filter from issue query
    */
   public IssueQueryResult execute(IssueQuery issueQuery) {
     return issueFilterService.execute(issueQuery);
   }
 
+  /**
+   * Execute issue filter from existing filter
+   */
+  public IssueQueryResult execute(Long issueFilterId) {
+    return issueFilterService.execute(issueFilterId, UserSession.get());
+  }
+
+  /**
+   * List user issue filter
+   */
+  public List<DefaultIssueFilter> findIssueFiltersForUser() {
+    return issueFilterService.findByUser(UserSession.get());
+  }
+
   /**
    * Create issue filter
    */
   public Result<DefaultIssueFilter> createIssueFilter(Map<String, String> params) {
-    Result<DefaultIssueFilter> result = Result.of();
-    try {
-      // mandatory parameters
-      String name = params.get("name");
-      String description = params.get("description");
-      String data = params.get("data");
-      Boolean shared = RubyUtils.toBoolean(params.get("shared"));
+    Result<DefaultIssueFilter> result = createIssueFilterResult(params, false);
+    if (result.ok()) {
+      try {
+        result.set(issueFilterService.save(result.get(), UserSession.get()));
+      } catch (Exception e) {
+        result.addError(e.getMessage());
+      }
+    }
+    return result;
+  }
 
-      if (result.ok()) {
-        DefaultIssueFilter defaultIssueFilter = DefaultIssueFilter.create(name)
-          .setDescription(description)
-          .setShared(shared)
-          .setData(data);
-        defaultIssueFilter = issueFilterService.save(defaultIssueFilter, UserSession.get());
-        result.set(defaultIssueFilter);
+  /**
+   * Update issue filter
+   */
+  public Result<DefaultIssueFilter> updateIssueFilter(Map<String, String> parameters) {
+    Result<DefaultIssueFilter> result = createIssueFilterResult(parameters, true);
+    if (result.ok()) {
+      try {
+        result.set(issueFilterService.update(result.get(), UserSession.get()));
+      } catch (Exception e) {
+        result.addError(e.getMessage());
       }
+    }
+    return result;
+  }
 
+  /**
+   * Update issue filter data
+   */
+  public Result<DefaultIssueFilter> updateIssueFilterData(Long issueFilterId, Map<String, Object> data) {
+    Result<DefaultIssueFilter> result = Result.of();
+    try {
+      result.set(issueFilterService.updateData(issueFilterId, data, UserSession.get()));
     } catch (Exception e) {
       result.addError(e.getMessage());
     }
@@ -412,37 +445,45 @@ public class InternalRubyIssueService implements ServerComponent {
   }
 
   @VisibleForTesting
-  Result<DefaultIssueFilter> createIssueFilterResult(Map<String, String> params) {
+  Result<DefaultIssueFilter> createIssueFilterResult(Map<String, String> params, boolean isUpdate) {
     Result<DefaultIssueFilter> result = Result.of();
 
-    // mandatory parameters
+    String id = params.get("id");
     String name = params.get("name");
     String description = params.get("description");
     String data = params.get("data");
     Boolean shared = RubyUtils.toBoolean(params.get("shared"));
 
+    if (isUpdate) {
+      checkMandatoryParameter(id, "id", result);
+    }
     checkMandatorySizeParameter(name, "name",  100, result);
     checkOptionnalSizeParameter(description, "description",  4000, result);
 
-     // TODO check name uniquness
-
     if (result.ok()) {
       DefaultIssueFilter defaultIssueFilter = DefaultIssueFilter.create(name)
         .setDescription(description)
         .setShared(shared)
         .setData(data);
+      if (isUpdate) {
+        defaultIssueFilter.setId(Long.valueOf(id));
+      }
+
       result.set(defaultIssueFilter);
     }
     return result;
   }
 
-  private void checkMandatorySizeParameter(String value, String paramName, Integer size, Result result){
+  private void checkMandatoryParameter(String value, String paramName, Result result){
     if (Strings.isNullOrEmpty(value)) {
       result.addError(Result.Message.ofL10n("errors.cant_be_empty", paramName));
-    } else {
-      if (value.length() > size) {
+    }
+  }
+
+  private void checkMandatorySizeParameter(String value, String paramName, Integer size, Result result){
+    checkMandatoryParameter(value, paramName, result);
+    if (!Strings.isNullOrEmpty(value) && value.length() > size) {
         result.addError(Result.Message.ofL10n("errors.is_too_long", paramName, size));
-      }
     }
   }
 
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 7b0abcffa85..772c0db0c8e 100644
--- a/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java
+++ b/sonar-server/src/main/java/org/sonar/server/issue/IssueFilterService.java
@@ -20,6 +20,8 @@
 
 package org.sonar.server.issue;
 
+import com.google.common.base.Function;
+import com.google.common.collect.Iterables;
 import org.sonar.api.ServerComponent;
 import org.sonar.api.issue.IssueFinder;
 import org.sonar.api.issue.IssueQuery;
@@ -31,8 +33,12 @@ import org.sonar.server.user.UserSession;
 
 import javax.annotation.CheckForNull;
 
+import java.util.Collections;
+import java.util.List;
 import java.util.Map;
 
+import static com.google.common.collect.Lists.newArrayList;
+
 public class IssueFilterService implements ServerComponent {
 
   private IssueFilterDao issueFilterDao;
@@ -43,47 +49,61 @@ public class IssueFilterService implements ServerComponent {
     this.issueFinder = issueFinder;
   }
 
-  @CheckForNull
-  public DefaultIssueFilter createEmptyFilter(Map<String, Object> mapData) {
-    return new DefaultIssueFilter(mapData);
-  }
-
   @CheckForNull
   public DefaultIssueFilter findById(Long id, UserSession userSession) {
-    // TODO
-//    checkAuthorization(userSession, project, UserRole.ADMIN);
     verifyLoggedIn(userSession);
-//    access_denied unless filter.shared || filter.owner?(current_user)
+    IssueFilterDto issueFilterDto = findIssueFilter(id);
+    verifyCurrentUserIsOwnerOfFilter(issueFilterDto, userSession);
+    return issueFilterDto.toIssueFilter();
+  }
 
-    IssueFilterDto issueFilterDto = issueFilterDao.selectById(id);
-    if (issueFilterDto == null) {
-      return null;
+  public List<DefaultIssueFilter> findByUser(UserSession userSession) {
+    if (userSession.isLoggedIn()) {
+      List<IssueFilterDto> issueFilterDtoList = issueFilterDao.selectByUser(userSession.login());
+      return newArrayList(Iterables.transform(issueFilterDtoList, new Function<IssueFilterDto, DefaultIssueFilter>() {
+        @Override
+        public DefaultIssueFilter apply(IssueFilterDto issueFilterDto) {
+          return issueFilterDto.toIssueFilter();
+        }
+      }));
     }
-    return issueFilterDto.toIssueFilter();
+    return Collections.emptyList();
   }
 
   public DefaultIssueFilter save(DefaultIssueFilter issueFilter, UserSession userSession) {
-    // TODO
-//    checkAuthorization(userSession, project, UserRole.ADMIN);
     verifyLoggedIn(userSession);
     issueFilter.setUser(userSession.login());
+    VerifyNameIsNotAlreadyUsed(issueFilter, userSession);
+
     IssueFilterDto issueFilterDto = IssueFilterDto.toIssueFilter(issueFilter);
     issueFilterDao.insert(issueFilterDto);
     return issueFilterDto.toIssueFilter();
   }
 
   public DefaultIssueFilter update(DefaultIssueFilter issueFilter, UserSession userSession) {
-    // TODO
-//    checkAuthorization(userSession, project, UserRole.ADMIN);
     verifyLoggedIn(userSession);
+    IssueFilterDto issueFilterDto = findIssueFilter(issueFilter.id());
+    verifyCurrentUserIsOwnerOfFilter(issueFilterDto, userSession);
+    VerifyNameIsNotAlreadyUsed(issueFilter, userSession);
+
+    issueFilterDao.update(IssueFilterDto.toIssueFilter(issueFilter));
+    return issueFilter;
+  }
+
+  public DefaultIssueFilter updateData(Long issueFilterId, Map<String, Object> mapData, UserSession userSession) {
+    verifyLoggedIn(userSession);
+    IssueFilterDto issueFilterDto = findIssueFilter(issueFilterId);
+    verifyCurrentUserIsOwnerOfFilter(issueFilterDto, userSession);
+    DefaultIssueFilter issueFilter = issueFilterDto.toIssueFilter();
+    issueFilter.setData(mapData);
     issueFilterDao.update(IssueFilterDto.toIssueFilter(issueFilter));
     return issueFilter;
   }
 
   public void delete(Long issueFilterId, UserSession userSession) {
-    // TODO
-    //checkAuthorization(userSession, findActionPlanDto(actionPlanKey).getProjectKey(), UserRole.ADMIN);
     verifyLoggedIn(userSession);
+    IssueFilterDto issueFilterDto = findIssueFilter(issueFilterId);
+    verifyCurrentUserIsOwnerOfFilter(issueFilterDto, userSession);
     issueFilterDao.delete(issueFilterId);
   }
 
@@ -91,26 +111,41 @@ public class IssueFilterService implements ServerComponent {
     return issueFinder.find(issueQuery);
   }
 
-  public IssueQueryResult execute(Long issueFilterId) {
-    IssueFilterDto issueFilterDto = issueFilterDao.selectById(issueFilterId);
-    if (issueFilterDto == null) {
-      // TODO throw 404
-      throw new IllegalArgumentException("Issue filter " + issueFilterId + " has not been found.");
-    }
+  public IssueQueryResult execute(Long issueFilterId, UserSession userSession) {
+    IssueFilterDto issueFilterDto = findIssueFilter(issueFilterId);
+    verifyCurrentUserIsOwnerOfFilter(issueFilterDto, userSession);
 
     DefaultIssueFilter issueFilter = issueFilterDto.toIssueFilter();
-    // convert data to issue query
-    issueFilter.data();
+    IssueQuery issueQuery = PublicRubyIssueService.toQuery(issueFilter.dataAsMap());
+    return issueFinder.find(issueQuery);
+  }
 
-//    return issueFinder.find(issueQuery);
-    return null;
+  public IssueFilterDto findIssueFilter(Long id){
+    IssueFilterDto issueFilterDto = issueFilterDao.selectById(id);
+    if (issueFilterDto == null) {
+      // TODO throw 404
+      throw new IllegalArgumentException("Filter not found: " + id);
+    }
+    return issueFilterDto;
   }
 
   private void verifyLoggedIn(UserSession userSession) {
     if (!userSession.isLoggedIn()) {
-      // must be logged
       throw new IllegalStateException("User is not logged in");
     }
   }
 
+  private void verifyCurrentUserIsOwnerOfFilter(IssueFilterDto issueFilterDto, UserSession userSession){
+    if (!issueFilterDto.getUserLogin().equals(userSession.login())) {
+      // TODO throw unauthorized
+      throw new IllegalStateException("User is not authorized to get this filter");
+    }
+  }
+
+  private void VerifyNameIsNotAlreadyUsed(DefaultIssueFilter issueFilter, UserSession userSession){
+    if (issueFilterDao.selectByNameAndUser(issueFilter.name(), userSession.login(), issueFilter.id()) != null) {
+      throw new IllegalArgumentException("Name already exists");
+    }
+  }
+
 }
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/issues_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/issues_controller.rb
index cafdd7f75a7..eef280470d1 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/issues_controller.rb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/issues_controller.rb
@@ -21,6 +21,7 @@
 class IssuesController < ApplicationController
 
   before_filter :init_options
+  before_filter :load_user_filters, :only => [:index, :search, :filter, :manage]
 
   # GET /issues/index
   def index
@@ -35,7 +36,7 @@ class IssuesController < ApplicationController
       @filter = Internal.issues.createFilterFromMap(criteria_params)
     end
 
-    @issue_query = Internal.issues.toQuery(@filter.dataAsMap)
+    @issue_query = Internal.issues.toQuery(criteria_params)
     @issues_result = Internal.issues.execute(@issue_query)
   end
 
@@ -44,18 +45,23 @@ class IssuesController < ApplicationController
   def filter
     require_parameters :id
 
-    @filter = find_filter(params[:id].to_i)
-    @issue_query = Internal.issues.toQuery(@filter.dataAsMap)
-
     # criteria can be overridden
     # TODO ?
     #@filter.override_criteria(criteria_params)
 
+    @filter = find_filter(params[:id].to_i)
+    @issue_query = Internal.issues.toQuery(@filter.dataAsMap)
     @issues_result = Internal.issues.execute(@issue_query)
     @unchanged = true
+
     render :action => 'search'
   end
 
+  # GET /issues/manage
+  def manage
+    @issue_query = Internal.issues.toQuery({})
+  end
+
   # GET /issues/save_as_form?[id=<id>][&criteria]
   def save_as_form
     if params[:id].present?
@@ -69,22 +75,15 @@ class IssuesController < ApplicationController
   # POST /issues/save_as?[id=<id>]&name=<name>[&parameters]
   def save_as
     verify_post_request
-    access_denied unless logged_in?
-
-    options = {'name' => params[:name], 'description' => params[:description], 'data' => URI.unescape(params[:data]), 'shared' => params[:shared]=='true' }
-    add_to_favourites=false
+    options = {'id' => params[:id], 'name' => params[:name], 'description' => params[:description], 'data' => URI.unescape(params[:data]), 'shared' => params[:shared]=='true' }
     if params[:id].present?
-      # TODO
-      #@filter = Internal.issues.updateIssueFilter(params[:id], options)
+      @filter = Internal.issues.updateIssueFilter(options)
     else
       filter_result = Internal.issues.createIssueFilter(options)
-      add_to_favourites=true
     end
 
     if filter_result.ok
       @filter = filter_result.get()
-      puts "#### "+ @filter.id.to_s
-      #current_user.favourited_measure_filters<<@filter if add_to_favourites
       render :text => @filter.id.to_s, :status => 200
     else
       @errors = filter_result.errors
@@ -93,24 +92,51 @@ class IssuesController < ApplicationController
   end
 
   # POST /issues/save?id=<id>&[criteria]
-  # TODO
   def save
     verify_post_request
     require_parameters :id
-    access_denied unless logged_in?
 
-    @filter = find_filter(params[:id])
+    filter_result = Internal.issues.updateIssueFilterData(params[:id].to_i, criteria_params)
+    if filter_result.ok
+      @filter = filter_result.get()
+      redirect_to :action => 'filter', :id => @filter.id.to_s
+    else
+      @errors = filter_result.errors
+
+      @filter = find_filter(params[:id].to_i)
+      @issue_query = Internal.issues.toQuery(@filter.dataAsMap)
+      @issues_result = Internal.issues.execute(@issue_query)
+      @unchanged = true
+
+      render :action => 'search'
+    end
+  end
 
-    #@filter = Internal.issues.updateIssueFilter(params[:id], options)
+  # GET /issues/edit_form/<filter id>
+  def edit_form
+    require_parameters :id
+    @filter = find_filter(params[:id].to_i)
+    render :partial => 'issues/edit_form'
+  end
 
-    #@filter.criteria=criteria_params_without_page_id
-    #@filter.convert_criteria_to_data
-    #unless @filter.save
-    #  flash[:error]='Error'
-    #end
-    redirect_to :action => 'filter', :id => @filter.id
+  # POST /issues/edit/<filter id>?name=<name>&description=<description>&shared=<true|false>
+  def edit
+    verify_post_request
+
+    existing_filter = find_filter(params[:id].to_i)
+    options = {'id' => params[:id].to_s, 'name' => params[:name], 'description' => params[:description], 'data' => existing_filter.data, 'shared' => params[:shared]=='true' }
+    filter_result = Internal.issues.updateIssueFilter(options)
+    if filter_result.ok
+      @filter = filter_result.get()
+      render :text => @filter.id.to_s, :status => 200
+    else
+      @errors = filter_result.errors
+      @filter = find_filter(params[:id].to_i)
+      render :partial => 'issues/edit_form', :status => 400
+    end
   end
 
+
   private
 
   def init_options
@@ -118,12 +144,12 @@ class IssuesController < ApplicationController
     @options_for_resolutions = Internal.issues.listResolutions().map {|s| [message('issue.resolution.' + s), s]}
   end
 
-  # TODO
+  def load_user_filters
+    @my_filters = Internal.issues.findIssueFiltersForUser()
+  end
+
   def find_filter(id)
-    filter = Internal.issues.findIssueFilter(id)
-    # TODO
-    #access_denied unless filter.shared || filter.owner?(current_user)
-    filter
+    Internal.issues.findIssueFilter(id)
   end
 
   def criteria_params
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_display_errors.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_display_errors.html.erb
new file mode 100644
index 00000000000..90b29b1d880
--- /dev/null
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_display_errors.html.erb
@@ -0,0 +1,7 @@
+<%
+   if @errors
+     @errors.each do |msg| %>
+    <div class="error"><%= h (msg.text ? msg.text : Api::Utils.message(msg.l10nKey, :params => msg.l10nParams)) -%></div>
+  <% end
+   end
+%>
\ No newline at end of file
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_edit_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_edit_form.html.erb
new file mode 100644
index 00000000000..809dab66674
--- /dev/null
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_edit_form.html.erb
@@ -0,0 +1,33 @@
+<form id="edit-filter-form" method="post" action="<%= ApplicationController.root_context -%>/issues/edit">
+  <input type="hidden" name="id" value="<%= @filter.id -%>">
+  <fieldset>
+    <div class="modal-head">
+      <h2><%= message('issue_filter.edit_filter') -%></h2>
+    </div>
+    <div class="modal-body">
+      <%= render :partial => 'display_errors' %>
+      <div class="modal-field">
+        <label for="name"><%= h message('name') -%> <em class="mandatory">*</em></label>
+        <input id="name" name="name" type="text" size="50" maxlength="100" value="<%= h @filter.name -%>" autofocus="autofocus"/>
+      </div>
+      <div class="modal-field">
+        <label for="description"><%= h message('description') -%></label>
+        <input id="description" name="description" type="text" size="50" maxlength="4000" value="<%= h @filter.description -%>"/>
+      </div>
+      <!--TODO-->
+      <div class="modal-field hidden">
+        <label for="shared"><%= h message('issue_filter.form.share') -%></label>
+        <input id="shared" name="shared" type="checkbox" value="true" <%= 'checked' if @filter.shared -%>/>
+      </div>
+    </div>
+    <div class="modal-foot">
+      <input type="submit" value="<%= h message('save') -%>" id="save-submit"/>
+      <a href="#" onclick="return closeModalWindow()" id="save-cancel"><%= h message('cancel') -%></a>
+    </div>
+  </fieldset>
+</form>
+<script>
+  $j("#edit-filter-form").modalForm({success: function (data) {
+    window.location = baseUrl + '/issues/filter/' + data;
+  }});
+</script>
\ No newline at end of file
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_favourites.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_favourites.html.erb
new file mode 100644
index 00000000000..0a288c80ae2
--- /dev/null
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_favourites.html.erb
@@ -0,0 +1,7 @@
+<div id="sidebar-favourites">
+<% if logged_in? %>
+  <li class="sidebar-title"><%= message('issue_filter.favourite_filters') -%></li>
+  <li><a href="<%= ApplicationController.root_context -%>/issues/manage" class="link-action"><%= message('manage') %></a></li>
+  <li class="spacer"></li>
+<% end %>
+</div>
\ No newline at end of file
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_save_as_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_save_as_form.html.erb
index 10dcaf7a402..77797e3616a 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_save_as_form.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_save_as_form.html.erb
@@ -7,13 +7,7 @@
     </div>
 
     <div class="modal-body">
-      <%
-         if @errors
-         @errors.each do |msg| %>
-        <div class="error"><%= h (msg.text ? msg.text : Api::Utils.message(msg.l10nKey, :params => msg.l10nParams)) -%></div>
-      <% end
-        end
-      %>
+      <%= render :partial => 'display_errors' %>
       <div class="modal-field">
         <label for="name"><%= message('issue_filter.form.name') -%> <em class="mandatory">*</em></label>
         <input id="name" name="name" type="text" size="50" maxlength="100" value="<%= h @filter.name -%>" autofocus="autofocus"/>
@@ -22,7 +16,8 @@
         <label for="description"><%= message('issue_filter.form.description') -%></label>
         <input id="description" name="description" type="text" size="50" maxlength="4000" value="<%= h @filter.description -%>"/>
       </div>
-      <div class="modal-field">
+      <!--TODO-->
+      <div class="modal-field hidden">
         <label for="shared"><%= message('issue_filter.form.share') -%></label>
         <input id="shared" name="shared" type="checkbox" value="true" <%= 'checked' if @filter.shared -%>/>
       </div>
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_sidebar.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_sidebar.html.erb
index 01e6a783115..48126ba5df4 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_sidebar.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/_sidebar.html.erb
@@ -1,7 +1,9 @@
 <ul class="sidebar gray-sidebar">
+  <%= render :partial => 'issues/favourites' -%>
+
   <form method="GET" action="<%= ApplicationController.root_context -%>/issues/search" >
 
-    <% if @filter.id %>
+    <% if @filter && @filter.id %>
       <input type="hidden" name="id" value="<%= @filter.id.to_s -%>">
     <% end %>
     <input type="hidden" name="sort" value="<%= @issue_query.sort -%>"/>
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/manage.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/manage.html.erb
new file mode 100644
index 00000000000..0f95f2ae53a
--- /dev/null
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/manage.html.erb
@@ -0,0 +1,53 @@
+<div>
+  <div class="page-split-left">
+    <%= render :partial => 'issues/sidebar' -%>
+  </div>
+  <div class="page-split-right">
+    <div id="content">
+      <h1><%= message 'issue_filter.manage.my_filters' -%></h1>
+      <table class="data marginbottom10" id="my-issue-filters">
+        <thead>
+        <tr>
+          <th><%= message('name') -%></th>
+          <th class="right"><%= message('operations') -%></th>
+        </tr>
+        </thead>
+        <tbody>
+        <% if @my_filters.empty? %>
+          <tr class="even">
+            <td colspan="4"><%= message('issue_filter.no_filters') -%></td>
+          </tr>
+        <% else %>
+          <% @my_filters.each do |filter| %>
+            <tr id="my-issue-filter-<%= filter.name.parameterize -%>" class="<%= cycle('even', 'odd', :name => 'my-filters') -%>">
+              <td>
+                <%= link_to h(filter.name), :action => 'filter', :id => filter.id -%>
+                <% if filter.description %>
+                  <div class="note"><%= h filter.description -%></div>
+                <% end %>
+              </td>
+              <td class="thin nowrap right">
+                <!--TODO copy-->
+                <a class="hidden" id="copy-<%= filter.name.parameterize -%>" href="<%= ApplicationController.root_context -%>/issues/copy_form/<%= filter.id -%>"
+                   class="link-action open-modal"><%= message('copy') -%></a>
+                &nbsp;
+                <a id="edit_<%= filter.name.parameterize -%>" href="<%= ApplicationController.root_context -%>/issues/edit_form/<%= filter.id -%>"
+                   class="link-action open-modal"><%= message('edit') -%></a>
+                &nbsp;
+                <%= link_to_action message('delete'), "#{ApplicationController.root_context}/issues/delete/#{filter.id}",
+                                   :class => 'link-action link-red',
+                                   :id => "delete_#{filter.name.parameterize}",
+                                   :confirm_button => message('delete'),
+                                   :confirm_title => 'issue_filter.delete_confirm_title',
+                                   :confirm_msg => 'issue_filter.are_you_sure_want_delete_filter_x',
+                                   :confirm_msg_params => [filter.name] -%>
+              </td>
+            </tr>
+          <% end %>
+        <% end %>
+        </tbody>
+      </table>
+
+    </div>
+  </div>
+</div>
\ No newline at end of file
diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/search.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/search.html.erb
index 7ffbc9df5d3..48f0fbb4f42 100644
--- a/sonar-server/src/main/webapp/WEB-INF/app/views/issues/search.html.erb
+++ b/sonar-server/src/main/webapp/WEB-INF/app/views/issues/search.html.erb
@@ -5,6 +5,8 @@
 
   <div class="page-split-right">
     <div id="content">
+      <%= render :partial => 'display_errors' %>
+
       <div class="line-block marginbottom10">
         <%= render :partial => 'action_links' -%>
       </div>
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 eac0c788819..495bcf5ea4b 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/InternalRubyIssueServiceTest.java
@@ -24,9 +24,11 @@ import org.junit.Before;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
 import org.sonar.api.issue.ActionPlan;
+import org.sonar.api.issue.IssueQuery;
 import org.sonar.api.issue.internal.FieldDiffs;
 import org.sonar.api.user.User;
 import org.sonar.core.issue.DefaultActionPlan;
+import org.sonar.core.issue.DefaultIssueFilter;
 import org.sonar.core.resource.ResourceDao;
 import org.sonar.core.resource.ResourceDto;
 import org.sonar.core.resource.ResourceQuery;
@@ -67,10 +69,10 @@ public class InternalRubyIssueServiceTest {
     parameters.put("project", "org.sonar.Sample");
     parameters.put("deadLine", "2113-05-13");
 
-    ArgumentCaptor<ActionPlan> actionPlanCaptor = ArgumentCaptor.forClass(ActionPlan.class);
     Result result = service.createActionPlan(parameters);
     assertThat(result.ok()).isTrue();
 
+    ArgumentCaptor<ActionPlan> actionPlanCaptor = ArgumentCaptor.forClass(ActionPlan.class);
     verify(actionPlanService).create(actionPlanCaptor.capture(), any(UserSession.class));
     ActionPlan actionPlan = actionPlanCaptor.getValue();
 
@@ -91,10 +93,10 @@ public class InternalRubyIssueServiceTest {
     parameters.put("deadLine", "2113-05-13");
     parameters.put("project", "org.sonar.MultiSample");
 
-    ArgumentCaptor<ActionPlan> actionPlanCaptor = ArgumentCaptor.forClass(ActionPlan.class);
     Result result = service.updateActionPlan("ABCD", parameters);
     assertThat(result.ok()).isTrue();
 
+    ArgumentCaptor<ActionPlan> actionPlanCaptor = ArgumentCaptor.forClass(ActionPlan.class);
     verify(actionPlanService).update(actionPlanCaptor.capture(), any(UserSession.class));
     ActionPlan actionPlan = actionPlanCaptor.getValue();
 
@@ -276,14 +278,6 @@ public class InternalRubyIssueServiceTest {
     assertThat(result.errors()).contains(Result.Message.ofL10n("action_plans.errors.project_does_not_exist", "org.sonar.Sample"));
   }
 
-  public String createLongString(int size) {
-    String result = "";
-    for (int i = 0; i < size; i++) {
-      result += "c";
-    }
-    return result;
-  }
-
   @Test
   public void test_changelog() throws Exception {
     IssueChangelog changelog = new IssueChangelog(Collections.<FieldDiffs>emptyList(), Collections.<User>emptyList());
@@ -294,13 +288,47 @@ public class InternalRubyIssueServiceTest {
     assertThat(result).isSameAs(changelog);
   }
 
+  @Test
+  public void should_create_issue_filter() {
+    Map<String, String> parameters = newHashMap();
+    parameters.put("name", "Long term");
+    parameters.put("description", "Long term issues");
+
+    Result<DefaultIssueFilter> result = service.createIssueFilter(parameters);
+    assertThat(result.ok()).isTrue();
+
+    ArgumentCaptor<DefaultIssueFilter> issueFilterCaptor = ArgumentCaptor.forClass(DefaultIssueFilter.class);
+    verify(issueFilterService).save(issueFilterCaptor.capture(), any(UserSession.class));
+    DefaultIssueFilter issueFilter =  issueFilterCaptor.getValue();
+    assertThat(issueFilter.name()).isEqualTo("Long term");
+    assertThat(issueFilter.description()).isEqualTo("Long term issues");
+  }
+
+  @Test
+  public void should_update_issue_filter() {
+    Map<String, String> parameters = newHashMap();
+    parameters.put("id", "10");
+    parameters.put("name", "Long term");
+    parameters.put("description", "Long term issues");
+
+    Result<DefaultIssueFilter> result = service.updateIssueFilter(parameters);
+    assertThat(result.ok()).isTrue();
+
+    ArgumentCaptor<DefaultIssueFilter> issueFilterCaptor = ArgumentCaptor.forClass(DefaultIssueFilter.class);
+    verify(issueFilterService).update(issueFilterCaptor.capture(), any(UserSession.class));
+    DefaultIssueFilter issueFilter =  issueFilterCaptor.getValue();
+    assertThat(issueFilter.id()).isEqualTo(10L);
+    assertThat(issueFilter.name()).isEqualTo("Long term");
+    assertThat(issueFilter.description()).isEqualTo("Long term issues");
+  }
+
   @Test
   public void should_get_error_on_issue_filter_result_when_no_name() {
     Map<String, String> parameters = newHashMap();
     parameters.put("name", null);
     parameters.put("description", "Long term issues");
 
-    Result result = service.createIssueFilterResult(parameters);
+    Result result = service.createIssueFilterResult(parameters, false);
     assertThat(result.ok()).isFalse();
     assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "name"));
   }
@@ -311,7 +339,7 @@ public class InternalRubyIssueServiceTest {
     parameters.put("name", createLongString(101));
     parameters.put("description", "Long term issues");
 
-    Result result = service.createIssueFilterResult(parameters);
+    Result result = service.createIssueFilterResult(parameters, false);
     assertThat(result.ok()).isFalse();
     assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_too_long", "name", 100));
   }
@@ -322,8 +350,54 @@ public class InternalRubyIssueServiceTest {
     parameters.put("name", "Long term");
     parameters.put("description", createLongString(4001));
 
-    Result result = service.createIssueFilterResult(parameters);
+    Result result = service.createIssueFilterResult(parameters, false);
     assertThat(result.ok()).isFalse();
     assertThat(result.errors()).contains(Result.Message.ofL10n("errors.is_too_long", "description", 4000));
   }
+
+  @Test
+  public void should_get_error_on_issue_filter_result_when_id_is_null_on_update() {
+    Map<String, String> parameters = newHashMap();
+    parameters.put("id", null);
+    parameters.put("name", "Long term");
+    parameters.put("description", "Long term issues");
+
+    Result result = service.createIssueFilterResult(parameters, true);
+    assertThat(result.ok()).isFalse();
+    assertThat(result.errors()).contains(Result.Message.ofL10n("errors.cant_be_empty", "id"));
+  }
+
+  @Test
+  public void should_execute_issue_filter_from_issue_query() {
+    service.execute(IssueQuery.builder().build());
+    verify(issueFilterService).execute(any(IssueQuery.class));
+  }
+
+  @Test
+  public void should_execute_issue_filter_from_existing_filter() {
+    service.execute(10L);
+    verify(issueFilterService).execute(eq(10L), any(UserSession.class));
+  }
+
+  @Test
+  public void should_find_user_issue_filters() {
+    service.findIssueFiltersForUser();
+    verify(issueFilterService).findByUser(any(UserSession.class));
+  }
+
+  @Test
+  public void should_update_data() {
+    Map<String, Object> data = newHashMap();
+    service.updateIssueFilterData(10L, data);
+    verify(issueFilterService).updateData(eq(10L), eq(data), any(UserSession.class));
+  }
+
+  private String createLongString(int size) {
+    String result = "";
+    for (int i = 0; i < size; i++) {
+      result += "c";
+    }
+    return result;
+  }
+
 }
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 6a608f25a47..2bb870c6b26 100644
--- a/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java
+++ b/sonar-server/src/test/java/org/sonar/server/issue/IssueFilterServiceTest.java
@@ -21,23 +21,238 @@
 package org.sonar.server.issue;
 
 import org.junit.Before;
+import org.junit.Test;
+import org.mockito.ArgumentCaptor;
 import org.sonar.api.issue.IssueFinder;
+import org.sonar.api.issue.IssueQuery;
+import org.sonar.core.issue.DefaultIssueFilter;
 import org.sonar.core.issue.db.IssueFilterDao;
+import org.sonar.core.issue.db.IssueFilterDto;
+import org.sonar.server.user.UserSession;
 
-import static org.mockito.Mockito.mock;
+import java.util.List;
+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.fest.assertions.Fail.fail;
+import static org.mockito.Matchers.any;
+import static org.mockito.Mockito.*;
 
 public class IssueFilterServiceTest {
 
-  IssueFilterService service;
+  private IssueFilterService service;
+
+  private IssueFilterDao issueFilterDao;
+  private IssueFinder issueFinder;
 
-  IssueFilterDao issueFilterDao;
-  IssueFinder issueFinder;
+  private UserSession userSession;
 
   @Before
   public void before() {
-    issueFinder = mock(IssueFinder.class);
+    userSession = mock(UserSession.class);
+    when(userSession.isLoggedIn()).thenReturn(true);
+    when(userSession.userId()).thenReturn(10);
+    when(userSession.login()).thenReturn("john");
+
     issueFilterDao = mock(IssueFilterDao.class);
+    issueFinder = mock(IssueFinder.class);
+
     service = new IssueFilterService(issueFilterDao, issueFinder);
   }
 
+  @Test
+  public void should_find_by_id() {
+    IssueFilterDto issueFilterDto = new IssueFilterDto().setId(1L).setName("My Issue").setUserLogin("john");
+    when(issueFilterDao.selectById(1L)).thenReturn(issueFilterDto);
+
+    DefaultIssueFilter issueFilter = service.findById(1L, userSession);
+    assertThat(issueFilter).isNotNull();
+    assertThat(issueFilter.id()).isEqualTo(1L);
+  }
+
+  @Test
+  public void should_not_find_by_id_on_not_existing_issue() {
+    when(issueFilterDao.selectById(1L)).thenReturn(null);
+    try {
+      service.findById(1L, userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Filter not found: 1");
+    }
+  }
+
+  @Test
+  public void should_not_find_by_id_if_not_logged() {
+    when(userSession.isLoggedIn()).thenReturn(false);
+    try {
+      service.findById(1L, userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+    }
+
+    verifyZeroInteractions(issueFilterDao);
+  }
+
+  @Test
+  public void should_not_find_if_user_is_not_the_owner_of_filter() {
+    IssueFilterDto issueFilterDto = new IssueFilterDto().setId(1L).setName("My Issue").setUserLogin("eric");
+    when(issueFilterDao.selectById(1L)).thenReturn(issueFilterDto);
+    try {
+      // John is not authorized to see eric filters
+      service.findById(1L, userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not authorized to get this filter");
+    }
+  }
+
+  @Test
+  public void should_find_by_user() {
+    when(issueFilterDao.selectByUser("john")).thenReturn(newArrayList(new IssueFilterDto().setId(1L).setName("My Issue").setUserLogin("john")));
+
+    List<DefaultIssueFilter> issueFilter = service.findByUser(userSession);
+    assertThat(issueFilter).hasSize(1);
+    assertThat(issueFilter.get(0).id()).isEqualTo(1L);
+  }
+
+  @Test
+  public void should_find_by_user_return_empty_result_for_not_logged_user() {
+    when(userSession.isLoggedIn()).thenReturn(false);
+
+    assertThat(service.findByUser(userSession)).isEmpty();
+  }
+
+  @Test
+  public void should_save() {
+    DefaultIssueFilter issueFilter = new DefaultIssueFilter().setName("My Issue");
+    DefaultIssueFilter result = service.save(issueFilter, userSession);
+    assertThat(result.name()).isEqualTo("My Issue");
+    assertThat(result.user()).isEqualTo("john");
+
+    verify(issueFilterDao).insert(any(IssueFilterDto.class));
+  }
+
+  @Test
+  public void should_not_save_if_not_logged() {
+    when(userSession.isLoggedIn()).thenReturn(false);
+    try {
+      DefaultIssueFilter issueFilter = new DefaultIssueFilter().setName("My Issue");
+      service.save(issueFilter, userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalStateException.class).hasMessage("User is not logged in");
+    }
+    verify(issueFilterDao, never()).insert(any(IssueFilterDto.class));
+  }
+
+  @Test
+  public void should_not_save_if_name_already_used() {
+    when(issueFilterDao.selectByNameAndUser(eq("My Issue"), eq("john"), eq((Long) null))).thenReturn(new IssueFilterDto());
+    try {
+      DefaultIssueFilter issueFilter = new DefaultIssueFilter().setName("My Issue");
+      service.save(issueFilter, userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Name already exists");
+    }
+    verify(issueFilterDao, never()).insert(any(IssueFilterDto.class));
+  }
+
+  @Test
+  public void should_update() {
+    when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Old Filter").setUserLogin("john"));
+
+    DefaultIssueFilter result = service.update(new DefaultIssueFilter().setId(1L).setName("My New Filter"), userSession);
+    assertThat(result.name()).isEqualTo("My New Filter");
+
+    verify(issueFilterDao).update(any(IssueFilterDto.class));
+  }
+
+  @Test
+  public void should_not_update_if_filter_not_found() {
+    when(issueFilterDao.selectById(1L)).thenReturn(null);
+
+    try {
+      service.update(new DefaultIssueFilter().setId(1L).setName("My New Filter"), userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Filter not found: 1");
+    }
+    verify(issueFilterDao, never()).update(any(IssueFilterDto.class));
+  }
+
+  @Test
+  public void should_not_update_if_name_already_used() {
+    when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Old Filter").setUserLogin("john"));
+    when(issueFilterDao.selectByNameAndUser(eq("My Issue"), eq("john"), eq(1L))).thenReturn(new IssueFilterDto());
+
+    try {
+      service.update(new DefaultIssueFilter().setId(1L).setName("My Issue"), userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Name already exists");
+    }
+    verify(issueFilterDao, never()).update(any(IssueFilterDto.class));
+  }
+
+  @Test
+  public void should_update_data() {
+    when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Old Filter").setUserLogin("john"));
+
+    Map<String, Object> data = newHashMap();
+    data.put("componentRoots", "struts");
+
+    DefaultIssueFilter result = service.updateData(1L, data, userSession);
+    assertThat(result.data()).isEqualTo("componentRoots=struts");
+
+    verify(issueFilterDao).update(any(IssueFilterDto.class));
+  }
+
+  @Test
+  public void should_delete() {
+    when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Issues").setUserLogin("john"));
+
+    service.delete(1L, userSession);
+
+    verify(issueFilterDao).delete(1L);
+  }
+
+  @Test
+  public void should_not_delete_if_filter_not_found() {
+    when(issueFilterDao.selectById(1L)).thenReturn(null);
+
+    try {
+      service.delete(1L, userSession);
+      fail();
+    } catch (Exception e) {
+      assertThat(e).isInstanceOf(IllegalArgumentException.class).hasMessage("Filter not found: 1");
+    }
+    verify(issueFilterDao, never()).delete(anyLong());
+  }
+
+  @Test
+  public void should_execute_from_issue_query() {
+    IssueQuery issueQuery = IssueQuery.builder().build();
+
+    service.execute(issueQuery);
+
+    verify(issueFinder).find(issueQuery);
+  }
+
+  @Test
+  public void should_execute_from_filter_id() {
+    when(issueFilterDao.selectById(1L)).thenReturn(new IssueFilterDto().setId(1L).setName("My Old Filter").setUserLogin("john").setData("componentRoots=struts"));
+
+    ArgumentCaptor<IssueQuery> issueQueryCaptor = ArgumentCaptor.forClass(IssueQuery.class);
+
+    service.execute(1L, userSession);
+    verify(issueFinder).find(issueQueryCaptor.capture());
+
+    IssueQuery issueQuery = issueQueryCaptor.getValue();
+    assertThat(issueQuery.componentRoots()).contains("struts");
+  }
+
 }
-- 
2.39.5