Migrate ::FeatureFlag and ::FeatureFlagsUser to ActiveRecord #15841

amiedes · 2020-09-17T12:51:12Z

Story details: https://app.clubhouse.io/cartoteam/story/104742

What does this PR do?

Migrates the ::FeatureFlag and ::FeatureFlagsUsere Sequel models to the ActiveRecord equivalents: Carto::FeatureFlag and Carto::FeatureFlagsUser
Unifies the API for reading user feature flags. Before the behavior was not coherent between ::User and Carto::User, and some method names were misleading.
Writes more specs for the modified methods

amiedes · 2020-09-21T14:23:00Z

spec/models/carto/user_spec.rb

-        it { should be_false }
-      end
-    end
-  end


This was moved to user_commons_spec.rb

amiedes · 2020-09-21T14:23:51Z

spec/models/user_part_crud_spec.rb

-    SequelRails.connection["select count(*) from feature_flags_users where user_id = '#{user_id}'"].first[:count].should eq 0
-    SequelRails.connection["select count(*) from feature_flags where id = '#{ff.id}'"].first[:count].should eq 1
-  end
-


This was moved to user_commons_spec.rb as describe '#update_feature_flags' do ...

amiedes · 2020-09-21T14:26:26Z

app/controllers/superadmin/feature_flags_controller.rb

-      @feature_flag.id = feature_flag_params[:id]
-    end
-    @feature_flag.name = feature_flag_params[:name]
-    @feature_flag.restricted = feature_flag_params[:restricted]


I refactored this code because it was a bit counterintuitive. We were assigning new values to the feature flag attributes even when the request only aimed to do a #destroy.

amiedes · 2020-09-21T14:27:52Z

app/controllers/superadmin/feature_flags_controller.rb


  def create
-    @feature_flag.save
-    render json: @feature_flag, status: 204
+    Carto::FeatureFlag.create!(feature_flag_params)


I switched things like #save and #destroy for the ! equivalents so we are able to spot an error. Otherwise we were always returning a 204 no matter if the action succeeded or not.

that indeed keeps the behavior consistent: https://www.rubydoc.info/github/jeremyevans/sequel/Sequel%2FModel%2FClassMethods:raise_on_save_failure

that indeed keeps the behavior consistent

Oh! I did not notice that it's indeed the Sequel equivalent. So the consistency was "by accident" 🤣 🤣

amiedes · 2020-09-21T14:28:36Z

app/controllers/superadmin/feature_flags_controller.rb

+  end
+
+  def feature_flag_params
+    params.require(:feature_flag).permit(:id, :name, :restricted)


This is the standard Rails way to define which parameters are allowed to be updated from a controller

amiedes · 2020-09-21T14:29:50Z

app/controllers/superadmin/users_controller.rb

@@ -54,7 +54,7 @@ def create
    if @user.save
      @user.reload
      CartoDB::Visualization::CommonDataService.load_common_data(@user, self) if @user.should_load_common_data?
-      @user.set_relationships_from_central(user_param)
+      @user.update_feature_flags(user_param[:feature_flags])


For simplicity, I removed the additional level of indirection added by set_relationships_from_central, since calling it is equivalent to calling update_feature_flags directly.

Is it ok to do @user.update_feature_flags(nil)? that's the check you may be missing, right? What's the semantics? Hope it is not to reset feature flags to empty. EDIT: to be honest, I don't know if that situation is possible, but I think it is as Central only sends "attributes" (or pseudo-attributes) that have changed.

The way of calling it should be:

user.update_feature_flags(["123", "456"])

Hope it is not to reset feature flags to empty

This is taken care of here. On the other hand you made me realize that this is an important-enough case to have a specific spec to check that flags are not reset by accident, so I'm gonna add it 👍

amiedes · 2020-09-21T14:31:15Z

app/models/carto/feature_flag.rb


-    def self.find_by_user(user)
-      FeatureFlag.where(restricted: false) + user.feature_flags.select(&:restricted)
-    end


…ureflag-and-featureflaguser

amiedes · 2020-09-22T09:46:09Z

