Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Cinstance N+1 issues clean-up #3889

Draft
wants to merge 13 commits into
base: master
Choose a base branch
from
Draft

Cinstance N+1 issues clean-up #3889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c16f40d
THREESCALE-11139 simplify applications query
akostadinov Jul 10, 2024
8ed62c9
optimize application queries further
akostadinov Jul 11, 2024
f788d8d
Bullet optimization of Admin::Api::ApplicationsController
akostadinov Jul 11, 2024
634e6ec
UnoptimizedQuery ApplicationPlan => [:original]
akostadinov Aug 3, 2024
081a8c4
debugging info on failure
akostadinov Aug 4, 2024
37a6b21
eager loading :default_application_plan of Service
akostadinov Aug 4, 2024
4e95548
fix user can access Cinstance
akostadinov Aug 6, 2024
ef6760b
access cinstance service directly not through plan
akostadinov Sep 11, 2024
aaea00d
remove some n+1 issues
akostadinov Sep 11, 2024
82e5492
allow tracing SQL queries
akostadinov Sep 12, 2024
01d602c
fix N+1s for NginxesController
akostadinov Sep 12, 2024
53d0222
N+1 in signup controller
akostadinov Sep 12, 2024
62b07d9
fix cinstances N+1 bullets
akostadinov Sep 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,8 @@ group :test do
end

group :development, :test do
gem 'active_record_query_trace'

gem 'bootsnap', '~> 1.16'
gem 'bullet', '~> 6.1.5'
gem 'colorize'
Expand Down
3 changes: 3 additions & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,8 @@ GEM
erubi (~> 1.4)
rails-dom-testing (~> 2.0)
rails-html-sanitizer (~> 1.1, >= 1.2.0)
active_record_query_trace (1.8.2)
activerecord (>= 6.0.0)
activejob (6.1.7.8)
activesupport (= 6.1.7.8)
globalid (>= 0.3.6)
Expand Down Expand Up @@ -942,6 +944,7 @@ DEPENDENCIES
3scale_time_range (= 0.0.6)
RedCloth (~> 4.3)
active-docs!
active_record_query_trace
activejob-uniqueness
activemerchant (~> 1.107.4)
activemodel-serializers-xml
Expand Down
2 changes: 1 addition & 1 deletion app/concerns/new_application_form.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def buyers
end

def products
paginated_products.map { |p| ServicePresenter.new(p).new_application_data.as_json }
paginated_products.includes(:default_application_plan).map { |p| ServicePresenter.new(p).new_application_data.as_json }
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you identify the N+1 scenarios? From Bullet logs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bullet errors are enabled in testing but many are whitelisted to avoid breaking test suite. See the environment file.

end

def application_defined_fields_data(provider)
Expand Down
15 changes: 13 additions & 2 deletions app/controllers/admin/api/accounts_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,22 +22,24 @@ def index
def find
buyer_account = find_buyer_account
authorize! :read, buyer_account
respond_with(buyer_account)
respond_with(to_present(buyer_account))
end

# Account Read
# GET /admin/api/accounts/{id}.xml
def show
authorize! :read, buyer_account

respond_with(buyer_account)
respond_with(to_present(buyer_account))
end

# Account Update
# PUT /admin/api/accounts/{id}.xml
def update
authorize! :update, buyer_account

to_present(buyer_account)

buyer_account.vat_rate = params[:vat_rate].to_f if params[:vat_rate]
buyer_account.settings.attributes = billing_params
buyer_account.update_with_flattened_attributes(flat_params)
Expand Down Expand Up @@ -71,6 +73,7 @@ def change_plan
def approve
authorize! :approve, buyer_account

to_present buyer_account
buyer_account.approve

respond_with(buyer_account)
Expand All @@ -81,6 +84,7 @@ def approve
def reject
authorize! :reject, buyer_account

to_present buyer_account
buyer_account.reject

respond_with(buyer_account)
Expand All @@ -91,6 +95,7 @@ def reject
def make_pending
authorize! :update, buyer_account

to_present buyer_account
buyer_account.make_pending

respond_with(buyer_account)
Expand All @@ -110,6 +115,12 @@ def buyer_account
@buyer_account ||= buyer_accounts.find(params[:id])
end

