On 04/25/2012 04:43 PM, Tzu-Mainn Chen wrote:
---
src/app/controllers/logs_controller.rb | 116 ++++++++++++++++++++++++++++++++
1 files changed, 116 insertions(+), 0 deletions(-)
create mode 100644 src/app/controllers/logs_controller.rb
diff --git a/src/app/controllers/logs_controller.rb
b/src/app/controllers/logs_controller.rb
new file mode 100644
index 0000000..1a65f46
--- /dev/null
+++ b/src/app/controllers/logs_controller.rb
@@ -0,0 +1,116 @@
+#
+# Copyright 2011 Red Hat, Inc.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#
http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+class LogsController< ApplicationController
+ before_filter :require_user
+
+ def index
+ clear_breadcrumbs
+ save_breadcrumb(logs_path)
+
+ load_headers
+ load_options
+ load_logs
+ respond_to do |format|
+ format.html
+ end
+ end
+
+ def filter
+ redirect_to_original({"source_type" => params[:source_type],
"pool_select" => params[:pool_select], "provider_select" =>
params[:provider_select], "owner_id" => params[:owner_id]})
+ end
+
+ protected
+
+ def load_logs
+ @source_type = params[:source_type].nil? ? "" : params[:source_type]
+ @pool_select = params[:pool_select].nil? ? "" : params[:pool_select]
+ @provider_select = params[:pool_select].nil? ? "" :
params[:provider_select]
^ shouldn't be params[:provider_select]?
+ @owner_id = params[:owner_id].nil? ? "" :
params[:owner_id]
+
+ conditions = []
+ if ( @source_type != "" )
there is no need to use brackets for simple conditions
+ conditions += ["source_type = ?", @source_type]
+ end
+
+ @events = Event.unscoped.find(:all,
+ :include => {:source => [:pool_family, :pool,
:owner]},
+ :conditions => conditions
+ )
+
+ @events = @events.find_all{|event|
+ match = true
+ if ( @provider_select != "" )
+ provider_option, provider_option_id = @provider_select.split(":")
+ if ( provider_option == "provider" )
+ if ( event.source.provider_account.provider.id.to_s != provider_option_id )
provider_account can be nil (if an account is deleted, instances are kept)
+ match = false
+ end
+ elsif ( provider_option == "provider_account" )
+ if ( event.source.provider_account.id.to_s != provider_option_id )
+ match = false
+ end
+ end
+ end
+ if ( @pool_select != ""&& match )
+ pool_option, pool_option_id = @pool_select.split(":")
+ if ( pool_option == "pool_family" )
+ if ( event.source.pool_family_id.to_s != pool_option_id )
+ match = false
+ end
+ elsif ( pool_option == "pool" )
+ if ( event.source.pool_id.to_s != pool_option_id )
+ match = false
+ end
+ end
+ end
+ if ( @owner_id != ""&& match )
+ if ( event.source.owner_id.to_s != @owner_id )
+ match = false
+ end
+ end
+ match
+ }
you could probably get rid of the whole nested 'if-else' structure by
using 'next', for example:
@events.find_all do |event|
next if @owner_id && event.source.owner_id.to_s != @owner_id
next if pool_option == "pool" && event.source.pool_id.to_s !=
pool_option_id
...
end
+
+ @events = @events.paginate(:page => params[:page], :per_page => PER_PAGE)
paginate_collection method defined in application controller can be used
(it takes care of boundaries)
+ end
+
+ def load_options
+ @source_type_options = [[t('logs.options.default_event_types'),
""], t('logs.options.deployment_event_type'),
t('logs.options.instance_event_type')]
+ @pool_options = [[t('logs.options.default_pools'), ""]]
+ PoolFamily.find(:all, :include => :pools, :order => "name",
:select => ["id", "name"]).each do |pool_family|
+ @pool_options<< [pool_family.name, "pool_family:" +
pool_family.id.to_s]
+ @pool_options += pool_family.pools.map{|x| [" -- " + x.name,
"pool:" + x.id.to_s]}
+ end
+ @provider_options = [[t('logs.options.default_providers'), ""]]
+ Provider.find(:all, :include => :provider_accounts, :order =>
"name", :select => ["id", "name"]).each do |provider|
+ @provider_options<< [provider.name, "provider:" +
provider.id.to_s]
+ @provider_options += provider.provider_accounts.map{|x| [" -- " +
x.name, "provider_account:" + x.id.to_s]}
+ end
+ @owner_options = [[t('logs.options.default_users'), ""]] +
User.find(:all, :order => "login", :select => ["id",
"login"]).map{|x| [x.login, x.id]}
+ end
+
+ def load_headers
+ @header = [
+ { :name => t('logs.index.event_time'), :sortable => false },
+ { :name => t('logs.index.deployment'), :sortable => false },
+ { :name => t('logs.index.pool'), :sortable => false },
+ { :name => t('logs.index.provider'), :sortable => false },
+ { :name => t('logs.index.owner'), :sortable => false },
+ { :name => t('logs.index.summary'), :sortable => false },
+ ]
+ end
+end
Hi Mainn,
this is not proper review, only few notes:
- permissions checks are missing - we should filter only records
accessible by current user
- it would be nice to add a test which covers this new feature, if possible
(+ some notes inline)
Anyway it's a good start, Jan