Merge pull request #721 from henrique-ft/fix-context-shim-override-behavior

Fix context shim override behavior

Author: andrehjr

lib/split/encapsulated_helper.rb

@@ -23,6 +23,14 @@ def initialize(context)
         @context = context
       end
 
+      def params
+        request.params if request_present?
+      end
+
+      def request
+        @context.request if @context.respond_to?(:request)
+      end
+
       def ab_user
         @ab_user ||= Split::User.new(@context)
       end

lib/split/helper.rb

@@ -121,11 +121,11 @@ def override_alternative(experiment_name)
     end
 
     def override_alternative_by_params(experiment_name)
-      defined?(params) && params[OVERRIDE_PARAM_NAME] && params[OVERRIDE_PARAM_NAME][experiment_name]
+      params_present? && params[OVERRIDE_PARAM_NAME] && params[OVERRIDE_PARAM_NAME][experiment_name]
     end
 
     def override_alternative_by_cookies(experiment_name)
-      return unless defined?(request)
+      return unless request_present?
 
       if request.cookies && request.cookies.key?("split_override")
         experiments = JSON.parse(request.cookies["split_override"]) rescue {}
@@ -134,34 +134,42 @@ def override_alternative_by_cookies(experiment_name)
     end
 
     def split_generically_disabled?
-      defined?(params) && params["SPLIT_DISABLE"]
+      params_present? && params["SPLIT_DISABLE"]
     end
 
     def ab_user
       @ab_user ||= User.new(self)
     end
 
     def exclude_visitor?
-      defined?(request) && (instance_exec(request, &Split.configuration.ignore_filter) || is_ignored_ip_address? || is_robot? || is_preview?)
+      request_present? && (instance_exec(request, &Split.configuration.ignore_filter) || is_ignored_ip_address? || is_robot? || is_preview?)
     end
 
     def is_robot?
-      defined?(request) && request.user_agent =~ Split.configuration.robot_regex
+      request_present? && request.user_agent =~ Split.configuration.robot_regex
     end
 
     def is_preview?
-      defined?(request) && defined?(request.headers) && request.headers["x-purpose"] == "preview"
+      request_present? && defined?(request.headers) && request.headers["x-purpose"] == "preview"
     end
 
     def is_ignored_ip_address?
       return false if Split.configuration.ignore_ip_addresses.empty?
 
       Split.configuration.ignore_ip_addresses.each do |ip|
-        return true if defined?(request) && (request.ip == ip || (ip.class == Regexp && request.ip =~ ip))
+        return true if request_present? && (request.ip == ip || (ip.class == Regexp && request.ip =~ ip))
       end
       false
     end
 
+    def params_present?
+      defined?(params) && params != nil
+    end
+
+    def request_present?
+      defined?(request) && request != nil
+    end
+
     def active_experiments
       ab_user.active_experiments
     end

spec/encapsulated_helper_spec.rb

@@ -5,19 +5,21 @@
 describe Split::EncapsulatedHelper do
   include Split::EncapsulatedHelper
 
-  def params
-    raise NoMethodError, "This method is not really defined"
-  end
-
   describe "ab_test" do
     before do
       allow_any_instance_of(Split::EncapsulatedHelper::ContextShim).to receive(:ab_user)
       .and_return(mock_user)
     end
 
-    it "should not raise an error when params raises an error" do
-      expect { params }.to raise_error(NoMethodError)
-      expect { ab_test("link_color", "blue", "red") }.not_to raise_error
+    context "when params raises an error" do
+      before do
+        allow(self).to receive(:params).and_raise(NoMethodError)
+      end
+
+      it "should not raise an error " do
+        expect { params }.to raise_error(NoMethodError)
+        expect { ab_test("link_color", "blue", "red") }.not_to raise_error
+      end
     end
 
     it "calls the block with selected alternative" do
@@ -43,8 +45,43 @@ def params
         include Split::EncapsulatedHelper
         public :session
       }.new
+
       expect(ctx).to receive(:session) { {} }
       expect { ctx.ab_test("link_color", "blue", "red") }.not_to raise_error
     end
+
+    context "when request is defined in context of ContextShim" do
+      context "when overriding by params" do
+        it do
+          ctx = Class.new {
+            public :session
+            def request
+              build_request(params: {
+                "ab_test" => { "link_color" => "blue" }
+              })
+            end
+          }.new
+
+          context_shim = Split::EncapsulatedHelper::ContextShim.new(ctx)
+          expect(context_shim.ab_test("link_color", "blue", "red")).to be("blue")
+        end
+      end
+
+      context "when overriding by cookies" do
+        it do
+          ctx = Class.new {
+            public :session
+            def request
+              build_request(cookies: {
+                "split_override" => '{ "link_color": "red" }'
+              })
+            end
+          }.new
+
+          context_shim = Split::EncapsulatedHelper::ContextShim.new(ctx)
+          expect(context_shim.ab_test("link_color", "blue", "red")).to be("red")
+        end
+      end
+    end
   end
 end

spec/spec_helper.rb

@@ -44,11 +44,20 @@ def params
   @params ||= {}
 end
 
-def request(ua = "Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; de-de) AppleWebKit/533.20.25 (KHTML, like Gecko) Version/5.0.4 Safari/533.20.27")
-  @request ||= begin
-    r = OpenStruct.new
-    r.user_agent = ua
-    r.ip = "192.168.1.1"
-    r
-  end
+def request
+  @request ||= build_request
+end
+
+def build_request(
+  ua: "Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_6; de-de) AppleWebKit/533.20.25 (KHTML, like Gecko) Version/5.0.4 Safari/533.20.27",
+  ip: "192.168.1.1",
+  params: {},
+  cookies: {}
+)
+  r = OpenStruct.new
+  r.user_agent = ua
+  r.ip = ip
+  r.params = params
+  r.cookies = cookies
+  r
 end