Merge branch 'main' of github.com:maybe-finance/maybe into zachgoll/plaid-sync-domain-improvements

Gemfile.lock

@@ -120,7 +120,7 @@ GEM
       smart_properties
     bigdecimal (3.1.9)
     bindex (0.8.1)
-    bootsnap (1.18.4)
+    bootsnap (1.18.6)
       msgpack (~> 1.2)
     brakeman (7.0.2)
       racc
@@ -243,10 +243,10 @@ GEM
       rdoc (>= 4.0.0)
       reline (>= 0.4.2)
     jmespath (1.6.2)
-    json (2.11.3)
+    json (2.12.0)
     jwt (2.10.1)
       base64
-    language_server-protocol (3.17.0.4)
+    language_server-protocol (3.17.0.5)
     launchy (3.1.1)
       addressable (~> 2.8)
       childprocess (~> 5.0)
@@ -361,7 +361,7 @@ GEM
       nio4r (~> 2.0)
     raabro (1.4.0)
     racc (1.8.1)
-    rack (3.1.13)
+    rack (3.1.15)
     rack-mini-profiler (3.3.1)
       rack (>= 1.2.0)
     rack-session (2.1.0)
@@ -411,7 +411,7 @@ GEM
     rb-fsevent (0.11.2)
     rb-inotify (0.11.1)
       ffi (~> 1.0)
-    rbs (3.9.2)
+    rbs (3.9.4)
       logger
     rdoc (6.13.1)
       psych (>= 4.0.0)
@@ -458,13 +458,13 @@ GEM
       rubocop (>= 1.72)
       rubocop-performance (>= 1.24)
       rubocop-rails (>= 2.30)
-    ruby-lsp (0.23.16)
+    ruby-lsp (0.23.20)
       language_server-protocol (~> 3.17.0)
       prism (>= 1.2, < 2.0)
       rbs (>= 3, < 4)
       sorbet-runtime (>= 0.5.10782)
-    ruby-lsp-rails (0.4.2)
-      ruby-lsp (>= 0.23.16, < 0.24.0)
+    ruby-lsp-rails (0.4.3)
+      ruby-lsp (>= 0.23.18, < 0.24.0)
     ruby-openai (8.1.0)
       event_stream_parser (>= 0.3.0, < 2.0.0)
       faraday (>= 1)
@@ -485,14 +485,14 @@ GEM
       rexml (~> 3.2, >= 3.2.5)
       rubyzip (>= 1.2.2, < 3.0)
       websocket (~> 1.0)
-    sentry-rails (5.23.0)
+    sentry-rails (5.24.0)
       railties (>= 5.0)
-      sentry-ruby (~> 5.23.0)
-    sentry-ruby (5.23.0)
+      sentry-ruby (~> 5.24.0)
+    sentry-ruby (5.24.0)
       bigdecimal
       concurrent-ruby (~> 1.0, >= 1.0.2)
-    sentry-sidekiq (5.23.0)
-      sentry-ruby (~> 5.23.0)
+    sentry-sidekiq (5.24.0)
+      sentry-ruby (~> 5.24.0)
       sidekiq (>= 3.0)
     sidekiq (8.0.3)
       connection_pool (>= 2.5.0)
@@ -500,7 +500,7 @@ GEM
       logger (>= 1.6.2)
       rack (>= 3.1.0)
       redis-client (>= 0.23.2)
-    sidekiq-cron (2.2.0)
+    sidekiq-cron (2.3.0)
       cronex (>= 0.13.0)
       fugit (~> 1.8, >= 1.11.1)
       globalid (>= 1.0.1)
@@ -514,7 +514,7 @@ GEM
     skylight (6.0.4)
       activesupport (>= 5.2.0)
     smart_properties (1.17.0)
-    sorbet-runtime (0.5.12060)
+    sorbet-runtime (0.5.12115)
     stimulus-rails (1.3.4)
       railties (>= 6.0.0)
     stringio (3.1.7)
@@ -546,7 +546,7 @@ GEM
     useragent (0.16.11)
     vcr (6.3.1)
       base64
