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.

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

[Dashboard] optimization #1856

Merged
merged 6 commits into from Jun 9, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions Gemfile.base
Expand Up @@ -166,6 +166,7 @@ gem 'slim-rails', '~> 3.2'
gem 'ruby-openid'

gem 'draper', '~> 3.0'
gem 'activerecord-precounter'
Copy link
Contributor

Choose a reason for hiding this comment

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

My 1st reaction was "what is that?" so I went to https://github.com/k0kubun/activerecord-precounter and saw:

Yet Another N+1 COUNT Query Killer for ActiveRecord, counter_cache alternative.
ActiveRecord::Precounter allows you to cache count of associated records by eager loading.

Great, but we already use counter_cache for some things, and I know that they behave VERY differently, so I wondered if we really need both or what was the plan for the future.

We spoke about it and counter_cache should eventually be completely replaced for this one because it is not as handy. Once this PR is merged, we will have a Jira issue for this goal 👍


group :development do
gem 'listen'
Expand Down
3 changes: 3 additions & 0 deletions Gemfile.lock
Expand Up @@ -153,6 +153,8 @@ GEM
activerecord (~> 5.0.0)
arel (~> 7.1.4)
ruby-plsql (>= 0.5.0)
activerecord-precounter (0.3.3)
activerecord (>= 5)
activesupport (5.0.7.2)
concurrent-ruby (~> 1.0, >= 1.0.2)
i18n (>= 0.7, < 2)
Expand Down Expand Up @@ -807,6 +809,7 @@ DEPENDENCIES
activemerchant (~> 1.77.0)
activemodel-serializers-xml
activerecord-oracle_enhanced-adapter (~> 1.7.0)
activerecord-precounter
acts-as-taggable-on (~> 4.0)
acts_as_list (~> 0.9.17)
acts_as_tree
Expand Down
3 changes: 3 additions & 0 deletions Gemfile.prod.lock
Expand Up @@ -154,6 +154,8 @@ GEM
activerecord (~> 5.0.0)
arel (~> 7.1.4)
ruby-plsql (>= 0.5.0)
activerecord-precounter (0.3.3)
activerecord (>= 5)
activesupport (5.0.7.2)
concurrent-ruby (~> 1.0, >= 1.0.2)
i18n (>= 0.7, < 2)
Expand Down Expand Up @@ -808,6 +810,7 @@ DEPENDENCIES
activemerchant (~> 1.77.0)
activemodel-serializers-xml
activerecord-oracle_enhanced-adapter (~> 1.7.0)
activerecord-precounter
acts-as-taggable-on (~> 4.0)
acts_as_list (~> 0.9.17)
acts_as_tree
Expand Down
4 changes: 3 additions & 1 deletion app/controllers/provider/admin/dashboards_controller.rb
Expand Up @@ -13,7 +13,9 @@ def show
# .scoped(:include => [ :message => [ :sender ]])
#
# but 'Cannot eagerly load the polymorphic association :sender'
@services = current_user.accessible_services
@services = current_user.accessible_services.includes(:proxy, :cinstances, :account)
ActiveRecord::Precounter.new(@services).precount(:unread_alerts, :api_docs_services, :cinstances_with_traffic, :sandbox_proxy_configs)

@messages_presenter = current_presenter
@unread_messages_presenter = unread_messages_presenter
end
Expand Down
17 changes: 10 additions & 7 deletions app/helpers/menu_helper.rb
@@ -1,3 +1,5 @@
# frozen_string_literal: true

module MenuHelper
def main_menu_item(id, label, path, options = {})
fake_active = respond_to?(:active_upgrade_notice) && (id == active_upgrade_notice)
Expand Down Expand Up @@ -27,9 +29,7 @@ def menu_item(label, path, options = {})

def menu_link(level, title, path, options = {}, li_options = {}, &block)
current_title = options.delete(:title) || title
if active_menu?(level, current_title) || current_page?(path)
li_options[:class] = :active
end
li_options[:class] = :active if active_menu?(level, current_title) || current_page?(path)

# humanize only if it is NOT a string, otherwise it would destroy capitalization
title = title.to_s.humanize.camelize unless title.is_a?(String)
Expand Down Expand Up @@ -67,7 +67,7 @@ def link_to_switch_or_upgrade(name, options, path, switch)
if can?(:see, switch)
link_to(name, path, options)
elsif upgrade
options[:class].gsub!(/(?: fancybox |^fancybox$)/, ' ') if options[:class]
options[:class]&.gsub!(/(?: fancybox |^fancybox$)/, ' ')
upgrade_notice_link(switch, name, options)
end

