Andrew
Andrew

Reputation: 43113

Rails: controller won't update model correctly

I apologize in advance, this is going to be a long question.

Short version:

I have a Meeting model that has a date, start_time, and end_time. These are time objects, which of course are a pain for users to input, so I'm using virtual attributes to accept strings which are parsed by Chronic before save.

I have a plain vanilla rails controller that receives these virtual attributes from the form and passes them along to the model. Here is the controller:

def create
  @meeting = @member.meetings.build(params[:meeting])
  if @meeting.save
    redirect_to member_meetings_path(@member), :notice => "Meeting Added"
  else
    render :new
  end
end

def update
  @meeting = @member.meetings.find(params[:id])
  if @meeting.update_attributes(params[:meeting])
    redirect_to member_meetings_path(@member), :notice => "Meeting Updated"
  else
    render :new
  end
end

I've verified that the controller receives the correct parameters from the form, for instance params[:meeting][:date_string] is set as expected.

Problems:

On create, the date gets set correctly, but the times are assigned to the year 2000, set in UTC, and won't display in local time on the front end.

On update, the date won't update. The times update but stay in UTC for 2000-01-01.

Longer Version

What makes this super bizarre to me is I have decent test coverage indicating all of this works at the model layer.

Here is the model:

# DEPENDENCIES
require 'chronic'
class Meeting < ActiveRecord::Base
  # MASS ASSIGNMENT PROTECTION
  attr_accessible :name, :location, :description, :contact_id, :member_id, :time_zone, 
                  :date, :start_time, :end_time, :date_string, :start_time_string, :end_time_string

  # RELATIONSHIPS
  belongs_to :member
  belongs_to :contact

  # CALLBACKS
  before_save :parse_time

  # Time IO Formatting
  attr_writer :date_string, :start_time_string, :end_time_string

  # Display time as string, year optional    
  def date_string(year=true)
    if date
      str = "%B %e"
      str += ", %Y" if year
      date.strftime(str).gsub('  ',' ')
    else
      ""
    end
  end

  # Display time as string, AM/PM optional
  def start_time_string(meridian=true)
    if start_time
      str = "%l:%M"
      str += " %p" if meridian
      start_time.strftime(str).lstrip
    else
      ""
    end
  end

  # Display time as string, AM/PM optional    
  def end_time_string(meridian=true)
    if end_time
      str = "%l:%M"
      str += " %p" if meridian
      end_time.strftime(str).lstrip
    else
      ""
    end
  end

  # Display Date and Time for Front-End    
  def time
    date.year == Date.today.year ? y = false : y = true
    start_time.meridian != end_time.meridian ? m = true : m = false
    [date_string(y),'; ',start_time_string(m),' - ',end_time_string].join
  end

  private
    # Time Input Processing, called in `before_save`
    def parse_time
      set_time_zone
      self.date ||= @date_string ? Chronic.parse(@date_string).to_date : Date.today
      self.start_time = Chronic.parse @start_time_string, :now => self.date
      self.end_time = Chronic.parse @end_time_string, :now => self.date
    end

    def set_time_zone
      if time_zone
        Time.zone = time_zone
      elsif member && member.time_zone
        Time.zone = member.time_zone
      end
      Chronic.time_class = Time.zone
    end

end

Here is the spec. Note that to test the parse_time callback in isolation I'm calling @meeting.send(:parse_time) in these tests whenever I'm not actually creating or updating a record.

require "minitest_helper"

describe Meeting do
  before do
    @meeting = Meeting.new
  end

  describe "accepting dates in natural language" do
    it "should recognize months and days" do
      @meeting.date_string = 'December 17'
      @meeting.send(:parse_time)
      @meeting.date.must_equal Date.new(Time.now.year,12,17)
    end

    it "should assume a start time is today" do
      @meeting.start_time_string = '1pm'
      @meeting.send(:parse_time)
      @meeting.start_time.must_equal Time.zone.local(Date.today.year,Date.today.month,Date.today.day, 13,0,0)
    end

    it "should assume an end time is today" do
      @meeting.end_time_string = '3:30'
      @meeting.send(:parse_time)
      @meeting.end_time.must_equal Time.zone.local(Date.today.year,Date.today.month,Date.today.day, 15,30,0)
    end

    it "should set start time to the given date" do
      @meeting.date = Date.new(Time.now.year,12,1)
      @meeting.start_time_string = '4:30 pm'
      @meeting.send(:parse_time)
      @meeting.start_time.must_equal Time.zone.local(Time.now.year,12,1,16,30)
    end

    it "should set end time to the given date" do
      @meeting.date = Date.new(Time.now.year,12,1)
      @meeting.end_time_string = '6pm'
      @meeting.send(:parse_time)
      @meeting.end_time.must_equal Time.zone.local(Time.now.year,12,1,18,0)
    end
  end

  describe "displaying time" do
    before do
      @meeting.date = Date.new(Date.today.year,12,1)
      @meeting.start_time = Time.new(Date.today.year,12,1,16,30)
      @meeting.end_time = Time.new(Date.today.year,12,1,18,0)
    end

    it "should print a friendly time" do
      @meeting.time.must_equal "December 1; 4:30 - 6:00 PM"
    end
  end

  describe "displaying if nil" do
    it "should handle nil date" do
      @meeting.date_string.must_equal ""
    end

    it "should handle nil start_time" do
      @meeting.start_time_string.must_equal ""
    end

    it "should handle nil end_time" do
      @meeting.end_time_string.must_equal ""
    end
  end

  describe "time zones" do
    before do
      @meeting.assign_attributes(
        time_zone: 'Central Time (US & Canada)',
        date_string: "December 1, #{Time.now.year}",
        start_time_string: "4:30 PM",
        end_time_string: "6:00 PM"
      )
      @meeting.save
    end

    it "should set meeting start times in the given time zone" do
      Time.zone = 'Central Time (US & Canada)'
      @meeting.start_time.must_equal Time.zone.local(Time.now.year,12,1,16,30)
    end

    it "should set the correct UTC offset" do
      @meeting.start_time.utc_offset.must_equal -(6*60*60)
    end

    after do
      @meeting.destroy
    end
  end

  describe "updating" do
    before do
      @m = Meeting.create(
        time_zone: 'Central Time (US & Canada)',
        date_string: "December 1, #{Time.now.year}",
        start_time_string: "4:30 PM",
        end_time_string: "6:00 PM"
      )
      @m.update_attributes start_time_string: '2pm', end_time_string: '3pm'
      Time.zone = 'Central Time (US & Canada)'
    end

    it "should update start time via mass assignment" do
      @m.start_time.must_equal Time.zone.local(Time.now.year,12,1,14,00)
    end

    it "should update end time via mass assignment" do
      @m.end_time.must_equal Time.zone.local(Time.now.year,12,1,15,00)
    end

    after do
      @m.destroy
    end
  end

end

I have even specifically mixed in creating and updating records via mass assignment in later test methods to ensure that those work as expected. All those tests pass.

I appreciate any insight into the following:

  1. Why doesn't the date update in the controller#update action?

  2. Why aren't times getting the year from the date that is set? This works in the model and in specs, but not when submitted via form through the controller.

  3. Why don't times get set to the time zone that is passed in from the form? Again, these specs pass, what is wrong on the controller?

  4. Why won't times display in their time zone on the front end?

Thanks for the help, I feel like I must be losing the forest for the trees on this one as I've been going at it for hours.


Update:

Thanks to the help of AJcodez, I saw some of the issues:

  1. Was assigning date wrong, thanks AJ! Now using:

    if @date_string.present?
        self.date = Chronic.parse(@date_string).to_date
    elsif self.date.nil?
        self.date = Date.today
    end
    
  2. I was using Chronic correctly, my mistake was at the database layer! I set the fields in the database to time instead of datetime, which ruins everything. Lesson to anyone reading this: never ever use time as a database field (unless you understand exactly what it does and why you're using it instead of datetime).

  3. Same problem as above, changing the fields to datetime fixed the problem.

  4. The problem here has to do with accessing time in the model vs. the view. If I move these time formatting methods into a helper so they're called in the current request scope they will work correctly.

Thanks AJ! Your suggestions got me past my blind spot.

Upvotes: 3

Views: 767

Answers (1)

AJcodez
AJcodez

Reputation: 34166

Well here goes..

1 . Why doesn't the date update in the controller#update action?

I see two potential issues. Looks like you're not parsing the dates again. Try this:

def update
  @meeting = @member.meetings.find(params[:id])
  @meeting.assign_attributes params[:meeting]
  @meeting.send :parse_time
  if @meeting.save
  ...

assign_attributes sets but doesnt save new values: http://apidock.com/rails/ActiveRecord/AttributeAssignment/assign_attributes

Also, in your parse_time method, you use this assignment: self.date ||= which will always set self.date back to itself if it is assigned. In other words you can't update the date unless its falsey.


2 . Why aren't times getting the year from the date that is set? This works in the model and in specs, but not when submitted via form through the controller.

No idea, looks like you are using Chronic#parse correctly.


3 . Why don't times get set to the time zone that is passed in from the form? Again, these specs pass, what is wrong on the controller?

Try debugging time_zone and make sure it is returning whats in params[:meeting][:time_zone]. Again it looks correct by Chronic.

Side note: if you pass an invalid string to Time#zone= it will blow up with an error. For instance Time.zone = 'utc' is all bad.


4 . Why won't times display in their time zone on the front end?

See Time#in_time_zone http://api.rubyonrails.org/classes/Time.html#method-i-in_time_zone and just explicitly name your time zone every time.

Not sure if you're already doing this, but try to explicitly save Times in UTC on the database, and then display them in local time.

Upvotes: 1

Related Questions