From 6f25e1b52930884bc96ba4dfe508e41f9ff756a1 Mon Sep 17 00:00:00 2001 From: Simon Brandhof Date: Tue, 18 Dec 2012 22:40:16 +0100 Subject: [PATCH] Improve UX of measure filters * two different actions for "Save" (an existing filter) and "Save as" (a new filter) * replace the link "Edit" by a pencil icon on the left --- .../resources/org/sonar/l10n/core.properties | 3 +- .../app/controllers/measures_controller.rb | 41 +++++++++++++---- .../app/controllers/treemap_controller.rb | 10 ++--- .../WEB-INF/app/models/measure_filter.rb | 16 +++++-- .../app/models/measure_filter_display_list.rb | 7 +-- .../models/measure_filter_display_treemap.rb | 4 +- .../app/views/measures/_display_list.html.erb | 45 +++++++++---------- .../views/measures/_display_treemap.html.erb | 17 +++---- .../app/views/measures/_edit_form.html.erb | 4 +- ...e_form.html.erb => _save_as_form.html.erb} | 8 ++-- .../app/views/measures/search.html.erb | 42 ++++++++++------- 11 files changed, 119 insertions(+), 78 deletions(-) rename sonar-server/src/main/webapp/WEB-INF/app/views/measures/{_save_form.html.erb => _save_as_form.html.erb} (79%) 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 c980f85eb5d..787986ec9fd 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 @@ -110,6 +110,7 @@ review_verb=Review rule=Rule rules=Rules save=Save +save_as=Save as search_verb=Search select_all=Select all select_verb=Select @@ -386,7 +387,6 @@ session.flash_notice.logged_out=You have been logged out. # MEASURE FILTERS # #------------------------------------------------------------------------------ -measure_filter.close_and_save=Close & Save measure_filter.delete_column=Delete column measure_filter.no_filters=No filters measure_filter.display_as=Display as @@ -449,6 +449,7 @@ measure_filter.treemap.change=Change Treemap measure_filter.add_column_button=Add Column measure_filter.widget.unknown_filter_warning=This widget is configured to display a measure filter that doesn't exist anymore. measure_filter.error.UNKNOWN=Unexpected error. Please contact the administrator. +measure_filter.error.TOO_MANY_RESULTS=Too many results. Please refine your search. #------------------------------------------------------------------------------ # diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb index 391c759f859..79417be5417 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/measures_controller.rb @@ -31,7 +31,7 @@ class MeasuresController < ApplicationController else @filter = MeasureFilter.new end - @filter.criteria=(params.merge({:controller => nil, :action => nil, :search => nil, :widget_id => nil})) + @filter.criteria=criteria_params @filter.enable_default_display @filter.execute(self, :user => current_user) @@ -47,25 +47,31 @@ class MeasuresController < ApplicationController @filter = find_filter(params[:id]) @filter.load_criteria_from_data + @filter.enable_default_display # criteria can be overridden - @filter.override_criteria(params.reject{|k,v| k=='controller' || k=='action' || k=='id'}) + @filter.override_criteria(criteria_params) + + @filter.execute(self, :user => current_user) + @unchanged = true - redirect_to @filter.criteria.merge({:controller => 'measures', :action => 'search', :id => params[:id]}) + render :action => 'search' end - def save_form + # GET /measures/save_as_form?[id=][&criteria] + def save_as_form if params[:id].present? @filter = find_filter(params[:id]) else @filter = MeasureFilter.new end - @filter.criteria=(params) + @filter.criteria=(criteria_params) @filter.convert_criteria_to_data - render :partial => 'measures/save_form' + render :partial => 'measures/save_as_form' end - def save + # POST /measures/save_as?[id=]&name=[¶meters] + def save_as verify_post_request access_denied unless logged_in? @@ -85,10 +91,25 @@ class MeasuresController < ApplicationController current_user.favourited_measure_filters<<@filter if add_to_favourites render :text => @filter.id.to_s, :status => 200 else - render :partial => 'measures/save_form', :status => 400 + render :partial => 'measures/save_as_form', :status => 400 end end + # POST /measures/save?id=&[criteria] + def save + verify_post_request + require_parameters :id + access_denied unless logged_in? + + @filter = find_filter(params[:id]) + @filter.criteria=criteria_params + @filter.convert_criteria_to_data + unless @filter.save + flash[:error]='Error' + end + redirect_to :action => 'filter', :id => @filter.id + end + # GET /measures/manage def manage access_denied unless logged_in? @@ -199,4 +220,8 @@ class MeasuresController < ApplicationController access_denied unless filter.shared || filter.owner?(current_user) filter end + + def criteria_params + params.merge({:controller => nil, :action => nil, :search => nil, :widget_id => nil, :edit => nil}) + end end diff --git a/sonar-server/src/main/webapp/WEB-INF/app/controllers/treemap_controller.rb b/sonar-server/src/main/webapp/WEB-INF/app/controllers/treemap_controller.rb index a2db5e892e8..5213d582629 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/controllers/treemap_controller.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/controllers/treemap_controller.rb @@ -41,11 +41,11 @@ class TreemapController < ApplicationController resource = resource.permanent_resource filter = MeasureFilter.new - filter.set_criteria_value('baseId', resource.id) - filter.set_criteria_value('onBaseComponents', 'true') - filter.set_criteria_value('display', 'treemap') - filter.set_criteria_value('tmSize', size_metric.key) if size_metric - filter.set_criteria_value('tmColor', color_metric.key) if color_metric + filter.set_criteria_value(:baseId, resource.id) + filter.set_criteria_value(:onBaseComponents, 'true') + filter.set_criteria_value(:display, 'treemap') + filter.set_criteria_value(:tmSize, size_metric.key) if size_metric + filter.set_criteria_value(:tmColor, color_metric.key) if color_metric filter.execute(self, :user => current_user) render :text => filter.display.html diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb index 587b4eea749..ca208d907bc 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter.rb @@ -66,7 +66,6 @@ class MeasureFilter < ActiveRecord::Base attr_reader :pagination, :security_exclusions, :base_row, :rows, :display - def sort_key criteria['sort'] end @@ -127,7 +126,8 @@ class MeasureFilter < ActiveRecord::Base @criteria ||= HashWithIndifferentAccess.new if key if value!=nil && value!='' && value!=[''] - @criteria[key]=(value.kind_of?(Array) ? value : value.to_s) + value = (value.kind_of?(Array) ? value : value.to_s) + @criteria[key]=value else @criteria.delete(key) end @@ -136,7 +136,17 @@ class MeasureFilter < ActiveRecord::Base # API used by Displays def set_criteria_default_value(key, value) - set_criteria_value(key, value) unless criteria.has_key?(key) + @criteria ||= HashWithIndifferentAccess.new + unless @criteria.has_key?(key) + if key + if value!=nil && value!='' && value!=[''] + value = (value.kind_of?(Array) ? value : value.to_s) + @criteria[key]=value + else + @criteria.delete(key) + end + end + end end def load_criteria_from_data diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_list.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_list.rb index 20a64295ace..2a109537790 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_list.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_list.rb @@ -20,6 +20,7 @@ require 'set' class MeasureFilterDisplayList < MeasureFilterDisplay KEY = :list + MAX_PAGE_SIZE = 200 class Column attr_reader :key, :metric, :period @@ -85,9 +86,9 @@ class MeasureFilterDisplayList < MeasureFilterDisplay # default values filter.set_criteria_default_value(:cols, ['metric:alert_status', 'name', 'date', 'metric:ncloc', 'metric:violations', 'links']) filter.set_criteria_default_value(:sort, 'name') - filter.set_criteria_default_value(:asc, 'true') - filter.set_criteria_default_value(:pageSize, '30') - filter.set_criteria_value(:pageSize, [filter.criteria[:pageSize].to_i, 200].min) + filter.set_criteria_default_value(:asc, true) + filter.set_criteria_default_value(:pageSize, 30) + filter.set_criteria_value(:pageSize, MAX_PAGE_SIZE) if filter.criteria(:pageSize).to_i>MAX_PAGE_SIZE @columns = [] metrics = [] diff --git a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_treemap.rb b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_treemap.rb index f4d70b014d9..bbaf3e8b765 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_treemap.rb +++ b/sonar-server/src/main/webapp/WEB-INF/app/models/measure_filter_display_treemap.rb @@ -22,7 +22,7 @@ class MeasureFilterDisplayTreemap < MeasureFilterDisplay include ActionView::Helpers::UrlHelper KEY = :treemap - PROPERTY_KEYS = Set.new([:tmSize, :tmColor]) + PROPERTY_KEYS = Set.new([:tmSize, :tmColor, :tmHeight]) MAX_RESULTS = 1000 DEFAULT_HEIGHT_PERCENTS = 55 attr_reader :id, :size, :size_metric, :color_metric, :height_percents @@ -37,8 +37,6 @@ class MeasureFilterDisplayTreemap < MeasureFilterDisplay @filter.metrics=([@size_metric, @color_metric].compact) @id_count = 0 - filter.set_criteria_value(:sort, "metric:#{@size_metric.key}") if @size_metric - filter.set_criteria_value(:asc, 'true') filter.set_criteria_value(:pageSize, MAX_RESULTS) filter.set_criteria_value(:page, 1) end diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_list.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_list.html.erb index 9df1f2374d8..83654fa0081 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_list.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_list.html.erb @@ -5,8 +5,10 @@ $j('#measure_filter_foot<%= widget_id -%>_pages').hide(); $j('#measure_filter_foot<%= widget_id -%>_loading').show(); - var url = decodeURI('<%= url_for filter.criteria.merge(:controller => 'measures', :action => 'search', :page => nil, :sort => nil, :asc => nil, :search => nil, :widget_id => widget_id, :trailing_slash => true) -%>'); - url += '&sort=' + sort + '&asc=' + asc + '&page=' + page; + filterCriteria['sort']=sort; + filterCriteria['asc']=asc; + filterCriteria['page']=page; + var url=baseUrl + '/measures/search/?' + $j.param(filterCriteria); <% if widget_id %> $j('#measure_filter_list<%= widget_id -%>').load(url); @@ -22,33 +24,32 @@ display_favourites = logged_in? colspan = filter.display.columns.size colspan += 1 if display_favourites - table_columns = [] - table_columns << '' if display_favourites - table_columns.concat(filter.display.columns.map { |c| c.key }) if edit_mode content_for :script do %> @@ -138,9 +136,6 @@ <%= message 'close' -%> - <% if filter.owner?(current_user) %> - <%= message('measure_filter.close_and_save') -%> - <% end %> diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_treemap.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_treemap.html.erb index 91f47ceae51..9715c75e694 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_treemap.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_display_treemap.html.erb @@ -47,9 +47,6 @@ <%= image_tag 'loading.gif', :id => "tm-loading-#{treemap_id}", :style => 'display:none' -%> <%= message 'close' -%> - <% if filter.owner?(current_user) %> - <%= message('measure_filter.close_and_save') -%> - <% end %> @@ -60,17 +57,17 @@ @@ -80,7 +77,7 @@ <% if filter.rows.empty? %>

