From 84de3006b7267b996479d876d35a7e12fe2f79ff Mon Sep 17 00:00:00 2001 From: NoRePercussions Date: Mon, 13 Nov 2023 15:32:08 -0500 Subject: [PATCH 1/2] Add foreign key constraint and relationship checks from timecard_entry to eventdate Prevent inconsistency when eventdates are deleted by enforcing that timecard entries must reference a valid eventdate --- app/models/timecard_entry.rb | 4 ++-- .../20231113195629_add_eventdate_fk_to_timecards.rb | 10 ++++++++++ db/schema.rb | 5 +++-- 3 files changed, 15 insertions(+), 4 deletions(-) create mode 100644 db/migrate/20231113195629_add_eventdate_fk_to_timecards.rb diff --git a/app/models/timecard_entry.rb b/app/models/timecard_entry.rb index a2217d85..2e2c5008 100644 --- a/app/models/timecard_entry.rb +++ b/app/models/timecard_entry.rb @@ -1,12 +1,12 @@ class TimecardEntry < ApplicationRecord belongs_to :member - belongs_to :eventdate + belongs_to :eventdate, required: true belongs_to :timecard extend Enumerize enumerize :eventpart, in: ["call", "show", "strike"], default: "show" - validates_presence_of :member_id, :eventdate_id, :hours, :eventpart + validates_presence_of :eventdate, :member_id, :eventdate_id, :hours, :eventpart validates_numericality_of :hours, :less_than_or_equal_to => 37.5, :greater_than => 0 validates_associated :member validates_inclusion_of :timecard, :in => ->(t){Timecard.valid_timecards}, :message => 'is not a current timecard', :allow_nil => true diff --git a/db/migrate/20231113195629_add_eventdate_fk_to_timecards.rb b/db/migrate/20231113195629_add_eventdate_fk_to_timecards.rb new file mode 100644 index 00000000..42638a40 --- /dev/null +++ b/db/migrate/20231113195629_add_eventdate_fk_to_timecards.rb @@ -0,0 +1,10 @@ +class AddEventdateFkToTimecards < ActiveRecord::Migration[6.1] + def up + change_column :timecard_entries, :eventdate_id, :bigint + add_foreign_key :timecard_entries, :eventdates + end + def down + remove_foreign_key :timecard_entries, :eventdates + change_column :timecard_entries, :eventdate_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 4f300bfb..4930f46a 100644 --- a/db/schema.rb +++ b/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: 2023_08_01_123836) do +ActiveRecord::Schema.define(version: 2023_11_13_195629) do create_table "accounts", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade do |t| t.string "name", limit: 255, null: false @@ -346,7 +346,7 @@ create_table "timecard_entries", charset: "utf8mb4", collation: "utf8mb4_bin", force: :cascade do |t| t.integer "member_id" t.float "hours" - t.integer "eventdate_id" + t.bigint "eventdate_id" t.integer "timecard_id" t.float "payrate" t.datetime "created_at" @@ -369,4 +369,5 @@ add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" + add_foreign_key "timecard_entries", "eventdates" end From b562178453a7d300cc2855d3054e20e8e4bb31bd Mon Sep 17 00:00:00 2001 From: NoRePercussions Date: Tue, 14 Nov 2023 13:35:05 -0500 Subject: [PATCH 2/2] Eventdate validation helpers and form errors Add can_destroy? helper method, orm-level checks before the eventdate is destroyed, and a validator to event so trying to destroy an eventdate that has been billed for raises an error. --- app/models/event.rb | 15 ++++++++++++++- app/models/eventdate.rb | 13 +++++++++++++ app/models/timecard_entry.rb | 2 +- app/views/events/_eventdate_fields.html.erb | 10 +++++++--- 4 files changed, 35 insertions(+), 5 deletions(-) diff --git a/app/models/event.rb b/app/models/event.rb index 2fc1f300..fe191078 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -69,7 +69,7 @@ class Event < ActiveRecord::Base validates_associated :organization, :emails, :event_roles, :eventdates validates_format_of :contactemail, :with => Event::EmailRegex, :multiline => true # validate :eventdate_valid? - validate :textable_social_valid? + validate :textable_social_valid?, :check_eventdates_to_be_destroyed scope :current_year, -> { where("representative_date >= ? or last_representative_date > ?", Account.magic_date, Account.magic_date) } @@ -230,4 +230,17 @@ def textable_social_valid? errors.add(:textable_social, "Textable must be enabled if social textable is enabled") end end + + def check_eventdates_to_be_destroyed + # If we are trying to update an event, someone might try to delete an eventdate. + # This is bad if someone has billed for it. There is before_destroy validation, + # and a foreign key check, but because the eventdate is removed from the event, + # it is not automatically validated on submission. This we must manually check + # that we can delete events here. + eventdates.each do |eventdate| + if eventdate.marked_for_destruction? and !eventdate.can_destroy? + errors.add(:eventdates, "Cannot delete an eventdate that has been billed for") + end + end + end end diff --git a/app/models/eventdate.rb b/app/models/eventdate.rb index 31d7824c..d2d8daf3 100644 --- a/app/models/eventdate.rb +++ b/app/models/eventdate.rb @@ -18,6 +18,8 @@ class Eventdate < ApplicationRecord before_validation :prune_roles after_save :synchronize_representative_dates + before_destroy :check_can_destroy + Event_Span_Days = 2; Event_Span_Seconds = Event_Span_Days * 24 * 60 * 60; @@ -63,6 +65,17 @@ def valid_strike? ((strikedate.to_i - enddate.to_i) < Event_Span_Seconds)) end + def can_destroy? + not self.timecard_entries.any? + end + + def check_can_destroy + if not self.can_destroy? + errors.add(:base, "Cannot delete an eventdate that has been billed for") + throw(:abort) + end + end + def has_call? self.calltype == "literal" or self.calltype == "startdate" end diff --git a/app/models/timecard_entry.rb b/app/models/timecard_entry.rb index 2e2c5008..c180b1c6 100644 --- a/app/models/timecard_entry.rb +++ b/app/models/timecard_entry.rb @@ -6,7 +6,7 @@ class TimecardEntry < ApplicationRecord extend Enumerize enumerize :eventpart, in: ["call", "show", "strike"], default: "show" - validates_presence_of :eventdate, :member_id, :eventdate_id, :hours, :eventpart + validates_presence_of :eventdate, :member_id, :hours, :eventpart validates_numericality_of :hours, :less_than_or_equal_to => 37.5, :greater_than => 0 validates_associated :member validates_inclusion_of :timecard, :in => ->(t){Timecard.valid_timecards}, :message => 'is not a current timecard', :allow_nil => true diff --git a/app/views/events/_eventdate_fields.html.erb b/app/views/events/_eventdate_fields.html.erb index cebfa003..4fa2c39d 100644 --- a/app/views/events/_eventdate_fields.html.erb +++ b/app/views/events/_eventdate_fields.html.erb @@ -108,10 +108,14 @@ <% if can? :tic, @event %>
Delete?
- <% if @event.new_record? %> -
<%= link_to_remove_fields image_tag("cross.png"), f, true %>
+ <% if f.object.can_destroy? %> + <% if @event.new_record? %> +
<%= link_to_remove_fields image_tag("cross.png"), f, true %>
+ <% else %> +
<%= link_to_remove_fields image_tag("cross.png"), f %>
+ <% end %> <% else %> -
<%= link_to_remove_fields image_tag("cross.png"), f %>
+
You can't delete this date, as someone has already billed for it.
<% end %>
<% end %>