diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 49c20f1655..bb22b6c9c0 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -300,6 +300,9 @@ follow this). Match the existing pattern: - The controller `index` branches on `turbo_frame_request?`: the frame request builds the filtered/paginated scope and renders the results view; otherwise it renders the full `index`. +- **The skeleton request runs none of the frame's queries.** Build scopes, counts, + and totals only inside the `turbo_frame_request?` branch; a `@total = scope.count` + above the branch runs on both requests, so the page queries twice. - **Name the frame-response view after its Turbo frame tag id** — the `turbo_frame_tag` id, the controller render target, and the view filename are one shared `_results` token (e.g. `turbo_frame_tag :grants_results` ← @@ -347,6 +350,27 @@ follow this). Match the existing pattern: `data-turbo-permanent` (plus an `id`) on any TomSelect wrapper so the morph doesn't strip its widget. Workshop logs is the reference. +## Turbo frames & streams + +Rules from past PR reviews: + +- **One lazy request per page.** When several parts of a page load lazily (badges, + counts, panels), load them from one action, not one frame per part — each lazy + request holds a Puma thread, so the user's next click queues behind them. Wrap + the parts in one frame (admin home `action_buttons` is the reference). +- **Keep a frame response a pure frame swap — no `` tags inside it.** + If a frame's response needs to update elements outside the frame, widen the + frame to contain them. Some older results views still tuck streams into their + frame; don't copy them. +- **Answer stream requests with a `*.turbo_stream.erb` template** via + `respond_to { |format| format.turbo_stream }`, not inline `render turbo_stream:` + in the controller — the template keeps markup in views and out of actions. +- **Don't set `turbo: false` to make something work — find the root cause.** + Links that leave a frame use `data: { turbo_frame: "_top" }` (or a `target: "_top"` + frame); keep `turbo: false` for downloads, new tabs, and pages that truly need a + full load. Turbo already disables a form's submitter while it submits, so a Turbo + form needs no double-submit guard (`submit_once` is for forms that stay non-Turbo). + ## Features & tips page (`/features`) The login-gated **Features & tips** page lists shipped, user-facing features diff --git a/CLAUDE.md b/CLAUDE.md index 726450a05b..ee0e35df00 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -310,6 +310,9 @@ follow this). Match the existing pattern: - The controller `index` branches on `turbo_frame_request?`: the frame request builds the filtered/paginated scope and renders the results view; otherwise it renders the full `index`. +- **The skeleton request runs none of the frame's queries.** Build scopes, counts, + and totals only inside the `turbo_frame_request?` branch; a `@total = scope.count` + above the branch runs on both requests, so the page queries twice. - **Name the frame-response view after its Turbo frame tag id** — the `turbo_frame_tag` id, the controller render target, and the view filename are one shared `_results` token (e.g. `turbo_frame_tag :grants_results` ← @@ -357,6 +360,27 @@ follow this). Match the existing pattern: `data-turbo-permanent` (plus an `id`) on any TomSelect wrapper so the morph doesn't strip its widget. Workshop logs is the reference. +## Turbo frames & streams + +Rules from past PR reviews: + +- **One lazy request per page.** When several parts of a page load lazily (badges, + counts, panels), load them from one action, not one frame per part — each lazy + request holds a Puma thread, so the user's next click queues behind them. Wrap + the parts in one frame (admin home `action_buttons` is the reference). +- **Keep a frame response a pure frame swap — no `` tags inside it.** + If a frame's response needs to update elements outside the frame, widen the + frame to contain them. Some older results views still tuck streams into their + frame; don't copy them. +- **Answer stream requests with a `*.turbo_stream.erb` template** via + `respond_to { |format| format.turbo_stream }`, not inline `render turbo_stream:` + in the controller — the template keeps markup in views and out of actions. +- **Don't set `turbo: false` to make something work — find the root cause.** + Links that leave a frame use `data: { turbo_frame: "_top" }` (or a `target: "_top"` + frame); keep `turbo: false` for downloads, new tabs, and pages that truly need a + full load. Turbo already disables a form's submitter while it submits, so a Turbo + form needs no double-submit guard (`submit_once` is for forms that stay non-Turbo). + ## Features & tips page (`/features`) The login-gated **Features & tips** page lists shipped, user-facing features diff --git a/app/controllers/admin/home_controller.rb b/app/controllers/admin/home_controller.rb index 2da18cbe46..f08f29c5cb 100644 --- a/app/controllers/admin/home_controller.rb +++ b/app/controllers/admin/home_controller.rb @@ -2,10 +2,11 @@ module Admin class HomeController < ApplicationController include AdminCardsHelper - DATA_HEALTH_BADGE_TITLE = "Data checks only. Possible duplicates and author credit are counted separately on the Data health page.".freeze + REVIEW_BADGE = "Review".freeze + DATA_HEALTH_BADGE_TITLE = "Data checks only. Possible duplicates and author credit are listed separately on the Data health page.".freeze def index - authorize! :home, to: :index? + authorize! :home @system_cards = system_cards @user_content_cards = user_content_cards @@ -14,38 +15,25 @@ def index @deduper_cards = deduper_cards @system_settings_cards = system_settings_cards @deprecated_data_cards = deprecated_data_cards - @pending_change_requests_count = ProfileChangeRequest.pending.count - end - - def data_health_count - authorize! :home, to: :index? - - render_count_badge(:data_health_count, title: DATA_HEALTH_BADGE_TITLE) { DataHealth.checks.sum(&:count) } end - def author_credit_count + def action_buttons authorize! :home, to: :index? - render_count_badge(:author_credit_count) { DataHealth::Worklists.new.author_credit.sum(&:count) } - end - - def unsynced_features_count - authorize! Feature, to: :create? - - render_count_badge(:unsynced_features_count) { FeatureCatalog.new.missing_count } + @pending_change_requests_count = safely { ProfileChangeRequest.pending.count } + @data_health_flagged = safely { DataHealth.checks.any?(&:flagged?) } + @author_credit_flagged = safely { AuthorCreditDivergenceQuery.new.any? } + @features_unsynced = safely { FeatureCatalog.new.recent_missing? } if allowed_to?(:create?, Feature) end private - # A badge is a nicety, so a failing count renders an empty frame and leaves the button usable. - def render_count_badge(frame_id, title: nil) - count = begin - yield - rescue StandardError => e - Rails.error.report(e) - nil - end - render partial: "count_badge_frame", locals: { frame_id: frame_id, count: count, title: title } + # A badge is a nicety, so a failing check leaves its badge empty and the buttons usable. + def safely + yield + rescue StandardError => e + Rails.error.report(e) + nil end end end diff --git a/app/services/author_credit_divergence_query.rb b/app/services/author_credit_divergence_query.rb index 53ac4a8b71..a0e0188d41 100644 --- a/app/services/author_credit_divergence_query.rb +++ b/app/services/author_credit_divergence_query.rb @@ -67,6 +67,11 @@ def call ) end + # Whether any section has a row, in SQL so even an empty worklist never loads a record. + def any? + authorable_models.any? { |model| unauthored_divergences(model).exists? || preference_divergences(model).exists? } + end + private attr_reader :person_id, :type, :preference, :include_reconciled @@ -101,6 +106,49 @@ def preference_groups group_by_person(records) end + # SQL twin of AuthorCreditable#author_credit_diverged? plus build_group's person rules. + def preference_divergences(model) + table = model.table_name + scope = model.where.not(author_credit_preference: nil) + .joins("INNER JOIN #{Person.table_name} authors ON authors.id = #{table}.author_id") + .where("TRIM(#{table}.author_credit_preference) <> ''") + .where("#{table}.author_credit_preference <> CASE WHEN authors.anonymous_contributions THEN 'anonymous' " \ + "ELSE COALESCE(NULLIF(TRIM(authors.display_name_preference), ''), 'full_name') END") + scope = scope.where(author_credit_preference: preference) if preference + counted_people(scope, :authors) + end + + # Every author-less record lands in exactly one of legacy, creator, or unattributed; + # only the creator section survives a preference or person filter. + def unauthored_divergences(model) + scope = model.where(author_id: nil).joins( + "LEFT JOIN #{User.table_name} creators ON creators.id = #{model.table_name}.created_by_id " \ + "LEFT JOIN #{Person.table_name} creator_people ON creator_people.id = creators.person_id" + ) + legacy = legacy_name_present(model) + creator_fallback = counted_people(scope, :creator_people) + if preference || person_id + return legacy ? creator_fallback.where.not(legacy) : creator_fallback + end + + unattributed_or_creator = scope.where(creator_people: { id: nil }).or(creator_fallback) + legacy ? unattributed_or_creator.or(scope.where(legacy)) : unattributed_or_creator + end + + def legacy_name_present(model) + model.legacy_author_name_columns.map do |qualified_column| + table, column = qualified_column.split(".") + Arel::Nodes::NamedFunction.new("COALESCE", [ Arel::Table.new(table)[column], Arel::Nodes.build_quoted("") ]) + .matches_regexp("[^[:space:]]") + end.reduce(:or) + end + + def counted_people(scope, person_alias) + scope = scope.where.not(person_alias => { id: nil }) + scope = scope.where(person_alias => { id: person_id.to_i }) if person_id.present? + include_reconciled ? scope : scope.where(person_alias => { author_credit_reconciled_at: nil }) + end + # No credited person here, which is the whole problem — so no person filter. def legacy_groups groups = authorable_models.flat_map do |model| diff --git a/app/services/data_health/check.rb b/app/services/data_health/check.rb index e55e317e74..dfe0ebffac 100644 --- a/app/services/data_health/check.rb +++ b/app/services/data_health/check.rb @@ -25,6 +25,9 @@ def count def any? = count.positive? + # Stops at the first wrong row, for callers that only need to know whether there is one. + def flagged? = scope.exists? + def page(number) ordered_scope.paginate(page: number, per_page: PER_PAGE, total_entries: count) end diff --git a/app/services/data_health/unconfigured_app_settings.rb b/app/services/data_health/unconfigured_app_settings.rb index b229748c04..80a0ed39a3 100644 --- a/app/services/data_health/unconfigured_app_settings.rb +++ b/app/services/data_health/unconfigured_app_settings.rb @@ -20,6 +20,8 @@ def count @count ||= missing.any? ? 1 : 0 end + def flagged? = any? + def lists_rows? = false def scope = Setting.none diff --git a/app/services/feature_catalog.rb b/app/services/feature_catalog.rb index a5f9581a8f..82dff2ab3c 100644 --- a/app/services/feature_catalog.rb +++ b/app/services/feature_catalog.rb @@ -6,6 +6,7 @@ # left entirely alone. class FeatureCatalog DATA_DIR = Rails.root.join("db/seeds/features") + RECENT_SEEDS = 20 SYNCED_FIELDS = %i[ area display_status released_on action_path pr_number @@ -49,20 +50,23 @@ def import! Result.new(created, updated, skipped) end - def missing_count - names = entries.map { |entry| entry.fetch("name") } + # Reads only the newest seeds, so a backdated seed merged after a sync waits for a newer one to flag it. + def recent_missing?(limit: RECENT_SEEDS) + names = seed_paths.last(limit).map { |path| load_seed(path).fetch("name") } existing = Feature.where(name: names).pluck(:name).to_set(&:downcase) - names.count { |name| existing.exclude?(name.downcase) } + names.any? { |name| existing.exclude?(name.downcase) } end def entries - Dir[File.join(@dir, "*.yml")].sort.map do |path| - YAML.safe_load_file(path, permitted_classes: [ Date ]) - end + seed_paths.map { |path| load_seed(path) } end private + def seed_paths = Dir[File.join(@dir, "*.yml")].sort + + def load_seed(path) = YAML.safe_load_file(path, permitted_classes: [ Date ]) + # Keyed by downcased name to match the case-insensitive column collation. def existing_features(names) Feature.with_rich_text_rhino_description.where(name: names).index_by { |feature| feature.name.downcase } diff --git a/app/views/admin/home/_action_buttons.html.erb b/app/views/admin/home/_action_buttons.html.erb new file mode 100644 index 0000000000..fc1cc4ef73 --- /dev/null +++ b/app/views/admin/home/_action_buttons.html.erb @@ -0,0 +1,27 @@ +
+ <%= link_to profile_change_requests_path, class: button_classes(:primary_outline) do %> + Changes requested + <%= render "count_badge", count: local_assigns[:pending_change_requests_count] %> + <% end %> + <%= link_to admin_data_health_path, class: button_classes(:primary_outline) do %> + Data health + <%= render "badge", text: (Admin::HomeController::REVIEW_BADGE if local_assigns[:data_health_flagged]), title: Admin::HomeController::DATA_HEALTH_BADGE_TITLE %> + <% end %> + <% if allowed_to?(:import?, EventRegistration) %> + <%= link_to new_event_registration_import_path(return_to: "admin_home"), class: button_classes(:primary_outline) do %> + Import registrants + <% end %> + <% end %> + <%= link_to author_credit_divergences_path, class: button_classes(:primary_outline) do %> + Author credit divergences + <%= render "badge", text: (Admin::HomeController::REVIEW_BADGE if local_assigns[:author_credit_flagged]) %> + <% end %> + <% if allowed_to?(:create?, Feature) %> + <%= button_to import_features_path, + class: button_classes(:primary_outline), + form: { data: { turbo_confirm: "Add any newly-shipped features and fill in missing details? Your existing edits are kept." } } do %> + Sync features & tips + <%= render "badge", text: (Admin::HomeController::REVIEW_BADGE if local_assigns[:features_unsynced]) %> + <% end %> + <% end %> +
diff --git a/app/views/admin/home/_badge.html.erb b/app/views/admin/home/_badge.html.erb new file mode 100644 index 0000000000..2ba299de7d --- /dev/null +++ b/app/views/admin/home/_badge.html.erb @@ -0,0 +1,3 @@ +<% if text.present? %> + <%= tag.span text, class: "bg-primary ms-1 rounded-full px-2 py-0.5 text-xs text-white ring-1 ring-white", title: local_assigns[:title] %> +<% end %> diff --git a/app/views/admin/home/_count_badge.html.erb b/app/views/admin/home/_count_badge.html.erb index 3b2b91cc56..ba2144ad3a 100644 --- a/app/views/admin/home/_count_badge.html.erb +++ b/app/views/admin/home/_count_badge.html.erb @@ -1,3 +1 @@ -<% if count.to_i.positive? %> - <%= tag.span number_with_delimiter(count), class: "bg-primary ms-1 rounded-full px-2 py-0.5 text-xs text-white ring-1 ring-white", title: local_assigns[:title] %> -<% end %> +<%= render "badge", text: (number_with_delimiter(count) if count.to_i.positive?), title: local_assigns[:title] %> diff --git a/app/views/admin/home/_count_badge_frame.html.erb b/app/views/admin/home/_count_badge_frame.html.erb deleted file mode 100644 index 3ace5eae81..0000000000 --- a/app/views/admin/home/_count_badge_frame.html.erb +++ /dev/null @@ -1,3 +0,0 @@ -<%= turbo_frame_tag frame_id do %> - <%= render "count_badge", count: count, title: title %> -<% end %> diff --git a/app/views/admin/home/action_buttons.html.erb b/app/views/admin/home/action_buttons.html.erb new file mode 100644 index 0000000000..972c14abd5 --- /dev/null +++ b/app/views/admin/home/action_buttons.html.erb @@ -0,0 +1,7 @@ +<%= turbo_frame_tag :action_buttons, target: "_top" do %> + <%= render "action_buttons", + pending_change_requests_count: @pending_change_requests_count, + data_health_flagged: @data_health_flagged, + author_credit_flagged: @author_credit_flagged, + features_unsynced: @features_unsynced %> +<% end %> diff --git a/app/views/admin/home/index.html.erb b/app/views/admin/home/index.html.erb index 09bec057d4..530c00881c 100644 --- a/app/views/admin/home/index.html.erb +++ b/app/views/admin/home/index.html.erb @@ -152,33 +152,9 @@
Actions -
- <%= link_to profile_change_requests_path, class: button_classes(:primary_outline) do %> - Changes requested - <%= render "count_badge", count: @pending_change_requests_count %> - <% end %> - <%= link_to admin_data_health_path, class: button_classes(:primary_outline) do %> - Data health - <%= turbo_frame_tag :data_health_count, src: admin_home_data_health_count_path, loading: :lazy %> - <% end %> - <% if allowed_to?(:import?, EventRegistration) %> - <%= link_to new_event_registration_import_path(return_to: "admin_home"), class: button_classes(:primary_outline) do %> - Import registrants - <% end %> - <% end %> - <%= link_to author_credit_divergences_path, class: button_classes(:primary_outline) do %> - Author credit divergences - <%= turbo_frame_tag :author_credit_count, src: admin_home_author_credit_count_path, loading: :lazy %> - <% end %> - <% if allowed_to?(:create?, Feature) %> - <%= button_to import_features_path, - class: button_classes(:primary_outline), - form: { data: { turbo_confirm: "Add any newly-shipped features and fill in missing details? Your existing edits are kept." } } do %> - Sync features & tips - <%= turbo_frame_tag :unsynced_features_count, src: admin_home_unsynced_features_count_path, loading: :lazy %> - <% end %> - <% end %> -
+ <%= turbo_frame_tag :action_buttons, src: admin_home_action_buttons_path, target: "_top" do %> + <%= render "action_buttons" %> + <% end %>
diff --git a/config/routes.rb b/config/routes.rb index 1a6281748f..5fd77b9a13 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -56,9 +56,7 @@ namespace :admin do get "/", to: "home#index" # admin home page - get "home/data_health_count", to: "home#data_health_count", as: "home_data_health_count" - get "home/author_credit_count", to: "home#author_credit_count", as: "home_author_credit_count" - get "home/unsynced_features_count", to: "home#unsynced_features_count", as: "home_unsynced_features_count" + get "home/action_buttons", to: "home#action_buttons", as: "home_action_buttons" get "activities/events", to: "ahoy_activities#index", as: "activities_events" get "activities/events/:id", to: "ahoy_activities#show", as: "activities_event" get "activities/visits", to: "ahoy_activities#visits", as: "activities_visits" diff --git a/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml new file mode 100644 index 0000000000..b025953609 --- /dev/null +++ b/db/seeds/features/2026-10-25-admin-home-clicks-without-waiting-for-badges.yml @@ -0,0 +1,9 @@ +name: "Admin home links work while the action badges load" +area: reporting +display_status: admin_facing +released_on: 2026-10-25 +pr_number: 2847 +summary: >- + Admin home's action badges now load together in the background, so clicks + no longer wait for them. Data health, Author credit, and Sync features & tips + show "Review" when something needs attention. diff --git a/spec/requests/admin/home_spec.rb b/spec/requests/admin/home_spec.rb index c28b71416f..0295a5be96 100644 --- a/spec/requests/admin/home_spec.rb +++ b/spec/requests/admin/home_spec.rb @@ -82,72 +82,71 @@ expect(response.body).to include(%(href="#{organization_statuses_path}")) end - it "loads the action counts in lazy frames" do + it "renders the action buttons right away and fills their badges from one frame request" do + create(:profile_change_request) + get "/admin" - expect(response.body).to include( - %(src="#{admin_home_data_health_count_path}"), - %(src="#{admin_home_author_credit_count_path}"), - %(src="#{admin_home_unsynced_features_count_path}") - ) + expect(response.body.scan(/]* src=/).size).to eq(1) + expect(response.body).to include(%(src="#{admin_home_action_buttons_path}")) + expect(response.body).to include(%(href="#{admin_data_health_path}"), %(href="#{author_credit_divergences_path}")) + expect(response.body[%r{}m]).not_to include("rounded-full") end - describe "count badges" do - it "renders the Data health total inside its frame" do - allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, count: 2), instance_double(DataHealth::Check, count: 3) ]) - - get admin_home_data_health_count_path, headers: { "Turbo-Frame" => "data_health_count" } - - expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="data_health_count")) - expect(response.body).to match(%r{]*>5}) - expect(response.body).to include("Data checks only") + describe "action buttons frame" do + def get_action_buttons + get admin_home_action_buttons_path, headers: { "Turbo-Frame" => "action_buttons" } end - it "renders the Author credit total inside its frame" do - entries = [ 1, 4 ].map { |count| DataHealth::Worklists::Entry.new("Label", count, "record", "/") } - allow_any_instance_of(DataHealth::Worklists).to receive(:author_credit).and_return(entries) - - get admin_home_author_credit_count_path, headers: { "Turbo-Frame" => "author_credit_count" } + def button(label) + response.body[%r{<(?:a|button)[^>]*>(?:(?!).)*#{label}.*?}m] + end - expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="author_credit_count")) - expect(response.body).to match(%r{]*>5}) + before do + allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, flagged?: false), instance_double(DataHealth::Check, flagged?: true) ]) + allow_any_instance_of(AuthorCreditDivergenceQuery).to receive(:any?).and_return(true) + allow_any_instance_of(FeatureCatalog).to receive(:recent_missing?).and_return(true) end - it "renders how many seeded features aren't synced yet" do - allow_any_instance_of(FeatureCatalog).to receive(:missing_count).and_return(3) + it "shows Review on Data health, Author credit, and Sync features, and the pending change request count" do + create_list(:profile_change_request, 2) - get admin_home_unsynced_features_count_path, headers: { "Turbo-Frame" => "unsynced_features_count" } + get_action_buttons expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="unsynced_features_count")) - expect(response.body).to match(%r{]*>3}) + expect(response.body).to include(%(id="action_buttons"), %(target="_top")) + expect(response.body).not_to include("]*>2}) + expect(button("Data health")).to match(%r{]*>Review}).and include("Data checks only") + expect(button("Author credit divergences")).to match(%r{]*>Review}) + expect(button("Sync features")).to match(%r{]*>Review}) end it "renders no badge when nothing is waiting" do - allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, count: 0) ]) + allow(DataHealth).to receive(:checks).and_return([ instance_double(DataHealth::Check, flagged?: false) ]) + allow_any_instance_of(AuthorCreditDivergenceQuery).to receive(:any?).and_return(false) + allow_any_instance_of(FeatureCatalog).to receive(:recent_missing?).and_return(false) - get admin_home_data_health_count_path, headers: { "Turbo-Frame" => "data_health_count" } + get_action_buttons - expect(response.body).to include(%(id="data_health_count")) + expect(response.body).to include("Data health", "Author credit divergences", "Sync features") expect(response.body).not_to include("rounded-full") end - it "renders an empty frame when a count fails" do + it "keeps the buttons and leaves only the failing badge empty when a check fails" do allow(DataHealth).to receive(:checks).and_raise(StandardError, "boom") - get admin_home_data_health_count_path, headers: { "Turbo-Frame" => "data_health_count" } + get_action_buttons expect(response).to have_http_status(:ok) - expect(response.body).to include(%(id="data_health_count")) - expect(response.body).not_to include("rounded-full") + expect(button("Data health")).not_to include("rounded-full") + expect(button("Author credit divergences")).to match(%r{]*>Review}) end it "denies a non-admin" do sign_in create(:user) - get admin_home_data_health_count_path, headers: { "Turbo-Frame" => "data_health_count" } + get_action_buttons expect(response).not_to have_http_status(:ok) end diff --git a/spec/services/author_credit_divergence_query_spec.rb b/spec/services/author_credit_divergence_query_spec.rb index e362488b54..e8ff047aed 100644 --- a/spec/services/author_credit_divergence_query_spec.rb +++ b/spec/services/author_credit_divergence_query_spec.rb @@ -69,6 +69,98 @@ def diverged_story end end + describe "#any?" do + let(:personless_user) { create(:user, person: nil) } + + def expect_any_to_match(query) + expect(query.any?).to eq(!query.call.empty?) + end + + it "is false when every record credits its author cleanly" do + person.update!(display_name_preference: "full_name") + create(:story, created_by: author_user, author: person, author_credit_preference: nil) + + expect(described_class.new(type: "Story").any?).to be(false) + expect_any_to_match(described_class.new(type: "Story")) + end + + it "is true for a diverged snapshot" do + diverged_story + + expect(described_class.new.any?).to be(true) + expect_any_to_match(described_class.new) + end + + it "is true for a legacy name, a creator fallback, and an unattributed record" do + [ + -> { create(:workshop, created_by: author_user, author: nil, full_name: "Legacy Name") }, + -> { create(:story, created_by: author_user, author: nil) }, + -> { create(:story, created_by: personless_user, author: nil) } + ].each do |make| + record = make.call + + expect(described_class.new.any?).to be(true) + expect_any_to_match(described_class.new) + record.destroy! + end + end + + it "is false when the only creator fallback belongs to a reconciled person" do + create(:story, created_by: author_user, author: nil) + person.update!(author_credit_reconciled_at: Time.current) + + expect(described_class.new(type: "Story").any?).to be(false) + expect(described_class.new(type: "Story", include_reconciled: "1").any?).to be(true) + expect_any_to_match(described_class.new(type: "Story")) + end + + it "treats a whitespace-only legacy name as no legacy name" do + create(:workshop, created_by: author_user, author: nil, full_name: " ") + person.update!(author_credit_reconciled_at: Time.current) + + expect(described_class.new(type: "Workshop").any?).to be(false) + expect_any_to_match(described_class.new(type: "Workshop")) + end + + it "compares a snapshot against an anonymous or blank-preference author" do + story = create(:story, created_by: author_user, author: person) + person.update_columns(anonymous_contributions: true) + story.update_columns(author_credit_preference: "anonymous") + expect(described_class.new(type: "Story").any?).to be(false) + + story.update_columns(author_credit_preference: "full_name") + expect(described_class.new(type: "Story").any?).to be(true) + + person.update_columns(anonymous_contributions: false, display_name_preference: "") + expect(described_class.new(type: "Story").any?).to be(false) + expect_any_to_match(described_class.new(type: "Story")) + end + + it "agrees with the full query under every filter" do + diverged_story + create(:story, created_by: personless_user, author: nil) + create(:workshop, created_by: personless_user, author: nil, full_name: "Legacy Name") + reconciled = create(:user, :with_person) + reconciled.person.update!(author_credit_reconciled_at: Time.current) + create(:story, created_by: reconciled, author: nil) + + [ {}, { type: "Story" }, { type: "Resource" }, { person_id: person.id }, { person_id: reconciled.person.id }, + { person_id: reconciled.person.id, include_reconciled: "1" }, { preference: "first_name_only" }, + { preference: "anonymous" }, { include_reconciled: "1" } ].each do |filters| + query = described_class.new(**filters) + expect(query.any?).to eq(!query.call.empty?), "disagrees for #{filters}" + end + end + + it "honors the person filter" do + create(:story, created_by: personless_user, author: nil) + diverged_story + + expect(described_class.new(person_id: person.id + 9999).any?).to be(false) + expect(described_class.new(person_id: person.id).any?).to be(true) + end + end + describe "filters" do before { diverged_story } diff --git a/spec/services/data_health/checks_spec.rb b/spec/services/data_health/checks_spec.rb index b6c404335d..c5f4627fd3 100644 --- a/spec/services/data_health/checks_spec.rb +++ b/spec/services/data_health/checks_spec.rb @@ -25,6 +25,14 @@ end end + it "flags exactly the checks that count rows" do + create(:story) + + described_class.checks.each do |check| + expect(check.flagged?).to eq(check.any?), "#{check.key} flags #{check.flagged?} but counts #{check.count}" + end + end + it "keeps every check's scope a relation, so counting doesn't load the table" do described_class.checks.each do |check| expect(check.scope).to be_a(ActiveRecord::Relation), "#{check.key} returned #{check.scope.class}" diff --git a/spec/services/feature_catalog_spec.rb b/spec/services/feature_catalog_spec.rb index ead803231d..5537bac6c9 100644 --- a/spec/services/feature_catalog_spec.rb +++ b/spec/services/feature_catalog_spec.rb @@ -143,17 +143,17 @@ end end - describe "#missing_count" do + describe "#recent_missing?" do let(:seed_dir) { Pathname(Dir.mktmpdir("features_seed")) } before do - %w[One Two].each do |label| - seed_dir.join("2026-08-01-seed-#{label.downcase}.yml").write(<<~YAML) + %w[One Two Three].each_with_index do |label, index| + seed_dir.join("2026-08-0#{index + 1}-seed-#{label.downcase}.yml").write(<<~YAML) name: "Seed feature #{label}" area: events display_status: user_facing summary: "Seeded." - released_on: 2026-08-01 + released_on: 2026-08-0#{index + 1} YAML end end @@ -162,16 +162,25 @@ subject(:catalog) { described_class.new(dir: seed_dir) } - it "counts seeds with no feature of that name yet, ignoring case" do + it "is true while a recent seed has no feature of that name yet, ignoring case" do create(:feature, name: "seed feature one") + create(:feature, name: "seed feature two") - expect(catalog.missing_count).to eq(1) + expect(catalog.recent_missing?).to be(true) end - it "is zero once everything is synced" do + it "is false once everything is synced" do catalog.import! - expect(catalog.missing_count).to eq(0) + expect(catalog.recent_missing?).to be(false) + end + + it "looks only at the newest seeds" do + create(:feature, name: "Seed feature Two") + create(:feature, name: "Seed feature Three") + + expect(catalog.recent_missing?(limit: 2)).to be(false) + expect(catalog.recent_missing?(limit: 3)).to be(true) end end