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

Use table name when generating SQL to filter tasks on Tasks screen #344

Merged
merged 7 commits into from
Feb 27, 2017
Merged

Use table name when generating SQL to filter tasks on Tasks screen #344

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3dd0a2a
use table name when generating SQL.
yrudman Feb 9, 2017
72c1381
sql generation should relay on @tabform (not on @layout) since @tabfo…
yrudman Feb 10, 2017
36af099
rspec: added table name when generating sql for UI tasks
yrudman Feb 10, 2017
92a454a
make miq-bot happy: get rid of long lines in miq_task_controller
yrudman Feb 10, 2017
ae9812b
removed not used calls to 'drop_breadcrumb' - breadcrumbs are not use…
yrudman Feb 10, 2017
5b0118a
make miq-bot happy: simplify and rename 'get_jobs' method to 'list_jobs'
yrudman Feb 10, 2017
171d44c
format sql nicer and remove not needed parameter passed to distinct
yrudman Feb 20, 2017
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
93 changes: 46 additions & 47 deletions app/controllers/miq_task_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,23 +42,20 @@ def build_jobs_tab
if role_allows?(:feature => "miq_task_all_ui")
@tabs.push(["4", _("All Other Tasks")])
end

@active_tab = @tabform.split("_").last
end

# Show job list for the current user
def jobs
build_jobs_tab
@title = _("Tasks for %{name}") % {:name => current_user.name}
@breadcrumbs = []
@lastaction = "jobs"

@edit = {}
@edit[:opts] = {}
@edit[:opts] = copy_hash(@tasks_options[@tabform]) # Backup current settings

list_jobs
if pagination_request?
get_jobs(tasks_condition(@tasks_options[@tabform]))
render :update do |page|
page << javascript_prologue
page.replace_html("gtl_div", :partial => "layouts/gtl", :locals => {:action_url => @lastaction})
Expand All @@ -69,38 +66,22 @@ def jobs
:headers => @view.headers})
page << "miqSparkle(false);" # Need to turn off sparkle in case original ajax element gets replaced
end
else # Came in from non-ajax, just get the jobs
get_jobs(tasks_condition(@tasks_options[@tabform]))
end
end

def get_jobs(conditions)
def list_jobs
Copy link
Member

Choose a reason for hiding this comment

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

something is nice about having the conditions explicitly passed into this method.

but it is duplicated in many places, so this will do

@lastaction = "jobs"
@active_tab = @tabform.split("_").last

if @tabform == "tasks_1"
@layout = "my_tasks"
@view, @pages = get_view(Job, :conditions => conditions) # Get the records (into a view) and the paginator
drop_breadcrumb(:name => _("My VM and Container Analysis Tasks"), :url => "/miq_task/index?jobs_tab=tasks")

elsif @tabform == "tasks_2"
# My UI Tasks
@layout = "my_ui_tasks"
@view, @pages = get_view(MiqTask, :conditions => conditions) # Get the records (into a view) and the paginator
drop_breadcrumb(:name => _("My Other UI Tasks"), :url => "/miq_task/index?jobs_tab=tasks")

elsif @tabform == "tasks_3" || @tabform == "alltasks_1"
@layout = "all_tasks"
@view, @pages = get_view(Job, :conditions => conditions) # Get the records (into a view) and the paginator
drop_breadcrumb(:name => _("All VM and Container Analysis Tasks"), :url => "/miq_task/index?jobs_tab=alltasks")
@user_names = Job.distinct("userid").pluck("userid").delete_if(&:blank?)

elsif @tabform == "tasks_4" || @tabform == "alltasks_2"
# All UI Tasks
@layout = "all_ui_tasks"
@view, @pages = get_view(MiqTask, :conditions => conditions) # Get the records (into a view) and the paginator
drop_breadcrumb(:name => _("All Other Tasks"), :url => "/miq_task/index?jobs_tab=alltasks")
@user_names = MiqTask.distinct("userid").pluck("userid").delete_if(&:blank?)
case @tabform
when "tasks_1" then @layout = "my_tasks"
when "tasks_2" then @layout = "my_ui_tasks"
when "tasks_3", "alltasks_1" then @layout = "all_tasks"
when "tasks_4", "alltasks_2" then @layout = "all_ui_tasks"
end

