Improve Ergonomics of Rollout::Feature#active?

This changes signature of `Rollout::Feature#active?` to not require
`rollout` instance as first argument.

When querying same feature for different users, it was required to do
something like:

```ruby
feature = $rollout.get(:foo)
users.each do |user|
   puts feature.active?($rollout, user)
end
```

This PR changes API so it's possible to just do:

```ruby
feature = $rollout.get(:foo)
users.each do |user|
   puts feature.active?(user)
end
```

----

Note: This API was never advertised but just to be sure, we just bump
major version when releasing this change.

lib/rollout.rb

@@ -107,7 +107,7 @@ def define_group(group, &block)
 
   def active?(feature, user = nil)
     feature = get(feature)
-    feature.active?(self, user)
+    feature.active?(user)
   end
 
   def user_in_active_users?(feature, user = nil)
@@ -138,7 +138,7 @@ def active_in_group?(group, user)
 
   def get(feature)
     string = @storage.get(key(feature))
-    Feature.new(feature, string, @options)
+    Feature.new(feature, state: string, rollout: self, options: @options)
   end
 
   def set_feature_data(feature, data)
@@ -157,7 +157,13 @@ def multi_get(*features)
     return [] if features.empty?
 
     feature_keys = features.map { |feature| key(feature) }
-    @storage.mget(*feature_keys).map.with_index { |string, index| Feature.new(features[index], string, @options) }
+
+    @storage
+      .mget(*feature_keys)
+      .map
+      .with_index do |string, index|
+        Feature.new(features[index], state: string, rollout: self, options: @options)
+      end
   end
 
   def features
@@ -166,13 +172,13 @@ def features
 
   def feature_states(user = nil)
     multi_get(*features).each_with_object({}) do |f, hash|
-      hash[f.name] = f.active?(self, user)
+      hash[f.name] = f.active?(user)
     end
   end
 
   def active_features(user = nil)
     multi_get(*features).select do |f|
-      f.active?(self, user)
+      f.active?(user)
     end.map(&:name)
   end
 
@@ -199,7 +205,7 @@ def with_feature(feature)
     f = get(feature)
 
     if count_observers > 0
-      before = Marshal.load(Marshal.dump(f))
+      before = f.deep_clone
       yield(f)
       save(f)
       changed

lib/rollout/feature.rb

@@ -5,12 +5,13 @@ class Feature
     attr_accessor :groups, :users, :percentage, :data
     attr_reader :name, :options
 
-    def initialize(name, string = nil, opts = {})
-      @options = opts
-      @name    = name
+    def initialize(name, rollout:, state: nil, options: {})
+      @name = name
+      @rollout = rollout
+      @options = options
 
-      if string
-        raw_percentage, raw_users, raw_groups, raw_data = string.split('|', 4)
+      if state
+        raw_percentage, raw_users, raw_groups, raw_data = state.split('|', 4)
         @percentage = raw_percentage.to_f
         @users = users_from_string(raw_users)
         @groups = groups_from_string(raw_groups)
@@ -48,12 +49,12 @@ def clear
       @data = {}
     end
 
-    def active?(rollout, user)
+    def active?(user)
       if user
         id = user_id(user)
         user_in_percentage?(id) ||
           user_in_active_users?(id) ||
-          user_in_active_group?(user, rollout)
+          user_in_active_group?(user)
       else
         @percentage == 100
       end
@@ -72,6 +73,14 @@ def to_hash
       }
     end
 
+    def deep_clone
+      c = self.clone
+      c.instance_variable_set('@rollout', nil)
+      c = Marshal.load(Marshal.dump(c))
+      c.instance_variable_set('@rollot', @rollout)
+      c
+    end
+
     private
 
     def user_id(user)
@@ -98,9 +107,9 @@ def user_id_for_percentage(user)
       end
     end
 
-    def user_in_active_group?(user, rollout)
+    def user_in_active_group?(user)
       @groups.any? do |g|
-        rollout.active_in_group?(g, user)
+        @rollout.active_in_group?(g, user)
       end
     end
 

spec/rollout/feature_spec.rb

@@ -0,0 +1,54 @@
+require "spec_helper"
+
+describe "Rollout::Feature" do
+  let(:rollout) { Rollout.new(Redis.current) }
+
+  describe "#add_user" do
+    it "ids a user using id_user_by" do
+      user    = double("User", email: "[email protected]")
+      feature = Rollout::Feature.new(:chat, state: nil, rollout: rollout, options: { id_user_by: :email })
+      feature.add_user(user)
+      expect(user).to have_received :email
+    end
+  end
+
+  describe "#initialize" do
+    describe "when string does not exist" do
+      it 'clears feature attributes when string is not given' do
+        feature = Rollout::Feature.new(:chat, rollout: rollout)
+        expect(feature.groups).to be_empty
+        expect(feature.users).to be_empty
+        expect(feature.percentage).to eq 0
+        expect(feature.data).to eq({})
+      end
+
+      it 'clears feature attributes when string is nil' do
+        feature = Rollout::Feature.new(:chat, state: nil, rollout: rollout)
+        expect(feature.groups).to be_empty
+        expect(feature.users).to be_empty
+        expect(feature.percentage).to eq 0
+        expect(feature.data).to eq({})
+      end
+
+      it 'clears feature attributes when string is empty string' do
+        feature = Rollout::Feature.new(:chat, state: "", rollout: rollout)
+        expect(feature.groups).to be_empty
+        expect(feature.users).to be_empty
+        expect(feature.percentage).to eq 0
+        expect(feature.data).to eq({})
+      end
+
+      describe "when there is no data" do
+        it 'sets @data to empty hash' do
+          feature = Rollout::Feature.new(:chat, state: "0||", rollout: rollout)
+          expect(feature.data).to eq({})
+        end
+
+        it 'sets @data to empty hash' do
+          feature = Rollout::Feature.new(:chat, state: "|||   ", rollout: rollout)
+          expect(feature.data).to eq({})
+        end
+      end
+    end
+  end
+end