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

[GAPRINDASHVILI] Set checkbox on load, sans default, to be false, not nil #17810

Merged
merged 1 commit into from
Aug 9, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Aug 7, 2018

Non-dynamic dialog field checkboxes without a default should load with value false, not nil.

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

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 7, 2018

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

@miq-bot miq-bot added the bug label Aug 7, 2018
@miq-bot miq-bot requested review from eclarizio and tinaafitz August 7, 2018 14:14
@miq-bot miq-bot changed the title Set checkbox on load, sans default, to be false, not nil [GAPRINDASHVILI] Set checkbox on load, sans default, to be false, not nil Aug 7, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 7, 2018

Uuum, I'mma say that @options.cumulative_rate_calculation? ? rates.sort_by(&:description) : unique_rates_by_tagged_resources(rates, metric_rollup_record_tags) failing has absolutely nothing to do with any of this code.

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

#initialize_value_context isn't meant to return a value, it's just meant to set up the @value instance variable. I think your specs need to call the method, and then you should simply be able to call dialog_field.value to check that the value was properly assigned. That was the my main goal of using the #initialize_value_context, since #value used to be used as not only a getter but also with a bunch of logic that we didn't want in there executing when we weren't sure it was executing.

it "returns automate vals" do
dialog_field.initialize_value_context

expect(dialog_field.value).to be_truthy
Copy link
Member

Choose a reason for hiding this comment

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

I know this has to do with our wonky "t" or "f", but it's kind of confusing that we're expecting a be_truthy for an "f". Technically, that's correct, but it's also correct if the default value is a "t" (as shown in the next spec down).

I think both of those specs should probably be changed from be_truthy to what the value is really supposed to be.

@miq-bot
Copy link
Member

miq-bot commented Aug 8, 2018

Checked commit d-m-u@6192982 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. 🍰

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 8, 2018

Once again, I'mma say that Failure/Error: expect(subject.fixed_compute_1_cost).to be_within(0.01).of(hourly_rate * hours_in_month) expected 4.8 to be within 0.01 of 7.2 has absolutely zip to do with this code.

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

Agreed, not sure why that one spec is failing.

¯\_(ツ)_/¯

@gmcculloug gmcculloug merged commit f5c9fc6 into ManageIQ:gaprindashvili Aug 9, 2018
@gmcculloug gmcculloug added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 9, 2018
@simaishi
Copy link
Contributor

@d-m-u Just noticed this PR... is there a corresponding 'master' PR for this?

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 16, 2018

@simaishi No, one isn't necessary, I believe it works on master.

@d-m-u d-m-u deleted the bz_1610685_gap branch February 1, 2019 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants