Merge pull request #244 from jnunemaker/groups

Allow Unregistered Groups

lib/flipper.rb

@@ -64,12 +64,9 @@ def self.group_exists?(name)
   #
   #   Flipper.group(:admins)
   #
-  # Returns the Flipper::Group if group registered.
-  # Raises Flipper::GroupNotRegistered if group is not registered.
+  # Returns Flipper::Group.
   def self.group(name)
-    groups_registry.get(name)
-  rescue Registry::KeyNotFound => e
-    raise GroupNotRegistered, "Group #{e.key.inspect} has not been registered"
+    groups_registry.get(name) || Types::Group.new(name)
   end
 
   # Internal: Registry of all groups_registry.

lib/flipper/api/v1/actions/groups_gate.rb

@@ -27,7 +27,23 @@ def delete
           private
 
           def ensure_valid_params
-            json_error_response(:group_not_registered) unless Flipper.group_exists?(group_name)
+            if group_name.nil? || group_name.empty?
+              json_error_response(:name_invalid)
+            end
+
+            return if allow_unregistered_groups?
+            return if Flipper.group_exists?(group_name)
+
+            json_error_response(:group_not_registered)
+          end
+
+          def allow_unregistered_groups?
+            allow_unregistered_groups = params['allow_unregistered_groups']
+            allow_unregistered_groups && allow_unregistered_groups == 'true'
+          end
+
+          def disallow_unregistered_groups?
+            !allow_unregistered_groups?
           end
 
           def feature_name

lib/flipper/registry.rb

@@ -46,9 +46,7 @@ def add(key, value)
     def get(key)
       key = key.to_sym
       @mutex.synchronize do
-        @source.fetch(key) do
-          raise KeyNotFound, key
-        end
+        @source[key]
       end
     end
 

lib/flipper/ui/actions/groups_gate.rb

@@ -25,17 +25,19 @@ def post
           feature = flipper[feature_name.to_sym]
           value = params['value'].to_s.strip
 
-          case params['operation']
-          when 'enable'
-            feature.enable_group value
-          when 'disable'
-            feature.disable_group value
-          end
+          if Flipper.group_exists?(value)
+            case params['operation']
+            when 'enable'
+              feature.enable_group value
+            when 'disable'
+              feature.disable_group value
+            end
 
-          redirect_to("/features/#{feature.key}")
-        rescue Flipper::GroupNotRegistered => e
-          error = Rack::Utils.escape("The group named #{value.inspect} has not been registered.")
-          redirect_to("/features/#{feature.key}/groups?error=#{error}")
+            redirect_to("/features/#{feature.key}")
+          else
+            error = Rack::Utils.escape("The group named #{value.inspect} has not been registered.")
+            redirect_to("/features/#{feature.key}/groups?error=#{error}")
+          end
         end
       end
     end

spec/flipper/api/v1/actions/groups_gate_spec.rb

@@ -26,6 +26,29 @@
     end
   end
 
+  describe 'enable without name params' do
+    before do
+      flipper[:my_feature].disable
+      Flipper.register(:admins) do |actor|
+        actor.respond_to?(:admin?) && actor.admin?
+      end
+      post '/features/my_feature/groups'
+    end
+
+    it 'returns correct status code' do
+      expect(last_response.status).to eq(422)
+    end
+
+    it 'returns formatted error' do
+      expected = {
+        'code' => 5,
+        'message' => 'Required parameter name is missing.',
+        'more_info' => api_error_code_reference_url,
+      }
+      expect(json_response).to eq(expected)
+    end
+  end
+
   describe 'disable' do
     before do
       flipper[:my_feature].disable
@@ -50,7 +73,7 @@
     end
   end
 
-  describe 'non-existent feature' do
+  describe 'disable for non-existent feature' do
     before do
       Flipper.register(:admins) do |actor|
         actor.respond_to?(:admin?) && actor.admin?
@@ -72,7 +95,7 @@
     end
   end
 
-  describe 'group not registered' do
+  describe 'disable for group not registered' do
     before do
       flipper[:my_feature].disable
       delete '/features/my_feature/groups', name: 'admins'
@@ -88,4 +111,47 @@
       expect(json_response).to eq(expected)
     end
   end
+
+  describe 'enable for group not registered when allow_unregistered_groups is true' do
+    before do
+      Flipper.unregister_groups
+      flipper[:my_feature].disable
+      post '/features/my_feature/groups', name: 'admins', allow_unregistered_groups: 'true'
+    end
+
+    it 'responds successfully' do
+      expect(last_response.status).to eq(200)
+    end
+
+    it 'returns decorated feature with group in groups set' do
+      group_gate = json_response['gates'].find { |m| m['name'] == 'group' }
+      expect(group_gate['value']).to eq(['admins'])
+    end
+
+    it 'enables group' do
+      expect(flipper[:my_feature].groups_value).to eq(Set["admins"])
+    end
+  end
+
+  describe 'disable for group not registered when allow_unregistered_groups is true' do
+    before do
+      Flipper.unregister_groups
+      flipper[:my_feature].disable
+      flipper[:my_feature].enable_group(:admins)
+      delete '/features/my_feature/groups', name: 'admins', allow_unregistered_groups: 'true'
+    end
+
+    it 'responds successfully' do
+      expect(last_response.status).to eq(200)
+    end
+
+    it 'returns decorated feature with group not in groups set' do
+      group_gate = json_response['gates'].find { |m| m['name'] == 'group' }
+      expect(group_gate['value']).to eq([])
+    end
+
+    it 'disables group' do
+      expect(flipper[:my_feature].groups_value).to be_empty
+    end
+  end
 end

spec/flipper/dsl_spec.rb

@@ -97,20 +97,9 @@
         @group = Flipper.register(:admins) {}
       end
 
-      it 'returns group' do
-        expect(subject.group(:admins)).to eq(@group)
-      end
-
-      it 'always returns same instance for same name' do
-        expect(subject.group(:admins)).to equal(subject.group(:admins))
-      end
-    end
-
-    context 'for unregistered group' do
-      it 'raises error' do
-        expect do
-          subject.group(:admins)
-        end.to raise_error(Flipper::GroupNotRegistered)
+      it 'delegates to Flipper' do
+        expect(Flipper).to receive(:group).with(:admins).and_return(@group)
+        expect(subject.group(:admins)).to be(@group)
       end
     end
   end

spec/flipper/registry_spec.rb

@@ -44,10 +44,8 @@
     end
 
     context 'key not registered' do
-      it 'raises key not found' do
-        expect do
-          subject.get(:admins)
-        end.to raise_error(Flipper::Registry::KeyNotFound)
+      it 'returns nil' do
+        expect(subject.get(:admins)).to be(nil)
       end
     end
   end

spec/flipper/ui/actions/groups_gate_spec.rb

@@ -8,6 +8,7 @@
       'a'
     end
   end
+
   let(:session) do
     if Rack::Protection::AuthenticityToken.respond_to?(:random_token)
       { csrf: token }

spec/flipper_spec.rb

@@ -80,10 +80,17 @@
     end
 
     context 'for unregistered group' do
-      it 'raises group not registered error' do
-        expect do
-          described_class.group(:cats)
-        end.to raise_error(Flipper::GroupNotRegistered, 'Group :cats has not been registered')
+      before do
+        @group = described_class.group(:cats)
+      end
+
+      it 'returns group' do
+        expect(@group).to be_instance_of(Flipper::Types::Group)
+        expect(@group.name).to eq(:cats)
+      end
+
+      it 'does not add group to registry' do
+        expect(described_class.group_exists?(@group.name)).to be(false)
       end
     end
   end