From 86f4767dc1afea9f0744e4fb0c5ce663bf7e3de8 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 23 Dec 2016 00:26:33 +0530
Subject: [PATCH] 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`).
---
config/routes/project.rb | 2 +-
.../projects/pages_domains_controller_spec.rb | 64 +++++++++++++++++++
spec/routing/project_routing_spec.rb | 37 +++++++++--
3 files changed, 95 insertions(+), 8 deletions(-)
create mode 100644 spec/controllers/projects/pages_domains_controller_spec.rb
diff --git a/config/routes/project.rb b/config/routes/project.rb
index ea3bfdd45e6..b6b432256df 100644
--- a/config/routes/project.rb
+++ b/config/routes/project.rb
@@ -40,7 +40,7 @@ constraints(ProjectUrlConstrainer.new) do
end
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
resources :compare, only: [:index, :create] do
diff --git a/spec/controllers/projects/pages_domains_controller_spec.rb b/spec/controllers/projects/pages_domains_controller_spec.rb
new file mode 100644
index 00000000000..2362df895a8
--- /dev/null
+++ b/spec/controllers/projects/pages_domains_controller_spec.rb
@@ -0,0 +1,64 @@
+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
diff --git a/spec/routing/project_routing_spec.rb b/spec/routing/project_routing_spec.rb
index 77549db2927..43be785ad9d 100644
--- a/spec/routing/project_routing_spec.rb
+++ b/spec/routing/project_routing_spec.rb
@@ -27,35 +27,42 @@ describe 'project routing' do
# let(:actions) { [:index] }
# let(:controller) { 'issues' }
# 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
let(:actions) { [:index, :create, :new, :edit, :show, :update, :destroy] }
+ let(:controller_path) { controller }
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
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
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
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
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
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
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
@@ -539,4 +546,20 @@ describe 'project routing' do
'projects/avatars#destroy', namespace_id: 'gitlab', project_id: 'gitlabhq')
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
--
GitLab