def to_present(accounts)
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the method should be called preload!? Calling it to_present makes me think of a conversion that doesn't modify the original given object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this has changed in the merged PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same one as in the other comments #3845

# ActiveRecord::Associations::Preloader.new(records: Array(accounts), associations: [:annotations, {bought_plans: %i[original]}]).call # Rails 7.x
ActiveRecord::Associations::Preloader.new.preload(Array(accounts), [:annotations, {bought_plans: %i[original]}])
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure how this Preloader class works. Is the data kept in the memory forever? or only for this controller instance life?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the associated objects are preloaded into the array of objects. Similar to how #includes on associations works. So you can say it is in memory until normal garbage collection takes place.

accounts
end

def buyer_users
@buyer_users ||= current_account.buyer_users
end
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/admin/api/application_plans_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class Admin::Api::ApplicationPlansController < Admin::Api::ServiceBaseController
# Application Plan List
# GET /admin/api/services/{service_id}/application_plans.xml
def index
respond_with(application_plans)
respond_with(application_plans.includes(:original, :issuer))
end

# Application Plan Create
Expand Down
16 changes: 5 additions & 11 deletions app/controllers/admin/api/applications_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ class Admin::Api::ApplicationsController < Admin::Api::BaseController
# GET /admin/api/applications.xml
def index
apps = applications.scope_search(search)
.serialization_preloading.paginate(:page => current_page, :per_page => per_page)
.serialization_preloading(request.format).paginate(:page => current_page, :per_page => per_page)
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

different preloading rules for different requested format (xml vs json). So a parameter had to be added. But this is already upstream, need to rebase.

respond_with(apps)
end

Expand All @@ -24,25 +24,19 @@ def application_filter_params
end

def applications
@applications ||= begin
cinstances = current_account.provided_cinstances.where(service: accessible_services)
if (service_id = params[:service_id])
cinstances = cinstances.where(service_id: service_id)
end
cinstances
end
@applications ||= current_account.provided_cinstances.merge(accessible_services)
end

def application
@application ||= case

when user_key = params[:user_key]
when param_key = params[:user_key]
# TODO: these scopes should be in model layer
# but there is scope named by_user_key already
applications.joins(:service).where("(services.backend_version = '1' AND cinstances.user_key = ?)", user_key).first!
applications.where.has { (service.backend_version == '1') & (user_key == param_key) }.first!

when app_id = params[:app_id]
applications.joins(:service).where("(services.backend_version <> '1' AND cinstances.application_id = ?)", app_id).first!
applications.where.has { (service.backend_version != '1') & (application_id == app_id) }.first!

else
applications.find(params[:application_id] || params[:id])
Expand Down
1 change: 1 addition & 0 deletions app/lib/apicast/provider_source.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ def attributes_for_proxy
]
}

ActiveRecord::Associations::Preloader.new.preload(provider, {services: [:service_tokens, {backend_api_configs: :backend_api, proxy: [:gateway_configuration, {proxy_rules: :metric}]}]})
provider.as_json(hash).merge(timestamp: Time.now.utc.iso8601)
end

Expand Down
2 changes: 1 addition & 1 deletion app/lib/applications_controller_methods.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def find_states
end

def find_cinstance
@cinstance = accessible_not_bought_cinstances.includes(plan: %i[service original plan_metrics pricing_rules])
@cinstance = accessible_not_bought_cinstances.includes(plan: %i[service pricing_rules])
.find(params[:id])
end

Expand Down
2 changes: 1 addition & 1 deletion app/lib/backend_api_logic/routing_policy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ def policy_chain
end

def with_subpaths?
backend_api_configs.with_subpath.any?
backend_api_configs.any?(&:with_subpath?)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is not the original better? It scopes the results. Your modified version always returns all configs and then calls :with_subpath? for each one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was already merged with #3845

If you look from the perspective of taking a service and calling this method on it, what you say makes sense. I fixed a number of N+1 though with the aforementioned PR and I don't remember which test it was related to. Also I looked at a couple of requests and optimized them to a maximum least number of queries.

