From 8b1f0a34a08fcfab724357e68d22a455ec51ef99 Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sat, 2 Mar 2013 17:11:19 -0700 Subject: [PATCH 1/6] Group project-related stats into a class. Reduce number of instance variables available to the views. Replace raw SQL with AR-type query. --- app/controllers/stats_controller.rb | 18 +----------------- app/models/stats/projects.rb | 22 ++++++++++++++++++++++ app/views/stats/_projects.html.erb | 6 +++--- app/views/stats/index.html.erb | 2 +- 4 files changed, 27 insertions(+), 21 deletions(-) create mode 100644 app/models/stats/projects.rb diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index b5fbf87a..b44182a9 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -14,9 +14,9 @@ class StatsController < ApplicationController @unique_tags_count = tag_ids.uniq.size @hidden_contexts = current_user.contexts.hidden @actions = Stats::Actions.new(current_user) + @projects = Stats::Projects.new(current_user) get_stats_contexts - get_stats_projects get_stats_tags end @@ -399,22 +399,6 @@ class StatsController < ApplicationController end end - def get_stats_projects - @projects_and_actions = Stats::TopProjectsQuery.new(current_user).result - @projects_and_actions_last30days = Stats::TopProjectsQuery.new(current_user, @cut_off_month).result - - # get the first 10 projects and their running time (creation date versus - # now()) - @projects_and_runtime = current_user.projects.find_by_sql( - "SELECT id, name, created_at "+ - "FROM projects "+ - "WHERE state='active' "+ - "AND user_id=#{current_user.id} "+ - "ORDER BY created_at ASC "+ - "LIMIT 10" - ) - end - def get_stats_tags tags = Stats::TagCloudQuery.new(current_user).result @tag_cloud = Stats::TagCloud.new(tags) diff --git a/app/models/stats/projects.rb b/app/models/stats/projects.rb new file mode 100644 index 00000000..45b437f4 --- /dev/null +++ b/app/models/stats/projects.rb @@ -0,0 +1,22 @@ +module Stats + class Projects + + attr_reader :user + def initialize(user) + @user = user + end + + def runtime + @runtime ||= user.projects.active.order('created_at ASC').limit(10) + end + + def actions + @actions ||= Stats::TopProjectsQuery.new(user).result + end + + def actions_last30days + @actions_last30days ||= Stats::TopProjectsQuery.new(user, 1.month.ago.beginning_of_day).result + end + + end +end diff --git a/app/views/stats/_projects.html.erb b/app/views/stats/_projects.html.erb index 5b2d6de3..918fc128 100755 --- a/app/views/stats/_projects.html.erb +++ b/app/views/stats/_projects.html.erb @@ -1,6 +1,6 @@ -<%= render :partial => 'projects_list', :locals => {:projects => @projects_and_actions, :key => 'projects', :n => :count} -%> +<%= render :partial => 'projects_list', :locals => {:projects => projects.actions, :key => 'projects', :n => :count} -%> -<%= render :partial => 'projects_list', :locals => {:projects => @projects_and_actions_last30days, :key => 'projects_30days', :n => :count} -%> +<%= render :partial => 'projects_list', :locals => {:projects => projects.actions_last30days, :key => 'projects_30days', :n => :count} -%> -<%= render :partial => 'projects_list', :locals => {:projects => @projects_and_runtime, :key => 'longrunning', :n => :age_in_days} -%> +<%= render :partial => 'projects_list', :locals => {:projects => projects.runtime, :key => 'longrunning', :n => :age_in_days} -%> diff --git a/app/views/stats/index.html.erb b/app/views/stats/index.html.erb index cd3704cc..f40f1dd3 100755 --- a/app/views/stats/index.html.erb +++ b/app/views/stats/index.html.erb @@ -12,7 +12,7 @@ <%= render :partial => 'contexts' -%>

<%= t('stats.projects') %>

- <%= render :partial => 'projects' -%> + <%= render :partial => 'projects', :locals => {:projects => @projects} -%>

<%= t('stats.tags') %>

<%= render :partial => 'tags', :locals => {:tag_cloud => @tag_cloud, :key => ''} -%> From 6ccb9a81fb52dd20d69c8c3d9ad135bb5d38b7a7 Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sat, 2 Mar 2013 08:11:15 -0500 Subject: [PATCH 2/6] Group context-related pieces in a class --- app/controllers/stats_controller.rb | 14 +------------ app/models/stats/contexts.rb | 27 ++++++++++++++++++++++++++ app/models/stats/top_contexts_query.rb | 4 ++-- app/views/stats/_contexts.html.erb | 7 ++++--- app/views/stats/index.html.erb | 2 +- 5 files changed, 35 insertions(+), 19 deletions(-) create mode 100644 app/models/stats/contexts.rb diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index b44182a9..c822747a 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -15,8 +15,8 @@ class StatsController < ApplicationController @hidden_contexts = current_user.contexts.hidden @actions = Stats::Actions.new(current_user) @projects = Stats::Projects.new(current_user) + @contexts = Stats::Contexts.new(current_user) - get_stats_contexts get_stats_tags end @@ -387,18 +387,6 @@ class StatsController < ApplicationController @cut_off_3months = 3.months.ago.beginning_of_day end - def get_stats_contexts - @actions_per_context = Stats::TopContextsQuery.new(current_user, :limit => 5).result - @running_actions_per_context = Stats::TopContextsQuery.new(current_user, :limit => 5, :running => true).result - - @context_charts = %w{ - context_total_actions_data - context_running_actions_data - }.map do |action| - Stats::Chart.new(action, :height => 325) - end - end - def get_stats_tags tags = Stats::TagCloudQuery.new(current_user).result @tag_cloud = Stats::TagCloud.new(tags) diff --git a/app/models/stats/contexts.rb b/app/models/stats/contexts.rb new file mode 100644 index 00000000..1aa04e38 --- /dev/null +++ b/app/models/stats/contexts.rb @@ -0,0 +1,27 @@ +module Stats + class Contexts + + attr_reader :user + def initialize(user) + @user = user + end + + def actions + @actions ||= Stats::TopContextsQuery.new(user, :limit => 5).result + end + + def running_actions + @running_actions ||= Stats::TopContextsQuery.new(user, :limit => 5, :running => true).result + end + + def charts + @charts = %w{ + context_total_actions_data + context_running_actions_data + }.map do |action| + Stats::Chart.new(action, :height => 325) + end + end + + end +end diff --git a/app/models/stats/top_contexts_query.rb b/app/models/stats/top_contexts_query.rb index 69a5e9e9..d47abfb1 100644 --- a/app/models/stats/top_contexts_query.rb +++ b/app/models/stats/top_contexts_query.rb @@ -1,5 +1,5 @@ -# Get action count for the top 5 contexts -# If initialized with :running, then only active +# Get action count for the top n contexts (default: all) +# If initialized with :running => true, then only active # and visible contexts will be included. module Stats class TopContextsQuery diff --git a/app/views/stats/_contexts.html.erb b/app/views/stats/_contexts.html.erb index 1bd8e419..2f9502e1 100755 --- a/app/views/stats/_contexts.html.erb +++ b/app/views/stats/_contexts.html.erb @@ -1,9 +1,10 @@ -<% @context_charts.each do |chart| %><%= +<% contexts.charts.each do |chart| %><%= render :partial => 'chart', :locals => {:chart => chart} -%><% end %>
-<%= render :partial => 'contexts_list', :locals => {:contexts => @actions_per_context, :key => 'contexts'} -%> +<%= render :partial => 'contexts_list', :locals => {:contexts => contexts.actions, :key => 'contexts'} -%> + +<%= render :partial => 'contexts_list', :locals => {:contexts => contexts.running_actions, :key => 'visible_contexts_with_incomplete_actions'} -%> -<%= render :partial => 'contexts_list', :locals => {:contexts => @running_actions_per_context, :key => 'visible_contexts_with_incomplete_actions'} -%> diff --git a/app/views/stats/index.html.erb b/app/views/stats/index.html.erb index f40f1dd3..65f7392a 100755 --- a/app/views/stats/index.html.erb +++ b/app/views/stats/index.html.erb @@ -9,7 +9,7 @@ <%= render :partial => 'actions', :locals => {:actions => @actions} -%>

<%= t('stats.contexts') %>

- <%= render :partial => 'contexts' -%> + <%= render :partial => 'contexts', :locals => {:contexts => @contexts} -%>

<%= t('stats.projects') %>