app/models/carto/organization.rb

@@ -160,7 +160,7 @@ def non_owner_users
    end

    def inheritable_feature_flags
-      inherit_owner_ffs ? owner.feature_flags_user : []
+      inherit_owner_ffs ? owner.self_feature_flags : Carto::FeatureFlag.none
    end


By returning Carto::FeatureFlag.none instead of [], we return an ActiveRecord association proxy, so we allow chaining more conditions to the query afterwards without running the query yet.

amiedes · 2020-09-22T10:32:00Z

The acceptance in staging looked ✅ 👍

I tested updated feature flags from superadmin and if the flag was restricted it was preserved in CartoDB and if it was not restricted it was removed (in production it works like that)
Tested creating new feature flags from superadmin (both restricted and unrestricted) and the sync with CartoDB was ok

rafatower

This is a lot of work and a lot of small conflicts solved nicely! 💪

The only thing that I think you need to review (or smoke test in staging) is syncing FF's from Central. EDIT: I mean, syncing something other than feature flags and checking it does not break with a nil usage (or delete flags)

rafatower · 2020-09-22T11:55:16Z

app/controllers/superadmin/feature_flags_controller.rb


  def create
-    @feature_flag.save
-    render json: @feature_flag, status: 204
+    Carto::FeatureFlag.create!(feature_flag_params)


that indeed keeps the behavior consistent: https://www.rubydoc.info/github/jeremyevans/sequel/Sequel%2FModel%2FClassMethods:raise_on_save_failure

rafatower · 2020-09-22T13:20:10Z

app/controllers/superadmin/users_controller.rb

@@ -54,7 +54,7 @@ def create
    if @user.save
      @user.reload
      CartoDB::Visualization::CommonDataService.load_common_data(@user, self) if @user.should_load_common_data?
-      @user.set_relationships_from_central(user_param)
+      @user.update_feature_flags(user_param[:feature_flags])


Is it ok to do @user.update_feature_flags(nil)? that's the check you may be missing, right? What's the semantics? Hope it is not to reset feature flags to empty. EDIT: to be honest, I don't know if that situation is possible, but I think it is as Central only sends "attributes" (or pseudo-attributes) that have changed.

rafatower · 2020-09-22T13:22:00Z

app/models/carto/feature_flag.rb


-    def self.find_by_user(user)
-      FeatureFlag.where(restricted: false) + user.feature_flags.select(&:restricted)
-    end


rafatower · 2020-09-22T13:24:55Z

app/models/carto/helpers/user_commons.rb

@@ -306,4 +306,36 @@ def has_access_to_coverband?
    organization&.name == 'team'
  end

+  def feature_flags
+    feature_flags_ids = self_feature_flags.pluck(:id) + Carto::FeatureFlag.not_restricted.pluck(:id)
+    feature_flags_ids += organization.inheritable_feature_flags.pluck(:id) if organization


I had to look it up in the docs 😅 :
https://apidock.com/rails/Enumerable/pluck
https://apidock.com/rails/ActiveRecord/Calculations/pluck

rafatower · 2020-09-22T14:38:58Z

app/models/carto/user.rb

@@ -39,8 +39,8 @@ class Carto::User < ActiveRecord::Base
  has_one :owned_organization, class_name: Carto::Organization, inverse_of: :owner, foreign_key: :owner_id
  has_one :static_notifications, class_name: Carto::UserNotification, inverse_of: :user

-  has_many :feature_flags_user, dependent: :destroy, foreign_key: :user_id, inverse_of: :user
-  has_many :feature_flags, through: :feature_flags_user
+  has_many :self_feature_flags_user, dependent: :destroy, foreign_key: :user_id, inverse_of: :user, class_name: Carto::FeatureFlagsUser


