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

Fix for a Dialog with Tag Control Failure #11773

Merged
merged 1 commit into from Oct 7, 2016

Conversation

eclarizio
Copy link
Member

The "raw_values" method for tag control got removed because I was consolidating similar functionality in commit c16275b. However, tag control values in the db are not set (they're calculated at runtime as they're based on a category), and thus when self[:values].to_miq_a was ran, it was only returning a single array, not a multidimensional one as expected. I believe this issue can happen as well with regular drop downs and radio buttons, so this should fix those too.

Steps for Testing/QA

Steps are included in the BZ, but I will include them here as well:

Attempt to order a service catalog with dialog that contains a tag control field attached. Before this fix, clicking order will cause a server error but no indication anything happened, and after, it should work as expected and show the tag control field.

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

/cc @gmcculloug

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

Also will fix the issue for either drop downs or radio buttons that
somehow end up with nil values instead of multidimentional arrays
@miq-bot
Copy link
Member

miq-bot commented Oct 7, 2016

Checked commit eclarizio@a72ab1b with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

@gmcculloug
Copy link
Member

@bzwei Please review

@@ -93,7 +93,7 @@ def sort_data(data_to_sort)
def raw_values
@raw_values ||= dynamic ? values_from_automate : self[:values].to_miq_a
unless @raw_values.collect { |value_pair| value_pair[0] }.include?(default_value)
self.default_value = sort_data(@raw_values).first.first
self.default_value = sort_data(@raw_values).first.try(:first)
Copy link
Member

Choose a reason for hiding this comment

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

@eclarizio It looks like @raw_values is an empty array here for the tag control. I would expect other field to return a multi-dimensional array (like [[1, "One"], [2, "Two"]]) if they had values, otherwise they could have empty arrays as well. Does that sound correct to you?

This change will now set default_value to nil when we get an empty @raw_values array.

Copy link
Member Author

Choose a reason for hiding this comment

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

@gmcculloug Yes, that is correct.

Maybe this has to do with timing, but the other alternative would be to check for an empty array and in that case not set the default_value. On line 98, though, we're using default_value, and it's already been established by line 95 that the default_value is non-existent within the @raw_values array, so setting it to nil is probably the better option since you shouldn't be able to select a value that isn't there as opposed to just not selecting a value.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, I like that we are setting default_value to nil in this case. 👍

@gmcculloug
Copy link
Member

LGTM

@bzwei
Copy link
Contributor

bzwei commented Oct 7, 2016

The fix looks good to me. Thanks.

@gmcculloug
Copy link
Member

This change is needed in Darga but will require a separate PR.

@gmcculloug gmcculloug merged commit bf2d99d into ManageIQ:master Oct 7, 2016
@gmcculloug gmcculloug added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 7, 2016
chessbyte pushed a commit that referenced this pull request Oct 13, 2016
Fix for a Dialog with Tag Control Failure
(cherry picked from commit bf2d99d)

https://bugzilla.redhat.com/show_bug.cgi?id=1382765
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log
commit 90c546a82f309c6b65c28366fc2eaba40c81e403
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Fri Oct 7 16:56:09 2016 -0400

    Merge pull request #11773 from eclarizio/BZ1382765

    Fix for a Dialog with Tag Control Failure
    (cherry picked from commit bf2d99dd8310cefa16305303822a86f6a3c05066)

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

@eclarizio
Copy link
Member Author

@gmcculloug I think we talked about this one and concluded that it just needed to be backported to darga after #11781, right? If that's the case, then we just need to add the darga/yes label, I think?

@gmcculloug
Copy link
Member

@eclarizio Correct, looks like all the proper flags are set now.

@chessbyte
Copy link
Member

chessbyte pushed a commit that referenced this pull request Oct 17, 2016
Fix for a Dialog with Tag Control Failure
(cherry picked from commit bf2d99d)

https://bugzilla.redhat.com/show_bug.cgi?id=1385887
@chessbyte
Copy link
Member

Darga Backport details:

$ git log -1
commit 4fe9d6660b977c18ac2e5e083d43d54c18205ed0
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Fri Oct 7 16:56:09 2016 -0400

    Merge pull request #11773 from eclarizio/BZ1382765

    Fix for a Dialog with Tag Control Failure
    (cherry picked from commit bf2d99dd8310cefa16305303822a86f6a3c05066)

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

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

5 participants