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

Load all the values along with their key names into the field update method #17973

Merged
merged 1 commit into from Sep 12, 2018
Merged

Load all the values along with their key names into the field update method #17973

merged 1 commit into from Sep 12, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Sep 11, 2018 •
edited

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1594469.

The main changes that got merged a little while ago, combined with #17855, break the custom actions API specs on ManageIQ/manageiq-api#448 because I'm an idiot who wrote that line that's breaking this to start with which shouldn't be just loading the values and definitely shouldn't just be loading the first one. Mea maxima culpa.

@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 11, 2018

@miq-bot assign @gmcculloug
@miq-bot add_label bug
@miq-bot add_reviewer @tinaafitz

@d-m-u d-m-u mentioned this pull request Sep 11, 2018
Merged
@miq-bot miq-bot added the bug label Sep 11, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 11, 2018 •
edited

Failure, smailure, it's the same sporadic failure we're seeing everywhere, the number precision thingy, it's got nothing to do with this change.

tinaafitz
tinaafitz approved these changes Sep 11, 2018
View reviewed changes
Copy link
Member

@tinaafitz tinaafitz left a comment

Choose a reason for hiding this comment

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

@d-m-u Looks good.
@mkanoor Please review.

@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 11, 2018

@miq-bot add_reviewer @bdunne
sorry for the silliness but you're faster than mkanoor 😆

@miq-bot miq-bot requested a review from bdunne September 11, 2018 18:13
bdunne
bdunne reviewed Sep 11, 2018
View reviewed changes
app/models/dialog.rb Outdated
@@ -89,7 +89,7 @@ def validate_field_data
end

def load_values_into_fields(values, overwrite = true)
values = values.with_indifferent_access
values = values.with_indifferent_access if values.kind_of?(Hash)
Copy link
Member

Choose a reason for hiding this comment

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

What else could values be if not a hash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a string in the api tests.

Copy link
Member

Choose a reason for hiding this comment

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

That sounds wrong to me, especially when [] is called on it here

Copy link
Member

Choose a reason for hiding this comment

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

Sounds like the tests need to be fixed

@d-m-u d-m-u changed the title Add check for only using indiff access with hash Load the all the values along with their key names into the field update method Sep 11, 2018
@d-m-u d-m-u changed the title Load the all the values along with their key names into the field update method Load all the values along with their key names into the field update method Sep 11, 2018
@d-m-u d-m-u changed the title Load all the values along with their key names into the field update method [WIP] Load all the values along with their key names into the field update method Sep 11, 2018
@miq-bot miq-bot added the wip label Sep 11, 2018
@d-m-u d-m-u changed the title [WIP] Load all the values along with their key names into the field update method Load all the values along with their key names into the field update method Sep 12, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 12, 2018

@miq_bot remove_label wip

@miq-bot miq-bot removed the wip label Sep 12, 2018
@bdunne
Copy link
Member

bdunne commented Sep 12, 2018

The fact that this doesn't break any tests (before or after the change) makes me think we don't have enough tests around it...

@miq-bot
Copy link
Member

miq-bot commented Sep 12, 2018

Checked commit d-m-u@966dc9d with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@bdunne bdunne merged commit 378577a into ManageIQ:master Sep 12, 2018
@bdunne bdunne added this to the Sprint 95 Ending Sep 24, 2018 milestone Sep 12, 2018
@bdunne bdunne assigned bdunne and unassigned gmcculloug Sep 12, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 12, 2018

@bdunne I think this should be g/yes cause it's for a g bug.

@d-m-u
Copy link
Contributor Author

d-m-u commented Sep 12, 2018

@miq-bot remove_label gaprindashvili/no
@miq-bot add_label gaprindashvili/yes

@d-m-u d-m-u deleted the fixing_tests_on_api_pr_448 branch February 1, 2019 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@bdunne bdunne bdunne left review comments

@tinaafitz tinaafitz tinaafitz approved these changes

Assignees

@bdunne bdunne

Labels
bug lifecycle/services
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@d-m-u @bdunne @miq-bot @Fryguy @tinaafitz @gmcculloug