From b70911d4893bfdad0259a761c962daa4c5595ec8 Mon Sep 17 00:00:00 2001 From: Joe Cohen Date: Wed, 12 Nov 2014 13:00:21 -0800 Subject: [PATCH 1/7] Add test and fixtures to detect incorrect target of Previous Version: _n_ link --- test/fixtures/glossary_terms.yml | 11 +++++++++ test/fixtures/glossary_terms_versions.yml | 25 +++++++++++++++++++++ test/functional/glossary_controller_test.rb | 7 ++++++ 3 files changed, 43 insertions(+) diff --git a/test/fixtures/glossary_terms.yml b/test/fixtures/glossary_terms.yml index 1fcd1511c2..d625e82a6d 100644 --- a/test/fixtures/glossary_terms.yml +++ b/test/fixtures/glossary_terms.yml @@ -28,3 +28,14 @@ plane_glossary_term: thumb_image_id: 11 created_at: 2013-08-17 06:33:50 updated_at: 2013-08-17 06:35:20 + +square_glossary_term: + id: 4 + version: 3 + user_id: 1 + name: Square + description: equilateral + thumb_image_id: nil + created_at: 2013-08-17 06:33:50 + updated_at: 2013-08-17 06:35:20 + diff --git a/test/fixtures/glossary_terms_versions.yml b/test/fixtures/glossary_terms_versions.yml index 6de3db6a94..ef2108b78e 100644 --- a/test/fixtures/glossary_terms_versions.yml +++ b/test/fixtures/glossary_terms_versions.yml @@ -23,3 +23,28 @@ conic_glossary_term_v1: updated_at: 2013-08-04 07:49:12 name: Conic description: Cute little cone head + +square_glossary_term_v1: + id: 4 + glossary_term_id: 4 + version: 1 + updated_at: 2013-08-04 07:49:12 + name: Square + description: + +square_glossary_term_v2: + id: 5 + glossary_term_id: 4 + version: 2 + updated_at: 2013-08-04 07:49:12 + name: Square + description: quadrilateral + +square_glossary_term_v3: + id: 6 + glossary_term_id: 4 + version: 3 + updated_at: 2013-08-04 07:49:12 + name: Square + description: equilateral + diff --git a/test/functional/glossary_controller_test.rb b/test/functional/glossary_controller_test.rb index 9ee52d0cd7..3981efb12f 100644 --- a/test/functional/glossary_controller_test.rb +++ b/test/functional/glossary_controller_test.rb @@ -98,4 +98,11 @@ def test_generate_and_show_past_glossary_term assert_template(action: 'show_past_glossary_term', partial: "_glossary_term") end + test "prior version link target" do + square = glossary_terms(:square_glossary_term) + prior_version_target = "/glossary/show_past_glossary_term/#{square.id}?version=#{square.version-1}" + get(:show_glossary_term, id: square.id) + assert_select "a[href=?]", prior_version_target + end + end From b88a7fdc46a182f2b8080e4785899f7f5c1aa760 Mon Sep 17 00:00:00 2001 From: Joe Cohen Date: Wed, 12 Nov 2014 13:06:32 -0800 Subject: [PATCH 2/7] Fix [Pivotal Issue 12252025](https://www.pivotaltracker.com/story/show/12252025) (Bug: "Show Previous Version" goes to current version instead of previous version) --- app/helpers/application_helper.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 9ef8ff9edf..18e2324cbf 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -611,7 +611,7 @@ def show_previous_version(obj) if previous_version = latest_version.previous html += link_with_query("#{:show_name_previous_version.t}: %d" % previous_version.version, :action => "show_past_#{type}", :id => obj.id, - :version => previous_version) + :version => previous_version.version) if (previous_version.merge_source_id rescue false) html += indent(1) + get_version_merge_link(obj, previous_version) end From 255f0dfeb51ddf71d4f233e741e9b5d2176cc96a Mon Sep 17 00:00:00 2001 From: Joe Cohen Date: Thu, 13 Nov 2014 14:26:15 -0800 Subject: [PATCH 3/7] Fix RuboCop format violations * replace single quotes with double * replace hash rockets with colons for keywords * limit line length to 80 * align continued lines * fix whitespace issues Move test prior_version_link_target --- test/functional/glossary_controller_test.rb | 85 +++++++++++---------- 1 file changed, 45 insertions(+), 40 deletions(-) diff --git a/test/functional/glossary_controller_test.rb b/test/functional/glossary_controller_test.rb index 3981efb12f..85b3c5c522 100644 --- a/test/functional/glossary_controller_test.rb +++ b/test/functional/glossary_controller_test.rb @@ -1,61 +1,70 @@ -require 'test_helper' +require "test_helper" +# functional tests of glossary controller and views class GlossaryControllerTest < FunctionalTestCase def test_show_glossary_term glossary_term = glossary_terms(:plane_glossary_term) - get_with_dump(:show_glossary_term, :id => glossary_term.id) - assert_template(action: 'show_glossary_term') + get_with_dump(:show_glossary_term, id: glossary_term.id) + assert_template(action: "show_glossary_term") + end + + test "prior version link target" do + square = glossary_terms(:square_glossary_term) + prior_version_target = "/glossary/show_past_glossary_term/" \ + "#{square.id}?version=#{square.version - 1}" + get(:show_glossary_term, id: square.id) + assert_select "a[href=?]", prior_version_target end def test_show_past_glossary_term conic = glossary_terms(:conic_glossary_term) - get_with_dump(:show_past_glossary_term, :id => conic.id, :version => conic.version - 1) - assert_template(action: 'show_past_glossary_term', partial: "_glossary_term") + get_with_dump(:show_past_glossary_term, + id: conic.id, + version: conic.version - 1) + assert_template(action: "show_past_glossary_term", + partial: "_glossary_term") end def test_show_past_glossary_term_no_version conic = glossary_terms(:conic_glossary_term) - get_with_dump(:show_past_glossary_term, :id => conic.id) + get_with_dump(:show_past_glossary_term, id: conic.id) assert_response(:redirect) end def test_index get_with_dump(:index) - assert_template(action: 'index') + assert_template(action: "index") end - def test_create_glossary_term + def test_create_glossary_term_not_logged_in get(:create_glossary_term) assert_response(:redirect) + end + def test_create_glossary_term_logged_in login get_with_dump(:create_glossary_term) - assert_template(action: 'create_glossary_term') + assert_template(action: "create_glossary_term") end - + def create_glossary_term_params - return { - :glossary_term => { - :name => 'Convex', - :description => 'Boring old convex', - }, - :copyright_holder => "Insil Choi", - :date => { - :copyright_year => '2013' - }, - :upload => { - :license_id => '1' + { glossary_term: + { name: "Convex", description: "Boring old convex" }, + copyright_holder: "Insil Choi", + date: { copyright_year: 2013 }, + upload: { license_id: 1 } } - } end - + def test_create_glossary_term_post user = login params = create_glossary_term_params post(:create_glossary_term, params) - glossary_term = GlossaryTerm.find(:all, :order => "created_at DESC")[0] + glossary_term = GlossaryTerm.find(:all, order: "created_at DESC")[0] + assert_equal(params[:glossary_term][:name], glossary_term.name) - assert_equal(params[:glossary_term][:description], glossary_term.description) + assert_equal(params[:glossary_term][:description], + glossary_term.description) assert_not_nil(glossary_term.rss_log) assert_equal(user.id, glossary_term.user_id) assert_response(:redirect) @@ -63,14 +72,14 @@ def test_create_glossary_term_post def test_edit_glossary_term conic = glossary_terms(:conic_glossary_term) - get_with_dump(:edit_glossary_term, :id => conic.id) + get_with_dump(:edit_glossary_term, id: conic.id) assert_response(:redirect) login - get_with_dump(:edit_glossary_term, :id => conic.id) - assert_template(action: 'edit_glossary_term') + get_with_dump(:edit_glossary_term, id: conic.id) + assert_template(action: "edit_glossary_term") end - + def test_edit_glossary_term_post conic = glossary_terms(:conic_glossary_term) count = GlossaryTerm::Version.count @@ -82,7 +91,7 @@ def test_edit_glossary_term_post conic.reload assert_equal(params[:glossary_term][:name], conic.name) assert_equal(params[:glossary_term][:description], conic.description) - assert_equal(count+1, GlossaryTerm::Version.count) + assert_equal(count + 1, GlossaryTerm::Version.count) assert_response(:redirect) end @@ -90,19 +99,15 @@ def test_generate_and_show_past_glossary_term login glossary_term = glossary_terms(:plane_glossary_term) old_count = glossary_term.versions.length - glossary_term.update_attributes(:description => 'Are we flying yet?') + glossary_term.update_attributes(description: "Are we flying yet?") glossary_term.reload new_count = glossary_term.versions.length assert_equal(1, new_count - old_count) - get_with_dump(:show_past_glossary_term, :id => glossary_term.id, :version => glossary_term.version - 1) - assert_template(action: 'show_past_glossary_term', partial: "_glossary_term") + get_with_dump(:show_past_glossary_term, + id: glossary_term.id, + version: glossary_term.version - 1) + assert_template(action: "show_past_glossary_term", + partial: "_glossary_term") end - test "prior version link target" do - square = glossary_terms(:square_glossary_term) - prior_version_target = "/glossary/show_past_glossary_term/#{square.id}?version=#{square.version-1}" - get(:show_glossary_term, id: square.id) - assert_select "a[href=?]", prior_version_target - end - end From 29f60cc6c57bbc0a132191ce10a11716c69053e6 Mon Sep 17 00:00:00 2001 From: Joe Cohen Date: Fri, 14 Nov 2014 17:52:51 -0800 Subject: [PATCH 4/7] fix RuboCop metric violations in tests of create_glossary_term * create one method for each assertion --- test/functional/glossary_controller_test.rb | 101 ++++++++++++-------- 1 file changed, 63 insertions(+), 38 deletions(-) diff --git a/test/functional/glossary_controller_test.rb b/test/functional/glossary_controller_test.rb index 85b3c5c522..793e06757b 100644 --- a/test/functional/glossary_controller_test.rb +++ b/test/functional/glossary_controller_test.rb @@ -2,40 +2,60 @@ # functional tests of glossary controller and views class GlossaryControllerTest < FunctionalTestCase + # ***** show ***** def test_show_glossary_term glossary_term = glossary_terms(:plane_glossary_term) get_with_dump(:show_glossary_term, id: glossary_term.id) assert_template(action: "show_glossary_term") end - test "prior version link target" do - square = glossary_terms(:square_glossary_term) - prior_version_target = "/glossary/show_past_glossary_term/" \ - "#{square.id}?version=#{square.version - 1}" - get(:show_glossary_term, id: square.id) - assert_select "a[href=?]", prior_version_target + def test_show_past_glossary_term_no_version + conic = glossary_terms(:conic_glossary_term) + get_with_dump(:show_past_glossary_term, id: conic.id) + assert_response(:redirect) end - def test_show_past_glossary_term + def test_show_past_glossary_term_with_version conic = glossary_terms(:conic_glossary_term) get_with_dump(:show_past_glossary_term, - id: conic.id, - version: conic.version - 1) + id: conic.id, version: conic.version - 1) assert_template(action: "show_past_glossary_term", partial: "_glossary_term") end - def test_show_past_glossary_term_no_version - conic = glossary_terms(:conic_glossary_term) - get_with_dump(:show_past_glossary_term, id: conic.id) - assert_response(:redirect) + test "show past glossary term prior version link target" do + square = glossary_terms(:square_glossary_term) + prior_version_target = "/glossary/show_past_glossary_term/" \ + "#{square.id}?version=#{square.version - 1}" + get(:show_glossary_term, id: square.id) + assert_select "a[href=?]", prior_version_target end + # ***** index ***** def test_index get_with_dump(:index) assert_template(action: "index") end + # ***** create ***** + def convex_params + { glossary_term: + { name: "Convex", description: "Boring" }, + copyright_holder: "Me", + date: { copyright_year: 2013 }, upload: { license_id: 1 } + } + end + + def posted_term + login_and_post_convex + GlossaryTerm.find(:all, order: "created_at DESC")[0] + end + + def login_and_post_convex + login + post(:create_glossary_term, convex_params) + end + def test_create_glossary_term_not_logged_in get(:create_glossary_term) assert_response(:redirect) @@ -47,29 +67,30 @@ def test_create_glossary_term_logged_in assert_template(action: "create_glossary_term") end - def create_glossary_term_params - { glossary_term: - { name: "Convex", description: "Boring old convex" }, - copyright_holder: "Insil Choi", - date: { copyright_year: 2013 }, - upload: { license_id: 1 } - } + def test_create_glossary_term_name + assert_equal(convex_params[:glossary_term][:name], posted_term.name) end - def test_create_glossary_term_post + def test_create_glossary_term_description + assert_equal(convex_params[:glossary_term][:description], + posted_term.description) + end + + def test_create_glossary_term_rss_log + assert_not_nil(posted_term.rss_log) + end + + def test_create_glossary_term_user_id user = login - params = create_glossary_term_params - post(:create_glossary_term, params) - glossary_term = GlossaryTerm.find(:all, order: "created_at DESC")[0] - - assert_equal(params[:glossary_term][:name], glossary_term.name) - assert_equal(params[:glossary_term][:description], - glossary_term.description) - assert_not_nil(glossary_term.rss_log) - assert_equal(user.id, glossary_term.user_id) + assert_equal(user.id, posted_term.user_id) + end + + def test_create_glossary_term_redirect + login_and_post_convex assert_response(:redirect) end + # ***** edit ***** def test_edit_glossary_term conic = glossary_terms(:conic_glossary_term) get_with_dump(:edit_glossary_term, id: conic.id) @@ -82,32 +103,36 @@ def test_edit_glossary_term def test_edit_glossary_term_post conic = glossary_terms(:conic_glossary_term) - count = GlossaryTerm::Version.count + old_count = GlossaryTerm::Version.count make_admin - params = create_glossary_term_params - params[:id] = conic.id + params = { id: conic.id, + glossary_term: + { name: "Convex", description: "Boring old convex" }, + copyright_holder: "Insil Choi", date: { copyright_year: 2013 }, + upload: { license_id: 1 } + } + post(:edit_glossary_term, params) + # see if conic changed conic.reload assert_equal(params[:glossary_term][:name], conic.name) assert_equal(params[:glossary_term][:description], conic.description) - assert_equal(count + 1, GlossaryTerm::Version.count) + assert_equal(old_count + 1, GlossaryTerm::Version.count) assert_response(:redirect) end def test_generate_and_show_past_glossary_term login glossary_term = glossary_terms(:plane_glossary_term) - old_count = glossary_term.versions.length + old_length = glossary_term.versions.length glossary_term.update_attributes(description: "Are we flying yet?") glossary_term.reload - new_count = glossary_term.versions.length - assert_equal(1, new_count - old_count) + assert_equal(old_length + 1, glossary_term.versions.length) get_with_dump(:show_past_glossary_term, id: glossary_term.id, version: glossary_term.version - 1) assert_template(action: "show_past_glossary_term", partial: "_glossary_term") end - end From 8de958b4e5937c76dd3841e479b5059513bfb1cd Mon Sep 17 00:00:00 2001 From: Joe Cohen Date: Sat, 15 Nov 2014 15:35:47 -0800 Subject: [PATCH 5/7] Fix RuboCop remaining metric violations in GlossaryControllerTest * make all methods <= 10 lines, mostly via Extract Method * reduce method complexity by having 1 explicit asertiion per test * reduce class size to <= 100 lines by creating subclasses of GlossaryController Test Miscellaneous other refactoring * mostly changes to names of methods and variables --- test/functional/glossary_controller_test.rb | 117 ++++++++++++++------ 1 file changed, 86 insertions(+), 31 deletions(-) diff --git a/test/functional/glossary_controller_test.rb b/test/functional/glossary_controller_test.rb index 793e06757b..6fd3aac7de 100644 --- a/test/functional/glossary_controller_test.rb +++ b/test/functional/glossary_controller_test.rb @@ -2,21 +2,37 @@ # functional tests of glossary controller and views class GlossaryControllerTest < FunctionalTestCase + def conic + glossary_terms(:conic_glossary_term) + end + + def plane + glossary_terms(:plane_glossary_term) + end + + def square + glossary_terms(:square_glossary_term) + end +end + +# tests of controller methods which do not change glossary terms +class GlossaryControllerShowAndIndexTest < GlossaryControllerTest + def setup + @controller = GlossaryController.new + end + # ***** show ***** def test_show_glossary_term - glossary_term = glossary_terms(:plane_glossary_term) - get_with_dump(:show_glossary_term, id: glossary_term.id) + get_with_dump(:show_glossary_term, id: plane.id) assert_template(action: "show_glossary_term") end def test_show_past_glossary_term_no_version - conic = glossary_terms(:conic_glossary_term) get_with_dump(:show_past_glossary_term, id: conic.id) assert_response(:redirect) end def test_show_past_glossary_term_with_version - conic = glossary_terms(:conic_glossary_term) get_with_dump(:show_past_glossary_term, id: conic.id, version: conic.version - 1) assert_template(action: "show_past_glossary_term", @@ -24,7 +40,6 @@ def test_show_past_glossary_term_with_version end test "show past glossary term prior version link target" do - square = glossary_terms(:square_glossary_term) prior_version_target = "/glossary/show_past_glossary_term/" \ "#{square.id}?version=#{square.version - 1}" get(:show_glossary_term, id: square.id) @@ -36,6 +51,13 @@ def test_index get_with_dump(:index) assert_template(action: "index") end +end + +# tests of controller methods which create glossary terms +class GlossaryControllerCreateTest < GlossaryControllerTest + def setup + @controller = GlossaryController.new + end # ***** create ***** def convex_params @@ -56,7 +78,7 @@ def login_and_post_convex post(:create_glossary_term, convex_params) end - def test_create_glossary_term_not_logged_in + def test_create_glossary_term_no_login get(:create_glossary_term) assert_response(:redirect) end @@ -89,49 +111,82 @@ def test_create_glossary_term_redirect login_and_post_convex assert_response(:redirect) end +end + +# tests of controller methods which edit glossary terms +class GlossaryControllerEditTest < GlossaryControllerTest + def setup + @controller = GlossaryController.new + end # ***** edit ***** - def test_edit_glossary_term - conic = glossary_terms(:conic_glossary_term) + def test_edit_glossary_term_no_login get_with_dump(:edit_glossary_term, id: conic.id) assert_response(:redirect) + end + def test_edit_glossary_term_logged_in login get_with_dump(:edit_glossary_term, id: conic.id) assert_template(action: "edit_glossary_term") end - def test_edit_glossary_term_post - conic = glossary_terms(:conic_glossary_term) - old_count = GlossaryTerm::Version.count - make_admin + def changes_to_conic + { id: conic.id, + glossary_term: { name: "Convex", description: "Boring old convex" }, + copyright_holder: "Insil Choi", date: { copyright_year: 2013 }, + upload: { license_id: 1 } + } + end - params = { id: conic.id, - glossary_term: - { name: "Convex", description: "Boring old convex" }, - copyright_holder: "Insil Choi", date: { copyright_year: 2013 }, - upload: { license_id: 1 } - } + def post_conic_edit_changes + make_admin + post(:edit_glossary_term, changes_to_conic) + end - post(:edit_glossary_term, params) - # see if conic changed + def post_conic_edit_changes_and_reload + post_conic_edit_changes conic.reload - assert_equal(params[:glossary_term][:name], conic.name) - assert_equal(params[:glossary_term][:description], conic.description) - assert_equal(old_count + 1, GlossaryTerm::Version.count) + end + + def test_edit_glossary_term_redirect + post_conic_edit_changes assert_response(:redirect) end - def test_generate_and_show_past_glossary_term + def test_edit_glossary_term_count + old_count = GlossaryTerm::Version.count + post_conic_edit_changes + assert_equal(old_count + 1, GlossaryTerm::Version.count) + end + + def test_edit_glossary_term_name + post_conic_edit_changes_and_reload + assert_equal(changes_to_conic[:glossary_term][:name], conic.name) + end + + def test_edit_glossary_term_name_and_description + post_conic_edit_changes_and_reload + assert_equal(changes_to_conic[:glossary_term][:description], + conic.description) + end + + def update_and_reload_plane_past_version login - glossary_term = glossary_terms(:plane_glossary_term) - old_length = glossary_term.versions.length - glossary_term.update_attributes(description: "Are we flying yet?") - glossary_term.reload - assert_equal(old_length + 1, glossary_term.versions.length) + plane.update_attributes(description: "Are we flying yet?") + plane.reload + end + + def test_generate_past_glossary_term + old_length = plane.versions.length + update_and_reload_plane_past_version + assert_equal(old_length + 1, plane.versions.length) + end + + def test_generate_and_show_past_glossary_term + update_and_reload_plane_past_version get_with_dump(:show_past_glossary_term, - id: glossary_term.id, - version: glossary_term.version - 1) + id: plane.id, version: plane.version - 1) assert_template(action: "show_past_glossary_term", partial: "_glossary_term") end From ea16312785c4e60995c75051fc7c82366ff0ef22 Mon Sep 17 00:00:00 2001 From: Nathan Wilson Date: Tue, 30 Dec 2014 15:15:29 +0000 Subject: [PATCH 6/7] Sped up the list_names page substantially by collapsing a query per name to a single query --- app/models/name.rb | 12 ++++++++++++ app/views/name/list_names.html.erb | 3 ++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/models/name.rb b/app/models/name.rb index 2ff0aa8da1..d57e26d2bb 100644 --- a/app/models/name.rb +++ b/app/models/name.rb @@ -1218,6 +1218,18 @@ def self.suggest_alternate_spellings(str) return results end + def self.count_observations(names) + ids = names.map(&:id) + counts_and_ids = Name.connection.select(%( + SELECT count(*) c, names.id i FROM observations, names + WHERE observations.name_id = names.id + AND names.id IN (#{ids.join(", ")}) group by names.id + )) + result = {} + counts_and_ids.each { |row| result[row["i"]] = row["c"] } + result + end + private # Guess correct name of partial string. diff --git a/app/views/name/list_names.html.erb b/app/views/name/list_names.html.erb index bae119c453..0baecf20a0 100644 --- a/app/views/name/list_names.html.erb +++ b/app/views/name/list_names.html.erb @@ -31,6 +31,7 @@ args = @test_pagination_args || {} %> <%= paginate_block(@pages, args) do %> <% if @objects != [] %> <% odd_or_even = 0 %> + <% counts = Name.count_observations(@objects) %> <% for name in @objects || [] odd_or_even = 1 - odd_or_even %> @@ -38,7 +39,7 @@ args = @test_pagination_args || {} %> + :id => name.id) + " [#{counts[name.id]}]" %> <% if @extra_data %> <% data = @extra_data[name.id] if data && !data.empty? From dd5510ca3aacd69fa51b17e42ea88e25dc8919d1 Mon Sep 17 00:00:00 2001 From: Nathan Wilson Date: Tue, 30 Dec 2014 20:02:09 +0000 Subject: [PATCH 7/7] Adding Turkish to the .gitignore --- config/locales/.gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/config/locales/.gitignore b/config/locales/.gitignore index 8a4572ceec..3bd7203489 100644 --- a/config/locales/.gitignore +++ b/config/locales/.gitignore @@ -7,3 +7,4 @@ it.txt pl.txt pt.txt ru.txt +tr.txt
<%= link_with_query(name.display_name.html_safe.t, :action => 'show_name', - :id => name.id) + " [#{name.observations.count}]" %>