Expand All @@ -85,9 +85,12 @@ def forcibly_denied_switch?(switch)
end

def api_selector_services
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't have a test and got too complicated/dangerous to don't have a test 😓
I wrote some in test/helpers/menu_helper_test.rb locally to be able to change the code having the tests.

require 'test_helper'

class MenuHelperTest < ActionView::TestCase
  def setup
    @provider = FactoryBot.create(:provider_account)
  end

  attr_reader :provider

  test 'switch_link with finance globally disabled' do
    expects(:forcibly_denied_switch?).with(:finance).returns(true)
    assert_nil switch_link('Billing', root_path, switch: :finance, upgrade_notice: true)
  end

  test 'switch_link with finance globally enabled' do
    expects(:forcibly_denied_switch?).with(:finance).returns(false)
    link = switch_link('Billing', root_path, switch: :finance, upgrade_notice: true)
    assert_equal upgrade_notice_link(:finance, 'Billing'), link
  end

  test 'forcibly_denied_switch?' do
    Settings.stubs(globally_denied_switches: [])
    assert @provider.settings.finance.denied?
    refute forcibly_denied_switch?(:finance)

    Settings.stubs(globally_denied_switches: [:finance])
    refute forcibly_denied_switch?(:finance)
  end

  test 'forcibly_denied_switch? for master' do
    ThreeScale.config.stubs(onpremises: false)
    @provider = master_account
    assert @provider.settings.finance.denied?
    refute forcibly_denied_switch?(:finance)

    ThreeScale.config.stubs(onpremises: true)
    assert @provider.settings.finance.denied?
    assert forcibly_denied_switch?(:finance)
  end

  test 'api_selector_services for tenant logged in without api_as_product' do
    FactoryBot.create(:simple_service, account: provider)
    FactoryBot.create(:simple_service, account: provider, state: Service::DELETE_STATE)

    rolling_updates_on
    rolling_update(:api_as_product, enabled: false)

    assert_equal provider.accessible_services.size, api_selector_services.size
    api_selector_services.each do |api_decorated|
      assert_equal ServiceDecorator, api_decorated.class
      assert_includes provider.accessible_services.pluck(:id), api_decorated.id
    end
  end

  test 'api_selector_services for tenant logged in with api_as_product' do
    FactoryBot.create(:simple_service, account: provider)
    FactoryBot.create(:simple_service, account: provider, state: Service::DELETE_STATE)
    FactoryBot.create(:backend_api, account: provider)
    FactoryBot.create(:backend_api, account: provider, state: BackendApi::DELETED_STATE)

    rolling_updates_on
    rolling_update(:api_as_product, enabled: true)

    services_decorated, backend_apis_decorated = api_selector_services.partition { |api_decorated| api_decorated.class == ServiceDecorator }
    assert_equal provider.accessible_services.size, services_decorated.size
    assert_equal provider.backend_apis.accessible.size, backend_apis_decorated.size
    assert_same_elements provider.accessible_services.pluck(:id), services_decorated.map(&:id)
    assert_same_elements provider.backend_apis.accessible.pluck(:id), backend_apis_decorated.map(&:id)
    assert backend_apis_decorated.all? { |api_decorated| api_decorated.class == BackendApiDecorator }
  end

  test 'api_selector_services if not logged in' do
    @current_account = nil
    assert_empty api_selector_services
  end

  test 'api_selector_services in developer portal' do
    @current_account = FactoryBot.create(:simple_buyer, provider_account: provider)
    assert_empty api_selector_services
  end


  protected

  def current_user
    @current_user ||= current_account.try!(:admin_user)
  end

  def current_account
    return @current_account if defined? @current_account
    @current_account = @provider
  end

  def ability
    @ability ||= Ability.new(current_user)
  end

  def can?(*args)
    ability.can?(*args)
  end
end

Copy link
Contributor

Choose a reason for hiding this comment

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

I know that it is not your fault, but this name at this point is a bit misleading... it sounds like it is only services 😕

@api_selector_services ||= (site_account.provider? && logged_in? ? current_user.accessible_services : Service.none).decorate
@api_selector_services.concat(current_user.account.backend_apis.decorate) if current_account.provider_can_use?(:api_as_product)
@api_selector_services
@api_selector_services ||= if current_account && current_account.provider?
accessible_backend_apis = current_account.provider_can_use?(:api_as_product) ? current_user.account.backend_apis.accessible : BackendApi.none
current_user.accessible_services.decorate.concat(accessible_backend_apis.decorate)
else
Service.none.decorate
end
end

