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

yrudman · 2017-02-10T00:09:43Z

This PR depends and should be merged together with ManageIQ/manageiq#13452

There would be task created for each job after changes in ManageIQ/manageiq#13452 and those new tasks associated with jobs should be filtered out on UI Tasks screen.
Since there are columns in jobs and miq_tasks tables with same name (like state, status,...) we need to use table name when generating SQL on joined tables.

@miq-bot add-label enhancement

local AFTER UI and model changes:

\cc @Fryguy @gtanzillo @dclarizio

…rm always correctly defined. Only task queries requier adding table name when accessing filed (after creating association between Job and MiqTask sql is joining 2 tables with the same column names), Job queries still the same.

…d on Task admin screen

gtanzillo · 2017-02-10T23:04:54Z

@yrudman Is there a "before" screen shot that goes with the "after" one? IIRC, there should not be any visible UI changes with this PR. I should just ensure the UI still works with you backend changes.

yrudman · 2017-02-11T02:44:49Z

@gtanzillo
BEFORE: Screenshot below is on master branches from both repo but DB is after migration - each job has corresponding task. You can see that after filtering 3 records with name starting with "Scan" the rest would be the same as in "after" screenshot:

Deleting 3 records linked to jobs from miq_tasks table will produce the same view as in "after"

yrudman · 2017-02-16T13:39:15Z

@mzazrivec could you review

mzazrivec · 2017-02-17T10:36:41Z

app/controllers/miq_task_controller.rb

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


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 ?

@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

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?

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...

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

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

mzazrivec · 2017-02-17T10:43:30Z

app/controllers/miq_task_controller.rb

+    if vm_analysis_task?
+      [sql, %w(finished waiting_to_start queued)]
+    else
+      [sql, %w(Finished waiting_to_start Queued)]


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

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.

gtanzillo

👍 LGTM

kbrock

This is pruning the code down.

Do we have general style suggestions on using @variables or munging sql?

good call Milan on the distinct users column. That will bite us.

kbrock · 2017-02-17T19:25:21Z

spec/controllers/miq_task_controller_spec.rb

+                "(miq_tasks.state!=? AND miq_tasks.state!=? AND miq_tasks.state!=?)) AND "\
+                "miq_tasks.updated_on>=? AND "\
+                "miq_tasks.updated_on<=? AND "\
+                "miq_tasks.state=?"


I know you didn't write this.

but miq_tasks.state is confusing here (in these specs in general)

kbrock · 2017-02-17T19:27:51Z

app/controllers/miq_task_controller.rb

    end
  end

-  def get_jobs(conditions)
+  def list_jobs


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

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

kbrock · 2017-02-17T19:29:51Z

app/controllers/miq_task_controller.rb

@@ -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?


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

kbrock · 2017-02-17T19:32:20Z

app/controllers/miq_task_controller.rb

@@ -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?


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?

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

@kbrock sql formating looks nicer now

kbrock · 2017-02-17T19:33:48Z

spec/controllers/miq_task_controller_spec.rb

@@ -851,6 +854,17 @@
      end
    end

+    describe "#list_jobs" do
+      it 'sets the active tab' do
+        controller.instance_variable_set(:@tabform, "ui_2")


yea, this is a side effect of all of those @ variables vs passing in the variables into list_jobs

I was trying to minimize changes in this PR and did not remove global variables as aproach to pass parameter into methods.

kbrock · 2017-02-17T19:37:00Z

app/controllers/miq_task_controller.rb

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


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?

miq-bot · 2017-02-20T15:48:10Z

Checked commits yrudman/manageiq-ui-classic@3dd0a2a~...171d44c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 0 offenses detected
Everything looks good. 👍

kbrock

This makes some nice changes.
Also nicely cleans it up in prep for merging those 2 models

👍

yrudman · 2017-02-27T19:42:20Z

@miq-bot remove-label pending core

yrudman · 2017-02-27T20:14:59Z

@mzazrivec @dclarizio back-end PR has been merged - ManageIQ/manageiq#13452

use table name when generating SQL.

3dd0a2a

miq-bot added the enhancement label Feb 10, 2017

mzazrivec added the pending core label Feb 10, 2017

yrudman changed the title ~~Use table name when generating SQL to filter jobs or tasks on Tasks screen~~ Use table name when generating SQL to filter tasks on Tasks screen Feb 10, 2017

