Merge pull request #124 from oliverguenther/accessibility/date-validation

Fix start_date and hours validation for meetings

app/controllers/meetings_controller.rb

@@ -74,7 +74,7 @@ class MeetingsController < ApplicationController
 
       redirect_to action: 'show', id: @meeting
     else
-      render action: 'new', project_id: @project
+      render template: 'meetings/new', project_id: @project
     end
   end
 
@@ -145,15 +145,6 @@ class MeetingsController < ApplicationController
     # instance variable.
     @converted_params = meeting_params
 
-    start_date = @converted_params.delete(:start_date)
-    start_time_hour = @converted_params.delete(:"start_time_hour")
-    begin
-      timestring = "#{start_date} #{start_time_hour}"
-      time = Time.zone.parse(timestring)
-      @converted_params[:start_time] = time
-    rescue ArgumentError
-      @converted_params[:start_time] = nil
-    end
     @converted_params[:duration] = @converted_params[:duration].to_hours
     # Force defaults on participants
     @converted_params[:participants_attributes] ||= {}

app/models/meeting.rb

@@ -61,7 +61,13 @@ class Meeting < ActiveRecord::Base
 
   accepts_nested_attributes_for :participants, allow_destroy: true
 
-  validates_presence_of :title, :start_time, :duration
+  validates_presence_of :title, :duration
+
+  # We only save start_time as an aggregated value of start_date and hour,
+  # but still need start_date and _hour for validation purposes
+  attr_reader :start_date, :start_time_hour
+  validate :validate_date_and_time
+  before_save :update_start_time!
 
   before_save :add_new_participants_as_watcher
 
@@ -71,13 +77,30 @@ class Meeting < ActiveRecord::Base
     Meeting.where(['author_id = ?', user.id]).update_all ['author_id = ?', DeletedUser.first.id]
   end
 
-  def start_date
-    # the text_field + calendar_for form helpers expect a Date
-    start_time.to_date if start_time.present?
+  ##
+  # Assign a date string without validation
+  # The actual aggregated start_time is derived after valdiation
+  def start_date=(value)
+    attribute_will_change! :start_date
+    @start_date = value
+  end
+
+  ##
+  # Assign a HH:MM hour string without validation
+  # The actual aggregated start_time is derived after valdiation
+  def start_time_hour=(value)
+    attribute_will_change! :start_time_hour
+    @start_time_hour = value
   end
 
-  def start_time_hour
-    start_time.present? ? start_time.strftime('%H:%M') : '00:00'
+  ##
+  # Return the computed start_time when changed
+  def start_time
+    if parse_start_time?
+      parsed_start_time
+    else
+      super
+    end
   end
 
   def start_month
@@ -144,7 +167,7 @@ class Meeting < ActiveRecord::Base
 
       objs.group_by(&:start_month).each do |month, objs|
 
-        objs.group_by(&:start_date).each do |date, objs|
+        objs.group_by(&:start_time).each do |date, objs|
 
           by_start_year_month_date[year][month][date] = objs
 
@@ -174,12 +197,76 @@ class Meeting < ActiveRecord::Base
 
   def set_initial_values
     # set defaults
-    self.start_time ||= Date.tomorrow + 10.hours
+    write_attribute(:start_time, Date.tomorrow + 10.hours) if start_time.nil?
     self.duration   ||= 1
+
+    @start_date = start_time.to_date.iso8601
+    @start_time_hour = start_time.strftime('%H:%M')
   end
 
   private
 