def audience_link
Expand Down
8 changes: 6 additions & 2 deletions app/helpers/provider/admin/dashboards_helper.rb
Expand Up @@ -39,11 +39,15 @@ def dashboard_secondary_link(link_text, path, options = {})
safe_wrap_with_parenthesis(dashboard_navigation_link(link_text, path, options))
end

def dashboard_collection_link(singular_name, collection, path, options = {})
link_text = pluralize(number_to_human(collection.size), singular_name, options.fetch(:plural, nil))
def dashboard_collection_size_link(singular_name, size, path, options = {})
link_text = pluralize(number_to_human(size), singular_name, options.fetch(:plural, nil))
dashboard_navigation_link(link_text, path, options)
end

def dashboard_collection_link(singular_name, collection, path, options = {})
dashboard_collection_size_link(singular_name, collection.size, path, options)
end

def dashboard_secondary_collection_link(singular_name, collection, path, options = {})
safe_wrap_with_parenthesis(dashboard_collection_link(singular_name, collection, path, options))
end
Expand Down
4 changes: 3 additions & 1 deletion app/lib/logic/provider_settings.rb
Expand Up @@ -29,8 +29,10 @@ def service_items_in_menu?
end

def multiservice?
return @multiservice_flag if defined? @multiservice_flag

account = buyer? ? provider_account : self
account.multiple_accessible_services?
@multiservice_flag = account.multiple_accessible_services?
end

def multiple_accessible_services?(scope = nil)
Expand Down
1 change: 1 addition & 0 deletions app/models/alert.rb
Expand Up @@ -12,6 +12,7 @@ class Alert < ApplicationRecord

belongs_to :cinstance
belongs_to :account
belongs_to :service

has_one :user_account, through: :cinstance

Expand Down
2 changes: 1 addition & 1 deletion app/models/cinstance.rb
Expand Up @@ -129,7 +129,7 @@ def validate_plan_is_unique?
# intentional, because deprecated cinstances can still be used (so they are
# "live", in a way).
scope :live, -> { where(:state => ['live', 'deprecated'])}

scope :with_any_traffic, -> { where.not(first_traffic_at: nil) }
scope :active_since, ->(activity_time) {
where.has { first_daily_traffic_at >= activity_time }
}
Expand Down
5 changes: 5 additions & 0 deletions app/models/service.rb
Expand Up @@ -69,6 +69,8 @@ def self.columns
scope :of_account, ->(account) { where.has { account_id == account.id } }

has_one :proxy, dependent: :destroy, inverse_of: :service, autosave: true
has_many :proxy_configs, through: :proxy, inverse_of: :proxy
has_many :sandbox_proxy_configs, -> { sandbox }, class_name: 'ProxyConfig', inverse_of: :proxy

belongs_to :default_service_plan, class_name: 'ServicePlan'
belongs_to :default_application_plan, class_name: 'ApplicationPlan'
Expand All @@ -84,6 +86,7 @@ def self.columns
alias provided_plans issued_plans

has_many :cinstances, inverse_of: :service
has_many :cinstances_with_traffic, -> { with_any_traffic }, inverse_of: :service, class_name: 'Cinstance'
has_many :service_contracts, through: :service_plans #, :readonly => true

has_many :contracts, through: :issued_plans do #, :readonly => true
Expand All @@ -110,6 +113,8 @@ def cinstance
has_many :top_level_metrics, -> { includes(:children).top_level }, class_name: 'Metric'

has_many :service_tokens, inverse_of: :service, dependent: :destroy
has_many :alerts, inverse_of: :service
has_many :unread_alerts, -> { unread }, class_name: 'Alert', inverse_of: :service

scope :accessible, -> { where.not(state: DELETE_STATE) }
scope :deleted, -> { where(state: DELETE_STATE) }
Expand Down
Expand Up @@ -23,11 +23,10 @@ nav.DashboardNavigation
admin_service_applications_path(service),
icon_name: 'cubes'

- unread_alerts = current_account.buyer_alerts.by_service(service).unread
- if can?(:manage, :monitoring) && unread_alerts.any?
- if can?(:manage, :monitoring) && service.unread_alerts_count > 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as #1856 (comment)

li.DashboardNavigation-list-item
= dashboard_collection_link 'Alert',
unread_alerts,
service.unread_alerts_count,
admin_service_alerts_path(service),
icon_name: 'exclamation-triangle',
notice: true
Expand All @@ -44,14 +43,14 @@ nav.DashboardNavigation
// Active docs
- if can?(:manage, :plans)
li.DashboardNavigation-list-item
= dashboard_collection_link 'ActiveDoc',
service.api_docs_services,
= dashboard_collection_size_link 'ActiveDoc',
service.api_docs_services_count,
admin_service_api_docs_path(service),
icon_name: 'file-text'
// Integration
- if can?(:manage, :plans)
li.DashboardNavigation-list-item
- if service.has_traffic? || (apiap? && service.proxy.proxy_configs.sandbox.any?)
- if service.cinstances_with_traffic_count > 0 || (apiap? && service.sandbox_proxy_configs_count > 0)
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you sure that this performs better than this?:

        - if service.cinstances_with_traffic.exists? || (apiap? && service.sandbox_proxy_configs.exists?)

