1
\$\begingroup\$

I found myself working in this coding exercise for a work offer. I tried to apply TDD in the process but I didn't get any feedback from the company and I really would like to know what can be improved (pretty sure there are a lot of things), so any advice or refactor suggestion will be very much appreciated.

Background:

A group event will be created by an user. The group event should run for a whole number of days e.g.. 30 or 60. There should be attributes to set and update the start, end or duration of the event (and calculate the other value). The event also has a name, description (which supports formatting) and location. The event should be draft or published. To publish all of the fields are required, it can be saved with only a subset of fields before it’s published. When the event is deleted/remove it should be kept in the database and marked as such.

Deliverable:

Write an AR model, spec and migration for a GroupEvent that would meet the needs of the description above. Then write the API controller and spec to support JSON request/responses to manage these GroupEvents. For the purposes of this exercise, ignore auth.

app/models/group_event.rb:

class GroupEvent < ApplicationRecord
   validates     :name, :description, :starts_at, :ends_at, :location,      presence: true, if: :published?
   validate      :duration_range_is_valid?, if: :published?
   before_save   :set_duration, if: :duration_range_is_valid?

   def draft?
     !published?
   end

  def check_duration_range
    errors.add(:start_date, "Start date must be before end date") unless duration_range_is_valid?
  end

  def has_duration_range?
    starts_at.present? && ends_at.present?
  end

 def duration_range_is_valid?
   has_duration_range? and starts_at <= ends_at
 end

 def set_duration
   self.duration = ((self.ends_at - self.starts_at) / 1.day).to_i if duration_range_is_valid?
 end

 def destroy
   update_attribute(:deleted, true)
 end
end

app/controllers/api/v1/group_events_controller.rb:

    class Api::V1::GroupEventsController < ApplicationController
  include Response
  include ExceptionHandler

  before_action :set_group_event, only: [:show, :update, :destroy]

  # GET /api/v1/group_events
  def index
    @group_events = GroupEvent.all
    json_response @group_events
  end

  # GET /api/v1/group_events/1
  def show
    json_response @group_event
  end

  # POST /api/v1/group_events
  def create
    @group_event = GroupEvent.create!(group_event_params)
    json_response(@group_event, :created)
  end

  # PATCH/PUT /api/v1/group_events/1
  def update
    if @group_event.update(group_event_params)
      json_response @group_event
    else
      json_response @group_event.errors, :unprocessable_entity
    end
  end

  # DELETE /api/v1/group_events/1
  def destroy
    @group_event.destroy
  end

  private
  def set_group_event
    @group_event = GroupEvent.find(params[:id])
  end

  def group_event_params
    params.permit(:name, :description, :deleted, :published, :location, :starts_at, :ends_at)
  end
end

config/routes.rb:

Rails.application.routes.draw do
  namespace :api do
    namespace :v1 do
      resources :group_events
    end
  end
end

spec/models/group_event_spec.rb:

require 'rails_helper'

RSpec.describe GroupEvent, type: :model do
  it 'has a valid factory' do
    expect(create(:group_event)).to be_valid
  end

  describe 'validations' do
    context 'published' do
      before { allow(subject).to receive(:published?).and_return(true) }
      it { is_expected.to validate_presence_of :name }
      it { is_expected.to validate_presence_of :description }
      it { is_expected.to validate_presence_of :starts_at }
      it { is_expected.to validate_presence_of :ends_at }
      it { is_expected.to validate_presence_of :location }
    end
    context 'draft' do
      before { allow(subject).to receive(:draft?).and_return(true) }
      it { is_expected.not_to validate_presence_of :name }
      it { is_expected.not_to validate_presence_of :description }
      it { is_expected.not_to validate_presence_of :starts_at }
      it { is_expected.not_to validate_presence_of :ends_at }
      it { is_expected.not_to validate_presence_of :location }
    end
  end

  describe '.draft?' do
    context 'published' do
      before { allow(subject).to receive(:published?).and_return(true) }
      it 'is false' do
        expect(subject.draft?).to eq(false)
      end
    end
    context 'draft' do
      before { allow(subject).to receive(:published?).and_return(false) }
      it 'is true' do
        expect(subject.draft?).to eq(true)
      end
    end
  end

  describe '.has_duration_range?' do
    context 'with both values' do
      before { allow(subject).to receive(:starts_at).and_return(DateTime.now) }
      before { allow(subject).to receive(:ends_at).and_return(DateTime.tomorrow) }
      it 'is true' do
        expect(subject.has_duration_range?).to eq(true)
      end
    end
    context 'without both values' do
      before { allow(subject).to receive(:starts_at).and_return(nil) }
      before { allow(subject).to receive(:ends_at).and_return(nil) }
      it 'is false' do
        expect(subject.has_duration_range?).to eq(false)
      end
    end
    context 'without only one value' do
      before { allow(subject).to receive(:starts_at).and_return(DateTime.now) }
      before { allow(subject).to receive(:ends_at).and_return(nil) }
      it 'is false' do
        expect(subject.has_duration_range?).to eq(false)
      end
    end
  end

  describe '.duration_range_is_valid?' do
    context 'with valid range' do
      before { allow(subject).to receive(:has_duration_range?).and_return(true) }
      before { allow(subject).to receive(:starts_at).and_return(DateTime.now) }
      before { allow(subject).to receive(:ends_at).and_return(DateTime.tomorrow) }
      it 'is true' do
        expect(subject.duration_range_is_valid?).to eq(true)
      end
    end
    context 'without invalid range' do
      before { allow(subject).to receive(:has_duration_range?).and_return(true) }
      before { allow(subject).to receive(:starts_at).and_return(DateTime.tomorrow) }
      before { allow(subject).to receive(:ends_at).and_return(DateTime.now) }
      it 'is false' do
        expect(subject.duration_range_is_valid?).to eq(false)
      end
    end
  end

  describe '.set_duration' do
    it 'with valid range' do
      group_event = GroupEvent.create!(starts_at: DateTime.now, ends_at: DateTime.now + 10.days)
      expect(group_event.duration).to eq(10)
    end
    it 'with invalid range' do
      group_event = GroupEvent.create!(starts_at: DateTime.now, ends_at: DateTime.now - 10.days)
      expect(group_event.duration).to eq(nil)
    end
  end

  describe '.destroy' do
    it 'does not destroy the event' do
      group_event = GroupEvent.create!
      count = GroupEvent.all.count
      group_event.destroy
      expect(GroupEvent.unscoped.count).to eq(count)
    end
    it 'sets deleted' do
      group_event = GroupEvent.create!
      group_event.destroy
      expect(group_event.deleted).to eq(true)
    end
  end
end
\$\endgroup\$

Your Answer

By clicking "Post Your Answer", you agree to our terms of service, privacy policy and cookie policy

Browse other questions tagged or ask your own question.