Allow overlapping committee memberships

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    effective_committees (0.10.0)
+    effective_committees (0.10.1)
       effective_bootstrap
       effective_datatables (>= 4.0.0)
       effective_resources

README.md

@@ -122,6 +122,8 @@ end
 
 ## Permissions
 
+A user can serve multiple terms on the same committee, including overlapping active terms. `user.committee_member(committee:)` and `committee.committee_member(user:)` return a currently active term (or `nil` if all terms are expired), which is enough for permission checks. For full lookups use `committee_members_for(...)`, which returns every term including expired ones.
+
 The permissions you actually want to define are as follows (using CanCan):
 
 ```ruby

app/models/concerns/effective_committees_user.rb

@@ -38,6 +38,11 @@ def committee_member(committee:)
     committee_members.select(&:active?).find { |cm| cm.committee_id == committee.id }
   end
 
+  # All terms (active and expired) this user has served on the given committee.
+  def committee_members_for(committee:)
+    committee_members.select { |cm| cm.committee_id == committee.id }
+  end
+
   # Find-active-or-build-new. If the user has no active term, build a fresh row
   # (expired terms are history and are not edited in place through this helper).
   def build_committee_member(committee:)

app/models/effective/committee.rb

@@ -12,10 +12,11 @@ class Committee < ActiveRecord::Base
     has_many :committee_members, -> { Effective::CommitteeMember.sorted }, class_name: 'Effective::CommitteeMember', inverse_of: :committee, dependent: :delete_all
     accepts_nested_attributes_for :committee_members, allow_destroy: true
 
-    has_many :committee_folders, -> { Effective::CommitteeFolder.sorted }, class_name: 'Effective::CommitteeFolder', inverse_of: :committee, dependent: :delete_all
+
+    has_many :committee_folders, -> { Effective::CommitteeFolder.sorted }, class_name: 'Effective::CommitteeFolder', inverse_of: :committee, dependent: :destroy
     accepts_nested_attributes_for :committee_folders, allow_destroy: true
 
-    has_many :committee_files, -> { Effective::CommitteeFile.sorted }, class_name: 'Effective::CommitteeFile', inverse_of: :committee, dependent: :delete_all
+    has_many :committee_files, -> { Effective::CommitteeFile.sorted }, class_name: 'Effective::CommitteeFile', inverse_of: :committee, dependent: :destroy
     accepts_nested_attributes_for :committee_files, allow_destroy: true
 
     has_many :committee_agenda_items, -> { Effective::CommitteeAgendaItem.sorted }, class_name: 'Effective::CommitteeAgendaItem', inverse_of: :committee, dependent: :delete_all

app/models/effective/committee_member.rb

@@ -40,17 +40,17 @@ class CommitteeMember < ActiveRecord::Base
     end
 
     after_commit(if: -> { user_ids.present? }) do
-      additional = (user_ids - CommitteeMember.where(committee_id: committee_id, user_id: user_ids).pluck(:user_id))
+      additional = user_ids
       max_position = CommitteeMember.where(committee_id: committee_id, committee_type: committee_type).maximum(:position) || -1
       additional = additional.each_with_index.map { |user_id, index| {committee_id: committee_id, committee_type: committee_type, user_id: user_id, user_type: user_type, roles_mask: roles_mask, start_on: start_on, end_on: end_on, position: max_position + index + 1} }
-      CommitteeMember.insert_all(additional)
+      CommitteeMember.insert_all(additional) if additional.present?
     end
 
     after_commit(if: -> { committee_ids.present? }) do
-      additional = (committee_ids - CommitteeMember.where(user_id: user_id, committee_id: committee_ids).pluck(:committee_id))
+      additional = committee_ids
       max_positions = CommitteeMember.where(committee_id: additional, committee_type: committee_type).group(:committee_id).maximum(:position)
       additional = additional.map { |committee_id| {committee_id: committee_id, committee_type: committee_type, user_id: user_id, user_type: user_type, roles_mask: roles_mask, start_on: start_on, end_on: end_on, position: (max_positions[committee_id] || -1) + 1} }
-      CommitteeMember.insert_all(additional)
+      CommitteeMember.insert_all(additional) if additional.present?
     end
 
     before_validation do
@@ -62,7 +62,7 @@ class CommitteeMember < ActiveRecord::Base
     validates :position, presence: true
 
     validate(if: -> { start_on && end_on }) do
-      errors.add(:end_on, 'must be after start date') unless end_on > start_on
+      errors.add(:end_on, 'must be on or after start date') unless end_on >= start_on
     end
 
     def to_s
@@ -73,13 +73,15 @@ def email
       user.try(:email)
     end
 
+    # end_on is exclusive: a member whose term ends on `date` is no longer
+    # active on that date.
     def active?(date: nil)
       return true if start_on.blank? && end_on.blank?
 
       date ||= Time.zone.now
       date = date.to_date if date.respond_to?(:to_date)
 
-      (start_on..end_on).cover?(date)  # Endless ranges
+      (start_on.nil? || start_on <= date) && (end_on.nil? || end_on > date)
     end
 
     def expired?(date: nil)

app/views/effective/committees/index.html.haml

@@ -18,19 +18,10 @@
           %p There are no #{committees_label.downcase} to display.
 
         - else
-          - committees.each do |committee|
-            .mb-5
-              %h3= link_to(committee.to_s, effective_committees.committee_path(committee), style: "text-decoration: none;")
-
-              - committe_folders = committee.committee_folders.select(&:top_level?)
-
-              - if committe_folders.present?
-                .card
-                  .list-group.list-group-flush
-                    - committe_folders.each do |folder|
-                      = link_to(folder.to_s, effective_committees.committee_committee_folder_path(committee, folder), class: 'list-group-item list-group-action')
-              - else
-                %p No files or folders.
+          .card
+            .list-group.list-group-flush
+              - committees.each do |committee|
+                = link_to(committee.to_s, effective_committees.committee_path(committee), class: 'list-group-item list-group-action')
 
       .col-lg
         - if current_user.committees.any?(&:agenda_mode?)

app/views/effective/committees/volunteers_and_committees.html.haml

@@ -3,7 +3,7 @@
     %h3= committee
     = committee.body
 
-    - members = committee.committee_members.select(&:active?).uniq(&:user_id)
+    - members = committee.committee_members.select(&:active?)
     %ul
       - members.each do |member|
         %li

test/dummy/db/schema.rb

@@ -49,6 +49,21 @@
     t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true
   end
 
+  create_table "committee_agenda_items", force: :cascade do |t|
+    t.string "code"
+    t.integer "committee_folder_id"
+    t.integer "committee_id"
+    t.string "committee_type"
+    t.datetime "created_at", precision: nil
+    t.integer "position"
+    t.string "presenter"
+    t.string "timed_at"
+    t.string "title"
+    t.datetime "updated_at", precision: nil
+    t.index ["committee_folder_id"], name: "index_committee_agenda_items_on_committee_folder_id"
+    t.index ["committee_id", "committee_type"], name: "index_committee_agenda_items_on_committee_id_and_committee_type"
+  end
+
   create_table "committee_files", force: :cascade do |t|
     t.integer "committee_folder_id"
     t.integer "committee_id"
@@ -70,11 +85,13 @@
     t.integer "committee_id"
     t.string "committee_type"
     t.datetime "created_at", precision: nil
+    t.datetime "meeting_date"
     t.integer "position"
     t.string "slug"
     t.string "title"
     t.datetime "updated_at", precision: nil
     t.index ["committee_id", "committee_type"], name: "index_committee_folders_on_committee_id_and_committee_type"
+    t.index ["committee_id", "meeting_date"], name: "index_committee_folders_on_committee_id_and_meeting_date"
     t.index ["committee_id"], name: "index_committee_folders_on_committee_id"
     t.index ["position"], name: "index_committee_folders_on_position"
   end
@@ -97,6 +114,7 @@
   end
 
   create_table "committees", force: :cascade do |t|
+    t.boolean "agenda_mode", default: false, null: false
     t.integer "committee_files_count", default: 0
     t.integer "committee_folders_count", default: 0
     t.integer "committee_members_count", default: 0
@@ -111,6 +129,21 @@
     t.index ["title"], name: "index_committees_on_title"
   end
 
+  create_table "logs", force: :cascade do |t|
+    t.string "status"
+    t.string "user_type"
+    t.integer "user_id"
+    t.string "changes_to_type"
+    t.integer "changes_to_id"
+    t.string "associated_type"
+    t.integer "associated_id"
+    t.string "associated_to_s"
+    t.text "message"
+    t.text "details"
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
+  end
+
   create_table "users", force: :cascade do |t|
     t.datetime "confirmation_sent_at", precision: nil
     t.datetime "confirmed_at", precision: nil

test/models/committees_destroy_test.rb

@@ -0,0 +1,73 @@
+require 'test_helper'
+
+# Deleting a committee must delete all of its associated files, including purging the
+# underlying Active Storage blobs. Previously the committee_folders/committee_files
+# associations used `dependent: :delete_all`, which deletes rows via raw SQL without
+# running callbacks — orphaning the uploaded files in storage (and leaving them
+# downloadable via their old URLs). They now use `dependent: :destroy`.
+#
+# Active Storage purges blobs in an `after_destroy_commit` callback, which only fires
+# on a real COMMIT, so these tests run without the wrapping test transaction.
+class CommitteesDestroyTest < ActiveSupport::TestCase
+  self.use_transactional_tests = false
+
+  teardown do
+    Effective::Committee.where(id: @committee_ids).destroy_all if @committee_ids.present?
+  end
+
+  def create_committee_with_unique_title
+    committee = Effective::Committee.create!(title: "Destroy Test #{SecureRandom.hex(6)}")
+    (@committee_ids ||= []) << committee.id
+    committee
+  end
+
+  def attach_file_to(folder, filename: 'document.txt')
+    committee_file = folder.committee_files.build
+    committee_file.file.attach(
+      io: StringIO.new('committee file contents'),
+      filename: filename,
+      content_type: 'text/plain'
+    )
+    committee_file.save!
+    committee_file
+  end
+
+  test 'destroying a committee destroys its folders and files and purges the attached blobs' do
+    committee = create_committee_with_unique_title
+    folder = committee.committee_folders.create!(title: 'Documents')
+    committee_file = attach_file_to(folder)
+
+    blob = committee_file.file.blob
+    key = blob.key
+
+    assert blob.service.exist?(key), 'precondition: file should be present in storage'
+    assert_equal 1, committee.reload.committee_files_count
+
+    committee.destroy!
+
+    refute Effective::Committee.exists?(committee.id), 'committee should be destroyed'
+    refute Effective::CommitteeFolder.exists?(folder.id), 'folder should be destroyed'
+    refute Effective::CommitteeFile.exists?(committee_file.id), 'committee file should be destroyed'
+    refute ActiveStorage::Blob.exists?(blob.id), 'blob record should be purged'
+    refute blob.service.exist?(key), 'file should be removed from storage'
+  end
+
+  test 'destroying a committee purges files nested in sub-folders' do
+    committee = create_committee_with_unique_title
+    parent = committee.committee_folders.create!(title: 'Parent')
+    child = committee.committee_folders.create!(title: 'Child', committee_folder: parent)
+    nested_file = attach_file_to(child, filename: 'nested.txt')
+
+    blob = nested_file.file.blob
+    key = blob.key
+    assert blob.service.exist?(key), 'precondition: nested file should be present in storage'
+
+    committee.destroy!
+
+    refute Effective::CommitteeFolder.exists?(parent.id), 'parent folder should be destroyed'
+    refute Effective::CommitteeFolder.exists?(child.id), 'child folder should be destroyed'
+    refute Effective::CommitteeFile.exists?(nested_file.id), 'nested file should be destroyed'
+    refute ActiveStorage::Blob.exists?(blob.id), 'nested blob record should be purged'
+    refute blob.service.exist?(key), 'nested file should be removed from storage'
+  end
+end

test/models/committees_member_test.rb

@@ -67,4 +67,110 @@ class CommitteesMemberTest < ActiveSupport::TestCase
     refute member.active?
   end
 
+  test 'active? treats end_on as exclusive' do
+    today = Date.current
+    committee = create_committee()
+    member = committee.committee_members.first
+
+    # Member whose term ends today is no longer active today
+    member.update!(start_on: today - 10.days, end_on: today)
+    refute member.active?(date: today)
+
+    # End date in the future — active today
+    member.update!(start_on: today - 10.days, end_on: today + 1.day)
+    assert member.active?(date: today)
+  end
+
+  test 'single-row end_on equals start_on is allowed' do
+    committee = create_committee()
+    member = committee.committee_members.first
+
+    member.start_on = Date.current
+    member.end_on = Date.current
+    assert member.valid?
+  end
+
+  test 'single-row end_on before start_on is invalid' do
+    committee = create_committee()
+    member = committee.committee_members.first
+
+    member.start_on = Date.current
+    member.end_on = Date.current - 1.day
+    refute member.valid?
+    assert member.errors[:end_on].present?
+  end
+
+  test 'overlapping terms for the same user on the same committee are allowed' do
+    committee = create_committee()
+    user = committee.committee_members.first.user
+
+    first_term = committee.committee_members.first
+    first_term.update!(start_on: Date.new(2024, 1, 1), end_on: Date.new(2026, 12, 31))
+
+    overlapping = committee.committee_members.create!(user: user, start_on: Date.new(2024, 6, 30), end_on: Date.new(2026, 12, 31))
+
+    assert first_term.active?(date: Date.new(2025, 1, 1))
+    assert overlapping.active?(date: Date.new(2025, 1, 1))
+    assert_equal 2, committee.reload.committee_members.select { |member| member.active?(date: Date.new(2025, 1, 1)) && member.user_id == user.id }.count
+    assert_equal 2, committee.reload.committee_members_for(user: user).count
+  end
+
+  test 'open-ended terms can overlap for the same user on the same committee' do
+    committee = create_committee()
+    user = committee.committee_members.first.user
+
+    duplicate = committee.committee_members.create!(user: user)
+
+    assert committee.committee_members.first.active?
+    assert duplicate.active?
+    assert_equal 2, committee.reload.committee_members_for(user: user).count
+  end
+
+  test 'committee.committee_member returns the currently active term' do
+    committee = create_committee()
+    member = committee.committee_members.first
+    user = member.user
+
+    member.update!(start_on: Date.new(2020, 1, 1), end_on: Date.new(2022, 12, 31))
+    active = committee.committee_members.create!(user: user, start_on: Date.new(2024, 1, 1))
+
+    committee.reload
+    assert_equal active.id, committee.committee_member(user: user).id
+  end
+
+  test 'committee.committee_member returns nil when all terms are expired' do
+    committee = create_committee()
+    member = committee.committee_members.first
+    user = member.user
+
+    member.update!(start_on: Date.new(2020, 1, 1), end_on: Date.new(2022, 12, 31))
+
+    committee.reload
+    assert_nil committee.committee_member(user: user)
+  end
+
+  test 'committee.committee_members_for returns full history' do
+    committee = create_committee()
+    member = committee.committee_members.first
+    user = member.user
+
+    member.update!(start_on: Date.new(2020, 1, 1), end_on: Date.new(2022, 12, 31))
+    committee.committee_members.create!(user: user, start_on: Date.new(2024, 1, 1))
+
+    committee.reload
+    assert_equal 2, committee.committee_members_for(user: user).length
+  end
+
+  test 'user.committee_members_for returns full history' do
+    committee = create_committee()
+    member = committee.committee_members.first
+    user = member.user
+
+    member.update!(start_on: Date.new(2020, 1, 1), end_on: Date.new(2022, 12, 31))
+    committee.committee_members.create!(user: user, start_on: Date.new(2024, 1, 1))
+
+    user.reload
+    assert_equal 2, user.committee_members_for(committee: committee).length
+  end
+
 end