Refactoring of code to filter/order tickets.

3 files changed, 69 insertions, 67 deletions

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