Batch update confirmation dialog - #2073
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances ActiveAdmin scoped collection batch actions by adding a post-submit confirmation dialog for batch updates, introducing datetime support (UI + validations) for batch update forms, and adding an “expired” scope for routing destinations.
Changes:
- Enable submit confirmation for “Update batch” and add a confirmation summary for “Delete batch”.
- Switch batch update
valid_from/valid_tillfrom date to datetime, including frontend picker wiring and updated validations/specs. - Add
expiredscope forRouting::Destinationplus feature coverage for the expired scope listing.
Reviewed changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/support/helpers/feature_test_helper.rb | Adds a helper to confirm the post-submit batch update dialog in feature specs. |
| spec/models/batch_update_form/dialpeer_spec.rb | Updates validation specs for datetime-based valid_from/valid_till behavior. |
| spec/models/batch_update_form/destination_spec.rb | Updates validation specs for datetime-based valid_from/valid_till behavior. |
| spec/features/routing/routing_plans/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/routing/routing_plan_static_routes/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/routing/routing_plan_lnp_rules/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/routing/numberlists/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/routing/dialpeers/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog and updates datetime inputs. |
| spec/features/routing/destinations/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog and updates datetime inputs. |
| spec/features/routing/customers_auths/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/equipment/gateways/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/equipment/gateway_groups/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/billing/contractors/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/billing/contacts/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| spec/features/billing/accounts/batch_update_spec.rb | Adjusts batch update flow to confirm the new post-submit dialog. |
| lib/resource_dsl/acts_as_async_update.rb | Enables confirm_submit for async batch update action. |
| lib/resource_dsl/acts_as_async_destroy.rb | Adds confirmation summary for async batch destroy action. |
| Gemfile | Pins active_admin_scoped_collection_actions to a GitHub branch for new confirmation behavior. |
| Gemfile.lock | Locks the git-sourced active_admin_scoped_collection_actions dependency. |
| app/models/routing/destination.rb | Adds expired scope for destinations. |
| app/forms/batch_update_form/dialpeer.rb | Switches valid_from/valid_till to datetime and uses validates_datetime. |
| app/forms/batch_update_form/destination.rb | Switches valid_from/valid_till to datetime and uses validates_datetime. |
| app/forms/batch_update_form/base.rb | Adds form metadata for datetime fields (plain input + class hook). |
| app/assets/javascripts/batch_update_datetimepicker.js | Hooks datetime picker initialization into the batch update modal lifecycle. |
| app/assets/javascripts/active_admin.js | Includes the new datetime picker batch update initializer. |
| app/admin/routing/destinations.rb | Exposes the new expired scope in the ActiveAdmin UI. |
| spec/features/routing/destinations/index_destination_spec.rb | Adds feature coverage for the destinations “expired” scope view. |
Comments suppressed due to low confidence (2)
spec/models/batch_update_form/dialpeer_spec.rb:240
- Same brittleness as above: this assertion hard-codes the formatted datetime instead of deriving it from
assign_params[:valid_till], so the test won’t automatically track changes to the input value/format.
it 'should have error:' do
subject
expect(subject.errors.to_a).to contain_exactly 'Valid from must be before or equal to 2020-01-01 10:30:00'
end
spec/models/batch_update_form/destination_spec.rb:126
- Same as above: hard-coding the formatted datetime here makes the spec easy to desync from
assign_params. Consider deriving the expected string from the input.
it 'should have error: :valid_from must be before or equal to' do
subject
expect(subject.errors.to_a).to contain_exactly 'Valid from must be before or equal to 2020-01-01 10:30:00'
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Build date time picker for datetime fields on update batch action form | ||
|
|
||
| // modal dialog pulls focus back into itself, so picker popup must be whitelisted, | ||
| // same as jquery ui does it for its own datepicker | ||
| var dialogAllowInteraction = $.ui.dialog.prototype._allowInteraction; | ||
| $.ui.dialog.prototype._allowInteraction = function (event) { | ||
| return dialogAllowInteraction.call(this, event) || !!$(event.target).closest('.xdsoft_datetimepicker').length; | ||
| }; | ||
|
|
||
| $(document).on('mass_update_modal_dialog:after_open', function (event, form) { | ||
| setupDateTimePicker(form); | ||
| }); |
| it 'should have error:' do | ||
| subject | ||
| expect(subject.errors.to_a).to contain_exactly "Valid from must be before or equal to #{assign_params[:valid_till]}" | ||
| expect(subject.errors.to_a).to contain_exactly 'Valid from must be before or equal to 2020-01-01 00:00:00' | ||
| end |
| it 'should have error: :valid_from must be before or equal to' do | ||
| subject | ||
| expect(subject.errors.to_a).to contain_exactly "Valid from must be before or equal to #{assign_params[:valid_till]}" | ||
| expect(subject.errors.to_a).to contain_exactly 'Valid from must be before or equal to 2020-01-01 00:00:00' | ||
| end |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
app/assets/javascripts/batch_update_datetimepicker.js:7
- This script assumes
$.ui.dialogand the globalsetupDateTimePickeralways exist. If either is missing on a page (or load order changes), it will raise and can break all ActiveAdmin JS.
Add lightweight guards before monkey-patching _allowInteraction and before calling setupDateTimePicker.
var dialogAllowInteraction = $.ui.dialog.prototype._allowInteraction;
$.ui.dialog.prototype._allowInteraction = function (event) {
return dialogAllowInteraction.call(this, event) || !!$(event.target).closest('.xdsoft_datetimepicker').length;
app/models/routing/destination.rb:77
- New
:expiredscope uses a database-specificNOW()call and embeds the time source in SQL. In this model the existing:time_validscope uses a bound parameter (:time), which is easier to test and keeps time zone behavior consistent with the Rails app time.
Consider rewriting :expired to use a bound timestamp (e.g. Time.current) instead of NOW().
scope :low_quality, -> { where quality_alarm: true }
scope :time_valid, -> { where('valid_till >= :time AND valid_from < :time', time: Time.now) }
scope :expired, -> { where('valid_till < NOW()') }
spec/features/routing/destinations/index_destination_spec.rb:28
- This test data uses a very small time margin (
valid_till: 1.second.ago) while the new:expiredscope compares against the database clock. That can make the spec flaky on slower CI or if DB/app clocks drift slightly.
Use a larger buffer for expired/non-expired records to keep the scope assertion stable.
create_list(:destination, 3, valid_from: 1.day.ago, valid_till: 1.second.ago)
end
before do
create(:destination, valid_from: 1.day.ago, valid_till: 1.minute.from_now)
No description provided.