From ef47ea3d39d9235c2842d1f374d0f03e56da7eeb Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 15 Apr 2015 12:24:44 -0400 Subject: [PATCH 1/5] Revert "Fix and improve help rendering" This reverts commit d365004e684e98459061fcd5fbaf9bea880934a8. --- app/controllers/help_controller.rb | 32 +++++++------------------ app/views/help/show.html.haml | 2 +- config/initializers/mime_types.rb | 1 - config/routes.rb | 2 +- features/steps/dashboard/help.rb | 2 +- spec/features/help_pages_spec.rb | 2 +- spec/routing/routing_spec.rb | 38 +++++++++++++++--------------- 7 files changed, 31 insertions(+), 48 deletions(-) diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb index 0e5567c7734..964f624d6d7 100644 --- a/app/controllers/help_controller.rb +++ b/app/controllers/help_controller.rb @@ -3,40 +3,24 @@ class HelpController < ApplicationController end def show - @filepath = clean_path_info(params[:filepath]) - @format = params[:format] + @category = clean_path_info(params[:category]) + @file = clean_path_info(params[:file]) - respond_to do |format| - format.md { render_doc } - format.all { send_file_data } + if File.exists?(Rails.root.join('doc', @category, @file + '.md')) + render 'show' + else + not_found! end end def shortcuts end - private - - def render_doc - if File.exists?(Rails.root.join('doc', @filepath + '.md')) - render 'show.html.haml' - else - not_found! - end - end - - def send_file_data - path = Rails.root.join('doc', "#{@filepath}.#{@format}") - if File.exists?(path) - send_file(path, disposition: 'inline') - else - head :not_found - end - end - def ui end + private + PATH_SEPS = Regexp.union(*[::File::SEPARATOR, ::File::ALT_SEPARATOR].compact) # Taken from ActionDispatch::FileHandler diff --git a/app/views/help/show.html.haml b/app/views/help/show.html.haml index f22aa92caf7..eca34dbff06 100644 --- a/app/views/help/show.html.haml +++ b/app/views/help/show.html.haml @@ -1,2 +1,2 @@ .documentation.wiki - = markdown File.read(Rails.root.join('doc', @filepath + '.md')).gsub("$your_email", current_user.email) + = markdown File.read(Rails.root.join('doc', @category, @file + '.md')).gsub("$your_email", current_user.email) diff --git a/config/initializers/mime_types.rb b/config/initializers/mime_types.rb index 6978ad93024..8f8bef42bef 100644 --- a/config/initializers/mime_types.rb +++ b/config/initializers/mime_types.rb @@ -6,4 +6,3 @@ Mime::Type.register_alias "text/plain", :diff Mime::Type.register_alias "text/plain", :patch -Mime::Type.register_alias 'text/html', :md diff --git a/config/routes.rb b/config/routes.rb index 4f33b11d220..af58e095d9a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -39,9 +39,9 @@ Gitlab::Application.routes.draw do # Help get 'help' => 'help#index' + get 'help/:category/:file' => 'help#show', as: :help_page get 'help/shortcuts' get 'help/ui' => 'help#ui' - get 'help/:filepath' => 'help#show', as: :help_page, constraints: { filepath: /[^\.]+/ } # # Global snippets diff --git a/features/steps/dashboard/help.rb b/features/steps/dashboard/help.rb index fa52e391f05..ef433c57c6e 100644 --- a/features/steps/dashboard/help.rb +++ b/features/steps/dashboard/help.rb @@ -8,7 +8,7 @@ class Spinach::Features::DashboardHelp < Spinach::FeatureSteps end step 'I visit the "Rake Tasks" help page' do - visit help_page_path('raketasks/maintenance', format: 'md') + visit help_page_path("raketasks", "maintenance") end step 'I should see "Rake Tasks" page markdown rendered' do diff --git a/spec/features/help_pages_spec.rb b/spec/features/help_pages_spec.rb index 28423eb8caa..41088ce8271 100644 --- a/spec/features/help_pages_spec.rb +++ b/spec/features/help_pages_spec.rb @@ -6,7 +6,7 @@ describe 'Help Pages', feature: true do login_as :user end it 'replace the variable $your_email with the email of the user' do - visit help_page_path(filepath: 'ssh/README', format: 'md') + visit help_page_path(category: 'ssh', file: 'README.md') expect(page).to have_content("ssh-keygen -t rsa -C \"#{@user.email}\"") end end diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index f5db548f97c..d4915b51952 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -73,41 +73,41 @@ end # help_markdown GET /help/markdown(.:format) help#markdown # help_ssh GET /help/ssh(.:format) help#ssh # help_raketasks GET /help/raketasks(.:format) help#raketasks -describe HelpController, 'routing' do - it 'to #index' do - expect(get('/help')).to route_to('help#index') +describe HelpController, "routing" do + it "to #index" do + expect(get("/help")).to route_to('help#index') end - it 'to #permissions' do - expect(get('/help/permissions/permissions')).to route_to('help#show', filepath: 'permissions/permissions') + it "to #permissions" do + expect(get("/help/permissions/permissions")).to route_to('help#show', category: "permissions", file: "permissions") end - it 'to #workflow' do - expect(get('/help/workflow/README')).to route_to('help#show', filepath: 'workflow/README') + it "to #workflow" do + expect(get("/help/workflow/README")).to route_to('help#show', category: "workflow", file: "README") end - it 'to #api' do - expect(get('/help/api/README')).to route_to('help#show', filepath: 'api/README') + it "to #api" do + expect(get("/help/api/README")).to route_to('help#show', category: "api", file: "README") end - it 'to #web_hooks' do - expect(get('/help/web_hooks/web_hooks')).to route_to('help#show', filepath: 'web_hooks/web_hooks') + it "to #web_hooks" do + expect(get("/help/web_hooks/web_hooks")).to route_to('help#show', category: "web_hooks", file: "web_hooks") end - it 'to #system_hooks' do - expect(get('/help/system_hooks/system_hooks')).to route_to('help#show', filepath: 'system_hooks/system_hooks') + it "to #system_hooks" do + expect(get("/help/system_hooks/system_hooks")).to route_to('help#show', category: "system_hooks", file: "system_hooks") end - it 'to #markdown' do - expect(get('/help/markdown/markdown')).to route_to('help#show',filepath: 'markdown/markdown') + it "to #markdown" do + expect(get("/help/markdown/markdown")).to route_to('help#show',category: "markdown", file: "markdown") end - it 'to #ssh' do - expect(get('/help/ssh/README')).to route_to('help#show', filepath: 'ssh/README') + it "to #ssh" do + expect(get("/help/ssh/README")).to route_to('help#show', category: "ssh", file: "README") end - it 'to #raketasks' do - expect(get('/help/raketasks/README')).to route_to('help#show', filepath: 'raketasks/README') + it "to #raketasks" do + expect(get("/help/raketasks/README")).to route_to('help#show', category: "raketasks", file: "README") end end From 1e27b68b364cfc0b504e7db429c2450d75b72cfd Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 11 Apr 2015 16:46:36 -0400 Subject: [PATCH 2/5] Add Markdown to Mime types --- config/initializers/mime_types.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/config/initializers/mime_types.rb b/config/initializers/mime_types.rb index 8f8bef42bef..ca58ae92d1b 100644 --- a/config/initializers/mime_types.rb +++ b/config/initializers/mime_types.rb @@ -6,3 +6,5 @@ Mime::Type.register_alias "text/plain", :diff Mime::Type.register_alias "text/plain", :patch +Mime::Type.register_alias 'text/html', :markdown +Mime::Type.register_alias 'text/html', :md From e24cb79f3196052395829b35d51693dc9de5afbe Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Sat, 11 Apr 2015 16:49:03 -0400 Subject: [PATCH 3/5] Add constraints to help#show route parameters --- config/routes.rb | 2 +- spec/routing/routing_spec.rb | 55 +++++++++++++----------------------- 2 files changed, 21 insertions(+), 36 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index af58e095d9a..8dbe6d80ab7 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -39,7 +39,7 @@ Gitlab::Application.routes.draw do # Help get 'help' => 'help#index' - get 'help/:category/:file' => 'help#show', as: :help_page + get 'help/:category/:file' => 'help#show', as: :help_page, constraints: { category: /[^\.]+/, file: /[^\.]+/ } get 'help/shortcuts' get 'help/ui' => 'help#ui' diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index d4915b51952..b1225f101b7 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -64,50 +64,35 @@ describe SnippetsController, "routing" do end end -# help GET /help(.:format) help#index -# help_permissions GET /help/permissions(.:format) help#permissions -# help_workflow GET /help/workflow(.:format) help#workflow -# help_api GET /help/api(.:format) help#api -# help_web_hooks GET /help/web_hooks(.:format) help#web_hooks -# help_system_hooks GET /help/system_hooks(.:format) help#system_hooks -# help_markdown GET /help/markdown(.:format) help#markdown -# help_ssh GET /help/ssh(.:format) help#ssh -# help_raketasks GET /help/raketasks(.:format) help#raketasks +# help GET /help(.:format) help#index +# help_page GET /help/:category/:file(.:format) help#show {:category=>/[^\.]+/, :file=>/[^\.]+/} +# help_shortcuts GET /help/shortcuts(.:format) help#shortcuts +# help_ui GET /help/ui(.:format) help#ui describe HelpController, "routing" do it "to #index" do expect(get("/help")).to route_to('help#index') end - it "to #permissions" do - expect(get("/help/permissions/permissions")).to route_to('help#show', category: "permissions", file: "permissions") + it 'to #show' do + path = '/help/markdown/markdown.md' + expect(get(path)).to route_to('help#show', + category: 'markdown', + file: 'markdown', + format: 'md') + + path = '/help/workflow/protected_branches/protected_branches1.png' + expect(get(path)).to route_to('help#show', + category: 'workflow/protected_branches', + file: 'protected_branches1', + format: 'png') end - it "to #workflow" do - expect(get("/help/workflow/README")).to route_to('help#show', category: "workflow", file: "README") + it 'to #shortcuts' do + expect(get('/help/shortcuts')).to route_to('help#shortcuts') end - it "to #api" do - expect(get("/help/api/README")).to route_to('help#show', category: "api", file: "README") - end - - it "to #web_hooks" do - expect(get("/help/web_hooks/web_hooks")).to route_to('help#show', category: "web_hooks", file: "web_hooks") - end - - it "to #system_hooks" do - expect(get("/help/system_hooks/system_hooks")).to route_to('help#show', category: "system_hooks", file: "system_hooks") - end - - it "to #markdown" do - expect(get("/help/markdown/markdown")).to route_to('help#show',category: "markdown", file: "markdown") - end - - it "to #ssh" do - expect(get("/help/ssh/README")).to route_to('help#show', category: "ssh", file: "README") - end - - it "to #raketasks" do - expect(get("/help/raketasks/README")).to route_to('help#show', category: "raketasks", file: "README") + it 'to #ui' do + expect(get('/help/ui')).to route_to('help#ui') end end From 3052e894207303bf9fed972aa60d3a655a6c58d9 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 15 Apr 2015 12:45:31 -0400 Subject: [PATCH 4/5] Re-fix image rendering for help pages --- app/controllers/help_controller.rb | 42 +++++++++++++--- app/views/help/show.html.haml | 2 +- spec/controllers/help_controller_spec.rb | 61 ++++++++++++++++++++++++ spec/features/help_pages_spec.rb | 2 +- 4 files changed, 99 insertions(+), 8 deletions(-) create mode 100644 spec/controllers/help_controller_spec.rb diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb index 964f624d6d7..10094d86dfb 100644 --- a/app/controllers/help_controller.rb +++ b/app/controllers/help_controller.rb @@ -3,13 +3,36 @@ class HelpController < ApplicationController end def show - @category = clean_path_info(params[:category]) - @file = clean_path_info(params[:file]) + category = clean_path_info(path_params[:category]) + file = clean_path_info(path_params[:file]) - if File.exists?(Rails.root.join('doc', @category, @file + '.md')) - render 'show' - else - not_found! + respond_to do |format| + format.any(:markdown, :md, :html) do + path = Rails.root.join('doc', category, "#{file}.md") + + if File.exist?(path) + @markdown = File.read(path) + + render 'show.html.haml' + else + # Force template to Haml + render 'errors/not_found.html.haml', layout: 'errors', status: 404 + end + end + + # Allow access to images in the doc folder + format.any(:png, :gif, :jpeg) do + path = Rails.root.join('doc', category, "#{file}.#{params[:format]}") + + if File.exist?(path) + send_file(path, disposition: 'inline') + else + head :not_found + end + end + + # Any other format we don't recognize, just respond 404 + format.any { head :not_found } end end @@ -21,6 +44,13 @@ class HelpController < ApplicationController private + def path_params + params.require(:category) + params.require(:file) + + params + end + PATH_SEPS = Regexp.union(*[::File::SEPARATOR, ::File::ALT_SEPARATOR].compact) # Taken from ActionDispatch::FileHandler diff --git a/app/views/help/show.html.haml b/app/views/help/show.html.haml index eca34dbff06..cc1be6a717a 100644 --- a/app/views/help/show.html.haml +++ b/app/views/help/show.html.haml @@ -1,2 +1,2 @@ .documentation.wiki - = markdown File.read(Rails.root.join('doc', @category, @file + '.md')).gsub("$your_email", current_user.email) + = markdown @markdown.gsub('$your_email', current_user.email) diff --git a/spec/controllers/help_controller_spec.rb b/spec/controllers/help_controller_spec.rb new file mode 100644 index 00000000000..93535ced7ae --- /dev/null +++ b/spec/controllers/help_controller_spec.rb @@ -0,0 +1,61 @@ +require 'spec_helper' + +describe HelpController do + let(:user) { create(:user) } + + before do + sign_in(user) + end + + describe 'GET #show' do + context 'for Markdown formats' do + context 'when requested file exists' do + before do + get :show, category: 'ssh', file: 'README', format: :md + end + + it 'assigns to @markdown' do + expect(assigns[:markdown]).not_to be_empty + end + + it 'renders HTML' do + expect(response).to render_template('show.html.haml') + expect(response.content_type).to eq 'text/html' + end + end + + context 'when requested file is missing' do + it 'renders not found' do + get :show, category: 'foo', file: 'bar', format: :md + expect(response).to be_not_found + end + end + end + + context 'for image formats' do + context 'when requested file exists' do + it 'renders the raw file' do + get :show, category: 'workflow/protected_branches', + file: 'protected_branches1', format: :png + expect(response).to be_success + expect(response.content_type).to eq 'image/png' + expect(response.headers['Content-Disposition']).to match(/^inline;/) + end + end + + context 'when requested file is missing' do + it 'renders not found' do + get :show, category: 'foo', file: 'bar', format: :png + expect(response).to be_not_found + end + end + end + + context 'for other formats' do + it 'always renders not found' do + get :show, category: 'ssh', file: 'README', format: :foo + expect(response).to be_not_found + end + end + end +end diff --git a/spec/features/help_pages_spec.rb b/spec/features/help_pages_spec.rb index 41088ce8271..8c6b669ce78 100644 --- a/spec/features/help_pages_spec.rb +++ b/spec/features/help_pages_spec.rb @@ -6,7 +6,7 @@ describe 'Help Pages', feature: true do login_as :user end it 'replace the variable $your_email with the email of the user' do - visit help_page_path(category: 'ssh', file: 'README.md') + visit help_page_path('ssh', 'README') expect(page).to have_content("ssh-keygen -t rsa -C \"#{@user.email}\"") end end From 2d4ffce826ef86af6bc69385ded6830297a8467f Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 15 Apr 2015 13:15:18 -0400 Subject: [PATCH 5/5] Loosen help page parameter constraints for category --- app/controllers/help_controller.rb | 2 +- config/routes.rb | 2 +- spec/routing/routing_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb index 10094d86dfb..35ece5b270b 100644 --- a/app/controllers/help_controller.rb +++ b/app/controllers/help_controller.rb @@ -4,7 +4,7 @@ class HelpController < ApplicationController def show category = clean_path_info(path_params[:category]) - file = clean_path_info(path_params[:file]) + file = path_params[:file] respond_to do |format| format.any(:markdown, :md, :html) do diff --git a/config/routes.rb b/config/routes.rb index 8dbe6d80ab7..744a99feded 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -39,7 +39,7 @@ Gitlab::Application.routes.draw do # Help get 'help' => 'help#index' - get 'help/:category/:file' => 'help#show', as: :help_page, constraints: { category: /[^\.]+/, file: /[^\.]+/ } + get 'help/:category/:file' => 'help#show', as: :help_page, constraints: { category: /.*/, file: /[^\/\.]+/ } get 'help/shortcuts' get 'help/ui' => 'help#ui' diff --git a/spec/routing/routing_spec.rb b/spec/routing/routing_spec.rb index b1225f101b7..e219a57c29e 100644 --- a/spec/routing/routing_spec.rb +++ b/spec/routing/routing_spec.rb @@ -65,7 +65,7 @@ describe SnippetsController, "routing" do end # help GET /help(.:format) help#index -# help_page GET /help/:category/:file(.:format) help#show {:category=>/[^\.]+/, :file=>/[^\.]+/} +# help_page GET /help/:category/:file(.:format) help#show {:category=>/.*/, :file=>/[^\/\.]+/} # help_shortcuts GET /help/shortcuts(.:format) help#shortcuts # help_ui GET /help/ui(.:format) help#ui describe HelpController, "routing" do