Skip to content

Refactoring provision method to use new rbac call #3890

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

Closed
wants to merge 1 commit into from

Conversation

Cboosa
Copy link

@Cboosa Cboosa commented May 3, 2018

Replaced provision method, added test.

Parent issue: #1134

@miq-bot add_label rbac
@miq-bot add_label refactoring

@lpichler @romanblanco

Please review @terezapliskova :].

@terezapliskova
Copy link

OK

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

Please add expected behaviour to the test.

You can for example test that the method process_hosts(hosts, method, display_name) is called with correct arguments.

Also the it is not very descriptive 😉

end

it "tests method change" do
controller.send(:host_button_operation, 'refresh_ems', _('Refresh'))
Copy link
Member

Choose a reason for hiding this comment

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

@Cboosa The expected behaviour is missing here.

@Cboosa Cboosa force-pushed the misc_pref_method_change branch from 92b8385 to ac15381 Compare May 10, 2018 09:42
end

it "tests that find_records_with_rbac is called and does not fail" do
expect(controller).to receive(:find_records_with_rbac).with(Host, Host.all).and_return(Host.all)
Copy link
Member

Choose a reason for hiding this comment

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

.with(Host, Host.all.ids)

login_as admin_user
allow(User).to receive(:current_user).and_return(admin_user)
allow(controller).to receive(:process_hosts).with([host.id], 'refresh_ems', 'Refresh')
controller.instance_variable_set(:@_params, {})
Copy link
Member

Choose a reason for hiding this comment

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

You can set the instance variable @params correctly
controller.instance_variable_set(:@_params, :id=> Host.all.id)
and remove
allow(controller).to receive(:find_checked_items).and_return(Host.all)

@Cboosa Cboosa force-pushed the misc_pref_method_change branch from ac15381 to f474aa7 Compare May 10, 2018 09:55
@miq-bot
Copy link
Member

miq-bot commented May 10, 2018

Checked commit Cboosa@f474aa7 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 👍

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@@ -222,7 +222,7 @@ def host_button_operation(method, display_name)

# Either a list or coming from a different controller (eg from ems screen, go to its hosts)
if @lastaction == "show_list" || @layout != "host"
hosts = find_checked_ids_with_rbac(Host)
hosts = find_records_with_rbac(Host, checked_or_params).ids
if hosts.empty?
Copy link
Member

Choose a reason for hiding this comment

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

The test for hosts presence is not necessary

@martinpovolny
Copy link
Member

@romanblanco: can we merge this? Or will you update it?

@romanblanco
Copy link
Member

@martinpovolny updated in #4241

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants