-
Notifications
You must be signed in to change notification settings - Fork 356
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
Fix displaying services after applying a filter (2) #3654
Fix displaying services after applying a filter (2) #3654
Conversation
@miq-bot add_label bug, services, expressions/filters |
@dclarizio @h-kataria @lgalis Now it should work right ;) |
@hstastna this is not the correct solution because, this is clearing everything that's stored in session[:adv_search] such as selected filters for other screens/controllers. If you go to list of Hosts and select a filter there, then leave and come back to Host list view, it remembers which filter user had selected previously, but with this change all the previously selected filters for other screens during the session are getting lost. I think we need to look at other screens to figure out how those are handling the same scenario. Steps to recreate:
|
18c2eca
to
e36813b
Compare
@h-kataria I made a change and now it should work, your scenario is not reproducible anymore. Anyway, using |
@miq-bot add_label gaprindashvili/yes |
@h-kataria Could you please look at my change in this PR? Thanks! ⭐ |
fixing ManageIQ#3261 Fix displaying all services after applying a filter and clicking on an other node in tree, in Services > My Services page.
e36813b
to
d6db487
Compare
@h-kataria, @skateman I've added some spec test for the fix. Feel free to comment, discuss. Thanks! ✨ |
@@ -103,7 +103,7 @@ | |||
end | |||
end | |||
|
|||
context "#show" do | |||
describe "#show" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
@@ -182,7 +182,7 @@ | |||
expect(response).to redirect_to(:action => 'explorer', :id => "s-#{service.id}") | |||
end | |||
|
|||
context "#button" do | |||
describe "#button" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
@@ -211,7 +211,7 @@ | |||
end | |||
end | |||
|
|||
context "#sanitize_output" do | |||
describe "#sanitize_output" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😍
end | ||
end | ||
|
||
def reset_session_values |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't define methods in tests if it's not absolutely necessary. Having a duplication in this case is totally fine IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see methods in test in many places where I consider it as less necessary than here :D ;) But, yeah, I will change this! ;)
end | ||
|
||
def reset_session_values | ||
expect(controller.session[:edit]).to be(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be_nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👼
|
||
def reset_session_values | ||
expect(controller.session[:edit]).to be(nil) | ||
expect(controller.session[:adv_search]["Service"]).to be(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
be_nil
context 'clicking on Active/Retired Services node in the tree, after applying a filter' do | ||
describe '#get_node_info' do | ||
before do | ||
allow(controller).to receive(:session).and_return(:adv_search => {"Service" => {:expression => {}}}, :edit => {:expression => {}}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make this a little bit more readable? Maybe expand it into multiple lines? Or just my screen is too small? 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's problem of github.. I sometimes have the same problems. Or I can use let
;) Anyway:
allow(controller).to receive(:session).and_return(:adv_search => {"Service" => {:expression => {}}}, :edit => {:expression => {}})
Add spec test which checks resetting session values to same values as first time in.
d6db487
to
4f5ee64
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checked commits hstastna/manageiq-ui-classic@1df3f00~...4f5ee64 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0 |
@h-kataria I think this PR is ready :) |
fixing issue: #3261
more info: #3395
Fix displaying services after applying a filter and clicking on an other node in tree (Active/Retired Services folder), in Services > My Services page so that Active/Retired services without filtering are displayed.
Details of a problem:
There is an issue in My Services page, after applying/clicking on some filter from Global/My Filters in accordion: if we click on an other node in tree, for example Active Services (or Retired Services), services are not displayed properly: there is inconsistency between displayed services and the title of the page, filter is applied on Active or Retired services, not on all of the services.
Steps to reproduce:
and you will see that filtering is applied on all of the services (this works right)
=> you will see active (or retired) services, filtered by previously selected filter, without any " - Filtered by name_of_filter"
Step 2:
Before: (step 3)
After: (the title is consistent with services displayed on the page)
Notes: When fixing the problem, I also had to deal with normal Search - to keep it working without any changes which lead to adding a necessary condition to resetting some values of
session
inget_node_info
.