Capture what a save actually changed

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

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"]),
@@ -64,15 +69,25 @@ 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 } ] : []
       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?
 
@@ -81,7 +96,9 @@ 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) }
+      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
@@ -91,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 — 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
+
     def named_entity?(item)
       item.is_a?(Hash) && (item["name"].present? || item["title"].present?)
     end
@@ -101,15 +131,52 @@ 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)
+      rows = [ reference_row(label, item, depth) ]
+      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
+
+    def blank_change?(diff)
+      diff["before"].blank? && diff["after"].blank?
+    end
+
     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).

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 print search filter download].freeze
+
+  scope :mutations, -> {
+    NON_MUTATION_PREFIXES.reduce(all) { |scope, prefix| scope.where.not(arel_table[:name].matches("#{prefix}.%")) }
+  }
 end