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/decorators/ahoy/event_decorator.rb b/app/decorators/ahoy/event_decorator.rb index 54c3a95b4c..ae77fb0fcd 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"]), @@ -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). diff --git a/app/models/ahoy/event.rb b/app/models/ahoy/event.rb index 4c3125b538..86ba2be484 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 print 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/models/concerns/ahoy_trackable.rb b/app/models/concerns/ahoy_trackable.rb index 4c8116740f..c06dd538f4 100644 --- a/app/models/concerns/ahoy_trackable.rb +++ b/app/models/concerns/ahoy_trackable.rb @@ -1,16 +1,48 @@ 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_changes + after_save :resolve_pending_association_ids 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 @@ -40,7 +72,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? @@ -53,112 +85,142 @@ 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 = {} + def pending_association_change(assoc_name, 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 - self.class.nested_attributes_options.each_key do |assoc_name| - assoc = association(assoc_name.to_s) - next unless assoc.loaded? + record_changes = record.changes.except("updated_at", "created_at") + return if record_changes.empty? - 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 + { assoc: assoc_name, record: record, action: "updated", changes: format_tracked_changes(record_changes) } + end - # Merge in removals captured before save - if @_pending_association_removals.present? - @_pending_association_removals.each do |assoc_name, removals| - changes[assoc_name] ||= [] - changes[assoc_name].concat(removals) - end - end + # 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 = [] - # Track rich text changes - collect_rich_text_changes(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] + entry[:attributes] = pending[:attributes] if pending[:attributes].present? + @_unresolved_association_ids << [ entry, pending[:record] ] if pending[:record] && entry[:id].nil? - # Track attachment changes - collect_attachment_changes(changes) + (changes[pending[:assoc]] ||= []) << entry + end + end - changes + # 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 - 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 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? + 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 - 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: removal ? removed_attachment_filenames(name) : attachment_filenames(change) + }.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? + def attachment_filenames(change) + blob_names(change.try(:blobs) || Array(change.try(:blob))) + end - 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 + # 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 def capture_destroy_snapshot @@ -175,7 +237,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?) @@ -203,7 +265,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? @@ -294,6 +356,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/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/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/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? %> - - <% 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..92a6e7d000 --- /dev/null +++ b/app/views/admin/ahoy_activities/_event_details.html.erb @@ -0,0 +1,56 @@ +<%# 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? %> + + <% end %> + <% activity.detail_rows.each do |row| %> +
+ <% if row[:label] %> + <%= row[:label] %><%= ":" if row[:value] || row[:link] %> + <% end %> + <% if row[:action] %><%= row[:action] %><% end %> + <% if row[:link] %> + <% 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[:change] %> +
+
+ Before: + <%= row[:change][:before] %> +
+
+ After: + <%= row[:change][:after] %> +
+
+ <% elsif row[:value] %> + "><%= row[:value] %> + <% end %> +
+ <% end %> +
+<% else %> + +<% end %> 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 %> + +
+<% end %> diff --git a/app/views/category_types/edit.html.erb b/app/views/category_types/edit.html.erb index 0a2c32bc4c..966b058f35 100644 --- a/app/views/category_types/edit.html.erb +++ b/app/views/category_types/edit.html.erb @@ -1,19 +1,26 @@ <% 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 %>

<%= render "form" %>
+ + <%= 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/continuing_education_registrations/edit.html.erb b/app/views/continuing_education_registrations/edit.html.erb index 775963fde4..9c884d7c64 100644 --- a/app/views/continuing_education_registrations/edit.html.erb +++ b/app/views/continuing_education_registrations/edit.html.erb @@ -145,4 +145,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/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/event_registrations/edit.html.erb b/app/views/event_registrations/edit.html.erb index f0ea528285..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 %> @@ -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 "application/activity_log", record: @event_registration %> +
    + <% 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/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..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 %>
    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/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/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/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/app/views/topic_subscriptions/edit.html.erb b/app/views/topic_subscriptions/edit.html.erb index a537133221..00b92c3516 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,10 @@ Remove <% end %>
    + <%= render "shared/audit_info", resource: @topic_subscription %> + <% if allowed_to?(:index?, with: Admin::AhoyActivityPolicy) %> +
    + <%= render "application/activity_log", record: @topic_subscription %> +
    + <% end %>
    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." diff --git a/spec/decorators/ahoy/event_decorator_spec.rb b/spec/decorators/ahoy/event_decorator_spec.rb index 7526de9f81..a37dc1e7a4 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([]) @@ -160,4 +165,107 @@ 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 + + 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 + + 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 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 + + 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/models/concerns/ahoy_trackable_spec.rb b/spec/models/concerns/ahoy_trackable_spec.rb new file mode 100644 index 0000000000..e320fc9015 --- /dev/null +++ b/spec/models/concerns/ahoy_trackable_spec.rb @@ -0,0 +1,211 @@ +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 + + 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) + 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 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") + 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") + 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 + 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 "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) + 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", 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 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/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..0a537fa1fe 100644 --- a/spec/requests/continuing_education_registrations_spec.rb +++ b/spec/requests/continuing_education_registrations_spec.rb @@ -12,6 +12,16 @@ 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 + + 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 9d21797f33..bfee1b4c3d 100644 --- a/spec/requests/event_registrations_spec.rb +++ b/spec/requests/event_registrations_spec.rb @@ -462,6 +462,52 @@ 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 "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) + + expect(response.body).to include("Change log empty") + end + it "shows a Delete button for a deletable registration" do get edit_event_registration_path(existing_registration) 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/organizations_change_log_spec.rb b/spec/requests/organizations_change_log_spec.rb new file mode 100644 index 0000000000..0b8879eb93 --- /dev/null +++ b/spec/requests/organizations_change_log_spec.rb @@ -0,0 +1,58 @@ +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 + + # 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_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", + 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 } ] } + } + ) + end + + 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 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 diff --git a/spec/requests/people_change_log_spec.rb b/spec/requests/people_change_log_spec.rb new file mode 100644 index 0000000000..8a1ff57130 --- /dev/null +++ b/spec/requests/people_change_log_spec.rb @@ -0,0 +1,27 @@ +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) + 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/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/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..7a7c9cea86 --- /dev/null +++ b/spec/support/shared_examples/change_log_page.rb @@ -0,0 +1,42 @@ +# 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).not_to include("Change log empty") + expect(response.body).to include("after value") + end + + it "leaves out views and other reads of the record" do + create( + :ahoy_event, + name: "view.#{record.class.name.underscore}", + resource_type: record.class.name, + resource_id: record.id, + properties: { resource_type: record.class.name, resource_id: record.id } + ) + + get page_path + + expect(response.body).to include("Change log empty") + end + + 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 empty") + end +end