Merge branch 'rs-issue-2212' into 'master'

Revert and re-fix image rendering in help pages

Closes #2212

See merge request !1765

app/controllers/help_controller.rb

@@ -3,40 +3,54 @@ class HelpController < ApplicationController
   end
 
   def show
-    @filepath = clean_path_info(params[:filepath])
-    @format = params[:format]
+    category = clean_path_info(path_params[:category])
+    file = path_params[:file]
 
     respond_to do |format|
-      format.md { render_doc }
-      format.all { send_file_data }
-    end
-  end
+      format.any(:markdown, :md, :html) do
+        path = Rails.root.join('doc', category, "#{file}.md")
 
-  def shortcuts
-  end
+        if File.exist?(path)
+          @markdown = File.read(path)
 
-  private
+          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
 
-  def render_doc
-    if File.exists?(Rails.root.join('doc', @filepath + '.md'))
-      render 'show.html.haml'
-    else
-      not_found!
+      # Any other format we don't recognize, just respond 404
+      format.any { head :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
+  def shortcuts
   end
 
   def ui
   end
 
+  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

app/views/help/show.html.haml

 .documentation.wiki
-  = markdown File.read(Rails.root.join('doc', @filepath + '.md')).gsub("$your_email", current_user.email)
+  = markdown @markdown.gsub('$your_email', current_user.email)

config/initializers/mime_types.rb

@@ -6,4 +6,5 @@
 
 Mime::Type.register_alias "text/plain", :diff
 Mime::Type.register_alias "text/plain", :patch
-Mime::Type.register_alias 'text/html', :md
+Mime::Type.register_alias 'text/html',  :markdown
+Mime::Type.register_alias 'text/html',  :md

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, constraints: { category: /.*/, file: /[^\/\.]+/ }
   get 'help/shortcuts'
   get 'help/ui'               => 'help#ui'
-  get 'help/:filepath'        => 'help#show', as: :help_page, constraints: { filepath: /[^\.]+/ }
 
   #
   # Global snippets

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

spec/controllers/help_controller_spec.rb

+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

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('ssh', 'README')
       expect(page).to have_content("ssh-keygen -t rsa -C \"#{@user.email}\"")
     end
   end

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
-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')
-  end
-
-  it 'to #workflow' do
-    expect(get('/help/workflow/README')).to route_to('help#show', filepath: 'workflow/README')
-  end
-
-  it 'to #api' do
-    expect(get('/help/api/README')).to route_to('help#show', filepath: 'api/README')
-  end
-
-  it 'to #web_hooks' do
-    expect(get('/help/web_hooks/web_hooks')).to route_to('help#show', filepath: 'web_hooks/web_hooks')
+#            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 #system_hooks' do
-    expect(get('/help/system_hooks/system_hooks')).to route_to('help#show', filepath: 'system_hooks/system_hooks')
-  end
+  it 'to #show' do
+    path = '/help/markdown/markdown.md'
+    expect(get(path)).to route_to('help#show',
+                                  category: 'markdown',
+                                  file: 'markdown',
+                                  format: 'md')
 
-  it 'to #markdown' do
-    expect(get('/help/markdown/markdown')).to route_to('help#show',filepath: 'markdown/markdown')
+    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 #ssh' do
-    expect(get('/help/ssh/README')).to route_to('help#show', filepath: 'ssh/README')
+  it 'to #shortcuts' do
+    expect(get('/help/shortcuts')).to route_to('help#shortcuts')
   end
 
-  it 'to #raketasks' do
-    expect(get('/help/raketasks/README')).to route_to('help#show', filepath: 'raketasks/README')
+  it 'to #ui' do
+    expect(get('/help/ui')).to route_to('help#ui')
   end
 end