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

Add uniqueness validation on Endpoint#url #12068

Merged
merged 2 commits into from
Oct 20, 2016

Conversation

bdunne
Copy link
Member

@bdunne bdunne commented Oct 19, 2016

Don't allow Foreman or AnsibleTower providers to be added multiple times.

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

Endpoint.create!(:url => "abc")
expect { Endpoint.create!(:url => "abc") }.to raise_error("Validation failed: Url has already been taken")
end
end
Copy link
Member

Choose a reason for hiding this comment

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

TBH, I'm indifferent about these specs. You're essentially testing that ActiveRecord does it's job (which is also why I'm generally against shoulda-matchers. Are you sure you want to add these db writes to the tests? Your choice but thought I'd throw my thoughts out there.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was on the fence about writing them since it is just testing rails validations.

My only argument for these kinds of tests is that it shows the intent of the validations for people making future changes. It's essentially testing this use case.

Copy link
Member

Choose a reason for hiding this comment

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

Alrighty 👍

@chrisarcand chrisarcand self-assigned this Oct 19, 2016
@miq-bot
Copy link
Member

miq-bot commented Oct 20, 2016

Checked commits bdunne/manageiq@b019e53~...2a1e51f with ruby 2.2.5, rubocop 0.37.2, and haml-lint 0.16.1
3 files checked, 0 offenses detected
Everything looks good. 🍪

@bdunne bdunne closed this Oct 20, 2016
@bdunne bdunne reopened this Oct 20, 2016
controller.instance_variable_set(:@provider_cfgmgmt, provider2)
allow(controller).to receive(:render_flash)
controller.save_provider_foreman
expect(assigns(:flash_array).first[:message]).to include("Configuration_manager.name has already been taken")
Copy link
Member

Choose a reason for hiding this comment

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

Grumble. I guess.

@chrisarcand chrisarcand merged commit 97e2b60 into ManageIQ:master Oct 20, 2016
@chrisarcand chrisarcand added this to the Sprint 48 Ending Oct 24, 2016 milestone Oct 20, 2016
@bdunne bdunne deleted the ansible_tower_duplicates branch October 20, 2016 19:06
chessbyte pushed a commit that referenced this pull request Oct 21, 2016
@chessbyte
Copy link
Member

Euwe Backport details:

$ git log -1
commit 13c6d9cfc3c8df082c5a99dddba7b0318b26cec1
Author: Chris Arcand <chrisarcand@users.noreply.github.com>
Date:   Thu Oct 20 13:57:54 2016 -0500

    Merge pull request #12068 from bdunne/ansible_tower_duplicates

    Add uniqueness validation on Endpoint#url
    (cherry picked from commit 97e2b60505bba824ea91617afb796f830a85fa77)

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

@chessbyte
Copy link
Member

@simaishi Please make Euwe BZ from https://bugzilla.redhat.com/show_bug.cgi?id=1382627

@simaishi
Copy link
Contributor

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

6 participants