Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix context shim override behavior #721

Merged

Conversation

henrique-ft
Copy link
Contributor

@henrique-ft henrique-ft commented Feb 29, 2024

I'll try to summarize the problem as much as possible:

The split has this class:

class ContextShim
  include Split::Helper
  public :ab_test, :ab_finished

  def initialize(context)
    @context = context
  end

  def ab_user
    @ab_user ||= Split::User.new(@context)
  end
end

Which includes:

include Split::Helper

If we use the context shim this way:

def split
  # self is a class that has the methods 'params' and 'request'
  @split ||= ::Split::EncapsulatedHelper::ContextShim.new(self) 
end

inside a high level abstraction, we got a problem.

Problem

The override of ab via URL (?ab_test[my_test]=variant) and Split panel (cookies) doesn't work.
It doesn't work because Split::Helper uses these methods:

def override_alternative_by_params(experiment_name)
  defined?(params) && params[OVERRIDE_PARAM_NAME] && params[OVERRIDE_PARAM_NAME][experiment_name]
end

def override_alternative_by_cookies(experiment_name)
  return unless defined?(request)

  if request.cookies && request.cookies.key?("split_override")
    experiments = JSON.parse(request.cookies["split_override"]) rescue {}
    experiments[experiment_name]
  end
end

Which depend on defined?(params) and defined?(request), which are in @context but not in the scope of ContextShim.
In other words:
If we add params and request to the scope of ContextShim, we can solve the problem.

Solution

Include these methods in ContextShim:

def params
  request.params if request_present?
end

def request
  @context.request if @context.respond_to?(:request)
end

I also create a request_present? and params_present? methods in Split::Helper to avoid problems when request or params are defined but returning nil: This is the case when @context don't implement request or params

end

it "should not raise an error " do
expect { params }.to raise_error(NoMethodError)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this expect... it will aways raise this error due to

def params # previous version
  raise NoMethodError, "This method is not really defined"
end

or

allow(self).to receive(:params).and_raise(NoMethodError)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and this definitions are inside the spec

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed this on #723

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also checked if we were re-introducing #257

Which was why the NoMethodError was added in first place... Tested in Rails 7+ and it was ok.

@andrehjr
Copy link
Member

andrehjr commented Mar 3, 2024

I'll merge this as-is. I'm making some changes on a new PR. Not sure why github actions didn't run on this PR 🤔

Main branch was broken due some other issue with newer rack. I've fixed that too.

@andrehjr andrehjr merged commit 7a075be into splitrb:main Mar 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants