From 03de773f2bebe4ae70d25086f85e88eb8fccb3b0 Mon Sep 17 00:00:00 2001 From: Dan Rice Date: Mon, 18 Mar 2013 23:31:57 +0200 Subject: [PATCH 1/7] Fix a case mismatch in one of the tests --- features/view_done.feature | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/features/view_done.feature b/features/view_done.feature index a586b64c..5f184879 100644 --- a/features/view_done.feature +++ b/features/view_done.feature @@ -35,7 +35,7 @@ Feature: Show done Scenario Outline: I can see all todos completed in the last timeperiod When I go to the Then I should see "todo 1" - And I should see "Completed Today" + And I should see "Completed today" And I should see "Completed in the rest of this week" And I should see "Completed in the rest of this month" From 6e2f1a8e5f0700119a764203197d20a5becafff7 Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sun, 17 Mar 2013 19:38:57 -0600 Subject: [PATCH 2/7] De-dupe pie chart view Pull logic into the controller from the view. There were only a couple of tiny differences between the running actions and the total actions view, so I added a couple of instance variables and then called the same template twice. --- app/controllers/stats_controller.rb | 12 ++++++++++-- .../stats/context_running_actions_data.html.erb | 15 --------------- .../stats/context_total_actions_data.html.erb | 15 --------------- app/views/stats/pie_chart_data.html.erb | 9 +++++++++ 4 files changed, 19 insertions(+), 32 deletions(-) delete mode 100755 app/views/stats/context_running_actions_data.html.erb delete mode 100755 app/views/stats/context_total_actions_data.html.erb create mode 100755 app/views/stats/pie_chart_data.html.erb diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 545a87a1..5e669cf0 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -179,17 +179,21 @@ class StatsController < ApplicationController def context_total_actions_data all_actions_per_context = Stats::TopContextsQuery.new(current_user).result + @title = t('stats.spread_of_actions_for_all_context') + @alpha = 70 prep_context_data_for_view(all_actions_per_context) - render :layout => false + render :pie_chart_data, :layout => false end def context_running_actions_data all_actions_per_context = Stats::TopContextsQuery.new(current_user, :running => true).result + @title = t('stats.spread_of_running_actions_for_visible_contexts') + @alpha = 60 prep_context_data_for_view(all_actions_per_context) - render :layout => false + render :pie_chart_data, :layout => false end def actions_day_of_week_all_data @@ -363,6 +367,10 @@ class StatsController < ApplicationController end @truncate_chars = 15 + + @pie_slices = Array.new(size){|i| @actions_per_context[i]['total'].to_i*100/@sum } + @pie_labels = Array.new(size){|i| @actions_per_context[i]['name'].truncate(@truncate_chars, :omission => '...') } + @pie_links = Array.new(size){|i| context_path(@actions_per_context[i]['id'])} end def init diff --git a/app/views/stats/context_running_actions_data.html.erb b/app/views/stats/context_running_actions_data.html.erb deleted file mode 100755 index 690a82f4..00000000 --- a/app/views/stats/context_running_actions_data.html.erb +++ /dev/null @@ -1,15 +0,0 @@ -<% - size = @actions_per_context.size() - pie_slices = Array.new(size){|i| @actions_per_context[i]['total'].to_i*100/@sum } - pie_labels = Array.new(size){|i| truncate(@actions_per_context[i]['name'], :length => @truncate_chars, :omission => '...') } - pie_links = Array.new(size){|i| url_for :controller => "contexts", :action => "show", :id=>@actions_per_context[i]['id']} --%> -&title=<%= t('stats.spread_of_running_actions_for_visible_contexts') %>,{font-size:16}& -&pie=60,#505050,{font-size: 12px; color: #404040;}& -&x_axis_steps=1& &y_ticks=5,10,5& &line=3,#87421F& &y_min=0& &y_max=20& -&values=<%= pie_slices.join(",") %>& -&pie_labels=<%= pie_labels.join(",") %>& -&links=<%= pie_links.join(",") %>& -&colours=#d01f3c,#356aa0,#C79810,#c61fd0,#1fc6d0,#1fd076,#72d01f,#c6d01f,#d0941f,#40941f& -&tool_tip=#x_label#: #val#%25& -&x_label_style=9,,2,1& \ No newline at end of file diff --git a/app/views/stats/context_total_actions_data.html.erb b/app/views/stats/context_total_actions_data.html.erb deleted file mode 100755 index 733bb5a7..00000000 --- a/app/views/stats/context_total_actions_data.html.erb +++ /dev/null @@ -1,15 +0,0 @@ -<% - size = @actions_per_context.size() - pie_slices = Array.new(size){|i| @actions_per_context[i]['total'].to_i*100/@sum } - pie_labels = Array.new(size){|i| truncate(@actions_per_context[i]['name'], :length => @truncate_chars, :omission => '...') } - pie_links = Array.new(size){|i| url_for :controller => "contexts", :action => "show", :id=>@actions_per_context[i]['id']} --%> -&title=<%= t('stats.spread_of_actions_for_all_context') %>,{font-size:16}& -&pie=70,#505050,{font-size: 12px; color: #404040;}& -&x_axis_steps=1& &y_ticks=5,10,5& &line=3,#87421F& &y_min=0& &y_max=20& -&values=<%= pie_slices.join(",") %>& -&pie_labels=<%= pie_labels.join(",") %>& -&links=<%= pie_links.join(",") %>& -&colours=#d01f3c,#356aa0,#C79810,#c61fd0,#1fc6d0,#1fd076,#72d01f,#c6d01f,#d0941f,#40941f& -&tool_tip=#x_label#: #val#%25& -&x_label_style=9,,2,1& \ No newline at end of file diff --git a/app/views/stats/pie_chart_data.html.erb b/app/views/stats/pie_chart_data.html.erb new file mode 100755 index 00000000..de466212 --- /dev/null +++ b/app/views/stats/pie_chart_data.html.erb @@ -0,0 +1,9 @@ +&title=<%= @title %>,{font-size:16}& +&pie=<%= @alpha %>,#505050,{font-size: 12px; color: #404040;}& +&x_axis_steps=1& &y_ticks=5,10,5& &line=3,#87421F& &y_min=0& &y_max=20& +&values=<%= @pie_slices.join(",") %>& +&pie_labels=<%= @pie_labels.join(",") %>& +&links=<%= @pie_links.join(",") %>& +&colours=#d01f3c,#356aa0,#C79810,#c61fd0,#1fc6d0,#1fd076,#72d01f,#c6d01f,#d0941f,#40941f& +&tool_tip=#x_label#: #val#%25& +&x_label_style=9,,2,1& From f3a076c2af622750bf8383697846ace18734a5c8 Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sun, 17 Mar 2013 19:42:51 -0600 Subject: [PATCH 3/7] Turn unreferenced ivars into local variables --- app/controllers/stats_controller.rb | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 5e669cf0..26609e5c 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -347,30 +347,28 @@ class StatsController < ApplicationController def prep_context_data_for_view(all_actions_per_context) - @sum = all_actions_per_context.inject(0){|sum, apc| sum += apc['total'].to_i } + sum = all_actions_per_context.inject(0){|sum, apc| sum += apc['total'].to_i } pie_cutoff=10 size = [all_actions_per_context.size, pie_cutoff].min # explicitely copy contents of hash to avoid ending up with two arrays pointing to same hashes - @actions_per_context = Array.new(size){|i| { + actions_per_context = Array.new(size){|i| { 'name' => all_actions_per_context[i]['name'], 'total' => all_actions_per_context[i]['total'].to_i, 'id' => all_actions_per_context[i]['id'] } } if all_actions_per_context.size > pie_cutoff - @actions_per_context[size-1]['name']=t('stats.other_actions_label') - @actions_per_context[size-1]['total']=@actions_per_context[size-1]['total'] - @actions_per_context[size-1]['id']=-1 - size.upto(all_actions_per_context.size-1){ |i| @actions_per_context[size-1]['total']+=(all_actions_per_context[i]['total'].to_i) } + actions_per_context[size-1]['name']=t('stats.other_actions_label') + actions_per_context[size-1]['total']=actions_per_context[size-1]['total'] + actions_per_context[size-1]['id']=-1 + size.upto(all_actions_per_context.size-1){ |i| actions_per_context[size-1]['total']+=(all_actions_per_context[i]['total'].to_i) } end - @truncate_chars = 15 - - @pie_slices = Array.new(size){|i| @actions_per_context[i]['total'].to_i*100/@sum } - @pie_labels = Array.new(size){|i| @actions_per_context[i]['name'].truncate(@truncate_chars, :omission => '...') } - @pie_links = Array.new(size){|i| context_path(@actions_per_context[i]['id'])} + @pie_slices = Array.new(size){|i| actions_per_context[i]['total'].to_i*100/sum } + @pie_labels = Array.new(size){|i| actions_per_context[i]['name'].truncate(15, :omission => '...') } + @pie_links = Array.new(size){|i| context_path(actions_per_context[i]['id'])} end def init From 9f3470f9dc4f570811ab8d205c8d21934335dacf Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sun, 17 Mar 2013 19:47:35 -0600 Subject: [PATCH 4/7] Shave off slight redundancy --- app/controllers/stats_controller.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 26609e5c..afe189c5 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -347,26 +347,25 @@ class StatsController < ApplicationController def prep_context_data_for_view(all_actions_per_context) - sum = all_actions_per_context.inject(0){|sum, apc| sum += apc['total'].to_i } + sum = all_actions_per_context.inject(0){|sum, apc| sum + apc['total']} pie_cutoff=10 size = [all_actions_per_context.size, pie_cutoff].min - # explicitely copy contents of hash to avoid ending up with two arrays pointing to same hashes + # explicitly copy contents of hash to avoid ending up with two arrays pointing to same hashes actions_per_context = Array.new(size){|i| { 'name' => all_actions_per_context[i]['name'], - 'total' => all_actions_per_context[i]['total'].to_i, + 'total' => all_actions_per_context[i]['total'], 'id' => all_actions_per_context[i]['id'] } } if all_actions_per_context.size > pie_cutoff - actions_per_context[size-1]['name']=t('stats.other_actions_label') - actions_per_context[size-1]['total']=actions_per_context[size-1]['total'] - actions_per_context[size-1]['id']=-1 - size.upto(all_actions_per_context.size-1){ |i| actions_per_context[size-1]['total']+=(all_actions_per_context[i]['total'].to_i) } + actions_per_context[-1]['name']=t('stats.other_actions_label') + actions_per_context[-1]['id']=-1 + size.upto(all_actions_per_context.size-1){ |i| actions_per_context[-1]['total']+=(all_actions_per_context[i]['total']) } end - @pie_slices = Array.new(size){|i| actions_per_context[i]['total'].to_i*100/sum } + @pie_slices = Array.new(size){|i| actions_per_context[i]['total']*100/sum } @pie_labels = Array.new(size){|i| actions_per_context[i]['name'].truncate(15, :omission => '...') } @pie_links = Array.new(size){|i| context_path(actions_per_context[i]['id'])} end From 961227da0c78a00fa35d97404a6c7268117fc55f Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sun, 17 Mar 2013 20:02:16 -0600 Subject: [PATCH 5/7] Extract pie chart data logic into model layer Move most of the tests for this logic into the unit test layer. --- app/controllers/stats_controller.rb | 41 +----- app/models/stats/pie_chart_data.rb | 37 ++++++ app/views/stats/pie_chart_data.html.erb | 10 +- test/functional/context_actions_data_test.rb | 130 +------------------ test/unit/pie_chart_data_test.rb | 88 +++++++++++++ 5 files changed, 142 insertions(+), 164 deletions(-) create mode 100644 app/models/stats/pie_chart_data.rb create mode 100644 test/unit/pie_chart_data_test.rb diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index afe189c5..107eab85 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -178,20 +178,18 @@ class StatsController < ApplicationController end def context_total_actions_data - all_actions_per_context = Stats::TopContextsQuery.new(current_user).result - @title = t('stats.spread_of_actions_for_all_context') - @alpha = 70 - prep_context_data_for_view(all_actions_per_context) + actions = Stats::TopContextsQuery.new(current_user).result + + @data = Stats::PieChartData.new(actions, t('stats.spread_of_actions_for_all_context'), 70) + @data.calculate render :pie_chart_data, :layout => false end def context_running_actions_data - all_actions_per_context = Stats::TopContextsQuery.new(current_user, :running => true).result - - @title = t('stats.spread_of_running_actions_for_visible_contexts') - @alpha = 60 - prep_context_data_for_view(all_actions_per_context) + actions = Stats::TopContextsQuery.new(current_user, :running => true).result + @data = Stats::PieChartData.new(actions, t('stats.spread_of_running_actions_for_visible_contexts'), 60) + @data.calculate render :pie_chart_data, :layout => false end @@ -345,31 +343,6 @@ class StatsController < ApplicationController private - def prep_context_data_for_view(all_actions_per_context) - - sum = all_actions_per_context.inject(0){|sum, apc| sum + apc['total']} - - pie_cutoff=10 - size = [all_actions_per_context.size, pie_cutoff].min - - # explicitly copy contents of hash to avoid ending up with two arrays pointing to same hashes - actions_per_context = Array.new(size){|i| { - 'name' => all_actions_per_context[i]['name'], - 'total' => all_actions_per_context[i]['total'], - 'id' => all_actions_per_context[i]['id'] - } } - - if all_actions_per_context.size > pie_cutoff - actions_per_context[-1]['name']=t('stats.other_actions_label') - actions_per_context[-1]['id']=-1 - size.upto(all_actions_per_context.size-1){ |i| actions_per_context[-1]['total']+=(all_actions_per_context[i]['total']) } - end - - @pie_slices = Array.new(size){|i| actions_per_context[i]['total']*100/sum } - @pie_labels = Array.new(size){|i| actions_per_context[i]['name'].truncate(15, :omission => '...') } - @pie_links = Array.new(size){|i| context_path(actions_per_context[i]['id'])} - end - def init @me = self # for meta programming diff --git a/app/models/stats/pie_chart_data.rb b/app/models/stats/pie_chart_data.rb new file mode 100644 index 00000000..10cc1eec --- /dev/null +++ b/app/models/stats/pie_chart_data.rb @@ -0,0 +1,37 @@ +module Stats + class PieChartData + + attr_reader :all_actions_per_context, :values, :labels, :ids, :alpha, :title + def initialize(all_actions_per_context, title, alpha) + @all_actions_per_context = all_actions_per_context + @title = title + @alpha = alpha + end + + def calculate + sum = all_actions_per_context.inject(0){|sum, apc| sum + apc['total']} + + pie_cutoff=10 + size = [all_actions_per_context.size, pie_cutoff].min + + # explicitly copy contents of hash to avoid ending up with two arrays pointing to same hashes + actions_per_context = Array.new(size){|i| { + 'name' => all_actions_per_context[i]['name'], + 'total' => all_actions_per_context[i]['total'], + 'id' => all_actions_per_context[i]['id'] + } } + + if all_actions_per_context.size > pie_cutoff + actions_per_context[-1]['name']=I18n.t('stats.other_actions_label') + actions_per_context[-1]['id']=-1 + size.upto(all_actions_per_context.size-1){ |i| actions_per_context[-1]['total']+=(all_actions_per_context[i]['total']) } + end + + @values = Array.new(size){|i| actions_per_context[i]['total']*100/sum } + @labels = Array.new(size){|i| actions_per_context[i]['name'].truncate(15, :omission => '...') } + @ids = Array.new(size){|i| actions_per_context[i]['id']} + end + + + end +end diff --git a/app/views/stats/pie_chart_data.html.erb b/app/views/stats/pie_chart_data.html.erb index de466212..31beb727 100755 --- a/app/views/stats/pie_chart_data.html.erb +++ b/app/views/stats/pie_chart_data.html.erb @@ -1,9 +1,9 @@ -&title=<%= @title %>,{font-size:16}& -&pie=<%= @alpha %>,#505050,{font-size: 12px; color: #404040;}& +&title=<%= @data.title %>,{font-size:16}& +&pie=<%= @data.alpha %>,#505050,{font-size: 12px; color: #404040;}& &x_axis_steps=1& &y_ticks=5,10,5& &line=3,#87421F& &y_min=0& &y_max=20& -&values=<%= @pie_slices.join(",") %>& -&pie_labels=<%= @pie_labels.join(",") %>& -&links=<%= @pie_links.join(",") %>& +&values=<%= @data.values.join(",") %>& +&pie_labels=<%= @data.labels.join(",") %>& +&links=<%= @data.ids.map{|id| context_path(id)}.join(",") %>& &colours=#d01f3c,#356aa0,#C79810,#c61fd0,#1fc6d0,#1fd076,#72d01f,#c6d01f,#d0941f,#40941f& &tool_tip=#x_label#: #val#%25& &x_label_style=9,,2,1& diff --git a/test/functional/context_actions_data_test.rb b/test/functional/context_actions_data_test.rb index e87f6f01..7bce3991 100644 --- a/test/functional/context_actions_data_test.rb +++ b/test/functional/context_actions_data_test.rb @@ -3,152 +3,32 @@ require File.expand_path(File.dirname(__FILE__) + '/../test_helper') class ContextActionsDataTest < ActionController::TestCase tests StatsController - def test_total_with_0_items - login_as(:admin_user) - Stats::TopContextsQuery.any_instance.stubs(:result).returns [] - - get :context_total_actions_data - - assert_equal [], assigns[:actions_per_context] - end - - def test_total_with_less_than_10_items - login_as(:admin_user) - contexts = [ - {'id' => 1, 'name' => 'one', 'total' => 11}, - {'id' => 2, 'name' => 'two', 'total' => 4}, - {'id' => 3, 'name' => 'three', 'total' => 8}, - {'id' => 4, 'name' => 'four', 'total' => 13}, - {'id' => 5, 'name' => 'five', 'total' => 20}, - {'id' => 6, 'name' => 'six', 'total' => 17}, - {'id' => 7, 'name' => 'seven', 'total' => 5}, - {'id' => 8, 'name' => 'eight', 'total' => 1}, - {'id' => 9, 'name' => 'nine', 'total' => 6} - ] - Stats::TopContextsQuery.any_instance.stubs(:result).returns contexts - - get :context_total_actions_data - - assert_equal contexts, assigns[:actions_per_context] - end - - def test_total_with_exactly_10_items - login_as(:admin_user) - contexts = [ - {'id' => 1, 'name' => 'one', 'total' => 11}, - {'id' => 2, 'name' => 'two', 'total' => 4}, - {'id' => 3, 'name' => 'three', 'total' => 8}, - {'id' => 4, 'name' => 'four', 'total' => 13}, - {'id' => 5, 'name' => 'five', 'total' => 20}, - {'id' => 6, 'name' => 'six', 'total' => 17}, - {'id' => 7, 'name' => 'seven', 'total' => 5}, - {'id' => 8, 'name' => 'eight', 'total' => 1}, - {'id' => 9, 'name' => 'nine', 'total' => 6}, - {'id' => 10, 'name' => 'ten', 'total' => 19} - ] - Stats::TopContextsQuery.any_instance.stubs(:result).returns contexts - - get :context_total_actions_data - - assert_equal contexts, assigns[:actions_per_context] - end - def test_total_with_more_than_10_items login_as(:admin_user) contexts = [ {'id' => 1, 'name' => 'one', 'total' => 11}, {'id' => 2, 'name' => 'two', 'total' => 4}, - {'id' => 3, 'name' => 'three', 'total' => 8}, - {'id' => 4, 'name' => 'four', 'total' => 13}, - {'id' => 5, 'name' => 'five', 'total' => 20}, - {'id' => 6, 'name' => 'six', 'total' => 17}, - {'id' => 7, 'name' => 'seven', 'total' => 5}, - {'id' => 8, 'name' => 'eight', 'total' => 1}, - {'id' => 9, 'name' => 'nine', 'total' => 6}, - {'id' => 10, 'name' => 'ten', 'total' => 19}, - {'id' => 11, 'name' => 'eleven', 'total' => 14} + {'id' => 3, 'name' => 'three', 'total' => 8} ] Stats::TopContextsQuery.any_instance.stubs(:result).returns contexts get :context_total_actions_data - contexts.pop - contexts[-1] = {'id' => -1, 'name' => '(others)', 'total' => 33} - assert_equal contexts, assigns[:actions_per_context] + assert_equal [47, 17, 34], assigns[:data].values end - def test_running_with_0_items - login_as(:admin_user) - Stats::TopContextsQuery.any_instance.stubs(:result).returns [] - - get :context_running_actions_data - - assert_equal [], assigns[:actions_per_context] - end - - def test_running_with_less_than_10_items + def test_running_actions login_as(:admin_user) contexts = [ {'id' => 1, 'name' => 'one', 'total' => 11}, {'id' => 2, 'name' => 'two', 'total' => 4}, - {'id' => 3, 'name' => 'three', 'total' => 8}, - {'id' => 4, 'name' => 'four', 'total' => 13}, - {'id' => 5, 'name' => 'five', 'total' => 20}, - {'id' => 6, 'name' => 'six', 'total' => 17}, - {'id' => 7, 'name' => 'seven', 'total' => 5}, - {'id' => 8, 'name' => 'eight', 'total' => 1}, - {'id' => 9, 'name' => 'nine', 'total' => 6} + {'id' => 3, 'name' => 'three', 'total' => 8} ] Stats::TopContextsQuery.any_instance.stubs(:result).returns contexts get :context_running_actions_data - assert_equal contexts, assigns[:actions_per_context] - end - - def test_running_with_exactly_10_items - login_as(:admin_user) - contexts = [ - {'id' => 1, 'name' => 'one', 'total' => 11}, - {'id' => 2, 'name' => 'two', 'total' => 4}, - {'id' => 3, 'name' => 'three', 'total' => 8}, - {'id' => 4, 'name' => 'four', 'total' => 13}, - {'id' => 5, 'name' => 'five', 'total' => 20}, - {'id' => 6, 'name' => 'six', 'total' => 17}, - {'id' => 7, 'name' => 'seven', 'total' => 5}, - {'id' => 8, 'name' => 'eight', 'total' => 1}, - {'id' => 9, 'name' => 'nine', 'total' => 6}, - {'id' => 10, 'name' => 'ten', 'total' => 19} - ] - Stats::TopContextsQuery.any_instance.stubs(:result).returns contexts - - get :context_running_actions_data - - assert_equal contexts, assigns[:actions_per_context] - end - - def test_running_with_more_than_10_items - login_as(:admin_user) - contexts = [ - {'id' => 1, 'name' => 'one', 'total' => 11}, - {'id' => 2, 'name' => 'two', 'total' => 4}, - {'id' => 3, 'name' => 'three', 'total' => 8}, - {'id' => 4, 'name' => 'four', 'total' => 13}, - {'id' => 5, 'name' => 'five', 'total' => 20}, - {'id' => 6, 'name' => 'six', 'total' => 17}, - {'id' => 7, 'name' => 'seven', 'total' => 5}, - {'id' => 8, 'name' => 'eight', 'total' => 1}, - {'id' => 9, 'name' => 'nine', 'total' => 6}, - {'id' => 10, 'name' => 'ten', 'total' => 19}, - {'id' => 11, 'name' => 'eleven', 'total' => 14} - ] - Stats::TopContextsQuery.any_instance.stubs(:result).returns contexts - - get :context_running_actions_data - - contexts.pop - contexts[-1] = {'id' => -1, 'name' => '(others)', 'total' => 33} - assert_equal contexts, assigns[:actions_per_context] + assert_equal [47, 17, 34], assigns[:data].values end end diff --git a/test/unit/pie_chart_data_test.rb b/test/unit/pie_chart_data_test.rb new file mode 100644 index 00000000..bf5a787d --- /dev/null +++ b/test/unit/pie_chart_data_test.rb @@ -0,0 +1,88 @@ +require File.expand_path(File.dirname(__FILE__) + '/../minimal_test_helper') +require 'app/models/stats/pie_chart_data' +require 'active_support/core_ext/string' + +class Stats::PieChartDataTest < Test::Unit::TestCase + + def setup + xx = { :stats => { :other_actions_label => '(other)' } } + I18n.backend.store_translations(:xx, xx) + I18n.locale = :xx + end + + def test_with_0_items + data = Stats::PieChartData.new([], 'a chart', 50) + data.calculate + + assert_equal [], data.values + assert_equal [], data.labels + assert_equal [], data.ids + end + + def test_with_less_than_10_items + items = [ + {'id' => 1, 'name' => 'one', 'total' => 11}, + {'id' => 2, 'name' => 'two', 'total' => 4}, + {'id' => 3, 'name' => 'three', 'total' => 8}, + {'id' => 4, 'name' => 'four', 'total' => 13}, + {'id' => 5, 'name' => 'five', 'total' => 20}, + {'id' => 6, 'name' => 'six', 'total' => 17}, + {'id' => 7, 'name' => 'seven', 'total' => 5}, + {'id' => 8, 'name' => 'eight', 'total' => 1}, + {'id' => 9, 'name' => 'nine', 'total' => 6} + ] + + data = Stats::PieChartData.new(items, 'a chart', 50) + data.calculate + + assert_equal [12, 4, 9, 15, 23, 20, 5, 1, 7], data.values + assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine"], data.labels + assert_equal [1, 2, 3, 4, 5, 6, 7, 8, 9], data.ids + end + + def test_with_exactly_10_items + items = [ + {'id' => 1, 'name' => 'one', 'total' => 11}, + {'id' => 2, 'name' => 'two', 'total' => 4}, + {'id' => 3, 'name' => 'three', 'total' => 8}, + {'id' => 4, 'name' => 'four', 'total' => 13}, + {'id' => 5, 'name' => 'five', 'total' => 20}, + {'id' => 6, 'name' => 'six', 'total' => 17}, + {'id' => 7, 'name' => 'seven', 'total' => 5}, + {'id' => 8, 'name' => 'eight', 'total' => 1}, + {'id' => 9, 'name' => 'nine', 'total' => 6}, + {'id' => 10, 'name' => 'ten', 'total' => 19} + ] + + data = Stats::PieChartData.new(items, 'a chart', 50) + data.calculate + + assert_equal [10, 3, 7, 12, 19, 16, 4, 0, 5, 18], data.values + assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"], data.labels + assert_equal [1, 2, 3, 4, 5, 6, 7, 8, 9, 10], data.ids + end + + def test_with_more_than_10_items + items = [ + {'id' => 1, 'name' => 'one', 'total' => 11}, + {'id' => 2, 'name' => 'two', 'total' => 4}, + {'id' => 3, 'name' => 'three', 'total' => 8}, + {'id' => 4, 'name' => 'four', 'total' => 13}, + {'id' => 5, 'name' => 'five', 'total' => 20}, + {'id' => 6, 'name' => 'six', 'total' => 17}, + {'id' => 7, 'name' => 'seven', 'total' => 5}, + {'id' => 8, 'name' => 'eight', 'total' => 1}, + {'id' => 9, 'name' => 'nine', 'total' => 6}, + {'id' => 10, 'name' => 'ten', 'total' => 19}, + {'id' => 11, 'name' => 'eleven', 'total' => 14} + ] + + data = Stats::PieChartData.new(items, 'a chart', 50) + data.calculate + + assert_equal [9, 3, 6, 11, 16, 14, 4, 0, 5, 27], data.values + assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "(other)"], data.labels + assert_equal [1, 2, 3, 4, 5, 6, 7, 8, 9, -1], data.ids + end + +end From a690e4a4cdd9c02fb548531435c6486b1f486a9f Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sun, 17 Mar 2013 20:37:29 -0600 Subject: [PATCH 6/7] Extract methods in pie chart data model --- app/controllers/stats_controller.rb | 2 - app/models/stats/pie_chart_data.rb | 97 ++++++++++++++++++++++------- test/unit/pie_chart_data_test.rb | 4 -- 3 files changed, 73 insertions(+), 30 deletions(-) diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 107eab85..b92e7287 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -181,7 +181,6 @@ class StatsController < ApplicationController actions = Stats::TopContextsQuery.new(current_user).result @data = Stats::PieChartData.new(actions, t('stats.spread_of_actions_for_all_context'), 70) - @data.calculate render :pie_chart_data, :layout => false end @@ -189,7 +188,6 @@ class StatsController < ApplicationController def context_running_actions_data actions = Stats::TopContextsQuery.new(current_user, :running => true).result @data = Stats::PieChartData.new(actions, t('stats.spread_of_running_actions_for_visible_contexts'), 60) - @data.calculate render :pie_chart_data, :layout => false end diff --git a/app/models/stats/pie_chart_data.rb b/app/models/stats/pie_chart_data.rb index 10cc1eec..9a5ded53 100644 --- a/app/models/stats/pie_chart_data.rb +++ b/app/models/stats/pie_chart_data.rb @@ -1,37 +1,86 @@ module Stats class PieChartData - attr_reader :all_actions_per_context, :values, :labels, :ids, :alpha, :title - def initialize(all_actions_per_context, title, alpha) - @all_actions_per_context = all_actions_per_context + attr_reader :all_totals, :alpha, :title + def initialize(all_totals, title, alpha) + @all_totals = all_totals @title = title @alpha = alpha end - def calculate - sum = all_actions_per_context.inject(0){|sum, apc| sum + apc['total']} - - pie_cutoff=10 - size = [all_actions_per_context.size, pie_cutoff].min - - # explicitly copy contents of hash to avoid ending up with two arrays pointing to same hashes - actions_per_context = Array.new(size){|i| { - 'name' => all_actions_per_context[i]['name'], - 'total' => all_actions_per_context[i]['total'], - 'id' => all_actions_per_context[i]['id'] - } } - - if all_actions_per_context.size > pie_cutoff - actions_per_context[-1]['name']=I18n.t('stats.other_actions_label') - actions_per_context[-1]['id']=-1 - size.upto(all_actions_per_context.size-1){ |i| actions_per_context[-1]['total']+=(all_actions_per_context[i]['total']) } + def values + @values ||= Array.new(slices) do |i| + chart_totals[i]['total'] * 100 / sum end - - @values = Array.new(size){|i| actions_per_context[i]['total']*100/sum } - @labels = Array.new(size){|i| actions_per_context[i]['name'].truncate(15, :omission => '...') } - @ids = Array.new(size){|i| actions_per_context[i]['id']} end + def labels + @labels ||= Array.new(slices) do |i| + chart_totals[i]['name'].truncate(15, :omission => '...') + end + end + + def ids + @ids ||= Array.new(slices) do |i| + chart_totals[i]['id'] + end + end + + private + + def sum + @sum ||= totals.inject(0) do |sum, amount| + sum + amount + end + end + + def pie_cutoff + 10 + end + + def slices + @slices ||= [all_totals.size, pie_cutoff].min + end + + def subtotal(from, to) + totals[from..to].inject(0) do |sum, amount| + sum + amount + end + end + + def chart_totals + unless @chart_totals + @chart_totals = first_n_totals(10) + if all_totals.size > pie_cutoff + @chart_totals[-1] = other + end + end + @chart_totals + end + + def first_n_totals(n) + # create a duplicate so that we don't accidentally + # overwrite the original array + Array.new(slices) do |i| + { + 'name' => all_totals[i]['name'], + 'total' => all_totals[i]['total'], + 'id' => all_totals[i]['id'] + } + end + end + + def other + { + 'name' => I18n.t('stats.other_actions_label'), + 'id' => -1, + 'total' => subtotal(slices-1, all_totals.size-1) + } + end + + def totals + @totals ||= all_totals.map { |item| item['total'] } + end end end diff --git a/test/unit/pie_chart_data_test.rb b/test/unit/pie_chart_data_test.rb index bf5a787d..d5933d12 100644 --- a/test/unit/pie_chart_data_test.rb +++ b/test/unit/pie_chart_data_test.rb @@ -12,7 +12,6 @@ class Stats::PieChartDataTest < Test::Unit::TestCase def test_with_0_items data = Stats::PieChartData.new([], 'a chart', 50) - data.calculate assert_equal [], data.values assert_equal [], data.labels @@ -33,7 +32,6 @@ class Stats::PieChartDataTest < Test::Unit::TestCase ] data = Stats::PieChartData.new(items, 'a chart', 50) - data.calculate assert_equal [12, 4, 9, 15, 23, 20, 5, 1, 7], data.values assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine"], data.labels @@ -55,7 +53,6 @@ class Stats::PieChartDataTest < Test::Unit::TestCase ] data = Stats::PieChartData.new(items, 'a chart', 50) - data.calculate assert_equal [10, 3, 7, 12, 19, 16, 4, 0, 5, 18], data.values assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"], data.labels @@ -78,7 +75,6 @@ class Stats::PieChartDataTest < Test::Unit::TestCase ] data = Stats::PieChartData.new(items, 'a chart', 50) - data.calculate assert_equal [9, 3, 6, 11, 16, 14, 4, 0, 5, 27], data.values assert_equal ["one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "(other)"], data.labels From a4ae0c03bfc496c1c5599eacad36884e4285ce43 Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Mon, 18 Mar 2013 21:54:19 -0600 Subject: [PATCH 7/7] Fix failing tests --- app/models/stats/pie_chart_data.rb | 12 ++++----- test/functional/stats_controller_test.rb | 32 ++++++++++++++---------- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/app/models/stats/pie_chart_data.rb b/app/models/stats/pie_chart_data.rb index 9a5ded53..9111c9b5 100644 --- a/app/models/stats/pie_chart_data.rb +++ b/app/models/stats/pie_chart_data.rb @@ -26,14 +26,14 @@ module Stats end end - private - def sum - @sum ||= totals.inject(0) do |sum, amount| - sum + amount + @sum ||= totals.inject(0) do |sum, total| + sum + total end end + private + def pie_cutoff 10 end @@ -43,8 +43,8 @@ module Stats end def subtotal(from, to) - totals[from..to].inject(0) do |sum, amount| - sum + amount + totals[from..to].inject(0) do |sum, total| + sum + total end end diff --git a/test/functional/stats_controller_test.rb b/test/functional/stats_controller_test.rb index 6912e39e..87077ded 100644 --- a/test/functional/stats_controller_test.rb +++ b/test/functional/stats_controller_test.rb @@ -26,12 +26,19 @@ class StatsControllerTest < ActionController::TestCase actions_day_of_week_30days_data actions_time_of_day_all_data actions_time_of_day_30days_data + }.each do |action| + get action + assert_response :success + assert_template "stats/"+action + end + + %w{ context_total_actions_data context_running_actions_data }.each do |action| get action assert_response :success - assert_template "stats/"+action + assert_template "stats/pie_chart_data" end end @@ -262,8 +269,8 @@ class StatsControllerTest < ActionController::TestCase get :context_total_actions_data assert_response :success - assert_equal 9, assigns['sum'], "Nine todos in 1 context" - assert_equal 1, assigns['actions_per_context'].size + assert_equal 9, assigns['data'].sum, "Nine todos in 1 context" + assert_equal 1, assigns['data'].values.size # Given 10 more todos in 10 different contexts 1.upto(10) do |i| @@ -275,10 +282,10 @@ class StatsControllerTest < ActionController::TestCase get :context_total_actions_data assert_response :success - assert_equal 19, assigns['sum'], "added 10 todos" - assert_equal 10, assigns['actions_per_context'].size, "pie slices limited to max 10" - assert_equal 2, assigns['actions_per_context'][9]['total'], "pie slices limited to max 10; last pie contains sum of rest" - assert_equal "(others)", assigns['actions_per_context'][9]['name'], "pie slices limited to max 10; last slice contains label for others" + assert_equal 19, assigns['data'].sum, "added 10 todos" + assert_equal 10, assigns['data'].values.size, "pie slices limited to max 10" + assert_equal 10, assigns['data'].values[9], "pie slices limited to max 10; last pie contains sum of rest (in percentage)" + assert_equal "(others)", assigns['data'].labels[9], "pie slices limited to max 10; last slice contains label for others" end def test_context_running_actions_data @@ -292,8 +299,8 @@ class StatsControllerTest < ActionController::TestCase get :context_running_actions_data assert_response :success - assert_equal 4, assigns['sum'], "Four running todos in 1 context" - assert_equal 1, assigns['actions_per_context'].size + assert_equal 4, assigns['data'].sum, "Four todos in 1 context" + assert_equal 1, assigns['data'].values.size # Given 10 more todos in 10 different contexts 1.upto(10) do |i| @@ -305,10 +312,9 @@ class StatsControllerTest < ActionController::TestCase get :context_running_actions_data assert_response :success - assert_equal 14, assigns['sum'], "added 10 todos" - assert_equal 10, assigns['actions_per_context'].size, "pie slices limited to max 10" - assert_equal 2, assigns['actions_per_context'][9]['total'], "pie slices limited to max 10; last pie contains sum of rest" - assert_equal "(others)", assigns['actions_per_context'][9]['name'], "pie slices limited to max 10; last slice contains label for others" + assert_equal 10, assigns['data'].values.size, "pie slices limited to max 10" + assert_equal 14, assigns['data'].values[9], "pie slices limited to max 10; last pie contains sum of rest (in percentage)" + assert_equal "(others)", assigns['data'].labels[9], "pie slices limited to max 10; last slice contains label for others" end def test_actions_day_of_week_all_data