Add the possibility to fetch all features already released to a given resource #67

nsoufr · 2020-06-25T15:57:30Z

This PR introduces the ability to fetch which features were released to a given resource (account, user, etc).

acc = Account.find(2)
acc.release(:email_marketing, :whitelabel)

Account.all_released_ids(:email_marketing, :whitelabel) 
#=> [2]

# not available before it
acc.releases
#=> ["account:email_marketing:whitelabel"]

This method is reponsible to fetch all related features from a given resource.

henrich-m · 2020-06-29T14:37:46Z

spec/feature_flagger/storage/redis_spec.rb

-        expect(storage.all_values(key).sort).to eq values.sort
+      it 'returns all resource_ids for the given feature_key' do
+        redis.sadd(feature_key, resource_ids)
+        expect(storage.all_values(feature_key).sort).to eq resource_ids.sort


match_array

henrich-m · 2020-06-29T14:44:35Z

spec/feature_flagger/storage/redis_spec.rb

-        it { expect(storage).to have_value(key, value) }
+    describe '#has_values?' do
+      context 'resource_id is stored for given feature_key' do
+        before { redis.sadd(feature_key, resource_id) }


I think we could use the storage.add method, what do you think?

make sense, done!

spec/feature_flagger/storage/redis_spec.rb

henrich-m · 2020-06-29T14:45:51Z

spec/feature_flagger/storage/redis_spec.rb

-        it { expect(storage).not_to have_value(key, value) }
+      context 'when there is features under global structure' do
+        before do
+          storage.add(feature_key, resource_name, resource_id)


It looks like the test doesn't need this setup :)

henrich-m · 2020-06-29T14:47:52Z

spec/feature_flagger/storage/redis_spec.rb

+        storage.add(feature_key, resource_name, resource_id)
+
+        expect(redis.sismember(feature_key, resource_id)).to be_truthy
+        expect(redis.sismember(resource_key, feature_key)).to be_truthy


Why use the redis to make the assertion? And why there are two assertions and only one context?

henrich-m · 2020-06-29T14:50:59Z

spec/feature_flagger/storage/redis_spec.rb

+        storage.add_all(global_key, resource_id)
+        expect(redis.sismember(global_key, resource_id)).to be_truthy
+
+        expect(redis.sismember(feature_key, resource_id)).to be_falsey


http://www.betterspecs.org/#single

this can be discussed/done in another PR

IMHO it's fine as it is. The single expectation tip is not well accepted by the community and will probably be removed from the guide.

betterspecs/betterspecs#5
https://stackoverflow.com/questions/38437162/whats-the-difference-between-rspecs-subject-and-let-when-should-they-be-used/38459039#38459039

Besides, the expects are co-dependent, we don't want to isolate then, in order the expect from line 71 be valid, it needs to expect from line 69 to pass.

But I think this can be discussed in another issue, or PR. not related to this change, as long this is actually an opinionated style.

henrich-m · 2020-06-29T14:53:02Z

spec/feature_flagger/control_spec.rb

@@ -19,19 +20,19 @@ module FeatureFlagger
        it { expect(result).to be_falsey }

        context 'and a feature is release to all' do
-          before { storage.add(FeatureFlagger::Control::RELEASED_FEATURES, key) }
+          before { storage.add(FeatureFlagger::Control::RELEASED_FEATURES, resource_name, key) }


We should be using control.release in all control tests.

make sense, done

henrich-m · 2020-06-29T14:53:59Z

spec/feature_flagger/control_spec.rb

        it { expect(result).to be_falsey }
      end

      context 'when feature was released to all' do
-        before { storage.add(FeatureFlagger::Control::RELEASED_FEATURES, key) }
+        before { storage.add_all(FeatureFlagger::Control::RELEASED_FEATURES, key) }


control.release_to_all

you're right, tests are better now

I think you missed this one :) It still using storage, IMHO this spec we should not use the storage class at all only the public API of FeatureFlagger::Control does that make sense to you? I think we can do this refactor in another time anyways :D

lib/feature_flagger/storage/redis_keys.rb

henrich-m · 2020-06-29T14:57:45Z

spec/feature_flagger/storage/redis_spec.rb

-        before { redis.sadd(key, value) }
-        it { expect(storage).to have_value(key, value) }
+    describe '#has_values?' do
+      context 'resource_id is stored for given feature_key' do


When describing a context, start its description with "when", "with" or "without".

* Cleanup features when add_all and remove_all to prevent invalid objects on redis

henrich-m · 2020-07-01T16:30:55Z

spec/feature_flagger/storage/redis_spec.rb

+      end
+
+      context 'when add_all is called right after add' do
+        it 'adds resource_id to redis global key, and clear both resource_id and feature_key' do


Don't you think a and here means this is two separate contexts?

lib/feature_flagger/control.rb

lib/feature_flagger/storage/redis.rb

resource_keys.

henrich-m · 2020-07-07T18:02:23Z

spec/feature_flagger/control_spec.rb

        it { expect(result).to be_falsey }
      end

      context 'when feature was released to all' do
-        before { storage.add(FeatureFlagger::Control::RELEASED_FEATURES, key) }
+        before { storage.add_all(FeatureFlagger::Control::RELEASED_FEATURES, key) }


