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

Adds field unique validator check to dialog #16487

Merged
merged 1 commit into from
Nov 21, 2017

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Nov 16, 2017

This adds a uniqueness check on dialog field names (from this bz: https://bugzilla.redhat.com/show_bug.cgi?id=1491790) so we don't end up with dialogs with two equal field names which goes kaput.

Linked PR adds back in flash messages on dialog edit screen that went kaput when new dialog editor came online. Without it, this check fails silently and is yuck.

Will be a terrible experience unless this gets in soon too: ( 🎉 )

ManageIQ/manageiq-ui-classic#2772

(and by soon I mean basically at the same time)

@miq-bot miq-bot added the wip label Nov 16, 2017
@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 16, 2017

@miq-bot assign @gmcculloug

@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 16, 2017

@miq-bot add_label bug

@@ -38,6 +38,10 @@ def validate_children
errors.add(:base, _("Box %{box_label} must have at least one Element") % {:box_label => label})
end

if dialog_fields.collect(&:name).duplicates.present?
errors.add(:base, _("Dialog field name cannot be duplicated on a dialog: %{duplicates}") % {:duplicates => dialog_fields.collect(&:name).duplicates.join(', ')})
Copy link
Member

Choose a reason for hiding this comment

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

This does not belong on the group object since dialog_fields would only validate objects in that group and not the entire dialog.

@d-m-u d-m-u force-pushed the unique_dialog_field_names branch 6 times, most recently from adb28df to c238ee0 Compare November 16, 2017 20:57
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 to me like it should do the job 👍

@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 17, 2017

@eclarizio The problem with this currently is that we have no flash messages in new dialog editor, so this error is never shown anywhere.

@eclarizio
Copy link
Member

@d-m-u Right, I realize that's why it's marked as WIP, but just wanted to give the 👍 approval for if/when the UI side gets sorted out.

@d-m-u d-m-u changed the title [WIP] Adds field unique validator check to group model Adds field unique validator check to group model Nov 20, 2017
@d-m-u
Copy link
Contributor Author

d-m-u commented Nov 20, 2017

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Nov 20, 2017
@d-m-u d-m-u changed the title Adds field unique validator check to group model Adds field unique validator check to dialog Nov 20, 2017
@miq-bot miq-bot removed the wip label Nov 20, 2017
@@ -61,6 +61,10 @@ def validate_children
errors.add(:base, _("Dialog %{dialog_label} must have at least one Tab") % {:dialog_label => label})
end

if dialog_fields.collect(&:name).duplicates.present?
Copy link
Member

Choose a reason for hiding this comment

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

@d-m-u Set dialog_fields.collect(&:name).duplicates to a variable so we do not need to call it twice if there are duplicates.

@miq-bot
Copy link
Member

miq-bot commented Nov 21, 2017

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

@gmcculloug gmcculloug merged commit ece6e89 into ManageIQ:master Nov 21, 2017
@gmcculloug gmcculloug added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 21, 2017
@d-m-u d-m-u deleted the unique_dialog_field_names branch November 21, 2017 14:56
@simaishi
Copy link
Contributor

gaprindashvili/yes ?

@gmcculloug
Copy link
Member

@simaishi I set the gaprindashvili/yes flag. Thanks

simaishi pushed a commit that referenced this pull request Nov 28, 2017
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 89ebd845ee7f8bcaa93960840c66844afa634496
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Tue Nov 21 09:51:41 2017 -0500

    Merge pull request #16487 from d-m-u/unique_dialog_field_names
    
    Adds field unique validator check to dialog
    (cherry picked from commit ece6e891290beda5b9945a00014ecd53431479d6)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1518267

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.

5 participants