Merge branch '4142-show-inline-video' into 'master'

Add support for inline videos in issue, MR and notes (on issue, commit, MR, and MR diff)

## What does this MR do?

It adds support for inline videos in issue, MR and notes (on issue, commit, MR, and MR diff). Most of the work was done by @hayesr in !3508 but a few improvements were still missing.

## Why was this MR needed?

To be able to play uploaded videos in GitLab!

## What are the relevant issue numbers?

Closes #4142.

## Screenshots

### Video players

![Screen_Shot_2016-07-19_at_18.44.09](/uploads/e85e531b455a41c3e66b26b356abaafd/Screen_Shot_2016-07-19_at_18.44.09.png)

-----

![Screen_Shot_2016-07-19_at_18.44.29](/uploads/05f52a812760210d1eae86a7f8fc48bc/Screen_Shot_2016-07-19_at_18.44.29.png)

-----

## Does this MR meet the acceptance criteria?

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG) entry added
- [x] [Documentation created/updated](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/doc/development/doc_styleguide.md)
- Tests
  - [x] Test `VideoLinkFilter`
  - [x] Test in `spec/features/markdown_spec.rb`
  - [x] Improve `spec/uploaders/file_uploader_spec.rb`
  - [x] All builds are passing
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if you do - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

See merge request !5215

CHANGELOG

@@ -79,6 +79,7 @@ v 8.10.0 (unreleased)
   - Allow expanding and collapsing files in diff view (!4990)
   - Collapse large diffs by default (!4990)
   - Fix mentioned users list on diff notes
+  - Add support for inline videos in GitLab Flavored Markdown. !5215 (original implementation by Eric Hayes)
   - Fix creation of deployment on build that is retried, redeployed or rollback
   - Don't parse Rinku returned value to DocFragment when it didn't change the original html string.
   - Check for conflicts with existing Project's wiki path when creating a new project.

app/assets/stylesheets/pages/issuable.scss

