From 9a6ab05eef5a431e704cfadbf5682be82166d0ed Mon Sep 17 00:00:00 2001 From: Reinier Balt Date: Mon, 3 Jun 2013 15:17:38 +0200 Subject: [PATCH] fix #1429 by correctly handling tags with dots This was a very old regression. This used to work before the rails 3.2 upgrade... Added tests to prevent future unnoticed breakage --- app/controllers/todos_controller.rb | 33 ++++++++++++++++------ config/routes.rb | 7 ++++- test/functional/todos_controller_test.rb | 36 ++++++++++++++++++++++++ test/unit/tagging_test.rb | 2 +- 4 files changed, 68 insertions(+), 10 deletions(-) diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index 8ac538ae..59f3239c 100644 --- a/app/controllers/todos_controller.rb +++ b/app/controllers/todos_controller.rb @@ -657,13 +657,8 @@ class TodosController < ApplicationController get_params_for_tag_view @page_title = t('todos.tagged_page_title', :tag_name => @tag_title) @source_view = params['_source_view'] || 'tag' - - if mobile? - # mobile tags are routed with :name ending on .m. So we need to chomp it - @tag_name = @tag_name.chomp('.m') - else - init_data_for_sidebar - end + + init_data_for_sidebar unless mobile? todos_with_tag_ids = find_todos_with_tag_expr(@tag_expr) @@ -945,8 +940,9 @@ class TodosController < ApplicationController end def get_params_for_tag_view - # use sanitize to prevent XSS attacks + filter_format_for_tag_view + # use sanitize to prevent XSS attacks @tag_expr = [] @tag_expr << sanitize(params[:name]).split(',') @tag_expr << sanitize(params[:and]).split(',') if params[:and] @@ -962,6 +958,27 @@ class TodosController < ApplicationController @tag_title = @single_tag ? @tag_name : tag_title(@tag_expr) end + def filter_format_for_tag_view + # routes for tag view do not set :format + if params[:name] =~ /.*\.m$/ + set_format_for_tag_view(:m) + elsif params[:name] =~ /.*\.txt$/ + set_format_for_tag_view(:txt) + # set content-type to text/plain or it remains text/html + response.headers["Content-Type"] = 'text/plain' + elsif params[:format].nil? + # if no format is given, default to html + # note that if url has ?format=m, we should not overwrite it here + request.format, params[:format] = :html, :html + end + end + + def set_format_for_tag_view(format) + # tag name ends with .m, set format to :m en remove .m from name + request.format, params[:format] = format, format + params[:name] = params[:name].chomp(".#{format.to_s}") +end + def get_ids_from_tag_expr(tag_expr) ids = [] tag_expr.each do |tag_list| diff --git a/config/routes.rb b/config/routes.rb index b7ff5503..69b41837 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -101,7 +101,12 @@ Tracksapp::Application.routes.draw do post 'add_predecessor' end end - match 'todos/tag/:name' => 'todos#tag', :as => :tag + + # match /todos/tag and put everything in :name, including extensions like .m and .txt. + # This means the controller action needs to parse the extension and set format/content type + # Needed for /todos/tag/first.last.m to work + match 'todos/tag/:name' => 'todos#tag', :as => :tag, :format => false, :name => /.*/ + match 'tags.autocomplete' => "todos#tags", :format => 'autocomplete' match 'todos/done/tag/:name' => "todos#done_tag", :as => :done_tag diff --git a/test/functional/todos_controller_test.rb b/test/functional/todos_controller_test.rb index bd1cdd64..c34fd0d6 100644 --- a/test/functional/todos_controller_test.rb +++ b/test/functional/todos_controller_test.rb @@ -121,6 +121,28 @@ class TodosControllerTest < ActionController::TestCase assert_equal 3, @tagged end + def test_find_tagged_with_terms_separated_with_dot + login_as :admin_user + create_todo(description: "test dotted tag", tag_list: "first.last, second") + t = assigns['todo'] + assert_equal "first.last, second", t.tag_list + + get :tag, name: 'first.last.m' + assert_equal "text/html", request.format, "controller should set right content type" + assert_equal "text/html", @response.content_type + assert_equal "first.last", assigns['tag_name'], ".m should be chomped" + + get :tag, name: 'first.last.txt' + assert_equal "text/plain", request.format, "controller should set right content type" + assert_equal "text/plain", @response.content_type + assert_equal "first.last", assigns['tag_name'], ".txt should be chomped" + + get :tag, name: 'first.last' + assert_equal "text/html", request.format, "controller should set right content type" + assert_equal "text/html", @response.content_type + assert_equal "first.last", assigns['tag_name'], ":name should be correct" + end + def test_get_boolean_expression_from_parameters_of_tag_view_single_tag login_as(:admin_user) get :tag, :name => "single" @@ -897,5 +919,19 @@ class TodosControllerTest < ActionController::TestCase assert t4.pending?, "t4 should remain pending" assert t4.predecessors.map(&:id).include?(t3.id) end + + private + + def create_todo(params={}) + defaults = { source_view: 'todo', + context_name: "library", project_name: "Build a working time machine", + notes: "note", description: "a new todo", due: nil, tag_list: "a,b,c"} + + params=params.reverse_merge(defaults) + + put :create, _source_view: params[:_source_view], + context_name: params[:context_name], project_name: params[:project_name], tag_list: params[:tag_list], + todo: {notes: params[:notes], description: params[:description], due: params[:due]} + end end diff --git a/test/unit/tagging_test.rb b/test/unit/tagging_test.rb index 7335450e..68c28017 100644 --- a/test/unit/tagging_test.rb +++ b/test/unit/tagging_test.rb @@ -11,6 +11,6 @@ class TaggingTest < ActiveSupport::TestCase tagging.destroy - assert_nil Tag.find_by_name("hello") + assert_nil Tag.find_by_name("hello"), "Tag should be destroyed when last use in tagging was removed" end end