<%= render :partial => 'projects', :locals => {:projects => @projects} -%> From 615a9e46c9ef00a3bcc5443a8f0de729dc64f01c Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sat, 2 Mar 2013 17:22:15 -0700 Subject: [PATCH 3/6] Encapsulate counts and totals into a class. Move queries out of the view and into the model layer. --- app/controllers/stats_controller.rb | 6 +- app/models/stats/totals.rb | 88 +++++++++++++++++++++++++++++ app/views/stats/_totals.html.erb | 32 +++++------ app/views/stats/index.html.erb | 2 +- 4 files changed, 106 insertions(+), 22 deletions(-) create mode 100644 app/models/stats/totals.rb diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index c822747a..ec435cb8 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -7,13 +7,9 @@ class StatsController < ApplicationController def index @page_title = t('stats.index_title') - - @first_action = current_user.todos.reorder("created_at ASC").first - tag_ids = Stats::UserTagsQuery.new(current_user).result.map(&:id) - @tags_count = tag_ids.size - @unique_tags_count = tag_ids.uniq.size @hidden_contexts = current_user.contexts.hidden @actions = Stats::Actions.new(current_user) + @totals = Stats::Totals.new(current_user) @projects = Stats::Projects.new(current_user) @contexts = Stats::Contexts.new(current_user) diff --git a/app/models/stats/totals.rb b/app/models/stats/totals.rb new file mode 100644 index 00000000..a80f8e71 --- /dev/null +++ b/app/models/stats/totals.rb @@ -0,0 +1,88 @@ +module Stats + class Totals + + attr_reader :user + def initialize(user) + @user = user + end + + def empty? + actions.empty? + end + + def tags + @tags ||= tag_ids.size + end + + def unique_tags + @unique_tags ||= tag_ids.uniq.size + end + + def first_action_at + first_action.created_at if first_action + end + + def projects + user.projects.count + end + + def active_projects + user.projects.active.count + end + + def hidden_projects + user.projects.hidden.count + end + + def completed_projects + user.projects.completed.count + end + + def contexts + user.contexts.count + end + + def visible_contexts + user.contexts.active.count + end + + def hidden_contexts + user.contexts.hidden.count + end + + def all_actions + actions.count + end + + def completed_actions + actions.completed.count + end + + def incomplete_actions + actions.not_completed.count + end + + def deferred_actions + actions.deferred.count + end + + def blocked_actions + actions.blocked.count + end + + private + + def actions + user.todos + end + + def first_action + @first_action ||= user.todos.reorder("created_at ASC").first + end + + def tag_ids + @tag_ids ||= Stats::UserTagsQuery.new(user).result.map(&:id) + end + + end +end diff --git a/app/views/stats/_totals.html.erb b/app/views/stats/_totals.html.erb index 4d2dac0c..7962ad49 100755 --- a/app/views/stats/_totals.html.erb +++ b/app/views/stats/_totals.html.erb @@ -1,23 +1,23 @@ -

<%= t('stats.totals_project_count', :count => current_user.projects.count) %> - <%= t('stats.totals_active_project_count', :count => current_user.projects.active.count) %>, - <%= t('stats.totals_hidden_project_count', :count => current_user.projects.hidden.count) %> - <%= t('stats.totals_completed_project_count', :count => current_user.projects.completed.count) %>

+

<%= t('stats.totals_project_count', :count => totals.projects) %> + <%= t('stats.totals_active_project_count', :count => totals.active_projects) %>, + <%= t('stats.totals_hidden_project_count', :count => totals.hidden_projects) %> + <%= t('stats.totals_completed_project_count', :count => totals.completed_projects) %>

-

<%= t('stats.totals_context_count', :count => current_user.contexts.count ) %> -<%= t('stats.totals_visible_context_count', :count => current_user.contexts.active.count) %> -<%= t('stats.totals_hidden_context_count', :count => current_user.contexts.hidden.count) %> +

<%= t('stats.totals_context_count', :count => totals.contexts) %> +<%= t('stats.totals_visible_context_count', :count => totals.visible_contexts) %> +<%= t('stats.totals_hidden_context_count', :count => totals.hidden_contexts) %> -<% unless current_user.todos.empty? -%> -

<%= t('stats.totals_first_action', :date => format_date(@first_action.created_at)) %> -<%= t('stats.totals_action_count', :count => current_user.todos.count) %>, -<%= t('stats.totals_actions_completed', :count => current_user.todos.completed.count) %> +<% unless totals.empty? -%> +

<%= t('stats.totals_first_action', :date => format_date(totals.first_action_at)) %> +<%= t('stats.totals_action_count', :count => totals.all_actions) %>, +<%= t('stats.totals_actions_completed', :count => totals.completed_actions) %> -