yrudman force-pushed the keep-same-ui-after-making-job-belongs-to-task branch from 9110973 to deda7d7 Compare February 10, 2017 17:36

yrudman mentioned this pull request Feb 10, 2017

Link jobs and miq_tasks tables ManageIQ/manageiq#13452

Merged

rspec: added table name when generating sql for UI tasks

36af099

yrudman force-pushed the keep-same-ui-after-making-job-belongs-to-task branch from deda7d7 to 36af099 Compare February 10, 2017 17:44

yrudman added 2 commits February 10, 2017 14:05

make miq-bot happy: get rid of long lines in miq_task_controller

92a454a

removed not used calls to 'drop_breadcrumb' - breadcrumbs are not use…

ae9812b

…d on Task admin screen

yrudman force-pushed the keep-same-ui-after-making-job-belongs-to-task branch from 7e305ae to ee1f3a9 Compare February 10, 2017 21:21

make miq-bot happy: simplify and rename 'get_jobs' method to 'list_jobs'

5b0118a

yrudman force-pushed the keep-same-ui-after-making-job-belongs-to-task branch from ee1f3a9 to 5b0118a Compare February 10, 2017 21:27

dclarizio requested a review from mzazrivec February 10, 2017 22:27

mzazrivec reviewed Feb 17, 2017

View reviewed changes

gtanzillo approved these changes Feb 17, 2017

View reviewed changes

kbrock reviewed Feb 17, 2017

View reviewed changes

format sql nicer and remove not needed parameter passed to distinct

171d44c

yrudman closed this Feb 20, 2017

yrudman reopened this Feb 20, 2017

kbrock approved these changes Feb 23, 2017

View reviewed changes

miq-bot removed the pending core label Feb 27, 2017

dclarizio self-assigned this Feb 27, 2017

dclarizio added tasks euwe/no labels Feb 27, 2017

dclarizio merged commit 5a6599d into ManageIQ:master Feb 27, 2017

dclarizio added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 27, 2017

yrudman deleted the keep-same-ui-after-making-job-belongs-to-task branch February 28, 2017 12:56

yrudman mentioned this pull request Mar 2, 2017

Use table name when generating SQL to filter jobs on ... Container Analysis Tasks screens #551

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

yrudman commented Feb 10, 2017

gtanzillo commented Feb 10, 2017

yrudman commented Feb 11, 2017 •

edited

Loading

yrudman commented Feb 16, 2017

mzazrivec Feb 17, 2017

yrudman Feb 17, 2017

kbrock Feb 17, 2017 •

edited

Loading

yrudman Feb 17, 2017 •

edited

Loading

kbrock Feb 20, 2017

yrudman Feb 20, 2017

mzazrivec Feb 17, 2017

yrudman Feb 17, 2017 •

edited

Loading

gtanzillo left a comment

kbrock left a comment

kbrock Feb 17, 2017

kbrock Feb 17, 2017

kbrock Feb 17, 2017

kbrock Feb 17, 2017 •

edited

Loading

yrudman Feb 17, 2017

yrudman Feb 20, 2017

kbrock Feb 17, 2017

yrudman Feb 17, 2017 •

edited

Loading

kbrock Feb 17, 2017 •

edited

Loading

miq-bot commented Feb 20, 2017

kbrock left a comment

yrudman commented Feb 27, 2017 •

edited

Loading

yrudman commented Feb 27, 2017

		end

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

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

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

Conversation

yrudman commented Feb 10, 2017

gtanzillo commented Feb 10, 2017

yrudman commented Feb 11, 2017 • edited Loading

yrudman commented Feb 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

yrudman Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrudman Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

gtanzillo left a comment

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kbrock Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yrudman Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

kbrock Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

miq-bot commented Feb 20, 2017

kbrock left a comment

Choose a reason for hiding this comment

yrudman commented Feb 27, 2017 • edited Loading

yrudman commented Feb 27, 2017

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

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

yrudman commented Feb 11, 2017 •

edited

Loading

kbrock Feb 17, 2017 •

edited

Loading

yrudman Feb 17, 2017 •

edited

Loading

yrudman Feb 17, 2017 •

edited

Loading

kbrock Feb 17, 2017 •

edited

Loading

yrudman Feb 17, 2017 •

edited

Loading

kbrock Feb 17, 2017 •

edited

Loading

yrudman commented Feb 27, 2017 •

edited

Loading