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.

Sign up for GitHub

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

Jump to bottom

Display Cancel after applying changes in non-explorer Compare screen #6542

Merged
merged 1 commit into from Jan 6, 2020
Merged

Display Cancel after applying changes in non-explorer Compare screen #6542

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/application_controller/compare.rb
View file
Open in desktop
Expand Up @@ -420,7 +420,7 @@ def drift_same
# AJAX driven routine to check for changes in ANY field on the form
def sections_field_changed
@keep_compare = true
@explorer = %w[VMs Templates].include?(session[:db_title])
@explorer = %w[VMs Templates].include?(session[:db_title]) && @display.nil?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes the issue, but I think the condition is wrong anyway.

What we usually do to determine explorer:

  • if BASE_MODEL_EXPLORER_CLASSES.include?(cls)
  • if request.xml_http_request?
  • if @edit[:explorer]

Can any of those work instead?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old logic of "VM" or "Template" is in title => explorer, is wrong, so this is fixing a wrong thing by adding stuff instead of replacing to logic to make sense.

Perhaps the place which renders the template should be setting @edit[:explorer] and then we can get it from session.

Copy link
Contributor Author

@hstastna hstastna Jan 6, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tried the second two of them, before I made the change in this PR, of course, but it was not good enough (it fixed the issue for non explorer screens but stopped working for explorer ones).

if @edit[:explorer] did not work, we need @edit to be set, for this case - and this is not always done even when we are in an explorer screen. Even when I've set @edit[:explorer] to true or false at the beginning of comparing items (after user clicked on Compare action), later right after calling sections_field_changed @edit became nil, the information 'got lost'.

And request.xml_http_request? returned something even when we weren't in explorer screen (this happens to me sometimes also for other places and controllers).

Copy link
Contributor

@himdel himdel Jan 6, 2020 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, merging as is then, sounds like a future refactoring :).

(created #6580 )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also checking some classes was not enough to decide if we are in a explorer screen or not, because of nested lists which are mostly in non-explorer screens.

if params[:check] == "drift"
section_checked(:drift)
elsif params[:check] == "compare_miq"
Expand Down
28 changes: 28 additions & 0 deletions spec/controllers/application_controller/compare_spec.rb
View file
Open in desktop
Expand Up @@ -166,6 +166,34 @@ def init_fields_and_results(clusters)
controller.send(:sections_field_changed)
expect(session[:selected_sections]).to eq(["_model_", "hardware"])
end

context 'changes in Comparison Sections' do
before do
allow(controller).to receive(:render)
allow(controller).to receive(:session).and_return(:db_title => 'VMs')
controller.instance_variable_set(:@display, 'instances')
controller.params = {:check => 'true'}
end

it 'sets @explorer to false' do
controller.send(:sections_field_changed)
expect(controller.instance_variable_get(:@explorer)).to be(false)
end

it 'calls set_checked_sections according to the params' do
expect(controller).to receive(:set_checked_sections)
controller.send(:sections_field_changed)
end

context 'explorer screen' do
before { controller.instance_variable_set(:@display, nil) }

it 'sets @explorer to true' do
controller.send(:sections_field_changed)
expect(controller.instance_variable_get(:@explorer)).to be(true)
end
end
end
end

context 'drifts' do
Expand Down