<%= message('no_data') -%>

<% elsif filter.pagination.pages>1 %> -

<%= message('measure_filter.errors.TOO_MANY_RESULTS') -%>

+

<%= message('measure_filter.error.TOO_MANY_RESULTS') -%>

<% else %>
<%= filter.display.html -%> diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_edit_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_edit_form.html.erb index 530cfefd2e4..14af4202db5 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_edit_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_edit_form.html.erb @@ -28,6 +28,8 @@ \ No newline at end of file diff --git a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_form.html.erb b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_as_form.html.erb similarity index 79% rename from sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_form.html.erb rename to sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_as_form.html.erb index f848acdbd07..d834e3ab733 100644 --- a/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_form.html.erb +++ b/sonar-server/src/main/webapp/WEB-INF/app/views/measures/_save_as_form.html.erb @@ -1,4 +1,4 @@ -
+
@@ -24,13 +24,13 @@
- - <%= h message('cancel') -%> + + <%= h message('cancel') -%>
+
@@ -11,32 +13,39 @@ <% if @filter.display - edit_mode = (params[:edit]=='true') - unless edit_mode + edit_mode = (params[:edit]=='true') + unless edit_mode %> +
  • + <%= message("measure_filter.#{@filter.display.key}.change") -%> +
    • + <% end %>
  • - <%= message("measure_filter.#{@filter.display.key}.change") -%> + <%= message 'measure_filter.display_as' -%>: + <% MeasureFilterDisplay.keys.each do |display_key| %> + <%= link_to_if display_key!=@filter.display.key, message("measure_filter.display.#{display_key}"), @filter.criteria.merge(:action => 'search', :display => display_key, :id => @filter.id), :id => "display_as_#{display_key}" -%> + <% end %>
    • <% end %> -
  • - <%= message 'measure_filter.display_as' -%>: - <% MeasureFilterDisplay.keys.each do |display_key| %> - <%= link_to_if display_key!=@filter.display.key, message("measure_filter.display.#{display_key}"), params.merge(:action => 'search', :display => display_key, :id => @filter.id), :id => "display_as_#{display_key}" -%> - <% end %> -
    • - <% end %> <% if logged_in? %> <% if @filter.id %> -
  • <%= message('copy') -%>
    • +
  • <%= message('copy') -%>
    • <% end %> - <% if @filter.owner?(current_user) %> -
  • <%= message('save') -%>
    • + <% if !defined?(@unchanged) && @filter.id && @filter.owner?(current_user)%> +
  • + <%= link_to message('save'), params.merge({:action => 'save', :id => @filter.id}), :class => 'link-action', :id => 'save', :method => :post -%> +
    • + <% end %> + <% unless @filter.id %> +
  • + <%= message('save_as') -%> +
    • <% end %> <% end %>
    - <% if @filter.name.present? %> + <% if @filter.id && @filter.name.present? %> <%= h @filter.name -%> <% if !@filter.shared %> @@ -52,6 +61,9 @@ <% if @filter.description.present? %> - <%= h @filter.description -%> <% end %> + <% if @filter.owner?(current_user) %> +  <%= image_tag 'pencil-small.png', :alt => message('edit') -%> + <% end %> <% end %>
    -- 2.39.5