I think you missed this one :) It still using storage, IMHO this spec we should not use the storage class at all only the public API of FeatureFlagger::Control does that make sense to you? I think we can do this refactor in another time anyways :D

henrich-m · 2020-07-07T18:17:47Z

lib/feature_flagger/manager.rb

@@ -2,7 +2,7 @@ module FeatureFlagger
  class Manager

    def self.detached_feature_keys
-      persisted_features = FeatureFlagger.control.search_keys("*").to_a
+      persisted_features = FeatureFlagger.control.feature_keys


henrich-m

LGTM

andrehjr · 2020-07-27T14:16:21Z

lib/feature_flagger/control.rb

@@ -13,15 +13,27 @@ def released?(feature_key, resource_id)
    end

    def release(feature_key, resource_id)
-      @storage.add(feature_key, resource_id)
+      resource_name = FeatureFlagger::Storage::RedisKeys.extract_resource_name_from_feature_key(


Should calls made to RedisKeys only be made inside the Redis Storage adapter?

Or is it something we would be able to re-use to other types of storages? In that case, I think we should rename RedisKeys to something else.

Why? Keys class it's responsible for decomposing the feature_key and this logic will be needed in others Storages as well. So we are renaming it to make clear it doesn't depends on Redis.

codeclimate · 2020-07-30T21:18:52Z

Code Climate has analyzed commit 1eb46df and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (80% is the threshold).

This pull request will bring the total coverage in the repository to 99.0% (0.2% change).

View more on Code Climate.

Nando Sousa and others added 5 commits June 24, 2020 15:06

Adds Storage::Redis#fetch_releases

2a9e18c

This method is reponsible to fetch all related features from a given resource.

Modifies Storage::Redis#add to supports new structure.

cdbf8b5

Merge branch 'master' into ns-features-by-account

d83cd8b

Some refactorings

25180ad

Implements releases on model and control layers

52c0919

henrich-m reviewed Jun 29, 2020

View reviewed changes

Nando Sousa added 5 commits July 1, 2020 10:42

Stop testing by its implementation, instead testing its implementation

33b3358

add when to context

3b65b47

add testes for redis keys

56178cb

Fix detached_feature_keys

5984e80

* Cleanup features when add_all and remove_all to prevent invalid objects on redis

replace redis by storage

7d6ce1b

henrich-m reviewed Jul 1, 2020

View reviewed changes

andrehjr suggested changes Jul 1, 2020

View reviewed changes

lib/feature_flagger/control.rb Outdated Show resolved Hide resolved

lib/feature_flagger/storage/redis.rb Show resolved Hide resolved

lib/feature_flagger/storage/redis.rb Outdated Show resolved Hide resolved

Nando Sousa added 6 commits July 1, 2020 16:12

add missing expectation

67426b3

remove never reached line

9187826

Fix test message

7322aee

add Redis#feature_keys to return all feature keys filtering out

43479d1

resource_keys.

fix deprecated doc

71648b7

Most optimized way to remove feature_keys from resources

679fcc2

henrich-m reviewed Jul 7, 2020

View reviewed changes

henrich-m previously approved these changes Jul 7, 2020

View reviewed changes

nsoufr requested a review from andrehjr July 27, 2020 13:35

andrehjr reviewed Jul 27, 2020

View reviewed changes

Rename RedisKeys to Keys only

2977d70

Why? Keys class it's responsible for decomposing the feature_key and this logic will be needed in others Storages as well. So we are renaming it to make clear it doesn't depends on Redis.

henrich-m dismissed their stale review via 2977d70 July 30, 2020 21:12

Rename redis_keys to keys in require too

1eb46df

andrehjr approved these changes Jul 31, 2020

View reviewed changes

masisiliani merged commit bb74c2b into master Jul 31, 2020

henrich-m deleted the ns-features-by-account branch August 3, 2020 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the possibility to fetch all features already released to a given resource #67

Add the possibility to fetch all features already released to a given resource #67

nsoufr commented Jun 25, 2020 •

edited

Loading

henrich-m Jun 29, 2020

nsoufr Jul 1, 2020

henrich-m Jun 29, 2020

nsoufr Jul 1, 2020

henrich-m Jun 29, 2020

nsoufr Jul 1, 2020

henrich-m Jun 29, 2020

henrich-m Jun 29, 2020

nsoufr Jul 1, 2020

dayvsonlima Jul 1, 2020

nsoufr Jul 1, 2020

nsoufr Jul 1, 2020

henrich-m Jun 29, 2020

nsoufr Jul 1, 2020

henrich-m Jun 29, 2020

nsoufr Jul 1, 2020

henrich-m Jul 7, 2020

henrich-m Jun 29, 2020

henrich-m Jul 1, 2020 •

edited

Loading

henrich-m Jul 7, 2020

henrich-m Jul 7, 2020

henrich-m left a comment

andrehjr Jul 27, 2020

codeclimate bot commented Jul 30, 2020

Add the possibility to fetch all features already released to a given resource #67

Add the possibility to fetch all features already released to a given resource #67

Conversation

nsoufr commented Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrich-m Jul 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrich-m left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codeclimate bot commented Jul 30, 2020

nsoufr commented Jun 25, 2020 •

edited

Loading

henrich-m Jul 1, 2020 •

edited

Loading