-    vernier (1.7.0)
+    vernier (1.7.1)
     view_component (3.22.0)
       activesupport (>= 5.2.0, < 8.1)
       concurrent-ruby (= 1.3.4)

app/controllers/transactions_controller.rb

@@ -61,7 +61,7 @@ class TransactionsController < ApplicationController
     if @entry.save
       @entry.sync_account_later
       @entry.lock_saved_attributes!
-      @entry.transaction.lock!(:tag_ids) if @entry.transaction.tags.any?
+      @entry.transaction.lock_attr!(:tag_ids) if @entry.transaction.tags.any?
 
       flash[:notice] = "Transaction created"
 
@@ -88,7 +88,7 @@ class TransactionsController < ApplicationController
 
       @entry.sync_account_later
       @entry.lock_saved_attributes!
-      @entry.transaction.lock!(:tag_ids) if @entry.transaction.tags.any?
+      @entry.transaction.lock_attr!(:tag_ids) if @entry.transaction.tags.any?
 
       respond_to do |format|
         format.html { redirect_back_or_to account_path(@entry.account), notice: "Transaction updated" }

app/javascript/controllers/rule/conditions_controller.js

@@ -13,7 +13,7 @@ export default class extends Controller {
 
   addSubCondition() {
     const html = this.subConditionTemplateTarget.innerHTML.replaceAll(
-      "IDX_PLACEHOLDER",
+      "IDX_CHILD_PLACEHOLDER",
       this.#uniqueKey(),
     );
 
@@ -110,6 +110,6 @@ export default class extends Controller {
   }
 
   #uniqueKey() {
-    return Math.random().toString(36).substring(2, 15);
+    return Date.now();
   }
 }

app/models/concerns/enrichable.rb

@@ -42,17 +42,17 @@ module Enrichable
     !locked?(attr)
   end
 
-  def lock!(attr)
+  def lock_attr!(attr)
     update!(locked_attributes: locked_attributes.merge(attr.to_s => Time.current))
   end
 
-  def unlock!(attr)
+  def unlock_attr!(attr)
     update!(locked_attributes: locked_attributes.except(attr.to_s))
   end
 
   def lock_saved_attributes!
     saved_changes.keys.reject { |attr| ignored_enrichable_attributes.include?(attr) }.each do |attr|
-      lock!(attr)
+      lock_attr!(attr)
     end
   end
 

app/models/concerns/syncable.rb

@@ -9,20 +9,28 @@ module Syncable
     raise NotImplementedError, "Subclasses must implement the syncing? method"
   end
 
+  # Schedules a sync for syncable.  If there is an existing sync pending/syncing for this syncable,
+  # we do not create a new sync, and attempt to expand the sync window if needed.
   def sync_later(parent_sync: nil, window_start_date: nil, window_end_date: nil)
     Sync.transaction do
-      # Since we're scheduling a new sync, mark old syncs for this syncable as stale
-      self.syncs.incomplete.find_each(&:mark_stale!)
+      with_lock do
+        sync = self.syncs.incomplete.first
 
-      new_sync = self.syncs.create!(
-        parent: parent_sync,
-        window_start_date: window_start_date,
-        window_end_date: window_end_date
-      )
+        if sync
+          Rails.logger.info("There is an existing sync, expanding window if needed (#{sync.id})")
+          sync.expand_window_if_needed(window_start_date, window_end_date)
+        else
+          sync = self.syncs.create!(
+            parent: parent_sync,
+            window_start_date: window_start_date,
+            window_end_date: window_end_date
+          )
 
-      SyncJob.perform_later(new_sync)
+          SyncJob.perform_later(sync)
+        end
 
-      new_sync
+        sync
+      end
     end
   end
 

app/models/entry.rb

@@ -85,7 +85,7 @@ class Entry < ApplicationRecord
           entry.update! bulk_attributes
 
           entry.lock_saved_attributes!
-          entry.entryable.lock!(:tag_ids) if entry.transaction? && entry.transaction.tags.any?
+          entry.entryable.lock_attr!(:tag_ids) if entry.transaction? && entry.transaction.tags.any?
         end
       end
 

