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 all 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
104 changes: 56 additions & 48 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.pluck("userid").delete_if(&:blank?) if @active_tab.to_i > 2
@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,9 +353,14 @@ 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 nil, nil
sql = "#{db_table}userid=?"
if %w(tasks_1 tasks_2).include?(@tabform)
[sql, session[:userid]]
elsif opts[:user_choice] && opts[:user_choice] != "all"
[sql, opts[:user_choice]]
else
[nil, nil]
end
end

def build_query_for_status(opts)
Expand All @@ -378,7 +374,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 +390,44 @@ 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]]
sql = "(#{db_table}state=? AND #{db_table}status=?)"
if vm_analysis_task?
[sql, ["finished", status]]
else
[sql, ["Finished", status.capitalize]]
end
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