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

Replace conditions with named scopes #2638

Merged
merged 2 commits into from
Jan 8, 2018

Conversation

alexander-demicev
Copy link

@alexander-demicev alexander-demicev commented Nov 6, 2017

Replace conditions with named scopes.

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

@alexander-demicev
Copy link
Author

@miq-bot miq-bot added the wip label Nov 6, 2017
@alexander-demicev alexander-demicev changed the title [WIP] Replace conditions with named scopes Replace conditions with named scopes Nov 25, 2017
@miq-bot miq-bot removed the wip label Nov 25, 2017
@alexander-demicev
Copy link
Author

@martinpovolny Hi, added some tests, can you make a review?

@miq-bot
Copy link
Member

miq-bot commented Nov 27, 2017

Some comments on commits alexander-demicev/manageiq-ui-classic@5bb45cb~...887d28e

spec/controllers/host_controller_spec.rb

  • ⚠️ - 217 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 233 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 249 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 265 - Detected expect_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Nov 27, 2017

Checked commits alexander-demicev/manageiq-ui-classic@5bb45cb~...887d28e with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. ⭐

Copy link
Member

@martinpovolny martinpovolny left a comment

Choose a reason for hiding this comment

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

The changes look very good.

Thanks for the tests.

Can you get someone from your group test this in the UI? @aufi? @mansam?

Thx!

@martinpovolny
Copy link
Member

Need to wait for the provider PR anyway.

@alexander-demicev
Copy link
Author

@martinpovolny Related PR's are merged :)

@martinpovolny
Copy link
Member

I'd love to merge this. But I don't have the data to properly test this now.

Can you get someone from your group test this in the UI? @aufi? @mansam?

Ping @aufi, @mansam, @tzumainn : can you, please, confirm this works as expected?

@mansam
Copy link
Contributor

mansam commented Dec 18, 2017

I will review this as soon as possible.

@martinpovolny
Copy link
Member

ping @mansam?

@mansam
Copy link
Contributor

mansam commented Jan 8, 2018

@martinpovolny Looks like it's working as intended to me.

@martinpovolny martinpovolny merged commit 1390980 into ManageIQ:master Jan 8, 2018
@martinpovolny martinpovolny self-assigned this Jan 8, 2018
@martinpovolny martinpovolny added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 8, 2018
@alexander-demicev alexander-demicev deleted the delete-condtitions branch March 7, 2018 14:14
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