diff options
| author | Rémy Coutable <remy@rymai.me> | 2016-07-21 07:29:38 +0000 |
|---|---|---|
| committer | Rémy Coutable <remy@rymai.me> | 2016-07-21 07:29:38 +0000 |
| commit | ed19b9cc4302572cff435fa4e78eb6dd89d436c6 (patch) | |
| tree | 914721665618a394143a3ee82957691ccfecde9b | |
| parent | bb6bdbed910a21a45f04f8ba807335b2cf7ab992 (diff) | |
| parent | ab86d27d3390b0388bbd92f123adf71321faa492 (diff) | |
| download | gitlab-ce-ed19b9cc4302572cff435fa4e78eb6dd89d436c6.tar.gz | |
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
-----
-----
## 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
24 files changed, 249 insertions, 44 deletions
diff --git a/CHANGELOG b/CHANGELOG index 6d373a05276..edbf1d8f06e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -89,6 +89,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. diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index ded437625ee..7a50bc9c832 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/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; diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb index d3dd98c8a4e..f7b44099b78 100644 --- a/app/controllers/help_controller.rb +++ b/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]}") diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb index caed064dfbc..e617be8f9fb 100644 --- a/app/controllers/projects/uploads_controller.rb +++ b/app/controllers/projects/uploads_controller.rb @@ -1,6 +1,6 @@ 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 diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb index 1af9e9b0edb..2f5f49f7de7 100644 --- a/app/uploaders/file_uploader.rb +++ b/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 diff --git a/app/uploaders/uploader_helper.rb b/app/uploaders/uploader_helper.rb index 5ef440f3367..b10ad71d052 100644 --- a/app/uploaders/uploader_helper.rb +++ b/app/uploaders/uploader_helper.rb @@ -1,16 +1,37 @@ # 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? diff --git a/config/initializers/mime_types.rb b/config/initializers/mime_types.rb index ca58ae92d1b..3e553120205 100644 --- a/config/initializers/mime_types.rb +++ b/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 diff --git a/doc/api/projects.md b/doc/api/projects.md index dceee7b4ea7..0ba0bffb4ac 100644 --- a/doc/api/projects.md +++ b/doc/api/projects.md @@ -850,7 +850,6 @@ Parameters: { "alt": "dk", "url": "/uploads/66dbcd21ec5d24ed6ea225176098d52b/dk.png", - "is_image": true, "markdown": "" } ``` diff --git a/doc/markdown/img/video.mp4 b/doc/markdown/img/video.mp4 Binary files differnew file mode 100644 index 00000000000..1fc478842f5 --- /dev/null +++ b/doc/markdown/img/video.mp4 diff --git a/doc/markdown/markdown.md b/doc/markdown/markdown.md index fb2dd582754..2bbe4b2a36e 100644 --- a/doc/markdown/markdown.md +++ b/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: + +  + +Here's a sample video: + + + # Standard Markdown ## Headers diff --git a/lib/banzai/filter/video_link_filter.rb b/lib/banzai/filter/video_link_filter.rb new file mode 100644 index 00000000000..fd8b9a6f0cc --- /dev/null +++ b/lib/banzai/filter/video_link_filter.rb @@ -0,0 +1,59 @@ +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 diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb index b27ecf3c923..8d94b199c66 100644 --- a/lib/banzai/pipeline/gfm_pipeline.rb +++ b/lib/banzai/pipeline/gfm_pipeline.rb @@ -7,6 +7,7 @@ module Banzai Filter::SanitizationFilter, Filter::UploadLinkFilter, + Filter::VideoLinkFilter, Filter::ImageLinkFilter, Filter::EmojiFilter, Filter::TableOfContentsFilter, diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb index 0893ee89f6a..71d0e4be834 100644 --- a/spec/controllers/projects/uploads_controller_spec.rb +++ b/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 diff --git a/spec/features/markdown_spec.rb b/spec/features/markdown_spec.rb index 09ccc77c101..32159559c37 100644 --- a/spec/features/markdown_spec.rb +++ b/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 diff --git a/spec/fixtures/markdown.md.erb b/spec/fixtures/markdown.md.erb index c75d28d9801..f3e7c2d1a9f 100644 --- a/spec/fixtures/markdown.md.erb +++ b/spec/fixtures/markdown.md.erb @@ -256,3 +256,7 @@ However the wrapping tags can not be mixed as such - - [+ additions +} - {- delletions -] - [- delletions -} + +### Videos + + diff --git a/spec/fixtures/video_sample.mp4 b/spec/fixtures/video_sample.mp4 Binary files differnew file mode 100644 index 00000000000..acd45190998 --- /dev/null +++ b/spec/fixtures/video_sample.mp4 diff --git a/spec/lib/banzai/filter/video_link_filter_spec.rb b/spec/lib/banzai/filter/video_link_filter_spec.rb new file mode 100644 index 00000000000..cc4349f80ba --- /dev/null +++ b/spec/lib/banzai/filter/video_link_filter_spec.rb @@ -0,0 +1,51 @@ +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 diff --git a/spec/lib/gitlab/email/attachment_uploader_spec.rb b/spec/lib/gitlab/email/attachment_uploader_spec.rb index 476a21bf996..08b2577ecc4 100644 --- a/spec/lib/gitlab/email/attachment_uploader_spec.rb +++ b/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 diff --git a/spec/lib/gitlab/email/receiver_spec.rb b/spec/lib/gitlab/email/receiver_spec.rb index 36267faeb93..84d2584a791 100644 --- a/spec/lib/gitlab/email/receiver_spec.rb +++ b/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 } diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index afa0599807f..8c6a7e6529d 100644 --- a/spec/requests/api/projects_spec.rb +++ b/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 diff --git a/spec/services/projects/download_service_spec.rb b/spec/services/projects/download_service_spec.rb index f252e2c5902..122a7cea2a1 100644 --- a/spec/services/projects/download_service_spec.rb +++ b/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 diff --git a/spec/services/projects/upload_service_spec.rb b/spec/services/projects/upload_service_spec.rb index 9268a9fb1a2..c42eeba4b9c 100644 --- a/spec/services/projects/upload_service_spec.rb +++ b/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 diff --git a/spec/support/matchers/markdown_matchers.rb b/spec/support/matchers/markdown_matchers.rb index e005058ba5b..8c98b1f988c 100644 --- a/spec/support/matchers/markdown_matchers.rb +++ b/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 diff --git a/spec/uploaders/file_uploader_spec.rb b/spec/uploaders/file_uploader_spec.rb new file mode 100644 index 00000000000..e8300abed5d --- /dev/null +++ b/spec/uploaders/file_uploader_spec.rb @@ -0,0 +1,45 @@ +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 |