-
Notifications
You must be signed in to change notification settings - Fork 7
feat(settings): add reviewed bulk cleanup flows #1630
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 10 commits
d206669
e3ae39e
9c9e6c6
ff3e3ea
9e98ed9
4b4cac0
e0087ed
ad7456f
6287683
27a2783
d0d5148
2feb858
6db4812
f2d3c51
4db76cc
9c81230
402dd20
4f83204
140dc82
716bb34
49a606a
4c0e431
fdbb039
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,7 @@ | ||
| class CategoriesController < ApplicationController | ||
| MergeTargetNotFound = Class.new(StandardError) | ||
| EmptyCategoryMerge = Class.new(StandardError) | ||
|
|
||
| before_action :set_category, only: %i[edit update destroy] | ||
| before_action :set_categories, only: %i[update edit] | ||
| before_action :set_transaction, only: :create | ||
|
|
@@ -14,6 +17,12 @@ def new | |
| set_categories | ||
| end | ||
|
|
||
| def merge | ||
| @categories = Current.family.categories.alphabetically | ||
|
|
||
| render layout: "settings" | ||
| end | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| def create | ||
| @category = Current.family.categories.new(category_params) | ||
|
|
||
|
|
@@ -67,6 +76,35 @@ def bootstrap | |
| redirect_back_or_to categories_path, notice: t(".success") | ||
| end | ||
|
|
||
| def perform_merge | ||
| permitted_params = category_merge_params | ||
|
|
||
| if conflicting_merge_target?(permitted_params) | ||
| return redirect_to merge_categories_path, alert: t(".conflicting_target") | ||
| end | ||
|
|
||
| if target_selected_as_source?(permitted_params) | ||
| return redirect_to merge_categories_path, alert: t(".target_selected_as_source") | ||
| end | ||
|
|
||
| sources = Current.family.categories.where(id: permitted_params[:source_ids]) | ||
| unless sources.any? | ||
| return redirect_to merge_categories_path, alert: t(".invalid_categories") | ||
| end | ||
|
|
||
| merger = merge_categories!(permitted_params, sources) | ||
|
|
||
| redirect_to categories_path, notice: t(".success", count: merger.merged_count) | ||
| rescue MergeTargetNotFound | ||
| redirect_to merge_categories_path, alert: t(".target_not_found") | ||
| rescue EmptyCategoryMerge | ||
| redirect_to merge_categories_path, alert: t(".no_categories_selected") | ||
| rescue Category::Merger::UnauthorizedCategoryError => e | ||
|
Comment on lines
+92
to
+102
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Controller Initializing A cleaner approach: move the redirect logic so that all branching happens inside the transaction for the happy path, and only rescue/redirect outside. For example: def perform_merge
permitted_params = category_merge_params
return redirect_to merge_categories_path, alert: t(".conflicting_target") if conflicting_merge_target?(permitted_params)
sources = Current.family.categories.where(id: permitted_params[:source_ids])
return redirect_to merge_categories_path, alert: t(".invalid_categories") unless sources.any?
ActiveRecord::Base.transaction do
target = merge_target_category(permitted_params) or raise ActiveRecord::Rollback
merger = Category::Merger.new(family: Current.family, target_category: target, source_categories: sources)
merger.merge! or raise ActiveRecord::Rollback
# if we reach here, commit and redirect
return redirect_to categories_path, notice: t(".success", count: merger.merged_count)
end
redirect_to merge_categories_path, alert: t(".no_categories_selected")
rescue Category::Merger::UnauthorizedCategoryError => e
redirect_to merge_categories_path, alert: e.message
rescue ActiveRecord::RecordInvalid => e
redirect_to merge_categories_path, alert: e.record.errors.full_messages.to_sentence
endThis makes the control flow explicit: return from inside the transaction on success, fall through to an alert redirect if the transaction was rolled back. Generated by Claude Code |
||
| redirect_to merge_categories_path, alert: e.message | ||
| rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotDestroyed => e | ||
| redirect_to merge_categories_path, alert: record_error_message(e) | ||
| end | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| private | ||
| def set_category | ||
| @category = Current.family.categories.find(params[:id]) | ||
|
|
@@ -89,4 +127,48 @@ def set_transaction | |
| def category_params | ||
| params.require(:category).permit(:name, :color, :parent_id, :lucide_icon) | ||
| end | ||
|
|
||
| def category_merge_params | ||
| params.permit(:target_id, :new_target_name, :new_target_color, :new_target_icon, source_ids: []) | ||
| end | ||
|
|
||
| def conflicting_merge_target?(permitted_params) | ||
| permitted_params[:target_id].present? && permitted_params[:new_target_name].present? | ||
| end | ||
|
|
||
| def target_selected_as_source?(permitted_params) | ||
| permitted_params[:target_id].present? && Array(permitted_params[:source_ids]).include?(permitted_params[:target_id]) | ||
| end | ||
|
|
||
| def merge_target_category(permitted_params) | ||
| if permitted_params[:new_target_name].present? | ||
| Current.family.categories.create!( | ||
| name: permitted_params[:new_target_name], | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The form pre-populates the color picker with A safer fallback is the same default the form uses: color: permitted_params[:new_target_color].presence || Category::COLORS.first,The same issue exists in Generated by Claude Code |
||
| color: permitted_params[:new_target_color].presence || Category::COLORS.first, | ||
| lucide_icon: permitted_params[:new_target_icon].presence || Category.suggested_icon(permitted_params[:new_target_name]) | ||
| ) | ||
| else | ||
| Current.family.categories.find_by(id: permitted_params[:target_id]) | ||
| end | ||
| end | ||
|
|
||
| def merge_categories!(permitted_params, sources) | ||
| Category.transaction do | ||
| target = merge_target_category(permitted_params) || raise(MergeTargetNotFound) | ||
| merger = Category::Merger.new( | ||
| family: Current.family, | ||
| target_category: target, | ||
| source_categories: sources | ||
| ) | ||
|
|
||
| raise EmptyCategoryMerge unless merger.merge! | ||
|
|
||
| merger | ||
| end | ||
| end | ||
|
|
||
| def record_error_message(error) | ||
| record = error.respond_to?(:record) ? error.record : nil | ||
| record&.errors&.full_messages&.to_sentence.presence || error.message | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -100,22 +100,55 @@ def enhance | |
|
|
||
| def merge | ||
| @merchants = all_family_merchants | ||
| @default_merchant_color = FamilyMerchant.default_color | ||
| end | ||
|
|
||
| def bulk_websites | ||
| @merchants = all_family_merchants | ||
| end | ||
|
|
||
| def bulk_update_websites | ||
| permitted_params = bulk_website_params | ||
| merchants = all_family_merchants.where(id: permitted_params[:merchant_ids]) | ||
| website_url = Merchant.extract_domain(permitted_params[:website_url]) | ||
|
|
||
| unless merchants.any? && website_url.present? | ||
| return redirect_to bulk_websites_family_merchants_path, alert: t(".invalid_selection") | ||
| end | ||
|
|
||
| Merchant.transaction do | ||
| merchants.each do |merchant| | ||
| merchant.update!(website_url: website_url) | ||
| merchant.generate_logo_url_from_website! if merchant.is_a?(ProviderMerchant) | ||
| end | ||
| end | ||
|
|
||
| redirect_to family_merchants_path, notice: t(".success", count: merchants.count) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| rescue ActiveRecord::RecordInvalid => e | ||
| error_message = e.record.errors.full_messages.to_sentence.presence || e.message | ||
| redirect_to bulk_websites_family_merchants_path, alert: t(".failure", error: error_message) | ||
| end | ||
|
|
||
| def perform_merge | ||
| # Scope lookups to merchants valid for this family (FamilyMerchants + assigned ProviderMerchants) | ||
| valid_merchants = all_family_merchants | ||
| permitted_params = merchant_merge_params | ||
|
|
||
| target = valid_merchants.find_by(id: params[:target_id]) | ||
| unless target | ||
| return redirect_to merge_family_merchants_path, alert: t(".target_not_found") | ||
| if conflicting_merge_target?(permitted_params) | ||
| return redirect_to merge_family_merchants_path, alert: t(".conflicting_target") | ||
| end | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing error handling — potential 500.
redirect_to family_merchants_path, notice: t(".success", count: merchants.count)
rescue ActiveRecord::RecordInvalid => e
redirect_to bulk_websites_family_merchants_path, alert: e.record.errors.full_messages.to_sentence
endGenerated by Claude Code |
||
|
|
||
| sources = valid_merchants.where(id: params[:source_ids]) | ||
| # Scope lookups to merchants valid for this family (FamilyMerchants + assigned ProviderMerchants) | ||
| valid_merchants = all_family_merchants | ||
|
|
||
| sources = valid_merchants.where(id: permitted_params[:source_ids]) | ||
| unless sources.any? | ||
| return redirect_to merge_family_merchants_path, alert: t(".invalid_merchants") | ||
| end | ||
|
|
||
| target = merge_target_merchant(valid_merchants, permitted_params) | ||
| unless target | ||
| return redirect_to merge_family_merchants_path, alert: t(".target_not_found") | ||
| end | ||
|
|
||
| merger = Merchant::Merger.new( | ||
| family: Current.family, | ||
| target_merchant: target, | ||
|
|
@@ -129,6 +162,8 @@ def perform_merge | |
| end | ||
| rescue Merchant::Merger::UnauthorizedMerchantError => e | ||
| redirect_to merge_family_merchants_path, alert: e.message | ||
| rescue ActiveRecord::RecordInvalid => e | ||
| redirect_to merge_family_merchants_path, alert: e.record.errors.full_messages.to_sentence | ||
| end | ||
|
|
||
| private | ||
|
|
@@ -145,6 +180,18 @@ def merchant_params | |
| params.require(key).permit(:name, :color, :website_url) | ||
| end | ||
|
|
||
| def bulk_website_params | ||
| params.permit(:website_url, merchant_ids: []) | ||
| end | ||
|
|
||
| def merchant_merge_params | ||
| params.permit(:target_id, :new_target_name, :new_target_color, :new_target_website_url, source_ids: []) | ||
| end | ||
|
|
||
| def conflicting_merge_target?(permitted_params) | ||
| permitted_params[:target_id].present? && permitted_params[:new_target_name].present? | ||
| end | ||
|
|
||
| def all_family_merchants | ||
| family_merchant_ids = Current.family.merchants.pluck(:id) | ||
| provider_merchant_ids = Current.family.assigned_merchants.where(type: "ProviderMerchant").pluck(:id) | ||
|
|
@@ -153,4 +200,16 @@ def all_family_merchants | |
| Merchant.where(id: combined_ids) | ||
| .order(Arel.sql("LOWER(COALESCE(name, ''))")) | ||
| end | ||
|
|
||
| def merge_target_merchant(valid_merchants, permitted_params) | ||
| if permitted_params[:new_target_name].present? | ||
| Current.family.merchants.create!( | ||
| name: permitted_params[:new_target_name], | ||
| color: permitted_params[:new_target_color].presence || FamilyMerchant.default_color, | ||
| website_url: Merchant.extract_domain(permitted_params[:new_target_website_url]) | ||
| ) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| else | ||
| valid_merchants.find_by(id: permitted_params[:target_id]) | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,49 @@ | ||
| import { Controller } from "@hotwired/stimulus" | ||
|
|
||
| const DISABLED_CLASSES = ["opacity-50", "pointer-events-none"] | ||
|
|
||
| export default class extends Controller { | ||
| static targets = [ | ||
| "existingSection", | ||
| "existingTarget", | ||
| "newSection", | ||
| "newTextInput", | ||
| "newColorInput" | ||
| ] | ||
| static values = { defaultColor: String } | ||
|
|
||
| connect() { | ||
| this.sync() | ||
| } | ||
|
|
||
| sync() { | ||
| const existingTargetSelected = this.hasExistingTargetTarget && this.existingTargetTarget.value !== "" | ||
| const newTargetStarted = this.newTargetStarted() | ||
|
|
||
| this.setSectionDisabled(this.existingSectionTarget, newTargetStarted) | ||
| this.setSectionDisabled(this.newSectionTarget, existingTargetSelected) | ||
| } | ||
|
|
||
| beforeSubmit() { | ||
| this.sync() | ||
| } | ||
|
|
||
| newTargetStarted() { | ||
| const textEntered = this.newTextInputTargets.some((input) => input.value.trim() !== "") | ||
| const colorChanged = this.hasNewColorInputTarget && | ||
| this.defaultColorValue && | ||
| this.newColorInputTarget.value.toLowerCase() !== this.defaultColorValue.toLowerCase() | ||
|
|
||
| return textEntered || colorChanged | ||
| } | ||
|
|
||
| setSectionDisabled(section, disabled) { | ||
| section.setAttribute("aria-disabled", disabled.toString()) | ||
| section.classList.toggle("cursor-not-allowed", disabled) | ||
| DISABLED_CLASSES.forEach((className) => section.classList.toggle(className, disabled)) | ||
|
|
||
| section.querySelectorAll("input, button, select, textarea").forEach((input) => { | ||
| input.disabled = disabled | ||
| }) | ||
| } | ||
| } |
Uh oh!
There was an error while loading. Please reload this page.