identifying conflicting appointments in rails works for update but not create?

Question

I wrote this conflict method to check if an appointment being saved or created dosent conflict with one thats already saved for a particular trainer. this method is called first thing after a person attempts to create or update existing appointments in the def create and def update.

it works when updating the appointment but dosent identify the conflict when creating.

any ideas?

  def is_conflicting()
    @new_appointment = person.appointments.build(params[:appointment])
    @appointments = Appointment.all(:conditions => { :date_of_appointment => @new_appointment.date_of_appointment, :doctor_id => @new_appointment.doctor_id})
    @appointments.each do |appointment|
      logger.info( appointment)
        if(@new_appointment.start_time < appointment.end_time && appointment.start_time < @new_appointment.end_time)
          return true 
        end
    end
    return false
  end

def create
    @appointment = person.appointments.build(params[:appointment])
      respond_to do |format|
        if(is_conflicting == false)
        if @appointment.save
....more code...
        end
        end
      end 
end

  def update
    @appointment = person.appointments.find(params[:id])
      respond_to do |format|
        if(is_conflicting == false)
          if @appointment.update_attributes(params[:appointment])
        .....more code...........
           end
         end
     end
   end

the part of the form where doctor gets set.

  <p>
    <%= f.label :doctor_id %>
    <%= f.select :doctor_id, Doctor.find(:all, :order => "name").collect { |s|
        [s.name, s.id]} %>
  </p>

thank you

Answer 1

You're creating an impossible condition. Your condition says @new_appointment must have a start_time after appointment's end_time, and an end_time before appointment's start_time...this is logically impossible.

I'd suggest using this: http://api.rubyonrails.org/classes/ActiveSupport/CoreExtensions/Range/Overlaps.html

You will need to create ranges based on the start and end times doing something like @new_appointment.start_time..@new_appointment.end_time

Answer 2

build only operates on objects already persisted in the database: see here: http://apidock.com/rails/ActiveRecord/Associations/ClassMethods and here Ruby on Rails. How do I use the Active Record .build method in a :belongs to relationship?

also, your code could still use some refactoring. your if statement will either return true or false, so why are you bothering to specifically return true. Also, you dont need empty parens and you should define your methods that return a boolean as ending with a question mark. Finally, why are you building a new appointment in the create method, then making a new object in the validation method?

def conflicting? appointment
  @appointments = Appointment.all(:conditions => {... all of them})
  # Enumerable#any? returns true or false for the collection, 
  # so you dont have to specify a return value 
  # since its the last evaluation in the method
  @appointments.any?{|apt| appointment.start_time < apt.end_time && apt.start_time < appointment.end_time} #=> takes each appointment in appointments assigns to apt and checks against the passed in appointment object
end

and then in your create or update method

# assuming start/end times are form parameters coming from a view
@appointment = Appointment.new params[:appointment]
# substituting the lookup and update_attributes in the update action, obviously
@appointment.save unless conflicting? @appointment

Answer 3

I think what you would want to do is push this down to a validation on the Appointment Model. See http://api.rubyonrails.org/classes/ActiveRecord/Validations.html#M001391.

In the snippet below appt_range builds a range from start to end time, and the the validate method should be called on create/update.

Perhaps something like.

class Appointment < ActiveRecord::Base

  def appt_range
    start_time..end_time 
   end

   ...rest of code...
   protected
   def validate
     @appointments = Appointment.all(:conditions => { :date_of_appointment => date_of_appointment, :doctor_id => doctor_id})
     errors.add_to_base("Appointment Conflict") if @appointments.any? {|appt| appt.appt_range.overlaps? appt_range}
  end 
end

and then your controller would have

 def create
 @appointment = person.appointments.new(params[:appointment]))
    if @appointment.save
       ...
    end
end 

def update
    @appointment = person.appointments.find(params[:id])
    if @appointment.update_attributes(params[:appointment])
    ...
    end
end

