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

Set value of static text box to default when default exists #17631

Merged
merged 1 commit into from
Jun 26, 2018
Merged

Set value of static text box to default when default exists #17631

merged 1 commit into from
Jun 26, 2018

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jun 25, 2018 •
edited by simaishi
Loading

's for this idiocy: https://bugzilla.redhat.com/show_bug.cgi?id=1576107

If the value is blank and the field is static, the value should be set to the default if the default exists. 😭

@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 25, 2018

Travis failure looks to be unrelated.

@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 25, 2018

@miq-bot add_label bug

@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 25, 2018

@miq-bot assign @gmcculloug
@eclarizio can you please 👀 for me, thanks 🎉

@miq-bot miq-bot added the bug label Jun 25, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 25, 2018

@miq-bot add_label gaprindashvili/yes

@JPrause
Copy link
Member

JPrause commented Jun 25, 2018

@miq-bot add_label blocker

@gmcculloug
Copy link
Member

@d-m-u Tests?

@d-m-u d-m-u force-pushed the fixing_automate_hash_with_default_value branch 2 times, most recently from 081f35c to 0198f83 Compare June 25, 2018 17:11
eclarizio
eclarizio reviewed Jun 25, 2018
View reviewed changes
Copy link
Member

@eclarizio eclarizio left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just thinking about why this change is necessary?

The #automate_output_value method is meant to be called after the automate method has been called, so when the automate method ends up setting the default_value, that seems like the wrong thing to be setting. It's a text box, the default_value and value should just be the same thing for dynamic ones. If it were static, a default_value makes sense.

So to me it seems like this is either fixing a problem where the configuration of the automate method needs to change, or we have a different problem that I'm misinterpreting.

I was thinking of the #extract_dynamic_values method in my above "rant", but still talking it through, we need some other way to determine whether or not to simply use the default_value. If we blindly use default_value if it exists, if they've changed the text box from the default, they won't see the results they want.

spec/models/dialog_field_text_box_spec.rb Outdated
@@ -337,6 +337,10 @@
described_class.new(:value => "12test", :data_type => data_type, :protected => protected_attr)
end

let(:dialog_field_with_default_value) do
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 is pedantic but can you move this into the context that uses it?

@d-m-u d-m-u changed the title Check text box for default value for automate hash correctness [WIP] Check text box for default value for automate hash correctness Jun 25, 2018
@miq-bot miq-bot added the wip label Jun 25, 2018
@d-m-u d-m-u force-pushed the fixing_automate_hash_with_default_value branch from 0198f83 to f348c2f Compare June 25, 2018 18:48
@d-m-u d-m-u force-pushed the fixing_automate_hash_with_default_value branch from f348c2f to 0a705d3 Compare June 25, 2018 18:51
@d-m-u d-m-u changed the title [WIP] Check text box for default value for automate hash correctness Check text box for default value for automate hash correctness Jun 25, 2018
@miq-bot
Copy link
Member

miq-bot commented Jun 25, 2018

Checked commit d-m-u@0a705d3 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. 🏆

@miq-bot miq-bot removed the wip label Jun 25, 2018
@d-m-u d-m-u changed the title Check text box for default value for automate hash correctness Set value of static text box to default when default exists Jun 25, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 25, 2018

Still think the Travis failure is unrelated.

eclarizio
eclarizio approved these changes Jun 26, 2018
View reviewed changes
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.

Looks fine to me now, not sure why travis is failing?

@gmcculloug gmcculloug merged commit 71f88fa into ManageIQ:master Jun 26, 2018
@gmcculloug gmcculloug added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 26, 2018
simaishi pushed a commit that referenced this pull request Jun 26, 2018
@gmcculloug @simaishi
Merge pull request #17631 from d-m-u/fixing_automate_hash_with_defaul…
bd0e686 
…t_value

Set value of static text box to default when default exists
(cherry picked from commit 71f88fa)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1591425
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit bd0e686323f0ffd5d5c4aa20698e73715f346dd8
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Jun 26 07:57:01 2018 -0400

    Merge pull request #17631 from d-m-u/fixing_automate_hash_with_default_value
    
    Set value of static text box to default when default exists
    (cherry picked from commit 71f88fae80bbfa79faeddd5decd0d7fa77ed2ff6)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1591425

@d-m-u d-m-u deleted the fixing_automate_hash_with_default_value branch October 25, 2018 14:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eclarizio eclarizio eclarizio approved these changes

Assignees

@gmcculloug gmcculloug

Labels
blocker bug gaprindashvili/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@d-m-u @JPrause @gmcculloug @miq-bot @simaishi @eclarizio