Fixes & Improvements (#316)

* Some improvements

- Fix issue with lunch flow accounts that were imported
- Remove the period comparison section from reports

* Add cleanup migration

* FIX for dynamic config

* Fix linter

* FIX settings setter

Reuse the base class’ atomic setter to leverage its locking and cache invalidation.

* Make upsert atomic

* Remove migration file

Signed-off-by: soky srm <[email protected]>

* Delete db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb

Signed-off-by: soky srm <[email protected]>

* Fix cache reset

* Revert "Remove migration file"

This reverts commit 1f2a21e.

* Revert "Delete db/migrate/20251111094448_migrate_dynamic_fields_to_individual_entries.rb"

This reverts commit 29dcaaa.

* Fix Plaid initialiser

---------

Signed-off-by: soky srm <[email protected]>

Author: sokie

app/controllers/reports_controller.rb

@@ -25,9 +25,6 @@ def index
     # Calculate summary metrics
     @summary_metrics = build_summary_metrics
 
-    # Build comparison data
-    @comparison_data = build_comparison_data
-
     # Build trend data (last 6 months)
     @trends_data = build_trends_data
 
@@ -195,25 +192,6 @@ def calculate_budget_performance
       nil
     end
 
-    def build_comparison_data
-      currency_symbol = Money::Currency.new(Current.family.currency).symbol
-
-      # Totals are BigDecimal amounts in dollars - pass directly to Money.new()
-      {
-        current: {
-          income: @current_income_totals.total,
-          expenses: @current_expense_totals.total,
-          net: @current_income_totals.total - @current_expense_totals.total
-        },
-        previous: {
-          income: @previous_income_totals.total,
-          expenses: @previous_expense_totals.total,
-          net: @previous_income_totals.total - @previous_expense_totals.total
-        },
-        currency_symbol: currency_symbol
-      }
-    end
-
     def build_trends_data
       # Generate month-by-month data based on the current period filter
       trends = []

app/controllers/settings/providers_controller.rb

@@ -28,9 +28,6 @@ def update
 
     updated_fields = []
 
-    # This hash will store only the updates for dynamic (non-declared) fields
-    dynamic_updates = {}
-
     # Perform all updates within a transaction for consistency
     Setting.transaction do
       provider_params.each do |param_key, param_value|
@@ -57,32 +54,13 @@ def update
           # This is safe and uses the proper setter.
           Setting.public_send("#{key_str}=", value)
         else
-          # If it's a dynamic field, add it to our batch hash
-          # to avoid the Read-Modify-Write conflict.
-          dynamic_updates[key_str] = value
+          # If it's a dynamic field, set it as an individual entry
+          # Each field is stored independently, preventing race conditions
+          Setting[key_str] = value
         end
 
         updated_fields << param_key
       end
-
-      # Now, if we have any dynamic updates, apply them all at once
-      if dynamic_updates.any?
-        # 1. READ the current hash once
-        current_dynamic = Setting.dynamic_fields.dup
-
-        # 2. MODIFY by merging changes
-        # Treat nil values as deletions to keep the hash clean
-        dynamic_updates.each do |key, value|
-          if value.nil?
-            current_dynamic.delete(key)
-          else
-            current_dynamic[key] = value
-          end
-        end
-
-        # 3. WRITE the complete, merged hash back once
-        Setting.dynamic_fields = current_dynamic
-      end
     end
 
     if updated_fields.any?

app/models/lunchflow_item/importer.rb

@@ -29,8 +29,12 @@ def import
     accounts_failed = 0
 
     if accounts_data[:accounts].present?
-      # Get all existing lunchflow account IDs for this item (normalize to strings for comparison)
-      existing_account_ids = lunchflow_item.lunchflow_accounts.pluck(:account_id).map(&:to_s)
+      # Get only linked lunchflow account IDs (ones actually imported/used by the user)
+      # This prevents updating orphaned accounts from old behavior that saved everything
+      existing_account_ids = lunchflow_item.lunchflow_accounts
+                                           .joins(:account_provider)
+                                           .pluck(:account_id)
+                                           .map(&:to_s)
 
       accounts_data[:accounts].each do |account_data|
         account_id = account_data[:id]&.to_s

app/models/provider/configurable.rb

@@ -1,8 +1,8 @@
 # Module for providers to declare their configuration requirements
 #
 # Providers can declare their own configuration fields without needing to modify
-# the Setting model. Settings are stored dynamically using RailsSettings::Base's
-# hash-style access (Setting[:key] = value).
+# the Setting model. Settings are stored dynamically as individual entries using
+# RailsSettings::Base's bracket-style access (Setting[:key] = value).
 #
 # Configuration fields are automatically registered and displayed in the UI at
 # /settings/providers. The system checks Setting storage first, then ENV variables,
@@ -186,8 +186,8 @@ def setting_key
 
     # Get the value for this field (Setting -> ENV -> default)
     def value
-      # First try Setting using dynamic hash-style access
-      # This works even without explicit field declarations in Setting model
+      # First try Setting using dynamic bracket-style access
+      # Each field is stored as an individual entry without explicit field declarations
       setting_value = Setting[setting_key]
       return normalize_value(setting_value) if setting_value.present?
 

app/models/setting.rb

@@ -11,9 +11,8 @@ class ValidationError < StandardError; end
   field :openai_model, type: :string, default: ENV["OPENAI_MODEL"]
   field :brand_fetch_client_id, type: :string, default: ENV["BRAND_FETCH_CLIENT_ID"]
 
-  # Single hash field for all dynamic provider credentials and other dynamic settings
-  # This allows unlimited dynamic fields without declaring them upfront
-  field :dynamic_fields, type: :hash, default: {}
+  # Dynamic fields are now stored as individual entries with "dynamic:" prefix
+  # This prevents race conditions and ensures each field is independently managed
 
   # Onboarding and app settings
   ONBOARDING_STATES = %w[open closed invite_only].freeze
@@ -50,16 +49,16 @@ def onboarding_state=(state)
     end
 
     # Support dynamic field access via bracket notation
-    # First checks if it's a declared field, then falls back to dynamic_fields hash
+    # First checks if it's a declared field, then falls back to individual dynamic entries
     def [](key)
       key_str = key.to_s
 
       # Check if it's a declared field first
       if respond_to?(key_str)
         public_send(key_str)
       else
-        # Fall back to dynamic_fields hash
-        dynamic_fields[key_str]
+        # Fall back to individual dynamic entry lookup
+        find_by(var: dynamic_key_name(key_str))&.value
       end
     end
 
@@ -70,38 +69,50 @@ def []=(key, value)
       if respond_to?("#{key_str}=")
         public_send("#{key_str}=", value)
       else
-        # Otherwise, manage in dynamic_fields hash
-        current_dynamic = dynamic_fields.dup
+        # Store as individual dynamic entry
+        dynamic_key = dynamic_key_name(key_str)
         if value.nil?
-          current_dynamic.delete(key_str)          # treat nil as delete
+          where(var: dynamic_key).destroy_all
+          clear_cache
         else
-          current_dynamic[key_str] = value
+          # Use upsert for atomic insert/update to avoid race conditions
+          upsert({ var: dynamic_key, value: value.to_yaml }, unique_by: :var)
+          clear_cache
         end
-        self.dynamic_fields = current_dynamic      # persists & busts cache
       end
     end
 
     # Check if a dynamic field exists (useful to distinguish nil value vs missing key)
     def key?(key)
       key_str = key.to_s
-      respond_to?(key_str) || dynamic_fields.key?(key_str)
+      return true if respond_to?(key_str)
+
+      # Check if dynamic entry exists
+      where(var: dynamic_key_name(key_str)).exists?
     end
 
     # Delete a dynamic field
     def delete(key)
       key_str = key.to_s
       return nil if respond_to?(key_str) # Can't delete declared fields
 
-      current_dynamic = dynamic_fields.dup
-      value = current_dynamic.delete(key_str)
-      self.dynamic_fields = current_dynamic
+      dynamic_key = dynamic_key_name(key_str)
+      value = self[key_str]
+      where(var: dynamic_key).destroy_all
+      clear_cache
       value
     end
 
     # List all dynamic field keys (excludes declared fields)
     def dynamic_keys
-      dynamic_fields.keys
+      where("var LIKE ?", "dynamic:%").pluck(:var).map { |var| var.sub(/^dynamic:/, "") }
     end
+
+    private
+
+      def dynamic_key_name(key_str)
+        "dynamic:#{key_str}"
+      end
   end
 
   # Validates OpenAI configuration requires model when custom URI base is set

app/views/lunchflow_items/select_accounts.html.erb

@@ -16,10 +16,10 @@
           <div class="space-y-2">
             <% @available_accounts.each do |account| %>
               <% has_blank_name = account[:name].blank? %>
-              <label class="flex items-start gap-3 p-3 border <%= has_blank_name ? 'border-error bg-error/5' : 'border-primary' %> rounded-lg <%= has_blank_name ? 'cursor-not-allowed opacity-60' : 'hover:bg-subtle cursor-pointer' %> transition-colors">
+              <label class="flex items-start gap-3 p-3 border <%= has_blank_name ? "border-error bg-error/5" : "border-primary" %> rounded-lg <%= has_blank_name ? "cursor-not-allowed opacity-60" : "hover:bg-subtle cursor-pointer" %> transition-colors">
                 <%= check_box_tag "account_ids[]", account[:id], false, disabled: has_blank_name, class: "mt-1" %>
                 <div class="flex-1">
-                  <div class="font-medium text-sm <%= has_blank_name ? 'text-error' : 'text-primary' %>">
+                  <div class="font-medium text-sm <%= has_blank_name ? "text-error" : "text-primary" %>">
                     <% if has_blank_name %>
                       <%= t(".no_name_placeholder") %>
                     <% else %>

app/views/lunchflow_items/select_existing_account.html.erb

@@ -16,10 +16,10 @@
           <div class="space-y-2">
             <% @available_accounts.each do |account| %>
               <% has_blank_name = account[:name].blank? %>
-              <label class="flex items-start gap-3 p-3 border <%= has_blank_name ? 'border-error bg-error/5' : 'border-primary' %> rounded-lg <%= has_blank_name ? 'cursor-not-allowed opacity-60' : 'hover:bg-subtle cursor-pointer' %> transition-colors">
+              <label class="flex items-start gap-3 p-3 border <%= has_blank_name ? "border-error bg-error/5" : "border-primary" %> rounded-lg <%= has_blank_name ? "cursor-not-allowed opacity-60" : "hover:bg-subtle cursor-pointer" %> transition-colors">
                 <%= radio_button_tag "lunchflow_account_id", account[:id], false, disabled: has_blank_name, class: "mt-1" %>
                 <div class="flex-1">
-                  <div class="font-medium text-sm <%= has_blank_name ? 'text-error' : 'text-primary' %>">
+                  <div class="font-medium text-sm <%= has_blank_name ? "text-error" : "text-primary" %>">
                     <% if has_blank_name %>
                       <%= t(".no_name_placeholder") %>
                     <% else %>