Bendyworks/deduplicate starting salary

app/controllers/employees_controller.rb

@@ -12,8 +12,10 @@ def new
   def edit; end
 
   def create
-    @employee = Employee.new(employee_params)
+    salary_params = employee_params.dig(:salaries_attributes, "0")
+    @employee = Employee.new(employee_params.except(:salaries_attributes))
     if @employee.save
+      @employee.tenures.first.salaries.create(salary_params) unless salary_params.blank?
       redirect_to @employee, notice: 'Employee successfully created.'
     else
       @errors = @employee.errors.full_messages
@@ -52,6 +54,7 @@ def employee_params
                                      :planning_raise_date,
                                      :planning_raise_salary,
                                      :planning_notes,
-                                     tenures_attributes: [:id, :start_date, :end_date, :_destroy])
+                                     tenures_attributes: [:id, :start_date, :end_date, :_destroy],
+                                     salaries_attributes: [:id, :start_date, :annual_amount])
   end
 end

app/controllers/salaries_controller.rb

@@ -3,14 +3,15 @@
 class SalariesController < ApplicationController
   def new
     employee = Employee.find(params[:employee_id])
-    @salary = employee.salaries.new
+    @salary = employee.tenures.last.salaries.new
   end
 
   def create
     employee = Employee.find(params[:employee_id])
-    @salary = employee.salaries.new(salary_params)
+    tenure = employee.tenures.last
+    @salary = tenure.salaries.new(salary_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

app/models/employee.rb

@@ -1,14 +1,17 @@
 # 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
 
+  accepts_nested_attributes_for :salaries,
+    reject_if: proc { |attributes| attributes['annual_amount'].blank? }
+
   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 }
@@ -77,6 +80,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 +92,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 +108,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 +141,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 +208,4 @@ def format_salary(salary)
       "$#{format('%g', salary_in_ks)}K"
     end
   end
-end
+end

app/models/salary.rb

@@ -1,22 +1,25 @@
 # 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
 
-  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 +33,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,15 @@
 
 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
 
+  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 +28,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

app/views/employees/_form.html.haml

@@ -49,11 +49,14 @@
         .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
+        = f.fields_for :salaries, (employee.salaries.first || employee.salaries.build) do |salary_f|
+          = salary_f.hidden_field :id unless employee.salaries.count == 0
+          = salary_f.hidden_field :start_date unless employee.salaries.count == 0
+          .form-group.col-lg-12
+            = salary_f.label :annual_amount, 'Starting salary annual amount', class: 'control-label'
+            .input-group
+              %span.input-group-addon $
+              = salary_f.number_field :annual_amount, 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,15 +38,15 @@
           %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
+              -if i == 0
+                (To change starting salary, use edit button above.)
+              -else
+                = link_to 'Delete', employee_salary_path(employee_id: @employee, id: salary), method: :delete
   - if @employee.notes?
     %li.active
       = link_to 'Notes', "#"

db/migrate/20210315122533_migrate_starting_salary_to_salaries.rb

@@ -0,0 +1,42 @@
+class MigrateStartingSalaryToSalaries < ActiveRecord::Migration[6.1]
+  def up
+    add_reference :salaries, :tenure, foreign_key: true
+
+    Employee.all.each do |employee|
+      Salary.where(employee_id: employee.id).all.each do |salary|
+        # look for tenure date range containing salary start_date.  Fall back to first if none found
+        tenure = employee.tenures.where(
+          'start_date <= :date and (end_date >= :date or end_date is NULL)',
+          date: salary.start_date).first
+        tenure = employee.tenures.first unless tenure
+        salary.update(tenure_id: tenure.id)
+      end
+      tenure = employee.tenures.first
+      tenure.salaries << Salary.new(
+        start_date: tenure.start_date,
+        annual_amount: employee.attributes['starting_salary'],
+        employee_id: employee.id)
+      tenure.save!
+    end
+
+    remove_reference :salaries, :employee, foreign_key: true
+    remove_column :employees, :starting_salary
+  end
+
+  def down
+    add_column :employees, :starting_salary, :decimal, default: 0, null: false
+    add_reference :salaries, :employee, foreign_key: true
+
+    Employee.all.each do |employee|
+      employee.tenures.each do |tenure|
+        tenure.salaries.each { |salary| salary.update(employee_id: employee.id) }
+      end
+      tenure = employee.tenures.first
+      salary = tenure.salaries.first if tenure
+      employee.update(starting_salary: salary.annual_amount) if salary
+      salary.destroy!
+    end
+
+    remove_reference :salaries, :tenure
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2020_12_14_224051) do
+ActiveRecord::Schema.define(version: 2021_03_15_122533) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -46,7 +46,6 @@
     t.datetime "updated_at"
     t.integer "direct_experience", default: 0, null: false
     t.integer "indirect_experience", default: 0, null: false
-    t.decimal "starting_salary", default: "0.0", null: false
     t.text "notes"
     t.date "planning_raise_date"
     t.string "planning_raise_salary"
@@ -55,11 +54,11 @@
 
   create_table "salaries", force: :cascade do |t|
     t.date "start_date", null: false
-    t.integer "employee_id", null: false
     t.decimal "annual_amount", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.index ["employee_id"], name: "index_salaries_on_employee_id"
+    t.bigint "tenure_id"
+    t.index ["tenure_id"], name: "index_salaries_on_tenure_id"
   end
 
   create_table "tenures", force: :cascade do |t|
@@ -102,5 +101,5 @@
   end
 
   add_foreign_key "balances", "accounts"
-  add_foreign_key "salaries", "employees", name: "salaries_employee_id_fk"
+  add_foreign_key "salaries", "tenures"
 end