<%= t('stats.totals_incomplete_actions', :count => current_user.todos.not_completed.count) %> -<%= t('stats.totals_deferred_actions', :count => current_user.todos.deferred.count) %> -<%= t('stats.totals_blocked_actions', :count => current_user.todos.blocked.count) %> +

<%= t('stats.totals_incomplete_actions', :count => totals.incomplete_actions) %> +<%= t('stats.totals_deferred_actions', :count => totals.deferred_actions) %> +<%= t('stats.totals_blocked_actions', :count => totals.blocked_actions) %>

-

<%= t('stats.totals_tag_count', :count => @tags_count) %> - <%= t('stats.totals_unique_tags', :count => @unique_tags_count) %> +

<%= t('stats.totals_tag_count', :count => totals.tags) %> + <%= t('stats.totals_unique_tags', :count => totals.unique_tags) %> <% end -%> diff --git a/app/views/stats/index.html.erb b/app/views/stats/index.html.erb index 65f7392a..1dff44c2 100755 --- a/app/views/stats/index.html.erb +++ b/app/views/stats/index.html.erb @@ -1,7 +1,7 @@

<%= t('stats.totals') %>

- <%= render :partial => 'totals' -%> + <%= render :partial => 'totals', :locals => {:totals => @totals} -%> <% unless current_user.todos.empty? -%> From ac4a483b1805e6777ea4e9e38c54538d7088d851 Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sat, 2 Mar 2013 17:24:05 -0700 Subject: [PATCH 4/6] Inline helper method --- app/controllers/stats_controller.rb | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index ec435cb8..1fc22166 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -12,8 +12,10 @@ class StatsController < ApplicationController @totals = Stats::Totals.new(current_user) @projects = Stats::Projects.new(current_user) @contexts = Stats::Contexts.new(current_user) - - get_stats_tags + tags = Stats::TagCloudQuery.new(current_user).result + @tag_cloud = Stats::TagCloud.new(tags) + tags = Stats::TagCloudQuery.new(current_user, @cut_off_3months).result + @tag_cloud_90days = Stats::TagCloud.new(tags) end def actions_done_last12months_data @@ -383,14 +385,6 @@ class StatsController < ApplicationController @cut_off_3months = 3.months.ago.beginning_of_day end - def get_stats_tags - tags = Stats::TagCloudQuery.new(current_user).result - @tag_cloud = Stats::TagCloud.new(tags) - - tags = Stats::TagCloudQuery.new(current_user, @cut_off_3months).result - @tag_cloud_90days = Stats::TagCloud.new(tags) - end - def get_ids_from (actions, week_from, week_to, at_end) selected_todo_ids = [] From 6df3534baf5df6bf2f701e52501638421db3a327 Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sat, 2 Mar 2013 17:27:38 -0700 Subject: [PATCH 5/6] Inline last pieces into StatsController#index We can now skip the before filter that sets instance variables. --- app/controllers/stats_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 1fc22166..5180ef63 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -3,7 +3,7 @@ class StatsController < ApplicationController SECONDS_PER_DAY = 86400; helper :todos, :projects, :recurring_todos - append_before_filter :init + append_before_filter :init, :except => :index def index @page_title = t('stats.index_title') @@ -14,7 +14,8 @@ class StatsController < ApplicationController @contexts = Stats::Contexts.new(current_user) tags = Stats::TagCloudQuery.new(current_user).result @tag_cloud = Stats::TagCloud.new(tags) - tags = Stats::TagCloudQuery.new(current_user, @cut_off_3months).result + cutoff = 3.months.ago.beginning_of_day + tags = Stats::TagCloudQuery.new(current_user, cutoff).result @tag_cloud_90days = Stats::TagCloud.new(tags) end @@ -382,7 +383,6 @@ class StatsController < ApplicationController @cut_off_year = 12.months.ago.beginning_of_day @cut_off_year_plus3 = 15.months.ago.beginning_of_day @cut_off_month = 1.month.ago.beginning_of_day - @cut_off_3months = 3.months.ago.beginning_of_day end def get_ids_from (actions, week_from, week_to, at_end) From d5a555fbac93b095bd5fcc605da8c76837d853f6 Mon Sep 17 00:00:00 2001 From: Katrina Owen Date: Sat, 2 Mar 2013 17:35:19 -0700 Subject: [PATCH 6/6] Encapsulate dependencies of stats index page This may be a bit extreme. It's modeled after the ideal "rails way". In the controller, we now know the name of a single resource. It doesn't happen to be backed by a database table, but it does know all about the task of collecting stats, leaving the controller concerned with just munging params and rendering stuff. I called the resource `IndexPage`, to avoid the temptation of trying to reuse it, which can get pretty messy. Later, if a better abstraction appears, it should be fairly painless to alter. --- app/controllers/stats_controller.rb | 10 +------ app/models/stats/index_page.rb | 43 +++++++++++++++++++++++++++++ app/views/stats/index.html.erb | 12 ++++---- 3 files changed, 50 insertions(+), 15 deletions(-) create mode 100644 app/models/stats/index_page.rb diff --git a/app/controllers/stats_controller.rb b/app/controllers/stats_controller.rb index 5180ef63..15ded4a9 100644 --- a/app/controllers/stats_controller.rb +++ b/app/controllers/stats_controller.rb @@ -8,15 +8,7 @@ class StatsController < ApplicationController def index @page_title = t('stats.index_title') @hidden_contexts = current_user.contexts.hidden - @actions = Stats::Actions.new(current_user) - @totals = Stats::Totals.new(current_user) - @projects = Stats::Projects.new(current_user) - @contexts = Stats::Contexts.new(current_user) - tags = Stats::TagCloudQuery.new(current_user).result - @tag_cloud = Stats::TagCloud.new(tags) - cutoff = 3.months.ago.beginning_of_day - tags = Stats::TagCloudQuery.new(current_user, cutoff).result - @tag_cloud_90days = Stats::TagCloud.new(tags) + @stats = Stats::IndexPage.new(current_user) end def actions_done_last12months_data diff --git a/app/models/stats/index_page.rb b/app/models/stats/index_page.rb new file mode 100644 index 00000000..0fabe7b1 --- /dev/null +++ b/app/models/stats/index_page.rb @@ -0,0 +1,43 @@ +module Stats + class IndexPage + + attr_reader :user + def initialize(user) + @user = user + end + + def actions + @actions ||= Stats::Actions.new(user) + end + + def totals + @totals ||= Stats::Totals.new(user) + end + + def projects + @projects ||= Stats::Projects.new(user) + end + + def contexts + @contexts ||= Stats::Contexts.new(user) + end + + def tag_cloud + unless @tag_cloud + tags = Stats::TagCloudQuery.new(user).result + @tag_cloud = Stats::TagCloud.new(tags) + end + @tag_cloud + end + + def tag_cloud_90days + unless @tag_cloud_90days + cutoff = 3.months.ago.beginning_of_day + tags = Stats::TagCloudQuery.new(user, cutoff).result + @tag_cloud_90days = Stats::TagCloud.new(tags) + end + @tag_cloud_90days + end + + end +end diff --git a/app/views/stats/index.html.erb b/app/views/stats/index.html.erb index 1dff44c2..44c4f458 100755 --- a/app/views/stats/index.html.erb +++ b/app/views/stats/index.html.erb @@ -1,22 +1,22 @@

