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.

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

Display name of a filter in Infrastructure Providers #12307

Merged

Conversation

hstastna
Copy link

fixing issue #12199

Display name of a choosen filter from My Filters which
was not displayed in Compute -> Infrastructure -> Providers
and also in many other places.

Before
name_before

After
name_after

@hstastna
Copy link
Author

@miq-bot add_label bug, ui

@mzazrivec mzazrivec self-assigned this Oct 31, 2016
@mzazrivec
Copy link
Contributor

While the fix shown would fix the problem at hand, the real problem here is that the search
filter is not being correctly extracted (and set) earlier in the code in
https://github.com/ManageIQ/manageiq/blob/master/app/controllers/application_controller/filter.rb#L533

This broke with commit a014ce4 which changed the expression stored in session from a hash
to a new object (struct). Since the new object doesn't support fetch_path(), it would always
return nil here.

@hstastna please reach out to @isimluk and try to hash out a nice solution for this problem.

@hstastna
Copy link
Author

hstastna commented Nov 1, 2016

@mzazrivec @isimluk what about
@edit[:expression].selected[:description]
instead of
@edit.fetch_path(@expkey, :selected, :description)
in https://github.com/ManageIQ/manageiq/blob/master/app/controllers/application_controller/filter.rb#L533 ?
(and def adv_search_set_text would be the same as before
https://github.com/ManageIQ/manageiq/blob/master/app/controllers/application_controller/filter.rb#L1011)
For me it works.

@hstastna hstastna force-pushed the Name_of_filter_in_Infra_providers branch from ab90089 to 1bf843a Compare November 2, 2016 13:07
@isimluk
Copy link
Member

isimluk commented Nov 4, 2016

I am sorry, I introduced this problem in a014ce4.

I didn't think of fetch_path as of something MiQ defines only for some of the base data structures. Here are my notes from the analysis

  • fetch_path is our custom method defined for Arrays and Hashes (in more_core_extensions)
  • fetch_path is lenient. That means when it finds a nil, it will returns nil. While [:x][:y][:z] will blow up 💣
  • by replacing Hash with Struct in a014ce4 I broke all the statements that use fetch_path over this Expression 😭
  • You can see find some them with git grep edit.fetch_path.*exp[kr]
  • This pr potentially fixes one of the problems (although this also inserts a 💣 prepared to detonate 🔥 once either @edit[:expression] or @edit[:expression].selected is nil).
  • We don't know if these can be nil -> (A) perhaps fetch_path was used, because there can be nil. (B) perhaps fetch_path was used, because it gave author that cool feeling. 🍸
  • I suggest that we plug all the problems (of git grep edit.fetch_path.*exp[kr]) by introducing fetch_path method on the Expression class.
  • That can be achieved by ApplicationController::Filter::Expression.send(:include, MoreCoreExtensions::Shared::Nested)

We can try:

    ApplicationController::Filter::Expression.send(:include, MoreCoreExtensions::Shared::Nested)
    x = {:a => ApplicationController::Filter::Expression.new}
    x.fetch_path(:a, :selected, :HEHE)
=> nil
    x[:a].selected[:HEHE]
NoMethodError: undefined method `[]' for nil:NilClass

I think we can fix this bug, by the one liner:

ApplicationController::Filter::Expression.send(:include, MoreCoreExtensions::Shared::Nested)

and then we can continue with extracting code manipulating expression to this class (on my todo list, but I could use some help). When we extract code manipulating Expression to the Expression class, most of the need for fetch_path method will go way. I am willing to bet real money on that.

@hstastna
Copy link
Author

hstastna commented Nov 8, 2016

For me it works if I add
ApplicationController::Filter::Expression.send(:include, MoreCoreExtensions::Shared::Nested)
to app/controllers/application_controller/filter/expression.rb
so I am fine with this solution.

@mzazrivec
Copy link
Contributor

@hstastna @isimluk I'm also OK with the solution proposed above.

@hstastna hstastna force-pushed the Name_of_filter_in_Infra_providers branch from 1bf843a to 356f3c4 Compare November 9, 2016 12:01
@hstastna
Copy link
Author

hstastna commented Nov 9, 2016

@mzazrivec I made changes and repushed so it can be merged

@hstastna hstastna force-pushed the Name_of_filter_in_Infra_providers branch from 356f3c4 to d94c227 Compare November 9, 2016 12:35
@isimluk
Copy link
Member

isimluk commented Nov 15, 2016

@hstastna the failure is not related to this pr.

You don't need to be admin to restart travis. You can restart travis, by either rebasing this pr, or simply touching that commit. i.e amending the date...

Display name of a choosen filter from My Filters which
was not displayed in Compute -> Infrastructure -> Providers
and also in many other places.
@hstastna hstastna force-pushed the Name_of_filter_in_Infra_providers branch from d94c227 to 410ed57 Compare November 15, 2016 12:59
@miq-bot
Copy link
Member

miq-bot commented Nov 15, 2016

Checked commit hstastna@410ed57 with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
1 file checked, 0 offenses detected
Everything looks good. ⭐

@himdel himdel closed this Nov 16, 2016
@himdel himdel reopened this Nov 16, 2016
@mzazrivec mzazrivec added this to the Sprint 50 Ending Dec 5, 2016 milestone Nov 16, 2016
@mzazrivec mzazrivec merged commit e65ddca into ManageIQ:master Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants