Skip to content
This repository has been archived by the owner on Jan 7, 2018. It is now read-only.

Commit

Permalink
Fix api url_matches and servers validations not applying on updates.
Browse files Browse the repository at this point in the history
Since we're using the Rails nested attributes stuff, when items are
removed, they're only marked for deletion, which means the validations
still think they exist. We have to explicitly look at each record to see
if it's marked for deletion to really determine whether any url matches
or servers will still exist after saving.

Fixes 18F/api.data.gov#215
  • Loading branch information
GUI committed Apr 19, 2015
1 parent 229befe commit 4d50cb2
Show file tree
Hide file tree
Showing 7 changed files with 142 additions and 10 deletions.
2 changes: 1 addition & 1 deletion app/assets/javascripts/admin/templates/apis/_form.hbs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
optionValuePath='content.id'
optionLabelPath='content.name'}}

<table class="table table-striped table-condensed">
<table id="servers_table" class="table table-striped table-condensed">
<thead>
<tr>
<th>{{t 'admin.api.servers.server'}}</th>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<div id="url_matches">
<table id="url_matches" class="table table-striped table-condensed">
<table id="url_matches_table" class="table table-striped table-condensed">
<thead>
<tr>
<th>Frontend Prefix</th>
Expand Down
2 changes: 1 addition & 1 deletion app/models/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ class Api
validates :balance_algorithm,
:inclusion => { :in => %w(round_robin least_conn ip_hash) }
validates_each :servers, :url_matches do |record, attr, value|
if(value.blank?)
if(value.blank? || (value && value.reject(&:marked_for_destruction?).blank?))
record.errors.add(:base, "must have at least one #{attr}")
end
end
Expand Down
12 changes: 6 additions & 6 deletions spec/cassettes/elasticsearch_templates.yml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

108 changes: 107 additions & 1 deletion spec/controllers/api/v1/apis_controller_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,97 @@
@empty_url_prefixes_api.save!(:validate => false)
end

shared_examples "validates nested attributes presence - create" do |field|
it "returns a validation error if #{field} is set to nil" do
admin_token_auth(@admin)
attributes = FactoryGirl.attributes_for(:api, {
field => nil,
})

expect do
post :create, :format => "json", :api => attributes
response.status.should eql(422)
data = MultiJson.load(response.body)
data.keys.should eql(["errors"])
data["errors"].should eql({
"base" => ["must have at least one #{field}"],
})
end.to_not change { Api.count }
end

it "returns a validation error if #{field} is set to an empty array" do
admin_token_auth(@admin)
attributes = FactoryGirl.attributes_for(:api, {
field => [],
})

expect do
post :create, :format => "json", :api => attributes
response.status.should eql(422)
data = MultiJson.load(response.body)
data.keys.should eql(["errors"])
data["errors"].should eql({
"base" => ["must have at least one #{field}"],
})
end.to_not change { Api.count }
end

it "accepts the input if at least one url prefix exists" do
admin_token_auth(@admin)
attributes = FactoryGirl.attributes_for(:api, {
field => [FactoryGirl.attributes_for(:"api_#{field.to_s.singularize}")],
})

expect do
post :create, :format => "json", :api => attributes
response.status.should eql(201)
data = MultiJson.load(response.body)
data["api"]["name"].should eql(attributes[:name])
end.to change { Api.count }.by(1)
end
end

shared_examples "validates nested attributes presence - update" do |field|
it "returns a validation error if #{field} is set to nil" do
admin_token_auth(@admin)
attributes = @api.serializable_hash
attributes[field.to_s] = nil

put :update, :format => "json", :id => @api.id, :api => attributes
response.status.should eql(422)
data = MultiJson.load(response.body)
data.keys.should eql(["errors"])
data["errors"].should eql({
"base" => ["must have at least one #{field}"],
})
end

it "returns a validation error if #{field} is set to an empty array" do
admin_token_auth(@admin)
attributes = @api.serializable_hash
attributes[field.to_s] = []

put :update, :format => "json", :id => @api.id, :api => attributes
response.status.should eql(422)
data = MultiJson.load(response.body)
data.keys.should eql(["errors"])
data["errors"].should eql({
"base" => ["must have at least one #{field}"],
})
end

it "accepts the input if at least one #{field} exists" do
admin_token_auth(@admin)
attributes = @api.serializable_hash
attributes[field.to_s] = [FactoryGirl.attributes_for(:"api_#{field.to_s.singularize}")]

put :update, :format => "json", :id => @api.id, :api => attributes
response.status.should eql(204)
@api = Api.find(@api.id)
@api[field].length.should eql(1)
end
end

shared_examples "api settings header fields - show" do |field|
it "returns no headers as an empty string" do
api = FactoryGirl.create(:api, {
Expand Down Expand Up @@ -229,7 +320,6 @@
api.settings.send(field).length.should eql(1)
end.to change { Api.count }.by(1)
end

end

shared_examples "api settings header fields - update" do |field|
Expand Down Expand Up @@ -758,6 +848,14 @@
end
end

describe "servers" do
it_behaves_like "validates nested attributes presence - create", :servers
end

describe "matching url prefixes" do
it_behaves_like "validates nested attributes presence - create", :url_matches
end

describe "request headers" do
it_behaves_like "api settings header fields - create", :headers
end
Expand Down Expand Up @@ -1035,6 +1133,14 @@
end
end

describe "servers" do
it_behaves_like "validates nested attributes presence - update", :servers
end

describe "matching url prefixes" do
it_behaves_like "validates nested attributes presence - update", :url_matches
end

describe "request headers" do
it_behaves_like "api settings header fields - update", :headers
end
Expand Down
2 changes: 2 additions & 0 deletions spec/factories/api_url_matches.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,7 @@

FactoryGirl.define do
factory :api_url_match, :class => 'Api::UrlMatch' do
frontend_prefix "/example-frontend/"
backend_prefix "/example-backend/"
end
end
24 changes: 24 additions & 0 deletions spec/features/admin/apis_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -146,4 +146,28 @@
find_field("Frontend Host").value.should eql("example2.com")
end
end

it "returns a validation error when all servers are removed from an existing API" do
api = FactoryGirl.create(:api)
visit "/admin/#/apis/#{api.id}/edit"
find("#servers_table a", :text => /Remove/).click
click_link("OK")
click_button("Save")
page.should have_content("must have at least one servers")

api = Api.find(api.id)
api.servers.length.should eql(1)
end

it "returns a validation error when all url prefixes are removed from an existing API" do
api = FactoryGirl.create(:api)
visit "/admin/#/apis/#{api.id}/edit"
find("#url_matches_table a", :text => /Remove/).click
click_link("OK")
click_button("Save")
page.should have_content("must have at least one url_matches")

api = Api.find(api.id)
api.url_matches.length.should eql(1)
end
end

0 comments on commit 4d50cb2

Please sign in to comment.