Remove without_callback

.rubocop_todo.yml

@@ -354,7 +354,6 @@ Layout/MultilineMethodCallBraceLayout:
 Layout/MultilineMethodCallIndentation:
   Exclude:
     - 'app/api/v01/routes.rb'
-    - 'config/initializers/without_callback.rb'
     - 'test/api/v01/devices/fleet_reporting_test.rb'
     - 'test/api/v01/zonings_test.rb'
     - 'test/controllers/zonings_controller_test.rb'
@@ -674,14 +673,6 @@ Lint/ParenthesesAsGroupedExpression:
     - 'app/views/stops/_show.json.jbuilder'
     - 'db/migrate/20160201165010_split_table_destination_to_visit.rb'
 
-# Offense count: 1
-# This cop supports unsafe autocorrection (--autocorrect-all).
-# Configuration parameters: AllowedImplicitNamespaces.
-# AllowedImplicitNamespaces: Gem
-Lint/RaiseException:
-  Exclude:
-    - 'config/initializers/without_callback.rb'
-
 # Offense count: 1
 # This cop supports unsafe autocorrection (--autocorrect-all).
 Lint/RedundantRequireStatement:
@@ -1728,7 +1719,6 @@ Style/SoleNestedConditional:
     - 'app/models/vehicle.rb'
     - 'app/views/routes/_edit.json.jbuilder'
     - 'app/views/stops/_show.json.jbuilder'
-    - 'config/initializers/without_callback.rb'
     - 'lib/notifications.rb'
 
 # Offense count: 1

CHANGELOG.md