+  ##
+  # Validate date and time setters.
+  # If start_time has been changed, check that value.
+  # Otherwise start_{date, time_hour} was used, then validate those
+  def validate_date_and_time
+    if parse_start_time?
+      errors.add :start_date, :not_an_iso_date if parsed_start_date.nil?
+      errors.add :start_time_hour, :invalid_time_format if parsed_start_time_hour.nil?
+    else
+      errors.add :start_time, :invalid if start_time.nil?
+    end
+  end
+
+  ##
+  # Actually sets the aggregated start_time attribute.
+  def update_start_time!
+    write_attribute(:start_time, start_time)
+  end
+
+  ##
+  # Determines whether new raw values werde provided.
+  def parse_start_time?
+    !(changed & %w(start_date start_time_hour)).empty?
+  end
+
+  ##
+  # Returns the parse result of both start_date and start_time_hour
+  def parsed_start_time
+    date = parsed_start_date
+    time = parsed_start_time_hour
+
+    if date.nil? || time.nil?
+      raise ArgumentError, 'Provided composite start_time is invalid.'
+    end
+
+    DateTime.new(
+      date.year,
+      date.month,
+      date.day,
+      time.hour,
+      time.min
+    )
+  end
+
+  ##
+  # Enforce ISO 8601 date parsing for the given input string
+  # This avoids weird parsing of dates due to malformed input.
+  def parsed_start_date
+    Date.iso8601(@start_date)
+  rescue ArgumentError
+    nil
+  end
+
+  ##
+  # Enforce HH::MM time parsing for the given input string
+  def parsed_start_time_hour
+    Time.strptime(@start_time_hour, '%H:%M')
+  rescue ArgumentError
+    nil
+  end
+
   def add_new_participants_as_watcher
     participants.select(&:new_record?).each do |p|
       add_watcher(p.user)

config/locales/en.yml

@@ -29,6 +29,10 @@ en:
         participants_attended: "Attendees"
         participants_invited: "Invitees"
         start_time: "Time"
+        start_time_hour: "Starting time"
+    errors:
+      messages:
+        invalid_time_format: "is not a valid time. Required format: HH:MM"
     models:
       meeting_agenda: "Agenda"
       meeting_minutes: "Minutes"

spec/controllers/meetings_controller_spec.rb

@@ -21,8 +21,11 @@
 require File.dirname(__FILE__) + '/../spec_helper'
 
 describe MeetingsController, type: :controller do
-  before(:each) do
-    @p = mock_model(Project)
+  let(:project) { FactoryGirl.build :project }
+
+  before do
+    allow(Project).to receive(:find).and_return(project)
+
     allow(@controller).to receive(:authorize)
     allow(@controller).to receive(:check_if_login_required)
   end
@@ -30,10 +33,10 @@ describe MeetingsController, type: :controller do
   describe 'GET' do
     describe 'index' do
       before(:each) do
-        allow(Project).to receive(:find).and_return(@p)
         @ms = [mock_model(Meeting), mock_model(Meeting), mock_model(Meeting)]
         allow(@ms).to receive(:from_tomorrow).and_return(@ms)
-        allow(@p).to receive(:meetings).and_return(@ms)
+
+        allow(project).to receive(:meetings).and_return(@ms)
         [:with_users_by_date, :page, :per_page].each do |meth|
           expect(@ms).to receive(meth).and_return(@ms)
         end
@@ -42,7 +45,7 @@ describe MeetingsController, type: :controller do
       end
       describe 'html' do
         before(:each) do
-          get 'index', project_id: @p.id
+          get 'index', project_id: project.id
         end
         it { expect(response).to be_success }
         it { expect(assigns(:meetings_by_start_year_month_date)).to eql @grouped }
@@ -53,7 +56,7 @@ describe MeetingsController, type: :controller do
       before(:each) do
         @m = mock_model(Meeting)
         allow(Meeting).to receive_message_chain(:includes, :find).and_return(@m)
-        allow(@m).to receive(:project).and_return(@p)
+        allow(@m).to receive(:project).and_return(project)
         allow(allow(@m).to receive(:agenda)).to receive(:present?).and_return(false)
       end
       describe 'html' do
@@ -66,7 +69,7 @@ describe MeetingsController, type: :controller do
 
     describe 'new' do
       before(:each) do
-        allow(Project).to receive(:find).and_return(@p)
+        allow(Project).to receive(:find).and_return(project)
         @m = mock_model(Meeting)
         allow(@m).to receive(:project=)
         allow(@m).to receive(:author=)
@@ -74,7 +77,7 @@ describe MeetingsController, type: :controller do
       end
       describe 'html' do
         before(:each) do
-          get 'new', project_id: @p.id
+          get 'new', project_id: project.id
         end
         it { expect(response).to be_success }
         it { expect(assigns(:meeting)).to eql @m }
@@ -85,7 +88,7 @@ describe MeetingsController, type: :controller do
       before(:each) do
         @m = mock_model(Meeting)
         allow(Meeting).to receive_message_chain(:includes, :find).and_return(@m)