@user_names = db_class.distinct("userid").pluck("userid").delete_if(&:blank?) if @active_tab.to_i > 2
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 this has been there before, but is there perhaps a better way to do this? (select non-null user names
that have ever queued a job or a task). Esp. since miq_task.userid is a varchar and isn't indexed.

@kbrock ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mzazrivec This is good point!
Should that improvement belong to another PR ?

I was not aware that we keep history of users accessing specific screens/performing action. If we do, that table should be pretty big (vs users table) and which way is faster - need some benchmarking

Copy link
Member

@kbrock kbrock Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is getting the list of every user who has ever created a job or task?
Yea, I'd imagine the tasks table is much bigger than the users table.

All the queries I imagine depend upon having an index on userid. do we want to add that?

Copy link
Contributor Author

@yrudman yrudman Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Ohh, I've misunderstood Milan's comment, user table is not involved here. @kbrock it may make sense to add index on userid in jobs and miq_tasks tables. I am not sure since there is trade-off on creating index...

Copy link
Member

Choose a reason for hiding this comment

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

yea, there is a tradeoff on creating tasks vs searching this screen.
And since we create a bunch of tasks that are not in the ui, this may be a hit we don't want.

Do we have another mechanism to pick users?
Since this is just moving code (not creating it), I think fixing this is for another PR

trivial change, loose the parameter to distinct - it is not necessary due to the pluck

-@user_names = db_class.distinct("userid").pluck("userid").delete_if(&:blank?) if @active_tab.to_i > 2
+@user_names = db_class.distinct.pluck("userid").delete_if(&:blank?) if @active_tab.to_i > 2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you Keenan for suggestion! Done

@gtanzillo, @dclarizio result of this query supposed to populate Users filter but at this time this filter not visible. I will create separate PR to make User filter visible on appropriate tabs

@view, @pages = get_view(db_class, :conditions => tasks_condition(@tasks_options[@tabform]))
end

# Cancel a single selected job
Expand Down Expand Up @@ -288,7 +269,7 @@ def tasks_button
@edit[:opts] = copy_hash(@tasks_options[@tabform]) # Backup current settings
end

get_jobs(tasks_condition(@tasks_options[@tabform])) # Get the jobs based on the latest options
list_jobs # Get the jobs based on the latest options
@pp_choices = PPCHOICES2 # Get special pp choices for jobs/tasks lists

render :update do |page|
Expand All @@ -308,9 +289,16 @@ def tasks_button
private ############################

def db_class
case @layout
when 'my_tasks', 'all_tasks' then Job
when 'my_ui_tasks', 'all_ui_tasks' then MiqTask
case @tabform
when 'tasks_1', 'tasks_3' then Job
when 'tasks_2', 'tasks_4' then MiqTask
end
end

def db_table
case @tabform
when 'tasks_1', 'tasks_3' then ""
when 'tasks_2', 'tasks_4' then "miq_tasks."
end
end

Expand All @@ -334,6 +322,9 @@ def tasks_set_default_options
# Create a condition from the passed in options
def tasks_condition(opts, use_times = true)
cond = [[]]

cond = add_to_condition(cond, "jobs.guid IS NULL", nil) unless vm_analysis_task?
Copy link
Member

Choose a reason for hiding this comment

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

not for today (or possibly ever) but...
active record's where() already builds queries for us. seems odd we are reinventing this wheel


cond = add_to_condition(cond, *build_query_for_userid(opts))

if !opts[:ok] && !opts[:queued] && !opts[:error] && !opts[:warn] && !opts[:running]
Expand Down Expand Up @@ -362,8 +353,8 @@ def add_to_condition(cond, query, values)
end

