feat(settings): add reviewed bulk cleanup flows#1754
Conversation
superagent-security
Bot
added
contributor:verified
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes domain extraction and Brandfetch logo generation; adds bulk account-domain and family-merchant-website management with modal UIs; introduces Category::Merger and merchant-merge refactors; delegates logo generation in models; adds Stimulus form sync, routes, locales, views, and comprehensive tests. ChangesAll Changes (single cohesive cohort)
Sequence Diagram(s)sequenceDiagram
participant Client
participant FamilyMerchantsController
participant Merchant
participant DB
Client->>FamilyMerchantsController: POST bulk_update_websites(params: website_url, merchant_ids)
FamilyMerchantsController->>Merchant: extract_domain(website_url)
Merchant-->>FamilyMerchantsController: normalized_domain or nil
FamilyMerchantsController->>DB: transaction begin
FamilyMerchantsController->>DB: update website_url for selected merchants
FamilyMerchantsController->>Merchant: brandfetch_logo_url_for(website_url) for ProviderMerchant
Merchant-->>FamilyMerchantsController: brandfetch_url or nil
FamilyMerchantsController->>DB: update logo_url
FamilyMerchantsController->>DB: transaction commit
FamilyMerchantsController-->>Client: redirect with success or alert
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b611165bf1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/controllers/family_merchants_controller_test.rb (1)
287-315: ⚡ Quick winMove the Brandfetch-clearing checks into merchant model tests.
These two cases never hit
FamilyMerchantsController; they exerciseProviderMerchant/FamilyMerchantbehavior directly. Keeping them here makes the controller suite own model behavior and obscures which layer actually regressed.As per coding guidelines, "Never test the implementation details of one class in another class's test suite" and "
test/**/*_test.rb: Tests must mirrorapp/structure."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/family_merchants_controller_test.rb` around lines 287 - 315, These two tests in family_merchants_controller_test.rb are exercising ProviderMerchant and FamilyMerchant model behavior (e.g., ProviderMerchant.create!, FamilyMerchant.create!, ProviderMerchant#generate_logo_url_from_website!, and Brandfetch setting stubs) and should be moved into the respective model test files; move the provider-merchant test into test/models/provider_merchant_test.rb and the family-merchant test into test/models/family_merchant_test.rb (or the existing model test files), keep the same setup/assertions and the Setting.stubs(:brand_fetch_client_id).returns(nil) lines, and remove them from family_merchants_controller_test.rb so controller tests only cover controller behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/controllers/family_merchants_controller.rb`:
- Around line 119-123: Update the validation logic in FamilyMerchantsController
around where you call Merchant.extract_domain(permitted_params[:website_url]) so
malformed domains are rejected with a specific error: first check if
permitted_params[:website_url].present? && website_url.blank? and in that case
redirect_to bulk_websites_family_merchants_path with alert:
t(".invalid_domain"); otherwise keep the original selection check and
redirect_to bulk_websites_family_merchants_path with alert:
t(".invalid_selection") when merchants.empty? or website_url.blank?; apply the
same change to the analogous block at lines 133-135. Ensure you reference
Merchant.extract_domain, permitted_params[:website_url],
bulk_websites_family_merchants_path, t(".invalid_domain") and
t(".invalid_selection") when making the edits.
- Around line 190-192: The method conflicting_merge_target? should treat any
populated new_target_* field as mutually exclusive with target_id; update it to
return true when permitted_params[:target_id].present? and any key in
permitted_params whose name starts with "new_target_" has a present/non-blank
value. In other words, replace the single-key check (new_target_name) with a
scan like permitted_params.any? { |k, v| k.to_s.start_with?("new_target_") &&
v.present? } so symbol or string keys and all new_target_* fields are
considered.
In `@app/models/merchant.rb`:
- Line 18: The host normalization currently uses domain =
URI.parse(normalized_url).host&.sub(/\Awww\./, "") which is case-sensitive and
fails to strip uppercase "WWW."; update that call to use a case-insensitive
regex (e.g. sub(/\Awww\./i, "")) so it matches "www" in any case and keep
behavior consistent with sanitized_domain_from; also add a unit test in
test/models/merchant_test.rb asserting an input like "https://WWW.example.com"
yields "example.com".
---
Nitpick comments:
In `@test/controllers/family_merchants_controller_test.rb`:
- Around line 287-315: These two tests in family_merchants_controller_test.rb
are exercising ProviderMerchant and FamilyMerchant model behavior (e.g.,
ProviderMerchant.create!, FamilyMerchant.create!,
ProviderMerchant#generate_logo_url_from_website!, and Brandfetch setting stubs)
and should be moved into the respective model test files; move the
provider-merchant test into test/models/provider_merchant_test.rb and the
family-merchant test into test/models/family_merchant_test.rb (or the existing
model test files), keep the same setup/assertions and the
Setting.stubs(:brand_fetch_client_id).returns(nil) lines, and remove them from
family_merchants_controller_test.rb so controller tests only cover controller
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a22a426f-db33-40be-8a17-e0b77cf86247
📒 Files selected for processing (26)
app/controllers/accounts_controller.rbapp/controllers/categories_controller.rbapp/controllers/family_merchants_controller.rbapp/javascript/controllers/merchant_merge_target_controller.jsapp/models/account.rbapp/models/category/merger.rbapp/models/family_merchant.rbapp/models/merchant.rbapp/models/merchant/merger.rbapp/models/provider_merchant.rbapp/views/accounts/bulk_domains.html.erbapp/views/accounts/index.html.erbapp/views/categories/index.html.erbapp/views/categories/merge.html.erbapp/views/family_merchants/bulk_websites.html.erbapp/views/family_merchants/index.html.erbapp/views/family_merchants/merge.html.erbconfig/locales/views/accounts/en.ymlconfig/locales/views/categories/en.ymlconfig/locales/views/merchants/en.ymlconfig/routes.rbtest/controllers/accounts_controller_test.rbtest/controllers/categories_controller_test.rbtest/controllers/family_merchants_controller_test.rbtest/models/account_test.rbtest/models/merchant_test.rb
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/controllers/accounts_controller_test.rb (1)
41-51: ⚡ Quick winConsider verifying flash message for consistency.
This test asserts the redirect behavior when
update!raisesRecordInvalid, but unlike the test at lines 53-64, it doesn't verify thatflash[:alert]orflash[:notice]is set. If the controller is expected to set a flash message for validation errors (as it does for domain validation errors), add an assertion here for completeness.💡 Suggested assertion
assert_redirected_to bulk_domains_accounts_path + assert_not_nil flash[:alert] end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/controllers/accounts_controller_test.rb` around lines 41 - 51, The test "bulk domain update redirects when an account update fails validation" currently only asserts the redirect; add an assertion to verify the controller sets the expected flash message (e.g., assert_equal or assert_match on flash[:alert] or flash[:notice]) when Account.any_instance.stubs(:update!).raises(ActiveRecord::RecordInvalid.new(`@account`)); ensure the added assertion matches the same flash key and message style used in the companion test that handles domain validation errors so behavior is consistent with bulk_update_domains_accounts_path -> bulk_domains_accounts_path flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/controllers/accounts_controller_test.rb`:
- Around line 41-51: The test "bulk domain update redirects when an account
update fails validation" currently only asserts the redirect; add an assertion
to verify the controller sets the expected flash message (e.g., assert_equal or
assert_match on flash[:alert] or flash[:notice]) when
Account.any_instance.stubs(:update!).raises(ActiveRecord::RecordInvalid.new(`@account`));
ensure the added assertion matches the same flash key and message style used in
the companion test that handles domain validation errors so behavior is
consistent with bulk_update_domains_accounts_path -> bulk_domains_accounts_path
flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdda2638-e794-40e1-b56f-aa4832eac58e
📒 Files selected for processing (12)
app/controllers/accounts_controller.rbapp/controllers/categories_controller.rbapp/controllers/family_merchants_controller.rbapp/models/merchant.rbconfig/locales/views/accounts/en.ymlconfig/locales/views/merchants/en.ymltest/controllers/accounts_controller_test.rbtest/controllers/categories_controller_test.rbtest/controllers/family_merchants_controller_test.rbtest/models/family_merchant_test.rbtest/models/merchant_test.rbtest/models/provider_merchant_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/models/provider_merchant_test.rb
🚧 Files skipped from review as they are similar to previous changes (8)
- config/locales/views/merchants/en.yml
- app/models/merchant.rb
- config/locales/views/accounts/en.yml
- app/controllers/categories_controller.rb
- test/controllers/categories_controller_test.rb
- app/controllers/accounts_controller.rb
- app/controllers/family_merchants_controller.rb
- test/models/merchant_test.rb
|
Good set of features — the reviewed-before-applying approach is the right call for destructive bulk ops. A couple of things to flag: Gemfile / Gemfile.lock changes should be scoped to this PR's intent
Generated by Claude Code |
Reject non-public domain inputs for account and merchant website cleanup, and make the merchant merge target controller clear the inactive target mode deterministically.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/models/merchant_test.rb (1)
38-57: ⚡ Quick winConsider adding a test for nil domain handling.
The
extract_domaintests demonstrate that the method can returnnil(lines 16-32). Consider adding a test to verifybrandfetch_logo_url_forhandles this case gracefully—either by returningnil, a fallback URL, or raising a clear error.📝 Suggested test addition
test "brandfetch_logo_url_for handles nil domain" do Setting.stubs(:brand_fetch_client_id).returns("test-client") Merchant.stubs(:extract_domain).returns(nil) logo_url = Merchant.brandfetch_logo_url_for("invalid-url") # Assert expected behavior: nil, fallback, or error assert_nil logo_url end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/models/merchant_test.rb` around lines 38 - 57, Add a test covering the case where Merchant.extract_domain returns nil: stub Setting.brand_fetch_client_id and Setting.brand_fetch_logo_size as needed, stub Merchant.extract_domain to return nil, call Merchant.brandfetch_logo_url_for with an invalid URL (e.g., "invalid-url") and assert the expected behavior (for this repo assert_nil logo_url) to ensure brandfetch_logo_url_for handles nil domains gracefully.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/models/merchant_test.rb`:
- Around line 38-57: Add a test covering the case where Merchant.extract_domain
returns nil: stub Setting.brand_fetch_client_id and
Setting.brand_fetch_logo_size as needed, stub Merchant.extract_domain to return
nil, call Merchant.brandfetch_logo_url_for with an invalid URL (e.g.,
"invalid-url") and assert the expected behavior (for this repo assert_nil
logo_url) to ensure brandfetch_logo_url_for handles nil domains gracefully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5c554dbc-901a-4f5d-bc14-67552a6d817f
📒 Files selected for processing (4)
app/javascript/controllers/merchant_merge_target_controller.jsapp/models/merchant.rbtest/models/account_test.rbtest/models/merchant_test.rb
✅ Files skipped from review due to trivial changes (1)
- test/models/account_test.rb
🚧 Files skipped from review as they are similar to previous changes (2)
- app/models/merchant.rb
- app/javascript/controllers/merchant_merge_target_controller.js
Add regression coverage for Brandfetch logo generation when domain extraction fails.
Thanks, should be addressed now.
Validated locally in the devcontainer with focused Rails tests, Biome, scoped RuboCop, Brakeman/Codex Security, and CodeRabbit. |
2 participants
Supersedes #1630. The original PR head was on the old
JSONbored/surefork network; this replacement branch lives onJSONbored/sure-upstream, the current fork ofwe-promise/sure.Summary
Feature objective
Large imported or synced datasets can create duplicate merchants/categories and incomplete account/provider metadata. This PR adds reviewed, family-scoped bulk cleanup flows so users can clean up common data issues faster without auto-applying destructive changes.
What changed
Screenshots
Hosted on a dedicated screenshot asset branch so the recreated PR renders the user-facing cleanup flows consistently.
Bulk update institution domains
Bulk update merchant websites
Merchant merge mutual exclusion
Category merge
Why
Large imported or synced datasets often create duplicate merchants, categories, and incomplete account/provider metadata. Reviewed bulk cleanup makes Sure easier to maintain while keeping changes scoped, transactional, and reviewable before applying.
Validation
Run through the packaged devcontainer stack for this worktree:
devcontainer up --workspace-folder . --mount-git-worktree-common-dir --include-configurationdevcontainer exec --workspace-folder . bash -lc 'cd /workspace && RAILS_ENV=test bin/rails db:prepare'devcontainer exec --workspace-folder . bash -lc 'cd /workspace && RAILS_ENV=test bin/rails assets:precompile'devcontainer exec --workspace-folder . bash -lc 'cd /workspace && bin/rails test test/controllers/accounts_controller_test.rb test/controllers/categories_controller_test.rb test/controllers/family_merchants_controller_test.rb test/models/account_test.rb test/models/merchant_test.rb'108 runs, 348 assertions, 0 failures, 0 errors, 0 skipsdevcontainer exec --workspace-folder . bash -lc 'cd /workspace && npm run lint'Checked 94 files in 89ms. No fixes applied.bin/rubocop14 files inspected, no offenses detectedgit diff --checkNotes
Summary by CodeRabbit
New Features
Improvements
Tests