feat: Optimizely Factory Revised (#204)

* feat: Optimizely Factory Revised

* Line Endings

* address feedback

* use instance_variable_get

* revert private method names

Co-authored-by: msohailhussain <mirza.sohailhussain@gmail.com>

Author: oakbani

.rubocop_todo.yml

@@ -1,6 +1,6 @@
 # This configuration was generated by
 # `rubocop --auto-gen-config`
-# on 2017-12-06 17:48:00 +0500 using RuboCop version 0.51.0.
+# on 2020-04-28 01:01:46 +0500 using RuboCop version 0.60.0.
 # The point is for the user to remove these configuration records
 # one by one as the offenses are removed from the code base.
 # Note that changes in the inspected code, or installation of new
@@ -11,32 +11,18 @@ Lint/HandleExceptions:
   Exclude:
     - 'Rakefile'
 
-# Offense count: 1
-Lint/LiteralAsCondition:
-  Exclude:
-    - 'lib/optimizely/config/datafile_project_config.rb'
-
-# Offense count: 1
+# Offense count: 8
 # Configuration parameters: CountKeywordArgs.
 Metrics/ParameterLists:
-  Max: 6
-  Exclude:
-    - 'lib/optimizely.rb'
-    - 'lib/optimizely/config_manager/http_project_config_manager.rb'
-    - 'lib/optimizely/event/batch_event_processor.rb'
-    - 'lib/optimizely/event/entity/conversion_event.rb'
-    - 'lib/optimizely/event/entity/event_context.rb'
-    - 'lib/optimizely/event/entity/impression_event.rb'
-    - 'lib/optimizely/event/entity/snapshot_event.rb'
-    - 'lib/optimizely/optimizely_factory.rb'
+  Max: 12
 
+# Offense count: 2
 Naming/AccessorMethodName:
   Exclude:
-    - 'lib/optimizely/config_manager/http_project_config_manager.rb'
-    - 'spec/optimizely_factory_spec.rb'
     - 'lib/optimizely.rb'
+    - 'lib/optimizely/config_manager/http_project_config_manager.rb'
 
-# Offense count: 1
+# Offense count: 2
 # Configuration parameters: NamePrefix, NamePrefixBlacklist, NameWhitelist, MethodDefinitionMacros.
 # NamePrefix: is_, has_, have_
 # NamePrefixBlacklist: is_, has_, have_
@@ -46,3 +32,17 @@ Naming/PredicateName:
   Exclude:
     - 'spec/**/*'
     - 'lib/optimizely.rb'
+
+# Offense count: 2
+# Cop supports --auto-correct.
+# Configuration parameters: ExactNameMatch, AllowPredicates, AllowDSLWriters, IgnoreClassMethods, Whitelist.
+# Whitelist: to_ary, to_a, to_c, to_enum, to_h, to_hash, to_i, to_int, to_io, to_open, to_path, to_proc, to_r, to_regexp, to_str, to_s, to_sym
+Style/TrivialAccessors:
+  Exclude:
+    - 'lib/optimizely/optimizely_factory.rb'
+
+# Offense count: 2465
+# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns.
+# URISchemes: http, https
+Metrics/LineLength:
+  Max: 215

lib/optimizely/optimizely_factory.rb

@@ -17,16 +17,18 @@
 #
 
 require 'optimizely'
+require 'optimizely/error_handler'
 require 'optimizely/event_dispatcher'
 require 'optimizely/event/batch_event_processor'
+require 'optimizely/logger'
+require 'optimizely/notification_center'
+
 module Optimizely
   class OptimizelyFactory
-    attr_reader :max_event_batch_size, :max_event_flush_interval
-
     # Convenience method for setting the maximum number of events contained within a batch.
     # @param batch_size Integer - Sets size of EventQueue.
     # @param logger - Optional LoggerInterface Provides a log method to log messages.
-    def self.max_event_batch_size(batch_size, logger)
+    def self.max_event_batch_size(batch_size, logger = NoOpLogger.new)
       unless batch_size.is_a? Integer
         logger.log(
           Logger::ERROR,
@@ -48,7 +50,7 @@ def self.max_event_batch_size(batch_size, logger)
     # Convenience method for setting the maximum time interval in milliseconds between event dispatches.
     # @param flush_interval Numeric - Time interval between event dispatches.
     # @param logger - Optional LoggerInterface Provides a log method to log messages.
-    def self.max_event_flush_interval(flush_interval, logger)
+    def self.max_event_flush_interval(flush_interval, logger = NoOpLogger.new)
       unless flush_interval.is_a? Numeric
         logger.log(
           Logger::ERROR,
@@ -67,12 +69,42 @@ def self.max_event_flush_interval(flush_interval, logger)
       @max_event_flush_interval = flush_interval
     end
 
+    # Convenience method for setting frequency at which datafile has to be polled and ProjectConfig updated.
+    #
+    # @param polling_interval Numeric - Time in seconds after which to update datafile.
+    def self.polling_interval(polling_interval)
+      @polling_interval = polling_interval
+    end
+
+    # Convenience method for setting timeout to block the config call until config has been initialized.
+    #
+    # @param blocking_timeout Numeric - Time in seconds.
+    def self.blocking_timeout(blocking_timeout)
+      @blocking_timeout = blocking_timeout
+    end
+
     # Returns a new optimizely instance.
     #
     # @params sdk_key - Required String uniquely identifying the fallback datafile corresponding to project.
     # @param fallback datafile - Optional JSON string datafile.
     def self.default_instance(sdk_key, datafile = nil)
-      Optimizely::Project.new(datafile, nil, nil, nil, nil, nil, sdk_key)
+      error_handler = NoOpErrorHandler.new
+      logger = NoOpLogger.new
+      notification_center = NotificationCenter.new(logger, error_handler)
+
+      config_manager = Optimizely::HTTPProjectConfigManager.new(
+        sdk_key: sdk_key,
+        polling_interval: @polling_interval,
+        blocking_timeout: @blocking_timeout,
+        datafile: datafile,
+        logger: logger,
+        error_handler: error_handler,
+        notification_center: notification_center
+      )
+
+      Optimizely::Project.new(
+        datafile, nil, logger, error_handler, nil, nil, sdk_key, config_manager, notification_center
+      )
     end
 
     # Returns a new optimizely instance.
@@ -108,10 +140,27 @@ def self.custom_instance(
       config_manager = nil,
       notification_center = nil
     )
+
+      error_handler ||= NoOpErrorHandler.new
+      logger ||= NoOpLogger.new
+      notification_center = notification_center.is_a?(Optimizely::NotificationCenter) ? notification_center : NotificationCenter.new(logger, error_handler)
+
       event_processor = BatchEventProcessor.new(
         event_dispatcher: event_dispatcher || EventDispatcher.new,
         batch_size: @max_event_batch_size,
         flush_interval: @max_event_flush_interval,
+        logger: logger,
+        notification_center: notification_center
+      )
+
+      config_manager ||= Optimizely::HTTPProjectConfigManager.new(
+        sdk_key: sdk_key,
+        polling_interval: @polling_interval,
+        blocking_timeout: @blocking_timeout,
+        datafile: datafile,
+        logger: logger,
+        error_handler: error_handler,
+        skip_json_validation: skip_json_validation,
         notification_center: notification_center
       )
 

spec/optimizely_factory_spec.rb

@@ -30,22 +30,38 @@
   let(:event_dispatcher) { Optimizely::EventDispatcher.new }
   let(:notification_center) { Optimizely::NotificationCenter.new(spy_logger, error_handler) }
 
-  describe '.default_instance' do
-    it 'should take http config manager' do
-      allow(Optimizely::HTTPProjectConfigManager).to receive(:new)
-
-      http_project_config_manager = Optimizely::HTTPProjectConfigManager.new(
-        sdk_key: 'sdk_key',
-        datafile: datafile,
-        logger: spy_logger,
-        error_handler: error_handler,
-        skip_json_validation: false,
-        notification_center: notification_center
+  before(:example) do
+    WebMock.allow_net_connect!
+    stub_request(:get, 'https://cdn.optimizely.com/datafiles/sdk_key.json')
+      .with(
+        headers: {
+          'Content-Type' => 'application/json'
+        }
       )
+      .to_return(status: 200, body: '', headers: {})
+  end
+
+  describe '.default_instance' do
+    it 'should create http config manager when sdk_key is given' do
+      optimizely_instance = Optimizely::OptimizelyFactory.default_instance('sdk_key', datafile)
+      expect(optimizely_instance.config_manager). to be_instance_of(Optimizely::HTTPProjectConfigManager)
+    end
 
+    it 'should create http config manager when polling interval and blocking timeout are set' do
+      Optimizely::OptimizelyFactory.polling_interval(40)
+      Optimizely::OptimizelyFactory.blocking_timeout(5)
       optimizely_instance = Optimizely::OptimizelyFactory.default_instance('sdk_key', datafile)
 
-      expect(optimizely_instance.config_manager). to eq(http_project_config_manager)
+      # Verify that values set in OptimizelyFactory are being used inside config manager.
+      expect(optimizely_instance.config_manager.instance_variable_get(:@polling_interval)). to eq(40)
+      expect(optimizely_instance.config_manager.instance_variable_get(:@blocking_timeout)). to eq(5)
+    end
+
+    it 'should create http config manager with the same components as the instance' do
+      optimizely_instance = Optimizely::OptimizelyFactory.default_instance('sdk_key', datafile)
+      expect(optimizely_instance.error_handler). to be(optimizely_instance.config_manager.instance_variable_get(:@error_handler))
+      expect(optimizely_instance.logger). to be(optimizely_instance.config_manager.instance_variable_get(:@logger))
+      expect(optimizely_instance.notification_center). to be(optimizely_instance.config_manager.instance_variable_get(:@notification_center))
     end
   end
 
@@ -57,61 +73,63 @@ class CustomConfigManager
 
       custom_config_manager = CustomConfigManager.new
       optimizely_instance = Optimizely::OptimizelyFactory.default_instance_with_config_manager(custom_config_manager)
-      expect(optimizely_instance.config_manager). to eq(custom_config_manager)
+      expect(optimizely_instance.config_manager). to be(custom_config_manager)
     end
   end
 
   describe '.custom_instance' do
-    it 'should take http config manager when sdk key is given' do
-      allow(Optimizely::HTTPProjectConfigManager).to receive(:new)
-      http_project_config_manager = Optimizely::HTTPProjectConfigManager.new(
-        sdk_key: 'sdk_key',
-        datafile: datafile,
-        logger: spy_logger,
-        error_handler: error_handler,
-        skip_json_validation: false,
-        notification_center: notification_center
-      )
-
+    it 'should take http config manager when sdk key, polling interval, blocking timeout are given' do
+      Optimizely::OptimizelyFactory.polling_interval(50)
+      Optimizely::OptimizelyFactory.blocking_timeout(10)
       optimizely_instance = Optimizely::OptimizelyFactory.custom_instance(
         'sdk_key',
         datafile,
         event_dispatcher,
-        spy_logger,
+        Optimizely::NoOpLogger.new,
         error_handler,
         false,
         user_profile_service,
         nil,
         notification_center
       )
-      expect(optimizely_instance.config_manager). to eq(http_project_config_manager)
+
+      # Verify that values set in OptimizelyFactory are being used inside config manager.
+      expect(optimizely_instance.config_manager.instance_variable_get(:@polling_interval)). to eq(50)
+      expect(optimizely_instance.config_manager.instance_variable_get(:@blocking_timeout)). to eq(10)
     end
 
     it 'should take event processor when flush interval and batch size are set' do
-      Optimizely::OptimizelyFactory.max_event_flush_interval(5, spy_logger)
-      Optimizely::OptimizelyFactory.max_event_batch_size(100, spy_logger)
-
-      event_processor = Optimizely::BatchEventProcessor.new(
-        event_dispatcher: event_dispatcher,
-        batch_size: 100,
-        flush_interval: 5,
-        notification_center: notification_center
-      )
+      Optimizely::OptimizelyFactory.max_event_flush_interval(5)
+      Optimizely::OptimizelyFactory.max_event_batch_size(100)
 
+      optimizely_instance = Optimizely::OptimizelyFactory.custom_instance('sdk_key')
+
+      expect(optimizely_instance.event_processor.flush_interval).to eq(5)
+      expect(optimizely_instance.event_processor.batch_size).to eq(100)
+      optimizely_instance.close
+    end
+
+    it 'should assign passed components to both the instance and http manager' do
+      logger = Optimizely::NoOpLogger.new
       optimizely_instance = Optimizely::OptimizelyFactory.custom_instance(
         'sdk_key',
         datafile,
         event_dispatcher,
-        spy_logger,
+        logger,
         error_handler,
         false,
         user_profile_service,
         nil,
         notification_center
       )
 
-      expect(optimizely_instance.event_processor).equal? event_processor
-      optimizely_instance.close
+      expect(error_handler). to be(optimizely_instance.config_manager.instance_variable_get(:@error_handler))
+      expect(logger). to be(optimizely_instance.config_manager.instance_variable_get(:@logger))
+      expect(notification_center). to be(optimizely_instance.config_manager.instance_variable_get(:@notification_center))
+
+      expect(error_handler). to be(optimizely_instance.error_handler)
+      expect(logger). to be(optimizely_instance.logger)
+      expect(notification_center). to be(optimizely_instance.notification_center)
     end
   end