diff --git a/Gemfile.lock b/Gemfile.lock index 143b5a3..2b65607 100644 --- a/Gemfile.lock +++ b/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 diff --git a/README.md b/README.md index 1a0f5cf..30f508e 100644 --- a/README.md +++ b/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 diff --git a/app/models/concerns/effective_committees_user.rb b/app/models/concerns/effective_committees_user.rb index da5286b..8b58b7f 100644 --- a/app/models/concerns/effective_committees_user.rb +++ b/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:) diff --git a/app/models/effective/committee.rb b/app/models/effective/committee.rb index 63cfc7a..0832a2e 100644 --- a/app/models/effective/committee.rb +++ b/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 diff --git a/app/models/effective/committee_member.rb b/app/models/effective/committee_member.rb index a25c773..20c4ef0 100644 --- a/app/models/effective/committee_member.rb +++ b/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) diff --git a/app/views/effective/committees/index.html.haml b/app/views/effective/committees/index.html.haml index 2d12fa9..77f3544 100644 --- a/app/views/effective/committees/index.html.haml +++ b/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?) diff --git a/app/views/effective/committees/volunteers_and_committees.html.haml b/app/views/effective/committees/volunteers_and_committees.html.haml index 62ae2a6..a829260 100644 --- a/app/views/effective/committees/volunteers_and_committees.html.haml +++ b/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 diff --git a/test/dummy/db/schema.rb b/test/dummy/db/schema.rb index 5ab0a9c..258bd17 100644 --- a/test/dummy/db/schema.rb +++ b/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 diff --git a/test/models/committees_destroy_test.rb b/test/models/committees_destroy_test.rb new file mode 100644 index 0000000..ecf737f --- /dev/null +++ b/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 diff --git a/test/models/committees_member_test.rb b/test/models/committees_member_test.rb index 3a9e35c..6553a40 100644 --- a/test/models/committees_member_test.rb +++ b/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