Skip to content

Commit

Permalink
fixes #19035 - rewrite TopbarSweeper without rails-observers
Browse files Browse the repository at this point in the history
Moves from the observer object into two mixins, one on the model and one
on the top-level controllers to observe creates/updates/destroys on
monitored models. Replaces rails-observers as it lacks Rails 5 support.
  • Loading branch information
domcleal authored and lzap committed Mar 28, 2017
1 parent 567cb9f commit 0e52ebb
Show file tree
Hide file tree
Showing 17 changed files with 102 additions and 52 deletions.
1 change: 0 additions & 1 deletion Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ gem 'net-ssh'
gem 'net-ldap', '>= 0.8.0'
gem 'net-ping', :require => false
gem 'activerecord-session_store', '>= 0.1.1', '< 2'
gem 'rails-observers', '~> 0.1'
gem 'sprockets', '~> 3'
gem 'sprockets-rails', '>= 2.3.3', '< 3'
gem 'responders', '~> 2.0'
Expand Down
2 changes: 0 additions & 2 deletions app/controllers/api/base_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ class BaseController < ActionController::Base
before_action :session_expiry, :update_activity_time
around_action :set_timezone

cache_sweeper :topbar_sweeper

respond_to :json

after_action :log_response_body
Expand Down
2 changes: 0 additions & 2 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ class ApplicationController < ActionController::Base

attr_reader :original_search_parameter

cache_sweeper :topbar_sweeper

def welcome
if (model_of_controller.first.nil? rescue false)
@welcome = true
Expand Down
1 change: 1 addition & 0 deletions app/controllers/concerns/application_shared.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ module ApplicationShared
include Foreman::Controller::MigrationChecker
include Foreman::Controller::Authentication
include Foreman::Controller::Session
include Foreman::Controller::TopbarSweeper
include Foreman::ThreadSession::Cleaner
include FindCommon

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ def clear
def clear_current_taxonomy_from_session
taxonomy_class.current = nil
session[taxonomy_id] = nil
TopbarSweeper.expire_cache(self)
TopbarSweeper.expire_cache
end

def mismatches
Expand Down Expand Up @@ -222,6 +222,6 @@ def switch_taxonomy
taxonomy_class.current = @taxonomy
session[taxonomy_id] = @taxonomy ? @taxonomy.id : nil

TopbarSweeper.expire_cache(self)
TopbarSweeper.expire_cache
end
end
18 changes: 18 additions & 0 deletions app/controllers/concerns/foreman/controller/topbar_sweeper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
module Foreman
module Controller
module TopbarSweeper
extend ActiveSupport::Concern

included do
around_action :set_topbar_sweeper_controller
end

def set_topbar_sweeper_controller
::TopbarSweeper.instance.controller = self
yield
ensure
::TopbarSweeper.instance.controller = nil
end
end
end
end
33 changes: 0 additions & 33 deletions app/controllers/topbar_sweeper.rb

This file was deleted.

4 changes: 2 additions & 2 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ def logout
return
end

TopbarSweeper.expire_cache(self)
TopbarSweeper.expire_cache
sso_logout_path = get_sso_method.try(:logout_url)
session[:user] = @user = User.current = nil
if flash[:notice] || flash[:error]
Expand Down Expand Up @@ -149,7 +149,7 @@ def login_user(user)
uri = session.to_hash.with_indifferent_access[:original_uri]
session[:original_uri] = nil
set_current_taxonomies(user, {:session => session})
TopbarSweeper.expire_cache(self)
TopbarSweeper.expire_cache
redirect_to (uri || hosts_path)
end

Expand Down
13 changes: 13 additions & 0 deletions app/models/concerns/topbar_cache_expiry.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
module TopbarCacheExpiry
extend ActiveSupport::Concern

included do
after_create :expire_topbar_cache_within_controller
after_update :expire_topbar_cache_within_controller
after_destroy :expire_topbar_cache_within_controller
end

def expire_topbar_cache_within_controller
expire_topbar_cache if TopbarSweeper.instance.controller.present?
end
end
7 changes: 4 additions & 3 deletions app/models/filter.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
class Filter < ActiveRecord::Base
include Taxonomix
include Authorizable
include TopbarCacheExpiry

attr_writer :resource_type
attr_accessor :unlimited
Expand Down Expand Up @@ -140,9 +141,9 @@ def search_condition
searches.join(' and ')
end

def expire_topbar_cache(sweeper)
role.users.each { |u| u.expire_topbar_cache(sweeper) }
role.usergroups.each { |g| g.expire_topbar_cache(sweeper) }
def expire_topbar_cache
role.users.each { |u| u.expire_topbar_cache }
role.usergroups.each { |g| g.expire_topbar_cache }
end