But that being said (and this is a problem in your original code as well), there is a race condition/issue. Suppose a patient has an appt from 10:00 -> 10:30, and they want to move it to 10:15->10:45. The update will fail since the Dr is already booked for the patient at that time. Perhaps adding patient_id not current patient would solve that edge case, but your tests should cover that possibility .

Also I just whipped this up out of my head, and haven't tested it, so your mileage may vary (you didn't specify version of rails,, but from the code. looks to 2.3.x?). But hopefully this points you in the better direction ..

Edit...

I built a barebones/simple rails 2.3.8 app, to test it out, and it appears to work on create. take a look at http://github.com/doon/appt_test I included the dev db as well.

 rake db:migrate                                                                                                                                                  
==  CreateAppointments: migrating =============================================
-- create_table(:appointments)
   -> 0.0019s
==  CreateAppointments: migrated (0.0020s) ====================================

Loading development environment (Rails 2.3.8)
ruby-1.8.7-p299 > a=Appointment.new(:patient_id=>1, :doctor_id=>1, :date_of_appointment=>'08/10/2010', :start_time=>" 2010-08-10 8:00", :end_time=>"2010-08-10 10:00")
 => #<Appointment id: nil, patient_id: 1, doctor_id: 1, date_of_appointment: "2010-08-10", start_time: "2000-01-01 08:00:00", end_time: "2000-01-01 10:00:00", created_at: nil, updated_at: nil> 
ruby-1.8.7-p299 > a.save
  Appointment Load (0.2ms)   SELECT * FROM "appointments" WHERE ("appointments"."doctor_id" = 1 AND "appointments"."date_of_appointment" = '2010-08-10') 
  Appointment Create (0.5ms)   INSERT INTO "appointments" ("end_time", "created_at", "updated_at", "patient_id", "doctor_id", "date_of_appointment", "start_time") VALUES('2000-01-01 10:00:00', '2010-08-07 22:20:33', '2010-08-07 22:20:33', 1, 1, '2010-08-10', '2000-01-01 08:00:00')
 => true 
ruby-1.8.7-p299 > b=Appointment.new(:patient_id=>1, :doctor_id=>1, :date_of_appointment=>'08/10/2010', :start_time=>" 2010-08-10 9:00", :end_time=>"2010-08-10 11:00")
 => #<Appointment id: nil, patient_id: 1, doctor_id: 1, date_of_appointment: "2010-08-10", start_time: "2000-01-01 09:00:00", end_time: "2000-01-01 11:00:00", created_at: nil, updated_at: nil> 
ruby-1.8.7-p299 > b.save
  Appointment Load (0.4ms)   SELECT * FROM "appointments" WHERE ("appointments"."doctor_id" = 1 AND "appointments"."date_of_appointment" = '2010-08-10') 
 => false 
ruby-1.8.7-p299 > b.errors['base']
 => "Appointment Conflict" 
ruby-1.8.7-p299 > c=Appointment.new(:patient_id=>1, :doctor_id=>1, :date_of_appointment=>'08/10/2010', :start_time=>" 2010-08-10 11:00", :end_time=>"2010-08-10 12:00")
 => #<Appointment id: nil, patient_id: 1, doctor_id: 1, date_of_appointment: "2010-08-10", start_time: "2000-01-01 11:00:00", end_time: "2000-01-01 12:00:00", created_at: nil, updated_at: nil> 
ruby-1.8.7-p299 > c.save
  Appointment Load (0.3ms)   SELECT * FROM "appointments" WHERE ("appointments"."doctor_id" = 1 AND "appointments"."date_of_appointment" = '2010-08-10') 
  Appointment Create (0.4ms)   INSERT INTO "appointments" ("end_time", "created_at", "updated_at", "patient_id", "doctor_id", "date_of_appointment", "start_time") VALUES('2000-01-01 12:00:00', '2010-08-07 22:21:39', '2010-08-07 22:21:39', 1, 1, '2010-08-10', '2000-01-01 11:00:00')
 => true 

