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

Delete unnecessary X.empty? check after calling find_record_with_rbac #3186

Merged

Conversation

PanSpagetka
Copy link
Contributor

@PanSpagetka PanSpagetka commented Jan 8, 2018

Delete unnecessary X.empty? check after calling find_record_with_rbac. After #3131 find_record_with_rbac raises exception if it should return empty array.

Links

#3131

@miq-bot
Copy link
Member

miq-bot commented Jan 8, 2018

Checked commit PanSpagetka@f468658 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. 🍰

@PanSpagetka PanSpagetka closed this Jan 8, 2018
@PanSpagetka PanSpagetka reopened this Jan 8, 2018
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.

Looks good 👍

@romanblanco
Copy link
Member

@miq-bot add_label technical debt
@miq-bot add_label gaprindashvili/no
@miq-bot assign @martinpovolny

@martinpovolny
Copy link
Member

Are we sure that with the normal order of operations the no selection case is properly handled?

@romanblanco
Copy link
Member

romanblanco commented Jan 9, 2018

@martinpovolny I'm not sure what you mean by properly handled.

At current state, find_records_with_rbac raises an exception in case it receives an empty list of records (which user should not be able to send in normal situation, AFAIK), or in case user tries to fetch records he's not permited to access.

I can't think of any case where the user could run into an exception after doing steps he's expected to do. In all other cases I think the users workflow was incorrect, or there was other error in the code, and thus he should end up with an exception.

So I'd say it is. Or is there something more that should be kept in mind?

@PanSpagetka
Copy link
Contributor Author

Agree with @romanblanco when it blows up user either was doing something, he shouldn't be doing or there is some bug in the code. Deleting X.empty? conditions doesn't change the behavior at all.

@martinpovolny martinpovolny merged commit 36a0514 into ManageIQ:master Jan 10, 2018
@martinpovolny martinpovolny added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 10, 2018
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.

None yet

4 participants