From 66ce152d5124be52f31d51fc4171fd53ba3a915c Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 18 Dec 2012 13:45:49 -0800 Subject: Refactoring of code to filter/order tickets. --- help/app/models/ticket.rb | 80 +++++++++++-------------------------- help/app/models/ticket_selection.rb | 38 ++++++++++++++++++ help/test/unit/ticket_test.rb | 18 ++++----- 3 files changed, 69 insertions(+), 67 deletions(-) create mode 100644 help/app/models/ticket_selection.rb (limited to 'help') diff --git a/help/app/models/ticket.rb b/help/app/models/ticket.rb index 781216e..7192cb3 100644 --- a/help/app/models/ticket.rb +++ b/help/app/models/ticket.rb @@ -37,17 +37,19 @@ class Ticket < CouchRest::Model::Base design do #TODO--clean this all up - view :by_is_open - view :by_created_by + #view :by_is_open + #view :by_created_by view :by_updated_at view :by_created_at - view :by_is_open_and_created_by + #view :by_is_open_and_created_by view :by_is_open_and_created_at view :by_is_open_and_updated_at - view :includes_post_by, + + #TODO: This view is only used in tests--should we keep it? + view :by_includes_post_by, :map => "function(doc) { var arr = {} @@ -62,7 +64,7 @@ class Ticket < CouchRest::Model::Base } }", :reduce => "function(k,v,r) { return sum(v); }" - view :includes_post_by_and_open_status_and_updated_at, + view :by_includes_post_by_and_is_open_and_updated_at, :map => "function(doc) { var arr = {} @@ -77,7 +79,7 @@ class Ticket < CouchRest::Model::Base } }", :reduce => "function(k,v,r) { return sum(v); }" - view :includes_post_by_and_open_status_and_created_at, + view :by_includes_post_by_and_is_open_and_created_at, :map => "function(doc) { var arr = {} @@ -92,7 +94,7 @@ class Ticket < CouchRest::Model::Base } }", :reduce => "function(k,v,r) { return sum(v); }" - view :includes_post_by_and_updated_at, + view :by_includes_post_by_and_updated_at, :map => "function(doc) { var arr = {} @@ -108,7 +110,7 @@ class Ticket < CouchRest::Model::Base }", :reduce => "function(k,v,r) { return sum(v); }" - view :includes_post_by_and_created_at, + view :by_includes_post_by_and_created_at, :map => "function(doc) { var arr = {} @@ -144,60 +146,26 @@ class Ticket < CouchRest::Model::Base # TODO: Time.now + 2.days is to catch tickets created in future. shouldn't happen but does on my computer now, so this at least catches for now. # TODO handle default values correctly: options[:open_status] = 'open' if !options[:open_status] #hacky. redo this when handling defaults correctly + options[:sort_order] = 'updated_at_desc' if !options[:sort_order] #hacky. redo this when handling defaults correctly - if (is_admin && (options[:admin_status] != 'mine')) - # show all (selected) tickets to admin - if options[:open_status] == 'all' - if options[:sort_order] == 'created_at_desc' - Ticket.by_created_at.startkey(0).endkey(Time.now + 2.days).descending - elsif options[:sort_order] == 'updated_at_asc' - Ticket.by_updated_at.startkey(0).endkey(Time.now + 2.days) - elsif options[:sort_order] == 'created_at_asc' - Ticket.by_created_at.startkey(0).endkey(Time.now + 2.days) - else - Ticket.by_updated_at.startkey(0).endkey(Time.now + 2.days).descending - end - else - if options[:sort_order] == 'created_at_desc' - Ticket.by_is_open_and_created_at.startkey([(options[:open_status] == 'open'), 0]).endkey([(options[:open_status] == 'open'), Time.now + 2.days]).descending - elsif options[:sort_order] == 'updated_at_asc' - Ticket.by_is_open_and_updated_at.startkey([(options[:open_status] == 'open'), 0]).endkey([(options[:open_status] == 'open'), Time.now + 2.days]) - elsif options[:sort_order] == 'created_at_asc' - Ticket.by_is_open_and_created_at.startkey([(options[:open_status] == 'open'), 0]).endkey([(options[:open_status] == 'open'), Time.now + 2.days]) - else - Ticket.by_is_open_and_updated_at.startkey([(options[:open_status] == 'open'), 0]).endkey([(options[:open_status] == 'open'), Time.now + 2.days]).descending - end - end + options[:user_id] = user.id + options[:is_admin] = is_admin + + @selection = TicketSelection.new(options) + + #TODO: can this be more succinct? + if @selection.order + @tickets = Ticket.send(@selection.finder_method).startkey(@selection.startkey).endkey(@selection.endkey).send(@selection.order) else - # only show tickets this user has commented on, as user is non-admin or admin viewing only their tickets - if options[:open_status] == 'all' - if options[:sort_order] == 'created_at_desc' - Ticket.includes_post_by_and_created_at.startkey([user.id, 0]).endkey([user.id, Time.now + 2.days]).descending - elsif options[:sort_order] == 'updated_at_asc' - Ticket.includes_post_by_and_updated_at.startkey([user.id, 0]).endkey([user.id, Time.now + 2.days]) - elsif options[:sort_order] == 'created_at_asc' - Ticket.includes_post_by_and_created_at.startkey([user.id, 0]).endkey([user.id, Time.now + 2.days]) - else - Ticket.includes_post_by_and_updated_at.startkey([user.id, 0]).endkey([user.id, Time.now + 2.days]).descending - end - else - if options[:sort_order] == 'created_at_desc' - Ticket.includes_post_by_and_open_status_and_created_at.startkey([user.id, (options[:open_status] == 'open'), 0]).endkey([user.id, (options[:open_status] == 'open'), Time.now + 2.days]).descending - elsif options[:sort_order] == 'updated_at_asc' - Ticket.includes_post_by_and_open_status_and_updated_at.startkey([user.id, (options[:open_status] == 'open'), 0]).endkey([user.id, (options[:open_status] == 'open'), Time.now + 2.days]) - elsif options[:sort_order] == 'created_at_asc' - Ticket.includes_post_by_and_open_status_and_created_at.startkey([user.id, (options[:open_status] == 'open'), 0]).endkey([user.id, (options[:open_status] == 'open'), Time.now + 2.days]) - else - Ticket.includes_post_by_and_open_status_and_updated_at.startkey([user.id, (options[:open_status] == 'open'), 0]).endkey([user.id, (options[:open_status] == 'open'), Time.now + 2.days]).descending - end - end + @tickets = Ticket.send(@selection.finder_method).startkey(@selection.startkey).endkey(@selection.endkey) end - end - def self.tickets_by_commenter(user_id)#, options = {}) - Ticket.includes_post_by_and_updated_at.startkey([user_id, 0]).endkey([user_id, Time.now]) end + #def self.tickets_by_commenter(user_id)#, options = {}) + # Ticket.includes_post_by_and_updated_at.startkey([user_id, 0]).endkey([user_id, Time.now]) + #end + def is_creator_validated? !!created_by end diff --git a/help/app/models/ticket_selection.rb b/help/app/models/ticket_selection.rb new file mode 100644 index 0000000..77720f7 --- /dev/null +++ b/help/app/models/ticket_selection.rb @@ -0,0 +1,38 @@ +class TicketSelection + + def initialize(options = {}) + @options = options + end + + def finder_method + method = 'by_' + method += 'includes_post_by_and_' if !@options[:is_admin] or (@options[:admin_status] == 'mine') + method += 'is_open_and_' if @options[:open_status] != 'all' + method += @options[:sort_order].sub(/_(de|a)sc$/, '') + end + + def startkey + startkeys = [] + startkeys << @options[:user_id] if !@options[:is_admin] or (@options[:admin_status] == 'mine') + startkeys << (@options[:open_status] == 'open') if @options[:open_status] != 'all' + startkeys << 0 + startkeys = startkeys.join if startkeys.length == 1 #want string not array if just one thing in array + startkeys + end + + def endkey + endtime = Time.now + 2.days #TODO + if self.startkey.is_a?(Array) + endkeys = self.startkey + endkeys.pop + endkeys << endtime + else + endtime + end + end + + def order + 'descending' if @options[:sort_order].end_with? 'desc' + end + +end diff --git a/help/test/unit/ticket_test.rb b/help/test/unit/ticket_test.rb index e93a121..ac6426e 100644 --- a/help/test/unit/ticket_test.rb +++ b/help/test/unit/ticket_test.rb @@ -56,38 +56,34 @@ class TicketTest < ActiveSupport::TestCase end =end - test "finds open tickets sorted by created_at" do - tickets = Ticket.by_is_open_and_created_at. - startkey([true, 0]). - endkey([true, Time.now + 10.hours]) # some tickets were created in the future - assert_equal Ticket.by_is_open.key(true).count, tickets.count - end test "find tickets user commented on" do # clear old tickets just in case # this will cause RestClient::ResourceNotFound errors if there are multiple copies of the same ticket returned - Ticket.includes_post_by.key('123').each {|t| t.destroy} + Ticket.by_includes_post_by.key('123').each {|t| t.destroy} + # TODO: the by_includes_post_by view is only used for tests. Maybe we should get rid of it and change the test to including ordering? + testticket = Ticket.create :title => "test retrieving commented tickets" comment = TicketComment.new :body => "my email broke", :posted_by => "123" assert_equal 0, testticket.comments.count - assert_equal [], Ticket.includes_post_by.key('123').all + assert_equal [], Ticket.by_includes_post_by.key('123').all testticket.comments << comment testticket.save assert_equal 1, testticket.reload.comments.count - assert_equal [testticket], Ticket.includes_post_by.key('123').all + assert_equal [testticket], Ticket.by_includes_post_by.key('123').all comment = TicketComment.new :body => "another comment", :posted_by => "123" testticket.comments << comment testticket.save # this will ensure that the ticket is only included once, even though the user has commented on the ticket twice: - assert_equal [testticket], Ticket.includes_post_by.key('123').all + assert_equal [testticket], Ticket.by_includes_post_by.key('123').all testticket.destroy - assert_equal [], Ticket.includes_post_by.key('123').all; + assert_equal [], Ticket.by_includes_post_by.key('123').all; end end -- cgit v1.2.3 From 05fd50e769e00e2f63eb4e0eae44ecbc47a6606a Mon Sep 17 00:00:00 2001 From: jessib Date: Tue, 18 Dec 2012 13:46:48 -0800 Subject: Minor CSS improvement using bootstrap. --- help/app/views/tickets/new.html.haml | 5 +++-- help/app/views/tickets/show.html.haml | 4 ++-- 2 files changed, 5 insertions(+), 4 deletions(-) (limited to 'help') diff --git a/help/app/views/tickets/new.html.haml b/help/app/views/tickets/new.html.haml index ca036a5..a8c5f8f 100644 --- a/help/app/views/tickets/new.html.haml +++ b/help/app/views/tickets/new.html.haml @@ -5,5 +5,6 @@ = render :partial => 'new_comment', :locals => {:f => f} = # regarding_user if not logged in = # email if not logged in - = f.button :submit - = link_to t(:cancel), tickets_path, :class => :btn + .form-actions + = f.button :submit, :class => 'btn-primary' + = link_to t(:cancel), tickets_path, :class => :btn diff --git a/help/app/views/tickets/show.html.haml b/help/app/views/tickets/show.html.haml index a4f3b5f..46d86de 100644 --- a/help/app/views/tickets/show.html.haml +++ b/help/app/views/tickets/show.html.haml @@ -28,7 +28,7 @@ = render :partial => 'new_comment', :locals => {:f => f} = f.button :submit, @post_reply_str - if @ticket.is_open - = f.button :submit, @reply_close_str + = f.button :submit, @reply_close_str, :class => 'btn-primary' = #link_to t(:destroy), ticket_path, :confirm => 'are you sure?', :method => :delete, :class => :btn if admin? # for link_to to work with delete, need to figure out jquery interaction correctly. see http://stackoverflow.com/questions/3774925/delete-link-sends-get-instead-of-delete-in-rails-3-view etc.. -= button_to 'destroy', ticket_path, :confirm => 'are you sure?', :method => :delete if admin? += button_to 'Destroy', ticket_path, :confirm => 'are you sure?', :class => 'btn btn-danger', :method => :delete if admin? = link_to t(:cancel), tickets_path, :class => :btn -- cgit v1.2.3 From d6c881294880a934424e4d399786561d5c107167 Mon Sep 17 00:00:00 2001 From: jessib Date: Wed, 19 Dec 2012 11:13:50 -0800 Subject: Some cleanup of code --- help/app/models/ticket.rb | 14 +------------- help/app/models/ticket_selection.rb | 26 +++++++++++++++++++++++--- 2 files changed, 24 insertions(+), 16 deletions(-) (limited to 'help') diff --git a/help/app/models/ticket.rb b/help/app/models/ticket.rb index 7192cb3..fa056b4 100644 --- a/help/app/models/ticket.rb +++ b/help/app/models/ticket.rb @@ -141,25 +141,13 @@ class Ticket < CouchRest::Model::Base def self.for_user(user, options = {}, is_admin = false) - # TODO: This is obviously super tedious. we will refactor later. # TODO: thought i should reverse keys for descending, but that didn't work. look into whether that should be tweaked, and whether it works okay with pagination (seems to now...) - # TODO: Time.now + 2.days is to catch tickets created in future. shouldn't happen but does on my computer now, so this at least catches for now. - # TODO handle default values correctly: - options[:open_status] = 'open' if !options[:open_status] #hacky. redo this when handling defaults correctly - options[:sort_order] = 'updated_at_desc' if !options[:sort_order] #hacky. redo this when handling defaults correctly options[:user_id] = user.id options[:is_admin] = is_admin @selection = TicketSelection.new(options) - - #TODO: can this be more succinct? - if @selection.order - @tickets = Ticket.send(@selection.finder_method).startkey(@selection.startkey).endkey(@selection.endkey).send(@selection.order) - else - @tickets = Ticket.send(@selection.finder_method).startkey(@selection.startkey).endkey(@selection.endkey) - end - + @selection.tickets end #def self.tickets_by_commenter(user_id)#, options = {}) diff --git a/help/app/models/ticket_selection.rb b/help/app/models/ticket_selection.rb index 77720f7..cb20f5b 100644 --- a/help/app/models/ticket_selection.rb +++ b/help/app/models/ticket_selection.rb @@ -2,18 +2,33 @@ class TicketSelection def initialize(options = {}) @options = options + @options[:open_status] ||= 'open' + @options[:sort_order] ||= 'updated_at_desc' + + end + + def tickets + #TODO: can this be more succinct? + if order + Ticket.send(finder_method).startkey(startkey).endkey(endkey).send(order) + else + Ticket.send(finder_method).startkey(startkey).endkey(endkey) + end end + protected + + def finder_method method = 'by_' - method += 'includes_post_by_and_' if !@options[:is_admin] or (@options[:admin_status] == 'mine') + method += 'includes_post_by_and_' if only_mine? method += 'is_open_and_' if @options[:open_status] != 'all' method += @options[:sort_order].sub(/_(de|a)sc$/, '') end def startkey startkeys = [] - startkeys << @options[:user_id] if !@options[:is_admin] or (@options[:admin_status] == 'mine') + startkeys << @options[:user_id] if only_mine? startkeys << (@options[:open_status] == 'open') if @options[:open_status] != 'all' startkeys << 0 startkeys = startkeys.join if startkeys.length == 1 #want string not array if just one thing in array @@ -21,7 +36,7 @@ class TicketSelection end def endkey - endtime = Time.now + 2.days #TODO + endtime = Time.now + 2.days #TODO. this obviously isn't ideal if self.startkey.is_a?(Array) endkeys = self.startkey endkeys.pop @@ -35,4 +50,9 @@ class TicketSelection 'descending' if @options[:sort_order].end_with? 'desc' end + + def only_mine? + !@options[:is_admin] or (@options[:admin_status] == 'mine') + end + end -- cgit v1.2.3