Because the exists? works with LIMIT 1 which implies that instead of counting them all, it only checks until it find the first one if there is any 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

- if has_out_of_date_configuration?(service)
= dashboard_navigation_link 'Integration',
path_to_service(service),
Expand Down
3 changes: 2 additions & 1 deletion config/webpacker.yml
Expand Up @@ -31,7 +31,8 @@ default: &default

development:
<<: *default
compile: true
compile: false
check_yarn_integrity: false
webpack_compile_output: true

dev_server:
Expand Down
61 changes: 56 additions & 5 deletions test/helpers/menu_helper_test.rb
@@ -1,12 +1,12 @@
require 'test_helper'

class MenuHelperTest < ActionView::TestCase

def setup
@provider = FactoryBot.create(:provider_account)
@ability = Ability.new(@provider.admins.first)
end

attr_reader :provider

test 'switch_link with finance globally disabled' do
expects(:forcibly_denied_switch?).with(:finance).returns(true)
assert_nil switch_link('Billing', root_path, switch: :finance, upgrade_notice: true)
Expand Down Expand Up @@ -38,13 +38,64 @@ def setup
assert forcibly_denied_switch?(:finance)
end

test 'api_selector_services for tenant logged in without api_as_product' do
FactoryBot.create(:simple_service, account: provider)
FactoryBot.create(:simple_service, account: provider, state: Service::DELETE_STATE)

rolling_updates_on
rolling_update(:api_as_product, enabled: false)

assert_equal provider.accessible_services.size, api_selector_services.size
api_selector_services.each do |api_decorated|
assert_equal ServiceDecorator, api_decorated.class
assert_includes provider.accessible_services.pluck(:id), api_decorated.id
end
end

test 'api_selector_services for tenant logged in with api_as_product' do
FactoryBot.create(:simple_service, account: provider)
FactoryBot.create(:simple_service, account: provider, state: Service::DELETE_STATE)
FactoryBot.create(:backend_api, account: provider)
FactoryBot.create(:backend_api, account: provider, state: BackendApi::DELETED_STATE)

rolling_updates_on
rolling_update(:api_as_product, enabled: true)

services_decorated, backend_apis_decorated = api_selector_services.partition { |api_decorated| api_decorated.class == ServiceDecorator }
assert_equal provider.accessible_services.size, services_decorated.size
assert_equal provider.backend_apis.accessible.size, backend_apis_decorated.size
assert_same_elements provider.accessible_services.pluck(:id), services_decorated.map(&:id)
assert_same_elements provider.backend_apis.accessible.pluck(:id), backend_apis_decorated.map(&:id)
assert backend_apis_decorated.all? { |api_decorated| api_decorated.class == BackendApiDecorator }
end

test 'api_selector_services if not logged in' do
@current_account = nil
assert_empty api_selector_services
end

test 'api_selector_services in developer portal' do
@current_account = FactoryBot.create(:simple_buyer, provider_account: provider)
assert_empty api_selector_services
end


protected

def current_user
@current_user ||= current_account.try!(:admin_user)
end

def current_account
@provider
return @current_account if defined? @current_account
@current_account = @provider
end

def ability
@ability ||= Ability.new(current_user)
end

def can?(*args)
@ability.can?(*args)
ability.can?(*args)
end
end
end
7 changes: 6 additions & 1 deletion test/unit/logic/provider_settings_test.rb
Expand Up @@ -21,13 +21,18 @@ class Logic::ProviderSettingsTest < ActiveSupport::TestCase
assert provider.has_visible_services_with_plans?
end

test '#multiservice?' do
test '#multiservice? true' do
provider = FactoryBot.build(:simple_provider)
buyer = FactoryBot.build(:simple_buyer, provider_account: provider)

provider.stubs(multiple_accessible_services?: true)
assert provider.multiservice?
assert buyer.multiservice?
end

test '#multiservice? false' do
provider = FactoryBot.build(:simple_provider)
buyer = FactoryBot.build(:simple_buyer, provider_account: provider)

provider.stubs(multiple_accessible_services?: false)
refute provider.multiservice?
Expand Down