Migrate FeatureFlag & PricePlan sync to Message Broker

chore/ch130874/move-feature-flag-management-to-broker_green-base
Alberto Miedes Garcés 4 years ago
parent 2f4e5dd472
commit c3a558dc56

@ -118,6 +118,7 @@ end
group :development, :test do
gem 'byebug'
gem 'database_cleaner-active_record'
gem 'faker'
gem 'pry-byebug', '3.9.0'
gem 'rack'

@ -136,6 +136,10 @@ GEM
redis
crass (1.0.6)
daemons (1.3.1)
database_cleaner (1.99.0)
database_cleaner-active_record (1.99.0)
activerecord
database_cleaner (~> 1.99.0)
db-query-matchers (0.4.0)
dbf (2.0.6)
fastercsv (~> 1.5.4)
@ -542,6 +546,7 @@ DEPENDENCIES
ci_reporter (= 1.8.4)
compass (= 1.0.3)
coverband
database_cleaner-active_record
db-query-matchers (= 0.4.0)
dbf (= 2.0.6)
delorean

@ -195,11 +195,17 @@ WORKING_SPECS_5 = \
spec/lib/carto/db/user_schema_spec.rb \
spec/lib/carto/db/sql_interface_spec.rb \
spec/lib/carto/file_system/sanitize_spec.rb \
spec/commands/feature_flag_commands/create_spec.rb \
spec/commands/feature_flag_commands/delete_spec.rb \
spec/commands/feature_flag_commands/update_spec.rb \
spec/commands/map_views_commands/update_spec.rb \
spec/commands/organization_commands/create_spec.rb \
spec/commands/organization_commands/delete_spec.rb \
spec/commands/organization_commands/update_spec.rb \
spec/commands/central_user_commands_spec.rb \
spec/commands/account_type_commands/create_spec.rb \
spec/commands/account_type_commands/update_spec.rb \
spec/commands/account_type_commands/delete_spec.rb \
$(NULL)
# TODO: This block also breaks if run alongside other specs, needs checking why
@ -235,8 +241,7 @@ WORKING_SPECS_9 = \
spec/requests/signup_controller_spec.rb \
spec/requests/account_tokens_controller_spec.rb \
spec/requests/password_change_controller_spec.rb \
spec/requests/superadmin/account_types_spec.rb \
spec/requests/superadmin/feature_flag_spec.rb \
spec/requests/superadmin/users_spec.rb \
spec/requests/superadmin/oauth_apps_spec.rb \
spec/requests/superadmin/organizations_controller_spec.rb \
spec/requests/superadmin/platform_controller_spec.rb \

