diff --git a/.claude/settings.local.json b/.claude/settings.local.json index 9f86e65f5..46e3e8bf6 100644 --- a/.claude/settings.local.json +++ b/.claude/settings.local.json @@ -6,10 +6,8 @@ "Bash(bundle exec rails runner:*)", "Bash(bundle exec rspec:*)", "Bash(bundle exec rubocop:*)", - "Bash(RAILS_ENV=test bundle exec rails runner:*)", "Bash(RAILS_ENV=test bundle exec rspec:*)", - "Bash(git add:*)", "Bash(git apply:*)", "Bash(git checkout:*)", @@ -19,20 +17,18 @@ "Bash(git push:*)", "Bash(git reset:*)", "Bash(git stash:*)", - "Bash(bin/rails db:migrate:*)", "Bash(bin/rails runner:*)", - "Bash(chmod:*)", "Bash(mysql -u root:*)", - "Bash(git -C /Users/maebeale/programming/awbw branch --show-current)", - "Bash(bundle exec rails routes:*)", "Bash(bundle exec rubocop:*)", "Bash(git -C /Users/maebeale/programming/awbw diff --name-only main...HEAD)", - "Bash(git log:*)" - + "Bash(git log:*)", + "Bash(bin/rails generate:*)", + "Bash(bin/rails db:schema:dump:*)", + "Bash(RAILS_ENV=test bin/rails:*)" ] } } diff --git a/app/controllers/community_news_controller.rb b/app/controllers/community_news_controller.rb index 621e51116..1258fe1c8 100644 --- a/app/controllers/community_news_controller.rb +++ b/app/controllers/community_news_controller.rb @@ -108,7 +108,7 @@ def destroy # Optional hooks for setting variables for forms or index def set_form_variables @organizations = Organization.pluck(:name, :id).sort_by(&:first) - @authors = User.active.or(User.where(id: @community_news.author_id)) + @authors = User.has_access.or(User.where(id: @community_news.author_id)) .includes(:person) .map { |u| [ u.full_name, u.id ] }.sort_by(&:first) @categories_grouped = diff --git a/app/controllers/event_registrations_controller.rb b/app/controllers/event_registrations_controller.rb index 2dc1ca3cf..39f963ab7 100644 --- a/app/controllers/event_registrations_controller.rb +++ b/app/controllers/event_registrations_controller.rb @@ -99,7 +99,7 @@ def set_form_variables .or(Event.where(id: @event_registration.event_id)) .distinct .order(start_date: :desc) - @registrants = User.active.includes(:person).order("people.first_name, people.last_name") + @registrants = User.has_access.includes(:person).order("people.first_name, people.last_name") end private diff --git a/app/controllers/people_controller.rb b/app/controllers/people_controller.rb index fcfa60cd6..62d0c63f6 100644 --- a/app/controllers/people_controller.rb +++ b/app/controllers/people_controller.rb @@ -77,7 +77,7 @@ def edit :contact_methods, :addresses, { avatar_attachment: :blob }, - { comments: :created_by }, + { comments: [ :created_by, :updated_by ] }, { sectorable_items: :sector }, organization_people: { organization: :logo_attachment } ).find(params[:id]).decorate @@ -88,7 +88,8 @@ def edit def create @person = Person.new(person_params.except(:user_attributes)) authorize! @person - @person.user ||= (User.find(params[:person][:user_attributes][:id]) if params[:person][:user_attributes]) + @person.user ||= User.find_by(id: params[:user_id]) if params[:user_id].present? + @person.user ||= User.find_by(id: params.dig(:person, :user_attributes, :id)) if params.dig(:person, :user_attributes, :id).present? unless params[:skip_duplicate_check].present? duplicates = find_duplicate_people( diff --git a/app/controllers/resources_controller.rb b/app/controllers/resources_controller.rb index a78f3900e..28eff208f 100644 --- a/app/controllers/resources_controller.rb +++ b/app/controllers/resources_controller.rb @@ -149,7 +149,7 @@ def set_form_variables @resource.build_downloadable_asset if @resource.downloadable_asset.blank? @resource.gallery_assets.build @windows_types = WindowsType.all - @authors = User.active.or(User.where(id: @resource.user_id)) + @authors = User.has_access.or(User.where(id: @resource.user_id)) .includes(:person) .order("people.first_name, people.last_name") .map { |u| [ u.full_name, u.id ] } diff --git a/app/controllers/stories_controller.rb b/app/controllers/stories_controller.rb index c76351125..9cf402cbc 100644 --- a/app/controllers/stories_controller.rb +++ b/app/controllers/stories_controller.rb @@ -178,7 +178,7 @@ def set_form_variables .order(:created_at) @windows_types = WindowsType.all @workshops = Workshop.all.order(:title) - @users = User.active.or(User.where(id: @story.created_by_id)) + @users = User.has_access.or(User.where(id: @story.created_by_id)) .includes(:person) .order("people.first_name, people.last_name") @categories_grouped = diff --git a/app/controllers/story_ideas_controller.rb b/app/controllers/story_ideas_controller.rb index 3dcd18424..4a6628b85 100644 --- a/app/controllers/story_ideas_controller.rb +++ b/app/controllers/story_ideas_controller.rb @@ -87,7 +87,7 @@ def set_form_variables @windows_types = WindowsType.all @workshops = Workshop.order(:title) - @users = User.active.includes(:person) + @users = User.has_access.includes(:person) @users = @users.or(User.where(id: @story_idea.created_by_id)) if @story_idea&.created_by_id @users = @users.distinct.order("people.first_name, people.last_name") @story_idea.build_primary_asset if @story_idea.primary_asset.blank? diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index fd4605d81..252e6bf74 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -6,7 +6,8 @@ class UsersController < ApplicationController def index authorize! per_page = params[:number_of_items_per_page].presence || 25 - base_scope = authorized_scope(User.includes(avatar_attachment: :blob, + base_scope = authorized_scope(User.includes(:created_by, :updated_by, + avatar_attachment: :blob, person: { avatar_attachment: :blob })) filtered = base_scope.search_by_params(params).order(:first_name, :last_name) @users_count = filtered.count @@ -16,6 +17,23 @@ def index def show authorize! @user @user = User.find(params[:id]).decorate + @comments = @user.comments.includes(:created_by).newest_first.paginate(page: params[:comments_page], per_page: 5) + + user_auth_events = Ahoy::Event + .where("name LIKE 'auth.%' OR name LIKE 'update.user'") + .where( + "(CAST(JSON_EXTRACT(properties, '$.record_id') AS UNSIGNED) = :id AND JSON_UNQUOTE(JSON_EXTRACT(properties, '$.record_type')) = 'User') OR " \ + "(CAST(JSON_EXTRACT(properties, '$.resource_id') AS UNSIGNED) = :id AND JSON_UNQUOTE(JSON_EXTRACT(properties, '$.resource_type')) = 'User')", + id: @user.id + ) + + @last_admin_event = user_auth_events.where(name: %w[auth.admin_granted auth.admin_revoked]).order(time: :desc).first + @last_lock_event = user_auth_events.where(name: %w[auth.account_locked auth.account_unlocked]).order(time: :desc).first + + @account_events = user_auth_events + .includes(:user) + .order(time: :desc) + .paginate(page: params[:page], per_page: 10) end def new @@ -25,6 +43,7 @@ def new end def edit + @user = User.includes(comments: [ :created_by, :updated_by ]).find(params[:id]) authorize! @user set_form_variables end @@ -37,9 +56,10 @@ def create unless params[:skip_duplicate_check].present? email = @user.email if email.present? && !email.downcase.end_with?("@example.com") - duplicates = find_duplicate_users(email) + person_id = params[:person_id].presence || params.dig(:user, :person_id).presence || @user.person_id + duplicates = find_duplicate_users(email, exclude_person_id: person_id) if duplicates.any? - redirect_to check_duplicates_users_path(email: email, person_id: params[:person_id].presence || params.dig(:user, :person_id).presence) + redirect_to check_duplicates_users_path(email: email, person_id: person_id) return end end @@ -55,6 +75,8 @@ def create # assign person person_id = params[:person_id].presence || params.dig(:user, :person_id).presence @user.person = Person.find(person_id) if person_id + @user.created_by = current_user + @user.updated_by = current_user if @user.save # @user.notifications.create(notification_type: 0) @@ -70,7 +92,7 @@ def check_duplicates @email = params[:email] @person_id = params[:person_id] - @duplicates = find_duplicate_users(@email) + @duplicates = find_duplicate_users(@email, exclude_person_id: @person_id) @blocked = @duplicates.any? { |d| d[:blocked] } end @@ -83,9 +105,16 @@ def update bypass_sign_in(@user) end - if @user.update(user_params.except(:password, :password_confirmation)) - # @user.notifications.create(notification_type: 1) - redirect_to users_path, notice: "User was successfully updated." + @user.assign_attributes(user_params.except(:password, :password_confirmation)) + @user.updated_by = current_user + @user.comments.select(&:new_record?).each { |c| c.created_by = current_user } + @user.comments.select(&:changed?).each { |c| c.updated_by = current_user } + + if @user.save + bypass_sign_in(@user) if @user == current_user + notice = "User was successfully updated." + notice += " A confirmation email has been sent to #{@user.unconfirmed_email}." if @user.unconfirmed_email.present? && @user.saved_change_to_unconfirmed_email? + redirect_to users_path, notice: notice else flash[:alert] = "Unable to update user." set_form_variables @@ -251,33 +280,41 @@ def password_params params.require(:user).permit(:current_password, :password, :password_confirmation) end - def find_duplicate_users(email) + def find_duplicate_users(email, exclude_person_id: nil) return [] if email.blank? email_lower = email.downcase duplicates = [] # Check existing users with same email - User.where("LOWER(email) = ?", email_lower).limit(10).each do |user| + users_scope = User.where("LOWER(email) = ?", email_lower).includes(:person) + users_scope = users_scope.where.not(person_id: exclude_person_id) if exclude_person_id + users_scope.limit(10).each do |user| duplicates << { id: user.id, name: user.person&.full_name || "#{user.first_name} #{user.last_name}".strip, + person_id: user.person_id, email: user.email, type: "user", blocked: true } end - # Check people with matching email (who may not have a user yet) - Person.includes(:user) - .left_joins(:user) + # Check people with matching email or secondary email + exclude_person_ids = duplicates.map { |d| d[:person_id] }.compact + exclude_person_ids << exclude_person_id.to_i if exclude_person_id + people_scope = Person.includes(:user) .where("LOWER(people.email) = :email OR LOWER(people.email_2) = :email", email: email_lower) - .where(users: { id: nil }) - .limit(10).each do |person| + people_scope = people_scope.where.not(id: exclude_person_ids) if exclude_person_ids.any? + people_scope.limit(10).each do |person| + primary_match = person.email&.downcase == email_lower duplicates << { id: person.id, name: person.full_name, - email: person.email || person.email_2, + email: primary_match ? person.email : person.email_2, + email_field: primary_match ? "primary" : "secondary", + has_user: person.user.present?, + user_email: person.user&.email, type: "person", blocked: false } @@ -288,7 +325,7 @@ def find_duplicate_users(email) def user_params params.require(:user).permit( - :email, :comment, :person_id, :inactive, :primary_address, :time_zone, :super_user, + :email, :comment, :person_id, :inactive, :locked, :primary_address, :time_zone, :super_user, ##### legacy to remove later :agency_id, :legacy, :legacy_id, :subscribecode, :avatar, :first_name, :last_name, # legacy to remove later @@ -297,6 +334,7 @@ def user_params ##### organization_people_attributes: [ :id, :organization_id, :position, :title, :inactive, :primary_contact, :start_date, :end_date, :_destroy ], + comments_attributes: [ :id, :body ], ) end end diff --git a/app/controllers/workshop_variation_ideas_controller.rb b/app/controllers/workshop_variation_ideas_controller.rb index 0938a3231..291a79e04 100644 --- a/app/controllers/workshop_variation_ideas_controller.rb +++ b/app/controllers/workshop_variation_ideas_controller.rb @@ -88,7 +88,7 @@ def set_form_variables @workshops = Workshop.published.order(:title) @organizations = authorized_scope(Organization.all).order(:name) @windows_types = WindowsType.order(:name) - @users = User.active.or(User.where(id: @workshop_variation_idea.created_by_id)) + @users = User.has_access.or(User.where(id: @workshop_variation_idea.created_by_id)) .order(:first_name, :last_name) end diff --git a/app/frontend/javascript/controllers/comment_edit_toggle_controller.js b/app/frontend/javascript/controllers/comment_edit_toggle_controller.js index f7cbe2960..108aa8abc 100644 --- a/app/frontend/javascript/controllers/comment_edit_toggle_controller.js +++ b/app/frontend/javascript/controllers/comment_edit_toggle_controller.js @@ -1,5 +1,10 @@ import { Controller } from "@hotwired/stimulus"; +// Connects to data-controller="comment-edit-toggle" +// +// Toggles between view and edit modes for inline comment editing. +// When exiting edit mode, syncs textarea values back to the truncated display. +// export default class extends Controller { static targets = ["editLabel", "viewLabel"]; @@ -9,6 +14,21 @@ export default class extends Controller { toggle() { this.editing = !this.editing; + + // When leaving edit mode, sync textarea values to view display + if (!this.editing) { + this.element.querySelectorAll(".nested-fields").forEach((item) => { + const textarea = item.querySelector(".comment-edit textarea"); + const viewBody = item.querySelector(".comment-view .comment-body"); + if (textarea && viewBody) { + const text = textarea.value; + viewBody.textContent = + text.length > 135 ? text.substring(0, 132) + "..." : text; + viewBody.title = text; + } + }); + } + this.element .querySelectorAll(".comment-view") .forEach((el) => (el.style.display = this.editing ? "none" : "")); @@ -17,8 +37,8 @@ export default class extends Controller { .forEach((el) => (el.style.display = this.editing ? "" : "none")); if (this.hasEditLabelTarget && this.hasViewLabelTarget) { - this.editLabelTarget.classList.toggle("hidden", this.editing); - this.viewLabelTarget.classList.toggle("hidden", !this.editing); + this.editLabelTarget.style.display = this.editing ? "none" : ""; + this.viewLabelTarget.style.display = this.editing ? "" : "none"; } } } diff --git a/app/frontend/javascript/controllers/dirty_form_controller.js b/app/frontend/javascript/controllers/dirty_form_controller.js index c99853c6e..7d23cc11b 100644 --- a/app/frontend/javascript/controllers/dirty_form_controller.js +++ b/app/frontend/javascript/controllers/dirty_form_controller.js @@ -2,14 +2,13 @@ import { Controller } from "@hotwired/stimulus"; // Connects to data-controller="dirty-form" // -// Tracks whether a form has unsaved changes and prompts on cancel. -// Listens for input changes and cocoon nested field additions/removals. -// -// data-dirty-form-target="cancel" on the cancel link +// Tracks whether a form has unsaved changes and warns before navigating away. +// Uses three layers of protection: +// 1. turbo:before-visit – catches Turbo Drive link clicks (custom message) +// 2. beforeunload – catches non-Turbo navigations / tab close (browser message) +// 3. confirmCancel action – explicit cancel button binding (custom message) // export default class extends Controller { - static targets = ["cancel"]; - connect() { this.dirty = false; this.handleChange = () => (this.dirty = true); @@ -18,12 +17,35 @@ export default class extends Controller { this.element.addEventListener("change", this.handleChange); this.element.addEventListener("cocoon:after-insert", this.handleChange); this.element.addEventListener("cocoon:after-remove", this.handleChange); + + // Clear dirty flag on form submit so guards don't block saving + this.handleSubmit = () => (this.dirty = false); + this.element.addEventListener("submit", this.handleSubmit); + + // Guard Turbo Drive navigations (links without data-turbo="false") + this.handleTurboVisit = (event) => { + if (this.dirty && !confirm("You have unsaved changes. Are you sure you want to leave?")) { + event.preventDefault(); + } + }; + document.addEventListener("turbo:before-visit", this.handleTurboVisit); + + // Guard non-Turbo navigations (data-turbo="false" links, browser back, tab close) + this.handleBeforeUnload = (event) => { + if (this.dirty) { + event.preventDefault(); + event.returnValue = ""; + } + }; + window.addEventListener("beforeunload", this.handleBeforeUnload); } confirmCancel(event) { if (this.dirty) { if (!confirm("You have unsaved changes. Are you sure you want to leave?")) { event.preventDefault(); + } else { + this.dirty = false; } } } @@ -33,5 +55,8 @@ export default class extends Controller { this.element.removeEventListener("change", this.handleChange); this.element.removeEventListener("cocoon:after-insert", this.handleChange); this.element.removeEventListener("cocoon:after-remove", this.handleChange); + this.element.removeEventListener("submit", this.handleSubmit); + document.removeEventListener("turbo:before-visit", this.handleTurboVisit); + window.removeEventListener("beforeunload", this.handleBeforeUnload); } } diff --git a/app/frontend/javascript/controllers/index.js b/app/frontend/javascript/controllers/index.js index 03e6953bd..f3d1a1cd5 100644 --- a/app/frontend/javascript/controllers/index.js +++ b/app/frontend/javascript/controllers/index.js @@ -74,3 +74,6 @@ application.register("timeframe", TimeframeController) import ToggleLockController from "./toggle_lock_controller" application.register("toggle-lock", ToggleLockController) + +import ToggleUserIconController from "./toggle_user_icon_controller" +application.register("toggle-user-icon", ToggleUserIconController) diff --git a/app/frontend/javascript/controllers/toggle_lock_controller.js b/app/frontend/javascript/controllers/toggle_lock_controller.js index 7b5b7a175..b64cb351d 100644 --- a/app/frontend/javascript/controllers/toggle_lock_controller.js +++ b/app/frontend/javascript/controllers/toggle_lock_controller.js @@ -1,44 +1,25 @@ import { Controller } from "@hotwired/stimulus"; -import { post } from "@rails/request.js"; /* - * Usage - * ===== + * Toggles lock icons and label text when the "Locked" checkbox changes. * - * Add data-controller="toggle-lock" to the button element - * - * Add data-toggle-lock-user-id-value="<%= user.id %>" to the button - * - * Example: - * + * Targets: + * checkbox - the + * icon - lock icon elements (shown when checked, removed when unchecked) + * labelText - the text span toggled between "Lock" / "Locked" */ export default class extends Controller { - static values = { - userId: Number - }; + static targets = ["checkbox", "icon", "labelText"]; - get url() { - // Construct the URL from the user ID - return `/users/${this.userIdValue}/toggle_lock_status`; - } + toggle() { + const locked = this.checkboxTarget.checked; - async toggle(event) { - event.preventDefault(); - event.stopPropagation(); - - try { - // Use turbo-stream response kind to handle Turbo Stream responses - await post(this.url, { - responseKind: "turbo-stream" - }); - // Turbo Stream will automatically update the page and show flash messages - } catch (error) { - console.error("Error toggling lock status:", error); + if (this.hasLabelTextTarget) { + this.labelTextTarget.textContent = locked ? "Locked" : "Lock"; } + + this.iconTargets.forEach((el) => { + el.style.display = locked ? "" : "none"; + }); } } diff --git a/app/frontend/javascript/controllers/toggle_user_icon_controller.js b/app/frontend/javascript/controllers/toggle_user_icon_controller.js new file mode 100644 index 000000000..34b7ea84f --- /dev/null +++ b/app/frontend/javascript/controllers/toggle_user_icon_controller.js @@ -0,0 +1,36 @@ +import { Controller } from "@hotwired/stimulus"; + +/* + * Toggles lock/admin icons in the page title and form labels + * when the corresponding checkboxes change. + * + * Targets: + * lockCheckbox - the lock + * lockIcon - lock icon elements (shown when checked, hidden when unchecked) + * lockLabel - text span toggled between "Lock" / "Locked" + * adminCheckbox - the admin + * adminIcon - admin icon elements (shown when checked, hidden when unchecked) + */ +export default class extends Controller { + static targets = ["lockCheckbox", "lockIcon", "lockLabel", "adminCheckbox", "adminIcon"]; + + toggleLock() { + const locked = this.lockCheckboxTarget.checked; + + if (this.hasLockLabelTarget) { + this.lockLabelTarget.textContent = locked ? "Locked" : "Lock"; + } + + this.lockIconTargets.forEach((el) => { + el.style.display = locked ? "" : "none"; + }); + } + + toggleAdmin() { + const admin = this.adminCheckboxTarget.checked; + + this.adminIconTargets.forEach((el) => { + el.style.display = admin ? "" : "none"; + }); + } +} diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 76c4fa82f..4e9f8c3f5 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -142,10 +142,12 @@ def favicon_file end def email_confirmation_icon(user) - if user.confirmed_at.present? - content_tag(:span, "confirmed", class: "text-green-600 ml-2 font-medium", title: "Email confirmed") + if user.unconfirmed_email.present? + content_tag(:span, "pending confirmation", class: "text-yellow-600 font-medium", title: "Email change pending confirmation") + elsif user.confirmed_at.present? + content_tag(:span, "confirmed", class: "text-green-600 font-medium", title: "Email confirmed") else - content_tag(:span, "unconfirmed", class: "text-red-600 ml-2 font-medium", title: "Email not confirmed") + content_tag(:span, "unconfirmed", class: "text-red-600 font-medium", title: "Email not confirmed") end end diff --git a/app/mailers/devise_mailer.rb b/app/mailers/devise_mailer.rb index 9167acef2..a39ab3cf5 100644 --- a/app/mailers/devise_mailer.rb +++ b/app/mailers/devise_mailer.rb @@ -6,6 +6,7 @@ class DeviseMailer < Devise::Mailer before_action :set_branding after_action :create_notification_record + after_action :track_devise_email_event default from: ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") default reply_to: ENV.fetch("REPLY_TO_EMAIL", "programs@awbw.org") @@ -100,4 +101,21 @@ def notify_admin_if_needed(kind) ) end end + + def track_devise_email_event + return unless @record.is_a?(User) + + event_name = { + "reset_password_instructions" => "auth.reset_password_email_sent", + "confirmation_instructions" => "auth.confirmation_email_sent", + "unlock_instructions" => "auth.unlock_email_sent" + }[action_name] + return unless event_name + + Analytics::AhoyTracker.track_auth_event( + event_name, + { record_id: @record.id, record_type: "User" }, + user: Current.user + ) + end end diff --git a/app/models/concerns/ahoy_trackable.rb b/app/models/concerns/ahoy_trackable.rb index ff4c98406..060e36965 100644 --- a/app/models/concerns/ahoy_trackable.rb +++ b/app/models/concerns/ahoy_trackable.rb @@ -2,9 +2,11 @@ module AhoyTrackable extend ActiveSupport::Concern included do - after_create -> { track_lifecycle_event("create") } + after_create -> { track_create_event } after_update -> { track_update_event } - after_destroy -> { track_lifecycle_event("destroy") } + after_destroy -> { track_lifecycle_event("destroy", @_destroy_snapshot || {}) } + before_save :capture_pending_association_removals + before_destroy :capture_destroy_snapshot end private @@ -22,17 +24,245 @@ def devise_only_changes?(changes) (changes.keys - auth_fields).empty? end + def track_create_event + extra = {} + + attrs = attributes.except("updated_at", "created_at") + safe_attrs = attrs.select { |k, v| v.present? && !k.match?(/password|token|secret|key|digest|salt|otp/i) } + extra[:attributes] = safe_attrs if safe_attrs.present? + + assoc_records = snapshot_nested_associated_records + extra[:associated_records] = assoc_records if assoc_records.present? + + track_lifecycle_event("create", extra) + end + def track_update_event return if previously_new_record? # Skip the fake "update" that happens right after create changes = previous_changes.except("updated_at", "created_at") - return if changes.empty? - return if devise_only_changes?(changes) + assoc_changes = collect_association_changes + + return if changes.empty? && assoc_changes.empty? + return if devise_only_changes?(changes) && assoc_changes.empty? + + extra = {} + extra[:changes] = format_tracked_changes(changes) if changes.present? && !devise_only_changes?(changes) + extra[:association_changes] = assoc_changes if assoc_changes.present? + + track_lifecycle_event("update", extra) + end + + # Capture records marked for destruction before autosave removes them from the target + def capture_pending_association_removals + @_pending_association_removals = {} + + self.class.nested_attributes_options.each_key do |assoc_name| + assoc = association(assoc_name.to_s) + next unless assoc.loaded? + + marked = Array(assoc.target).compact.select(&:marked_for_destruction?) + next if marked.empty? + + @_pending_association_removals[assoc_name] = marked.map do |record| + { action: "removed", id: record.id, type: record.class.name } + end + end + end + + def collect_association_changes + changes = {} + + self.class.nested_attributes_options.each_key do |assoc_name| + assoc = association(assoc_name.to_s) + next unless assoc.loaded? + + Array(assoc.target).compact.each do |record| + if record.previously_new_record? + changes[assoc_name] ||= [] + changes[assoc_name] << { action: "added", id: record.id, type: record.class.name } + else + record_changes = record.previous_changes.except("updated_at", "created_at") + next if record_changes.empty? + + changes[assoc_name] ||= [] + changes[assoc_name] << { action: "updated", id: record.id, type: record.class.name, + changes: format_tracked_changes(record_changes) } + end + end + end + + # Merge in removals captured before save + if @_pending_association_removals.present? + @_pending_association_removals.each do |assoc_name, removals| + changes[assoc_name] ||= [] + changes[assoc_name].concat(removals) + end + end + + # Track rich text changes + collect_rich_text_changes(changes) + + # Track attachment changes + collect_attachment_changes(changes) + + changes + end + + def collect_rich_text_changes(changes) + self.class.reflect_on_all_associations(:has_one).each do |assoc| + next if assoc.polymorphic? + next unless safe_assoc_class_name(assoc) == "ActionText::RichText" + + record = public_send(assoc.name) + next unless record&.persisted? + + rt_changes = record.previous_changes.slice("body", "plain_text_body") + next if rt_changes.empty? + + changes[assoc.name] ||= [] + changes[assoc.name] << { action: "updated", id: record.id, type: "ActionText::RichText", + changes: format_tracked_changes(rt_changes) } + end + end + + def collect_attachment_changes(changes) + self.class.reflect_on_all_associations.each do |assoc| + next if assoc.polymorphic? + next unless safe_assoc_class_name(assoc) == "ActiveStorage::Attachment" + + if assoc.macro == :has_many + Array(association(assoc.name.to_s).target).compact.each do |record| + next unless record.previously_new_record? + + changes[assoc.name] ||= [] + changes[assoc.name] << { action: "added", type: "ActiveStorage::Attachment", record_id: record.id, blob_id: record.blob_id } + end + elsif assoc.macro == :has_one + record = public_send(assoc.name) + next unless record&.previously_new_record? + + changes[assoc.name] ||= [] + changes[assoc.name] << { action: "added", type: "ActiveStorage::Attachment", record_id: record.id, blob_id: record.blob_id } + end + end - track_lifecycle_event("update") + if @_pending_association_removals.blank? + # Check for attachment removals via attachment_changes (Rails built-in tracking) + return unless respond_to?(:attachment_changes, true) && attachment_changes.present? + + attachment_changes.each do |name, change| + assoc_name = "#{name}_attachment".to_sym + if change.is_a?(ActiveStorage::Attached::Changes::DeleteOne) || change.is_a?(ActiveStorage::Attached::Changes::DeleteMany) + changes[assoc_name] ||= [] + changes[assoc_name] << { action: "removed", type: "ActiveStorage::Attachment" } + end + end + end + end + + def capture_destroy_snapshot + attrs = attributes.except("updated_at", "created_at") + safe_attrs = attrs.select { |k, v| v.present? && !k.match?(/password|token|secret|key|digest|salt|otp/i) } + + @_destroy_snapshot = { + attributes: safe_attrs, + associated_records: snapshot_associated_records + } + end + + def snapshot_nested_associated_records + records = {} + + self.class.nested_attributes_options.each_key do |assoc_name| + assoc = association(assoc_name.to_s) + next unless assoc.loaded? + + created = Array(assoc.target).compact.select(&:persisted?) + next if created.empty? + + records[assoc_name] = created.map { |r| { record_type: r.class.name, record_id: r.id } } + end + + # Capture rich text content set on create + self.class.reflect_on_all_associations(:has_one).each do |assoc| + next if assoc.polymorphic? + next unless safe_assoc_class_name(assoc) == "ActionText::RichText" + + record = public_send(assoc.name) + next unless record&.persisted? + + content = record.plain_text_body.presence || record.to_plain_text + next if content.blank? + + records[assoc.name] = { record_type: "ActionText::RichText", record_id: record.id, content: content } + end + + # Capture attachments added on create + self.class.reflect_on_all_associations.each do |assoc| + next if assoc.polymorphic? + next unless safe_assoc_class_name(assoc) == "ActiveStorage::Attachment" + + target = association(assoc.name.to_s).target + attached = Array(target).compact.select(&:persisted?) + next if attached.empty? + + records[assoc.name] = attached.map { |a| { action: "added", record_type: "ActiveStorage::Attachment", record_id: a.id, blob_id: a.blob_id } } + end + + records + end + + def snapshot_associated_records + records = {} + + self.class.reflect_on_all_associations(:has_many).each do |assoc| + next if assoc.is_a?(ActiveRecord::Reflection::ThroughReflection) + next if assoc.polymorphic? + class_name = safe_assoc_class_name(assoc) + next unless class_name + next if class_name == "ActiveStorage::Blob" + + if class_name == "ActiveStorage::Attachment" + attachments = public_send(assoc.name) + next unless attachments.any? + + records[assoc.name] = attachments.map { |a| { record_type: "ActiveStorage::Attachment", record_id: a.id, blob_id: a.blob_id } } + next + end + + begin + ids = public_send(assoc.name).pluck(:id) + next if ids.empty? + + records[assoc.name] = ids.map { |id| { record_type: assoc.klass.name, record_id: id } } + rescue ActiveRecord::StatementInvalid + next + end + end + + self.class.reflect_on_all_associations(:has_one).each do |assoc| + next if assoc.polymorphic? + class_name = safe_assoc_class_name(assoc) + next unless class_name + next if class_name == "ActiveStorage::Blob" + + record = public_send(assoc.name) + next unless record + + if class_name == "ActionText::RichText" + records[assoc.name] = { record_type: "ActionText::RichText", record_id: record.id, content: record.plain_text_body.presence || record.to_plain_text } + elsif class_name == "ActiveStorage::Attachment" + records[assoc.name] = { record_type: "ActiveStorage::Attachment", record_id: record.id, blob_id: record.blob_id } + else + records[assoc.name] = { record_type: assoc.klass.name, record_id: record.id } + end + end + + records end - def track_lifecycle_event(action) + def track_lifecycle_event(action, extra_properties = {}) return unless Current.user return if self.class.name.start_with?("Ahoy::") return if self.class.name.in?(%w[Notification ActiveStorage::Attachment ActiveStorage::Blob]) @@ -42,10 +272,24 @@ def track_lifecycle_event(action) Thread.current[:_ahoy_tracking] = true payload = Analytics::EventBuilder.lifecycle(action, self, user: Current.user) + payload[:properties].merge!(extra_properties) if extra_properties.present? Analytics::LifecycleBuffer.push(payload) rescue => e Rails.logger.error "Ahoy lifecycle tracking failed: #{e.message}" ensure Thread.current[:_ahoy_tracking] = false end + + def safe_assoc_class_name(assoc) + assoc.klass.name + rescue ArgumentError + nil + end + + def format_tracked_changes(changes) + safe_changes = changes.reject { |attr, _| attr.match?(/password|token|secret|key|digest|salt|otp/i) } + safe_changes.each_with_object({}) do |(attr, (before, after)), h| + h[attr] = { before: before, after: after } + end + end end diff --git a/app/models/organization_person.rb b/app/models/organization_person.rb index ff0b31c28..c1a0e3f6c 100644 --- a/app/models/organization_person.rb +++ b/app/models/organization_person.rb @@ -4,7 +4,6 @@ class OrganizationPerson < ApplicationRecord # Validations validates_presence_of :organization_id - validates_presence_of :person_id # Enum enum :position, { default: 0, liaison: 1, leader: 2, assistant: 3 } diff --git a/app/models/user.rb b/app/models/user.rb index 3149220ef..ecf0cc25b 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,16 +4,28 @@ class User < ApplicationRecord devise :database_authenticatable, :recoverable, :confirmable, :rememberable, :trackable, :validatable + attr_accessor :locked_will_change + + before_save :sync_locked_at_from_locked + after_update :track_welcome_instructions after_update :track_welcome_completion, if: :welcome_token_cleared? after_update :track_login_event after_update :track_email_change + after_update :sync_email_to_person + after_update :track_lock_change + after_update :track_admin_change + after_update :track_name_change + after_update :track_inactive_change + after_update :track_password_reset_sent before_destroy :track_account_deleted before_destroy :reassign_reports_and_logs_to_orphaned_user # Associations belongs_to :person, optional: true + belongs_to :created_by, class_name: "User", optional: true + belongs_to :updated_by, class_name: "User", optional: true has_many :bookmarks, dependent: :destroy has_many :comments, -> { newest_first }, as: :commentable, dependent: :destroy has_many :event_registrations, foreign_key: :registrant_id, dependent: :destroy @@ -48,6 +60,7 @@ class User < ApplicationRecord # Nested attributes accepts_nested_attributes_for :user_forms + accepts_nested_attributes_for :comments, reject_if: proc { |attrs| attrs["body"].blank? } before_validation :strip_whitespace @@ -63,16 +76,22 @@ class User < ApplicationRecord include SearchCop search_scope :search do attributes [ :email, :first_name, :last_name, :phone ] + attributes person_email: "person.email" + attributes person_email_2: "person.email_2" attributes user: "organizations.name" end - scope :active, -> { where(inactive: false) } + scope :has_access, -> { where(locked_at: nil, inactive: false).where.not(confirmed_at: nil) } def self.search_by_params(params) results = is_a?(ActiveRecord::Relation) ? self : all results = results.search(params[:search]) if params[:search].present? results = results.where(super_user: params[:super_user]) if params[:super_user].present? - results = results.where(inactive: params[:inactive]) if params[:inactive].present? + if params[:access] == "true" + results = results.has_access + elsif params[:access] == "false" + results = results.where("inactive = ? OR locked_at IS NOT NULL OR confirmed_at IS NULL", true) + end results end @@ -105,24 +124,6 @@ def submitted_monthly_report(submitted_date = Date.today, windows_type, organiza windows_type: windows_type).last end - def recent_activity(activity_limit = 10) - recent = [] - - # recent.concat(events.order(updated_at: :desc).limit(activity_limit)) - recent.concat(bookmarks.order(updated_at: :desc).limit(activity_limit)) - recent.concat(workshops.order(updated_at: :desc).limit(activity_limit)) - recent.concat(workshop_logs.order(updated_at: :desc).limit(activity_limit)) - recent.concat(workshop_variations_as_creator.order(updated_at: :desc).limit(activity_limit)) - recent.concat(stories_as_creator.order(updated_at: :desc).limit(activity_limit)) - # recent.concat(quotes.order(updated_at: :desc).limit(activity_limit)) - recent.concat(resources.order(updated_at: :desc).limit(activity_limit)) - recent.concat(reports.where(owner_type: "MonthlyReport").order(updated_at: :desc).limit(activity_limit)) - recent.concat(reports.where(owner_id: 7).order(updated_at: :desc).limit(activity_limit)) # TODO: remove hard-coded - - # Sort by the most recent timestamp (updated_at preferred, fallback to created_at) - recent.sort_by { |item| item.try(:updated_at) || item.created_at }.reverse.first(activity_limit * 8) - end - def organization_monthly_workshop_logs(date, *windows_type) where = windows_type.map { |wt| "windows_type_id = ?" } @@ -152,23 +153,10 @@ def name "#{first_name} #{last_name}" end - def agency_name - agency ? agency.name : "No agency." - end - - def has_bookmarkable?(bookmarkable, type: nil) - bookmarkable_ids(bookmarkable_type: type || bookmarkable.object.class.name).include?(bookmarkable.id) - end - - def bookmarkable_ids(bookmarkable_type:) - public_send("bookmarked_#{bookmarkable_type.downcase.pluralize}") - .pluck(:id) - end - - def primary_asset # method needed for idea_submission_fyi mailer + def primary_asset # method needed for idea_submitted_fyi mailer end - def gallery_assets # method needed for idea_submission_fyi mailer + def gallery_assets # method needed for idea_submitted_fyi mailer [] end @@ -196,10 +184,22 @@ def welcome_instructions_token_valid? end def track_auth_event(name, properties = {}) - payload = { name: name, properties: properties.merge(user_id: id) } + payload = { name: name, properties: properties.merge( + record_id: id, record_type: "User", updated_by_id: updated_by_id, + resource_type: "User", resource_id: id, resource_title: "#{self.name} (#{email})" + ) } Analytics::LifecycleBuffer.push(payload) end + def locked + locked_at.present? + end + + def locked=(value) + @locked_will_change = true + @locked_value = ActiveModel::Type::Boolean.new.cast(value) + end + private def strip_whitespace @@ -257,8 +257,33 @@ def track_account_deleted end def track_email_change - return unless saved_change_to_email? - track_auth_event("auth.email_changed") + if saved_change_to_email? + from, to = saved_change_to_email + track_auth_event("auth.email_changed", { changes: { email: { before: from, after: to } } }) + elsif saved_change_to_unconfirmed_email? && unconfirmed_email.present? + track_auth_event("auth.email_update_requested", { changes: { email: { before: email, after: unconfirmed_email } } }) + end + end + + def track_lock_change + return unless saved_change_to_locked_at? + + if locked_at.present? + track_auth_event("auth.account_locked", { locked_at: locked_at }) + else + track_auth_event("auth.account_unlocked") + end + end + + def track_admin_change + return unless saved_change_to_super_user? + + from, to = saved_change_to_super_user + if super_user? + track_auth_event("auth.admin_granted", { changes: { admin: { before: "revoked", after: "granted" } } }) + else + track_auth_event("auth.admin_revoked", { changes: { admin: { before: "granted", after: "revoked" } } }) + end end def track_login_event @@ -277,4 +302,52 @@ def welcome_token_cleared? def track_welcome_completion track_auth_event("auth.account_setup_completed") end + + def track_name_change + return unless saved_change_to_first_name? || saved_change_to_last_name? + props = {} + if saved_change_to_first_name? + from, to = saved_change_to_first_name + props[:first_name_from] = from + props[:first_name_to] = to + end + if saved_change_to_last_name? + from, to = saved_change_to_last_name + props[:last_name_from] = from + props[:last_name_to] = to + end + track_auth_event("auth.name_changed", props) + end + + def track_inactive_change + return unless saved_change_to_inactive? + + if inactive? + track_auth_event("auth.account_deactivated") + else + track_auth_event("auth.account_reactivated") + end + end + + def track_password_reset_sent + return unless saved_change_to_reset_password_sent_at? && reset_password_sent_at.present? + track_auth_event("auth.password_reset_sent", { sent_at: reset_password_sent_at }) + end + + def sync_email_to_person + return unless saved_change_to_email? && person.present? + + person.update(email: email) + end + + def sync_locked_at_from_locked + return unless @locked_will_change + + if @locked_value + self.locked_at = Time.current unless locked_at.present? + else + self.locked_at = nil + self.failed_attempts = 0 + end + end end diff --git a/app/policies/comment_policy.rb b/app/policies/comment_policy.rb index 39c437a2f..d9f4696ff 100644 --- a/app/policies/comment_policy.rb +++ b/app/policies/comment_policy.rb @@ -1,9 +1,9 @@ class CommentPolicy < ApplicationPolicy def index? - authenticated? + admin? end def create? - authenticated? + admin? end end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index ade6a0638..719a4c484 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -20,7 +20,7 @@ def update_password? = authenticated? end relation_scope(:colleagues) do |relation| - next relation.active if admin? + next relation.has_access if admin? colleague_person_ids = OrganizationPerson.where( organization_id: user.organization_ids diff --git a/app/services/analytics/event_builder.rb b/app/services/analytics/event_builder.rb index c55636ee0..79bc4d105 100644 --- a/app/services/analytics/event_builder.rb +++ b/app/services/analytics/event_builder.rb @@ -14,6 +14,10 @@ def self.lifecycle(action, resource, user: nil) end def self.safe_title(resource) + if resource.is_a?(Asset) && resource.file.attached? + return resource.file.filename.to_s + end + resource.decorate.title rescue resource.try(:title) || resource.try(:name) || resource.id diff --git a/app/views/admin/ahoy_activities/index.html.erb b/app/views/admin/ahoy_activities/index.html.erb index 9ef5b948c..c62bbf710 100644 --- a/app/views/admin/ahoy_activities/index.html.erb +++ b/app/views/admin/ahoy_activities/index.html.erb @@ -98,6 +98,7 @@ Time Event + Resource User Visit ID Properties @@ -115,6 +116,10 @@ <%= event.name %> + + <%= event.properties["resource_title"].presence || "—" %> + + <% if event.user %> <%= link_to event.user.full_name, diff --git a/app/views/comments/_comment_fields.html.erb b/app/views/comments/_comment_fields.html.erb index cf02ff282..f0829b81c 100644 --- a/app/views/comments/_comment_fields.html.erb +++ b/app/views/comments/_comment_fields.html.erb @@ -1,4 +1,4 @@ -<% comment_user = f.object.created_by %> +<% comment_user = f.object.updated_by || f.object.created_by %> <% initials = if comment_user&.person "#{comment_user.person.first_name.first}#{comment_user.person.last_name.first}".upcase elsif comment_user diff --git a/app/views/people/_form.html.erb b/app/views/people/_form.html.erb index 09be7d5e2..6060fdbeb 100644 --- a/app/views/people/_form.html.erb +++ b/app/views/people/_form.html.erb @@ -4,6 +4,8 @@ <%= f.hidden_field :created_by_id, value: current_user&.id unless f.object.persisted? %> <%= f.hidden_field :updated_by_id, value: current_user&.id %> + <% user_id = @user&.id || @person&.user&.id %> + <%= hidden_field_tag :user_id, user_id if user_id %>
@@ -281,7 +283,8 @@
Comments
+ data-controller="paginated-fields comment-edit-toggle" + data-paginated-fields-per-page-value="5"> <%= f.simple_fields_for :comments do |cf| %> <%= render "comments/comment_fields", f: cf %> <% end %> @@ -291,28 +294,9 @@ <%= link_to_add_association "➕ Add Comment", f, diff --git a/app/views/users/_form.html.erb b/app/views/users/_form.html.erb index ff739a083..849a515cc 100644 --- a/app/views/users/_form.html.erb +++ b/app/views/users/_form.html.erb @@ -1,4 +1,5 @@ -<%= simple_form_for @user, html: { multipart: true, class: "space-y-8" } do |f| %> +<%= simple_form_for @user, html: { multipart: true, class: "space-y-8", + data: { controller: "dirty-form" } } do |f| %> <%= render "shared/errors", resource: @user if @user.errors.any? %> <%= f.hidden_field :person_id, value: @person&.id || f.object.person_id || params[:person_id] %> @@ -12,43 +13,36 @@ <% if @person %> <%= f.hidden_field :first_name, value: @person.first_name %> <%= f.hidden_field :last_name, value: @person.last_name %> -
-
- -
-
+
+
+
+ +
<%= @person.first_name %>
-

- Edit on <%= link_to "person profile", person_path(@person), class: "underline" %> -

-
-
-
-
- -
-
+
+ +
<%= @person.last_name %>
-

- Edit on <%= link_to "person profile", person_path(@person), class: "underline" %> -

+
+
+ Edit on <%= link_to "person profile", person_path(@person), class: "underline" %>
<% end %> -
+
<% if allowed_to?(:manage?, User) %>
<%= f.input :email, label: @user.persisted? ? email_label_with_confirmation_icon(@user) : "Email", - hint: "Only editable by admins", + hint: @user.persisted? ? "Changing the email will send a confirmation email to the new address" : "Only editable by admins", input_html: { value: f.object.email.presence || @person&.email, class: "w-full" }, wrapper_html: { class: "w-full" }, @@ -62,14 +56,13 @@ data-controller="confirm-email" data-confirm-email-user-id-value="<%= @user.id %>" data-action="click->confirm-email#confirm" - class="btn bg-orange-600 hover:bg-orange-700 text-white border-orange-600 border font-medium"> + class="btn btn-warning-outline"> Manually confirm email
<% end %>
- <%= render "toggle_lock_button", user: @user %>
<% else %> @@ -92,9 +85,32 @@
<% if allowed_to?(:manage?, User) %>
-
- <%= f.input :inactive %> - <%= f.input :super_user, label: "Admin" %> +
+ <% if params[:admin].present? || f.object.inactive? %> + <%= f.input :inactive %> + <% end %> + +
<% end %> @@ -103,7 +119,9 @@ <% if allowed_to?(:manage?, User) %>
- <%= f.input :comment, as: :text, input_html: { rows: 3, class: "w-full" } %> + <% if params[:admin].present? || f.object.comment.present? %> + <%= f.input :comment, as: :text, input_html: { rows: 3, class: "w-full" } %> + <% end %> <% if params[:admin] %>
@@ -128,12 +146,43 @@ wrapper_html: { class: "w-full max-w-md" } %>
+ <% if f.object.persisted? %> + +
+
Comments
+ +
+ <%= f.simple_fields_for :comments do |cf| %> + <%= render "comments/comment_fields", f: cf %> + <% end %> + +
+ + + <%= link_to_add_association "➕ Add Comment", + f, + :comments, + partial: "comments/comment_fields", + class: "btn btn-secondary-outline" %> +
+
+ <% end %> +
<% if allowed_to?(:destroy?, f.object) %> <%= link_to "Delete", @user, class: "btn btn-danger-outline", data: { turbo_method: :delete, turbo_confirm: "Are you sure you want to delete?" } %> <% end %> - <%= link_to "Cancel", users_path, class: "btn btn-secondary-outline" %> + <%= link_to "Cancel", users_path, class: "btn btn-secondary-outline", + data: { action: "click->dirty-form#confirmCancel" } %> <%= f.button :submit, class: "btn btn-primary" %>
<% end %> diff --git a/app/views/users/_lock_status_indicator.html.erb b/app/views/users/_lock_status_indicator.html.erb index eaf2bd1b2..ad149ca68 100644 --- a/app/views/users/_lock_status_indicator.html.erb +++ b/app/views/users/_lock_status_indicator.html.erb @@ -1,7 +1,7 @@
<% if user.locked_at.present? %> - + Locked account <% else %> diff --git a/app/views/users/_search_boxes.html.erb b/app/views/users/_search_boxes.html.erb index 73e0c532a..75152a94d 100644 --- a/app/views/users/_search_boxes.html.erb +++ b/app/views/users/_search_boxes.html.erb @@ -21,14 +21,14 @@
- +
- <%= label_tag :inactive, "Status", class: "block text-sm font-medium text-gray-700 mb-1" %> - <%= select_tag :inactive, + <%= label_tag :access, "Access", class: "block text-sm font-medium text-gray-700 mb-1" %> + <%= select_tag :access, options_for_select([["All", ""], - ["Inactive", true], - ["Active", false]], - params[:inactive]), + ["Has access", "true"], + ["No access", "false"]], + params[:access]), class: "w-full rounded-lg border border-gray-300 px-3 py-2 text-gray-800 shadow-sm focus:border-blue-500 focus:ring focus:ring-blue-200 focus:outline-none pe-10", onchange: "this.form.submit()" %> @@ -36,7 +36,7 @@
- <%= label_tag :super_user, "Access levels", class: "block text-sm font-medium text-gray-700 mb-1" %> + <%= label_tag :super_user, "Permission level", class: "block text-sm font-medium text-gray-700 mb-1" %> <%= select_tag :super_user, options_for_select([["All users", ""], ["Admins", true], diff --git a/app/views/users/check_duplicates.html.erb b/app/views/users/check_duplicates.html.erb index 7cd123cb4..0e3daead0 100644 --- a/app/views/users/check_duplicates.html.erb +++ b/app/views/users/check_duplicates.html.erb @@ -25,32 +25,44 @@
  • <% if dup[:type] == "user" %> - <%= link_to dup[:name].presence || dup[:email], - user_path(dup[:id]), - target: "_blank", - class: "text-blue-600 hover:text-blue-800 underline font-medium" %> + USER EMAIL + <%= link_to dup[:email], user_path(dup[:id]), target: "_blank", + class: "text-gray-700 hover:text-blue-600 font-medium" %> existing user <% else %> - <%= link_to dup[:name], - person_path(dup[:id]), - target: "_blank", - class: "text-blue-600 hover:text-blue-800 underline font-medium" %> - person (no user) + PERSON <%= dup[:email_field]&.upcase || "PRIMARY" %> EMAIL + <%= link_to dup[:email], person_path(dup[:id]), target: "_blank", + class: "text-gray-700 hover:text-blue-600 font-medium" %> + <% if dup[:has_user] %> + user email: <%= dup[:user_email] %> + <% else %> + no user + <% end %> <% end %>
    -
    - <%= dup[:email] %> -
    + <% if dup[:type] == "user" && dup[:person_id].present? %> +
    + PERSON + <%= link_to dup[:name], person_path(dup[:person_id]), target: "_blank", + class: "text-gray-500 hover:text-blue-600 text-xs" %> +
    + <% elsif dup[:type] == "person" %> +
    + PERSON + <%= link_to dup[:name], person_path(dup[:id]), target: "_blank", + class: "text-gray-500 hover:text-blue-600 text-xs" %> +
    + <% end %>
    • <% end %> <% if @blocked %> -

    - A user with this email already exists. +

    + An existing user with this email already exists.

    -

    - Please edit the existing user instead. +

    + Edit the existing user or change this user's email.

    <% end %> diff --git a/app/views/users/edit.html.erb b/app/views/users/edit.html.erb index 82c008eb9..d88c8bdf3 100644 --- a/app/views/users/edit.html.erb +++ b/app/views/users/edit.html.erb @@ -2,12 +2,16 @@
    + transition-shadow duration-200 p-6" data-controller="toggle-user-icon">

    - Edit <%= @user.class.model_name.human %> + Edit <%= @user.class.model_name.human %>: <%= @user.email %> + +

    @@ -49,6 +53,19 @@
    + <% if @user.unconfirmed_email.present? %> +
    +
    + + Email change pending confirmation +
    +

    + A confirmation email was sent to <%= @user.unconfirmed_email %>. + The current email will remain <%= @user.email %> until confirmed. +

    +
    + <% end %> +
    <%= render "form", user: @user %> diff --git a/app/views/users/index.html.erb b/app/views/users/index.html.erb index 4fdbb76ce..1c86d4d52 100644 --- a/app/views/users/index.html.erb +++ b/app/views/users/index.html.erb @@ -28,7 +28,7 @@ Name Email Confirmed - Inactive + Access Admin Actions @@ -56,37 +56,59 @@ <% if user.confirmed_at.present? %> - - <% elsif user.welcome_instructions_sent_at.nil? %> + + <% elsif user.welcome_instructions_sent_at.present? %> + <% sent_info = "Confirmation instructions sent #{l(user.welcome_instructions_sent_at, format: :long)}" + sent_info += " by #{user.updated_by.name}" if user.updated_by.present? %> + + <% else %> <%= link_to "Invite", send_welcome_instructions_user_path(user, search: params[:search], super_user: params[:super_user], - inactive: params[:inactive], + access: params[:access], page: params[:page], number_of_items_per_page: params[:number_of_items_per_page]), method: :post, class: "btn btn-secondary-outline", - data: { turbo: false } unless user.welcome_instructions_sent_at.present? %> - <% else %> - -- + data: { turbo: false } %> + <% end %> + <% if user.unconfirmed_email.present? %> + <% end %> - + <% if user.inactive? %> - <% else %> - -- + <% end %> + <% if user.locked_at.present? %> + <% + lock_info = "Locked #{l(user.locked_at, format: :long)}" + if user.failed_attempts >= Devise.maximum_attempts + lock_info += " (too many failed login attempts)" + elsif user.updated_by.present? + lock_info += " by #{user.updated_by.name}" + end + %> + + <% end %> + <% if user.confirmed_at.nil? %> + + <% end %> + <% if !user.inactive? && !user.locked_at.present? && user.confirmed_at.present? %> + <% created_info = "Created #{l(user.created_at, format: :long)}" if user.created_at.present? + created_info += " by #{user.created_by.name}" if user.created_by.present? %> + <% end %> <% if user.super_user? %> - + <%= " by #{user.updated_by.name}" if user.updated_by.present? %>"> <% else %> -- <% end %> @@ -95,7 +117,7 @@ <%= link_to "Edit", edit_user_path(user), class: "btn btn-secondary-outline" %> - <%= link_to "Details", user_path(user), class: "btn btn-secondary-outline" %> + <%= link_to "View", user_path(user), class: "btn btn-secondary-outline" %> <% end %> diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 98241fd93..d6654bfb8 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -1,6 +1,7 @@
    -
    -
    +
    +
    +
    @@ -24,6 +25,19 @@ <% end %>
    + <% if @user.unconfirmed_email.present? %> +
    +
    + + Email change pending confirmation +
    +

    + A confirmation email was sent to <%= @user.unconfirmed_email %>. + The current email will remain <%= @user.email %> until confirmed. +

    +
    + <% end %> +
    @@ -31,6 +45,12 @@

    <%= @user.name %> + <% if @user.super_user? %> + + <% end %> + <% if @user.locked_at.present? %> + + <% end %>

    <% if @user.avatar.attached? %> @@ -50,16 +70,12 @@ Identity -
    -
    -
    Email
    -
    <%= @user.email.presence || "—" %>
    -
    +
    +
    Email
    +
    <%= @user.email.presence || "—" %>
    -
    -
    Phone
    -
    <%= @user.phone.presence || "—" %>
    -
    +
    Phone
    +
    <%= @user.phone.presence || "—" %>
    @@ -73,28 +89,42 @@ Account status -
    -
    +
    + <% if @user.inactive? %>
    Active
    -
    - <%= @user.inactive? ? "No" : "Yes" %> -
    -
    +
    No
    + <% end %> -
    -
    Admin
    -
    - <%= @user&.super_user? ? "Yes" : "No" %> +
    Admin
    +
    <%= @user&.super_user? ? "Yes" : "No" %>
    + <% if @last_admin_event %> +
    +
    + <%= l(@last_admin_event.time, format: :long) %> + <% if @last_admin_event.user %> + by <%= @last_admin_event.user.name %> + <% end %>
    -
    + <% end %> <% if @user.respond_to?(:locked_at) %> -
    -
    Locked
    -
    - <%= @user.locked_at.present? ? "Yes" : "No" %> +
    Locked
    +
    + <% if @user.locked_at.present? %> + Yes + <% else %> + No + <% end %> +
    + <% if @last_lock_event %> +
    +
    + <%= l(@last_lock_event.time, format: :long) %> + <% if @last_lock_event.user %> + by <%= @last_lock_event.user.name %> + <% end %>
    -
    + <% end %> <% end %>
    @@ -105,30 +135,20 @@ Authentication -
    +
    <% if @user.respond_to?(:sign_in_count) %> -
    -
    Sign-in count
    -
    <%= @user.sign_in_count %>
    -
    +
    Sign-in count
    +
    <%= @user.sign_in_count %>
    <% end %> <% if @user.respond_to?(:current_sign_in_at) %> -
    -
    Current sign-in
    -
    - <%= l(@user.current_sign_in_at, format: :long) if @user.current_sign_in_at %> -
    -
    +
    Current sign-in
    +
    <%= l(@user.current_sign_in_at, format: :long) if @user.current_sign_in_at %>
    <% end %> <% if @user.respond_to?(:last_sign_in_at) %> -
    -
    Last sign-in
    -
    - <%= l(@user.last_sign_in_at, format: :long) if @user.last_sign_in_at %> -
    -
    +
    Last sign-in
    +
    <%= l(@user.last_sign_in_at, format: :long) if @user.last_sign_in_at %>
    <% end %>
    @@ -139,26 +159,100 @@ Audit -
    -
    -
    Created at
    -
    - <%= @user.created_at.present? ? l(@user.created_at, format: :long) : "—" %> -
    -
    +
    +
    Created at
    +
    <%= @user.created_at.present? ? l(@user.created_at, format: :long) : "—" %>
    -
    -
    Updated at
    -
    <%= l(@user.updated_at, format: :long) %>
    -
    +
    Created by
    +
    <%= @user.created_by&.name.presence || "—" %>
    + +
    Updated at
    +
    <%= l(@user.updated_at, format: :long) %>
    + +
    Updated by
    +
    <%= @user.updated_by&.name.presence || "—" %>
    + + <% if @account_events.any? %> +
    +

    Account Activity

    + + + + + + + + + + + <% @account_events.each do |event| %> + + + + + + + <% end %> + +
    EventChangesTimeBy
    + <% if event.name.start_with?("auth.") %> + <%= event.name.sub(/\Aauth\./, "").titleize %> + <% else %> + <%= event.name.split(".").first.titleize %> + <% end %> + + <% changes = event.properties["changes"] %> + <% assoc_changes = event.properties["association_changes"] %> + <% if changes.present? %> + <% changes.each do |attr, vals| %> +
    <%= attr.titleize %>: <%= vals["after"] %>
    + <% end %> + <% end %> + <% if assoc_changes.present? %> + <% assoc_changes.each do |assoc_name, records| %> + <% Array(records).each do |record| %> +
    <%= assoc_name.to_s.titleize %>: <%= record["action"] %>
    + <% end %> + <% end %> + <% end %> +     		<%= l(event.time, format: :long) %><%= event.user&.name.presence || "—" %>
    +
    + <%= tailwind_paginate @account_events %> +
    +
    + <% end %> + - <%= render "comments/section", commentable: @user %> + <% if @comments.any? %> +
    +

    Comments

    +
    + <% @comments.each do |comment| %> +
    + + <%= comment.created_by ? "#{comment.created_by.first_name&.first}#{comment.created_by.last_name&.first}".upcase : "—" %> + + + <%= comment.created_at.strftime("%-m/%-d/%Y %-I:%M %p") %> + +
    + <%= simple_format(comment.body) %> +
    +
    + <% end %> +
    +
    + <%= tailwind_paginate @comments, param_name: :comments_page %> +
    +
    + <% end %>
    +
    diff --git a/app/views/users/toggle_lock_status.turbo_stream.erb b/app/views/users/toggle_lock_status.turbo_stream.erb index 4314459e6..636c5d5fd 100644 --- a/app/views/users/toggle_lock_status.turbo_stream.erb +++ b/app/views/users/toggle_lock_status.turbo_stream.erb @@ -1,3 +1,2 @@ <%= turbo_stream.replace "user_lock_status", partial: "users/lock_status_indicator", locals: { user: @user } %> -<%= turbo_stream.replace "toggle_lock_button", partial: "users/toggle_lock_button", locals: { user: @user } %> <%= turbo_stream.replace "flash_now", partial: "shared/flash_messages" %> diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 3aae7612b..b8f22ffc8 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -163,6 +163,10 @@ # :none = No lock strategy. You should handle locking by yourself. config.lock_strategy = :failed_attempts + # Number of authentication tries before locking an account if lock_strategy + # is failed attempts. + config.maximum_attempts = 10 + # Defines which key will be used when locking and unlocking an account config.unlock_keys = [ :email ] @@ -172,11 +176,7 @@ # :both = Enables both strategies # :none = No unlock strategy. You should handle unlocking by yourself. # config.unlock_strategy = :both - config.unlock_strategy = :email - - # Number of authentication tries before locking an account if lock_strategy - # is failed attempts. - config.maximum_attempts = 10 + config.unlock_strategy = :none # Time interval to unlock the account if :time is enabled as unlock_strategy. # config.unlock_in = 1.hour diff --git a/db/migrate/20260216120927_add_created_by_and_updated_by_to_users.rb b/db/migrate/20260216120927_add_created_by_and_updated_by_to_users.rb new file mode 100644 index 000000000..35d98152d --- /dev/null +++ b/db/migrate/20260216120927_add_created_by_and_updated_by_to_users.rb @@ -0,0 +1,10 @@ +class AddCreatedByAndUpdatedByToUsers < ActiveRecord::Migration[8.1] + def change + unless column_exists?(:users, :created_by_id) + add_reference :users, :created_by, type: :integer, foreign_key: { to_table: :users } + end + unless column_exists?(:users, :updated_by_id) + add_reference :users, :updated_by, type: :integer, foreign_key: { to_table: :users } + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 105bddc8b..0a2e5a5b7 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[8.1].define(version: 2026_02_15_122713) do +ActiveRecord::Schema[8.1].define(version: 2026_02_16_120927) do create_table "action_text_mentions", charset: "utf8mb4", collation: "utf8mb4_unicode_ci", force: :cascade do |t| t.bigint "action_text_rich_text_id", null: false t.datetime "created_at", null: false @@ -869,6 +869,7 @@ t.string "confirmation_token" t.datetime "confirmed_at" t.datetime "created_at", precision: nil + t.integer "created_by_id" t.datetime "current_sign_in_at", precision: nil t.string "current_sign_in_ip" t.string "email", default: "", null: false @@ -901,6 +902,7 @@ t.string "unconfirmed_email" t.string "unlock_token" t.datetime "updated_at", precision: nil + t.integer "updated_by_id" t.datetime "welcome_instructions_created_at" t.datetime "welcome_instructions_sent_at" t.string "welcome_instructions_token" @@ -908,10 +910,12 @@ t.string "zip2" t.index ["agency_id"], name: "index_users_on_agency_id" t.index ["confirmation_token"], name: "index_users_on_confirmation_token", unique: true + t.index ["created_by_id"], name: "index_users_on_created_by_id" t.index ["email"], name: "index_users_on_email", unique: true t.index ["person_id"], name: "index_users_on_person_id" t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true t.index ["unlock_token"], name: "index_users_on_unlock_token", unique: true + t.index ["updated_by_id"], name: "index_users_on_updated_by_id" t.index ["welcome_instructions_token"], name: "index_users_on_welcome_instructions_token", unique: true end @@ -1246,6 +1250,8 @@ add_foreign_key "user_permissions", "users" add_foreign_key "users", "organizations", column: "agency_id" add_foreign_key "users", "people" + add_foreign_key "users", "users", column: "created_by_id" + add_foreign_key "users", "users", column: "updated_by_id" add_foreign_key "workshop_age_ranges", "age_ranges" add_foreign_key "workshop_age_ranges", "workshops" add_foreign_key "workshop_ideas", "users", column: "created_by_id" diff --git a/lib/tasks/bulk_invite.rake b/lib/tasks/bulk_invite.rake new file mode 100644 index 000000000..1802402c3 --- /dev/null +++ b/lib/tasks/bulk_invite.rake @@ -0,0 +1,57 @@ +namespace :users do + # bin/rails users:bulk_invite IDS=1,2,3 + # bin/rails users:bulk_invite IDS=1,2,3 DRY_RUN=true + desc "Bulk send welcome instructions to specified users" + task bulk_invite: :environment do + unless ENV["IDS"].present? + puts "Usage: bin/rails users:bulk_invite IDS=1,2,3 [DRY_RUN=true]" + next + end + + ids = ENV["IDS"].split(",").map(&:strip) + users = User.where(id: ids) + total = users.count + + missing = ids.map(&:to_i) - users.pluck(:id) + puts "Warning: IDs not found: #{missing.join(", ")}" if missing.any? + + if total == 0 + puts "No uninvited, unconfirmed users found." + next + end + + puts "Found #{total} uninvited, unconfirmed users:" + users.find_each { |u| puts " #{u.id}: #{u.name} <#{u.email}>" } + puts "" + + if ENV["DRY_RUN"] == "true" + puts "DRY_RUN=true — no invitations sent." + next + end + + print "Send welcome instructions to all #{total} users? (yes/no): " + confirm = $stdin.gets&.strip + unless confirm == "yes" + puts "Aborted." + next + end + + sent = 0 + errors = [] + + users.find_each do |user| + user.set_welcome_instructions_token! + user.update!(welcome_instructions_sent_at: Time.current) + user.send_confirmation_instructions + sent += 1 + puts " Invited #{user.email} (#{sent}/#{total})" + rescue => e + errors << { id: user.id, email: user.email, error: e.message } + puts " FAILED #{user.email}: #{e.message}" + end + + puts "" + puts "Done. Sent: #{sent}, Failed: #{errors.size}" + errors.each { |e| puts " FAILED: #{e[:email]} — #{e[:error]}" } if errors.any? + end +end diff --git a/spec/requests/users_check_duplicates_spec.rb b/spec/requests/users_check_duplicates_spec.rb index af57d8494..d4c51718e 100644 --- a/spec/requests/users_check_duplicates_spec.rb +++ b/spec/requests/users_check_duplicates_spec.rb @@ -18,8 +18,8 @@ expect(response).to have_http_status(:ok) expect(response.body).to include("existing user") - expect(response.body).to include("A user with this email already exists") - expect(response.body).to include("Please edit the existing user instead") + expect(response.body).to include("with this email already exists") + expect(response.body).to include("Edit the existing user or change this user") end it "hides the Create Anyway button" do @@ -57,7 +57,7 @@ get check_duplicates_users_path, params: { email: "solo@testmail.org" } expect(response).to have_http_status(:ok) - expect(response.body).to include("person (no user)") + expect(response.body).to include("no user") expect(response.body).to include("Solo Person") end @@ -70,7 +70,7 @@ it "does not show the block message" do get check_duplicates_users_path, params: { email: "solo@testmail.org" } - expect(response.body).not_to include("A user with this email already exists") + expect(response.body).not_to include("with this email already exists") end end @@ -88,7 +88,7 @@ expect(response).to have_http_status(:ok) expect(response.body).to include("Alt Email") - expect(response.body).to include("person (no user)") + expect(response.body).to include("no user") end end @@ -104,7 +104,7 @@ expect(response).to have_http_status(:ok) # Should not appear as a person match (person has a user, so person.email is skipped) - expect(response.body).not_to include("person (no user)") + expect(response.body).not_to include("no user") end end @@ -126,7 +126,7 @@ expect(response).to have_http_status(:ok) expect(response.body).to include("existing user") - expect(response.body).to include("person (no user)") + expect(response.body).to include("no user") end it "is blocked because a user exists" do @@ -193,7 +193,7 @@ get check_duplicates_users_path, params: { email: "brand.new@testmail.org" } expect(response.body).not_to include("existing user") - expect(response.body).not_to include("person (no user)") + expect(response.body).not_to include("no user") end end diff --git a/spec/system/reset_password_person_spec.rb b/spec/system/reset_password_person_spec.rb index b2839b977..89473bec2 100644 --- a/spec/system/reset_password_person_spec.rb +++ b/spec/system/reset_password_person_spec.rb @@ -9,8 +9,8 @@ sign_in person_user # Start on a page where the user nav is visible visit root_path - # Open user nav dropdown - find('#avatar button').click + # Open user nav dropdown (wait for avatar to appear after page load) + find('#avatar button', wait: 10).click # Click "Change password" click_link 'Change password' end diff --git a/spec/views/users/edit.html.erb_spec.rb b/spec/views/users/edit.html.erb_spec.rb index aeb9627d6..c84d2be0f 100644 --- a/spec/views/users/edit.html.erb_spec.rb +++ b/spec/views/users/edit.html.erb_spec.rb @@ -14,15 +14,29 @@ render assert_select "form[action=?][method=?]", user_path(user), "post" do - # Inputs - %w[ email inactive super_user ].each do |field| + %w[ email locked super_user ].each do |field| assert_select "input[name=?]", "user[#{field}]" end + end + end - # Textareas - %w[comment].each do |field| - assert_select "textarea[name=?]", "user[#{field}]" - end + it "renders the comments section" do + render + + assert_select "#comments-section" do + assert_select ".font-semibold", text: "Comments" + end + end + + context "when params[:admin] is present" do + before do + allow(view).to receive(:params).and_return(ActionController::Parameters.new(admin: "true")) + end + + it "renders the legacy comment field" do + render + + assert_select "textarea[name=?]", "user[comment]" end end end diff --git a/spec/views/users/new.html.erb_spec.rb b/spec/views/users/new.html.erb_spec.rb index f2f5d595c..c68a20141 100644 --- a/spec/views/users/new.html.erb_spec.rb +++ b/spec/views/users/new.html.erb_spec.rb @@ -17,9 +17,19 @@ assert_select "form[action=?][method=?]", users_path, "post" do assert_select "input[name=?]", "user[email]" - assert_select "textarea[name=?]", "user[comment]" - assert_select "input[name=?]", "user[inactive]" assert_select "input[name=?]", "user[super_user]" end end + + context "when params[:admin] is present" do + before do + allow(view).to receive(:params).and_return(ActionController::Parameters.new(admin: "true")) + end + + it "renders the legacy comment field" do + render + + assert_select "textarea[name=?]", "user[comment]" + end + end end diff --git a/spec/views/users/show.html.erb_spec.rb b/spec/views/users/show.html.erb_spec.rb index f92251c7f..82ed745dc 100644 --- a/spec/views/users/show.html.erb_spec.rb +++ b/spec/views/users/show.html.erb_spec.rb @@ -14,6 +14,8 @@ before do assign(:user, user) + assign(:account_events, Ahoy::Event.none.paginate(page: 1)) + assign(:comments, user.comments.none.paginate(page: 1)) end # -------------------------------------------------- @@ -39,7 +41,6 @@ it "renders account status section" do expect(rendered).to include("Account status") - expect(rendered).to include("Active") expect(rendered).to include("Admin") end