Display flash message on import/export custom report

[jzigmund]

The PR is recreated after the UI split, original PR ManageIQ/manageiq#13246

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

[mzazrivec]

This line should better read if @sb.key?(:flash_msg)

[jzigmund]

what if @sb is nil ? this approach doesn't need to test that ;-) but if you insist on that I can replace it ofc.

[mzazrivec]

if @sb is nil, then the line above that would fail as well, right?

[jzigmund]

I can imagine that someone will delete the line or change the lines order ;-)

[mzazrivec]

@sb is being set in ApplicationController before every action either to an empty hash or to whatever
the sandbox for the given controller contained before. I haven't seen this @sb.try(:[], ...) construct anywhere
in our code, I consider it a bad coding practice and I won't merge this PR looking like this.

[jzigmund]

btw. not sure what version of ruby, we support right now, but the best is using safe navigation operator .& ...

I haven't seen this @sb.try(:[], ...) construct anywhere
in our code, I consider it a bad coding practice and I won't merge this PR looking like this.

$ grep -rn \\.try app/ | wc -l
458

vs.

$ grep -rn \\.key? app/ | wc -l
259

[jzigmund]

FYI above numbers are for manageiq repo, for manageiq-ui-classic the numbers are:

$ grep -rn \\.try app/ | wc -l
330

$ grep -rn \\.key? app/ | wc -l
206

[dclarizio]

So looking at this, I think .try has its place, but perhaps not here. This is because @sb.try will not fail if @sb is nil, which in this case is bad. So, to me, I would prefer we use .key? and blow up with a nil error here, which would fail any test that runs this code and we would know that somehow we ended up here without our sandbox object in place. Anyplace we address @sb, except where we initialize it, we'd probably want it to fail if it's nil.

[miq-bot]

Checked commit jzigmund@1b92982 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. 👍

[simaishi]

Euwe backport (to manageiq repo) details:

$ git log -1
commit 9b0fe0b488ec76c74e8db0423f9341e45a364395
Author: Milan Zázrivec <[email protected]>
Date:   Mon Jan 16 13:37:51 2017 +0100

    Merge pull request #125 from jzigmund/bz1393244-flash_msg_import_export_custom_reports
    
    Display flash message on import/export custom report
    (cherry picked from commit 5384b7b77f8a913323ce45f913b5a6760fda54b1)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1414888