def build_query_for_userid(opts)
return ["userid=?", session[:userid]] if %w(tasks_1 tasks_2).include?(@tabform)
return ["userid=?", opts[:user_choice]] if opts[:user_choice] && opts[:user_choice] != "all"
return ["#{db_table}userid=?", session[:userid]] if %w(tasks_1 tasks_2).include?(@tabform)
return ["#{db_table}userid=?", opts[:user_choice]] if opts[:user_choice] && opts[:user_choice] != "all"
return nil, nil
end

Expand All @@ -378,7 +369,7 @@ def build_query_for_status(opts)
end

def build_query_for_queued
["(state=? OR state=?)", %w(waiting_to_start Queued)]
["(#{db_table}state=? OR #{db_table}state=?)", %w(waiting_to_start Queued)]
end

def build_query_for_ok
Expand All @@ -394,32 +385,40 @@ def build_query_for_warn
end

def build_query_for_status_completed(status)
return ["(state=? AND status=?)", ["finished", status]] if vm_analysis_task?
["(state=? AND status=?)", ["Finished", status.capitalize]]
return ["(#{db_table}state=? AND #{db_table}status=?)", ["finished", status]] if vm_analysis_task?
Copy link
Member

@kbrock kbrock Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I like the way you formatted the sql code in build_query_for_running and other places with a common sql block up top.

Would it make sense to do that change here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea, it does make sense to format in the same way, will do. Btw, reason for formatting was to make miq-bot happy and remove long lines and since that line was not long it wasn't touched

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kbrock sql formating looks nicer now

["(#{db_table}state=? AND #{db_table}status=?)", ["Finished", status.capitalize]]
end

def build_query_for_running
return ["(state!=? AND state!=? AND state!=?)", %w(finished waiting_to_start queued)] if vm_analysis_task?
["(state!=? AND state!=? AND state!=?)", %w(Finished waiting_to_start Queued)]
sql = "(#{db_table}state!=? AND #{db_table}state!=? AND #{db_table}state!=?)"
if vm_analysis_task?
[sql, %w(finished waiting_to_start queued)]
else
[sql, %w(Finished waiting_to_start Queued)]
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand the backend part, so I need to ask: why the upper case / lower case difference?

Copy link
Contributor Author

@yrudman yrudman Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

dynamic SQL generated against 2 different tables: jobs table (for tabs tasks_1 and tasks_3, when vm_analysis_task?=true) and miq_tasks table (for tabs task_2 and tasks_4)
state column present in both tables, but values are different: in jobs table there are starting with low case and in miq_tasks with with upper case. List of values to populate state filter for jobs table: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/ui_constants.rb#L410
and for miq_tasks table: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/helpers/ui_constants.rb#L415

There is waiting_to_start value in downcase which should not be there (https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/miq_task_controller.rb#L403), and may be we should delete it, however it is there from the beginning and I am not sure if it is some legitimate exception or old bug.

end
end

def build_query_for_status_none_selected
return ["(status!=? AND status!=? AND status!=? AND state!=? AND state!=?)",
%w(ok error warn finished waiting_to_start)] if vm_analysis_task?
["(status!=? AND status!=? AND status!=? AND state!=? AND state!=?)", %w(Ok Error Warn Finished Queued)]
sql = "(#{db_table}status!=? AND #{db_table}status!=? AND #{db_table}status!=? AND "\
"#{db_table}state!=? AND #{db_table}state!=?)"
if vm_analysis_task?
[sql, %w(ok error warn finished waiting_to_start)]
else
[sql, %w(Ok Error Warn Finished Queued)]
end
end

def build_query_for_time_period(opts)
t = format_timezone(opts[:time_period].to_i != 0 ? opts[:time_period].days.ago : Time.now, Time.zone, "raw")
["updated_on>=? AND updated_on<=?", [t.beginning_of_day, t.end_of_day]]
["#{db_table}updated_on>=? AND #{db_table}updated_on<=?", [t.beginning_of_day, t.end_of_day]]
end

def build_query_for_zone(opts)
["zone=?", opts[:zone]]
["#{db_table}zone=?", opts[:zone]]
end

def build_query_for_state(opts)
["state=?", opts[:state_choice]]
["#{db_table}state=?", opts[:state_choice]]
end

def vm_analysis_task?
Expand Down
Loading