Bendyworks/deduplicate starting salary

app/controllers/employees_controller.rb

@@ -42,16 +42,17 @@ def set_employee
   end
 
   def employee_params
-    params.require(:employee).permit(:first_name,
-                                     :last_name,
-                                     :starting_salary,
-                                     :direct_experience,
-                                     :indirect_experience,
-                                     :billable,
-                                     :notes,
-                                     :planning_raise_date,
-                                     :planning_raise_salary,
-                                     :planning_notes,
-                                     tenures_attributes: [:id, :start_date, :end_date, :_destroy])
+    emp_params = params.require(:employee)
+      .permit(:first_name,
+              :last_name,
+              :starting_salary,
+              :direct_experience,
+              :indirect_experience,
+              :billable,
+              :notes,
+              :planning_raise_date,
+              :planning_raise_salary,
+              :planning_notes,
+              tenures_attributes: [:id, :start_date, :end_date, :_destroy])
   end
 end

app/controllers/salaries_controller.rb

@@ -1,29 +1,62 @@
 # frozen_string_literal: true
 
 class SalariesController < ApplicationController
+  before_action :set_employee, except: [:edit, :update, :destroy]
+  before_action :set_salary, only: [:edit, :update, :destroy]
+
   def new
-    employee = Employee.find(params[:employee_id])
-    @salary = employee.salaries.new
+    salaries_count = last_tenure.salaries.count
+    @salary = last_tenure.salaries.new
+    # pre-fill start date from tenure for first salary to help user
+    @salary.start_date = last_tenure.start_date if salaries_count == 0
   end
 
   def create
-    employee = Employee.find(params[:employee_id])
-    @salary = employee.salaries.new(salary_params)
+    update_params = fill_start_date(@employee.tenures.last.start_date)
+    @salary = @employee.tenures.last.salaries.new(update_params)
     if @salary.save
-      redirect_to employee_path(@salary.employee), notice: 'Successfully recorded raise'
+      redirect_to employee_path(@employee), notice: 'Successfully recorded raise'
     else
       render :new
     end
   end
 
+  def edit; end
+
+  def update
+    update_params = fill_start_date(@salary.tenure_start_date)
+    if @salary.update(update_params)
+      redirect_to employee_path(@salary.employee), notice: 'Successfully upddated salary'
+    else
+      render :edit
+    end
+  end
+
   def destroy
-    @salary = Salary.find(params[:id])
     msg = @salary.destroy ? 'Salary deleted.' : 'Failed to delete salary. Please try again.'
     redirect_to employee_path(params[:employee_id]), notice: msg
   end
 
   private
 
+  def set_employee
+    @employee = Employee.find(params[:employee_id])
+  end
+
+  def set_salary
+    @salary = Salary.find(params[:id])
+  end
+
+  def last_tenure
+    @employee.tenures.last
+  end
+
+  def fill_start_date(date)
+    update_params = salary_params
+    update_params[:start_date] = date unless update_params.dig(:start_date)
+    update_params
+  end
+
   def salary_params
     params.require(:salary).permit(:start_date, :annual_amount)
   end

app/models/employee.rb

@@ -1,21 +1,16 @@
 # frozen_string_literal: true
 
 class Employee < ActiveRecord::Base
-  has_many :salaries, dependent: :destroy
   has_many :tenures, dependent: :destroy
+  has_many :salaries, through: :tenures, dependent: :destroy
+
   accepts_nested_attributes_for :tenures,
     reject_if: proc { |attributes| attributes['start_date'].blank? }, allow_destroy: true
 
   validates :first_name, presence: true
   validates :last_name, presence: true
-  validates :starting_salary, presence: true
 
   default_scope { order :first_name }
