Add Apply All button for rules with confirmation #73
Conversation
jjmata
added
the
codex
ChatGPT Codex Connector
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jjmata, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new feature that allows users to apply all existing rules at once. It provides a user interface with a confirmation dialog and implements the necessary backend logic and routing to activate and apply all rules, ensuring a streamlined bulk operation.
Highlights
- New Feature: Apply All Rules: Introduced a new 'Apply all' button on the rules index page, allowing users to activate and apply all existing rules in a single action.
- Confirmation Dialog: Added a modal confirmation dialog that displays the total number of affected resources before proceeding with the bulk rule application, enhancing user awareness and preventing accidental operations.
- Backend Logic and Routing: Implemented new controller actions (
confirm_allandapply_all) and corresponding routes to handle the confirmation flow and the asynchronous application of all rules, ensuring each rule is activated and processed via a background job. - Test Coverage: Added comprehensive integration tests for the new
confirm_allandapply_allactions, verifying correct responses, job enqueuing, and the successful activation of rules.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request adds functionality to apply all rules at once, including a confirmation step. The changes span the controller, views, routes, and tests. My review focuses on improving performance by addressing N+1 queries in the controller, enhancing user-facing text in the view, and making the new tests more robust.
| rule = rules(:one) | ||
| rule.update!(active: false) | ||
|
|
||
| assert_enqueued_jobs 1, only: RuleJob do | ||
| post apply_all_rules_url | ||
| end | ||
|
|
||
| assert_redirected_to rules_url | ||
| assert rule.reload.active |
There was a problem hiding this comment.
The test should apply all rules is brittle because it asserts that exactly one job is enqueued. This makes the test dependent on the fixture data having only one rule for the test user's family. If fixtures change, this test could easily break. The PR description mentions failing tests, and this might be related.
A more robust approach is to make the test independent of the number of rules in the fixtures. You can do this by counting the rules for the family at runtime and asserting against that count.
rules = @user.family.rules
rules.update_all(active: false)
assert_enqueued_jobs rules.count, only: RuleJob do
post apply_all_rules_url
end
assert_redirected_to rules_url
assert_equal rules.count, @user.family.rules.where(active: true).count| @rules = Current.family.rules | ||
| @affected_resource_count = @rules.sum(&:affected_resource_count) |
There was a problem hiding this comment.
The current implementation of confirm_all can lead to an N+1 query issue. The line @rules.sum(&:affected_resource_count) iterates over each rule and executes a separate database query for each one to calculate affected_resource_count. This can be inefficient, especially with a large number of rules.
To mitigate this, you can eager-load the conditions and sub_conditions associations, which are used within affected_resource_count. Additionally, the @rules instance variable is not used in the view, so it can be a local variable.
rules = Current.family.rules.includes(conditions: :sub_conditions)
@affected_resource_count = rules.sum(&:affected_resource_count)| Current.family.rules.find_each do |rule| | ||
| rule.update!(active: true) | ||
| rule.apply_later(ignore_attribute_locks: true) | ||
| end |
There was a problem hiding this comment.
The apply_all action updates each rule individually within the find_each block. This results in N separate UPDATE queries to the database, which is inefficient for a large number of rules.
You can significantly improve performance by using update_all to activate all rules in a single database query, and then iterate through them to enqueue the background jobs.
rules_to_apply = Current.family.rules
rules_to_apply.update_all(active: true)
rules_to_apply.find_each do |rule|
rule.apply_later(ignore_attribute_locks: true)
endThere was a problem hiding this comment.
Want to test/improve this with your ~30 rules, @steadyfall?
There was a problem hiding this comment.
Sure, I'll test it out and let you know the results.
app/views/rules/confirm_all.html.erb
Outdated
| <% dialog.with_body do %> | ||
| <p class="text-secondary text-sm mb-4"> | ||
| You are about to apply all rules to | ||
| <span class="text-primary font-medium"><%= @affected_resource_count %> resources</span>. |
There was a problem hiding this comment.
The text displays "X resources", which will be grammatically incorrect when there is only one affected resource (e.g., "1 resources"). You should use the pluralize helper to handle this gracefully.
<span class="text-primary font-medium"><%= pluralize(@affected_resource_count, "resource") %></span>
|
Maybe @vinteo can take a look here as well, help us debug that dismiss bug @steadyfall ran into? 🙏 |
|
Would be good to add a test for it before we fix it, BTW! |
|
Would be nice for self-hosting to also be able to run these as |
Bug has been documented in #84. |
- Replace 'resources' with 'transactions' in confirmation dialog - Move 'Apply all' action into dropdown menu - Rename 'Delete all rules' to 'Delete all' for brevity
Summary
Testing
npm run lintbin/rubocop -f github -abundle exec erb_lint ./app/**/*.erb -abin/brakeman --no-pagerbin/rails test(fails: There is an issue connecting with your hostname: 127.0.0.1.)https://chatgpt.com/codex/tasks/task_e_68914401cf8c8332b859e7bbb6d88ef6