def disable_overriding!
Expand Down
5 changes: 3 additions & 2 deletions app/models/taxonomy.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ class Taxonomy < ActiveRecord::Base

include Authorizable
include NestedAncestryCommon
include TopbarCacheExpiry

serialize :ignore_types, Array

Expand Down Expand Up @@ -170,8 +171,8 @@ def dup
end
end

def expire_topbar_cache(sweeper)
(users+User.only_admin).each { |u| u.expire_topbar_cache(sweeper) }
def expire_topbar_cache
(users+User.only_admin).each { |u| u.expire_topbar_cache }
end

def parent_params(include_source = false)
Expand Down
6 changes: 3 additions & 3 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class User < ActiveRecord::Base
include UserTime
include UserUsergroupCommon
include Exportable
include TopbarCacheExpiry
audited :except => [:last_login_on, :password, :password_hash, :password_salt, :password_confirmation]

ANONYMOUS_ADMIN = 'foreman_admin'
Expand Down Expand Up @@ -428,9 +429,8 @@ def self.random_password(size = 16)
size.times.collect {|i| set[rand(set.size)] }.join
end

def expire_topbar_cache(sweeper)
return if sweeper.controller.nil?
sweeper.expire_fragment(TopbarSweeper.fragment_name(id))
def expire_topbar_cache
TopbarSweeper.expire_cache(self)
end

def external_usergroups
Expand Down
2 changes: 2 additions & 0 deletions app/models/user_role.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.

class UserRole < ActiveRecord::Base
include TopbarCacheExpiry

belongs_to :owner, :polymorphic => true
belongs_to :role

Expand Down
5 changes: 3 additions & 2 deletions app/models/usergroup.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ class Usergroup < ActiveRecord::Base
extend FriendlyId
friendly_id :name
include Parameterizable::ByIdName
include TopbarCacheExpiry
include UserUsergroupCommon

validates_lengths_from_database
Expand Down Expand Up @@ -66,8 +67,8 @@ def all_usergroups(group_list = [self], user_list = [])
group_list.uniq.sort
end

def expire_topbar_cache(sweeper)
users.each { |u| u.expire_topbar_cache(sweeper) }
def expire_topbar_cache
users.each { |u| u.expire_topbar_cache }
end

def add_users(userlist)
Expand Down
22 changes: 22 additions & 0 deletions app/services/topbar_sweeper.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
class TopbarSweeper
include Singleton
attr_accessor :controller

def expire_cache(user = User.current)
controller.expire_fragment(self.class.fragment_name(user.id)) if controller.present? && user.present?
end

class << self
delegate :expire_cache, to: :instance

def fragment_name(id = User.current.id)
"tabs_and_title_records-#{id}"
end

def expire_cache_all_users
User.unscoped.pluck(:id).each do |id|
Rails.cache.delete("views/#{fragment_name(id)}")
end
end
end
end
22 changes: 22 additions & 0 deletions test/integration/top_bar_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,27 @@ def setup
assert page.has_selector?('h1', :text => "Trends")
end

test "taxonomy switcher" do
with_controller_caching(DashboardController, LocationsController) do
visit root_path
within("li.org-switcher") do
assert has_selector?(:xpath, "//a[contains(@class, 'dropdown-toggle') and text() = 'Any Context']")
assert has_link?(taxonomies(:location1), href: select_location_path(taxonomies(:location1)))
click_link(taxonomies(:location1))
end

# Page change within location context
within("li.org-switcher") do
assert has_selector?(:xpath, "//a[contains(@class, 'dropdown-toggle') and text() = '#{taxonomies(:location1).name}']")
click_link('Any Location')
end

# Page change out of location context
within("li.org-switcher") do
assert has_selector?(:xpath, "//a[contains(@class, 'dropdown-toggle') and text() = 'Any Context']")
end
end
end

#PENDING - click on Menu Bar js
end
7 changes: 7 additions & 0 deletions test/integration_test_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,13 @@ def set_request_user(user)
user = users(user) unless user.is_a?(User)
create_cookie('test_user', user.login)
end

def with_controller_caching(*controller_klasses)
controller_klasses.each { |c| c.perform_caching = true }
yield
ensure
controller_klasses.each { |c| c.perform_caching = false }
end
end

class IntegrationTestWithJavascript < ActionDispatch::IntegrationTest
Expand Down

0 comments on commit 0e52ebb

Please sign in to comment.