Repository navigation
show summary information on confirmation modal - #55
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The preview can miscount scoped records, permit confirmation after a failed count, or describe a checkbox change that is not submitted.
Review effort: Balanced
Findings: 1
What changed in this PR
This PR adds affected-record summaries to scoped collection action dialogs, helping administrators review a batch action before confirming it.
Changes:
- Adds record counts, loading indicators, and an optional second confirmation step.
- Supports custom classes on form inputs and documents the new options.
- Adds translations and feature specs for the new dialogs.
| File | Description |
|---|---|
| vendor/assets/stylesheets/active_admin_scoped_collection_actions.scss | Styles summaries, spinners, and confirmation controls. |
| vendor/assets/javascripts/lib/dialog_records_summary.js | Loads and displays record counts. |
| vendor/assets/javascripts/lib/dialog_mass_fields_update.js | Builds the field-change preview. |
| vendor/assets/javascripts/lib/dialog_confirm_submit.js | Adds the second confirmation step. |
| vendor/assets/javascripts/active_admin_scoped_collection_actions.js | Connects dialogs to count and action requests. |
| spec/support/admin.rb | Configures example actions. |
| spec/posts_actions_spec.rb | Tests update confirmation and counts. |
| spec/authors_actions_spec.rb | Tests delete summaries and custom inputs. |
| README.md | Documents the new options and inputs. |
| lib/active_admin_scoped_collection_actions/resource_extension.rb | Passes confirmation settings to the UI. |
| lib/active_admin_scoped_collection_actions/dsl.rb | Adds the record-count action. |
| config/locales/sv.yml | Adds Swedish summary text. |
| config/locales/es.yml | Adds Spanish summary text. |
| config/locales/en.yml | Adds English summary text. |
| config/locales/de.yml | Adds German summary text. |
| CHANGELOG.md | Records the new features. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The :scoped_collection_records_count batch action has no authorized? guard and its comment claims the authorization scope makes one unnecessary. Cover both halves: the scope is applied to the count, and the count is served to a user the action itself refuses.
Every existing spec reads the summary of an unfiltered collection, so the number itself was never asserted. Cover the current filter, a checked subset and a scoped_collection that already carries order and limit, which is what the .except in the count action strips off.
Both options take true/false, a String or a Proc, and only one form of each was exercised. Adds the String and Proc forms of :confirm_summary, the boolean fallback of :confirm_submit, and the branch most likely to regress: a Proc returning false has to leave the data attribute absent, not present and empty.
confirm_summary: true picks confirm_destroy_summary for :scoped_collection_destroy and confirm_action_summary for anything else. Only the destroy side of that ternary was exercised.
Pins that the number replaces the placeholder with nothing left over, and that
a summary without a {count} at all still renders and still lets the action be
confirmed.
Catches the next key that lands in en.yml only.
add_scoped_collection_records_count_action runs on every scoped_collection_action call, four times over for the authors resource. Pins that this leaves a single batch action and that it is never offered in the batch actions dropdown.
|
Pushed 7 test-only commits on top of The specs added, in order:
Plus Authorization finding
So the cardinality of a collection leaks to a user who may not act on it. Whether that matters is your call — deliberately fine for most apps, less so if row counts are themselves sensitive. I have not added a guard: that is a behaviour change for you to make, not something to slip in under a test. The specs pin today's behaviour and will go red if you add one, which is the right kind of red. Unrelated pre-existing bug spotted
CI still does not run on this PRThe push updated the PR head to Nothing a push can fix — it needs either that repo setting changed (Settings > Actions > General > Fork pull request workflows) or the branch mirrored into this repo. Until then the matrix below is all the signal there is. VerificationEvery new spec was checked against broken code: I reverted or flipped the line it covers, confirmed it went red, then restored. The one that caught me out was the "no summary" example — Full suite, 36 examples, 0 failures throughout:
That is every Rails in this branch's matrix (7.1, 7.2, 8.0) plus 8.1, and the oldest and newest ActiveAdmin in it. No order dependence across the seven seeds. Two notes from running it locally, neither caused by this branch:
Finally: this branch currently conflicts with |
…ummary # Conflicts: # CHANGELOG.md
Four real captures of the dummy app: the sidebar, the dialog while the count is still in flight, the same dialog with the summary resolved, and the result after confirming.
The repo already used screenshots/ for the two README images; a second directory for the same purpose is one more place to look.
The count action stripped :limit and :offset before counting, on the stated reasoning that the action goes on to touch the collection in full. Measured on ActiveRecord 8.0.5.1, it does not: find_each honours both. scope .limit(1) over 5 rows find_each touches 1, stripped count said 5 scope .offset(3) over 5 rows find_each touches 2, stripped count said 5 So the dialog promised records the action would never reach. A grouped scope was worse -- .count answers with a Hash of per-group tallies, which reaches the dialog as a JSON object and renders as [object Object]. Count through a subquery instead. Every clause survives, a grouped relation collapses to the number of rows it yields, and the answer is always a single number. Verified against find_each across plain, limit, offset, limit+offset, group, where and order+limit scopes: the two agree in every case. The spec that pinned the old behaviour asserted 3 for a .limit(1) scope. It now asserts 1, and a grouped resource covers the Hash case. Both fail on the previous implementation.
| # with a Hash the dialog would render as [object Object] -- into the | ||
| # single number the dialog needs. | ||
| scope = scoped_collection_records.except(:eager_load, :select, :order) | ||
| count = scope.model.unscoped.from(scope.select('1'), :subquery_for_count).count |
| # clause and collapses a grouped scope -- whose bare .count answers | ||
| # with a Hash the dialog would render as [object Object] -- into the | ||
| # single number the dialog needs. | ||
| scope = scoped_collection_records.except(:eager_load, :select, :order) |
| confirm_submit = confirm_submit.call if confirm_submit.is_a?(Proc) | ||
| if confirm_submit | ||
| b_data[:confirm_submit] = confirm_submit.is_a?(String) ? confirm_submit : I18n.t('active_admin_scoped_collection_actions.confirm_action_message') | ||
| b_data[:confirm_submit_summary] = I18n.t('active_admin_scoped_collection_actions.confirm_submit_summary') |
The comment above add_grouped_author_resource described order+limit and pointed at /admin/ordered_authors -- it belonged to the neighbouring helper. It was also stale on its own terms: the count action no longer strips limit, only eager_load, select and order. Each helper now describes the relation it actually registers.
No description provided.