I think in this case, this change is based on the premises that we have the backend_api_configs already preloaded for the service(s) we deal with. So calling with_subpath? on each is more effective than performing a new database query. Applying a scope results in a new query.

In the other PR I have added active_record_query_trace gem and I assume it showed to me that a query came from this line where I didn't expect a query at this point.

I think even if backend_api_configs is not preloaded, it wouldn't be a huge deal because I don't expect too many backends in services. And it will still be one query, although with more data returned than the original code. It will also load the backend_api_configs into the respective service in case they are further needed.

If you have spotted a particular call that is less efficient this way, we may think about it. But I think it might likely be an edge case when a single service is involved. But with original code I don't see how we can avoid N+1 when many services are loaded at once and we want to preload everything needed for presentation. As the original code will still try to perform a new query for each service.

Hope explanation makes sense. But better comment further on the original (merged) PR because I will be rebasing this one to avoid all the extra commits already in master.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, so data is preloaded and you save a query. Fine.

end

class Builder
Expand Down
1 change: 1 addition & 0 deletions app/lib/signup/plans_with_defaults.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ def service_plan_errors
end

def any_plan_for?(issuer:, plan_type:)
ActiveRecord::Associations::Preloader.new.preload(plans[plan_type], [:issuer])
plans[plan_type].any? { |plan| plan.issuer == issuer }
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/account.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,7 +544,7 @@ def on_trial?
# Grabs the support_email if defined, otherwise falls back to the email of first admin. Dog.
def support_email
se = self[:support_email]
se.presence || admins.first&.email
se.presence || first_admin&.email
end

def finance_support_email
Expand Down
4 changes: 4 additions & 0 deletions app/models/backend_api_config.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ class BackendApiConfig < ApplicationRecord

delegate :proxy, to: :service, allow_nil: true

def with_subpath?
path != ConfigPath::EMPTY_PATH
end

def path=(value)
super(ConfigPath.new(value).path)
end
Expand Down
18 changes: 14 additions & 4 deletions app/models/cinstance.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,9 @@ def validate_plan_is_unique?
}

def self.provided_by(account)
joins(:service).references(:service).merge(Service.of_account(account)).readonly(false)
# we can access service through plan but also keep service.id in sync with plan.service.id
# this is a simpler way to do the query used historically
joins(:service).where.has { service.sift(:of_account, account) }
end

scope :not_bought_by, ->(account) { where.has { user_account_id != account.id } }
Expand Down Expand Up @@ -199,9 +201,17 @@ def self.by_service(service)

# maybe move both limit methods to their models?

def self.serialization_preloading
includes(:application_keys, :plan, :user_account,
service: [:account, :default_application_plan])
def self.serialization_preloading(format = nil)
# With Rails 6.1 trying to include plan->issuer without service results in
# > Cannot eagerly load the polymorphic association :issuer
# When both have the same sub-includes, cache takes care of the duplicate queries.
service_includes = %i[proxy account]
plan_includes = [{issuer: service_includes}]
if format == "xml"
service_includes << :default_application_plan
plan_includes << :original
end
includes(:user_account, service: service_includes, plan: plan_includes)
end


Expand Down
6 changes: 5 additions & 1 deletion app/models/service.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,11 @@ class Service < ApplicationRecord # rubocop:disable Metrics/ClassLength
service.has_many :api_docs_services, class_name: 'ApiDocs::Service'
end

scope :of_account, ->(account) { where.has { account_id == account.id } }
sifter :of_account do |account|
account_id == account.id
end

scope :of_account, ->(account) { where.has { sift(:of_account, account) } }
Comment on lines +63 to +67
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is sifter better than a regular scope?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sifters can be applied to other models, scopes only to associations of same model


has_one :proxy, dependent: :destroy, inverse_of: :service, autosave: true

Expand Down
2 changes: 1 addition & 1 deletion app/presenters/applications_index_presenter.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ def raw_applications
def applications
@applications ||= raw_applications.scope_search(search)
.order_by(*sorting_params)
.preload(:service, user_account: %i[admin_user], plan: %i[pricing_rules])
.includes(plan: %i[pricing_rules])
.paginate(pagination_params)
.decorate
end
Expand Down
8 changes: 7 additions & 1 deletion config/abilities/provider_member.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,13 @@
can :create, Account
can :update, Account if account.provider_can_use?(:service_permissions)