@@ -269,7 +269,7 @@
   .issuable-header-btn {
     background: $gray-normal;
     border: 1px solid $border-gray-normal;
-    
+
     &:hover {
       background: $gray-dark;
       border: 1px solid $border-gray-dark;

app/controllers/help_controller.rb

@@ -30,7 +30,7 @@ class HelpController < ApplicationController
       end
 
       # Allow access to images in the doc folder
-      format.any(:png, :gif, :jpeg) do
+      format.any(:png, :gif, :jpeg, :mp4) do
         # Note: We are purposefully NOT using `Rails.root.join`
         path = File.join(Rails.root, 'doc', "#{@path}.#{params[:format]}")
 

app/controllers/projects/uploads_controller.rb

 class Projects::UploadsController < Projects::ApplicationController
   skip_before_action :reject_blocked!, :project,
-    :repository, if: -> { action_name == 'show' && image? }
+    :repository, if: -> { action_name == 'show' && image_or_video? }
 
   before_action :authorize_upload_file!, only: [:create]
 
@@ -24,7 +24,7 @@ class Projects::UploadsController < Projects::ApplicationController
   def show
     return render_404 if uploader.nil? || !uploader.file.exists?
 
-    disposition = uploader.image? ? 'inline' : 'attachment'
+    disposition = uploader.image_or_video? ? 'inline' : 'attachment'
     send_file uploader.file.path, disposition: disposition
   end
 
@@ -49,7 +49,7 @@ class Projects::UploadsController < Projects::ApplicationController
     @uploader
   end
 
-  def image?
-    uploader && uploader.file.exists? && uploader.image?
+  def image_or_video?
+    uploader && uploader.file.exists? && uploader.image_or_video?
   end
 end

app/uploaders/file_uploader.rb

@@ -33,16 +33,15 @@ class FileUploader < CarrierWave::Uploader::Base
   end
 
   def to_h
-    filename = image? ? self.file.basename : self.file.filename
+    filename = image_or_video? ? self.file.basename : self.file.filename
     escaped_filename = filename.gsub("]", "\\]")
 
     markdown = "[#{escaped_filename}](#{self.secure_url})"
-    markdown.prepend("!") if image?
+    markdown.prepend("!") if image_or_video?
 
     {
       alt:      filename,
       url:      self.secure_url,
-      is_image: image?,
       markdown: markdown
     }
   end

app/uploaders/uploader_helper.rb

 # Extra methods for uploader
 module UploaderHelper
+  IMAGE_EXT = %w[png jpg jpeg gif bmp tiff]
+  # We recommend using the .mp4 format over .mov. Videos in .mov format can
+  # still be used but you really need to make sure they are served with the
+  # proper MIME type video/mp4 and not video/quicktime or your videos won't play
+  # on IE >= 9.
+  # http://archive.sublimevideo.info/20150912/docs.sublimevideo.net/troubleshooting.html
+  VIDEO_EXT = %w[mp4 m4v mov webm ogv]
+
   def image?
-    img_ext = %w(png jpg jpeg gif bmp tiff)
-    if file.respond_to?(:extension)
-      img_ext.include?(file.extension.downcase)
-    else
-      # Not all CarrierWave storages respond to :extension
-      ext = file.path.split('.').last.downcase
-      img_ext.include?(ext)
-    end
-  rescue
-    false
+    extension_match?(IMAGE_EXT)
+  end
+
+  def video?
+    extension_match?(VIDEO_EXT)
+  end
+
+  def image_or_video?
+    image? || video?
+  end
+
+  def extension_match?(extensions)
+    return false unless file
+
+    extension =
+      if file.respond_to?(:extension)
+        file.extension
+      else
+        # Not all CarrierWave storages respond to :extension
+        File.extname(file.path).delete('.')
+      end
+
+    extensions.include?(extension.downcase)
   end
 
   def file_storage?

config/initializers/mime_types.rb

@@ -6,5 +6,9 @@
 
 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
+Mime::Type.register_alias "text/html",  :markdown
+Mime::Type.register_alias "text/html",  :md
+
+Mime::Type.register "video/mp4",  :mp4, [], [:m4v, :mov]
+Mime::Type.register "video/webm", :webm
+Mime::Type.register "video/ogg",  :ogv

doc/api/projects.md

@@ -850,7 +850,6 @@ Parameters:
 {
   "alt": "dk",
   "url": "/uploads/66dbcd21ec5d24ed6ea225176098d52b/dk.png",
-  "is_image": true,
   "markdown": "![dk](/uploads/66dbcd21ec5d24ed6ea225176098d52b/dk.png)"
 }
 ```

doc/markdown/markdown.md

@@ -13,6 +13,7 @@
 * [Emoji](#emoji)
 * [Special GitLab references](#special-gitlab-references)
 * [Task Lists](#task-lists)
+* [Videos](#videos)
 
 **[Standard Markdown](#standard-markdown)**
 
@@ -281,6 +282,20 @@ You can add task lists to issues, merge requests and comments. To create a task
 
 Task lists can only be created in descriptions, not in titles. Task item state can be managed by editing the description's Markdown or by toggling the rendered check boxes.
 
+## Videos
+
+Image tags with a video extension are automatically converted to a video player.
+
+The valid video extensions are `.mp4`, `.m4v`, `.mov`, `.webm`, and `.ogv`.
+
+    Here's a sample video:
+
+    ![Sample Video](img/video.mp4)
+
+Here's a sample video:
+
+![Sample Video](img/video.mp4)
+
 # Standard Markdown
 
 ## Headers

lib/banzai/filter/video_link_filter.rb

+module Banzai
+  module Filter
+
+    # Find every image that isn't already wrapped in an `a` tag, and that has
+    # a `src` attribute ending with a video extension, add a new video node and
+    # a "Download" link in the case the video cannot be played.
+    class VideoLinkFilter < HTML::Pipeline::Filter
+
+      def call
+        doc.xpath(query).each do |el|
+          el.replace(video_node(doc, el))
+        end
+
+        doc
+      end
+
+      private
+
+      def query
+        @query ||= begin
+          src_query = UploaderHelper::VIDEO_EXT.map do |ext|
+            "'.#{ext}' = substring(@src, string-length(@src) - #{ext.size})"
+          end
+
+          "descendant-or-self::img[not(ancestor::a) and (#{src_query.join(' or ')})]"
+        end
+      end
+
+      def video_node(doc, element)
+        container = doc.document.create_element(
+          'div',
+          class: 'video-container'
+        )
+
+        video = doc.document.create_element(
+          'video',
+          src: element['src'],
+          width: '400',
+          controls: true,
+          'data-setup' => '{}')
+
+        link = doc.document.create_element(
+          'a',
+          element['title'] || element['alt'],
+          href: element['src'],
+          target: '_blank',
+          title: "Download '#{element['title'] || element['alt']}'")
+        download_paragraph = doc.document.create_element('p')
+        download_paragraph.children = link
+
+        container.add_child(video)
+        container.add_child(download_paragraph)
+
+        container
+      end
+    end
+
+  end
+end

lib/banzai/pipeline/gfm_pipeline.rb

@@ -7,6 +7,7 @@ module Banzai
           Filter::SanitizationFilter,
 
           Filter::UploadLinkFilter,
+          Filter::VideoLinkFilter,
           Filter::ImageLinkFilter,
           Filter::EmojiFilter,
           Filter::TableOfContentsFilter,

spec/controllers/projects/uploads_controller_spec.rb

@@ -14,9 +14,9 @@ describe Projects::UploadsController do
 
     context "without params['file']" do
       it "returns an error" do
-        post :create, 
+        post :create,
           namespace_id: project.namespace.to_param,
-          project_id: project.to_param, 
+          project_id: project.to_param,
           format: :json
         expect(response).to have_http_status(422)
       end
@@ -34,23 +34,21 @@ describe Projects::UploadsController do
       it 'returns a content with original filename, new link, and correct type.' do
         expect(response.body).to match '\"alt\":\"rails_sample\"'
         expect(response.body).to match "\"url\":\"/uploads"
-        expect(response.body).to match '\"is_image\":true'
       end
     end
 
     context 'with valid non-image file' do
       before do
-        post :create, 
+        post :create,
           namespace_id: project.namespace.to_param,
-          project_id: project.to_param, 
-          file: txt, 
+          project_id: project.to_param,
+          file: txt,
           format: :json
       end
 
       it 'returns a content with original filename, new link, and correct type.' do
         expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
         expect(response.body).to match "\"url\":\"/uploads"
-        expect(response.body).to match '\"is_image\":false'
       end
     end
   end

spec/features/markdown_spec.rb

@@ -236,6 +236,14 @@ describe 'GitLab Markdown', feature: true do
     it 'includes TaskListFilter' do
       expect(doc).to parse_task_lists
     end
+
+    it 'includes InlineDiffFilter' do
+      expect(doc).to parse_inline_diffs
+    end
+
+    it 'includes VideoLinkFilter' do
+      expect(doc).to parse_video_links
+    end
   end
 
   context 'wiki pipeline' do
@@ -293,6 +301,10 @@ describe 'GitLab Markdown', feature: true do
     it 'includes InlineDiffFilter' do
       expect(doc).to parse_inline_diffs
     end
+
+    it 'includes VideoLinkFilter' do
+      expect(doc).to parse_video_links
+    end
   end
 
   # Fake a `current_user` helper

spec/fixtures/markdown.md.erb

@@ -256,3 +256,7 @@ However the wrapping tags can not be mixed as such -
 - [+ additions +}
 - {- delletions -]
 - [- delletions -}
+
+### Videos
+
+![My Video](/assets/videos/gitlab-demo.mp4)

spec/lib/banzai/filter/video_link_filter_spec.rb

+require 'spec_helper'
+
+describe Banzai::Filter::VideoLinkFilter, lib: true do
+  def filter(doc, contexts = {})
+    contexts.reverse_merge!({
+      project: project
+    })
+
+    described_class.call(doc, contexts)
+  end
+
+  def link_to_image(path)
+    %(<img src="#{path}" />)
+  end
+
+  let(:project) { create(:project) }
+
+  context 'when the element src has a video extension' do
+    UploaderHelper::VIDEO_EXT.each do |ext|
+      it "replaces the image tag 'path/video.#{ext}' with a video tag" do
+        container = filter(link_to_image("/path/video.#{ext}")).children.first
+
+        expect(container.name).to eq 'div'
+        expect(container['class']).to eq 'video-container'
+
+        video, paragraph = container.children
+
+        expect(video.name).to eq 'video'
+        expect(video['src']).to eq "/path/video.#{ext}"
+
+        expect(paragraph.name).to eq 'p'
+
+        link = paragraph.children.first
+
+        expect(link.name).to eq 'a'
+        expect(link['href']).to eq "/path/video.#{ext}"
+        expect(link['target']).to eq '_blank'
+      end
+    end
+  end
+
+  context 'when the element src is an image' do
+    it 'leaves the document unchanged' do
+      element = filter(link_to_image('/path/my_image.jpg')).children.first
+
+      expect(element.name).to eq 'img'
+      expect(element['src']).to eq '/path/my_image.jpg'
+    end
+  end
+
+end

spec/lib/gitlab/email/attachment_uploader_spec.rb

@@ -11,7 +11,6 @@ describe Gitlab::Email::AttachmentUploader, lib: true do
       link = links.first
 
       expect(link).not_to be_nil
-      expect(link[:is_image]).to be_truthy
       expect(link[:alt]).to eq("bricks")
       expect(link[:url]).to include("bricks.png")
     end

spec/lib/gitlab/email/receiver_spec.rb

@@ -115,7 +115,6 @@ describe Gitlab::Email::Receiver, lib: true do
         [
           {
             url: "uploads/image.png",
-            is_image: true,
             alt: "image",
             markdown: markdown
           }

spec/requests/api/projects_spec.rb

@@ -396,7 +396,6 @@ describe API::API, api: true  do
       expect(json_response['alt']).to eq("dk")
       expect(json_response['url']).to start_with("/uploads/")
       expect(json_response['url']).to end_with("/dk.png")
-      expect(json_response['is_image']).to eq(true)
     end
   end
 

spec/services/projects/download_service_spec.rb

@@ -35,8 +35,6 @@ describe Projects::DownloadService, services: true do
 
         it { expect(@link_to_file).to have_key(:alt) }
         it { expect(@link_to_file).to have_key(:url) }
-        it { expect(@link_to_file).to have_key(:is_image) }
-        it { expect(@link_to_file[:is_image]).to be true }
         it { expect(@link_to_file[:url]).to match('rails_sample.jpg') }
         it { expect(@link_to_file[:alt]).to eq('rails_sample') }
       end
@@ -49,8 +47,6 @@ describe Projects::DownloadService, services: true do
 
         it { expect(@link_to_file).to have_key(:alt) }
         it { expect(@link_to_file).to have_key(:url) }
-        it { expect(@link_to_file).to have_key(:is_image) }
-        it { expect(@link_to_file[:is_image]).to be false }
         it { expect(@link_to_file[:url]).to match('doc_sample.txt') }
         it { expect(@link_to_file[:alt]).to eq('doc_sample.txt') }
       end

spec/services/projects/upload_service_spec.rb

@@ -15,9 +15,7 @@ describe Projects::UploadService, services: true do
 
       it { expect(@link_to_file).to have_key(:alt) }
       it { expect(@link_to_file).to have_key(:url) }
-      it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('banana_sample') }
-      it { expect(@link_to_file[:is_image]).to equal(true) }
       it { expect(@link_to_file[:url]).to match('banana_sample.gif') }
     end
 
@@ -31,8 +29,6 @@ describe Projects::UploadService, services: true do
       it { expect(@link_to_file).to have_key(:alt) }
       it { expect(@link_to_file).to have_key(:url) }
       it { expect(@link_to_file).to have_value('dk') }
-      it { expect(@link_to_file).to have_key(:is_image) }
-      it { expect(@link_to_file[:is_image]).to equal(true) }
       it { expect(@link_to_file[:url]).to match('dk.png') }
     end
 
@@ -44,9 +40,7 @@ describe Projects::UploadService, services: true do
 
       it { expect(@link_to_file).to have_key(:alt) }
       it { expect(@link_to_file).to have_key(:url) }
-      it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('rails_sample') }
-      it { expect(@link_to_file[:is_image]).to equal(true) }
       it { expect(@link_to_file[:url]).to match('rails_sample.jpg') }
     end
 
@@ -58,9 +52,7 @@ describe Projects::UploadService, services: true do
 
       it { expect(@link_to_file).to have_key(:alt) }
       it { expect(@link_to_file).to have_key(:url) }
-      it { expect(@link_to_file).to have_key(:is_image) }
       it { expect(@link_to_file).to have_value('doc_sample.txt') }
-      it { expect(@link_to_file[:is_image]).to equal(false) }
       it { expect(@link_to_file[:url]).to match('doc_sample.txt') }
     end
 

spec/support/matchers/markdown_matchers.rb

@@ -178,6 +178,17 @@ module MarkdownMatchers
       expect(actual).to have_selector('span.idiff.deletion', count: 2)
     end
   end
+
+  # VideoLinkFilter
+  matcher :parse_video_links do
+    set_default_markdown_messages
+
+    match do |actual|
+      video = actual.at_css('video')
+
+      expect(video['src']).to end_with('/assets/videos/gitlab-demo.mp4')
+    end
+  end
 end
 
 # Monkeypatch the matcher DSL so that we can reduce some noisy duplication for

spec/uploaders/file_uploader_spec.rb

+require 'spec_helper'
+
+describe FileUploader do
+  let(:project) { create(:project) }
+
+  before do
+    @previous_enable_processing = FileUploader.enable_processing
+    FileUploader.enable_processing = false
+    @uploader = FileUploader.new(project)
+  end
+
+  after do
+    FileUploader.enable_processing = @previous_enable_processing
+    @uploader.remove!
+  end
+
+  describe '#image_or_video?' do
+    context 'given an image file' do
+      before do
+        @uploader.store!(File.new(Rails.root.join('spec', 'fixtures', 'rails_sample.jpg')))
+      end
+
+      it 'detects an image based on file extension' do
+        expect(@uploader.image_or_video?).to be true
+      end
+    end
+
+    context 'given an video file' do
+      before do
+        video_file = File.new(Rails.root.join('spec', 'fixtures', 'video_sample.mp4'))
+        @uploader.store!(video_file)
+      end
+
+      it 'detects a video based on file extension' do
+        expect(@uploader.image_or_video?).to be true
+      end
+    end
+
+    it 'does not return image_or_video? for other types' do
+      @uploader.store!(File.new(Rails.root.join('spec', 'fixtures', 'doc_sample.txt')))
+
+      expect(@uploader.image_or_video?).to be false
+    end
+  end
+end