From 9de975f8eb4ca3511e1dbd1da0812131920a09c2 Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Sun, 7 Apr 2019 19:59:01 -0500 Subject: [PATCH 1/7] Extract a query object for not done todos --- app/models/todos/undone_todos_query.rb | 44 ++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 app/models/todos/undone_todos_query.rb diff --git a/app/models/todos/undone_todos_query.rb b/app/models/todos/undone_todos_query.rb new file mode 100644 index 00000000..0c738d27 --- /dev/null +++ b/app/models/todos/undone_todos_query.rb @@ -0,0 +1,44 @@ +module Todos + class UndoneTodosQuery + attr_reader :current_user + def initialize(current_user) + @current_user = current_user + end + + def query(params) + if params[:done] + not_done_todos = current_user.todos.completed.completed_after(Time.zone.now - params[:done].to_i.days) + else + not_done_todos = current_user.todos.active.not_hidden + end + + not_done_todos = not_done_todos. + reorder(Arel.sql("todos.due IS NULL, todos.due ASC, todos.created_at ASC")). + includes(Todo::DEFAULT_INCLUDES) + + not_done_todos = not_done_todos.limit(sanitize(params[:limit])) if params[:limit] + + if params[:due] + due_within_when = Time.zone.now + params['due'].to_i.days + not_done_todos = not_done_todos.where('todos.due <= ?', due_within_when) + end + + if params[:tag] + tag = Tag.where(:name => params['tag']).first + not_done_todos = not_done_todos.where('taggings.tag_id = ?', tag.id) + end + + if params[:context_id] + context = current_user.contexts.find(params[:context_id]) + not_done_todos = not_done_todos.where('context_id' => context.id) + end + + if params[:project_id] + project = current_user.projects.find(params[:project_id]) + not_done_todos = not_done_todos.where('project_id' => project) + end + + return not_done_todos + end + end +end From ec1a4d78bad8adfa2dd155a550e0067bd2f7e0ba Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Sun, 7 Apr 2019 20:01:22 -0500 Subject: [PATCH 2/7] Temporarily add the sanitize helper This is so we can keep the tests passing by fixing a NoMethodError when attempting to run both the new query object and the existing code at the same time. --- app/models/todos/undone_todos_query.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/todos/undone_todos_query.rb b/app/models/todos/undone_todos_query.rb index 0c738d27..e39274a7 100644 --- a/app/models/todos/undone_todos_query.rb +++ b/app/models/todos/undone_todos_query.rb @@ -1,5 +1,7 @@ module Todos class UndoneTodosQuery + include ActionView::Helpers::SanitizeHelper + attr_reader :current_user def initialize(current_user) @current_user = current_user From eb7c34e973d5ad20c62b920d46a100d508a2de33 Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Sun, 7 Apr 2019 20:04:28 -0500 Subject: [PATCH 3/7] Run both the new query and the old query Throw away the results of the new query for now. Now that we know it will run without raising exceptions, we can replace the old query with the new query. --- app/controllers/todos_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index eb7be53d..99e6b741 100644 --- a/app/controllers/todos_controller.rb +++ b/app/controllers/todos_controller.rb @@ -1316,6 +1316,7 @@ end end def get_not_done_todos + Todos::UndoneTodosQuery.new(current_user).query(params) if params[:done] not_done_todos = current_user.todos.completed.completed_after(Time.zone.now - params[:done].to_i.days) else From df091c7ec5c85dc65ee80b7b92808bd59dd75f73 Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Sun, 7 Apr 2019 20:08:11 -0500 Subject: [PATCH 4/7] Use the results of the new query object This confirms that we've successfully extracted the code and now we can start removing the old code. --- app/controllers/todos_controller.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index 99e6b741..a004c943 100644 --- a/app/controllers/todos_controller.rb +++ b/app/controllers/todos_controller.rb @@ -1316,7 +1316,6 @@ end end def get_not_done_todos - Todos::UndoneTodosQuery.new(current_user).query(params) if params[:done] not_done_todos = current_user.todos.completed.completed_after(Time.zone.now - params[:done].to_i.days) else @@ -1349,7 +1348,7 @@ end not_done_todos = not_done_todos.where('project_id' => project) end - return not_done_todos + return Todos::UndoneTodosQuery.new(current_user).query(params) end def onsite_redirect_to(destination) From fc17a03bc0bd193bfd6817b36a01fae12629ee74 Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Thu, 11 Apr 2019 09:53:53 -0500 Subject: [PATCH 5/7] Add tests for the new object and fix a bug Each of the individual query chunks has their own test, in addition to a test for the full combination of parameters that could influence a query. There is also a bugfix for the tag query in here, since I want, as much as possible, to have passing tests on every commit. --- app/models/todos/undone_todos_query.rb | 2 +- test/fixtures/projects.yml | 10 ++ test/fixtures/taggings.yml | 14 ++- test/fixtures/todos.yml | 33 +++++++ test/models/todos/undone_todos_query_test.rb | 97 ++++++++++++++++++++ 5 files changed, 154 insertions(+), 2 deletions(-) create mode 100644 test/models/todos/undone_todos_query_test.rb diff --git a/app/models/todos/undone_todos_query.rb b/app/models/todos/undone_todos_query.rb index e39274a7..e9bc1cc2 100644 --- a/app/models/todos/undone_todos_query.rb +++ b/app/models/todos/undone_todos_query.rb @@ -27,7 +27,7 @@ module Todos if params[:tag] tag = Tag.where(:name => params['tag']).first - not_done_todos = not_done_todos.where('taggings.tag_id = ?', tag.id) + not_done_todos = not_done_todos.joins(:taggings).where('taggings.tag_id = ?', tag.id) end if params[:context_id] diff --git a/test/fixtures/projects.yml b/test/fixtures/projects.yml index e796feb8..c69f6230 100644 --- a/test/fixtures/projects.yml +++ b/test/fixtures/projects.yml @@ -56,3 +56,13 @@ attendrailsconf: user_id: 2 created_at: <%= today %> updated_at: <%= today %> + +attendgophercon: + id: 5 + name: Attend Gophercon + description: 'Because those little gopher drawing are cute' + position: 2 + state: 'active' + user_id: 2 + created_at: <%= today %> + updated_at: <%= today %> diff --git a/test/fixtures/taggings.yml b/test/fixtures/taggings.yml index 880ba6b4..431f9b84 100644 --- a/test/fixtures/taggings.yml +++ b/test/fixtures/taggings.yml @@ -22,4 +22,16 @@ foo2: id: 4 tag_id: 1 taggable_id: 3 # Buy milk - completed - taggable_type: Todo \ No newline at end of file + taggable_type: Todo + +bar1: + id: 5 + tag_id: 2 + taggable_id: 16 + taggable_type: Todo + +bar2: + tag_id: 2 + taggable_id: 20 + taggable_type: Todo + diff --git a/test/fixtures/todos.yml b/test/fixtures/todos.yml index 5a889c27..8d810c68 100644 --- a/test/fixtures/todos.yml +++ b/test/fixtures/todos.yml @@ -4,10 +4,17 @@ # rails does automatically in models or controllers! Convert to utc manually! <% +def yesterday + Time.zone.now.utc.beginning_of_day - 1.day +end def today Time.zone.now.utc.beginning_of_day.to_s(:db) end +def tomorrow + (Time.zone.now.utc.beginning_of_day + 1.day).to_s(:db) +end + def next_week 1.week.from_now.utc.beginning_of_day.to_s(:db) end @@ -255,3 +262,29 @@ email_broker: description: Ask about better stocks notes: ~ state: pending + +package_delivered: + id: 20 + context_id: 11 + project_id: 5 + description: Package delivery date + notes: ~ + state: active + created_at: <%= two_weeks_ago %> + due: <%= today %> + completed_at: ~ + show_from: ~ + user_id: 2 + +assemble_furniture: + id: 21 + context_id: 11 + project_id: 5 + description: Put together the furniture we bought + notes: ~ + state: completed + created_at: <%= two_weeks_ago %> + due: <%= today %> + completed_at: <%= yesterday %> + show_from: ~ + user_id: 2 diff --git a/test/models/todos/undone_todos_query_test.rb b/test/models/todos/undone_todos_query_test.rb new file mode 100644 index 00000000..15af079c --- /dev/null +++ b/test/models/todos/undone_todos_query_test.rb @@ -0,0 +1,97 @@ +require 'test_helper' + +module Todos + class UndoneTodosQueryTest < ActiveSupport::TestCase + def test_requires_a_user + assert_raises(ArgumentError) { UndoneTodosQuery.new } + end + + def test_default_query_is_all_active_not_hidden_todos + user = users(:other_user) + undone_todos = UndoneTodosQuery.new(user).query({}) + expected = [todos(:package_delivered), + todos(:buy_tix), + todos(:pal_confirmation)] + assert_equal expected, undone_todos.to_a + end + + def test_filtering_by_done + user = users(:other_user) + # This gets everything done from a week ago until now + undone_todos = UndoneTodosQuery.new(user).query(done: '7') + expected = [todos(:assemble_furniture)] + assert_equal expected, undone_todos.to_a + end + + def test_limiting_results + user = users(:other_user) + undone_todos = UndoneTodosQuery.new(user).query(limit: '1') + expected = [todos(:package_delivered)] + assert_equal expected, undone_todos.to_a + end + + def test_filtering_by_due_date + user = users(:other_user) + # FIXME normalize HashWithIndifferentHash usage + # Only gets todos that are due today or are past their due date. + undone_todos = UndoneTodosQuery.new(user).query(due: '0', 'due' => '0') + expected = [todos(:package_delivered)] + assert_equal expected, undone_todos.to_a + end + + def test_filtering_by_tag + user = users(:other_user) + # FIXME normalize HashWithIndifferentHash usage + undone_todos = UndoneTodosQuery.new(user).query(tag: 'bar', "tag" => "bar") + expected = [todos(:package_delivered), + todos(:buy_tix)] + assert_equal expected, undone_todos.to_a + end + + def test_filtering_by_context + user = users(:other_user) + # FIXME normalize HashWithIndifferentHash usage + undone_todos = UndoneTodosQuery.new(user).query(context_id: '11', 'context_id' => '11') + expected = [todos(:package_delivered), + todos(:pal_confirmation)] + assert_equal expected, undone_todos.to_a + end + + def test_using_a_non_existant_context_raises_an_exception + user = users(:other_user) + # FIXME normalize HashWithIndifferentHash usage + assert_raises(ActiveRecord::RecordNotFound) do + undone_todos = UndoneTodosQuery.new(user).query(context_id: '110', 'context_id' => '110') + end + end + + def test_filtering_by_project + user = users(:other_user) + # FIXME normalize HashWithIndifferentHash usage + undone_todos = UndoneTodosQuery.new(user).query(project_id: '5', 'project_id' => '5') + expected = [todos(:package_delivered)] + assert_equal expected, undone_todos.to_a + end + + def test_using_a_non_existant_project_raises_an_exception + user = users(:other_user) + # FIXME normalize HashWithIndifferentHash usage + assert_raises(ActiveRecord::RecordNotFound) do + undone_todos = UndoneTodosQuery.new(user).query(project_id: '110', 'project_id' => '110') + end + end + + def test_combination_of_all_params + user = users(:other_user) + # FIXME normalize HashWithIndifferentHash usage + undone_todos = UndoneTodosQuery.new(user).query({ + limit: "1", + project_id: "5", "project_id" => "5", + context_id: "11", "context_id" => "11", + tag: "bar", "tag" => "bar", + due: "0", "due" => "0"}) + expected = [todos(:package_delivered)] + assert_equal expected, undone_todos.to_a + end + end +end From 63ac90ebb264ea4c0289e5f10ca87562903d7415 Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Thu, 11 Apr 2019 11:28:43 -0500 Subject: [PATCH 6/7] Convert to using symbols everywhere --- app/models/todos/undone_todos_query.rb | 4 +-- test/models/todos/undone_todos_query_test.rb | 27 ++++++++------------ 2 files changed, 12 insertions(+), 19 deletions(-) diff --git a/app/models/todos/undone_todos_query.rb b/app/models/todos/undone_todos_query.rb index e9bc1cc2..b4980870 100644 --- a/app/models/todos/undone_todos_query.rb +++ b/app/models/todos/undone_todos_query.rb @@ -21,12 +21,12 @@ module Todos not_done_todos = not_done_todos.limit(sanitize(params[:limit])) if params[:limit] if params[:due] - due_within_when = Time.zone.now + params['due'].to_i.days + due_within_when = Time.zone.now + params[:due].to_i.days not_done_todos = not_done_todos.where('todos.due <= ?', due_within_when) end if params[:tag] - tag = Tag.where(:name => params['tag']).first + tag = Tag.where(:name => params[:tag]).first not_done_todos = not_done_todos.joins(:taggings).where('taggings.tag_id = ?', tag.id) end diff --git a/test/models/todos/undone_todos_query_test.rb b/test/models/todos/undone_todos_query_test.rb index 15af079c..13515c06 100644 --- a/test/models/todos/undone_todos_query_test.rb +++ b/test/models/todos/undone_todos_query_test.rb @@ -32,17 +32,15 @@ module Todos def test_filtering_by_due_date user = users(:other_user) - # FIXME normalize HashWithIndifferentHash usage # Only gets todos that are due today or are past their due date. - undone_todos = UndoneTodosQuery.new(user).query(due: '0', 'due' => '0') + undone_todos = UndoneTodosQuery.new(user).query(due: '0') expected = [todos(:package_delivered)] assert_equal expected, undone_todos.to_a end def test_filtering_by_tag user = users(:other_user) - # FIXME normalize HashWithIndifferentHash usage - undone_todos = UndoneTodosQuery.new(user).query(tag: 'bar', "tag" => "bar") + undone_todos = UndoneTodosQuery.new(user).query(tag: 'bar') expected = [todos(:package_delivered), todos(:buy_tix)] assert_equal expected, undone_todos.to_a @@ -50,8 +48,7 @@ module Todos def test_filtering_by_context user = users(:other_user) - # FIXME normalize HashWithIndifferentHash usage - undone_todos = UndoneTodosQuery.new(user).query(context_id: '11', 'context_id' => '11') + undone_todos = UndoneTodosQuery.new(user).query(context_id: '11') expected = [todos(:package_delivered), todos(:pal_confirmation)] assert_equal expected, undone_todos.to_a @@ -59,37 +56,33 @@ module Todos def test_using_a_non_existant_context_raises_an_exception user = users(:other_user) - # FIXME normalize HashWithIndifferentHash usage assert_raises(ActiveRecord::RecordNotFound) do - undone_todos = UndoneTodosQuery.new(user).query(context_id: '110', 'context_id' => '110') + undone_todos = UndoneTodosQuery.new(user).query(context_id: '110') end end def test_filtering_by_project user = users(:other_user) - # FIXME normalize HashWithIndifferentHash usage - undone_todos = UndoneTodosQuery.new(user).query(project_id: '5', 'project_id' => '5') + undone_todos = UndoneTodosQuery.new(user).query(project_id: '5') expected = [todos(:package_delivered)] assert_equal expected, undone_todos.to_a end def test_using_a_non_existant_project_raises_an_exception user = users(:other_user) - # FIXME normalize HashWithIndifferentHash usage assert_raises(ActiveRecord::RecordNotFound) do - undone_todos = UndoneTodosQuery.new(user).query(project_id: '110', 'project_id' => '110') + undone_todos = UndoneTodosQuery.new(user).query(project_id: '110') end end def test_combination_of_all_params user = users(:other_user) - # FIXME normalize HashWithIndifferentHash usage undone_todos = UndoneTodosQuery.new(user).query({ limit: "1", - project_id: "5", "project_id" => "5", - context_id: "11", "context_id" => "11", - tag: "bar", "tag" => "bar", - due: "0", "due" => "0"}) + project_id: "5", + context_id: "11", + tag: "bar", + due: "0"}) expected = [todos(:package_delivered)] assert_equal expected, undone_todos.to_a end From d08608c755d44ab6922c3d103ad1be4dafe684a1 Mon Sep 17 00:00:00 2001 From: Matt Rogers Date: Thu, 11 Apr 2019 14:38:56 -0500 Subject: [PATCH 7/7] Remove the query code from TodosController It has been completely replaced with our new query object --- app/controllers/todos_controller.rb | 34 +---------------------------- 1 file changed, 1 insertion(+), 33 deletions(-) diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index a004c943..e64c3352 100644 --- a/app/controllers/todos_controller.rb +++ b/app/controllers/todos_controller.rb @@ -1316,39 +1316,7 @@ end end def get_not_done_todos - if params[:done] - not_done_todos = current_user.todos.completed.completed_after(Time.zone.now - params[:done].to_i.days) - else - not_done_todos = current_user.todos.active.not_hidden - end - - not_done_todos = not_done_todos. - reorder(Arel.sql("todos.due IS NULL, todos.due ASC, todos.created_at ASC")). - includes(Todo::DEFAULT_INCLUDES) - - not_done_todos = not_done_todos.limit(sanitize(params[:limit])) if params[:limit] - - if params[:due] - due_within_when = Time.zone.now + params['due'].to_i.days - not_done_todos = not_done_todos.where('todos.due <= ?', due_within_when) - end - - if params[:tag] - tag = Tag.where(:name => params['tag']).first - not_done_todos = not_done_todos.where('taggings.tag_id = ?', tag.id) - end - - if params[:context_id] - context = current_user.contexts.find(params[:context_id]) - not_done_todos = not_done_todos.where('context_id' => context.id) - end - - if params[:project_id] - project = current_user.projects.find(params[:project_id]) - not_done_todos = not_done_todos.where('project_id' => project) - end - - return Todos::UndoneTodosQuery.new(current_user).query(params) + Todos::UndoneTodosQuery.new(current_user).query(params) end def onsite_redirect_to(destination)