(maybe I'm picking this out of context, but why the rename?)

This is because the feature flags of a user are not only the ones he/she has, but also the ones inherited from the organization owner. I had manly two ways to address this to avoid ambiguity:

Option 1

Carto::User.self_feature_flags # => An association which only returns the user feature flags Carto::User.feature_flags # => A method which returns self_feature_flags + organization feature flags

Option 2

Carto::User.feature_flags # => An association which only returns the user feature flags Carto::User.effective_feature_flags # => A method which returns feature_flags + organization feature flags

Even though 2 would be more consistent with the naming we used to address the similar issue with the user quotas, finally I did 1 because I thought it was more natural for some other developer to try to retrieve the feature flags doing user.feature_flags rather than knowing that the real method he/she has to use is user.effective_feature_flags

thedae

Nice! 👍

…ureflag-and-featureflaguser

amiedes changed the title ~~Migrate ::FeatureFlat and ::FeatureFlagsUser to ActiveRecord~~ Migrate ::FeatureFlag and ::FeatureFlagsUser to ActiveRecord Sep 17, 2020

Migrate ::FeatureFlag to ActiveRecord

a67da0b

amiedes force-pushed the chore/ch104742/migrate-and-delete-old-featureflag-and-featureflaguser branch from 07499b4 to a67da0b Compare September 21, 2020 10:46

amiedes added 4 commits September 21, 2020 14:12

Refactor feature flags controller

1401e28

Simplify API for activating featur flags. Fix rake tasks.

af15a61

Fix #update_feature_flags

bf1fbbc

Remove user method from module used in organization

da58a82

amiedes commented Sep 21, 2020

View reviewed changes

spec/models/carto/user_spec.rb

it { should be_false }

end

end

end

Copy link

Contributor Author

amiedes Sep 21, 2020

Choose a reason for hiding this comment

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

This was moved to user_commons_spec.rb

amiedes commented Sep 21, 2020

View reviewed changes

Merge branch 'master' into chore/ch104742/migrate-and-delete-old-feat…

49e967d

…ureflag-and-featureflaguser

amiedes marked this pull request as ready for review September 22, 2020 09:40

amiedes commented Sep 22, 2020

View reviewed changes

Remove factory for FeatureFlagsUser

857f235

amiedes requested review from rafatower and thedae September 22, 2020 10:16

rafatower approved these changes Sep 22, 2020

View reviewed changes

Add spec to avoid accidental feature_flags deletion

c47165d

thedae approved these changes Sep 23, 2020

View reviewed changes

Merge branch 'master' into chore/ch104742/migrate-and-delete-old-feat…

93e254e

…ureflag-and-featureflaguser

amiedes merged commit b451213 into master Sep 24, 2020

amiedes deleted the chore/ch104742/migrate-and-delete-old-featureflag-and-featureflaguser branch September 24, 2020 08:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate ::FeatureFlag and ::FeatureFlagsUser to ActiveRecord #15841

Migrate ::FeatureFlag and ::FeatureFlagsUser to ActiveRecord #15841

amiedes commented Sep 17, 2020 •

edited

Loading

amiedes Sep 21, 2020

amiedes Sep 21, 2020

amiedes Sep 21, 2020

amiedes Sep 21, 2020

rafatower Sep 22, 2020

amiedes Sep 23, 2020

amiedes Sep 21, 2020

amiedes Sep 21, 2020

rafatower Sep 22, 2020

amiedes Sep 23, 2020

amiedes Sep 21, 2020

rafatower Sep 22, 2020

amiedes Sep 22, 2020 •

edited

Loading

amiedes commented Sep 22, 2020

rafatower left a comment •

edited

Loading

rafatower Sep 22, 2020

rafatower Sep 22, 2020

rafatower Sep 22, 2020

rafatower Sep 22, 2020

rafatower Sep 22, 2020

amiedes Sep 23, 2020

thedae left a comment

Migrate ::FeatureFlag and ::FeatureFlagsUser to ActiveRecord #15841

Migrate ::FeatureFlag and ::FeatureFlagsUser to ActiveRecord #15841

Conversation

amiedes commented Sep 17, 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

amiedes Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

amiedes commented Sep 22, 2020

rafatower left a comment • 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

thedae left a comment

Choose a reason for hiding this comment

amiedes commented Sep 17, 2020 •

edited

Loading

amiedes Sep 22, 2020 •

edited

Loading

rafatower left a comment •

edited

Loading