fix: Preserve tags on bulk edits (take 3) (#889)

* fix: handle tags separately from entryable_attributes in bulk updates

Tags use a join table (taggings) rather than a direct column, which means
empty tag_ids clears all tags rather than meaning "no change". This caused
bulk category-only edits to accidentally clear existing tags.

This fix:
- Removes tag_ids from entryable_attributes in Entry.bulk_update!
- Adds update_tags parameter to explicitly control tag updates
- Uses params.key?(:tag_ids) in controller to detect explicit tag changes
- Preserves existing tags when tag_ids is not provided in the request

This is a cleaner architectural solution compared to tracking "touched"
state in the frontend, as it properly acknowledges the semantic difference
between column attributes and join table associations.

https://claude.ai/code/session_014CsmTwjteP4qJs6YZqCKnY

* fix: handle tags separately in API transaction updates

Apply the same pattern to the API endpoint: tags are now handled
separately from entryable_attributes to distinguish between "not
provided" (preserve existing tags) and "explicitly set to empty"
(clear all tags).

This allows API consumers to:
- Update other fields without affecting tags (omit tag_ids)
- Clear all tags (send tag_ids: [])
- Set specific tags (send tag_ids: [id1, id2])

https://claude.ai/code/session_014CsmTwjteP4qJs6YZqCKnY

* Proposed fix

* fix: improve tag handling in bulk updates for transactions

* fix: allow bulk edit to clear/preserve tags by omitting hidden multi-select field

* PR comments

* Dumb copy/paste error

* Linter

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Juan José Mata <juanjo.mata@gmail.com>
Co-authored-by: Juan José Mata <jjmata@jjmata.com>

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]

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

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
 

app/views/transactions/bulk_updates/new.html.erb

@@ -12,7 +12,7 @@
           <div class="space-y-2">
             <%= 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 %>
           </div>
         <% end %>

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.'
+              }
             }
           }
         }

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!(

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