Commit 86f4767d authored by Timothy Andrew's avatar Timothy Andrew Committed by James Edwards-Jones

Fix 500 error while navigating to the `pages_domains` 'show' page.

==================
= Implementation =
==================

1. The path of the page is of the form 'group/project/pages/domains/<domain_name>'
2. Rails looks at `params[:id]` (which should be the domain name), and finds the
   relevant model record.
3. Given a domain like `foo.bar`, Rails sets `params[:id]` to `foo` (should be
   `foo.bar`), and sets `params[:format]` to `bar`
4. This commit fixes the issue by adding a route constraint, so that
   `params[:id]` is set to the entire `foo.bar` domain name.

=========
= Tests =
=========

1. Add controller specs for the `PagesDomainController`. These are
   slightly orthogonal to this bug fix (they don't fail when this bug is
   present), but should be present nonetheless.
2. Add routing specs that catch this bug (by asserting that the `id`
   param is passed as expected when it contains a domain name).
3. Modify the 'RESTful project resources' routing spec shared example to
   accomodate controllers where the controller path (such as
   `pages/domains`) is different from the controller name (such as
   `pages_domains`).
parent 80f9794c
...@@ -40,7 +40,7 @@ constraints(ProjectUrlConstrainer.new) do ...@@ -40,7 +40,7 @@ constraints(ProjectUrlConstrainer.new) do
end end
resource :pages, only: [:show, :destroy] do resource :pages, only: [:show, :destroy] do
resources :domains, only: [:show, :new, :create, :destroy], controller: 'pages_domains' resources :domains, only: [:show, :new, :create, :destroy], controller: 'pages_domains', constraints: { id: /[^\/]+/ }
end end
resources :compare, only: [:index, :create] do resources :compare, only: [:index, :create] do
......
require 'spec_helper'
describe Projects::PagesDomainsController do
let(:user) { create(:user) }
let(:project) { create(:project) }
let(:request_params) do
{
namespace_id: project.namespace,
project_id: project
}
end
before do
sign_in(user)
project.team << [user, :master]
end
describe 'GET show' do
let!(:pages_domain) { create(:pages_domain, project: project) }
it "displays the 'show' page" do
get(:show, request_params.merge(id: pages_domain.domain))
expect(response).to have_http_status(200)
expect(response).to render_template('show')
end
end
describe 'GET new' do
it "displays the 'new' page" do
get(:new, request_params)
expect(response).to have_http_status(200)
expect(response).to render_template('new')
end
end
describe 'POST create' do
let(:pages_domain_params) do
build(:pages_domain, :with_certificate, :with_key).slice(:key, :certificate, :domain)
end
it "creates a new pages domain" do
expect do
post(:create, request_params.merge(pages_domain: pages_domain_params))
end.to change { PagesDomain.count }.by(1)
expect(response).to redirect_to(namespace_project_pages_path(project.namespace, project))
end
end
describe 'DELETE destroy' do
let!(:pages_domain) { create(:pages_domain, project: project) }
it "deletes the pages domain" do
expect do
delete(:destroy, request_params.merge(id: pages_domain.domain))
end.to change { PagesDomain.count }.by(-1)
expect(response).to redirect_to(namespace_project_pages_path(project.namespace, project))
end
end
end
...@@ -27,35 +27,42 @@ describe 'project routing' do ...@@ -27,35 +27,42 @@ describe 'project routing' do
# let(:actions) { [:index] } # let(:actions) { [:index] }
# let(:controller) { 'issues' } # let(:controller) { 'issues' }
# end # end
#
# # Different controller name and path
# it_behaves_like 'RESTful project resources' do
# let(:controller) { 'pages_domains' }
# let(:controller_path) { 'pages/domains' }
# end
shared_examples 'RESTful project resources' do shared_examples 'RESTful project resources' do
let(:actions) { [:index, :create, :new, :edit, :show, :update, :destroy] } let(:actions) { [:index, :create, :new, :edit, :show, :update, :destroy] }
let(:controller_path) { controller }
it 'to #index' do it 'to #index' do
expect(get("/gitlab/gitlabhq/#{controller}")).to route_to("projects/#{controller}#index", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:index) expect(get("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#index", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:index)
end end
it 'to #create' do it 'to #create' do
expect(post("/gitlab/gitlabhq/#{controller}")).to route_to("projects/#{controller}#create", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:create) expect(post("/gitlab/gitlabhq/#{controller_path}")).to route_to("projects/#{controller}#create", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:create)
end end
it 'to #new' do it 'to #new' do
expect(get("/gitlab/gitlabhq/#{controller}/new")).to route_to("projects/#{controller}#new", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:new) expect(get("/gitlab/gitlabhq/#{controller_path}/new")).to route_to("projects/#{controller}#new", namespace_id: 'gitlab', project_id: 'gitlabhq') if actions.include?(:new)
end end
it 'to #edit' do it 'to #edit' do
expect(get("/gitlab/gitlabhq/#{controller}/1/edit")).to route_to("projects/#{controller}#edit", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:edit) expect(get("/gitlab/gitlabhq/#{controller_path}/1/edit")).to route_to("projects/#{controller}#edit", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:edit)
end end
it 'to #show' do it 'to #show' do
expect(get("/gitlab/gitlabhq/#{controller}/1")).to route_to("projects/#{controller}#show", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:show) expect(get("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#show", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:show)
end end
it 'to #update' do it 'to #update' do
expect(put("/gitlab/gitlabhq/#{controller}/1")).to route_to("projects/#{controller}#update", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:update) expect(put("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#update", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:update)
end end
it 'to #destroy' do it 'to #destroy' do
expect(delete("/gitlab/gitlabhq/#{controller}/1")).to route_to("projects/#{controller}#destroy", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:destroy) expect(delete("/gitlab/gitlabhq/#{controller_path}/1")).to route_to("projects/#{controller}#destroy", namespace_id: 'gitlab', project_id: 'gitlabhq', id: '1') if actions.include?(:destroy)
end end
end end
...@@ -539,4 +546,20 @@ describe 'project routing' do ...@@ -539,4 +546,20 @@ describe 'project routing' do
'projects/avatars#destroy', namespace_id: 'gitlab', project_id: 'gitlabhq') 'projects/avatars#destroy', namespace_id: 'gitlab', project_id: 'gitlabhq')
end end
end end
describe Projects::PagesDomainsController, 'routing' do
it_behaves_like 'RESTful project resources' do
let(:actions) { [:show, :new, :create, :destroy] }
let(:controller) { 'pages_domains' }
let(:controller_path) { 'pages/domains' }
end
it 'to #destroy with a valid domain name' do
expect(delete('/gitlab/gitlabhq/pages/domains/my.domain.com')).to route_to('projects/pages_domains#destroy', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'my.domain.com')
end
it 'to #show with a valid domain' do
expect(get('/gitlab/gitlabhq/pages/domains/my.domain.com')).to route_to('projects/pages_domains#show', namespace_id: 'gitlab', project_id: 'gitlabhq', id: 'my.domain.com')
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment