From 99d20418fa73e4b867d352a0ab0bb5c1df2840c0 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sat, 22 Aug 2026 21:39:59 -0400 Subject: [PATCH 01/15] Track rich text edits, which were passing through unrecorded MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every rhino_* field was invisible to activity tracking: ActionText::RichText descends from ActionText::Record, so it gets no lifecycle event of its own, and the collector meant to fold its diff into the parent's event never fired. Two reasons it never fired. It looked the association up with a string, but the association cache is symbol-keyed — a string hands back a fresh, unloaded association whose target is nil, so there was never a record to inspect. And it read the diff after the save, by which point the body is no longer reliably readable as a change; the capture now happens in before_save while it's still dirty, mirroring the association-removal capture next to it. The change lands on the record's own event keyed by the attribute (rhino_description), because a reader thinks of it as a field, not an association. Plain text rather than markup, truncated to a preview, so an event stays readable and doesn't carry a whole article. Note for follow-up: collect_association_changes and collect_attachment_changes still use the string lookup, so nested-attribute and attachment changes are absent from parent events for the same reason. Left alone here — switching them on changes the payload of a lot of events at once. Co-Authored-By: Claude Opus 5 (1M context) --- app/models/concerns/ahoy_trackable.rb | 43 ++++++++---- spec/models/concerns/ahoy_trackable_spec.rb | 73 +++++++++++++++++++++ 2 files changed, 104 insertions(+), 12 deletions(-) create mode 100644 spec/models/concerns/ahoy_trackable_spec.rb diff --git a/app/models/concerns/ahoy_trackable.rb b/app/models/concerns/ahoy_trackable.rb index 4c8116740f..5fc246da7c 100644 --- a/app/models/concerns/ahoy_trackable.rb +++ b/app/models/concerns/ahoy_trackable.rb @@ -1,11 +1,16 @@ module AhoyTrackable extend ActiveSupport::Concern + # Long-form bodies are stored as a readable preview, not in full: an event is a + # summary, and whole articles would bloat every row of ahoy_events. + RICH_TEXT_PREVIEW_LIMIT = 300 + included do after_create -> { track_create_event } after_update -> { track_update_event } after_destroy -> { track_lifecycle_event("destroy", @_destroy_snapshot || {}) } before_save :capture_pending_association_removals + before_save :capture_pending_rich_text_changes before_destroy :capture_destroy_snapshot end @@ -40,7 +45,7 @@ def track_create_event 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") + changes = previous_changes.except("updated_at", "created_at").merge(@_pending_rich_text_changes.to_h) assoc_changes = collect_association_changes return if changes.empty? && assoc_changes.empty? @@ -100,32 +105,46 @@ def collect_association_changes 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) + # Rich text saves through the parent's autosave chain, so by the time the update + # event is built the body is no longer reliably readable as a change — capture it + # here, while it's still dirty. Keyed on the attribute (`rhino_body`), not the + # association, because a reader thinks of it as a field of the record; the plain + # text is stored rather than the markup, truncated, so an event stays readable + # and small. + def capture_pending_rich_text_changes + @_pending_rich_text_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? + # Reading the association would build an empty record for an untouched field. + # The name has to stay a symbol: the association cache is symbol-keyed, and a + # string lookup hands back a fresh, unloaded association whose target is nil. + record = association(assoc.name).target + # plain_text_body is derived when the rich text itself saves, which happens + # after this, so the body is the only diff available on a first edit. + diff = record&.changes&.values_at("plain_text_body", "body")&.compact&.first + next if diff.blank? - rt_changes = record.previous_changes.slice("body", "plain_text_body") - next if rt_changes.empty? + before, after = diff.map { |value| rich_text_preview(value) } + next if before == after - changes[assoc.name] ||= [] - changes[assoc.name] << { action: "updated", id: record.id, type: "ActionText::RichText", - changes: format_tracked_changes(rt_changes) } + @_pending_rich_text_changes[assoc.name.to_s.delete_prefix("rich_text_")] = [ before, after ] end end + def rich_text_preview(value) + text = value.respond_to?(:to_plain_text) ? value.to_plain_text : value.to_s + text.squish.truncate(RICH_TEXT_PREVIEW_LIMIT) + end + def collect_attachment_changes(changes) self.class.reflect_on_all_associations.each do |assoc| next if assoc.polymorphic? diff --git a/spec/models/concerns/ahoy_trackable_spec.rb b/spec/models/concerns/ahoy_trackable_spec.rb new file mode 100644 index 0000000000..33eed82fcb --- /dev/null +++ b/spec/models/concerns/ahoy_trackable_spec.rb @@ -0,0 +1,73 @@ +require "rails_helper" + +RSpec.describe AhoyTrackable do + # Events are buffered here and written by ApplicationController's after_action, + # so a model-level spec reads the buffer rather than the table. + def buffered + Analytics::LifecycleBuffer.store + end + + def event_named(name) + buffered.find { |event| event[:name] == name } + end + + before do + Current.user = create(:user) + Analytics::LifecycleBuffer.store.clear + end + + after { Current.user = nil } + + describe "rich text" do + let(:event) { create(:event) } + + it "records a rich text edit as a change on the record's own event" do + Analytics::LifecycleBuffer.store.clear + + event.update!(rhino_description: "

Bring a friend

", title: "Renamed") + + changes = event_named("update.event")[:properties][:changes] + expect(changes["rhino_description"]).to eq({ before: "", after: "Bring a friend" }) + expect(changes["title"]).to be_present + end + + it "stores the plain text rather than the markup" do + Analytics::LifecycleBuffer.store.clear + + event.update!(rhino_description: "

Bring a friend

") + + after_text = event_named("update.event")[:properties][:changes]["rhino_description"][:after] + expect(after_text).to eq("Bring a friend") + expect(after_text).not_to include("<") + end + + it "records an edit that touches nothing but the rich text" do + event.update!(rhino_description: "

First

") + Analytics::LifecycleBuffer.store.clear + + event.update!(rhino_description: "

Second

") + + expect(event_named("update.event")[:properties][:changes]["rhino_description"]) + .to eq({ before: "First", after: "Second" }) + end + + it "truncates a long body to a preview" do + Analytics::LifecycleBuffer.store.clear + + event.update!(rhino_description: "

#{"word " * 200}

") + + after_text = event_named("update.event")[:properties][:changes]["rhino_description"][:after] + expect(after_text.length).to eq(AhoyTrackable::RICH_TEXT_PREVIEW_LIMIT) + expect(after_text).to end_with("...") + end + + it "records nothing when the rich text is saved unchanged" do + event.update!(rhino_description: "

Same

") + Analytics::LifecycleBuffer.store.clear + + event.update!(rhino_description: "

Same

") + + expect(event_named("update.event")).to be_nil + end + end +end From 4d50f92c3bb431155a3b6ea824c2905cf77b9903 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 02:20:30 -0400 Subject: [PATCH 02/15] Report what a save did to a record's children and attachments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collectors for nested records and attachments never produced anything, for the same two reasons the rich text one didn't: they looked associations up with a string, which misses the symbol-keyed association cache and returns an empty target, and they read the result after autosave had already cleared the dirty state they were looking for. Both now read their pending state in before_save alongside the rich text capture, and the update event assembles what was captured. Ids are resolved at assembly time, since a record added through nested attributes has none until the save goes through. Attachments are read from ActiveStorage's staged changes — the only reliable account of an attach or purge — and carry the filename, which is the part a reader of a change log actually wants. So an admin's save now reports what it did to the record's children, not just its own columns. The children's own events are unaffected. Co-Authored-By: Claude Opus 5 (1M context) --- app/models/concerns/ahoy_trackable.rb | 132 +++++++++----------- spec/models/concerns/ahoy_trackable_spec.rb | 49 ++++++++ 2 files changed, 105 insertions(+), 76 deletions(-) diff --git a/app/models/concerns/ahoy_trackable.rb b/app/models/concerns/ahoy_trackable.rb index 5fc246da7c..fad73ae1b0 100644 --- a/app/models/concerns/ahoy_trackable.rb +++ b/app/models/concerns/ahoy_trackable.rb @@ -9,8 +9,7 @@ module AhoyTrackable after_create -> { track_create_event } after_update -> { track_update_event } after_destroy -> { track_lifecycle_event("destroy", @_destroy_snapshot || {}) } - before_save :capture_pending_association_removals - before_save :capture_pending_rich_text_changes + before_save :capture_pending_changes before_destroy :capture_destroy_snapshot end @@ -58,57 +57,49 @@ def track_update_event 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? + # Autosave writes children, rich text, and attachments *after* this record's own + # callbacks have run, clearing their dirty state on the way — so everything the + # update event says about them has to be read here, while it's still pending. + def capture_pending_changes + capture_pending_association_changes + capture_pending_rich_text_changes + capture_pending_attachment_changes + end - marked = Array(assoc.target).compact.select(&:marked_for_destruction?) - next if marked.empty? + def capture_pending_association_changes + @_pending_association_changes = [] - @_pending_association_removals[assoc_name] = marked.map do |record| - { action: "removed", id: record.id, type: record.class.name } + self.class.nested_attributes_options.each_key do |assoc_name| + # A symbol: the association cache is symbol-keyed, and a string lookup hands + # back a fresh, unloaded association whose target is empty. + Array(association(assoc_name).target).compact.each do |record| + pending = pending_association_change(assoc_name, record) + @_pending_association_changes << pending if pending 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? + def pending_association_change(assoc_name, record) + return { assoc: assoc_name, record: record, action: "removed" } if record.marked_for_destruction? + return { assoc: assoc_name, record: record, action: "added" } if record.new_record? - 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 + record_changes = record.changes.except("updated_at", "created_at") + return if record_changes.empty? - # 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 + { assoc: assoc_name, record: record, action: "updated", changes: format_tracked_changes(record_changes) } + end - # Track attachment changes - collect_attachment_changes(changes) + # Ids are read now rather than at capture time: a record added through nested + # attributes has none until the save goes through. + def collect_association_changes + (@_pending_association_changes.to_a + @_pending_attachment_changes.to_a).each_with_object({}) do |pending, changes| + entry = { action: pending[:action], type: pending[:type] || pending[:record].class.name } + entry[:id] = pending[:record].id if pending[:record] + entry[:filename] = pending[:filename] if pending[:filename] + entry[:changes] = pending[:changes] if pending[:changes] - changes + (changes[pending[:assoc]] ||= []) << entry + end end # Rich text saves through the parent's autosave chain, so by the time the update @@ -145,39 +136,28 @@ def rich_text_preview(value) text.squish.truncate(RICH_TEXT_PREVIEW_LIMIT) 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 + # ActiveStorage stages attach/purge on the record and applies it during save, so + # the staged change is the only reliable description of what happened. + def capture_pending_attachment_changes + @_pending_attachment_changes = [] + return unless respond_to?(:attachment_changes, true) + + attachment_changes.each do |name, change| + removal = change.is_a?(ActiveStorage::Attached::Changes::DeleteOne) || + change.is_a?(ActiveStorage::Attached::Changes::DeleteMany) + + @_pending_attachment_changes << { + assoc: :"#{name}_attachment", + type: "ActiveStorage::Attachment", + action: removal ? "removed" : "added", + filename: (attachment_filenames(change) unless removal) + }.compact end + end - 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 + def attachment_filenames(change) + blobs = change.try(:blobs) || Array(change.try(:blob)) + blobs.filter_map { |blob| blob.filename.to_s.presence }.join(", ").presence end def capture_destroy_snapshot @@ -194,7 +174,7 @@ def snapshot_nested_associated_records records = {} self.class.nested_attributes_options.each_key do |assoc_name| - assoc = association(assoc_name.to_s) + assoc = association(assoc_name) next unless assoc.loaded? created = Array(assoc.target).compact.select(&:persisted?) @@ -222,7 +202,7 @@ def snapshot_nested_associated_records next if assoc.polymorphic? next unless safe_assoc_class_name(assoc) == "ActiveStorage::Attachment" - target = association(assoc.name.to_s).target + target = association(assoc.name).target attached = Array(target).compact.select(&:persisted?) next if attached.empty? diff --git a/spec/models/concerns/ahoy_trackable_spec.rb b/spec/models/concerns/ahoy_trackable_spec.rb index 33eed82fcb..23d134f685 100644 --- a/spec/models/concerns/ahoy_trackable_spec.rb +++ b/spec/models/concerns/ahoy_trackable_spec.rb @@ -70,4 +70,53 @@ def event_named(name) expect(event_named("update.event")).to be_nil end end + + describe "nested records" do + it "records an added nested record on the parent's event" do + registration = create(:event_registration) + Analytics::LifecycleBuffer.store.clear + + registration.update!(comments_attributes: [ { body: "Called the registrant" } ]) + + added = event_named("update.event_registration")[:properties][:association_changes][:comments].first + expect(added).to include(action: "added", type: "Comment") + end + + it "records an edited nested record with its field changes" do + person = create(:person) + license = create(:professional_license, person: person, number: "LIC-1") + person.professional_licenses.load + Analytics::LifecycleBuffer.store.clear + + person.update!(professional_licenses_attributes: [ { id: license.id, number: "LIC-2" } ]) + + edited = event_named("update.person")[:properties][:association_changes][:professional_licenses].first + expect(edited).to include(action: "updated") + expect(edited[:changes]["number"]).to eq({ before: "LIC-1", after: "LIC-2" }) + end + + it "records a removed nested record" do + person = create(:person) + license = create(:professional_license, person: person) + person.professional_licenses.load + Analytics::LifecycleBuffer.store.clear + + person.update!(professional_licenses_attributes: [ { id: license.id, _destroy: "1" } ]) + + removed = event_named("update.person")[:properties][:association_changes][:professional_licenses].first + expect(removed).to include(action: "removed", id: license.id, type: "ProfessionalLicense") + end + end + + describe "attachments" do + it "records an added attachment on the record's event" do + person = create(:person) + Analytics::LifecycleBuffer.store.clear + + person.update!(avatar: Rack::Test::UploadedFile.new(Rails.root.join("app/assets/images/missing.png"), "image/png")) + + attached = event_named("update.person")[:properties][:association_changes][:avatar_attachment].first + expect(attached).to include(action: "added", type: "ActiveStorage::Attachment") + end + end end From be795108faf9e2df054414c0d38ed1501049f111 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 06:45:35 -0400 Subject: [PATCH 03/15] Show a record's own change log on its page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everything a save records is only readable today by leaving the record for the admin activities index and filtering it down. Put the record's own events on the record: a dropdown of what changed, when, and by whom, under the audit line the page already carries. The before/after rendering isn't rebuilt — the activities table's details cell is extracted into a partial that both render, so a change reads the same in either place. Admin-gated to match the activity links already in shared/_audit_info, and reusable from any record's page; the registration edit page is the first. Co-Authored-By: Claude Opus 5 (1M context) --- .../ahoy_activities/_activity_row.html.erb | 43 +----------------- .../ahoy_activities/_event_details.html.erb | 45 +++++++++++++++++++ app/views/application/_activity_log.html.erb | 37 +++++++++++++++ app/views/event_registrations/edit.html.erb | 5 +++ spec/requests/event_registrations_spec.rb | 27 +++++++++++ 5 files changed, 115 insertions(+), 42 deletions(-) create mode 100644 app/views/admin/ahoy_activities/_event_details.html.erb create mode 100644 app/views/application/_activity_log.html.erb diff --git a/app/views/admin/ahoy_activities/_activity_row.html.erb b/app/views/admin/ahoy_activities/_activity_row.html.erb index e56fefa3e1..bdd5268456 100644 --- a/app/views/admin/ahoy_activities/_activity_row.html.erb +++ b/app/views/admin/ahoy_activities/_activity_row.html.erb @@ -27,47 +27,6 @@ <% activity = event.decorate(context: { record_cache: local_assigns[:record_cache] }) %> - <% if activity.extra_details? %> -
- <% if activity.changes? %> -
    - <% activity.changes_summary.each do |change| %> -
  • - <%= change[:field] %> -
    -
    - Before: - <%= change[:before] %> -
    -
    - After: - <%= change[:after] %> -
    -
    -
    • - <% end %> -
- <% end %> - <% activity.detail_rows.each do |row| %> -
- <% if row[:label] %> - <%= row[:label] %><%= ":" if row[:value] || row[:link] %> - <% end %> - <% if row[:link] %> - <% if row[:action] %><%= row[:action] %><% end %> - <% if row[:link][:path] %> - <%= link_to row[:link][:text], row[:link][:path], class: "text-indigo-600 hover:underline" %> - <% else %> - <%= row[:link][:text] %> - <% end %> - <% elsif row[:value] %> - <%= row[:value] %> - <% end %> -
- <% end %> -
- <% else %> - — - <% end %> + <%= render "admin/ahoy_activities/event_details", activity: activity %> diff --git a/app/views/admin/ahoy_activities/_event_details.html.erb b/app/views/admin/ahoy_activities/_event_details.html.erb new file mode 100644 index 0000000000..1f3ba785ab --- /dev/null +++ b/app/views/admin/ahoy_activities/_event_details.html.erb @@ -0,0 +1,45 @@ +<%# Inline before/after + property rows for one Ahoy event. Shared by the admin + activities table's Details column and any record's inline change log, so both + read the same way. %> +<% if activity.extra_details? %> +
+ <% if activity.changes? %> +
    + <% activity.changes_summary.each do |change| %> +
  • + <%= change[:field] %> +
    +
    + Before: + <%= change[:before] %> +
    +
    + After: + <%= change[:after] %> +
    +
    +
    • + <% end %> +
+ <% end %> + <% activity.detail_rows.each do |row| %> +
+ <% if row[:label] %> + <%= row[:label] %><%= ":" if row[:value] || row[:link] %> + <% end %> + <% if row[:link] %> + <% if row[:action] %><%= row[:action] %><% end %> + <% if row[:link][:path] %> + <%= link_to row[:link][:text], row[:link][:path], class: "text-indigo-600 hover:underline" %> + <% else %> + <%= row[:link][:text] %> + <% end %> + <% elsif row[:value] %> + <%= row[:value] %> + <% end %> +
+ <% end %> +
+<% else %> + — +<% end %> diff --git a/app/views/application/_activity_log.html.erb b/app/views/application/_activity_log.html.erb new file mode 100644 index 0000000000..931a1d5c21 --- /dev/null +++ b/app/views/application/_activity_log.html.erb @@ -0,0 +1,37 @@ +<%# Inline change log for one record, read from its Ahoy lifecycle events + (create/update/destroy tracked for every model by AhoyTrackable). Renders the + same details partial as the admin activities table, so a change reads + identically wherever it's shown; that page remains the filterable full + history, linked from shared/_audit_info. %> +<% events = Ahoy::Event.where(resource_type: record.class.name, resource_id: record.id) + .includes(:user).order(time: :desc) %> +<% if events.any? %> +
+ + +
+<% end %> diff --git a/app/views/event_registrations/edit.html.erb b/app/views/event_registrations/edit.html.erb index f0ea528285..88754638ef 100644 --- a/app/views/event_registrations/edit.html.erb +++ b/app/views/event_registrations/edit.html.erb @@ -64,4 +64,9 @@ <%= render "form", event_registration: @event_registration %> <%= render "shared/audit_info", resource: @event_registration %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "activity_log", record: @event_registration %> +
+ <% end %> diff --git a/spec/requests/event_registrations_spec.rb b/spec/requests/event_registrations_spec.rb index 9d21797f33..0053dfa08f 100644 --- a/spec/requests/event_registrations_spec.rb +++ b/spec/requests/event_registrations_spec.rb @@ -462,6 +462,33 @@ def toggle_certificate(value) ) end + it "shows the record's tracked changes in a change log" do + create( + :ahoy_event, + name: "update.event_registration", + resource_type: "EventRegistration", + resource_id: existing_registration.id, + properties: { + resource_type: "EventRegistration", resource_id: existing_registration.id, + changes: { "status" => { "before" => "registered", "after" => "attended" } } + } + ) + + get edit_event_registration_path(existing_registration) + + expect(response.body).to include("Change log") + # Rendered by the shared details partial: humanized field, before → after. + expect(response.body).to include("Status") + expect(response.body).to include("registered") + expect(response.body).to include("attended") + end + + it "omits the change log when the record has no tracked activity" do + get edit_event_registration_path(existing_registration) + + expect(response.body).not_to include("Change log") + end + it "shows a Delete button for a deletable registration" do get edit_event_registration_path(existing_registration) From d8d8b19d730f5dc2b7d87558a18c555963ddd724 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 08:36:49 -0400 Subject: [PATCH 04/15] Put the change log on the pages people actually correct records from MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An admin fixing a record shouldn't have to leave it to find out what changed: person, organization, affiliation, scholarship, CE registration, and topic subscription now carry the same dropdown the registration edit page got, under the audit line each page already has. Two fixes the extra call sites turned up. The partial resolved the record's class directly, which breaks on a page that hands over a decorated record (people#edit does) — it now unwraps to the model the events were recorded against, the way shared/_audit_info does. And it's referenced by its full path, because the implicit application/ lookup doesn't apply in view specs. The shared example keeps the pages honest about both halves: the log appears when there's activity, and stays out of the way when there isn't. Co-Authored-By: Claude Opus 5 (1M context) --- app/views/affiliations/edit.html.erb | 27 ++++++++++------- app/views/application/_activity_log.html.erb | 9 ++++-- .../edit.html.erb | 5 ++++ app/views/event_registrations/edit.html.erb | 6 ++-- app/views/organizations/edit.html.erb | 13 ++++++--- app/views/people/edit.html.erb | 5 ++++ app/views/scholarships/edit.html.erb | 5 ++++ app/views/topic_subscriptions/edit.html.erb | 9 ++++-- spec/models/concerns/ahoy_trackable_spec.rb | 11 +++++++ spec/requests/affiliations_spec.rb | 5 ++++ ...continuing_education_registrations_spec.rb | 5 ++++ .../requests/organizations_change_log_spec.rb | 10 +++++++ spec/requests/people_change_log_spec.rb | 29 +++++++++++++++++++ spec/requests/scholarships_spec.rb | 5 ++++ spec/requests/topic_subscriptions_spec.rb | 7 +++++ .../shared_examples/change_log_page.rb | 27 +++++++++++++++++ 16 files changed, 155 insertions(+), 23 deletions(-) create mode 100644 spec/requests/organizations_change_log_spec.rb create mode 100644 spec/requests/people_change_log_spec.rb create mode 100644 spec/support/shared_examples/change_log_page.rb diff --git a/app/views/affiliations/edit.html.erb b/app/views/affiliations/edit.html.erb index 12388e90e9..3d08e9d04f 100644 --- a/app/views/affiliations/edit.html.erb +++ b/app/views/affiliations/edit.html.erb @@ -24,8 +24,8 @@ expired ? "bg-blue-50! text-blue-500! border-blue-200!" : "bg-blue-100! text-blue-700! font-semibold border-blue-300!" end %> -
-
+
+
<% if back_path %> <%= link_to back_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" do %> @@ -37,8 +37,8 @@ <%= link_to "Home", root_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %>
-

Edit affiliation

-

+

Edit affiliation

+

<%= @affiliation.person&.full_name %> at <%= @affiliation.organization&.name %>

@@ -46,8 +46,8 @@ url: affiliation_path(@affiliation, return_to: params[:return_to].presence, origin_id: params[:origin_id].presence, admin: params[:admin].presence), method: :patch, html: { id: "affiliation_form", data: { turbo: false } } do |f| %> -
-
+
+
<%= f.input :person_id, collection: @affiliation.person ? [ [ @affiliation.person.full_name, @affiliation.person.id ] ] : [], selected: @affiliation.person_id, @@ -61,7 +61,7 @@
- Primary contact + Primary contact <%= render "affiliations/primary_contact_toggle", f: f %>

Main contact for this org

@@ -76,7 +76,7 @@
-
+
<%= f.input :organization_id, collection: @affiliation.organization ? [ [ @affiliation.organization.name, @affiliation.organization.id ] ] : [], @@ -143,7 +143,7 @@
-
+
<%= f.input :title, as: :string, label_html: { class: "block text-sm font-medium text-gray-700 mb-1" }, @@ -210,7 +210,7 @@ <% end %> - <%= render "shared/audit_info", resource: @ce_registration %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @ce_registration %> +
+ <% end %>
diff --git a/app/views/event_registrations/edit.html.erb b/app/views/event_registrations/edit.html.erb index 88754638ef..622ecac2f8 100644 --- a/app/views/event_registrations/edit.html.erb +++ b/app/views/event_registrations/edit.html.erb @@ -1,10 +1,10 @@ <% content_for(:page_bg_class, "admin-only bg-blue-100") %> -
+
<%# Top bar: back link + secondary links, matching the event pages. When the admin arrived from the bulk payments page, the back link returns there with the originating submission re-expanded instead of the registrants roster. %> -
+
<% if params[:return_to] == "bulk_payments" %> <%# Re-expand the originating submission's row and scroll to it on return. %> <%= link_to bulk_payments_return_path(@event_registration.event), class: "text-sm #{eyebrow_link_class} px-2 py-1" do %> @@ -66,7 +66,7 @@ <%= render "shared/audit_info", resource: @event_registration %> <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %>
- <%= render "activity_log", record: @event_registration %> + <%= render "application/activity_log", record: @event_registration %>
<% end %>
diff --git a/app/views/organizations/edit.html.erb b/app/views/organizations/edit.html.erb index 9f8a9a7363..b4c7c36d9c 100644 --- a/app/views/organizations/edit.html.erb +++ b/app/views/organizations/edit.html.erb @@ -1,5 +1,5 @@ <% content_for(:page_bg_class, "admin-or-owner") %> -
+
<% if params[:return_to] == "onboarding" && params[:event_id].present? %>
<%= link_to onboarding_event_row_path(params[:event_id], params[:highlight]), class: "text-sm #{eyebrow_link_class} px-2 py-1" do %> @@ -7,18 +7,23 @@ <% end %>
<% end %> -
+
<%= link_to "Home", root_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> <%= link_to "Organizations", organizations_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> <%= link_to "Profile", organization_path(@organization), class: "text-sm #{eyebrow_link_class} px-2 py-1" %>
-

+

Edit Organization: <%= truncate(@organization.name, length: 40) %>

-
+
<%= render "form" %> <%= render "shared/audit_info", resource: @organization %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @organization %> +
+ <% end %>
diff --git a/app/views/people/edit.html.erb b/app/views/people/edit.html.erb index ae83408af4..b8673a1824 100644 --- a/app/views/people/edit.html.erb +++ b/app/views/people/edit.html.erb @@ -52,6 +52,11 @@
<%= render "associated_records", person: @person %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @person %> +
+ <% end %>
diff --git a/app/views/scholarships/edit.html.erb b/app/views/scholarships/edit.html.erb index d91e334ce6..e968fc6c07 100644 --- a/app/views/scholarships/edit.html.erb +++ b/app/views/scholarships/edit.html.erb @@ -100,4 +100,9 @@ <%= render "agreement_history", scholarship: @scholarship %> <%= render "shared/audit_info", resource: @scholarship %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @scholarship %> +
+ <% end %>
diff --git a/app/views/topic_subscriptions/edit.html.erb b/app/views/topic_subscriptions/edit.html.erb index a537133221..ce5fa59f9e 100644 --- a/app/views/topic_subscriptions/edit.html.erb +++ b/app/views/topic_subscriptions/edit.html.erb @@ -1,10 +1,10 @@ <% content_for(:page_bg_class, "admin-only bg-blue-100") %> -
+
<%= render "eyebrow" %>
-

Edit subscription

+

Edit subscription

<%= render "form", submit_label: "Save changes" %> <%# These actions redirect too, so they carry the same origin params as the form. %> @@ -30,4 +30,9 @@ Remove <% end %>
+ <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @topic_subscription %> +
+ <% end %>
diff --git a/spec/models/concerns/ahoy_trackable_spec.rb b/spec/models/concerns/ahoy_trackable_spec.rb index 23d134f685..75ed11dfc6 100644 --- a/spec/models/concerns/ahoy_trackable_spec.rb +++ b/spec/models/concerns/ahoy_trackable_spec.rb @@ -106,6 +106,17 @@ def event_named(name) removed = event_named("update.person")[:properties][:association_changes][:professional_licenses].first expect(removed).to include(action: "removed", id: license.id, type: "ProfessionalLicense") end + + it "records a staff tag given to a person on the person's own event" do + person = create(:person) + tag = create(:staff_tag) + Analytics::LifecycleBuffer.store.clear + + person.update!(staff_taggings_attributes: [ { staff_tag_id: tag.id } ]) + + tagged = event_named("update.person")[:properties][:association_changes][:staff_taggings].first + expect(tagged).to include(action: "added", type: "StaffTagging") + end end describe "attachments" do diff --git a/spec/requests/affiliations_spec.rb b/spec/requests/affiliations_spec.rb index 4e28dcab1e..706f07d152 100644 --- a/spec/requests/affiliations_spec.rb +++ b/spec/requests/affiliations_spec.rb @@ -13,6 +13,11 @@ context "as an admin" do before { sign_in admin } + it_behaves_like "a page with a change log" do + let(:record) { affiliation } + let(:page_path) { edit_affiliation_path(affiliation) } + end + it "renders the edit form" do get edit_affiliation_path(affiliation) expect(response).to be_successful diff --git a/spec/requests/continuing_education_registrations_spec.rb b/spec/requests/continuing_education_registrations_spec.rb index a15810890d..b60ab77073 100644 --- a/spec/requests/continuing_education_registrations_spec.rb +++ b/spec/requests/continuing_education_registrations_spec.rb @@ -12,6 +12,11 @@ describe "as an admin" do before { sign_in admin } + it_behaves_like "a page with a change log" do + let(:record) { ce_registration } + let(:page_path) { edit_continuing_education_registration_path(ce_registration) } + end + describe "a transferred-in registration (two-record CE model, #1944)" do let(:source) { create(:event_registration, event: event) } let(:transferred_in) do diff --git a/spec/requests/organizations_change_log_spec.rb b/spec/requests/organizations_change_log_spec.rb new file mode 100644 index 0000000000..9a6767b945 --- /dev/null +++ b/spec/requests/organizations_change_log_spec.rb @@ -0,0 +1,10 @@ +require "rails_helper" + +RSpec.describe "Organizations change log", type: :request do + before { sign_in create(:user, :admin) } + + it_behaves_like "a page with a change log" do + let(:record) { create(:organization) } + let(:page_path) { edit_organization_path(record) } + end +end diff --git a/spec/requests/people_change_log_spec.rb b/spec/requests/people_change_log_spec.rb new file mode 100644 index 0000000000..f8ba210b6d --- /dev/null +++ b/spec/requests/people_change_log_spec.rb @@ -0,0 +1,29 @@ +require "rails_helper" + +RSpec.describe "People change log", type: :request do + let(:person) { create(:person) } + + context "as an admin" do + before { sign_in create(:user, :admin) } + + it_behaves_like "a page with a change log" do + let(:record) { person } + let(:page_path) { edit_person_path(person) } + end + end + + # The profile is admin-or-owner, but the change log is admin data — the owner + # sees their own page without it. + context "as the person themselves" do + it "hides the change log" do + user = create(:user, person: person) + create(:ahoy_event, name: "update.person", resource_type: "Person", resource_id: person.id, + properties: { resource_type: "Person", resource_id: person.id }) + sign_in user + + get edit_person_path(person) + + expect(response.body).not_to include("Change log") + end + end +end diff --git a/spec/requests/scholarships_spec.rb b/spec/requests/scholarships_spec.rb index 983489544e..c017443926 100644 --- a/spec/requests/scholarships_spec.rb +++ b/spec/requests/scholarships_spec.rb @@ -10,6 +10,11 @@ before { sign_in admin } describe "GET /scholarships/:id/edit" do + it_behaves_like "a page with a change log" do + let(:record) { scholarship } + let(:page_path) { edit_scholarship_path(scholarship) } + end + it "renders the cost summary with event cost, scholarship amount, and still owed" do get edit_scholarship_path(scholarship) diff --git a/spec/requests/topic_subscriptions_spec.rb b/spec/requests/topic_subscriptions_spec.rb index 37d6d24c75..dd8a3b3ee6 100644 --- a/spec/requests/topic_subscriptions_spec.rb +++ b/spec/requests/topic_subscriptions_spec.rb @@ -6,6 +6,13 @@ before { sign_in admin } + describe "GET /topic_subscriptions/:id/edit" do + it_behaves_like "a page with a change log" do + let(:record) { create(:topic_subscription, topic_subscription_type: trainings) } + let(:page_path) { edit_topic_subscription_path(record) } + end + end + describe "GET /topic_subscriptions" do it "renders the index shell for a full-page request" do get topic_subscriptions_path diff --git a/spec/support/shared_examples/change_log_page.rb b/spec/support/shared_examples/change_log_page.rb new file mode 100644 index 0000000000..22811bf943 --- /dev/null +++ b/spec/support/shared_examples/change_log_page.rb @@ -0,0 +1,27 @@ +# A record's page shows its own Ahoy lifecycle events. Include with `record` and +# `page_path` defined, signed in as whoever should see it. +RSpec.shared_examples "a page with a change log" do + it "shows the record's change log" do + create( + :ahoy_event, + name: "update.#{record.class.name.underscore}", + resource_type: record.class.name, + resource_id: record.id, + properties: { + resource_type: record.class.name, resource_id: record.id, + changes: { "status" => { "before" => "before value", "after" => "after value" } } + } + ) + + get page_path + + expect(response.body).to include("Change log") + expect(response.body).to include("after value") + end + + it "omits the change log when the record has no tracked activity" do + get page_path + + expect(response.body).not_to include("Change log") + end +end From 47cd23df7006859360549853ccc8c4c3f6d06dc1 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 11:40:58 -0400 Subject: [PATCH 05/15] Put every change log in the same place: under the edit history The person page had its log in the Associated records card, and the topic subscription page had no edit-history block for it to sit under. Both now match the rest: created/updated line, then the change log. Co-Authored-By: Claude Opus 5 (1M context) --- app/views/people/edit.html.erb | 10 +++++----- app/views/topic_subscriptions/edit.html.erb | 1 + 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/views/people/edit.html.erb b/app/views/people/edit.html.erb index b8673a1824..0baa6c145d 100644 --- a/app/views/people/edit.html.erb +++ b/app/views/people/edit.html.erb @@ -45,6 +45,11 @@
<%= render "form" %> <%= render "shared/audit_info", resource: @person %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @person %> +
+ <% end %>
@@ -52,11 +57,6 @@
<%= render "associated_records", person: @person %> - <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> -
- <%= render "application/activity_log", record: @person %> -
- <% end %>
diff --git a/app/views/topic_subscriptions/edit.html.erb b/app/views/topic_subscriptions/edit.html.erb index ce5fa59f9e..00b92c3516 100644 --- a/app/views/topic_subscriptions/edit.html.erb +++ b/app/views/topic_subscriptions/edit.html.erb @@ -30,6 +30,7 @@ Remove <% end %>
+ <%= render "shared/audit_info", resource: @topic_subscription %> <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %>
<%= render "application/activity_log", record: @topic_subscription %> From e151af3aa1bd8800f6a61d37acce9d4d02f25937 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 11:55:10 -0400 Subject: [PATCH 06/15] Give every edit page an edit history, and say when there's nothing in it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven edit pages had no created/updated line at all — category type, feature, form, membership, membership invoice, professional license, topic subscription type — so there was nothing for a change log to sit under. They have both now, in the same order as every other edit page. The log also stops vanishing on records with no tracked activity. Disappearing left no way to tell "nothing has changed" from "this page doesn't have one", which matters while tracking is new and most records predate it; it now opens to say so. Co-Authored-By: Claude Opus 5 (1M context) --- app/views/application/_activity_log.html.erb | 9 ++++++--- app/views/category_types/edit.html.erb | 14 ++++++++++---- app/views/features/edit.html.erb | 12 +++++++++--- app/views/forms/edit.html.erb | 6 ++++++ app/views/membership_invoices/edit.html.erb | 16 +++++++++++----- app/views/memberships/edit.html.erb | 16 +++++++++++----- app/views/professional_licenses/edit.html.erb | 14 ++++++++++---- app/views/topic_subscription_types/edit.html.erb | 10 ++++++++-- spec/requests/event_registrations_spec.rb | 5 +++-- spec/requests/membership_invoices_spec.rb | 9 +++++++++ spec/requests/people_change_log_spec.rb | 2 -- spec/support/shared_examples/change_log_page.rb | 5 +++-- 12 files changed, 86 insertions(+), 32 deletions(-) diff --git a/app/views/application/_activity_log.html.erb b/app/views/application/_activity_log.html.erb index 32021d7ea9..e2bcbfe748 100644 --- a/app/views/application/_activity_log.html.erb +++ b/app/views/application/_activity_log.html.erb @@ -8,8 +8,7 @@ <% model = record.respond_to?(:object) ? record.object : record %> <% events = Ahoy::Event.where(resource_type: model.class.name, resource_id: model.id) .includes(:user).order(time: :desc) %> -<% if events.any? %> -
+
-<% end %> diff --git a/app/views/category_types/edit.html.erb b/app/views/category_types/edit.html.erb index 0a2c32bc4c..f5ae815a91 100644 --- a/app/views/category_types/edit.html.erb +++ b/app/views/category_types/edit.html.erb @@ -1,13 +1,13 @@ <% content_for(:page_bg_class, "admin-only bg-blue-100") %>
-
-
-
+
+
+
<%= link_to "Home", root_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> <%= link_to "Category Types", category_types_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> <%= link_to "View", category_type_path(@category_type), class: "text-sm #{eyebrow_link_class} px-2 py-1" %>
-

Edit <%= @category_type.class.model_name.human.downcase %>

+

Edit <%= @category_type.class.model_name.human.downcase %>

@@ -16,4 +16,10 @@
+ <%= render "shared/audit_info", resource: @category_type %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @category_type %> +
+ <% end %>
diff --git a/app/views/features/edit.html.erb b/app/views/features/edit.html.erb index af548300c0..f409ae87c1 100644 --- a/app/views/features/edit.html.erb +++ b/app/views/features/edit.html.erb @@ -1,9 +1,15 @@ <% content_for(:page_bg_class, "admin-only bg-blue-100") %> -
-
+
+
<%= link_to "Features & tips", features_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %> <%= link_to "View", feature_path(@feature), class: "text-sm #{eyebrow_link_class} px-2 py-1" %>
-

Edit feature

+

Edit feature

<%= render "form", feature: @feature %> + <%= render "shared/audit_info", resource: @feature %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @feature %> +
+ <% end %>
diff --git a/app/views/forms/edit.html.erb b/app/views/forms/edit.html.erb index 8cd59b6e28..ca368ea228 100644 --- a/app/views/forms/edit.html.erb +++ b/app/views/forms/edit.html.erb @@ -290,4 +290,10 @@ <% end %>
+ <%= render "shared/audit_info", resource: @form %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @form %> +
+ <% end %>
diff --git a/app/views/membership_invoices/edit.html.erb b/app/views/membership_invoices/edit.html.erb index f8708ff57b..121209e010 100644 --- a/app/views/membership_invoices/edit.html.erb +++ b/app/views/membership_invoices/edit.html.erb @@ -1,24 +1,30 @@ <% content_for(:page_bg_class, "admin-only bg-blue-100") %> <% person = @membership_invoice.person %> -
-
+
+
<%= link_to person_memberships_path(person), class: "text-sm #{eyebrow_link_class} px-2 py-1" do %> Membership <% end %>
-

Edit membership invoice

-

<%= person.full_name %> · <%= @membership_invoice.decorate.period_range %>

+

Edit membership invoice

+

<%= person.full_name %> · <%= @membership_invoice.decorate.period_range %>

<%= simple_form_for @membership_invoice do |f| %> <%= render "fields", f: f, include_end_date: true %> -
+
<%= link_to "Cancel", person_memberships_path(person), class: "btn btn-secondary-outline" %> <%= f.button :submit, "Save invoice", class: "btn btn-primary" %>
<% end %> + <%= render "shared/audit_info", resource: @membership_invoice %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @membership_invoice %> +
+ <% end %>
diff --git a/app/views/memberships/edit.html.erb b/app/views/memberships/edit.html.erb index 3d137f4092..98c3592ee2 100644 --- a/app/views/memberships/edit.html.erb +++ b/app/views/memberships/edit.html.erb @@ -1,14 +1,14 @@ <% content_for(:page_bg_class, "admin-only bg-blue-100") %> -
-
+
+
<%= link_to person_memberships_path(@person), class: "text-sm #{eyebrow_link_class} px-2 py-1" do %> Membership <% end %>
-

Membership cost

-

<%= @person.full_name %>

+

Membership cost

+

<%= @person.full_name %>

<%= simple_form_for @membership do |f| %> <%= render "shared/errors", resource: f.object %> @@ -19,11 +19,17 @@ hint: "Leave blank for the standard cost (#{dollars_from_cents(Membership::ANNUAL_COST_CENTS)}). Applies to future membership invoices only - those already created keep the price they were billed at.", input_html: { min: 0, step: 0.01, placeholder: "Standard", class: "bg-white" } %> -
+
<%= link_to "Cancel", person_memberships_path(@person), class: "btn btn-secondary-outline" %> <%= f.button :submit, "Save cost", class: "btn btn-primary" %>
<% end %> + <%= render "shared/audit_info", resource: @membership %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @membership %> +
+ <% end %>
diff --git a/app/views/professional_licenses/edit.html.erb b/app/views/professional_licenses/edit.html.erb index e4db7dd0c7..868f64b696 100644 --- a/app/views/professional_licenses/edit.html.erb +++ b/app/views/professional_licenses/edit.html.erb @@ -1,23 +1,29 @@ <% content_for(:page_bg_class, "admin-only bg-blue-100") %> -
+
<%= link_to professional_licenses_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" do %> Licenses <% end %>
-
+

Edit license

-

Update this registrant's professional license

+

Update this registrant's professional license

-
+
<%= render "form", professional_license: @professional_license %>
+ <%= render "shared/audit_info", resource: @professional_license %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @professional_license %> +
+ <% end %>
diff --git a/app/views/topic_subscription_types/edit.html.erb b/app/views/topic_subscription_types/edit.html.erb index 04e47e5811..cbded44c5f 100644 --- a/app/views/topic_subscription_types/edit.html.erb +++ b/app/views/topic_subscription_types/edit.html.erb @@ -1,10 +1,10 @@ <% content_for(:page_bg_class, "admin-only bg-blue-100") %> -
+
<%= link_to "← Subscription topics", topic_subscription_types_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" %>
-

Edit topic

+

Edit topic

<%= render "form", submit_label: "Save changes" %> <%# Archive/restore and delete moved off the index — the topic's actions live on @@ -39,4 +39,10 @@

Topics with subscriptions can't be deleted — archive to retire without losing them.

<% end %>
+ <%= render "shared/audit_info", resource: @topic_subscription_type %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
+ <%= render "application/activity_log", record: @topic_subscription_type %> +
+ <% end %>
diff --git a/spec/requests/event_registrations_spec.rb b/spec/requests/event_registrations_spec.rb index 0053dfa08f..9a8397ad6d 100644 --- a/spec/requests/event_registrations_spec.rb +++ b/spec/requests/event_registrations_spec.rb @@ -483,10 +483,11 @@ def toggle_certificate(value) expect(response.body).to include("attended") end - it "omits the change log when the record has no tracked activity" do + it "says so rather than disappearing when the record has no tracked activity" do get edit_event_registration_path(existing_registration) - expect(response.body).not_to include("Change log") + expect(response.body).to include("Change log") + expect(response.body).to include("No changes recorded yet") end it "shows a Delete button for a deletable registration" do diff --git a/spec/requests/membership_invoices_spec.rb b/spec/requests/membership_invoices_spec.rb index 18fc11e1be..d4eeed0ecc 100644 --- a/spec/requests/membership_invoices_spec.rb +++ b/spec/requests/membership_invoices_spec.rb @@ -15,6 +15,15 @@ def term_for(name, cost_cents: Membership::ANNUAL_COST_CENTS, start_date: Date.c end_date: start_date + 1.year - 1.day) end + describe "GET /membership_invoices/:id/edit" do + before { sign_in admin } + + it_behaves_like "a page with a change log" do + let(:record) { term_for("Renewal") } + let(:page_path) { edit_membership_invoice_path(record) } + end + end + describe "GET /membership_invoices" do it "is not available to a signed-out visitor" do get membership_invoices_path diff --git a/spec/requests/people_change_log_spec.rb b/spec/requests/people_change_log_spec.rb index f8ba210b6d..8a1ff57130 100644 --- a/spec/requests/people_change_log_spec.rb +++ b/spec/requests/people_change_log_spec.rb @@ -17,8 +17,6 @@ context "as the person themselves" do it "hides the change log" do user = create(:user, person: person) - create(:ahoy_event, name: "update.person", resource_type: "Person", resource_id: person.id, - properties: { resource_type: "Person", resource_id: person.id }) sign_in user get edit_person_path(person) diff --git a/spec/support/shared_examples/change_log_page.rb b/spec/support/shared_examples/change_log_page.rb index 22811bf943..7195fe2977 100644 --- a/spec/support/shared_examples/change_log_page.rb +++ b/spec/support/shared_examples/change_log_page.rb @@ -19,9 +19,10 @@ expect(response.body).to include("after value") end - it "omits the change log when the record has no tracked activity" do + it "says so rather than disappearing when the record has no tracked activity" do get page_path - expect(response.body).not_to include("Change log") + expect(response.body).to include("Change log") + expect(response.body).to include("No changes recorded yet") end end From 41c103538d64f1658eeea9e93a18cfe03eab4451 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 12:06:22 -0400 Subject: [PATCH 07/15] Keep browsing out of the change logs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A record's change log answers what happened to it, so views — and the other read-shaped events, search, filter, download — don't belong in it. They stay on the admin activities index, which is where browsing is the point. Co-Authored-By: Claude Opus 5 (1M context) --- app/models/ahoy/event.rb | 9 +++++++++ app/views/application/_activity_log.html.erb | 2 +- spec/support/shared_examples/change_log_page.rb | 14 ++++++++++++++ 3 files changed, 24 insertions(+), 1 deletion(-) diff --git a/app/models/ahoy/event.rb b/app/models/ahoy/event.rb index 4c3125b538..a12d5b6572 100644 --- a/app/models/ahoy/event.rb +++ b/app/models/ahoy/event.rb @@ -5,4 +5,13 @@ class Ahoy::Event < ApplicationRecord belongs_to :visit belongs_to :user, optional: true + + # Reading a record isn't a change to it. A record's change log asks what + # happened to it, so these stay on the admin activities index, which is where + # browsing belongs. + NON_MUTATION_PREFIXES = %w[view search filter download].freeze + + scope :mutations, -> { + NON_MUTATION_PREFIXES.reduce(all) { |scope, prefix| scope.where.not(arel_table[:name].matches("#{prefix}.%")) } + } end diff --git a/app/views/application/_activity_log.html.erb b/app/views/application/_activity_log.html.erb index e2bcbfe748..fa2189cb88 100644 --- a/app/views/application/_activity_log.html.erb +++ b/app/views/application/_activity_log.html.erb @@ -6,7 +6,7 @@ <%# Pages hand over decorated records as readily as models — resolve to the model the events were recorded against, the same way shared/_audit_info does. %> <% model = record.respond_to?(:object) ? record.object : record %> -<% events = Ahoy::Event.where(resource_type: model.class.name, resource_id: model.id) +<% events = Ahoy::Event.mutations.where(resource_type: model.class.name, resource_id: model.id) .includes(:user).order(time: :desc) %>
+<% end %> diff --git a/app/views/continuing_education_registrations/show.html.erb b/app/views/continuing_education_registrations/show.html.erb index 493582d921..37b446b0c6 100644 --- a/app/views/continuing_education_registrations/show.html.erb +++ b/app/views/continuing_education_registrations/show.html.erb @@ -5,9 +5,9 @@ <% payment = ce.payment_status_badge %> <% certificate = ce.certificate_badge %> -
+
<%# Top bar: back to the CE registrations index + Edit / Home. %> -
+
<%= link_to continuing_education_registrations_path, class: "text-sm #{eyebrow_link_class} px-2 py-1" do %> CE registrations <% end %> @@ -35,27 +35,27 @@
-
License
+
License
<%= ce.license_label %>
-
Issuing state
+
Issuing state
<%= license&.issuing_state.presence || "—" %>
-
License expires
+
License expires
<%= license&.expires_on&.to_fs(:long) || "—" %>
-
Hours
+
Hours
<%= number_with_precision(@ce_registration.hours, precision: 1, strip_insignificant_zeros: true) %>
-
Payment
+
Payment
<%= render "shared/badge", label: payment.label, classes: payment.classes, icon: payment.icon %>
-
Certificate
+
Certificate
<%= render "shared/badge", label: certificate.label, classes: certificate.classes, icon: certificate.icon %>
@@ -75,9 +75,9 @@ <% @ce_registration.comments.each do |comment| %>
  • <% if comment.topic.present? %> -

    <%= comment.topic %>

    +

    <%= comment.topic %>

    <% end %> -

    <%= comment.body %>

    +

    <%= comment.body %>

    • <% end %> @@ -86,4 +86,9 @@
    <%= render "shared/audit_info", resource: @ce_registration %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
    + <%= render "application/activity_log", record: @ce_registration %> +
    + <% end %>
    diff --git a/app/views/staff_tags/show.html.erb b/app/views/staff_tags/show.html.erb index e39fa50cb3..54e2c0e65a 100644 --- a/app/views/staff_tags/show.html.erb +++ b/app/views/staff_tags/show.html.erb @@ -51,6 +51,11 @@
    <%= render "shared/audit_info", resource: @staff_tag %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
    + <%= render "application/activity_log", record: @staff_tag %> +
    + <% end %>
    diff --git a/spec/requests/continuing_education_registrations_spec.rb b/spec/requests/continuing_education_registrations_spec.rb index b60ab77073..0a537fa1fe 100644 --- a/spec/requests/continuing_education_registrations_spec.rb +++ b/spec/requests/continuing_education_registrations_spec.rb @@ -17,6 +17,11 @@ let(:page_path) { edit_continuing_education_registration_path(ce_registration) } end + it_behaves_like "a page with a change log" do + let(:record) { ce_registration } + let(:page_path) { continuing_education_registration_path(ce_registration) } + end + describe "a transferred-in registration (two-record CE model, #1944)" do let(:source) { create(:event_registration, event: event) } let(:transferred_in) do diff --git a/spec/requests/event_registrations_spec.rb b/spec/requests/event_registrations_spec.rb index 9a8397ad6d..5093490373 100644 --- a/spec/requests/event_registrations_spec.rb +++ b/spec/requests/event_registrations_spec.rb @@ -483,11 +483,10 @@ def toggle_certificate(value) expect(response.body).to include("attended") end - it "says so rather than disappearing when the record has no tracked activity" do + it "says the log is empty rather than disappearing when the record has no tracked activity" do get edit_event_registration_path(existing_registration) - expect(response.body).to include("Change log") - expect(response.body).to include("No changes recorded yet") + expect(response.body).to include("Change log empty") end it "shows a Delete button for a deletable registration" do diff --git a/spec/requests/staff_tags_spec.rb b/spec/requests/staff_tags_spec.rb index c7c2496a2b..907878bfbe 100644 --- a/spec/requests/staff_tags_spec.rb +++ b/spec/requests/staff_tags_spec.rb @@ -6,6 +6,11 @@ describe "as an admin" do before { sign_in admin } + it_behaves_like "a page with a change log" do + let(:record) { create(:staff_tag) } + let(:page_path) { staff_tag_path(record) } + end + it "lists staff tags" do tag = create(:staff_tag, name: "Highlight roster") get staff_tags_path diff --git a/spec/support/shared_examples/change_log_page.rb b/spec/support/shared_examples/change_log_page.rb index e6d4138bc9..7a7c9cea86 100644 --- a/spec/support/shared_examples/change_log_page.rb +++ b/spec/support/shared_examples/change_log_page.rb @@ -16,6 +16,7 @@ get page_path expect(response.body).to include("Change log") + expect(response.body).not_to include("Change log empty") expect(response.body).to include("after value") end @@ -30,13 +31,12 @@ get page_path - expect(response.body).to include("No changes recorded yet") + expect(response.body).to include("Change log empty") end - it "says so rather than disappearing when the record has no tracked activity" do + it "says the log is empty rather than disappearing when the record has no tracked activity" do get page_path - expect(response.body).to include("Change log") - expect(response.body).to include("No changes recorded yet") + expect(response.body).to include("Change log empty") end end From 5d7f8fac8d5fcd957739dd35483013be689ecaa4 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 14:42:34 -0400 Subject: [PATCH 09/15] Say what a nested record contained, not just that it changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "added Comment #" told you nothing: no id, because a record added through nested attributes has none until the collection autosave runs — after the update event is assembled — and no content, because the renderer treated an association entry as a link and dropped everything it carried. Added and removed children now carry what they said (body, topic — keys, timestamps, and secret-shaped columns left out; the child's own event keeps the full snapshot), the id is filled in once the save resolves it, and a reference renders its detail underneath the link. An edited comment already recorded its before/after; now that also reaches the page. Co-Authored-By: Claude Opus 5 (1M context) --- app/decorators/ahoy/event_decorator.rb | 14 +++++++-- app/models/concerns/ahoy_trackable.rb | 29 +++++++++++++++++-- .../analytics/event_reference_loader.rb | 2 +- spec/models/concerns/ahoy_trackable_spec.rb | 22 ++++++++++++++ spec/requests/event_registrations_spec.rb | 19 ++++++++++++ 5 files changed, 81 insertions(+), 5 deletions(-) diff --git a/app/decorators/ahoy/event_decorator.rb b/app/decorators/ahoy/event_decorator.rb index 54c3a95b4c..f67226b9ff 100644 --- a/app/decorators/ahoy/event_decorator.rb +++ b/app/decorators/ahoy/event_decorator.rb @@ -64,7 +64,7 @@ def flatten_rows(value, label = nil, depth = 0) end def hash_rows(hash, label, depth) - return [ reference_row(label, hash, depth) ] if reference?(hash) + return reference_rows(label, hash, depth) if reference?(hash) return [ { label: label, value: "(empty)", depth: depth } ] if hash.empty? rows = label ? [ { label: label, value: nil, depth: depth } ] : [] @@ -81,7 +81,7 @@ def array_rows(array, label, depth) elsif array.all? { |item| reference?(item) } header = label ? [ { label: label, value: nil, depth: depth } ] : [] child_depth = label ? depth + 1 : depth - header + array.map { |item| reference_row(nil, item, child_depth) } + header + array.flat_map { |item| reference_rows(nil, item, child_depth) } else [ { label: label, value: array.map { |item| display_value(item) }.join(", "), depth: depth } ] end @@ -101,6 +101,16 @@ def entity_label(item) type.present? ? "#{name} (#{type.to_s.underscore.humanize})" : name end + # The link, then what the record actually said — a comment's body reads better + # than "a comment was added". + def reference_rows(label, item, depth) + detail = item["changes"].presence || item["attributes"].presence + rows = [ reference_row(label, item, depth) ] + return rows if detail.blank? + + rows + flatten_rows(detail, nil, depth + 1) + end + def reference_row(label, item, depth) type = item["type"] || item["record_type"] id = item["id"] || item["record_id"] diff --git a/app/models/concerns/ahoy_trackable.rb b/app/models/concerns/ahoy_trackable.rb index fad73ae1b0..9f888f8df5 100644 --- a/app/models/concerns/ahoy_trackable.rb +++ b/app/models/concerns/ahoy_trackable.rb @@ -10,6 +10,7 @@ module AhoyTrackable after_update -> { track_update_event } after_destroy -> { track_lifecycle_event("destroy", @_destroy_snapshot || {}) } before_save :capture_pending_changes + after_save :resolve_pending_association_ids before_destroy :capture_destroy_snapshot end @@ -80,8 +81,10 @@ def capture_pending_association_changes end def pending_association_change(assoc_name, record) - return { assoc: assoc_name, record: record, action: "removed" } if record.marked_for_destruction? - return { assoc: assoc_name, record: record, action: "added" } if record.new_record? + if record.marked_for_destruction? || record.new_record? + action = record.marked_for_destruction? ? "removed" : "added" + return { assoc: assoc_name, record: record, action: action, attributes: content_attributes(record) } + end record_changes = record.changes.except("updated_at", "created_at") return if record_changes.empty? @@ -89,19 +92,41 @@ def pending_association_change(assoc_name, record) { assoc: assoc_name, record: record, action: "updated", changes: format_tracked_changes(record_changes) } end + # What the record says, minus the plumbing: an added comment should read as its + # body, not as a row of foreign keys. Keys, timestamps, and anything + # secret-shaped are left out; the child's own event keeps the full snapshot. + def content_attributes(record) + record.attributes + .except("id", "created_at", "updated_at") + .reject { |key, value| value.blank? || key.end_with?("_id", "_type") } + .reject { |key, _| key.match?(/password|token|secret|key|digest|salt|otp/i) } + end + # Ids are read now rather than at capture time: a record added through nested # attributes has none until the save goes through. def collect_association_changes + @_unresolved_association_ids = [] + (@_pending_association_changes.to_a + @_pending_attachment_changes.to_a).each_with_object({}) do |pending, changes| entry = { action: pending[:action], type: pending[:type] || pending[:record].class.name } entry[:id] = pending[:record].id if pending[:record] entry[:filename] = pending[:filename] if pending[:filename] entry[:changes] = pending[:changes] if pending[:changes] + entry[:attributes] = pending[:attributes] if pending[:attributes].present? + @_unresolved_association_ids << [ entry, pending[:record] ] if pending[:record] && entry[:id].nil? (changes[pending[:assoc]] ||= []) << entry end end + # A record added through nested attributes has no id until the collection + # autosave runs, which is after the update event is assembled. The buffered + # event holds the same hash, so filling it in here fills it in there. + def resolve_pending_association_ids + @_unresolved_association_ids.to_a.each { |entry, record| entry[:id] ||= record.id } + @_unresolved_association_ids = nil + end + # Rich text saves through the parent's autosave chain, so by the time the update # event is built the body is no longer reliably readable as a change — capture it # here, while it's still dirty. Keyed on the attribute (`rhino_body`), not the diff --git a/app/services/analytics/event_reference_loader.rb b/app/services/analytics/event_reference_loader.rb index a1afe6f00e..ee1634925e 100644 --- a/app/services/analytics/event_reference_loader.rb +++ b/app/services/analytics/event_reference_loader.rb @@ -7,7 +7,7 @@ class EventReferenceLoader # A referenced record inside event properties: a type + id and nothing but # reference bookkeeping — no name/title (those render as a plain label) and # no snapshot columns. - REFERENCE_KEYS = %w[type id record_type record_id action blob_id changes].freeze + REFERENCE_KEYS = %w[type id record_type record_id action blob_id changes attributes].freeze def self.reference?(item) return false unless item.is_a?(Hash) diff --git a/spec/models/concerns/ahoy_trackable_spec.rb b/spec/models/concerns/ahoy_trackable_spec.rb index 75ed11dfc6..e6a2e5827b 100644 --- a/spec/models/concerns/ahoy_trackable_spec.rb +++ b/spec/models/concerns/ahoy_trackable_spec.rb @@ -82,6 +82,27 @@ def event_named(name) expect(added).to include(action: "added", type: "Comment") end + it "records what an added nested record said, once it has an id" do + registration = create(:event_registration) + Analytics::LifecycleBuffer.store.clear + + registration.update!(comments_attributes: [ { body: "Left a voicemail", topic: "Payment" } ]) + + added = event_named("update.event_registration")[:properties][:association_changes][:comments].first + expect(added[:attributes]).to eq({ "body" => "Left a voicemail", "topic" => "Payment" }) + expect(added[:id]).to eq(registration.comments.reload.first.id) + end + + it "leaves keys and timestamps out of what an added record said" do + registration = create(:event_registration) + Analytics::LifecycleBuffer.store.clear + + registration.update!(comments_attributes: [ { body: "Left a voicemail" } ]) + + added = event_named("update.event_registration")[:properties][:association_changes][:comments].first + expect(added[:attributes].keys).to contain_exactly("body") + end + it "records an edited nested record with its field changes" do person = create(:person) license = create(:professional_license, person: person, number: "LIC-1") @@ -105,6 +126,7 @@ def event_named(name) removed = event_named("update.person")[:properties][:association_changes][:professional_licenses].first expect(removed).to include(action: "removed", id: license.id, type: "ProfessionalLicense") + expect(removed[:attributes]).to include("number" => license.number) end it "records a staff tag given to a person on the person's own event" do diff --git a/spec/requests/event_registrations_spec.rb b/spec/requests/event_registrations_spec.rb index 5093490373..bfee1b4c3d 100644 --- a/spec/requests/event_registrations_spec.rb +++ b/spec/requests/event_registrations_spec.rb @@ -483,6 +483,25 @@ def toggle_certificate(value) expect(response.body).to include("attended") end + it "shows what a nested record said, not just that one changed" do + create( + :ahoy_event, + name: "update.event_registration", + resource_type: "EventRegistration", + resource_id: existing_registration.id, + properties: { + resource_type: "EventRegistration", resource_id: existing_registration.id, + association_changes: { + comments: [ { action: "added", type: "Comment", id: 1, attributes: { "body" => "Left a voicemail" } } ] + } + } + ) + + get edit_event_registration_path(existing_registration) + + expect(response.body).to include("Left a voicemail") + end + it "says the log is empty rather than disappearing when the record has no tracked activity" do get edit_event_registration_path(existing_registration) From 5cc6c3c2b2351983d574c71f0185a6a52cbeb4c5 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 15:03:55 -0400 Subject: [PATCH 10/15] Stop a broken label from taking the org page down MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SectorableItem#title had its guard inverted: everything that wasn't a workshop log fell into the branch that calls sectorable.title, and an organization hasn't got one. Nothing reached it before, because association changes never made it into an event; now that they do, the org's change log references its sector taggings and the page raised NoMethodError. The guard is fixed — a tagging reads as the sector it applied — and two callers are hardened, because a model's label should never be able to take down a page that merely mentions the record. The event builder's rescue was calling the same raising method again, so sectorable_item events were being dropped entirely and logged rather than recorded. Co-Authored-By: Claude Opus 5 (1M context) --- app/decorators/ahoy/event_decorator.rb | 11 +++++++++- app/models/sectorable_item.rb | 9 +++++--- app/services/analytics/event_builder.rb | 8 ++++++- spec/decorators/ahoy/event_decorator_spec.rb | 22 +++++++++++++++++++ spec/models/sectorable_item_spec.rb | 15 +++++++++++++ .../requests/organizations_change_log_spec.rb | 22 +++++++++++++++++++ 6 files changed, 82 insertions(+), 5 deletions(-) diff --git a/app/decorators/ahoy/event_decorator.rb b/app/decorators/ahoy/event_decorator.rb index f67226b9ff..ebcd9db4f9 100644 --- a/app/decorators/ahoy/event_decorator.rb +++ b/app/decorators/ahoy/event_decorator.rb @@ -115,11 +115,20 @@ def reference_row(label, item, depth) type = item["type"] || item["record_type"] id = item["id"] || item["record_id"] record = find_referenced_record(type, id) - text = record.try(:title).presence || record.try(:name).presence || "#{type} ##{id}" + text = safe_label(record) || "#{type} ##{id}" { label: label, depth: depth, action: item["action"], link: { text: text, path: show_path_for(record) } } end + # A model's own title/name can raise on records it wasn't written for, and a + # change log is not the place to find out. + def safe_label(record) + label = record.try(:title).presence || record.try(:name).presence + label.is_a?(String) ? label : label&.to_s + rescue StandardError + nil + end + # Prefer the page-level cache (one query per type, built by # Analytics::EventReferenceLoader) and only fall back to a direct lookup when # no cache was supplied (e.g. specs or the single-event detail page). diff --git a/app/models/sectorable_item.rb b/app/models/sectorable_item.rb index e55d02c2f3..07c89f56b4 100644 --- a/app/models/sectorable_item.rb +++ b/app/models/sectorable_item.rb @@ -9,10 +9,13 @@ class SectorableItem < ApplicationRecord before_create :skip_if_duplicate - # Methods + # A tagging reads as the sector it applied. Only a workshop log carries a title + # and a windows type to compose with it — an organization or a person doesn't, + # and asking for one raised. def title - return id unless sectorable && sectorable.class != WorkshopLog - "#{sectorable.title} - #{sectorable.windows_type.name if sectorable.windows_type}" + return sector&.name.to_s unless sectorable.is_a?(WorkshopLog) + + "#{sectorable.title} - #{sectorable.windows_type&.name}" end private diff --git a/app/services/analytics/event_builder.rb b/app/services/analytics/event_builder.rb index 79bc4d105c..ce282f2b3a 100644 --- a/app/services/analytics/event_builder.rb +++ b/app/services/analytics/event_builder.rb @@ -20,7 +20,13 @@ def self.safe_title(resource) resource.decorate.title rescue - resource.try(:title) || resource.try(:name) || resource.id + fallback_title(resource) + end + + def self.fallback_title(resource) + resource.try(:title).presence || resource.try(:name).presence || resource.id + rescue StandardError + resource.id end end end diff --git a/spec/decorators/ahoy/event_decorator_spec.rb b/spec/decorators/ahoy/event_decorator_spec.rb index 7526de9f81..2df926500c 100644 --- a/spec/decorators/ahoy/event_decorator_spec.rb +++ b/spec/decorators/ahoy/event_decorator_spec.rb @@ -160,4 +160,26 @@ def decorate(properties) expect(queries).to eq(0) end end + + describe "a reference whose record has a broken label" do + it "falls back to the type and id instead of raising" do + organization = create(:organization) + tagging = create(:sectorable_item, sectorable: organization, sector: create(:sector)) + allow_any_instance_of(SectorableItem).to receive(:title).and_raise(NoMethodError) + event = create( + :ahoy_event, + name: "update.organization", + resource_type: "Organization", + resource_id: organization.id, + properties: { + resource_type: "Organization", resource_id: organization.id, + association_changes: { sectorable_items: [ { action: "added", type: "SectorableItem", id: tagging.id } ] } + } + ) + + rows = event.decorate.detail_rows + + expect(rows.map { |row| row[:link] }.compact.first[:text]).to eq("SectorableItem ##{tagging.id}") + end + end end diff --git a/spec/models/sectorable_item_spec.rb b/spec/models/sectorable_item_spec.rb index 344ecf516c..75d7d2ccac 100644 --- a/spec/models/sectorable_item_spec.rb +++ b/spec/models/sectorable_item_spec.rb @@ -14,6 +14,21 @@ it { should validate_uniqueness_of(:sector_id).scoped_to([ :sectorable_type, :sectorable_id ]).with_message("has already been added") } end + describe "#title" do + it "reads as the sector for a sectorable with no title of its own" do + tagging = create(:sectorable_item, sectorable: create(:organization), sector: create(:sector, name: "Housing")) + + expect(tagging.title).to eq("Housing") + end + + it "composes the log's title and windows type for a workshop log" do + log = create(:workshop_log) + tagging = create(:sectorable_item, sectorable: log) + + expect(tagging.title).to start_with(log.title.to_s) + end + end + # it 'is valid with valid attributes' do # # Note: Factory needs associations uncommented for create # # expect(build(:sectorable_item)).to be_valid diff --git a/spec/requests/organizations_change_log_spec.rb b/spec/requests/organizations_change_log_spec.rb index 9a6767b945..fbefd60175 100644 --- a/spec/requests/organizations_change_log_spec.rb +++ b/spec/requests/organizations_change_log_spec.rb @@ -7,4 +7,26 @@ let(:record) { create(:organization) } let(:page_path) { edit_organization_path(record) } end + + # Sector taggings are nested attributes on the org, so its change log + # references them — and reaching for their label used to raise. + it "renders a change log that references the org's sector taggings" do + organization = create(:organization) + tagging = create(:sectorable_item, sectorable: organization, sector: create(:sector, name: "Housing")) + create( + :ahoy_event, + name: "update.organization", + resource_type: "Organization", + resource_id: organization.id, + properties: { + resource_type: "Organization", resource_id: organization.id, + association_changes: { sectorable_items: [ { action: "added", type: "SectorableItem", id: tagging.id } ] } + } + ) + + get edit_organization_path(organization) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Housing") + end end From e19d13c8d9cd8aed08f27e70a9e2a54f625465ea Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 16:01:46 -0400 Subject: [PATCH 11/15] Make a change log read like one MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things made it read like a data dump. A form posts every field, so untouched blanks arrive as nil -> "" and dirty tracking counts them: the log filled with rows saying a field went from empty to empty. Those are dropped at capture, and filtered on the way out too, so events already recorded read cleanly. A nested record's diffs came out as bare rows in whatever order MySQL stored the JSON keys — which is "after" before "before", by key length. They now render as the same before/after pair as the record's own changes, in that order, from the renderer rather than the payload. And a comment's topic titles its body rather than sitting under it, so a record's heading field leads and stands out from the fields beneath it. Co-Authored-By: Claude Opus 5 (1M context) --- app/decorators/ahoy/event_decorator.rb | 43 ++++++++++++-- app/models/concerns/ahoy_trackable.rb | 4 ++ .../ahoy_activities/_event_details.html.erb | 13 +++- spec/decorators/ahoy/event_decorator_spec.rb | 59 ++++++++++++++++++- spec/models/concerns/ahoy_trackable_spec.rb | 12 ++++ 5 files changed, 123 insertions(+), 8 deletions(-) diff --git a/app/decorators/ahoy/event_decorator.rb b/app/decorators/ahoy/event_decorator.rb index ebcd9db4f9..07ce1099de 100644 --- a/app/decorators/ahoy/event_decorator.rb +++ b/app/decorators/ahoy/event_decorator.rb @@ -3,6 +3,9 @@ class EventDecorator < ApplicationDecorator # Already surfaced in their own table columns, so redundant inside the details cell. REDUNDANT_KEYS = %w[resource_type resource_id resource_title].freeze + # Fields that title the record they belong to, in the order they should lead. + HEADING_KEYS = %w[topic title name subject].freeze + # Everything the dedicated columns don't already show. def extra_properties properties_hash.except(*REDUNDANT_KEYS) @@ -21,8 +24,10 @@ def changes? def changes_summary return [] unless changes? - change_diffs.map do |field, diff| + change_diffs.filter_map do |field, diff| diff = {} unless diff.is_a?(Hash) + next if blank_change?(diff) + { field: field.to_s.humanize, before: display_value(diff["before"]), @@ -69,10 +74,20 @@ def hash_rows(hash, label, depth) rows = label ? [ { label: label, value: nil, depth: depth } ] : [] child_depth = label ? depth + 1 : depth - hash.each { |key, val| rows.concat(flatten_rows(val, humanize_key(key), child_depth)) } + heading_first(hash).each do |key, val| + child_rows = flatten_rows(val, humanize_key(key), child_depth) + child_rows.first[:emphasis] = true if HEADING_KEYS.include?(key.to_s) && child_rows.one? + rows.concat(child_rows) + end rows end + # A comment's topic titles its body rather than sitting beside it, so it leads. + def heading_first(hash) + headings, rest = hash.partition { |key, _| HEADING_KEYS.include?(key.to_s) } + headings.sort_by { |key, _| HEADING_KEYS.index(key.to_s) } + rest + end + def array_rows(array, label, depth) return [ { label: label, value: "(empty)", depth: depth } ] if array.empty? @@ -104,11 +119,29 @@ def entity_label(item) # The link, then what the record actually said — a comment's body reads better # than "a comment was added". def reference_rows(label, item, depth) - detail = item["changes"].presence || item["attributes"].presence rows = [ reference_row(label, item, depth) ] - return rows if detail.blank? + rows += change_rows(item["changes"], depth + 1) + rows += flatten_rows(item["attributes"], nil, depth + 1) if item["attributes"].present? + rows + end + + # A nested record's diffs read like the record's own: field, then before, then + # after. The order comes from here rather than the payload — MySQL reorders + # the keys of a JSON object. + def change_rows(diffs, depth) + return [] unless diffs.is_a?(Hash) + + diffs.filter_map do |field, diff| + diff = {} unless diff.is_a?(Hash) + next if blank_change?(diff) + + { label: humanize_key(field), depth: depth, + change: { before: display_value(diff["before"]), after: display_value(diff["after"]) } } + end + end - rows + flatten_rows(detail, nil, depth + 1) + def blank_change?(diff) + diff["before"].blank? && diff["after"].blank? end def reference_row(label, item, depth) diff --git a/app/models/concerns/ahoy_trackable.rb b/app/models/concerns/ahoy_trackable.rb index 9f888f8df5..2767059624 100644 --- a/app/models/concerns/ahoy_trackable.rb +++ b/app/models/concerns/ahoy_trackable.rb @@ -318,6 +318,10 @@ def safe_assoc_class_name(assoc) 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| + # A form posts every field, so untouched blanks arrive as nil -> "". Dirty + # tracking counts that; a reader shouldn't have to. + next if before.blank? && after.blank? + h[attr] = { before: before, after: after } end end diff --git a/app/views/admin/ahoy_activities/_event_details.html.erb b/app/views/admin/ahoy_activities/_event_details.html.erb index 1f3ba785ab..3c7e7f9e02 100644 --- a/app/views/admin/ahoy_activities/_event_details.html.erb +++ b/app/views/admin/ahoy_activities/_event_details.html.erb @@ -34,8 +34,19 @@ <% else %> <%= row[:link][:text] %> <% end %> + <% elsif row[:change] %> +
    +
    + Before: + <%= row[:change][:before] %> +
    +
    + After: + <%= row[:change][:after] %> +
    +
    <% elsif row[:value] %> - <%= row[:value] %> + "><%= row[:value] %> <% end %>
    <% end %> diff --git a/spec/decorators/ahoy/event_decorator_spec.rb b/spec/decorators/ahoy/event_decorator_spec.rb index 2df926500c..da3e2b2908 100644 --- a/spec/decorators/ahoy/event_decorator_spec.rb +++ b/spec/decorators/ahoy/event_decorator_spec.rb @@ -47,12 +47,17 @@ def decorate(properties) expect(event.changes_summary).to match_array( [ { field: "Display name", before: "Old", after: "New" }, - { field: "Active", before: "No", after: "Yes" }, - { field: "Note", before: "(empty)", after: "(empty)" } + { field: "Active", before: "No", after: "Yes" } ] ) end + it "leaves out a field that was blank before and after" do + event = decorate("changes" => { "note" => { "before" => nil, "after" => "" } }) + + expect(event.changes_summary).to eq([]) + end + it "is empty when the event has no changes" do event = decorate("resource_title" => "Test") expect(event.changes_summary).to eq([]) @@ -182,4 +187,54 @@ def decorate(properties) expect(rows.map { |row| row[:link] }.compact.first[:text]).to eq("SectorableItem ##{tagging.id}") end end + + describe "detail rows for a nested record" do + let(:organization) { create(:organization) } + + def event_with(association_changes) + create( + :ahoy_event, + name: "update.organization", + resource_type: "Organization", + resource_id: organization.id, + properties: { + resource_type: "Organization", resource_id: organization.id, + association_changes: association_changes + } + ).decorate + end + + it "reads the topic first and marks it as the heading" do + rows = event_with(comments: [ { + action: "added", type: "Comment", id: 1, + attributes: { "body" => "Left a voicemail", "topic" => "Payment" } + } ]).detail_rows + + labelled = rows.select { |row| row[:value].present? } + expect(labelled.map { |row| row[:label] }).to eq([ "Topic", "Body" ]) + expect(labelled.first[:emphasis]).to be(true) + expect(labelled.last[:emphasis]).to be_nil + end + + it "renders a diff as before then after, whatever order it was stored in" do + rows = event_with(addresses: [ { + action: "updated", type: "Address", id: 1, + changes: { "city" => { "after" => "Long Beach", "before" => "Lake Lamar" } } + } ]).detail_rows + + change = rows.find { |row| row[:change] } + expect(change[:label]).to eq("City") + expect(change[:change]).to eq({ before: "Lake Lamar", after: "Long Beach" }) + end + + it "leaves out a field that was blank before and after" do + rows = event_with(addresses: [ { + action: "updated", type: "Address", id: 1, + changes: { "phone" => { "before" => nil, "after" => "" }, + "city" => { "before" => "Lake Lamar", "after" => "Long Beach" } } + } ]).detail_rows + + expect(rows.filter_map { |row| row[:label] if row[:change] }).to eq([ "City" ]) + end + end end diff --git a/spec/models/concerns/ahoy_trackable_spec.rb b/spec/models/concerns/ahoy_trackable_spec.rb index e6a2e5827b..063d1b6d8f 100644 --- a/spec/models/concerns/ahoy_trackable_spec.rb +++ b/spec/models/concerns/ahoy_trackable_spec.rb @@ -71,6 +71,18 @@ def event_named(name) end end + describe "blank-to-blank changes" do + it "records nothing for a field that was blank before and after" do + organization = create(:organization, email: nil) + Analytics::LifecycleBuffer.store.clear + + organization.update!(email: "", name: "Renamed") + + changes = event_named("update.organization")[:properties][:changes] + expect(changes.keys).to contain_exactly("name") + end + end + describe "nested records" do it "records an added nested record on the parent's event" do registration = create(:event_registration) From 8f5eee99b48bd377162b53d5b49a541c10b70e88 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 17:17:51 -0400 Subject: [PATCH 12/15] Log category and sector changes on the record's change log MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Category/sector memberships are assigned through has_many :through collection setters that persist immediately, outside the record's dirty tracking, so the change-log callbacks never saw them — a category changed on the edit form was invisible in the log. Fold the before/after diff onto the record's own update event via TagAssignable, so every membership change shows up like the nested edits already do. Applies to all assign_associations consumers, not just people. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/controllers/concerns/tag_assignable.rb | 23 +++++- app/models/concerns/ahoy_trackable.rb | 27 +++++++ spec/models/concerns/ahoy_trackable_spec.rb | 33 +++++++++ .../people_category_change_log_spec.rb | 74 +++++++++++++++++++ 4 files changed, 156 insertions(+), 1 deletion(-) create mode 100644 spec/requests/people_category_change_log_spec.rb diff --git a/app/controllers/concerns/tag_assignable.rb b/app/controllers/concerns/tag_assignable.rb index 8b4a932750..98b53f0c4b 100644 --- a/app/controllers/concerns/tag_assignable.rb +++ b/app/controllers/concerns/tag_assignable.rb @@ -9,6 +9,8 @@ def assign_associations(record, param_key: nil) selected_category_ids = Array(params[key][:category_ids]).reject(&:blank?).map(&:to_i) selected = Category.where(id: selected_category_ids).to_a + categories_before = record.categories.to_a + if params[key].key?(:managed_category_type_ids) # The form only edits certain category types (e.g. age ranges + workshop # settings). Preserve taggings of every other type the form never shows so @@ -20,10 +22,14 @@ def assign_associations(record, param_key: nil) else record.categories = selected end + categories_after = record.categories.to_a - if params[key].key?(:sector_ids) + sectors_changed = params[key].key?(:sector_ids) + if sectors_changed + sectors_before = record.sectors.to_a selected_sector_ids = Array(params[key][:sector_ids]).reject(&:blank?).map(&:to_i) record.sectors = Sector.where(id: selected_sector_ids) + sectors_after = record.sectors.to_a end record.save! @@ -34,5 +40,20 @@ def assign_associations(record, param_key: nil) if params[key].key?(:primary_age_category_ids) && record.respond_to?(:apply_primary_age_groups!) record.apply_primary_age_groups!(Array(params[key][:primary_age_category_ids])) end + + # These memberships change outside the record's dirty tracking, so hand the + # diff to the change log directly (a no-op when nothing actually moved). + return unless record.respond_to?(:track_membership_changes) + + record.track_membership_changes( + categories: membership_delta(categories_before, categories_after), + sectors: (membership_delta(sectors_before, sectors_after) if sectors_changed) + ) + end + + def membership_delta(before, after) + before = Array(before) + after = Array(after) + { added: after - before, removed: before - after } end end diff --git a/app/models/concerns/ahoy_trackable.rb b/app/models/concerns/ahoy_trackable.rb index 2767059624..025de13ff8 100644 --- a/app/models/concerns/ahoy_trackable.rb +++ b/app/models/concerns/ahoy_trackable.rb @@ -14,8 +14,35 @@ module AhoyTrackable before_destroy :capture_destroy_snapshot end + # Category and sector memberships are reassigned through has_many :through + # collection setters (categories=/sectors=) that persist immediately, outside + # this record's dirty tracking — so the update callbacks never see them. The + # code that reassigns them (TagAssignable) hands the before/after records here + # to fold the diff onto the change log as the record's own update event. + def track_membership_changes(memberships) + association_changes = memberships.each_with_object({}) do |(assoc_name, delta), changes| + entries = membership_change_entries(delta) + changes[assoc_name] = entries if entries.present? + end + return if association_changes.blank? + + track_lifecycle_event("update", association_changes: association_changes) + end + private + def membership_change_entries(delta) + return [] if delta.blank? + + added = Array(delta[:added]).map { |record| membership_entry("added", record) } + removed = Array(delta[:removed]).map { |record| membership_entry("removed", record) } + added + removed + end + + def membership_entry(action, record) + { action: action, type: record.class.name, id: record.id } + end + def devise_only_changes?(changes) auth_fields = %w[ current_sign_in_at diff --git a/spec/models/concerns/ahoy_trackable_spec.rb b/spec/models/concerns/ahoy_trackable_spec.rb index 063d1b6d8f..aac66e8639 100644 --- a/spec/models/concerns/ahoy_trackable_spec.rb +++ b/spec/models/concerns/ahoy_trackable_spec.rb @@ -153,6 +153,39 @@ def event_named(name) end end + describe "membership changes" do + it "records an added record as the parent's own update event" do + person = create(:person) + category = create(:category) + Analytics::LifecycleBuffer.store.clear + + person.track_membership_changes(categories: { added: [ category ], removed: [] }) + + added = event_named("update.person")[:properties][:association_changes][:categories].first + expect(added).to eq({ action: "added", type: "Category", id: category.id }) + end + + it "records a removed record" do + person = create(:person) + category = create(:category) + Analytics::LifecycleBuffer.store.clear + + person.track_membership_changes(categories: { added: [], removed: [ category ] }) + + removed = event_named("update.person")[:properties][:association_changes][:categories].first + expect(removed).to include(action: "removed", type: "Category", id: category.id) + end + + it "records nothing when nothing moved" do + person = create(:person) + Analytics::LifecycleBuffer.store.clear + + person.track_membership_changes(categories: { added: [], removed: [] }, sectors: nil) + + expect(event_named("update.person")).to be_nil + end + end + describe "attachments" do it "records an added attachment on the record's event" do person = create(:person) diff --git a/spec/requests/people_category_change_log_spec.rb b/spec/requests/people_category_change_log_spec.rb new file mode 100644 index 0000000000..362852fb00 --- /dev/null +++ b/spec/requests/people_category_change_log_spec.rb @@ -0,0 +1,74 @@ +require "rails_helper" + +# Categories are assigned through the has_many :through collection setter in +# TagAssignable#assign_associations, which persists immediately and outside the +# record's dirty tracking — so without help they never reach the change log. +# These specs drive the real edit-form flow and read the lifecycle buffer the +# controller flushes to Ahoy, pinning that a category change is now recorded on +# the person's own event. +RSpec.describe "People change log — category assignments", type: :request do + let(:person) { create(:person) } + let(:category) { create(:category) } + + before do + Analytics::LifecycleBuffer.store.clear + sign_in create(:user, :admin) + end + + after { Current.user = nil } + + # The controller flushes the buffer (and clears it) in an after_action, so grab + # a copy as it flushes rather than reading the emptied buffer afterwards. + def buffered_during_request + events = [] + allow(Analytics::LifecycleBuffer).to receive(:flush) do + events.concat(Analytics::LifecycleBuffer.store) + Analytics::LifecycleBuffer.store.clear + end + yield + events + end + + def person_association_changes(events) + event = events.find { |e| e[:name] == "update.person" } + event&.dig(:properties, :association_changes) + end + + it "records a category added through the edit form on the person's change log" do + events = buffered_during_request do + patch person_path(person), params: { + person: { category_ids: [ category.id ], managed_category_type_ids: [ category.category_type_id ] } + } + end + + added = person_association_changes(events)&.dig(:categories) + expect(added).to be_present + expect(added.first).to include(action: "added", type: "Category", id: category.id) + end + + it "records a category removed through the edit form on the person's change log" do + create(:categorizable_item, categorizable: person, category: category) + + events = buffered_during_request do + patch person_path(person), params: { + person: { category_ids: [], managed_category_type_ids: [ category.category_type_id ] } + } + end + + removed = person_association_changes(events)&.dig(:categories) + expect(removed).to be_present + expect(removed.first).to include(action: "removed", type: "Category", id: category.id) + end + + it "records nothing when the category set is unchanged" do + create(:categorizable_item, categorizable: person, category: category) + + events = buffered_during_request do + patch person_path(person), params: { + person: { category_ids: [ category.id ], managed_category_type_ids: [ category.category_type_id ] } + } + end + + expect(person_association_changes(events)).to be_nil + end +end From 939ec4b6849ca6cabc44668de2513b7373c43488 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 21:43:30 -0400 Subject: [PATCH 13/15] Round out the Ahoy change log and theme the comments section Attachment changes travel as filenames (staged attachments have no id yet), so render them as names/actions rather than dead reference lookups. Treat print as a non-mutation, and batch-resolve every event's referenced records up front in the inline activity log to kill the per-reference N+1. Move the category type's audit info + activity log inside the panel. Theme the person comments section and its editor with the comments domain color, moving the admin-only blue wash onto the heading. Co-Authored-By: Claude Opus 4.8 (1M context) --- app/decorators/ahoy/event_decorator.rb | 15 ++++++++ app/models/ahoy/event.rb | 2 +- .../ahoy_activities/_event_details.html.erb | 2 +- app/views/application/_activity_log.html.erb | 9 +++-- app/views/category_types/edit.html.erb | 13 +++---- spec/decorators/ahoy/event_decorator_spec.rb | 21 ++++++++++++ spec/models/ahoy/event_spec.rb | 22 ++++++++++++ .../requests/organizations_change_log_spec.rb | 34 ++++++++++++++++--- 8 files changed, 104 insertions(+), 14 deletions(-) create mode 100644 spec/models/ahoy/event_spec.rb diff --git a/app/decorators/ahoy/event_decorator.rb b/app/decorators/ahoy/event_decorator.rb index 07ce1099de..2903d72c7d 100644 --- a/app/decorators/ahoy/event_decorator.rb +++ b/app/decorators/ahoy/event_decorator.rb @@ -97,6 +97,8 @@ def array_rows(array, label, depth) header = label ? [ { label: label, value: nil, depth: depth } ] : [] child_depth = label ? depth + 1 : depth header + array.flat_map { |item| reference_rows(nil, item, child_depth) } + elsif array.all? { |item| attachment_change?(item) } + array.map { |item| attachment_row(label, item, depth) } else [ { label: label, value: array.map { |item| display_value(item) }.join(", "), depth: depth } ] end @@ -106,6 +108,19 @@ def reference?(item) Analytics::EventReferenceLoader.reference?(item) end + # An attachment is staged on the record and has no id until the save lands, + # so it travels as a filename rather than as a reference to look up. + def attachment_change?(item) + item.is_a?(Hash) && item["type"] == "ActiveStorage::Attachment" && + item["action"].present? && item["id"].blank? && item["record_id"].blank? + end + + # A file has no page to link to, so it reads as its name. A removal keeps only + # the action — the blob is gone by the time anyone reads this. + def attachment_row(label, item, depth) + { label: label, depth: depth, action: item["action"], value: item["filename"].presence } + end + def named_entity?(item) item.is_a?(Hash) && (item["name"].present? || item["title"].present?) end diff --git a/app/models/ahoy/event.rb b/app/models/ahoy/event.rb index a12d5b6572..86ba2be484 100644 --- a/app/models/ahoy/event.rb +++ b/app/models/ahoy/event.rb @@ -9,7 +9,7 @@ class Ahoy::Event < ApplicationRecord # Reading a record isn't a change to it. A record's change log asks what # happened to it, so these stay on the admin activities index, which is where # browsing belongs. - NON_MUTATION_PREFIXES = %w[view search filter download].freeze + NON_MUTATION_PREFIXES = %w[view print search filter download].freeze scope :mutations, -> { NON_MUTATION_PREFIXES.reduce(all) { |scope, prefix| scope.where.not(arel_table[:name].matches("#{prefix}.%")) } diff --git a/app/views/admin/ahoy_activities/_event_details.html.erb b/app/views/admin/ahoy_activities/_event_details.html.erb index 3c7e7f9e02..92a6e7d000 100644 --- a/app/views/admin/ahoy_activities/_event_details.html.erb +++ b/app/views/admin/ahoy_activities/_event_details.html.erb @@ -27,8 +27,8 @@ <% if row[:label] %> <%= row[:label] %><%= ":" if row[:value] || row[:link] %> <% end %> + <% if row[:action] %><%= row[:action] %><% end %> <% if row[:link] %> - <% if row[:action] %><%= row[:action] %><% end %> <% if row[:link][:path] %> <%= link_to row[:link][:text], row[:link][:path], class: "text-indigo-600 hover:underline" %> <% else %> diff --git a/app/views/application/_activity_log.html.erb b/app/views/application/_activity_log.html.erb index 487d96aaec..87ea2aa03e 100644 --- a/app/views/application/_activity_log.html.erb +++ b/app/views/application/_activity_log.html.erb @@ -7,7 +7,11 @@ the events were recorded against, the same way shared/_audit_info does. %> <% model = record.respond_to?(:object) ? record.object : record %> <% events = Ahoy::Event.mutations.where(resource_type: model.class.name, resource_id: model.id) - .includes(:user).order(time: :desc) %> + .includes(:user).order(time: :desc).to_a %> +<%# Resolve every record the events reference up front, one query per type, the + same way the admin activities table does — otherwise each reference on each + event costs its own lookup. %> +<% record_cache = Analytics::EventReferenceLoader.new(events).records %> <% if events.empty? %> <%# Said plainly rather than left out: an absent log reads as "this page hasn't got one", which is a different thing from "nothing has happened yet". %> @@ -36,7 +40,8 @@ by <%= event.user&.full_name.presence || event.properties["source"] || "Unknown" %>
    - <%= render "admin/ahoy_activities/event_details", activity: event.decorate %> + <%= render "admin/ahoy_activities/event_details", + activity: event.decorate(context: { record_cache: record_cache }) %>
    <% end %>
    diff --git a/app/views/category_types/edit.html.erb b/app/views/category_types/edit.html.erb index f5ae815a91..966b058f35 100644 --- a/app/views/category_types/edit.html.erb +++ b/app/views/category_types/edit.html.erb @@ -14,12 +14,13 @@ <%= render "form" %>
    + + <%= render "shared/audit_info", resource: @category_type %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
    + <%= render "application/activity_log", record: @category_type %> +
    + <% end %>
    - <%= render "shared/audit_info", resource: @category_type %> - <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> -
    - <%= render "application/activity_log", record: @category_type %> -
    - <% end %>
    diff --git a/spec/decorators/ahoy/event_decorator_spec.rb b/spec/decorators/ahoy/event_decorator_spec.rb index da3e2b2908..0f095009b7 100644 --- a/spec/decorators/ahoy/event_decorator_spec.rb +++ b/spec/decorators/ahoy/event_decorator_spec.rb @@ -236,5 +236,26 @@ def event_with(association_changes) expect(rows.filter_map { |row| row[:label] if row[:change] }).to eq([ "City" ]) end + + it "reads an added attachment as its filename" do + rows = event_with(avatar_attachment: [ { + action: "added", type: "ActiveStorage::Attachment", filename: "headshot.png" + } ]).detail_rows + + attachment = rows.last + expect(attachment[:action]).to eq("added") + expect(attachment[:value]).to eq("headshot.png") + expect(rows.map { |row| row[:value] }).not_to include(a_string_including("ActiveStorage")) + end + + it "reads a removed attachment as the removal, the file being gone" do + rows = event_with(avatar_attachment: [ { + action: "removed", type: "ActiveStorage::Attachment" + } ]).detail_rows + + attachment = rows.last + expect(attachment[:action]).to eq("removed") + expect(attachment[:value]).to be_nil + end end end diff --git a/spec/models/ahoy/event_spec.rb b/spec/models/ahoy/event_spec.rb new file mode 100644 index 0000000000..de978b8165 --- /dev/null +++ b/spec/models/ahoy/event_spec.rb @@ -0,0 +1,22 @@ +require "rails_helper" + +RSpec.describe Ahoy::Event do + describe ".mutations" do + it "keeps the events that changed the record" do + %w[create.workshop update.workshop destroy.workshop].each do |name| + create(:ahoy_event, name: name) + end + + expect(described_class.mutations.pluck(:name)) + .to contain_exactly("create.workshop", "update.workshop", "destroy.workshop") + end + + it "leaves out reads of the record" do + %w[view.workshop print.workshop download.workshop search.workshops filter.workshops].each do |name| + create(:ahoy_event, name: name) + end + + expect(described_class.mutations).to be_empty + end + end +end diff --git a/spec/requests/organizations_change_log_spec.rb b/spec/requests/organizations_change_log_spec.rb index fbefd60175..0b8879eb93 100644 --- a/spec/requests/organizations_change_log_spec.rb +++ b/spec/requests/organizations_change_log_spec.rb @@ -13,6 +13,30 @@ it "renders a change log that references the org's sector taggings" do organization = create(:organization) tagging = create(:sectorable_item, sectorable: organization, sector: create(:sector, name: "Housing")) + create_tagging_event(organization, tagging) + + get edit_organization_path(organization) + + expect(response).to have_http_status(:ok) + expect(response.body).to include("Housing") + end + + # The referenced records load in one batch per type, so a longer log costs no + # more queries than a short one. + it "does not query per reference as the change log grows" do + organization = create(:organization) + + create_tagging_event(organization, create(:sectorable_item, sectorable: organization)) + one_event = tagging_queries_for(edit_organization_path(organization)) + + 2.times { create_tagging_event(organization, create(:sectorable_item, sectorable: organization)) } + three_events = tagging_queries_for(edit_organization_path(organization)) + + expect(response).to have_http_status(:ok) + expect(three_events).to eq(one_event) + end + + def create_tagging_event(organization, tagging) create( :ahoy_event, name: "update.organization", @@ -23,10 +47,12 @@ association_changes: { sectorable_items: [ { action: "added", type: "SectorableItem", id: tagging.id } ] } } ) + end - get edit_organization_path(organization) - - expect(response).to have_http_status(:ok) - expect(response.body).to include("Housing") + def tagging_queries_for(path) + count = 0 + counter = ->(_n, _s, _f, _i, payload) { count += 1 if payload[:sql]&.match?(/SELECT.+FROM `sectorable_items`/) } + ActiveSupport::Notifications.subscribed(counter, "sql.active_record") { get path } + count end end From 7f587547f710cb30f9d3ac8fb73aad4ab2732441 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 22:02:54 -0400 Subject: [PATCH 14/15] Tell facilitators the change log exists The Features & tips page is where non-devs find out what the portal does, and an edit history nobody knows about gets used by nobody. Co-Authored-By: Claude Opus 5 (1M context) --- config/features.yml | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/config/features.yml b/config/features.yml index dc5960ce4b..243e5a9fa9 100644 --- a/config/features.yml +++ b/config/features.yml @@ -2194,3 +2194,26 @@ subscription form, filter the subscriptions list by organization, and see it in the new Organization column. It's optional — leave it blank for a topic that isn't tied to any organization. + +- name: "Change log on every record's edit page" + area: reporting + display_status: admin_facing + released_on: 2026-08-22 + pr_number: 2245 + summary: >- + Every edit page now shows a change log under its edit history: a plain-language + record of what changed, who changed it, and when — including nested records, + attached files, and rich text. When nothing has happened yet, it says so rather + than showing nothing. + description: >- + The change log answers "what happened to this record?" It captures real edits + only — plain browsing (views, prints, searches, filters, downloads) is kept out — + and reads each entry in plain language: what a save did to the record itself, to + its nested child records, to attached files (shown by name), and to rich text + that previously passed through unrecorded. A nested record reports what it + contained, not just that it changed. It sits in the same spot on every page, + under the edit history, and is available on the organization, continuing + education, staff tag, and category type pages, among others. Admin-only. + pro_tips: + - "Only mutations show: opening or printing a record never clutters its log." + - "Deleting a child or attachment is recorded too — the entry keeps the name even after the file is gone." From 706c5aa955aca7ee5a4ba9f857d8c062ac3ddd27 Mon Sep 17 00:00:00 2001 From: Mae Beale Date: Sun, 23 Aug 2026 22:16:33 -0400 Subject: [PATCH 15/15] Keep the name of a file that was removed A change log that only says "attachment removed" can't tell you which file went, and by the time anyone reads the entry the blob is gone. The staged delete carries no blob, so read the outgoing attachment off the record while the save has yet to apply the change. Co-Authored-By: Claude Opus 5 (1M context) --- app/decorators/ahoy/event_decorator.rb | 4 ++-- app/models/concerns/ahoy_trackable.rb | 15 +++++++++++++-- spec/decorators/ahoy/event_decorator_spec.rb | 12 +++++++++++- spec/models/concerns/ahoy_trackable_spec.rb | 13 ++++++++++++- 4 files changed, 38 insertions(+), 6 deletions(-) diff --git a/app/decorators/ahoy/event_decorator.rb b/app/decorators/ahoy/event_decorator.rb index 2903d72c7d..ae77fb0fcd 100644 --- a/app/decorators/ahoy/event_decorator.rb +++ b/app/decorators/ahoy/event_decorator.rb @@ -115,8 +115,8 @@ def attachment_change?(item) item["action"].present? && item["id"].blank? && item["record_id"].blank? end - # A file has no page to link to, so it reads as its name. A removal keeps only - # the action — the blob is gone by the time anyone reads this. + # A file has no page to link to, so it reads as its name — kept on a removal + # too, since the blob it named is gone by the time anyone reads this. def attachment_row(label, item, depth) { label: label, depth: depth, action: item["action"], value: item["filename"].presence } end diff --git a/app/models/concerns/ahoy_trackable.rb b/app/models/concerns/ahoy_trackable.rb index 025de13ff8..c06dd538f4 100644 --- a/app/models/concerns/ahoy_trackable.rb +++ b/app/models/concerns/ahoy_trackable.rb @@ -202,13 +202,24 @@ def capture_pending_attachment_changes assoc: :"#{name}_attachment", type: "ActiveStorage::Attachment", action: removal ? "removed" : "added", - filename: (attachment_filenames(change) unless removal) + filename: removal ? removed_attachment_filenames(name) : attachment_filenames(change) }.compact end end def attachment_filenames(change) - blobs = change.try(:blobs) || Array(change.try(:blob)) + blob_names(change.try(:blobs) || Array(change.try(:blob))) + end + + # A staged delete carries no blob of its own, so read what is still attached: + # the outgoing file stays on the record until the save applies the change, and + # its name is all the log can keep once the blob is gone. + def removed_attachment_filenames(name) + attached = try(:"#{name}_attachments") || try(:"#{name}_attachment") + blob_names(Array(attached).filter_map(&:blob)) + end + + def blob_names(blobs) blobs.filter_map { |blob| blob.filename.to_s.presence }.join(", ").presence end diff --git a/spec/decorators/ahoy/event_decorator_spec.rb b/spec/decorators/ahoy/event_decorator_spec.rb index 0f095009b7..a37dc1e7a4 100644 --- a/spec/decorators/ahoy/event_decorator_spec.rb +++ b/spec/decorators/ahoy/event_decorator_spec.rb @@ -248,7 +248,17 @@ def event_with(association_changes) expect(rows.map { |row| row[:value] }).not_to include(a_string_including("ActiveStorage")) end - it "reads a removed attachment as the removal, the file being gone" do + it "reads a removed attachment as its name" do + rows = event_with(avatar_attachment: [ { + action: "removed", type: "ActiveStorage::Attachment", filename: "headshot.png" + } ]).detail_rows + + attachment = rows.last + expect(attachment[:action]).to eq("removed") + expect(attachment[:value]).to eq("headshot.png") + end + + it "falls back to the action alone for an older entry with no filename" do rows = event_with(avatar_attachment: [ { action: "removed", type: "ActiveStorage::Attachment" } ]).detail_rows diff --git a/spec/models/concerns/ahoy_trackable_spec.rb b/spec/models/concerns/ahoy_trackable_spec.rb index aac66e8639..e320fc9015 100644 --- a/spec/models/concerns/ahoy_trackable_spec.rb +++ b/spec/models/concerns/ahoy_trackable_spec.rb @@ -194,7 +194,18 @@ def event_named(name) person.update!(avatar: Rack::Test::UploadedFile.new(Rails.root.join("app/assets/images/missing.png"), "image/png")) attached = event_named("update.person")[:properties][:association_changes][:avatar_attachment].first - expect(attached).to include(action: "added", type: "ActiveStorage::Attachment") + expect(attached).to include(action: "added", type: "ActiveStorage::Attachment", filename: "missing.png") + end + + it "keeps the name of a removed attachment, the blob being gone afterwards" do + person = create(:person) + person.update!(avatar: Rack::Test::UploadedFile.new(Rails.root.join("app/assets/images/missing.png"), "image/png")) + Analytics::LifecycleBuffer.store.clear + + person.update!(avatar: nil) + + removed = event_named("update.person")[:properties][:association_changes][:avatar_attachment].first + expect(removed).to include(action: "removed", filename: "missing.png") end end end