Krdytkyu

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.

Here is my 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

Here my 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

My config/routes.rb:

Rails.application.routes.draw do

  namespace :api do

    namespace :v1 do

      resources :group_events

    end

  end

end

And last but not least here is my 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