can %i[read show edit update], Cinstance, user.accessible_cinstances.where_values_hash
# Using historical optimized way and leave canonical way (through plan) commented out below
# The resulting hash presently is something like {"type"=>"Cinstance", "service_id"=>[ids..]}
can %i[read show edit update], Cinstance, Cinstance.permitted_for(user).where_values_hash
# can %i[read show edit update], Cinstance, user.accessible_cinstances do |cinstance|
# cinstance.plan&.issuer_type == "Service" && cinstance.plan.issuer.account == user.account &&
# (!user.forbidden_some_services? || user.member_permission_service_ids.include?(cinstance.plan.issuer.id))
# end

# abilities for buyer users
can %i[read update update_role destroy suspend unsuspend], User, account: { provider_account_id: user.account_id }
Expand Down
15 changes: 0 additions & 15 deletions config/environments/test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,8 +120,6 @@
Bullet.add_safelist class_name: "Alert", type: :n_plus_one_query, association: :cinstance
Bullet.add_safelist class_name: "ApiDocs::Service", type: :unused_eager_loading, association: :service
Bullet.add_safelist class_name: "ApplicationPlan", type: :n_plus_one_query, association: :customizations
Bullet.add_safelist class_name: "ApplicationPlan", type: :n_plus_one_query, association: :issuer
Bullet.add_safelist class_name: "ApplicationPlan", type: :n_plus_one_query, association: :original
Bullet.add_safelist class_name: "ApplicationPlan", type: :n_plus_one_query, association: :pricing_rules
Bullet.add_safelist class_name: "ApplicationPlan", type: :n_plus_one_query, association: :usage_limits
Bullet.add_safelist class_name: "ApplicationPlan", type: :unused_eager_loading, association: :issuer
Expand All @@ -133,17 +131,6 @@
Bullet.add_safelist class_name: "CMS::File", type: :n_plus_one_query, association: :provider
Bullet.add_safelist class_name: "CMS::Page", type: :n_plus_one_query, association: :provider
Bullet.add_safelist class_name: "CMS::Page", type: :n_plus_one_query, association: :section
Bullet.add_safelist class_name: "Cinstance", type: :n_plus_one_query, association: :plan
Bullet.add_safelist class_name: "Cinstance", type: :n_plus_one_query, association: :service
Bullet.add_safelist class_name: "Cinstance", type: :n_plus_one_query, association: :user_account
Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :plan
Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :service
Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :service
Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :service
Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :plan
Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :user_account
Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :user_account
Bullet.add_safelist class_name: "Cinstance", type: :unused_eager_loading, association: :service
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't know about this list. Nice to remove items from it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://issues.redhat.com/browse/THREESCALE-9973

Bullet.add_safelist class_name: "Invoice", type: :counter_cache, association: :payment_transactions
Bullet.add_safelist class_name: "Invoice", type: :n_plus_one_query, association: :buyer_account
Bullet.add_safelist class_name: "Invoice", type: :n_plus_one_query, association: :provider_account
Expand All @@ -170,10 +157,8 @@
Bullet.add_safelist class_name: "Service", type: :counter_cache, association: :backend_api_configs
Bullet.add_safelist class_name: "Service", type: :counter_cache, association: :cinstances
Bullet.add_safelist class_name: "Service", type: :n_plus_one_query, association: :account
Bullet.add_safelist class_name: "Service", type: :n_plus_one_query, association: :default_application_plan
Bullet.add_safelist class_name: "Service", type: :n_plus_one_query, association: :default_service_plan
Bullet.add_safelist class_name: "Service", type: :n_plus_one_query, association: :metrics
Bullet.add_safelist class_name: "Service", type: :n_plus_one_query, association: :proxy
Bullet.add_safelist class_name: "Service", type: :unused_eager_loading, association: :application_plans
Bullet.add_safelist class_name: "ServiceContract", type: :n_plus_one_query, association: :plan
Bullet.add_safelist class_name: "ServiceContract", type: :n_plus_one_query, association: :user_account
Expand Down
5 changes: 5 additions & 0 deletions config/initializers/active-record-query-trace.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
if (Rails.env.development? || Rails.env.test?) && ENV["TRACE_SQL"] == "1"
ActiveRecordQueryTrace.enabled = true
ActiveRecordQueryTrace.level = :app
Rails.application.config.log_level = :debug
end
3 changes: 1 addition & 2 deletions lib/developer_portal/app/controllers/developer_portal/admin/plans_widget_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ def index
end