-        allow(@m).to receive(:project).and_return(@p)
+        allow(@m).to receive(:project).and_return(project)
       end
       describe 'html' do
         before(:each) do
@@ -95,5 +98,54 @@ describe MeetingsController, type: :controller do
         it { expect(assigns(:meeting)).to eql @m }
       end
     end
+
+    describe 'create' do
+      render_views
+
+      before do
+        allow(Project).to receive(:find).and_return(project)
+        post :create, project_id: project.id,
+                      meeting: {
+                        title: 'Foobar',
+                        duration: '1.0',
+                      }.merge(params)
+      end
+
+      describe 'invalid start_date' do
+        let(:params) {
+          {
+            start_date: '-',
+            start_time_hour: '10:00'
+          }
+        }
+
+        it 'renders an error' do
+          expect(response).to be_success
+          expect(response).to render_template :new
+          expect(response.body)
+            .to have_selector '#errorExplanation li',
+                              text: "Start date " +
+                                    I18n.t('activerecord.errors.messages.not_an_iso_date')
+        end
+      end
+
+      describe 'invalid start_time_hour' do
+        let(:params) {
+          {
+            start_date: '2015-06-01',
+            start_time_hour: '-'
+          }
+        }
+
+        it 'renders an error' do
+          expect(response).to be_success
+          expect(response).to render_template :new
+          expect(response.body)
+            .to have_selector '#errorExplanation li',
+                              text: "Starting time " +
+                                    I18n.t('activerecord.errors.messages.invalid_time_format')
+        end
+      end
+    end
   end
 end

spec/models/meeting_spec.rb

@@ -24,8 +24,6 @@ describe Meeting, type: :model do
   it { is_expected.to belong_to :project }
   it { is_expected.to belong_to :author }
   it { is_expected.to validate_presence_of :title }
-  it { is_expected.to validate_presence_of :start_time }
-  it { skip; is_expected.to accept_nested_attributes_for :participants } # geht das?
 
   let(:project) { FactoryGirl.create(:project) }
   let(:user1) { FactoryGirl.create(:user) }
@@ -47,7 +45,7 @@ describe Meeting, type: :model do
   end
 
   describe 'start_date' do
-    it { expect(@m.start_date).to eq(Date.tomorrow) }
+    it { expect(@m.start_date).to eq(Date.tomorrow.iso8601) }
   end
 
   describe 'start_month' do
@@ -62,8 +60,44 @@ describe Meeting, type: :model do
     it { expect(@m.end_time).to eq(Date.tomorrow + 11.hours) }
   end
 
-  describe 'time-sorted finder' do
-    it { skip }
+  describe 'date validations' do
+    it 'marks invalid start dates' do
+      @m.start_date = '-'
+      expect(@m.start_date).to eq('-')
+      expect { @m.start_time }.to raise_error(ArgumentError)
+      expect(@m).not_to be_valid
+      expect(@m.errors.count).to eq(1)
+    end
+
+    it 'marks invalid start hours' do
+      @m.start_time_hour = '-'
+      expect(@m.start_time_hour).to eq('-')
+      expect { @m.start_time }.to raise_error(ArgumentError)
+      expect(@m).not_to be_valid
+      expect(@m.errors.count).to eq(1)
+    end
+
+    it 'is not invalid when setting date_time explicitly' do
+      @m.start_time = DateTime.now
+      expect(@m).to be_valid
+    end
+
+    it 'is invalid when setting date_time wrong' do
+      @m.start_time = '-'
+      expect(@m).not_to be_valid
+    end
+
+    it 'accepts changes after invalid dates' do
+      @m.start_date = '-'
+      expect { @m.start_time }.to raise_error(ArgumentError)
+      expect(@m).not_to be_valid
+
+      @m.start_date = Date.today.iso8601
+      expect(@m).to be_valid
+
+      @m.save!
+      expect(@m.start_time).to eq(Date.today + 10.hours)
+    end
   end
 
   describe 'Journalized Objects' do
@@ -159,7 +193,8 @@ describe Meeting, type: :model do
 
       project.save!
 
-      meeting.start_time = DateTime.new(2013, 3, 27, 15, 35)
+      meeting.start_date = '2013-03-27'
+      meeting.start_time_hour = '15:35'
       meeting.participants.build(user: user2)
       meeting.save!
     end