and here is my Appointment class (I used the validate :symbol method)

class Appointment < ActiveRecord::Base

  validate :conflicting_appts

  def appt_range
    start_time..end_time 
  end


  private
  def conflicting_appts
    @appointments = Appointment.all(:conditions => { :date_of_appointment => date_of_appointment, :doctor_id => doctor_id})
    errors.add_to_base("Appointment Conflict") if @appointments.any? {|appt| appt.appt_range.overlaps? appt_range}
  end
end

Also in playing around with this, though of another case you should be sure to test for. patient A has appt with dr A from 10-11. Patient B has appt with Dr a from 11-12. These will overlap in the current implementation since they share 11 in common, and will be marked as conflict.

So I am not sure why it isn't working on create, if you want to show your code we can look at it.

Ok I figured out why it isn't working, and it has to do with the start and end time. Take a look at this.

from testing... (adding a logger inside the validation shows me this).

appt.range == Sat Jan 01 09:06:00 UTC 2000..Sat Jan 01 21:10:00 UTC 2000 , my range = Tue Aug 10 09:15:00 UTC 2010..Tue Aug 10 14:19:00 UTC 2010
appt.range == Sat Jan 01 22:06:00 UTC 2000..Sat Jan 01 23:06:00 UTC 2000 , my range = Tue Aug 10 09:15:00 UTC 2010..Tue Aug 10 14:19:00 UTC 2010
appt.range == Sat Jan 01 09:30:00 UTC 2000..Sat Jan 01 12:14:00 UTC 2000 , my range = Tue Aug 10 09:15:00 UTC 2010..Tue Aug 10 14:19:00 UTC 2010
appt.range == Sat Jan 01 09:31:00 UTC 2000..Sat Jan 01 12:20:00 UTC 2000 , my range = Tue Aug 10 09:15:00 UTC 2010..Tue Aug 10 14:19:00 UTC 2010

What is happening is the date part is getting truncated off and set to Jan 1, 2000, when you pull it from the DB.. So when you are querying against the database the ranges aren't going to overlap you are looking for dates in 2010. Making the start/end times a datetime would solve the issue, as then the date would be significant again. Else need to modify the appt_range to adjust the date back to date_of_appointment. It doesn't happen on update since you are dealing with all data from the db. so everything has the same date on it

see http://github.com/doon/EMR/commit/b453bb3e70b5b6064bb8693cf3986cf2219fbad5

def appt_range
   s=Time.local(date_of_appointment.year, date_of_appointment.month, date_of_appointment.day, start_time.hour, start_time.min, start_time.sec)
   e=Time.local(date_of_appointment.year, date_of_appointment.month, date_of_appointment.day, end_time.hour, end_time.min, end_time.sec)
  s..e
end

fixes it by coercing start and end time into using the date_of_appointment...

Answer 4

The number of if else statements in there hurt to look at. I can't tell what the logic is. Try using the enumerable class for this kind of work. You probably just have an error in your nesting. I've found that return true can often have undesirable results, like always returning true even when you think the logic is solid.

jruby-1.5.0 > apt1 = (1.hour.from_now..2.hour.from_now) #=> Fri, 06 Aug 2010 01:58:43 UTC +00:00..Fri, 06 Aug 2010 02:58:43 UTC +00:00 

jruby-1.5.0 > apt2 = (3.hour.from_now..4.hour.from_now)#=> Fri, 06 Aug 2010 03:59:07 UTC +00:00..Fri, 06 Aug 2010 04:59:07 UTC +00:00 

jruby-1.5.0 > @appointments = [apt1, apt2]

jruby-1.5.0 > @appointments.any?{|apt| (5.hour.from_now..6.hour.from_now).overlaps? apt } #=> false 

jruby-1.5.0 > @appointments.any?{|apt| (1.hour.from_now..3.hour.from_now).overlaps? apt } #=> true