@wizard = params[:wizard].to_s == 'true'
@plans = @service.application_plans.not_custom.published.to_a
@plans = @service.application_plans.not_custom.published.includes(:issuer).to_a
@plans.delete @plan
end

end
25 changes: 25 additions & 0 deletions test/integration/user-management-api/applications_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,14 +32,39 @@ def setup
User.any_instance.stubs(:has_access_to_all_services?).returns(false)
user = FactoryBot.create(:member, account: @provider, admin_sections: ['partners'])
token = FactoryBot.create(:access_token, owner: user, scopes: 'account_management')
service_2 = FactoryBot.create(:service, account: @provider)
service_3 = FactoryBot.create(:service, account: @provider)
application_plan_2 = FactoryBot.create(:application_plan, issuer: service_2)
application_plan_3 = FactoryBot.create(:application_plan, issuer: service_3)
application_2 = FactoryBot.create(:cinstance, plan: application_plan_2, user_account: @buyer)
FactoryBot.create(:cinstance, plan: application_plan_3, user_account: @buyer)

get(admin_api_applications_path)
assert_response :forbidden
get admin_api_applications_path, params: { access_token: token.value }
assert_response :success
assert_select "applications/application", false

User.any_instance.expects(:member_permission_service_ids).returns([@service.id]).at_least_once
get admin_api_applications_path, params: { access_token: token.value, service_id: @service.id + 1 }
assert_response :success
assert_select "applications/application", false

User.any_instance.expects(:member_permission_service_ids).returns([@service.id, service_2.id]).at_least_once
get admin_api_applications_path, params: { access_token: token.value }
assert_response :success
assert_select "applications/application", 2
assert_select "applications/application/id", @application.id.to_s
assert_select "applications/application/service_id", @service.id.to_s
assert_select "applications/application/id", application_2.id.to_s
assert_select "applications/application/service_id", service_2.id.to_s

User.any_instance.expects(:member_permission_service_ids).returns([@service.id, service_2.id]).at_least_once
get admin_api_applications_path, params: { access_token: token.value, service_id: @service.id }
assert_response :success
assert_select "applications/application", 1
assert_select "applications/application/id", @application.id.to_s
assert_select "applications/application/service_id", @service.id.to_s
end

# Provider key
Expand Down
6 changes: 3 additions & 3 deletions test/unit/abilities/provider_member_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,9 +121,9 @@ def test_cinstances
service_2 = FactoryBot.create(:simple_service, account: @account)
service_3 = FactoryBot.create(:simple_service, account: @account)

plan_1 = FactoryBot.create(:simple_application_plan, service: service_1)
plan_2 = FactoryBot.create(:simple_application_plan, service: service_2)
plan_3 = FactoryBot.create(:simple_application_plan, service: service_3)
plan_1 = FactoryBot.create(:simple_application_plan, issuer: service_1)
plan_2 = FactoryBot.create(:simple_application_plan, issuer: service_2)
plan_3 = FactoryBot.create(:simple_application_plan, issuer: service_3)

app_1 = FactoryBot.create(:simple_cinstance, plan: plan_1)
app_2 = FactoryBot.create(:simple_cinstance, plan: plan_2)
Expand Down
2 changes: 1 addition & 1 deletion test/unit/finance/billing_strategy_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ def setup
canaries = FactoryBot.create_list(:provider_with_billing, 4).map(&:id)
ThreeScale.config.payments.expects(:billing_canaries).at_least_once.returns(canaries)

Finance::BillingStrategy.expects(:daily_async).with { |scope| scope.pluck(:account_id) == canaries }.returns(true)
Finance::BillingStrategy.expects(:daily_async).with { |scope| assert_equal canaries, scope.pluck(:account_id) }.returns(true)
assert Finance::BillingStrategy.daily_canaries
end

Expand Down