@@ -21,6 +21,7 @@
     - Increase zone border weight & Change edit marker style [#207](https://github.com/cartoway/planner-web/pull/207)
 
   ### Removed
+  - `skip_callback` was not thread safe [#214](https://github.com/cartoway/planner-web/pull/214)
 
   ### Fixed
   - Api-web:  Remove blank header bar [#201](https://github.com/cartoway/planner-web/pull/201)

app/jobs/importer_base.rb

@@ -43,55 +43,49 @@ def import(data, name, synchronous, options)
     @synchronous = synchronous
     dests = false
     refs = Hash.new
-    Destination.without_callback(:create, :before, :check_max_destination) do
-      Destination.without_callback(:validation, :before, :update_geocode) do
-        VehicleUsageSet.without_callback(:create, :before, :check_max_vehicle_usage_set) do
-          Customer.transaction do
-            before_import(name, data, options)
-
-            dests = data.map.with_index{ |row, line|
-              # Switch from locale or custom to internal column name in case of csv
-              row = yield(row, line + 1 + (options[:line_shift] || 0))
-
-              next if row.empty? # Skip empty line
-
-              begin
-                if (ref = uniq_ref(row))
-                  if refs.key?(ref)
-                    raise ImportInvalidRef.new(I18n.t("destinations.import_file.#{ref.is_a?(Array) && ref[0].nil? ? 'refs_visit_duplicate' : 'refs_duplicate'}", refs: ref.is_a?(Array) ? ref.compact.join(' | ') : ref))
-                  elsif !options[:allow_duplicate]
-                    refs[ref] = nil
-                  end
-                end
-
-                dest = import_row(name, row, line, options)
-                if dest.nil?
-                  next
-                end
-
-                if !@synchronous && Mapotempo::Application.config.delayed_job_use && dest.respond_to?(:delay_geocode)
-                  dest.delay_geocode
-                end
-                dest
-              rescue ImportBaseError => e
-                if options[:ignore_errors]
-                  @warnings << e unless @warnings.include?(e)
-                else
-                  raise
-                end
-              end
-            }
-            raise ImportEmpty.new I18n.t('import.empty') if dests.all?(&:nil?)
-            yield(nil)
-
-            options[:dests] = dests
-
-            after_import(name, options)
-
-            finalize_import(name, options)
+    Customer.transaction do
+      before_import(name, data, options)
+
+      dests = data.map.with_index{ |row, line|
+        # Switch from locale or custom to internal column name in case of csv
+        row = yield(row, line + 1 + (options[:line_shift] || 0))
+
+        next if row.empty? # Skip empty line
+
+        begin
+          if (ref = uniq_ref(row))
+            if refs.key?(ref)
+              raise ImportInvalidRef.new(I18n.t("destinations.import_file.#{ref.is_a?(Array) && ref[0].nil? ? 'refs_visit_duplicate' : 'refs_duplicate'}", refs: ref.is_a?(Array) ? ref.compact.join(' | ') : ref))
+            elsif !options[:allow_duplicate]
+              refs[ref] = nil
+            end
+          end
+
+          dest = import_row(name, row, line, options)
+          if dest.nil?
+            next
+          end
+
+          if !@synchronous && Mapotempo::Application.config.delayed_job_use && dest.respond_to?(:delay_geocode)
+            dest.delay_geocode
+          end
+          dest
+        rescue ImportBaseError => e
+          if options[:ignore_errors]
+            @warnings << e unless @warnings.include?(e)
+          else
+            raise
           end
         end
-      end
+      }
+      raise ImportEmpty.new I18n.t('import.empty') if dests.all?(&:nil?)
+      yield(nil)
+
+      options[:dests] = dests
+
+      after_import(name, options)
+
+      finalize_import(name, options)
     end
 
     dests

app/jobs/importer_destinations.rb

@@ -176,7 +176,7 @@ def before_import(_name, data, options)
     @existing_destinations_by_ref = {}
     @existing_visits_by_ref = {}
     @destinations_visits_attributes_by_ref = {}
-    @customer.destinations.where.not(ref: nil).find_each{ |destination|
+    @customer.destinations.includes_visits.where.not(ref: nil).find_each{ |destination|
       @existing_destinations_by_ref[destination.ref.to_sym] = destination
       @existing_visits_by_ref[destination.ref.to_sym] = Hash[destination.visits.map{ |visit| [visit.ref&.to_sym, visit]}]
       @destinations_visits_attributes_by_ref[destination.ref.to_sym] = Hash.new
@@ -498,7 +498,8 @@ def bulk_import_destinations(destination_index_attributes_hash)
         import_result = Destination.import(
           destinations_attributes,
           on_duplicate_key_update: { conflict_target: [:id], columns: :all },
-          validate: true, all_or_none: true, track_validation_failures: true
+          validate: true, all_or_none: true, track_validation_failures: true,
+          validate_with_context: :import
         )
 
         raise ImportBulkError.new(import_errors_with_indices(slice_lines, slice_index, import_result.failed_instances)) if import_result.failed_instances.any?

app/jobs/importer_vehicle_usage_sets.rb

@@ -243,6 +243,7 @@ def after_import(_name, _options)
       @vehicle_usage_set.vehicle_usages.each{ |vu|
         vu.assign_attributes Hash[@common_configuration.keys.select{ |k| excluded_keys.exclude? k }.map{ |k| [k, nil] }]
       }
+      @vehicle_usage_set.import_skip = true
       @vehicle_usage_set.save!
     end
 

app/models/customer.rb

@@ -46,7 +46,7 @@ class Customer < ApplicationRecord
   has_many :stops_relations, inverse_of: :customer, autosave: true, dependent: :delete_all
   enum router_dimension: Router::DIMENSION
 
-  attr_accessor :deliverable_units_updated, :device, :exclude_users
+  attr_accessor :deliverable_units_updated, :device, :exclude_users, :migration_skip
 
   include HashBoolAttr
   store_accessor :router_options, :time, :distance, :avoid_zones, :isochrone, :isodistance, :traffic, :track, :motorway, :toll, :low_emission_zone, :trailers, :weight, :weight_per_axle, :height, :width, :length, :hazardous_goods, :max_walk_distance, :approach, :snap, :strict_restriction
@@ -100,7 +100,8 @@ class Customer < ApplicationRecord
   before_save :devices_update_vehicles, prepend: true
   after_create :create_default_store, :create_default_vehicle_usage_set, :create_default_deliverable_unit
 
-  before_update :update_max_vehicles, :update_enable_multi_visits, :update_outdated
+  before_update :update_max_vehicles, :update_enable_multi_visits
+  before_update :update_outdated, unless: :migration_skip
 
   include RefSanitizer
 
@@ -486,15 +487,13 @@ def update_enable_multi_visits
             destination.visits.each{ |visit|
               visit.ref ||= destination.ref
               visit.tags |= destination.tags
+              visit.internal_skip = true
             }
             destination.ref = nil
             destination.tag_ids = []
             # Don't load all plans to update them...
-            Destination.without_callback(:save, :before, :update_tags) do
-              Visit.without_callback(:save, :before, :update_tags) do
-                destination.save!
-              end
-            end
+            destination.internal_skip = true
+            destination.save!
           }
         else
           self.destinations.each{ |destination|
@@ -504,13 +503,11 @@ def update_enable_multi_visits
               destination.visits.each{ |visit|
                 visit.ref = nil
                 visit.tag_ids = []
+                visit.internal_skip = true
               }
               # Don't load all plans to update them...
-              Destination.without_callback(:save, :before, :update_tags) do
-                Visit.without_callback(:save, :before, :update_tags) do
-                  destination.save!
-                end
-              end
+              destination.internal_skip = true
+              destination.save!
             end
           }
         end

app/models/destination.rb

@@ -18,6 +18,9 @@
 class Destination < Location
   default_scope { order(:id) }
 
+  attr_accessor :internal_skip
+
+  belongs_to :customer
   has_many :visits, inverse_of: :destination, dependent: :delete_all
   accepts_nested_attributes_for :visits, allow_destroy: true
   has_many :tag_destinations
@@ -29,7 +32,8 @@ class Destination < Location
   validate_consistency [:tags]
 
   before_create :check_max_destination
-  before_save :save_visits, :update_tags
+  before_save :save_visits
+  before_save :update_tags, unless: :internal_skip
   after_save -> { @tag_ids_changed = false }
 
   include RefSanitizer

app/models/location.rb

@@ -44,7 +44,7 @@ class Location < ApplicationRecord
   validates_inclusion_of :geocoding_accuracy, in: 0..1, allow_nil: true, message: ->(*_) { I18n.t('activerecord.errors.models.location.geocoding_accuracy_outside_range') }
   validates_with LocalizationValidator, fields: [:street, :city, :lat, :lng]
 
-  before_validation :update_geocode
+  before_validation :update_geocode, unless: -> { validation_context == :import }
   before_update :update_outdated
 
   def position?

app/models/planning.rb

@@ -638,10 +638,8 @@ def fetch_stops_status
                 end
               end
 
-              Visit.without_callback(:update, :before, :update_outdated) do
-                # Do not flag route as outdated just for quantities change, route quantities are computed after loop
-                stops_map[s[:order_id]].visit.update(quantities: quantities)
-              end
+              # Do not flag route as outdated just for quantities change, route quantities are computed after loop
+              stops_map[s[:order_id]].visit.update(quantities: quantities, outdate_skip: true)
               routes_quantities_changed << stops_map[s[:order_id]].route
             end
 

app/models/route.rb

@@ -18,6 +18,8 @@
 class Route < ApplicationRecord
   RELATION_ORDER_KEYS = %i[pickup_delivery order sequence]
 
+  attr_accessor :migration_skip
+
   belongs_to :planning
   belongs_to :vehicle_usage, optional: true
   has_many :stops, inverse_of: :route, autosave: true, dependent: :delete_all, after_add: :update_stops_track, after_remove: :update_stops_track
@@ -34,7 +36,7 @@ class Route < ApplicationRecord
   attribute :end, ScheduleType.new
   time_attr :start, :end
 
-  before_update :update_vehicle_usage, :update_geojson
+  before_update :update_vehicle_usage, :update_geojson, unless: :migration_skip
 
   after_initialize :assign_defaults, if: -> { new_record? }
   after_create :complete_geojson

app/models/tag.rb

@@ -18,6 +18,8 @@
 class Tag < ApplicationRecord
   ICON_SIZE = %w(small medium large).freeze
 
+  attr_accessor :migration_skip
+
   default_scope { order(:label) }
 
   belongs_to :customer
@@ -52,7 +54,7 @@ class Tag < ApplicationRecord
 
   include RefSanitizer
 
-  before_update :update_outdated
+  before_update :update_outdated, unless: :migration_skip
   before_destroy :set_routes
   after_destroy :reset_routes_geojson_point
 

app/models/vehicle.rb

@@ -24,6 +24,8 @@ class Vehicle < ApplicationRecord
 
   default_scope { order(:id) }
 
+  attr_accessor :migration_skip
+
   belongs_to :customer
   belongs_to :router, optional: true
   has_many :vehicle_usages, inverse_of: :vehicle, dependent: :destroy, autosave: true
@@ -61,7 +63,8 @@ class Vehicle < ApplicationRecord
   before_validation :check_router_options_format
   before_create :create_vehicle_usage
   before_save :nilify_router_options_blanks
-  before_update :update_outdated, :update_color
+  before_update :update_color
+  before_update :update_outdated, unless: :migration_skip
 
   include Consistency
   validate_consistency [:tags]

app/models/vehicle_usage_set.rb

@@ -18,6 +18,8 @@
 class VehicleUsageSet < ApplicationRecord
   default_scope { order(:id) }
 
+  attr_accessor :import_skip
+
   belongs_to :customer, inverse_of: :vehicle_usage_sets
   belongs_to :store_start, class_name: 'Store', inverse_of: :vehicle_usage_set_starts, optional: true
   belongs_to :store_stop, class_name: 'Store', inverse_of: :vehicle_usage_set_stops, optional: true
@@ -57,7 +59,7 @@ class VehicleUsageSet < ApplicationRecord
   validates :max_ride_distance, numericality: true, allow_nil: true
 
   after_initialize :assign_defaults, if: :new_record?
-  before_create :check_max_vehicle_usage_set
+  before_create :check_max_vehicle_usage_set, unless: :import_skip
   before_update :update_outdated
 
   amoeba do

app/models/visit.rb

@@ -44,6 +44,8 @@ class Visit < ApplicationRecord
     always_final: 3
   }
 
+  attr_accessor :internal_skip, :outdate_skip
+
   include TimeAttr
   attribute :time_window_start_1, ScheduleType.new
   attribute :time_window_end_1, ScheduleType.new
@@ -65,8 +67,9 @@ class Visit < ApplicationRecord
   include Consistency
   validate_consistency([:tags]) { |visit| visit.destination.try :customer_id }
 
-  before_save :update_tags, :create_orders, :update_quantities
-  before_update :update_outdated
+  before_save :update_tags, unless: :internal_skip
+  before_save :create_orders, :update_quantities
+  before_update :update_outdated, unless: :outdate_skip
   after_save -> { @tag_ids_changed = false }
 
   include RefSanitizer