app/models/exchange_rate/importer.rb

@@ -18,8 +18,8 @@ class ExchangeRate::Importer
       return
     end
 
-    if clear_cache && provider_rates.empty?
-      Rails.logger.warn("Could not clear cache for #{from} to #{to} between #{start_date} and #{end_date} because provider returned no rates")
+    if provider_rates.empty?
+      Rails.logger.warn("Could not fetch rates for #{from} to #{to} between #{start_date} and #{end_date} because provider returned no rates")
       return
     end
 

app/models/family/auto_categorizer.rb

@@ -27,7 +27,7 @@ class Family::AutoCategorizer
     end
 
     scope.each do |transaction|
-      transaction.lock!(:category_id)
+      transaction.lock_attr!(:category_id)
 
       auto_categorization = result.data.find { |c| c.transaction_id == transaction.id }
 

app/models/family/auto_merchant_detector.rb

@@ -27,7 +27,7 @@ class Family::AutoMerchantDetector
     end
 
     scope.each do |transaction|
-      transaction.lock!(:merchant_id)
+      transaction.lock_attr!(:merchant_id)
 
       auto_detection = result.data.find { |c| c.transaction_id == transaction.id }
 

app/models/security/price/importer.rb

@@ -18,8 +18,8 @@ class Security::Price::Importer
       return 0
     end
 
-    if clear_cache && provider_prices.empty?
-      Rails.logger.warn("Could not clear cache for #{security.ticker} between #{start_date} and #{end_date} because provider returned no prices")
+    if provider_prices.empty?
+      Rails.logger.warn("Could not fetch prices for #{security.ticker} between #{start_date} and #{end_date} because provider returned no prices")
       return 0
     end
 

app/models/sync.rb

@@ -95,6 +95,29 @@ class Sync < ApplicationRecord
     parent&.finalize_if_all_children_finalized
   end
 
+  # If a sync is pending, we can adjust the window if new syncs are created with a wider window.
+  def expand_window_if_needed(new_window_start_date, new_window_end_date)
+    return unless pending?
+    return if self.window_start_date.nil? && self.window_end_date.nil? # already as wide as possible
+
+    earliest_start_date = if self.window_start_date && new_window_start_date
+      [self.window_start_date, new_window_start_date].min
+    else
+      nil
+    end
+
+    latest_end_date = if self.window_end_date && new_window_end_date
+      [self.window_end_date, new_window_end_date].max
+    else
+      nil
+    end
+
+    update(
+      window_start_date: earliest_start_date,
+      window_end_date: latest_end_date
+    )
+  end
+
   private
     def log_status_change
       Rails.logger.info("changing from #{aasm.from_state} to #{aasm.to_state} (event: #{aasm.current_event})")

app/views/rule/conditions/_condition_group.html.erb

@@ -28,13 +28,13 @@
 
   <%# Sub-condition template, used by Stimulus controller to add new sub-conditions dynamically %>
   <template data-rule--conditions-target="subConditionTemplate">
-    <%= form.fields_for :sub_conditions, Rule::Condition.new(parent: condition, condition_type: rule.condition_filters.first.key), child_index: "IDX_PLACEHOLDER" do |scf| %>
+    <%= form.fields_for :sub_conditions, Rule::Condition.new(parent: condition, condition_type: rule.condition_filters.first.key), child_index: "IDX_CHILD_PLACEHOLDER" do |scf| %>
       <%= render "rule/conditions/condition", form: scf, show_prefix: false %>
     <% end %>
   </template>
 
   <ul data-rule--conditions-target="subConditionsList" class="space-y-3">
-    <%= form.fields_for :sub_conditions do |scf| %>
+    <%= form.fields_for :sub_conditions, condition.sub_conditions.select(&:persisted?) do |scf| %>
       <%= render "rule/conditions/condition", form: scf, show_prefix: false %>
     <% end %>
   </ul>

test/controllers/concerns/auto_sync_test.rb

@@ -20,7 +20,7 @@ class AutoSyncTest < ActionDispatch::IntegrationTest
     travel_to Time.current.beginning_of_day
     last_sync_datetime = 1.hour.ago
 
-    Sync.create!(syncable: @family, created_at: last_sync_datetime)
+    Sync.create!(syncable: @family, created_at: last_sync_datetime, status: "completed")
 
     assert_difference "Sync.count", 1 do
       get root_path
@@ -32,7 +32,7 @@ class AutoSyncTest < ActionDispatch::IntegrationTest
 
     last_created_sync_at = 23.hours.ago
 
-    Sync.create!(syncable: @family, created_at: last_created_sync_at)
+    Sync.create!(syncable: @family, created_at: last_created_sync_at, status: "completed")
 
     assert_no_difference "Sync.count" do
       get root_path

test/interfaces/syncable_interface_test.rb

@@ -18,11 +18,19 @@ module SyncableInterfaceTest
     @syncable.perform_sync(mock_sync)
   end
 
-  test "any prior syncs for the same syncable entity are marked stale when new sync is requested" do
-    stale_sync = @syncable.sync_later
-    new_sync = @syncable.sync_later
+  test "second sync request widens existing pending window" do
+    later_start = 2.days.ago.to_date
+    first_sync = @syncable.sync_later(window_start_date: later_start, window_end_date: later_start)
 
-    assert_equal "stale", stale_sync.reload.status
-    assert_equal "pending", new_sync.reload.status
+    earlier_start = 5.days.ago.to_date
+    wider_end     = Date.current
+
+    assert_no_difference "@syncable.syncs.count" do
+      @syncable.sync_later(window_start_date: earlier_start, window_end_date: wider_end)
+    end
+
+    first_sync.reload
+    assert_equal earlier_start, first_sync.window_start_date
+    assert_equal wider_end, first_sync.window_end_date
   end
 end

test/models/rule/action_test.rb

@@ -21,7 +21,7 @@ class Rule::ActionTest < ActiveSupport::TestCase
 
   test "set_transaction_category" do
     # Does not modify transactions that are locked (user edited them)
-    @txn1.lock!(:category_id)
+    @txn1.lock_attr!(:category_id)
 
     action = Rule::Action.new(
       rule: @transaction_rule,
@@ -42,7 +42,7 @@ class Rule::ActionTest < ActiveSupport::TestCase
     tag = @family.tags.create!(name: "Rule test tag")
 
     # Does not modify transactions that are locked (user edited them)
-    @txn1.lock!(:tag_ids)
+    @txn1.lock_attr!(:tag_ids)
 
     action = Rule::Action.new(
       rule: @transaction_rule,
@@ -63,7 +63,7 @@ class Rule::ActionTest < ActiveSupport::TestCase
     merchant = @family.merchants.create!(name: "Rule test merchant")
 
     # Does not modify transactions that are locked (user edited them)
-    @txn1.lock!(:merchant_id)
+    @txn1.lock_attr!(:merchant_id)
 
     action = Rule::Action.new(
       rule: @transaction_rule,
@@ -84,7 +84,7 @@ class Rule::ActionTest < ActiveSupport::TestCase
     new_name = "Renamed Transaction"
 
     # Does not modify transactions that are locked (user edited them)
-    @txn1.lock!(:name)
+    @txn1.lock_attr!(:name)
 
     action = Rule::Action.new(
       rule: @transaction_rule,

test/models/sync_test.rb

@@ -188,4 +188,24 @@ class SyncTest < ActiveSupport::TestCase
     assert_equal "stale", stale_pending.reload.status
     assert_equal "stale", stale_syncing.reload.status
   end
+
+  test "expand_window_if_needed widens start and end dates on a pending sync" do
+    initial_start = 1.day.ago.to_date
+    initial_end   = 1.day.ago.to_date
+
+    sync = Sync.create!(
+      syncable: accounts(:depository),
+      window_start_date: initial_start,
+      window_end_date: initial_end
+    )
+
+    new_start = 5.days.ago.to_date
+    new_end   = Date.current
+
+    sync.expand_window_if_needed(new_start, new_end)
+    sync.reload
+
+    assert_equal new_start, sync.window_start_date
+    assert_equal new_end,   sync.window_end_date
+  end
 end