Commit ed19b9cc authored by Rémy Coutable's avatar Rémy Coutable

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
parents bb6bdbed ab86d27d
...@@ -89,6 +89,7 @@ v 8.10.0 (unreleased) ...@@ -89,6 +89,7 @@ v 8.10.0 (unreleased)
- Allow expanding and collapsing files in diff view (!4990) - Allow expanding and collapsing files in diff view (!4990)
- Collapse large diffs by default (!4990) - Collapse large diffs by default (!4990)
- Fix mentioned users list on diff notes - 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 - 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. - 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. - Check for conflicts with existing Project's wiki path when creating a new project.
......
...@@ -269,7 +269,7 @@ ...@@ -269,7 +269,7 @@
.issuable-header-btn { .issuable-header-btn {
background: $gray-normal; background: $gray-normal;
border: 1px solid $border-gray-normal; border: 1px solid $border-gray-normal;
&:hover { &:hover {
background: $gray-dark; background: $gray-dark;
border: 1px solid $border-gray-dark; border: 1px solid $border-gray-dark;
......
...@@ -30,7 +30,7 @@ class HelpController < ApplicationController ...@@ -30,7 +30,7 @@ class HelpController < ApplicationController
end end
# Allow access to images in the doc folder # 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` # Note: We are purposefully NOT using `Rails.root.join`
path = File.join(Rails.root, 'doc', "#{@path}.#{params[:format]}") path = File.join(Rails.root, 'doc', "#{@path}.#{params[:format]}")
......
class Projects::UploadsController < Projects::ApplicationController class Projects::UploadsController < Projects::ApplicationController
skip_before_action :reject_blocked!, :project, 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] before_action :authorize_upload_file!, only: [:create]
...@@ -24,7 +24,7 @@ class Projects::UploadsController < Projects::ApplicationController ...@@ -24,7 +24,7 @@ class Projects::UploadsController < Projects::ApplicationController
def show def show
return render_404 if uploader.nil? || !uploader.file.exists? 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 send_file uploader.file.path, disposition: disposition
end end
...@@ -49,7 +49,7 @@ class Projects::UploadsController < Projects::ApplicationController ...@@ -49,7 +49,7 @@ class Projects::UploadsController < Projects::ApplicationController
@uploader @uploader
end end
def image? def image_or_video?
uploader && uploader.file.exists? && uploader.image? uploader && uploader.file.exists? && uploader.image_or_video?
end end
end end
...@@ -33,16 +33,15 @@ class FileUploader < CarrierWave::Uploader::Base ...@@ -33,16 +33,15 @@ class FileUploader < CarrierWave::Uploader::Base
end end
def to_h 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("]", "\\]") escaped_filename = filename.gsub("]", "\\]")
markdown = "[#{escaped_filename}](#{self.secure_url})" markdown = "[#{escaped_filename}](#{self.secure_url})"
markdown.prepend("!") if image? markdown.prepend("!") if image_or_video?
{ {
alt: filename, alt: filename,
url: self.secure_url, url: self.secure_url,
is_image: image?,
markdown: markdown markdown: markdown
} }
end end
......
# Extra methods for uploader # Extra methods for uploader
module UploaderHelper 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? def image?
img_ext = %w(png jpg jpeg gif bmp tiff) extension_match?(IMAGE_EXT)
if file.respond_to?(:extension) end
img_ext.include?(file.extension.downcase)
else def video?
# Not all CarrierWave storages respond to :extension extension_match?(VIDEO_EXT)
ext = file.path.split('.').last.downcase end
img_ext.include?(ext)
end def image_or_video?
rescue image? || video?
false 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 end
def file_storage? def file_storage?
......
...@@ -6,5 +6,9 @@ ...@@ -6,5 +6,9 @@
Mime::Type.register_alias "text/plain", :diff Mime::Type.register_alias "text/plain", :diff
Mime::Type.register_alias "text/plain", :patch Mime::Type.register_alias "text/plain", :patch
Mime::Type.register_alias 'text/html', :markdown Mime::Type.register_alias "text/html", :markdown
Mime::Type.register_alias 'text/html', :md 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
...@@ -850,7 +850,6 @@ Parameters: ...@@ -850,7 +850,6 @@ Parameters:
{ {
"alt": "dk", "alt": "dk",
"url": "/uploads/66dbcd21ec5d24ed6ea225176098d52b/dk.png", "url": "/uploads/66dbcd21ec5d24ed6ea225176098d52b/dk.png",
"is_image": true,
"markdown": "![dk](/uploads/66dbcd21ec5d24ed6ea225176098d52b/dk.png)" "markdown": "![dk](/uploads/66dbcd21ec5d24ed6ea225176098d52b/dk.png)"
} }
``` ```
......
...@@ -13,6 +13,7 @@ ...@@ -13,6 +13,7 @@
* [Emoji](#emoji) * [Emoji](#emoji)
* [Special GitLab references](#special-gitlab-references) * [Special GitLab references](#special-gitlab-references)
* [Task Lists](#task-lists) * [Task Lists](#task-lists)
* [Videos](#videos)
**[Standard Markdown](#standard-markdown)** **[Standard Markdown](#standard-markdown)**
...@@ -281,6 +282,20 @@ You can add task lists to issues, merge requests and comments. To create a task ...@@ -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. 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 # Standard Markdown
## Headers ## Headers
......
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
...@@ -7,6 +7,7 @@ module Banzai ...@@ -7,6 +7,7 @@ module Banzai
Filter::SanitizationFilter, Filter::SanitizationFilter,
Filter::UploadLinkFilter, Filter::UploadLinkFilter,
Filter::VideoLinkFilter,
Filter::ImageLinkFilter, Filter::ImageLinkFilter,
Filter::EmojiFilter, Filter::EmojiFilter,
Filter::TableOfContentsFilter, Filter::TableOfContentsFilter,
......
...@@ -14,9 +14,9 @@ describe Projects::UploadsController do ...@@ -14,9 +14,9 @@ describe Projects::UploadsController do
context "without params['file']" do context "without params['file']" do
it "returns an error" do it "returns an error" do
post :create, post :create,
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
project_id: project.to_param, project_id: project.to_param,
format: :json format: :json
expect(response).to have_http_status(422) expect(response).to have_http_status(422)
end end
...@@ -34,23 +34,21 @@ describe Projects::UploadsController do ...@@ -34,23 +34,21 @@ describe Projects::UploadsController do
it 'returns a content with original filename, new link, and correct type.' 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 '\"alt\":\"rails_sample\"'
expect(response.body).to match "\"url\":\"/uploads" expect(response.body).to match "\"url\":\"/uploads"
expect(response.body).to match '\"is_image\":true'
end end
end end
context 'with valid non-image file' do context 'with valid non-image file' do
before do before do
post :create, post :create,
namespace_id: project.namespace.to_param, namespace_id: project.namespace.to_param,
project_id: project.to_param, project_id: project.to_param,
file: txt, file: txt,
format: :json format: :json
end end
it 'returns a content with original filename, new link, and correct type.' do 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 '\"alt\":\"doc_sample.txt\"'
expect(response.body).to match "\"url\":\"/uploads" expect(response.body).to match "\"url\":\"/uploads"
expect(response.body).to match '\"is_image\":false'
end end
end end
end end
......
...@@ -236,6 +236,14 @@ describe 'GitLab Markdown', feature: true do ...@@ -236,6 +236,14 @@ describe 'GitLab Markdown', feature: true do
it 'includes TaskListFilter' do it 'includes TaskListFilter' do
expect(doc).to parse_task_lists expect(doc).to parse_task_lists
end end
it 'includes InlineDiffFilter' do
expect(doc).to parse_inline_diffs
end
it 'includes VideoLinkFilter' do
expect(doc).to parse_video_links
end
end end
context 'wiki pipeline' do context 'wiki pipeline' do
...@@ -293,6 +301,10 @@ describe 'GitLab Markdown', feature: true do ...@@ -293,6 +301,10 @@ describe 'GitLab Markdown', feature: true do
it 'includes InlineDiffFilter' do it 'includes InlineDiffFilter' do
expect(doc).to parse_inline_diffs expect(doc).to parse_inline_diffs
end end
it 'includes VideoLinkFilter' do
expect(doc).to parse_video_links
end
end end
# Fake a `current_user` helper # Fake a `current_user` helper
......
...@@ -256,3 +256,7 @@ However the wrapping tags can not be mixed as such - ...@@ -256,3 +256,7 @@ However the wrapping tags can not be mixed as such -
- [+ additions +} - [+ additions +}
- {- delletions -] - {- delletions -]
- [- delletions -} - [- delletions -}
### Videos
![My Video](/assets/videos/gitlab-demo.mp4)
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
...@@ -11,7 +11,6 @@ describe Gitlab::Email::AttachmentUploader, lib: true do ...@@ -11,7 +11,6 @@ describe Gitlab::Email::AttachmentUploader, lib: true do
link = links.first link = links.first
expect(link).not_to be_nil expect(link).not_to be_nil
expect(link[:is_image]).to be_truthy
expect(link[:alt]).to eq("bricks") expect(link[:alt]).to eq("bricks")
expect(link[:url]).to include("bricks.png") expect(link[:url]).to include("bricks.png")
end end
......
...@@ -115,7 +115,6 @@ describe Gitlab::Email::Receiver, lib: true do ...@@ -115,7 +115,6 @@ describe Gitlab::Email::Receiver, lib: true do
[ [
{ {
url: "uploads/image.png", url: "uploads/image.png",
is_image: true,
alt: "image", alt: "image",
markdown: markdown markdown: markdown
} }
......
...@@ -396,7 +396,6 @@ describe API::API, api: true do ...@@ -396,7 +396,6 @@ describe API::API, api: true do
expect(json_response['alt']).to eq("dk") expect(json_response['alt']).to eq("dk")
expect(json_response['url']).to start_with("/uploads/") expect(json_response['url']).to start_with("/uploads/")
expect(json_response['url']).to end_with("/dk.png") expect(json_response['url']).to end_with("/dk.png")
expect(json_response['is_image']).to eq(true)
end end
end end
......
...@@ -35,8 +35,6 @@ describe Projects::DownloadService, services: true do ...@@ -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(:alt) }
it { expect(@link_to_file).to have_key(:url) } 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[:url]).to match('rails_sample.jpg') }
it { expect(@link_to_file[:alt]).to eq('rails_sample') } it { expect(@link_to_file[:alt]).to eq('rails_sample') }
end end
...@@ -49,8 +47,6 @@ describe Projects::DownloadService, services: true do ...@@ -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(:alt) }
it { expect(@link_to_file).to have_key(:url) } 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[:url]).to match('doc_sample.txt') }
it { expect(@link_to_file[:alt]).to eq('doc_sample.txt') } it { expect(@link_to_file[:alt]).to eq('doc_sample.txt') }
end end
......
...@@ -15,9 +15,7 @@ describe Projects::UploadService, services: true do ...@@ -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(:alt) }
it { expect(@link_to_file).to have_key(:url) } 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).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') } it { expect(@link_to_file[:url]).to match('banana_sample.gif') }
end end
...@@ -31,8 +29,6 @@ describe Projects::UploadService, services: true do ...@@ -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(:alt) }
it { expect(@link_to_file).to have_key(:url) } 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_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') } it { expect(@link_to_file[:url]).to match('dk.png') }
end end
...@@ -44,9 +40,7 @@ describe Projects::UploadService, services: true do ...@@ -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(:alt) }
it { expect(@link_to_file).to have_key(:url) } 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).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') } it { expect(@link_to_file[:url]).to match('rails_sample.jpg') }
end end
...@@ -58,9 +52,7 @@ describe Projects::UploadService, services: true do ...@@ -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(:alt) }
it { expect(@link_to_file).to have_key(:url) } 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).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') } it { expect(@link_to_file[:url]).to match('doc_sample.txt') }
end end
......
...@@ -178,6 +178,17 @@ module MarkdownMatchers ...@@ -178,6 +178,17 @@ module MarkdownMatchers
expect(actual).to have_selector('span.idiff.deletion', count: 2) expect(actual).to have_selector('span.idiff.deletion', count: 2)
end end
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 end
# Monkeypatch the matcher DSL so that we can reduce some noisy duplication for # Monkeypatch the matcher DSL so that we can reduce some noisy duplication for
......
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
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