@ -17,6 +17,7 @@ sudo make install
- Add preview/visualization of maps in DO catalog [#15973](https://github.com/CartoDB/cartodb/pull/15973)
- Add new user metrics to Home page [#15950](https://github.com/CartoDB/cartodb/pull/15950)
- Replace CRUD user operations in Central API client by publishing messages to the Message Broker [#16035](https://github.com/CartoDB/cartodb/pull/16035)
- Migrate FeatureFlag & PricePlan synchronization to the Message Broker [#16098](https://github.com/CartoDB/cartodb/pull/16098)
### Bug fixes / enhancements

@ -0,0 +1,21 @@
module AccountTypeCommands
class Create < ::CartoCommand
private
def run_command
@account_type = Carto::AccountType.find_by(account_type: account_type_literal) ||
Carto::AccountType.new(account_type: account_type_literal)
@account_type.rate_limit = Carto::RateLimit.from_api_attributes(
params[:price_plan][:rate_limit] || {}
)
@account_type.save!
end
def account_type_literal
params[:price_plan][:account_type]
end
end
end

@ -0,0 +1,31 @@
module AccountTypeCommands
class Delete < ::CartoCommand
private
def run_command
@account_type = Carto::AccountType.find_by(
account_type: account_type
)
# Don't break if account_type does not exist, as it is harmless
if @account_type.nil?
logger.warn(message: 'AccountType not found')
return
end
@account_type.destroy!
logger.info(message: 'AccountType destroyed')
end
def account_type
params[:price_plan][:account_type]
end
def log_context
super.merge(account_type: account_type)
end
end
end

@ -0,0 +1,33 @@
module AccountTypeCommands
class Update < ::CartoCommand
private
def run_command
@account_type = Carto::AccountType.find(account_type_params[:account_type])
if rate_limit_changed?
@account_type.rate_limit.update!(@received_rate_limit.rate_limit_attributes)
logger.info(message: 'Rate limit updated')
::Resque.enqueue(::Resque::UserJobs::RateLimitsJobs::SyncRedis, @account_type.account_type)
end
end
def rate_limit_changed?
@received_rate_limit = Carto::RateLimit.from_api_attributes(
account_type_params[:rate_limit] || {}
)
@account_type.rate_limit != @received_rate_limit
end
def account_type_params
params[:price_plan]
end
def log_context
super.merge(account_type: account_type_params[:account_type])
end
end
end

@ -0,0 +1,22 @@
module FeatureFlagCommands
class Create < ::CartoCommand
private
def run_command
Carto::FeatureFlag.create!(feature_flag_params)
end
def feature_flag_params
params[:feature_flag].slice(:id, :name, :restricted)
end
def log_context
super.merge(
feature_flag_id: feature_flag_params[:id],
feature_flag_name: feature_flag_params[:name]
)
end
end
end

@ -0,0 +1,16 @@
module FeatureFlagCommands
class Delete < ::CartoCommand
private
def run_command
Carto::FeatureFlag.find(params[:feature_flag][:id])
.destroy!
end
def log_context
super.merge(feature_flag_id: params[:feature_flag][:id])
end
end
end

@ -0,0 +1,27 @@
module FeatureFlagCommands
class Update < ::CartoCommand
private
def run_command
Carto::FeatureFlag.find(feature_flag_params[:id])
.update!(feature_flag_update_params)
end
def feature_flag_update_params
feature_flag_params.slice(:name, :restricted)
end
def feature_flag_params
params[:feature_flag]
end
def log_context
super.merge(
feature_flag_id: feature_flag_params[:id],
feature_flag_name: feature_flag_params[:name]
)
end
end
end

@ -1,53 +0,0 @@
class Superadmin::AccountTypesController < Superadmin::SuperadminController
respond_to :json
ssl_required :create, :update, :destroy
before_filter :get_account_type, only: [:update, :destroy]
before_filter :get_rate_limit, only: [:create, :update]
def create
if Carto::AccountType.exists?(params[:account_type][:account_type])
@account_type = Carto::AccountType.find(params[:account_type][:account_type])
else
@account_type = Carto::AccountType.new
@account_type.account_type = params[:account_type][:account_type]
end
@account_type.rate_limit = @rate_limit
@account_type.save!
render json: @account_type, status: 201
end
def update
if @account_type.rate_limit != @rate_limit
@account_type.rate_limit.update_attributes!(@rate_limit.rate_limit_attributes)
::Resque.enqueue(::Resque::UserJobs::RateLimitsJobs::SyncRedis, @account_type.account_type)
end
render json: @account_type, status: 200
end
def destroy
@account_type.destroy
render nothing: true, status: 204
end
private
def get_account_type
@account_type = Carto::AccountType.find(params[:id])
rescue ActiveRecord::RecordNotFound
render json: { error: 'ERROR. account_type not found' }, status: 404
end
def get_rate_limit
account_type_params = params[:account_type] || {}
@rate_limit = Carto::RateLimit.from_api_attributes(account_type_params[:rate_limit] || {})
render json: { error: 'ERROR. rate_limit object is not valid' }, status: 422 unless @rate_limit.valid?
end
end

@ -1,36 +0,0 @@
class Superadmin::FeatureFlagsController < Superadmin::SuperadminController
respond_to :json
ssl_required :create, :update, :destroy
before_filter :load_feature_flag, only: [:update, :destroy]
def create
Carto::FeatureFlag.create!(feature_flag_params)
render json: @feature_flag, status: :no_content
end
def update
@feature_flag.update!(feature_flag_params)
render json: @feature_flag, status: :no_content
end
def destroy
@feature_flag.destroy!
render json: @feature_flag, status: :no_content
end
private
def load_feature_flag
@feature_flag = Carto::FeatureFlag.find(params[:id])
end
def feature_flag_params
params.require(:feature_flag).permit(:id, :name, :restricted)
end
end

@ -548,8 +548,6 @@ CartoDB::Application.routes.draw do
end
resources :organizations, only: [:index, :show]
resources :synchronizations
resources :feature_flags
resources :account_types, only: [:create, :update, :destroy]
resources :oauth_apps, only: [:create, :update, :destroy]
end

@ -61,6 +61,48 @@ namespace :message_broker do
).run
end
subscription.register_callback(:create_feature_flag) do |message|
FeatureFlagCommands::Create.new(
message.payload,
{ logger: logger, request_id: message.request_id }
).run
end
subscription.register_callback(:update_feature_flag) do |message|
FeatureFlagCommands::Update.new(
message.payload,
{ logger: logger, request_id: message.request_id }
).run
end
subscription.register_callback(:delete_feature_flag) do |message|
FeatureFlagCommands::Delete.new(
message.payload,
{ logger: logger, request_id: message.request_id }
).run
end
subscription.register_callback(:create_price_plan) do |message|
AccountTypeCommands::Create.new(
message.payload,
{ logger: logger, request_id: message.request_id }
).run
end
subscription.register_callback(:update_price_plan) do |message|
AccountTypeCommands::Update.new(
message.payload,
{ logger: logger, request_id: message.request_id }
).run
end
subscription.register_callback(:delete_price_plan) do |message|
AccountTypeCommands::Delete.new(
message.payload,
{ logger: logger, request_id: message.request_id }
).run
end
at_exit do
logger.info(message: 'Stopping subscriber...')
subscription.stop!

@ -218,7 +218,6 @@ services/importer/spec/acceptance/osm_spec.rb
services/importer/spec/acceptance/mapinfo_spec.rb
spec/services/carto/user_multifactor_auth_update_service_spec.rb
spec/services/carto/do_licensing_service_spec.rb
spec/requests/superadmin/feature_flag_spec.rb
spec/requests/carto/api/data_import_presenter_spec.rb
spec/queries/carto/visualization_query_searcher_spec.rb
spec/models/synchronization/synchronization_oauth_spec.rb

@ -0,0 +1,49 @@
require 'spec_helper'
describe AccountTypeCommands::Create do
include_context 'with DatabaseCleaner'
let(:command) { described_class.new(params) }
let(:params) do
{
price_plan: {
account_type: account_type_literal,
rate_limit: rate_limit_params
}
}
end
let(:rate_limit_params) { create(:rate_limits).api_attributes }
let(:account_type_literal) { 'FREE' }
let(:created_account_type) { Carto::AccountType.find(account_type_literal) }
describe '#run' do
context 'when everything is ok' do
before { command.run }
it 'creates the account type' do
expect(created_account_type).to be_present
expect(created_account_type.account_type).to eq(account_type_literal)
end
it 'creates the corresponding rate limit' do
expect(created_account_type.rate_limit).to be_present
end
end
context 'when an error occurs' do
let(:rate_limit_params) { nil }
it 'raises an error' do
expect { command.run }.to raise_error(ActiveRecord::StatementInvalid)
end
end
context 'when the account type already exists' do
before { create(:account_type, account_type: account_type_literal) }
it 'updates it' do
expect { command.run }.to change(Carto::AccountType, :count).by(0)
end
end
end
end

@ -0,0 +1,31 @@
require 'spec_helper'
describe AccountTypeCommands::Delete do
include_context 'with DatabaseCleaner'
let(:command) { described_class.new(params, logger: logger) }
let(:params) { { price_plan: { account_type: account_type_literal } } }
let(:account_type_literal) { 'FREE' }
let(:logger) { Carto::Common::Logger.new }
let!(:account_type) { create(:account_type, account_type: account_type_literal) }
describe '#run' do
context 'when everything is ok' do
before { command.run }
it 'deletes the account type' do
expect { account_type.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
end
context 'when the account type does not exist' do
before { account_type.destroy! }
it 'logs a message and does not fail' do
logger.expects(:warn).with(message: 'AccountType not found')
command.run
end
end
end
end

@ -0,0 +1,87 @@
require 'spec_helper'
describe AccountTypeCommands::Update do
include_context 'with DatabaseCleaner'
let(:command) { described_class.new(params, logger: logger) }
let(:params) do
{
price_plan: {
account_type: account_type_literal,
rate_limit: rate_limit_params
}
}
end
let(:account_type_literal) { 'FREE' }
let(:logger) { Carto::Common::Logger.new }
let(:account_type) { create(:account_type, account_type: account_type_literal) }
let(:original_rate_limit) { account_type.rate_limit }
describe '#run' do
context 'when the associated rate limit has changed' do
let(:new_rate_limit) { create(:rate_limits_pro) }
let(:rate_limit_params) { new_rate_limit.api_attributes }
before { account_type.save }
it 'updates the rate limit' do
command.run
expect(account_type.reload.rate_limit).to eq(new_rate_limit)
end
it 'enqueues a job to sync rate limits in Redis' do
Resque.expects(:enqueue).once.with(
Resque::UserJobs::RateLimitsJobs::SyncRedis,
account_type.account_type
)
command.run
end
end
context 'when the associated rate limit has not changed' do
let(:rate_limit_params) { original_rate_limit.api_attributes }
before { account_type.save }
it 'does not update the rate limit attributes' do
command.run
expect(account_type.reload.rate_limit).to eq(original_rate_limit)
end
it 'does not sync rate limit information in Redis' do
Resque.expects(:enqueue).never
command.run
end
it 'does not create a new rate limit record' do
expect { command.run }.to change(Carto::RateLimit, :count).by(0)
end
end
context 'when not receiving any rate limit data' do
let(:params) { { price_plan: { account_type: account_type_literal } } }
before { account_type.save }
it 'preserves the current rate limit and raises an error' do
Resque.expects(:enqueue).never
expect { command.run }.to raise_error(ActiveRecord::RecordInvalid)
expect(account_type.reload.rate_limit).to eq(original_rate_limit)
end
end
context 'when the account type does not exist' do
let(:rate_limit_params) {}
it 'raises an error' do
expect { command.run }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
end

@ -0,0 +1,33 @@
require 'spec_helper'
describe FeatureFlagCommands::Create do
let(:command) { described_class.new(params) }
let(:params) { { feature_flag: feature_flag_params } }
let(:feature_flag_params) { { id: id_param, restricted: true, name: name_param } }
let(:id_param) { (Carto::FeatureFlag.count * 10) + 1 }
let(:name_param) { 'new-name' }
let(:created_feature_flag) { Carto::FeatureFlag.find(id_param) }
describe '#run' do
context 'when everything is ok' do
before { command.run }
it 'creates the feature flag' do
expect(created_feature_flag).to be_present
expect(created_feature_flag.id).to eq(id_param)
expect(created_feature_flag.name).to eq(name_param)
expect(created_feature_flag.restricted).to eq(true)
end
end
context 'when error occurs' do
let(:name_param) { nil }
it 'raises an error and does not create any record' do
expect { command.run }.to raise_error(ActiveRecord::RecordInvalid)
expect { created_feature_flag }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
end

@ -0,0 +1,36 @@
require 'spec_helper'
describe FeatureFlagCommands::Delete do
let(:feature_flag) { create(:feature_flag) }
let(:command) { described_class.new(params) }
let(:params) { { feature_flag: { id: id_param } } }
let(:id_param) { feature_flag.id }
describe '#run' do
context 'when everything is ok' do
let(:user) { create(:user) }
it 'deletes the feature flag' do
command.run
expect { feature_flag.reload }.to raise_error(ActiveRecord::RecordNotFound)
end
it 'unlinks the feature flag from users' do
user.activate_feature_flag!(feature_flag)
expect do
command.run
end.to change(Carto::FeatureFlagsUser, :count).by(-1)
end
end
context 'when error occurs' do
let(:id_param) { 'not-found' }
it 'raises an error' do
expect { command.run }.to raise_error(ActiveRecord::RecordNotFound)
end
end
end
end

@ -0,0 +1,33 @@
require 'spec_helper'
describe FeatureFlagCommands::Update do
let(:feature_flag) { create(:feature_flag, restricted: false) }
let(:updated_feature_flag) { feature_flag.reload }
let(:command) { described_class.new(params) }
let(:params) { { feature_flag: feature_flag_params } }
let(:feature_flag_params) { { id: id_param, restricted: restricted_param, name: name_param } }
let(:id_param) { feature_flag.id }
let(:restricted_param) { true }
let(:name_param) { 'new-name' }
describe '#run' do
context 'when everything is ok' do
before { command.run }
it 'updates the corresponding attributes' do
expect(updated_feature_flag.name).to eq(name_param)
expect(updated_feature_flag.restricted).to eq(restricted_param)
end
end
context 'when error occurs' do
let(:id_param) { 'not-found' }
it 'raises an error and does not update attributes' do
expect { command.run }.to raise_error(ActiveRecord::RecordNotFound)
expect(updated_feature_flag.name).not_to eq(name_param)
expect(updated_feature_flag.restricted).not_to eq(restricted_param)
end
end
end
end

@ -1,7 +1,7 @@
require 'spec_helper'
describe OrganizationCommands::Update do
let(:organization) { create_organization_with_users(seats: 10) }
let(:organization) { create(:organization_with_users, seats: 10) }
let(:command) { described_class.new(params) }
describe '#run' do

@ -1,16 +1,10 @@
require 'helpers/unique_names_helper'
include UniqueNamesHelper
FactoryGirl.define do
factory :feature_flag, class: 'Carto::FeatureFlag' do
sequence(:id)
sequence(:name) { |n| "feature-flag-name-#{n}" }
restricted { true }
factory :feature_flag, class: Carto::FeatureFlag do
id { unique_integer }
name { unique_name('ff') }
restricted true
trait(:restricted) { restricted true }
trait(:not_restricted) { restricted false }
trait(:restricted) { restricted { true } }
trait(:not_restricted) { restricted { false } }
end
end

@ -1,144 +0,0 @@
require_relative '../../acceptance_helper'
require 'helpers/account_types_helper'
describe Superadmin::AccountTypesController do
include AccountTypesHelper
describe '#create' do
before(:each) do
@account_type = create_account_type_fg('PRO')
@account_type_param = {
account_type: "Individual",
rate_limit: @account_type.rate_limit.api_attributes
}
end
after(:each) do
@account_type.destroy
Carto::AccountType.where(account_type: "Individual").each(&:destroy)
end
it 'should create account_type' do
expect {
post superadmin_account_types_url, { account_type: @account_type_param }.to_json, superadmin_headers
response.status.should == 201
}.to change(Carto::AccountType, :count).by(1)
end
it 'should update account_type if it already exists' do
@account_type_param[:account_type] = @account_type.account_type
expect {
post superadmin_account_types_url, { account_type: @account_type_param }.to_json, superadmin_headers
response.status.should == 201
}.to change(Carto::AccountType, :count).by(0)
end
end
describe '#update' do
before(:each) do
Carto::AccountType.where(account_type: "PRO").each(&:destroy)
@account_type = create_account_type_fg('PRO')
@rate_limits = FactoryGirl.create(:rate_limits_custom)
@account_type_param = {
account_type: @account_type.account_type,
rate_limit: @rate_limits.api_attributes
}
end
after(:each) do
@rate_limits.destroy
@account_type.destroy
end
it 'should update an account type' do
::Resque.expects(:enqueue)
.once
.with(Resque::UserJobs::RateLimitsJobs::SyncRedis, @account_type.account_type)
expect {
put superadmin_account_type_url(@account_type.account_type),
{ account_type: @account_type_param }.to_json,
superadmin_headers
@account_type.reload
@account_type.rate_limit.api_attributes.should eq @rate_limits.api_attributes
}.to change(Carto::RateLimit, :count).by(0)
end
it 'should not update an account type with empty rate limits' do
::Resque.expects(:enqueue).never
api_attributes = @account_type.rate_limit.api_attributes
put_json superadmin_account_type_url(@account_type.account_type),
{ account_type: { account_type: @account_type.account_type } }.to_json,
superadmin_headers do |response|
response.status.should == 422
response.body[:error].should =~ /ERROR. rate_limit object is not valid/
@account_type.rate_limit.api_attributes.should eq api_attributes
end
end
it 'should raise an error if non-existent account type' do
::Resque.expects(:enqueue).never
put_json superadmin_account_type_url("non_existent"),
{ account_type: { account_type: @account_type.account_type } }.to_json,
superadmin_headers do |response|
response.status.should == 404
response.body[:error].should =~ /ERROR. account_type not found/
end
end
it 'should not sync redis if rate limits do not change' do
::Resque.expects(:enqueue).never
current_rate_limits = @account_type.rate_limit.api_attributes
@account_type_param = {
account_type: @account_type.account_type,
rate_limit: current_rate_limits
}
expect {
put superadmin_account_type_url(@account_type.account_type),
{ account_type: @account_type_param }.to_json,
superadmin_headers
@account_type.reload
@account_type.rate_limit.api_attributes.should eq current_rate_limits
}.to_not raise_error
end
end
describe '#destroy' do
before(:each) do
Carto::AccountType.where(account_type: "PRO").each(&:destroy)
@account_type = create_account_type_fg('PRO')
end
after(:each) do
@account_type.destroy
end
it 'should destroy account type' do
expect {
delete superadmin_account_type_url(@account_type.account_type), nil, superadmin_headers
}.to change(Carto::AccountType, :count).by(-1)
expect {
Carto::AccountType.find(@account_type.id)
}.to raise_error(ActiveRecord::RecordNotFound)
end
it 'should raise an error if non-existent account type' do
delete_json superadmin_account_type_url("non_existent"), nil, superadmin_headers do |response|
response.status.should == 404
response.body[:error].should =~ /ERROR. account_type not found/
end
end
end
end

@ -1,59 +0,0 @@
require_relative '../../acceptance_helper'
describe Carto::FeatureFlag do
let(:feature_flag) { create(:feature_flag) }
describe '#create' do
let(:feature_flag) { build(:feature_flag) }
context 'when everything is OK' do
it 'should create feature flag' do
payload = { feature_flag: feature_flag.attributes }.to_json
expect {
post superadmin_feature_flags_url, payload, superadmin_headers
}.to change(Carto::FeatureFlag, :count).by(1)
expect(response.status).to eq(204)
end
end
context 'when an error occurs' do
it 'returns an error' do
post superadmin_feature_flags_url, {}.to_json, superadmin_headers
expect(response.status).to eq(500)
end
end
end
describe '#update' do
it 'should update feature flag name' do
payload = { feature_flag: feature_flag.attributes.merge('name' => 'new_name') }.to_json
put superadmin_feature_flag_url(feature_flag.id), payload, superadmin_headers
expect(feature_flag.reload.name).to eq('new_name')
end
end
describe '#destroy' do
let(:payload) { {}.to_json }
let(:user) { create(:user) }
before { feature_flag.save! }
it 'should destroy feature flag' do
expect {
delete superadmin_feature_flag_url(feature_flag.id), payload, superadmin_headers
}.to change(Carto::FeatureFlag, :count).by(-1)
end
it 'should destroy feature flag user relations' do
user.activate_feature_flag!(feature_flag)
expect {
delete superadmin_feature_flag_url(feature_flag.id), payload, superadmin_headers
}.to change(Carto::FeatureFlagsUser, :count).by(-1)
end
end
end

@ -3,8 +3,10 @@ require_relative './rspec_configuration'
require 'helpers/spec_helper_helpers'
require 'helpers/named_maps_helper'
require 'helpers/unique_names_helper'
require './spec/support/message_broker_stubs'
require './spec/support/shared_entities_spec_helper'
require 'database_cleaner/active_record'
require 'support/database_cleaner'
require 'support/message_broker_stubs'
require 'support/shared_entities_spec_helper'
# This file is copied to spec/ when you run 'rails generate rspec:install'
ENV['RAILS_ENV'] ||= 'test'

@ -0,0 +1,14 @@
##
# WARNING: the usage of this helper produces some weird hangups in PG12 builds.
# Use with caution until its integration is more spreaded
shared_context 'with DatabaseCleaner' do
around do |example|
DatabaseCleaner[:active_record].strategy = :truncation, { only: %w(account_types) }
DatabaseCleaner.start
example.run
DatabaseCleaner.clean
DatabaseCleaner[:active_record].strategy = nil
end
end
Loading…
Cancel
Save