-  scope :past, -> { joins(:tenures).where 'tenures.end_date < :today', today: Time.zone.today }
-  scope :current, lambda {
-    joins(:tenures).where 'tenures.start_date <= :today' \
-          ' AND (tenures.end_date IS NULL OR tenures.end_date >= :today)', today: Time.zone.today
-  }
   scope :future, -> { joins(:tenures).where 'tenures.start_date > :today', today: Time.zone.today }
   scope :non_current, lambda {
     joins(:tenures)
@@ -77,6 +72,7 @@ def end_date
   end
 
   def employed_on?(date)
+    return nil if date.nil?
     tenures.any? \
       { |tenure| date >= tenure.start_date && (tenure.end_date.nil? || date <= tenure.end_date) }
   end
@@ -88,16 +84,12 @@ def display_name
   def salary_on(date)
     return nil unless employed_on?(date)
 
-    salary_match = salaries.where('start_date <= ?', date).last
+    salary_match = salaries.where('salaries.start_date <= ?', date).last
     salary_match ? salary_match.annual_amount : starting_salary
   end
 
   def salary_data
     data = []
-    tenures.each do |tenure|
-      data << { c: [date_for_js(tenure.start_date), salary_on(tenure.start_date)] }
-    end
-
     salaries.ordered_dates_with_previous_dates.each do |date|
       data << { c: [date_for_js(date), salary_on(date)] }
     end
@@ -108,6 +100,11 @@ def salary_data
     data.sort_by { |salary| salary[:c][0]}
   end
 
+  def starting_salary
+    s = salaries.first&.annual_amount
+    s = s || 0
+  end
+
   def ending_salary
     end_date ? salary_on(end_date) : nil
   end
@@ -136,7 +133,7 @@ def display_previous_pay
   end
 
   def previous_pay
-    if salaries.empty?
+    if salaries.empty? || salaries.count == 1
       nil
     else
       salaries[-2].try(:annual_amount) || starting_salary
@@ -203,4 +200,4 @@ def format_salary(salary)
       "$#{format('%g', salary_in_ks)}K"
     end
   end
-end
+end

app/models/salary.rb

@@ -1,22 +1,26 @@
 # frozen_string_literal: true
 
 class Salary < ActiveRecord::Base
-  belongs_to :employee
-  validates :start_date, presence: true, uniqueness: { scope: :employee_id }
-  validates :employee_id, presence: true
+  belongs_to :tenure
+  has_one :employee, through: :tenure
+  validates :start_date, presence: true, uniqueness: { scope: :tenure_id }
+  validates_presence_of :tenure
   validates :annual_amount, presence: true
-  validate :no_salaries_outside_employment_dates, if: :employee
+  validate :no_salaries_outside_tenure_dates, if: :tenure
 
   delegate :first_name, :last_name, to: :employee, prefix: true
+  delegate :start_date, to: :tenure, prefix: true
 
-  default_scope { order('start_date') }
+  before_validation :ensure_start_date
+
+  default_scope { order('salaries.start_date') }
 
   def self.ordered_dates
-    select('distinct start_date').order('start_date').map(&:start_date)
+    select('salaries.start_date').order('salaries.start_date').map(&:start_date).uniq
   end
 
   def self.ordered_dates_with_previous_dates
-    ordered_dates.map { |date| [date - 1, date] }.flatten
+    ordered_dates.map { |date| [date - 1, date] }.flatten.tap(&:shift)
   end
 
   def self.all_dates
@@ -30,9 +34,17 @@ def self.history_dates
 
   private
 
-  def no_salaries_outside_employment_dates
-    unless employee.employed_on?(start_date)
+  def no_salaries_outside_tenure_dates
+    unless tenure.employed_on?(start_date)
       errors.add(:start_date, 'must be between employee start and end dates')
     end
   end
-end
+
+  def ensure_start_date
+    # first salary is entered as part of employee form so get start date from first tenure
+    #  this depends on employee nested_attributes_for :tenures getting called first
+    if start_date.blank? && employee&.salaries&.count == 0
+      self.start_date = employee.start_date
+    end
+  end
+end

app/models/tenure.rb

@@ -2,11 +2,17 @@
 
 class Tenure < ActiveRecord::Base
   belongs_to :employee
+  has_many :salaries, dependent: :destroy
+
   validates :start_date, presence: true, uniqueness: { scope: :employee_id }
   validates :employee, presence: true
   validate :tenures_are_sequential, if: :employee
   validate :start_date_is_before_end_date
 
+  after_save :sync_first_salary
+
+  default_scope { order :start_date }
+
   def self.ordered_start_dates
     select('distinct start_date').unscoped.order('start_date').map(&:start_date).uniq
   end
@@ -24,6 +30,11 @@ def next_tenure
     employee.tenures.where("id > ?", id).first
   end
 
+  def employed_on?(date)
+    return nil if date.nil?
+    date >= start_date && (end_date.nil? || date <= end_date)
+  end
+
   private
 
   def start_date_is_before_end_date
@@ -40,4 +51,10 @@ def tenures_are_sequential
       errors.add(:end_date, 'must be before the next start date')
     end
   end
+
+  def sync_first_salary
+    if salaries.count > 0 && salaries.first&.start_date != start_date
+      salaries.first.update(start_date: start_date)
+    end
+  end
 end

app/views/employees/_form.html.haml

@@ -49,11 +49,6 @@
         .form-group.col-lg-12
           = f.label :indirect_experience, 'Indirect experience (months)', class: 'control-label'
           = f.number_field :indirect_experience, class: 'form-control'
-        .form-group.col-lg-12
-          = f.label :starting_salary, 'Starting salary annual amount', class: 'control-label'
-          .input-group
-            %span.input-group-addon $
-            = f.number_field :starting_salary, class: 'form-control', required: true
         .form-group.col-lg-12
           = f.label :notes, 'Notes', class: 'control-label'
           = f.text_area :notes, class: 'form-control'

app/views/employees/show.html.haml

@@ -38,17 +38,18 @@
           %td Salary
           %td
       %tbody
-        %tr
-          %td= @employee.start_date.strftime('%b %e, %Y')
-          %td= number_to_currency(@employee.starting_salary, precision: 0)
-          %td  (To change starting salary, use edit button above.)
-        - @employee.salaries.each do |salary|
+        - @employee.salaries.each_with_index do |salary, i|
           %tr
             %td= salary.start_date.strftime('%b %e, %Y')
             %td= number_to_currency(salary.annual_amount, precision: 0)
-            %td= link_to 'Delete', employee_salary_path(employee_id: @employee, id: salary), method: :delete
+            %td
+              = link_to 'Edit', edit_employee_salary_path(employee_id: @employee, id: salary)
+              = link_to 'Delete', employee_salary_path(employee_id: @employee, id: salary), method: :delete
   - if @employee.notes?
     %li.active
       = link_to 'Notes', "#"
       %pre= @employee.notes
-  = link_to 'Record a Raise', new_employee_salary_path(@employee), class: 'btn btn-success'
+  - if @employee.tenures.last.salaries.count == 0
+    = link_to 'Record Starting Salary', new_employee_salary_path(@employee), class: 'btn btn-success'
+  - else
+    = link_to 'Record a Raise', new_employee_salary_path(@employee), class: 'btn btn-success'

app/views/salaries/_form.html.haml

@@ -0,0 +1,20 @@
+- if salary.errors.any?
+  .alert.alert-danger
+    %p #{pluralize(salary.errors.count, "error")} prohibited this raise from being saved:
+    %ul
+      - salary.errors.full_messages.each do |msg|
+        %li= msg
+.col-lg-9
+  .well.bs-component
+    %fieldset
+      .form-group.col-lg-12
+        = f.label :start_date, class: 'control-label'
+        = f.date_field :start_date, class: 'form-control', required: true, autofocus: salary.start_date != salary.tenure_start_date, disabled: (salary.start_date == salary.tenure_start_date)
+      .form-group.col-lg-12
+        = f.label :annual_amount, class: 'control-label'
+        .input-group
+          %span.input-group-addon $
+          = f.number_field :annual_amount, class: 'form-control', required: true
+      .form-group.col-lg-12
+        = f.submit 'Save', class: 'btn btn-primary'
+        = link_to 'Cancel', employee_path(salary.employee), class: 'btn btn-default'

app/views/salaries/edit.html.haml

@@ -0,0 +1,3 @@
+%h2= "Edit Salary for #{@salary.employee_first_name} #{@salary.employee_last_name}"
+= form_for(@salary, url: employee_salary_path(@salary), html: { class: 'form-horizontal' }) do |f|
+    = render 'form', salary: @salary, f: f

app/views/salaries/new.html.haml

@@ -1,22 +1,6 @@
-%h2= "Record a Raise for #{@salary.employee_first_name} #{@salary.employee_last_name}"
-- if @salary.errors.any?
-  .alert.alert-danger
-    %p= "#{pluralize(@salary.errors.count, "error")} prohibited this raise from being saved:"
-    %ul
-      - @salary.errors.full_messages.each do |msg|
-        %li= msg
-.col-lg-9
-  .well.bs-component
-    = form_for(@salary, url: employee_salaries_path(@salary.employee), html: { class: 'form-horizontal' }) do |f|
-      %fieldset
-        .form-group.col-lg-12
-          = f.label :start_date, class: 'control-label'
-          = f.date_field :start_date, class: 'form-control', required: true, autofocus: true
-        .form-group.col-lg-12
-          = f.label :annual_amount, class: 'control-label'
-          .input-group
-            %span.input-group-addon $
-            = f.number_field :annual_amount, class: 'form-control', required: true, value: @salary.employee.salary_on(Time.zone.today)
-        .form-group.col-lg-12
-          = f.submit 'Save', class: 'btn btn-primary'
-          = link_to 'Cancel', employee_path(@salary.employee), class: 'btn btn-default'
+- if @salary.tenure.salaries.count >= 1
+  %h2= "Record a Raise for #{@salary.employee_first_name} #{@salary.employee_last_name}"
+- else
+  %h2= "Record Starting Salary for #{@salary.employee_first_name} #{@salary.employee_last_name}"
+= form_for(@salary, url: employee_salaries_path(@salary.employee), html: { class: 'form-horizontal' }) do |f|
+  = render 'form', salary: @salary, f: f

config/routes.rb

@@ -17,7 +17,7 @@
   get 'planning', to: 'planning#index'
 
   resources :employees, except: [:index, :destroy] do
-    resources :salaries, only: [:new, :create, :destroy]
+    resources :salaries, only: [:new, :create, :edit, :update, :destroy]
   end
 
   resources :users, only: [:index, :destroy] do