<%= t('stats.totals') %>

- <%= render :partial => 'totals', :locals => {:totals => @totals} -%> + <%= render :partial => 'totals', :locals => {:totals => @stats.totals} -%> <% unless current_user.todos.empty? -%>

<%= t('stats.actions') %>

- <%= render :partial => 'actions', :locals => {:actions => @actions} -%> + <%= render :partial => 'actions', :locals => {:actions => @stats.actions} -%>

<%= t('stats.contexts') %>

- <%= render :partial => 'contexts', :locals => {:contexts => @contexts} -%> + <%= render :partial => 'contexts', :locals => {:contexts => @stats.contexts} -%>

<%= t('stats.projects') %>

- <%= render :partial => 'projects', :locals => {:projects => @projects} -%> + <%= render :partial => 'projects', :locals => {:projects => @stats.projects} -%>

<%= t('stats.tags') %>

- <%= render :partial => 'tags', :locals => {:tag_cloud => @tag_cloud, :key => ''} -%> - <%= render :partial => 'tags', :locals => {:tag_cloud => @tag_cloud_90days, :key => '_90days'} -%> + <%= render :partial => 'tags', :locals => {:tag_cloud => @stats.tag_cloud, :key => ''} -%> + <%= render :partial => 'tags', :locals => {:tag_cloud => @stats.tag_cloud_90days, :key => '_90days'} -%> <% else -%>