diff --git a/app/controllers/api/v1/transactions_controller.rb b/app/controllers/api/v1/transactions_controller.rb index 299ff274b..08ca81cee 100644 --- a/app/controllers/api/v1/transactions_controller.rb +++ b/app/controllers/api/v1/transactions_controller.rb @@ -105,19 +105,29 @@ class Api::V1::TransactionsController < Api::V1::BaseController end def update - if @entry.update(entry_params_for_update) - @entry.sync_account_later - @entry.lock_saved_attributes! - @entry.transaction.lock_attr!(:tag_ids) if @entry.transaction.tags.any? + Entry.transaction do + if @entry.update(entry_params_for_update) + # Handle tags separately - only when explicitly provided in the request + # This allows clearing tags with tag_ids: [] while preserving tags when not specified + if tags_provided? + @entry.transaction.tag_ids = transaction_params[:tag_ids] || [] + @entry.transaction.save! + @entry.transaction.lock_attr!(:tag_ids) if @entry.transaction.tags.any? + end - @transaction = @entry.transaction - render :show - else - render json: { - error: "validation_failed", - message: "Transaction could not be updated", - errors: @entry.errors.full_messages - }, status: :unprocessable_entity + @entry.sync_account_later + @entry.lock_saved_attributes! + + @transaction = @entry.transaction + render :show + else + render json: { + error: "validation_failed", + message: "Transaction could not be updated", + errors: @entry.errors.full_messages + }, status: :unprocessable_entity + raise ActiveRecord::Rollback + end end rescue => e @@ -283,8 +293,9 @@ end entryable_attributes: { id: @entry.entryable_id, category_id: transaction_params[:category_id], - merchant_id: transaction_params[:merchant_id], - tag_ids: transaction_params[:tag_ids] + merchant_id: transaction_params[:merchant_id] + # Note: tag_ids handled separately in update action to distinguish + # "not provided" from "explicitly set to empty" }.compact_blank } @@ -296,6 +307,12 @@ end entry_params.compact end + # Check if tag_ids was explicitly provided in the request. + # This distinguishes between "user wants to update tags" vs "user didn't specify tags". + def tags_provided? + params[:transaction].key?(:tag_ids) + end + def calculate_signed_amount amount = transaction_params[:amount].to_f nature = transaction_params[:nature] diff --git a/app/controllers/transactions/bulk_updates_controller.rb b/app/controllers/transactions/bulk_updates_controller.rb index 08c4befe2..8a115e58d 100644 --- a/app/controllers/transactions/bulk_updates_controller.rb +++ b/app/controllers/transactions/bulk_updates_controller.rb @@ -6,7 +6,7 @@ class Transactions::BulkUpdatesController < ApplicationController updated = Current.family .entries .where(id: bulk_update_params[:entry_ids]) - .bulk_update!(bulk_update_params) + .bulk_update!(bulk_update_params, update_tags: tags_provided?) redirect_back_or_to transactions_path, notice: "#{updated} transactions updated" end @@ -16,4 +16,11 @@ class Transactions::BulkUpdatesController < ApplicationController params.require(:bulk_update) .permit(:date, :notes, :category_id, :merchant_id, entry_ids: [], tag_ids: []) end + + # Check if tag_ids was explicitly provided in the request. + # This distinguishes between "user wants to update tags" vs "user didn't touch tags field". + def tags_provided? + bulk_update = params[:bulk_update] + bulk_update.respond_to?(:key?) && bulk_update.key?(:tag_ids) + end end diff --git a/app/models/entry.rb b/app/models/entry.rb index 2e4887098..3812e3d58 100644 --- a/app/models/entry.rb +++ b/app/models/entry.rb @@ -323,27 +323,50 @@ class Entry < ApplicationRecord 30.years.ago.to_date end - def bulk_update!(bulk_update_params) + # Bulk update entries with the given parameters. + # + # Tags are handled separately from other entryable attributes because they use + # a join table (taggings) rather than a direct column. This means: + # - category_id: nil means "no category" (column value) + # - tag_ids: [] means "delete all taggings" (join table operation) + # + # To avoid accidentally clearing tags when only updating other fields, + # tags are only modified when explicitly requested via update_tags: true. + # + # @param bulk_update_params [Hash] The parameters to update + # @param update_tags [Boolean] Whether to update tags (default: false) + def bulk_update!(bulk_update_params, update_tags: false) bulk_attributes = { date: bulk_update_params[:date], notes: bulk_update_params[:notes], entryable_attributes: { category_id: bulk_update_params[:category_id], - merchant_id: bulk_update_params[:merchant_id], - tag_ids: bulk_update_params[:tag_ids] + merchant_id: bulk_update_params[:merchant_id] }.compact_blank }.compact_blank - return 0 if bulk_attributes.blank? + tag_ids = Array.wrap(bulk_update_params[:tag_ids]).reject(&:blank?) + has_updates = bulk_attributes.present? || update_tags + + return 0 unless has_updates transaction do all.each do |entry| - bulk_attributes[:entryable_attributes][:id] = entry.entryable_id if bulk_attributes[:entryable_attributes].present? - entry.update! bulk_attributes + # Update standard attributes + if bulk_attributes.present? + bulk_attributes[:entryable_attributes][:id] = entry.entryable_id if bulk_attributes[:entryable_attributes].present? + entry.update! bulk_attributes + end + + # Handle tags separately - only when explicitly requested + if update_tags && entry.transaction? + entry.transaction.tag_ids = tag_ids + entry.transaction.save! + entry.entryable.lock_attr!(:tag_ids) if entry.transaction.tags.any? + end entry.lock_saved_attributes! entry.mark_user_modified! - entry.entryable.lock_attr!(:tag_ids) if entry.transaction? && entry.transaction.tags.any? end end diff --git a/app/views/transactions/bulk_updates/new.html.erb b/app/views/transactions/bulk_updates/new.html.erb index 7b6c1e080..a22b30e9f 100644 --- a/app/views/transactions/bulk_updates/new.html.erb +++ b/app/views/transactions/bulk_updates/new.html.erb @@ -12,7 +12,7 @@
<%= form.collection_select :category_id, Current.family.categories.alphabetically, :id, :name, { prompt: "Select a category", label: "Category", class: "text-subdued" } %> <%= form.collection_select :merchant_id, Current.family.available_merchants.alphabetically, :id, :name, { prompt: "Select a merchant", label: "Merchant", class: "text-subdued" } %> - <%= form.select :tag_ids, Current.family.tags.alphabetically.pluck(:name, :id), { include_blank: "None", multiple: true, label: "Tags" } %> + <%= form.select :tag_ids, Current.family.tags.alphabetically.pluck(:name, :id), { include_blank: "None", multiple: true, label: "Tags", include_hidden: false } %> <%= form.text_area :notes, label: "Notes", placeholder: "Enter a note that will be applied to selected transactions", rows: 5 %>
<% end %> diff --git a/spec/requests/api/v1/transactions_spec.rb b/spec/requests/api/v1/transactions_spec.rb index 575aaf4f4..5e114c705 100644 --- a/spec/requests/api/v1/transactions_spec.rb +++ b/spec/requests/api/v1/transactions_spec.rb @@ -282,7 +282,11 @@ RSpec.describe 'API V1 Transactions', type: :request do category_id: { type: :string, format: :uuid }, merchant_id: { type: :string, format: :uuid }, nature: { type: :string, enum: %w[income expense inflow outflow] }, - tag_ids: { type: :array, items: { type: :string, format: :uuid } } + tag_ids: { + type: :array, + items: { type: :string, format: :uuid }, + description: 'Array of tag IDs to assign. Omit to preserve existing tags; use [] to clear all tags.' + } } } } diff --git a/test/controllers/api/v1/transactions_controller_test.rb b/test/controllers/api/v1/transactions_controller_test.rb index 9b681d4de..8ddb0ebf7 100644 --- a/test/controllers/api/v1/transactions_controller_test.rb +++ b/test/controllers/api/v1/transactions_controller_test.rb @@ -257,6 +257,75 @@ class Api::V1::TransactionsControllerTest < ActionDispatch::IntegrationTest assert_response :unauthorized end + test "should preserve tags when tag_ids not provided in update" do + # Set up transaction with existing tags + original_tags = [ Tag.first, Tag.second ] + @transaction.tags = original_tags + @transaction.save! + + # Update only the name, without providing tag_ids + update_params = { + transaction: { + name: "Updated Name Only" + } + } + + put api_v1_transaction_url(@transaction), + params: update_params, + headers: api_headers(@api_key) + assert_response :success + + @transaction.reload + assert_equal "Updated Name Only", @transaction.entry.name + # Tags should be preserved since tag_ids was not in the request + assert_equal original_tags.map(&:id).sort, @transaction.tag_ids.sort + end + + test "should clear tags when empty tag_ids explicitly provided in update" do + # Set up transaction with existing tags + @transaction.tags = [ Tag.first, Tag.second ] + @transaction.save! + + # Explicitly provide empty tag_ids to clear tags + update_params = { + transaction: { + name: "Updated Name", + tag_ids: [] + } + } + + put api_v1_transaction_url(@transaction), + params: update_params, + headers: api_headers(@api_key) + assert_response :success + + @transaction.reload + # Tags should be cleared since tag_ids was explicitly provided as empty + assert_empty @transaction.tags + end + + test "should update tags when tag_ids explicitly provided in update" do + # Set up transaction with one tag + @transaction.tags = [ Tag.first ] + @transaction.save! + + new_tags = [ Tag.second ] + + update_params = { + transaction: { + tag_ids: new_tags.map(&:id) + } + } + + put api_v1_transaction_url(@transaction), + params: update_params, + headers: api_headers(@api_key) + assert_response :success + + @transaction.reload + assert_equal new_tags.map(&:id), @transaction.tag_ids + end + # DESTROY action tests test "should destroy transaction" do entry_to_delete = @account.entries.create!( diff --git a/test/controllers/transactions/bulk_updates_controller_test.rb b/test/controllers/transactions/bulk_updates_controller_test.rb index d7f360faa..0ab7fc016 100644 --- a/test/controllers/transactions/bulk_updates_controller_test.rb +++ b/test/controllers/transactions/bulk_updates_controller_test.rb @@ -32,4 +32,90 @@ class Transactions::BulkUpdatesControllerTest < ActionDispatch::IntegrationTest assert_equal [ Tag.first.id, Tag.second.id ], transaction.entryable.tag_ids.sort end end + + test "bulk update preserves tags when tag_ids not provided" do + transaction_entry = @user.family.entries.transactions.first + original_tags = [ Tag.first, Tag.second ] + transaction_entry.transaction.tags = original_tags + transaction_entry.transaction.save! + + # Update only the category, without providing tag_ids + post transactions_bulk_update_url, params: { + bulk_update: { + entry_ids: [ transaction_entry.id ], + category_id: Category.second.id + } + } + + assert_redirected_to transactions_url + + transaction_entry.reload + assert_equal Category.second, transaction_entry.transaction.category + # Tags should be preserved since tag_ids was not in the request + assert_equal original_tags.map(&:id).sort, transaction_entry.transaction.tag_ids.sort + end + + test "bulk update clears tags when tag_ids is blank string array (web multi-select None)" do + transaction_entry = @user.family.entries.transactions.first + original_tags = [ Tag.first, Tag.second ] + transaction_entry.transaction.tags = original_tags + transaction_entry.transaction.save! + + # For a multiple select, choosing the blank ("None") option submits a blank value. + post transactions_bulk_update_url, params: { + bulk_update: { + entry_ids: [ transaction_entry.id ], + category_id: Category.second.id, + tag_ids: [ "" ] + } + } + + assert_redirected_to transactions_url + + transaction_entry.reload + assert_equal Category.second, transaction_entry.transaction.category + assert_empty transaction_entry.transaction.tags + end + + test "bulk update clears tags when empty tag_ids explicitly provided (JSON)" do + transaction_entry = @user.family.entries.transactions.first + transaction_entry.transaction.tags = [ Tag.first, Tag.second ] + transaction_entry.transaction.save! + + post transactions_bulk_update_url, + params: { + bulk_update: { + entry_ids: [ transaction_entry.id ], + category_id: Category.second.id, + tag_ids: [] + } + }, + as: :json + + assert_redirected_to transactions_url + + transaction_entry.reload + assert_equal Category.second, transaction_entry.transaction.category + assert_empty transaction_entry.transaction.tags + end + + test "bulk update replaces tags when tag_ids explicitly provided" do + transaction_entry = @user.family.entries.transactions.first + transaction_entry.transaction.tags = [ Tag.first ] + transaction_entry.transaction.save! + + new_tag = Tag.second + + post transactions_bulk_update_url, params: { + bulk_update: { + entry_ids: [ transaction_entry.id ], + tag_ids: [ new_tag.id ] + } + } + + assert_redirected_to transactions_url + + transaction_entry.reload + assert_equal [ new_tag.id ], transaction_entry.transaction.tag_ids + end end