Search fixes for VmInfra #2842

martinpovolny · 2017-11-27T19:06:15Z

Fixing:
https://bugzilla.redhat.com/show_bug.cgi?id=1517392

I believe that earlier the @explorer branch worked for vm_infra. #2556 the added @layout test but that works just in some cases (as shown in https://bugzilla.redhat.com/show_bug.cgi?id=1517392).
So I am removing that.

Also the test for 'miq_policy' == @layout seems duplicate to testing @explorer because under Control/Explorer/Policies accordion where this holds true @explorer is also set.

But the root clause here is the improperly set @explorer variable -- added a .to_s to the check. I am not sure why params[:explorer] is a bool so that is the safest.

I am removing a strangeness with unless @display being only tested only if ::Settings.server.case_sensitive_name_search that seems to be a bug.

I am moving the session[:menu_click] condition to the front because if chart pop-up menu click we want the filtering disabled and no matter with branch of the condition passes.

I need more eyes and I need a list of screens where search is expected to work.

I can still break the search if I keep the search on and dive deep enough following the relations. But that is not to be fixed here. That might need changes not suitable for Gaprindashvili at this point.

Added specs for VmInfra search and Host search. More can be added following the pattern but I need to move forward.

Ping @mzazrivec, @himdel, @hstastna . Please, review.

If we would be able to make a list of places where search should work, it woud be nice.

Ping @dclarizio, @h-kataria : please, chime in if you have inputs on this.

TODO

~~WIP: fixing the search breaks nested list under VM such as VM --> {processes,files}. Working on that by passing @display throught the GTL.~~

Note:

When I click [x] in Hosts I get:
Request URL:http://localhost:3000/host/adv_search_text_clear?in_explorer=

adv_search_text_clear is the newly added method here #1893

and there's no else branch for non-explorers there. So this could never work.

I am not fixing that here.

himdel · 2017-12-01T14:17:56Z

Code looks good, but I still can't figure out when params[:explorer].to_s == "true" would be different from the old version - where does this come from? Can params[:anything] ever be a bool?

(Also not quite seeing a relation between that line, and the removal of %w(miq_policy vm_infra).include?(@layout)), but will test that it's there :).)

(Yet to test..)

hstastna · 2017-12-01T14:43:27Z

I am not sure about the other things but I agree with removal of
%w(miq_policy vm_infra).include?(@layout)
In the past, when I was fixing Search in vm infra, I was afraid to break things so I fixed that the simplest way and didn't know about more Search places which need fixing. Now it looks like this was not enough because more screens have problems with Search so it it obvious that it was not the best fix. I like the way you deal with it here. It looks good to me. I just want to test this on some more screens where I have found problems with Search, screens where it does not work at all (not just in some specific scenario).

himdel · 2017-12-01T14:58:22Z

spec/controllers/host_controller_spec.rb

+        )
+        results = assert_report_data_response
+        expect(results['data']['rows'].length).to eq(1)
+        expect(results['data']['rows'][0]['long_id']).to eq(h1.id)


This will need to eq(h1.id.to_s) - #2902

(depending on which one is merged first :))

himdel · 2017-12-01T14:58:42Z

spec/controllers/vm_infra_controller_spec.rb

+        )
+        results = assert_report_data_response
+        expect(results['data']['rows'].length).to eq(1)
+        expect(results['data']['rows'][0]['long_id']).to eq(vm1.id)


This will need to eq(h1.id.to_s) - #2902

(depending on which one is merged first :))

himdel · 2017-12-01T15:03:48Z

I am not sure about the other things but I agree with removal of
%w(miq_policy vm_infra).include?(@layout)

Perfect, do you know why it's no longer needed then? :)

martinpovolny · 2017-12-01T16:47:49Z

params[:explorer] was a bool
therefore @explorer was not true
therefore that part of the condition did not pass and
therefore @hstastna 's fix partly fixed the problem

hstastna · 2017-12-01T16:55:59Z

yes, I haven't seen it before (when I was fixing it some time ago). Simply, true is not the same as "true".

himdel · 2017-12-01T17:00:00Z

@martinpovolny I'm trying to find out how it happens that params[:explorer] is bool.

AFAIK params is rails magic and should always be just strings from the query string, shouldn't it? (Or strings from POST data.)
(Maybe if we had some POST requests which post json data, but I'm not aware of any such.)

therefore @hstastna 's fix partly fixed the problem

So.. there's some context in some other PR?

skateman · 2017-12-01T17:06:53Z

@himdel if you set the content type to JSON in a POST request and send it with the following body:

{
  "explorer": true,
  "exploder": "true"
}

Then the params will ✨ ✨ look like this:

params = { 'explorer' => true, 'exploder' => 'true' }

himdel · 2017-12-01T17:08:58Z

Oh ok, so I just didn't know we ever do a JSON POST 👍.

Asuming this comes from the GTL component?

Great, now only to test it :)

martinpovolny · 2017-12-01T18:41:59Z

So.. there's some context in some other PR?

f0861de

Asuming this comes from the GTL component?

yes

martinpovolny · 2017-12-01T18:45:58Z

@skateman, @hstastna, @himdel : if you happen to know where else in the application we have search, please, make a note into this PR.

I'd like to have test coverage for all the searches and actually have a list of the places for furute refactorings. Thx!

hstastna · 2017-12-01T19:48:18Z

@martinpovolny I have tested that this PR fixes Search also in Configuration Management Configured Systems page. It also works together with my PR #2424. I will continue with testing on some more pages.
This PR also fixes https://bugzilla.redhat.com/show_bug.cgi?id=1509318 . Should I mark it as a dup of yours or not?

martinpovolny · 2017-12-01T20:09:06Z

@hstastna: if this PR fixes another BZ then please, mark it as DUP. Thank you!

skateman · 2017-12-01T21:24:07Z

@martinpovolny as @karelhala mentioned above, this also fixes an issue with catalog grid view change, but it's not quite a duplicate.

martinpovolny · 2017-12-04T08:09:23Z

We should merge this one ASAP.

For one, it's fixing a number of BZs and
for two it changes a frequent codepath and If we merge it right before the build we will have no chance to catch any possible regressions.

miq-bot · 2017-12-04T08:13:50Z

Some comments on commits martinpovolny/manageiq-ui-classic@60a16a1~...d9835ed

spec/controllers/host_controller_spec.rb

⚠️ - 360 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

spec/controllers/vm_infra_controller_spec.rb

⚠️ - 545 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

miq-bot · 2017-12-04T08:14:44Z

Checked commits martinpovolny/manageiq-ui-classic@60a16a1~...d9835ed with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
5 files checked, 1 offense detected

app/controllers/application_controller.rb

❗ - Line 1471, Col 53 - Style/CommentIndentation - Incorrect indentation detected (column 52 instead of 6).

himdel · 2017-12-04T14:59:56Z

Note:

#2842 (comment)

is also happening in Control > Explorer

himdel · 2017-12-04T15:00:35Z

Code looks good, not seeing any breakage around miq policy either.. merged :).

Search fixes for VmInfra (cherry picked from commit e4e87c6) https://bugzilla.redhat.com/show_bug.cgi?id=1520667 https://bugzilla.redhat.com/show_bug.cgi?id=1520553

simaishi · 2017-12-04T22:05:25Z

Gaprindashvili backport details:

$ git log -1
commit ed8d48c7071c4538ea275abe30272f448f1a7eee
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Dec 4 15:00:11 2017 +0000

    Merge pull request #2842 from martinpovolny/search_fixes
    
    Search fixes for VmInfra
    (cherry picked from commit e4e87c6bb321642acaa1f475bddd17fd3361cad3)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1520667
    https://bugzilla.redhat.com/show_bug.cgi?id=1520553

martinpovolny added bug gaprindashvili/yes gtls labels Nov 27, 2017

martinpovolny force-pushed the search_fixes branch from 8e02f62 to a41ad70 Compare November 27, 2017 19:07

martinpovolny added the wip label Nov 27, 2017

martinpovolny changed the title ~~[WI] Search fixes~~ [WIP] Search fixes Nov 27, 2017

martinpovolny self-assigned this Nov 28, 2017

martinpovolny force-pushed the search_fixes branch 3 times, most recently from 770a5a1 to 384270f Compare November 28, 2017 15:10

martinpovolny changed the title ~~[WIP] Search fixes~~ Search fixes for VmInfra Nov 28, 2017

martinpovolny force-pushed the search_fixes branch from 384270f to b173a13 Compare November 28, 2017 15:56

martinpovolny removed the wip label Nov 28, 2017

martinpovolny force-pushed the search_fixes branch from b173a13 to 3a45115 Compare November 28, 2017 20:21

martinpovolny changed the title ~~Search fixes for VmInfra~~ [WIP] Search fixes for VmInfra Nov 29, 2017

martinpovolny added the wip label Nov 29, 2017

martinpovolny added 4 commits November 29, 2017 16:26

Fix search under VmInfra.

60a16a1

Fixing the root cause -- the @explorer not being set properly. Then cleaning up the conditions (some seem to be workarounds around @explorer not being set). Also fixing a strange difference between ::Settings.server.case_sensitive_name_search true/false

Host: add spec for simple search.

03938ce

VmInfra: add spec for simple search.

9485f71

martinpovolny force-pushed the search_fixes branch from 3a45115 to e217a0e Compare November 29, 2017 15:26

martinpovolny changed the title ~~[WIP] Search fixes for VmInfra~~ Search fixes for VmInfra Nov 29, 2017

miq-bot removed the wip label Nov 29, 2017

himdel reviewed Dec 1, 2017

View reviewed changes

himdel mentioned this pull request Dec 1, 2017

ReportData - change long_id to string #2902

Merged

GTL: long_id needs to be a string due to Javascript limitation.

d9835ed

himdel merged commit e4e87c6 into ManageIQ:master Dec 4, 2017

himdel added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 4, 2017

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Dec 4, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Search fixes for VmInfra #2842

Search fixes for VmInfra #2842

martinpovolny commented Nov 27, 2017 •

edited

Loading

karelhala commented Nov 29, 2017

martinpovolny commented Nov 29, 2017 •

edited

Loading

himdel commented Dec 1, 2017 •

edited

Loading

hstastna commented Dec 1, 2017 •

edited

Loading

himdel Dec 1, 2017 •

edited

Loading

himdel Dec 1, 2017

himdel commented Dec 1, 2017

martinpovolny commented Dec 1, 2017 •

edited

Loading

hstastna commented Dec 1, 2017

himdel commented Dec 1, 2017 •

edited

Loading

skateman commented Dec 1, 2017 •

edited

Loading

himdel commented Dec 1, 2017 •

edited

Loading

martinpovolny commented Dec 1, 2017

martinpovolny commented Dec 1, 2017

hstastna commented Dec 1, 2017

martinpovolny commented Dec 1, 2017

skateman commented Dec 1, 2017

martinpovolny commented Dec 4, 2017 •

edited

Loading

miq-bot commented Dec 4, 2017

miq-bot commented Dec 4, 2017

himdel commented Dec 4, 2017 •

edited

Loading

himdel commented Dec 4, 2017

simaishi commented Dec 4, 2017

Search fixes for VmInfra #2842

Search fixes for VmInfra #2842

Conversation

martinpovolny commented Nov 27, 2017 • edited Loading

TODO

Related problem

karelhala commented Nov 29, 2017

martinpovolny commented Nov 29, 2017 • edited Loading

Note:

himdel commented Dec 1, 2017 • edited Loading

hstastna commented Dec 1, 2017 • edited Loading

himdel Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

himdel Dec 1, 2017

Choose a reason for hiding this comment

himdel commented Dec 1, 2017

martinpovolny commented Dec 1, 2017 • edited Loading

hstastna commented Dec 1, 2017

himdel commented Dec 1, 2017 • edited Loading

skateman commented Dec 1, 2017 • edited Loading

himdel commented Dec 1, 2017 • edited Loading

martinpovolny commented Dec 1, 2017

martinpovolny commented Dec 1, 2017

hstastna commented Dec 1, 2017

martinpovolny commented Dec 1, 2017

skateman commented Dec 1, 2017

martinpovolny commented Dec 4, 2017 • edited Loading

miq-bot commented Dec 4, 2017

miq-bot commented Dec 4, 2017

himdel commented Dec 4, 2017 • edited Loading

himdel commented Dec 4, 2017

simaishi commented Dec 4, 2017

martinpovolny commented Nov 27, 2017 •

edited

Loading

martinpovolny commented Nov 29, 2017 •

edited

Loading

himdel commented Dec 1, 2017 •

edited

Loading

hstastna commented Dec 1, 2017 •

edited

Loading

himdel Dec 1, 2017 •

edited

Loading

martinpovolny commented Dec 1, 2017 •

edited

Loading

himdel commented Dec 1, 2017 •

edited

Loading

skateman commented Dec 1, 2017 •

edited

Loading

himdel commented Dec 1, 2017 •

edited

Loading

martinpovolny commented Dec 4, 2017 •

edited

Loading

himdel commented Dec 4, 2017 •

edited

Loading