Permalink
Browse files

fix #827. You can now select todos with tags using OR and AND

/todos/tag/tagA,tagB?and=tagC will select all todos with (tagA or tagB) AND tagC
  • Loading branch information...
1 parent 2accbd0 commit 58d8bc56d1e394db9e83dc80bf450e2288241663 @lrbalt lrbalt committed Oct 28, 2011
@@ -2,8 +2,8 @@ class TodosController < ApplicationController
helper :todos
- skip_before_filter :login_required, :only => [:index, :calendar]
- prepend_before_filter :login_or_feed_token_required, :only => [:index, :calendar]
+ skip_before_filter :login_required, :only => [:index, :calendar, :tag]
+ prepend_before_filter :login_or_feed_token_required, :only => [:index, :calendar, :tag]
append_before_filter :find_and_activate_ready, :only => [:index, :list_deferred]
# TODO: replace :except with :only
@@ -584,50 +584,11 @@ def filter_to_project
redirect_to project_todos_path(project, :format => 'm')
end
- def get_ids_from_tag_expr(tag_expr)
- ids = []
- tag_expr.each do |tag_list|
- id_list = []
- tag_list.each do |tag|
- tag = Tag.find_by_name(tag)
- id_list << tag.id if tag
- end
- ids << id_list
- end
- return ids
- end
-
- def get_params_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]
-
- i = 1
- while params['and'+i.to_s]
- @tag_expr << sanitize(params['and'+i.to_s]).split(',')
- i=i+1
- end
-
- @single_tag = @tag_expr.size == 1 && @tag_expr[0].size == 1
- @tag_name = @tag_expr[0][0] # if @single_tag
- end
-
- def find_todos_with_tag_ids(tag_ids)
- todos = current_user.todos
- tag_ids.each do |ids|
- todos = todos.with_tags(ids) unless ids.nil? || ids.empty?
- end
- return todos
- end
-
# /todos/tag/[tag_name] shows all the actions tagged with tag_name
def tag
- @page_title = t('todos.tagged_page_title', :tag_name => @tag_name)
- @source_view = params['_source_view'] || 'tag'
-
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
@@ -636,29 +597,23 @@ def tag
init_data_for_sidebar
end
- @tag = Tag.find_by_name(@tag_name)
- @tag = Tag.new(:name => @tag_name) if @tag.nil?
-
- @tag_ids = get_ids_from_tag_expr(@tag_expr)
- todos_with_tag_ids = find_todos_with_tag_ids(@tag_ids)
+ todos_with_tag_ids = find_todos_with_tag_expr(@tag_expr)
@not_done_todos = todos_with_tag_ids.active.not_hidden.find(:all,
:order => 'todos.due IS NULL, todos.due ASC, todos.created_at ASC', :include => Todo::DEFAULT_INCLUDES)
@hidden_todos = todos_with_tag_ids.hidden.find(:all,
:include => Todo::DEFAULT_INCLUDES,
:order => 'todos.completed_at DESC, todos.created_at DESC')
@deferred = todos_with_tag_ids.deferred.find(:all,
- :order => 'show_from ASC, todos.created_at DESC', :include => Todo::DEFAULT_INCLUDES)
+ :order => 'todos.show_from ASC, todos.created_at DESC', :include => Todo::DEFAULT_INCLUDES)
@pending = todos_with_tag_ids.blocked.find(:all,
- :order => 'show_from ASC, todos.created_at DESC', :include => Todo::DEFAULT_INCLUDES)
+ :order => 'todos.show_from ASC, todos.created_at DESC', :include => Todo::DEFAULT_INCLUDES)
# If you've set no_completed to zero, the completed items box isn't shown on
# the tag page
- max_completed = current_user.prefs.show_number_completed
- @done = current_user.todos.with_tag(@tag).completed.find(:all,
- :limit => max_completed,
- :order => 'todos.completed_at DESC',
- :include => Todo::DEFAULT_INCLUDES)
+ @done = todos_with_tag_ids.completed.find(:all,
+ :limit => current_user.prefs.show_number_completed,
+ :order => 'todos.completed_at DESC', :include => Todo::DEFAULT_INCLUDES)
@projects = current_user.projects
@contexts = current_user.contexts
@@ -680,6 +635,9 @@ def tag
cookies[:mobile_url]= {:value => request.request_uri, :secure => SITE_CONFIG['secure_cookies']}
render :action => "mobile_tag"
}
+ format.text {
+ render :action => 'index', :layout => false, :content_type => Mime::TEXT
+ }
end
end
@@ -1033,6 +991,51 @@ def init_todos_for_mobile_view
:include => [ :project, :context, :tags ])
end
+ def tag_title(tag_expr)
+ and_list = tag_expr.inject([]) { |s,tag_list| s << tag_list.join(',') }
+ return and_list.join(' AND ')
+ end
+
+ def get_params_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]
+
+ i = 1
+ while params['and'+i.to_s]
+ @tag_expr << sanitize(params['and'+i.to_s]).split(',')
+ i=i+1
+ end
+
+ @single_tag = @tag_expr.size == 1 && @tag_expr[0].size == 1
+ @tag_name = @tag_expr[0][0]
+ @tag_title = @single_tag ? @tag_name : tag_title(@tag_expr)
+ end
+
+ def get_ids_from_tag_expr(tag_expr)
+ ids = []
+ tag_expr.each do |tag_list|
+ id_list = []
+ tag_list.each do |tag|
+ tag = Tag.find_by_name(tag)
+ id_list << tag.id if tag
+ end
+ ids << id_list
+ end
+ return ids
+ end
+
+ def find_todos_with_tag_expr(tag_expr)
+ tag_ids = get_ids_from_tag_expr(tag_expr)
+ todos = current_user.todos
+ tag_ids.each do |ids|
+ todos = todos.with_tags(ids) unless ids.nil? || ids.empty?
+ end
+ return todos
+ end
+
def determine_down_count
source_view do |from|
from.todo do
View
@@ -28,18 +28,18 @@ class Todo < ActiveRecord::Base
named_scope :deferred_or_blocked, :conditions => ["(todos.completed_at IS NULL AND NOT(todos.show_from IS NULL)) OR (todos.state = ?)", "pending"]
named_scope :not_deferred_or_blocked, :conditions => ["todos.completed_at IS NULL AND todos.show_from IS NULL AND NOT(todos.state = ?)", "pending"]
named_scope :hidden,
- :joins => :context,
- :conditions => ["todos.state = ? OR (contexts.hide = ? AND (todos.state = ? OR todos.state = ? OR todos.state = ?))",
+ :joins => "INNER JOIN contexts c_hidden ON c_hidden.id = todos.context_id",
+ :conditions => ["todos.state = ? OR (c_hidden.hide = ? AND (todos.state = ? OR todos.state = ? OR todos.state = ?))",
'project_hidden', true, 'active', 'deferred', 'pending']
named_scope :not_hidden,
- :joins => [:context],
- :conditions => ['NOT(todos.state = ? OR (contexts.hide = ? AND (todos.state = ? OR todos.state = ? OR todos.state = ?)))',
+ :joins => "INNER JOIN contexts c_hidden ON c_hidden.id = todos.context_id",
+ :conditions => ['NOT(todos.state = ? OR (c_hidden.hide = ? AND (todos.state = ? OR todos.state = ? OR todos.state = ?)))',
'project_hidden', true, 'active', 'deferred', 'pending']
# other scopes
named_scope :are_due, :conditions => ['NOT (todos.due IS NULL)']
named_scope :with_tag, lambda { |tag| {:joins => :taggings, :conditions => ["taggings.tag_id = ? ", tag.id] } }
- named_scope :with_tags, lambda { |tag_ids| {:joins => :taggings, :conditions => ["taggings.tag_id IN (?) ", tag_ids] } }
+ named_scope :with_tags, lambda { |tag_ids| {:conditions => ["EXISTS(SELECT * from taggings t WHERE t.tag_id IN (?) AND t.taggable_id=todos.id AND t.taggable_type='Todo')", tag_ids] } }
named_scope :of_user, lambda { |user_id| {:conditions => ["todos.user_id = ? ", user_id] } }
named_scope :completed_after, lambda { |date| {:conditions => ["todos.completed_at > ? ", date] } }
named_scope :completed_before, lambda { |date| {:conditions => ["todos.completed_at < ? ", date] } }
@@ -2,24 +2,24 @@
<% if @not_done_todos.empty? -%>
<div class="container context">
<h2><%= t('todos.no_actions_found_title') %></h2>
-<div class="message"><%= t('todos.no_actions_with', :tag_name => @tag_name) %></div>
+<div class="message"><%= t('todos.no_actions_with', :tag_name => @tag_title) %></div>
</div>
<% end -%>
<%= render :partial => "contexts/mobile_context", :collection => @contexts_to_show -%>
-<h2><%= t('todos.deferred_actions_with', :tag_name=> @tag_name) %></h2>
+<h2><%= t('todos.deferred_actions_with', :tag_name=> @tag_title) %></h2>
<% unless (@deferred.nil? or @deferred.size == 0) -%>
<table cellpadding="0" cellspacing="0" border="0">
<%= render :partial => "todos/mobile_todo", :collection => @deferred, :locals => { :parent_container_type => "tag" } -%>
-</table>
+</table>
<% else -%>
-<%= t('todos.no_deferred_actions_with', :tag_name => @tag_name) %>
+<%= t('todos.no_deferred_actions_with', :tag_name => @tag_title) %>
<% end -%>
-<h2><%= t('todos.completed_actions_with', :tag_name => @tag_name) %></h2>
+<h2><%= t('todos.completed_actions_with', :tag_name => @tag_title) %></h2>
<% unless (@done.nil? or @done.size == 0) -%>
<table cellpadding="0" cellspacing="0" border="0">
<%= render :partial => "todos/mobile_todo", :collection => @done, :locals => { :parent_container_type => "tag" } %>
</table>
<% else -%>
-<%= t('todos.no_completed_actions_with', :tag_name => @tag_name) %>
+<%= t('todos.no_completed_actions_with', :tag_name => @tag_title) %>
<% end -%>
</div>
@@ -8,22 +8,22 @@
:locals => { :collapsible => true } %>
<% unless @deferred.nil? -%>
- <%= render :partial => "todos/deferred", :locals => {
- :deferred => @deferred,
+ <%= render :partial => "todos/deferred", :locals => {
+ :deferred => @deferred,
:pending => @pending,
- :collapsible => true,
- :append_descriptor => t('todos.tagged_with', :tag_name => @tag_name),
- :parent_container_type => 'tag'
+ :collapsible => true,
+ :append_descriptor => t('todos.tagged_with', :tag_name => @tag_title),
+ :parent_container_type => 'tag'
} %>
<% end -%>
<% unless @hidden_todos.nil? -%>
- <%= render :partial => "todos/hidden", :object => @hidden_todos, :locals => { :collapsible => true, :append_descriptor => t('todos.tagged_with', :tag_name => @tag_name) } %>
+ <%= render :partial => "todos/hidden", :object => @hidden_todos, :locals => { :collapsible => true, :append_descriptor => t('todos.tagged_with', :tag_name => @tag_title) } %>
<% end -%>
<% unless @done.nil? -%>
<%= render :partial => "todos/completed", :object => @done,
- :locals => { :collapsible => true, :append_descriptor => t('todos.tagged_with', :tag_name => @tag_name) } %>
+ :locals => { :collapsible => true, :append_descriptor => t('todos.tagged_with', :tag_name => @tag_title) } %>
<% end -%>
</div><!-- End of display_box -->
View
@@ -391,10 +391,10 @@ en:
all_completed: All completed actions
feed_title_in_context: in context '%{context}'
older_than_days: ""
- completed_tagged_page_title: TRACKS::Completed tasks with tag %{tag_name}
+ completed_tagged_page_title: TRACKS::Completed tasks with tag '%{tag_name}'
edit: Edit
pending: Pending
- completed_actions_with: Completed actions with the tag %{tag_name}
+ completed_actions_with: Completed actions with the tag '%{tag_name}'
deleted_success: The action was deleted succesfully.
completed_tasks_title: TRACKS::Completed tasks
feed_title_in_project: in project '%{project}'
View
@@ -40,6 +40,7 @@
# UPDATE: added support for mobile view. All tags ending on .m will be
# routed to mobile view of tags.
todos.mobile_tag 'todos/tag/:name.m', :action => "tag", :format => 'm'
+ todos.mobile_tag 'todos/tag/:name.txt', :action => "tag", :format => 'txt'
todos.tag 'todos/tag/:name', :action => "tag", :name => /.*/
todos.done_tag 'todos/done/tag/:name', :action => "done_tag"
todos.all_done_tag 'todos/all_done/tag/:name', :action => "all_done_tag"
@@ -644,4 +644,68 @@ def test_format_note_link_onenote
assert_select("div#notes_todo_#{todo.id} a", 'link me to onenote')
assert_select("div#notes_todo_#{todo.id} a[href=onenote:///E:\\OneNote\\dir\\notes.one#PAGE&amp;section-id={FD597D3A-3793-495F-8345-23D34A00DD3B}&amp;page-id={1C95A1C7-6408-4804-B3B5-96C28426022B}&amp;end]", 'link me to onenote')
end
+
+ def test_get_boolean_expression_from_parameters_of_tag_view_single_tag
+ login_as(:admin_user)
+ get :tag, :name => "single"
+ assert_equal true, assigns['single_tag'], "should recognize it is a single tag name"
+ assert_equal "single", assigns['tag_expr'][0][0], "should store the single tag"
+ end
+
+ def test_get_boolean_expression_from_parameters_of_tag_view_multiple_tags
+ login_as(:admin_user)
+ get :tag, :name => "multiple", :and => "tags", :and1 => "present", :and2 => "here"
+ assert_equal false, assigns['single_tag'], "should recognize it has multiple tags"
+ assert_equal 4, assigns['tag_expr'].size, "should have 4 AND expressions"
+ end
+
+ def test_get_boolean_expression_from_parameters_of_tag_view_multiple_tags_without_digitless_and
+ login_as(:admin_user)
+ get :tag, :name => "multiple", :and1 => "tags", :and2 => "present", :and3 => "here"
+ assert_equal false, assigns['single_tag'], "should recognize it has multiple tags"
+ assert_equal 4, assigns['tag_expr'].size, "should have 4 AND expressions"
+ end
+
+ def test_get_boolean_expression_from_parameters_of_tag_view_multiple_ORs
+ login_as(:admin_user)
+ get :tag, :name => "multiple,tags,present"
+ assert_equal false, assigns['single_tag'], "should recognize it has multiple tags"
+ assert_equal 1, assigns['tag_expr'].size, "should have 1 expressions"
+ assert_equal 3, assigns['tag_expr'][0].size, "should have 3 ORs in 1st expression"
+ end
+
+ def test_get_boolean_expression_from_parameters_of_tag_view_multiple_ORs_and_ANDS
+ login_as(:admin_user)
+ get :tag, :name => "multiple,tags,present", :and => "here,is,two", :and1=>"and,three"
+ assert_equal false, assigns['single_tag'], "should recognize it has multiple tags"
+ assert_equal 3, assigns['tag_expr'].size, "should have 3 expressions"
+ assert_equal 3, assigns['tag_expr'][0].size, "should have 3 ORs in 1st expression"
+ assert_equal 3, assigns['tag_expr'][1].size, "should have 3 ORs in 2nd expression"
+ assert_equal 2, assigns['tag_expr'][2].size, "should have 2 ORs in 3rd expression"
+ end
+
+ def test_set_right_title
+ login_as(:admin_user)
+
+ get :tag, :name => "foo"
+ assert_equal "foo", assigns['tag_title']
+ get :tag, :name => "foo,bar", :and => "baz"
+ assert_equal "foo,bar AND baz", assigns['tag_title']
+ end
+
+ def test_set_default_tag
+ login_as(:admin_user)
+
+ get :tag, :name => "foo"
+ assert_equal "foo", assigns['initial_tags']
+ get :tag, :name => "foo,bar", :and => "baz"
+ assert_equal "foo", assigns['initial_tags']
+ end
+
+ def test_tag_text_feed_not_accessible_to_anonymous_user_without_token
+ login_as nil
+ get :tag, {:name => "foo", :format => "txt" }
+ assert_response 401
+ end
+
end
Oops, something went wrong.

0 comments on commit 58d8bc5

Please sign in to comment.