Cleanup dhtmlxgrid partial.

[martinpovolny]

So we have this XML that we have inside a piece of javascript that is inside a script tag generated by a partial that we render to a string and assign to a variable that we pass to a dom-manipulation function $('#main_div').html(...).

It is crazy and has to be removed. But not now. Now we need to fix it...

[martinpovolny]

@romanblanco : review, please

[Fryguy]

So we have this XML that we have inside a piece of javascript that is inside a script tag generated by a partial that we render to a string and assign to a variable that we pass to a dom-manipulation function

Best description ever... cc @blomquisg 😆

[skateman]

[martinpovolny]

Hakiri does not like me calling j stuff.html_safe but I have to call both of the functions to get it actually right. So trying to swap the calls to make Hakiri happy.

[martinpovolny]

@matthewd, @kbrock : any hint how to make Hakiri happy here? Or do we add an exception for this one, @chessbyte?

[matthewd]

@martinpovolny I assumed the temp_variable = raw @original_thing pattern was working around that?

But personally, I vote we go with the "correct" #{raw j(@foo)}, and mark FPs on Hakiri as necessary. It's a good idea to avoid questionable coding patterns if it allows us to avoid a false positive, but if the checker is getting upset with the only correct spelling, we really have no choice but to ignore it. I think this is the thing @kbrock fixed on upstream brakeman recently.

[martinpovolny]

@matthewd : tried that pattern right now (see the current version of this PR) and it seems that it shows the same issue :-(

@matthewd : if you agree, I would remove the 2nd patch and ask for this being merged + exception added.

[dclarizio]

@martinpovolny 1 remaining Hakiri warning and a failed test in vmdb.

[martinpovolny]

@dclarizio : Hakiri warning is supposed to stay there until we mark it as false positive. Rebased and restarting travis, no failures locally and the stacktrace does not match the code. Travis went crazy?

[matthewd]

I can't explain the backtrace, or any inconsistency, but https://github.com/ManageIQ/manageiq/pull/4808/files#diff-b889b1cf33fb6736b4b3aebbb89f1196R20 sounds like it could cause that error.

[martinpovolny]

@matthewd : thx, looking into it

[dclarizio]

@martinpovolny I'm pretty sure this would be related to the changes, not sure why it passes locally.

Failures:
  1) layouts/_dhtmlxgrid.html.haml when showtype is 'performance' renders
     Failure/Error: render
     ActionView::Template::Error:
       undefined method `gsub' for 1:Fixnum
     # ./app/views/layouts/_dhtmlxgrid.html.haml:15:in `_app_views_layouts__dhtmlxgrid_html_haml__1518542955181751166_605109580'
     # ./spec/views/layouts/dhtmlxgrid.html.haml_spec.rb:9:in `block (3 levels) in <top (required)>'

[miq-bot]

Checked commit martinpovolny@be0ca66 with ruby 1.9.3, rubocop 0.34.2, and haml-lint 0.13.0
0 files checked, 0 offenses detected
Everything looks good. 👍