Merge pull request #464 from calagator/simplify_event

Simplify Event

app/controllers/calagator/venues_controller.rb

@@ -53,7 +53,7 @@ def show
       format.html
       format.xml  { render xml: @venue }
       format.json { render json: @venue, callback: params[:callback] }
-      format.ics  { render ics: @venue.events.order("start_time ASC").non_duplicates }
+      format.ics  { render ics: @venue.events.order("start_time ASC") }
     end
 
   rescue ActiveRecord::RecordNotFound => e

app/models/calagator/event.rb

@@ -27,6 +27,7 @@
 require "paper_trail"
 require "loofah-activerecord"
 require "loofah/activerecord/xss_foliate"
+require "active_model/sequential_validator"
 
 # == Event
 #
@@ -39,24 +40,24 @@ class Event < ActiveRecord::Base
   has_paper_trail
   acts_as_taggable
 
-  xss_foliate :strip => [:title], :sanitize => [:description, :venue_details]
+  xss_foliate strip: [:title, :description, :venue_details]
+
   include DecodeHtmlEntitiesHack
 
   # Associations
   belongs_to :venue, :counter_cache => true
   belongs_to :source
 
   # Validations
-  validates_presence_of :title, :start_time
-  validate :end_time_later_than_start_time
+  validates :title, :description, :url, blacklist: true
+  validates :start_time, :end_time, sequential: true
+  validates :title, :start_time, presence: true
   validates_format_of :url,
     :with => /\Ahttps?:\/\/(\w+:?\w*@)?(\S+)(:[0-9]+)?(\/|\/([\w#!:.?+=&%@!\-\/]))?\Z/,
     :allow_blank => true,
     :allow_nil => true
 
-  validates :title, :description, :url, blacklist: true
-
-  before_destroy :verify_lock_status
+  before_destroy { !locked } # prevent locked events from being destroyed
 
   # Duplicates
   include DuplicateChecking
@@ -75,7 +76,7 @@ class Event < ActiveRecord::Base
   }
   scope :before_date, lambda { |date|
     time = date.beginning_of_day
-    where("start_time < :time", :time => time).order(:start_time)
+    where("start_time < :time", :time => time).order(start_time: :desc)
   }
   scope :future, lambda { on_or_after_date(Time.zone.today) }
   scope :past, lambda { before_date(Time.zone.today) }
@@ -86,39 +87,21 @@ class Event < ActiveRecord::Base
     on_or_after_date(start_date).before_date(end_date)
   }
 
-  scope :future_with_venue, -> {
-    future.order("start_time ASC").non_duplicates.includes(:venue)
-  }
-  scope :past_with_venue, -> {
-    past.order("start_time DESC").non_duplicates.includes(:venue)
-  }
-
   # Expand the simple sort order names from the URL into more intelligent SQL order strings
-  scope :ordered_by_ui_field, lambda{|ui_field|
-    case ui_field
-      when 'name'
-        order('lower(events.title), start_time')
-      when 'venue'
-        includes(:venue).order('lower(venues.title), start_time').references(:venues)
-      else # when 'date', nil
-        order('start_time')
+  scope :ordered_by_ui_field, lambda { |ui_field|
+    scope = case ui_field
+    when 'name'
+      order('lower(events.title)')
+    when 'venue'
+      includes(:venue).order('lower(venues.title)').references(:venues)
+    else
+      all
     end
+    scope.order('start_time')
   }
 
   #---[ Overrides ]-------------------------------------------------------
 
-  # Return the title but strip out any whitespace.
-  def title
-    # TODO Generalize this code so we can use it on other attributes in the different model classes. The solution should use an #alias_method_chain to make sure it's not breaking any explicit overrides for an attribute.
-    read_attribute(:title).to_s.strip
-  end
-
-  # Return description without those pesky carriage-returns.
-  def description
-    # TODO Generalize this code so we can reuse it on other attributes.
-    read_attribute(:description).to_s.gsub("\r\n", "\n").gsub("\r", "\n")
-  end
-
   def url=(value)
     super UrlPrefixer.prefix(value)
   end
@@ -142,13 +125,14 @@ def end_time=(value)
   end
 
   def time_for(value)
-    value = value.join(' ') if value.kind_of?(Array)
     value = value.to_s if value.kind_of?(Date)
     value = Time.zone.parse(value) if value.kind_of?(String) # this will throw ArgumentError if invalid
     value
   end
   private :time_for
 
+  #---[ Lock toggling ]---------------------------------------------------
+
   def lock_editing!
     update_attribute(:locked, true)
   end
@@ -159,9 +143,6 @@ def unlock_editing!
 
   #---[ Searching ]-------------------------------------------------------
 
-  # NOTE: The `Event.search` method is implemented elsewhere! For example, it's
-  # added by SearchEngine::ActsAsSolr if you're using that search engine.
-
   def self.search_tag(tag, opts={})
     includes(:venue).tagged_with(tag).ordered_by_ui_field(opts[:order])
   end
@@ -170,61 +151,24 @@ def self.search(query, opts={})
     SearchEngine.search(query, opts)
   end
 
-  #---[ Transformations ]-------------------------------------------------
-
-  def location
-    venue && venue.location
-  end
-
-  def venue_title
-    venue && venue.title
-  end
-
   #---[ Date related ]----------------------------------------------------
 
-  # Returns an array of the dates spanned by the event.
-  def dates
-    raise ArgumentError, "can't get dates for an event with no start time" unless start_time
-    if end_time
-      (start_time.to_date..end_time.to_date).to_a
-    else
-      [start_time.to_date]
-    end
-  end
-
-  # Is this event current?
   def current?
     (end_time || start_time) >= Date.today.to_time
   end
 
-  # Is this event old?
   def old?
     (end_time || start_time + 1.hour) <= Time.zone.now.beginning_of_day
   end
 
-  # Did this event start before today but ends today or later?
   def ongoing?
-    start_time < Date.today.to_time && end_time && end_time >= Date.today.to_time
+    return unless end_time && start_time
+    (start_time..end_time).cover?(Date.today.to_time)
   end
 
   def duration
-    if end_time && start_time
-      (end_time - start_time)
-    else
-      0
-    end
-  end
-
-protected
-
-  def end_time_later_than_start_time
-    if start_time && end_time && end_time < start_time
-      errors.add(:end_time, "cannot be before start")
-    end
-  end
-
-  def verify_lock_status
-    return !locked
+    return 0 unless end_time && start_time
+    (end_time - start_time)
   end
 end
 

app/models/calagator/event/ical_renderer.rb

@@ -100,20 +100,20 @@ def url
     end
 
     def location
-      [event.venue_title, event.venue.full_address].compact.join(": ") if event.venue
+      [event.venue.title, event.venue.full_address].compact.join(": ") if event.venue
     end
 
     def dtstart
       if multiday?
-        event.dates.first
+        event.start_time.to_date
       else
         event.start_time
       end
     end
 
     def dtend
       if multiday?
-        event.dates.last + 1.day
+        event.end_time.to_date + 1.day
       else
         event.end_time || event.start_time + 1.hour
       end
@@ -149,7 +149,7 @@ def uid
 
     # Treat any event with a duration of at least 20 hours as a multiday event.
     def multiday?
-      event.dates.size > 1 && event.duration.seconds > 20.hours
+      event.duration.seconds > 20.hours
     end
   end
 end

app/models/calagator/event/saver.rb

@@ -5,8 +5,8 @@ class Saver < Struct.new(:event, :params, :failure)
     def save
       event.attributes = params[:event] || {}
       event.venue      = find_or_initialize_venue
-      event.start_time = [ params[:start_date], params[:start_time] ]
-      event.end_time   = [ params[:end_date], params[:end_time] ]
+      event.start_time = [ params[:start_date], params[:start_time] ].join(' ')
+      event.end_time   = [ params[:end_date], params[:end_time] ].join(' ')
       event.tags.reload # Reload the #tags association because its members may have been modified when #tag_list was set above.
 
       attempt_save?

app/models/calagator/event/search_engine/apache_sunspot.rb

@@ -36,8 +36,8 @@ def self.configure
           time :start_time
           time :end_time
 
-          text :venue_title
-          string :venue_title
+          text(:venue_title) { venue.try(:title) }
+          string(:venue_title) { venue.try(:title) }
 
           boolean(:duplicate) { |event| event.duplicate? }
         end

app/models/calagator/venue.rb

@@ -49,9 +49,7 @@ class Venue < ActiveRecord::Base
   include DecodeHtmlEntitiesHack
 
   # Associations
-  has_many :events, dependent: :nullify
-  def future_events; events.future_with_venue; end
-  def past_events; events.past_with_venue; end
+  has_many :events, -> { non_duplicates }, dependent: :nullify
   belongs_to :source
 
   # Triggers

app/views/calagator/events/_item.html.erb

@@ -19,7 +19,7 @@ html_classes << " contentbar" if has_contentbar
     <div class='date'><%= normalize_time(event) -%></div>
       <% unless event.venue.blank? %>
       <div class="clearfix location p-location h-card vcard<%= " closed" if event.venue.closed? %>"  itemprop="location" itemscope itemtype="http://schema.org/Place">
-          <%= map event %>
+          <%= map event.venue %>
           <a class="url u-url" href='<%= event.venue.new_record? ? "#" : venue_url(event.venue) %>'>
             <span class='fn org p-name' itemprop="name"><%= event.venue.title -%></span>
           </a>

app/views/calagator/venues/show.html.erb

@@ -100,12 +100,12 @@
   <div id='events'>
     <div id='future_events'>
       <h2>Future events happening here</h2>
-      <%= render :partial => 'calagator/events/list', :locals => { :events => @venue.future_events } %>
+      <%= render partial: 'calagator/events/list', locals: { events: @venue.events.future} %>
     </div>
 
     <div id='past_events'>
       <h2>Past events that happened here</h2>
-      <%= render :partial => 'calagator/events/list', :locals => { :events => @venue.past_events } %>
+      <%= render partial: 'calagator/events/list', locals: { events: @venue.events.past } %>
     </div>
   </div>
 </div>

lib/active_model/sequential_validator.rb

@@ -0,0 +1,11 @@
+class SequentialValidator < ActiveModel::Validator
+  def validate record
+    values = options[:attributes].map do |attribute|
+      record.send(attribute)
+    end.compact
+    if values.sort != values
+      record.errors.add options[:attributes].last, "cannot be before #{options[:attributes].first}"
+    end
+  end
+end
+

spec/models/calagator/event_spec.rb

@@ -222,11 +222,6 @@ module Calagator
       expect(event.start_time).to eq Time.zone.parse("2009-01-02 03:45")
     end
 
-    it "should set from an Array of Strings" do
-      event = Event.new(:start_time => ["2009-01-03", "02:14"])
-      expect(event.start_time).to eq Time.zone.parse("2009-01-03 02:14")
-    end
-
     it "should set from Date" do
       event = Event.new(:start_time => Date.parse("2009-02-01"))
       expect(event.start_time).to eq Time.zone.parse("2009-02-01")
@@ -260,11 +255,6 @@ module Calagator
       expect(event.end_time).to eq Time.zone.parse("2009-01-02 03:45")
     end
 
-    it "should set from an Array of Strings" do
-      event = Event.new(:end_time => ["2009-01-03", "02:14"])
-      expect(event.end_time).to eq Time.zone.parse("2009-01-03 02:14")
-    end
-
     it "should set from Date" do
       event = Event.new(:end_time => Date.parse("2009-02-01"))
       expect(event.end_time).to eq Time.zone.parse("2009-02-01")
@@ -281,26 +271,6 @@ module Calagator
     end
   end
 
-  describe "#dates" do
-    it "returns an array of dates spanned by the event" do
-      event = Event.new(start_time: "2010-01-01", end_time: "2010-01-03")
-      expect(event.dates).to eq([
-        Date.parse("2010-01-01"),
-        Date.parse("2010-01-02"),
-        Date.parse("2010-01-03"),
-      ])
-    end
-
-    it "returns an array of one date when there is no end time" do
-      event = Event.new(start_time: "2010-01-01")
-      expect(event.dates).to eq([Date.parse("2010-01-01")])
-    end
-
-    it "throws ArgumentError when there is no start time" do
-      expect { Event.new.dates }.to raise_error(ArgumentError)
-    end
-  end
-
   describe "#duration" do
     it "returns the event length in seconds" do
       event = Event.new(start_time: "2010-01-01", end_time: "2010-01-03")
@@ -312,14 +282,6 @@ module Calagator
     end
   end
 
-  describe "#location" do
-    it "delegates to the venue's location" do
-      event = Event.new
-      event.build_venue latitude: 45.5200, longitude: 122.6819
-      expect(event.location).to eq([45.5200, 122.6819])
-    end
-  end
-
   describe ".search_tag" do
     before do
       @c = FactoryGirl.create(:event, title: "c", tag_list: ["tag", "wtf"], start_time: 3.minutes.ago)
@@ -667,27 +629,6 @@ module Calagator
     end
   end
 
-  describe "when normalizing line-endings in the description" do
-    before do
-      @event = Event.new
-    end
-
-    it "should not molest contents without carriage-returns" do
-      @event.description         = "foo\nbar"
-      expect(@event.description).to eq "foo\nbar"
-    end
-
-    it "should replace CRLF with LF" do
-      @event.description         = "foo\r\nbar"
-      expect(@event.description).to eq "foo\nbar"
-    end
-
-    it "should replace stand-alone CR with LF" do
-      @event.description         = "foo\rbar"
-      expect(@event.description).to eq "foo\nbar"
-    end
-  end
-
   describe "when converting to iCal" do
     def ical_roundtrip(events, opts = {})
       parsed_events = RiCal.parse_string(Event::IcalRenderer.render(events, opts)).first.events