From 82adef2c5a6edbfa43407d69b434c907436498d0 Mon Sep 17 00:00:00 2001 From: dixpac Date: Sat, 15 Apr 2017 13:43:00 +0200 Subject: Increase line-height in build-header so elements don't overlap --- app/assets/stylesheets/pages/builds.scss | 1 + changelogs/unreleased/fix_build_header_line_height.yml | 4 ++++ 2 files changed, 5 insertions(+) create mode 100644 changelogs/unreleased/fix_build_header_line_height.yml diff --git a/app/assets/stylesheets/pages/builds.scss b/app/assets/stylesheets/pages/builds.scss index 411f1c4442b..724b4080ee0 100644 --- a/app/assets/stylesheets/pages/builds.scss +++ b/app/assets/stylesheets/pages/builds.scss @@ -200,6 +200,7 @@ .header-content { flex: 1; + line-height: 1.8; a { color: $gl-text-color; diff --git a/changelogs/unreleased/fix_build_header_line_height.yml b/changelogs/unreleased/fix_build_header_line_height.yml new file mode 100644 index 00000000000..95b6221f8d2 --- /dev/null +++ b/changelogs/unreleased/fix_build_header_line_height.yml @@ -0,0 +1,4 @@ +--- +title: Change line-height on build-header so elements don't overlap +merge_request: +author: Dino Maric -- cgit v1.2.1 From aa364386d2c08298cc4a201b4a7d2f779ff6fbb6 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Wed, 26 Apr 2017 09:35:41 +0100 Subject: Correct the resource counted for MR collapsed info --- app/views/shared/milestones/_sidebar.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/milestones/_sidebar.html.haml b/app/views/shared/milestones/_sidebar.html.haml index ccc808ff43e..774d20fb5ba 100644 --- a/app/views/shared/milestones/_sidebar.html.haml +++ b/app/views/shared/milestones/_sidebar.html.haml @@ -89,7 +89,7 @@ .sidebar-collapsed-icon %strong = icon('exclamation', 'aria-hidden': 'true') - %span= milestone.issues_visible_to_user(current_user).count + %span= milestone.merge_requests.count .title.hide-collapsed Merge requests %span.badge= milestone.merge_requests.count -- cgit v1.2.1 From 67974f1dfb413452ca6fc048c856360f4a843eb1 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 25 Apr 2017 16:47:57 +0100 Subject: remove invalid services --- app/models/service.rb | 1 + spec/models/service_spec.rb | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/app/models/service.rb b/app/models/service.rb index dc76bf925d3..cbb75186206 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -26,6 +26,7 @@ class Service < ActiveRecord::Base has_one :service_hook validates :project_id, presence: true, unless: proc { |service| service.template? } + validates :type, presence: true scope :visible, -> { where.not(type: 'GitlabIssueTrackerService') } scope :issue_trackers, -> { where(category: 'issue_tracker') } diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 0e2f07e945f..f5ba8f76f40 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -6,6 +6,10 @@ describe Service, models: true do it { is_expected.to have_one :service_hook } end + describe 'Validations' do + it { is_expected.to validate_presence_of(:type).on(:create) } + end + describe "Test Button" do before do @service = Service.new -- cgit v1.2.1 From 27317d55b0a2ad7d18b324fa30496f0acd07f211 Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Wed, 26 Apr 2017 11:02:32 +0100 Subject: Added test --- .../features/projects/milestones/milestone_spec.rb | 24 ++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/spec/features/projects/milestones/milestone_spec.rb b/spec/features/projects/milestones/milestone_spec.rb index dab78fd3571..5e19907eef9 100644 --- a/spec/features/projects/milestones/milestone_spec.rb +++ b/spec/features/projects/milestones/milestone_spec.rb @@ -63,4 +63,28 @@ feature 'Project milestone', :feature do expect(page).not_to have_content('Assign some issues to this milestone.') end end + + context 'when project has an issue' do + before do + create(:issue, project: project, milestone: milestone) + + visit namespace_project_milestone_path(project.namespace, project, milestone) + end + + describe 'the collapsed sidebar' do + before do + find('.milestone-sidebar .gutter-toggle').click + end + + it 'shows the total MR and issue counts' do + find('.milestone-sidebar .block', match: :first) + blocks = all('.milestone-sidebar .block') + + aggregate_failures 'MR and issue blocks' do + expect(blocks[3]).to have_content 1 + expect(blocks[4]).to have_content 0 + end + end + end + end end -- cgit v1.2.1 From b45b08cbbfeabdd0a7d0f1c03a2584e58ebe684b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 26 Apr 2017 17:37:01 +0200 Subject: Run `bundle check` after `bundle install` instead of having a dedicated job for it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- .gitlab-ci.yml | 7 ------- scripts/prepare_build.sh | 2 +- 2 files changed, 1 insertion(+), 8 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 4a16a0aaba0..5ea1d89e659 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -402,13 +402,6 @@ docs:check:links: # Check the internal links - bundle exec nanoc check internal_links -bundler:check: - stage: test - <<: *dedicated-runner - <<: *ruby-static-analysis - script: - - bundle check - bundler:audit: stage: test <<: *ruby-static-analysis diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index de7379425cf..fd173c0ba88 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -32,7 +32,7 @@ sed -i 's/localhost/redis/g' config/resque.yml cp config/gitlab.yml.example config/gitlab.yml if [ "$USE_BUNDLE_INSTALL" != "false" ]; then - retry bundle install --clean $BUNDLE_INSTALL_FLAGS + retry bundle install --clean $BUNDLE_INSTALL_FLAGS && bundle check fi # Only install knapsack after bundle install! Otherwise oddly some native -- cgit v1.2.1 From b9e573db5707a8921699a3035d73d5b621cbcdcd Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 26 Apr 2017 14:13:04 -0500 Subject: Skip validation when creating internal (ghost, service desk) users --- app/models/user.rb | 4 +++- changelogs/unreleased/dm-fix-ghost-user-validation.yml | 4 ++++ spec/models/user_spec.rb | 10 ++++++++++ 3 files changed, 17 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/dm-fix-ghost-user-validation.yml diff --git a/app/models/user.rb b/app/models/user.rb index 774d4caa806..bd9c9f99663 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1068,11 +1068,13 @@ class User < ActiveRecord::Base User.find_by_email(s) end - scope.create( + user = scope.build( username: username, email: email, &creation_block ) + user.save(validate: false) + user ensure Gitlab::ExclusiveLease.cancel(lease_key, uuid) end diff --git a/changelogs/unreleased/dm-fix-ghost-user-validation.yml b/changelogs/unreleased/dm-fix-ghost-user-validation.yml new file mode 100644 index 00000000000..4214786cb5a --- /dev/null +++ b/changelogs/unreleased/dm-fix-ghost-user-validation.yml @@ -0,0 +1,4 @@ +--- +title: Skip validation when creating internal (ghost, service desk) users +merge_request: +author: diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0a2860f2505..0bcebc27598 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -1556,6 +1556,16 @@ describe User, models: true do expect(ghost.email).to eq('ghost1@example.com') end end + + context 'when a domain whitelist is in place' do + before do + stub_application_setting(domain_whitelist: ['gitlab.com']) + end + + it 'creates a ghost user' do + expect(User.ghost).to be_persisted + end + end end describe '#update_two_factor_requirement' do -- cgit v1.2.1 From c7a6c267a61808fb42cf6bfdb61a796d197d10c4 Mon Sep 17 00:00:00 2001 From: blackst0ne Date: Thu, 27 Apr 2017 22:28:53 +1100 Subject: Make markdown tables thinner --- app/assets/stylesheets/framework/markdown_area.scss | 4 ++++ app/assets/stylesheets/framework/mixins.scss | 7 +++++++ app/assets/stylesheets/pages/notes.scss | 4 ++++ app/assets/stylesheets/pages/wiki.scss | 6 ++++++ changelogs/unreleased/make_markdown_tables_thinner.yml | 4 ++++ 5 files changed, 25 insertions(+) create mode 100644 changelogs/unreleased/make_markdown_tables_thinner.yml diff --git a/app/assets/stylesheets/framework/markdown_area.scss b/app/assets/stylesheets/framework/markdown_area.scss index a668a6c4c39..80691a234f8 100644 --- a/app/assets/stylesheets/framework/markdown_area.scss +++ b/app/assets/stylesheets/framework/markdown_area.scss @@ -120,6 +120,10 @@ // Ensure that image does not exceed viewport max-height: calc(100vh - 100px); } + + table { + @include markdown-table; + } } .toolbar-group { diff --git a/app/assets/stylesheets/framework/mixins.scss b/app/assets/stylesheets/framework/mixins.scss index b3340d41333..3a98332e46c 100644 --- a/app/assets/stylesheets/framework/mixins.scss +++ b/app/assets/stylesheets/framework/mixins.scss @@ -12,6 +12,13 @@ max-width: $max_width; } +/* + * Mixin for markdown tables + */ +@mixin markdown-table { + width: auto; +} + /* * Base mixin for lists in GitLab */ diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss index 98b93cca6e3..6cea9ff0e7a 100644 --- a/app/assets/stylesheets/pages/notes.scss +++ b/app/assets/stylesheets/pages/notes.scss @@ -97,6 +97,10 @@ ul.notes { padding-left: 1.3em; } } + + table { + @include markdown-table; + } } } diff --git a/app/assets/stylesheets/pages/wiki.scss b/app/assets/stylesheets/pages/wiki.scss index 9bc47bbe173..04ff2d52b91 100644 --- a/app/assets/stylesheets/pages/wiki.scss +++ b/app/assets/stylesheets/pages/wiki.scss @@ -159,3 +159,9 @@ ul.wiki-pages-list.content-list { padding: 5px 0; } } + +.wiki { + table { + @include markdown-table; + } +} diff --git a/changelogs/unreleased/make_markdown_tables_thinner.yml b/changelogs/unreleased/make_markdown_tables_thinner.yml new file mode 100644 index 00000000000..d03a26bdeb3 --- /dev/null +++ b/changelogs/unreleased/make_markdown_tables_thinner.yml @@ -0,0 +1,4 @@ +--- +title: Make markdown tables thinner +merge_request: 10909 +author: blackst0ne -- cgit v1.2.1 From a65224d327a50c2310c4c219667804adf1bb0bc2 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 27 Apr 2017 12:52:41 +0100 Subject: Fixed alignment of CI icon in related issues section Closes #31228 --- app/assets/stylesheets/pages/issues.scss | 13 +++++++++---- .../unreleased/related-branch-ci-status-icon-alignment.yml | 4 ++++ 2 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/related-branch-ci-status-icon-alignment.yml diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss index b2f45625a2a..2aa52986e0a 100644 --- a/app/assets/stylesheets/pages/issues.scss +++ b/app/assets/stylesheets/pages/issues.scss @@ -101,11 +101,16 @@ ul.related-merge-requests > li { } } -.merge-request-ci-status { +.merge-request-ci-status, +.related-merge-requests { + .ci-status-link { + display: block; + margin-top: 3px; + margin-right: 5px; + } + svg { - margin-right: 4px; - position: relative; - top: 1px; + display: block; } } diff --git a/changelogs/unreleased/related-branch-ci-status-icon-alignment.yml b/changelogs/unreleased/related-branch-ci-status-icon-alignment.yml new file mode 100644 index 00000000000..198b6ce15ae --- /dev/null +++ b/changelogs/unreleased/related-branch-ci-status-icon-alignment.yml @@ -0,0 +1,4 @@ +--- +title: Fixed alignment of CI icon in issues related branches +merge_request: +author: -- cgit v1.2.1 From eead6de867d50b2200a9df94c80413afee76d336 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Wed, 26 Apr 2017 08:33:43 -0500 Subject: Remove top padding on project list row --- app/views/shared/projects/_project.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/projects/_project.html.haml b/app/views/shared/projects/_project.html.haml index 71ed23476d2..cf0540afb38 100644 --- a/app/views/shared/projects/_project.html.haml +++ b/app/views/shared/projects/_project.html.haml @@ -54,5 +54,5 @@ = number_with_delimiter(project.star_count) %span.prepend-left-10.visibility-icon.has-tooltip{ data: { container: 'body', placement: 'left' }, title: visibility_icon_description(project) } = visibility_level_icon(project.visibility_level, fw: true) - .prepend-top-5 + .prepend-top-0 updated #{updated_tooltip} -- cgit v1.2.1 From 972440b15dfe2b76670f9bf6f39b8a9dd41589ca Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Tue, 25 Apr 2017 16:48:12 +0100 Subject: tadds migration to remove invalid services --- .../20170421102337_remove_nil_type_services.rb | 12 +++++++ spec/factories/services.rb | 13 +++++++ spec/lib/gitlab/import_export/project.json | 41 ++++++++++------------ .../gitlab/import_export/relation_factory_spec.rb | 2 +- .../project_services/issue_tracker_service_spec.rb | 2 +- spec/models/service_spec.rb | 2 +- 6 files changed, 47 insertions(+), 25 deletions(-) create mode 100644 db/migrate/20170421102337_remove_nil_type_services.rb diff --git a/db/migrate/20170421102337_remove_nil_type_services.rb b/db/migrate/20170421102337_remove_nil_type_services.rb new file mode 100644 index 00000000000..b835b9c6ed9 --- /dev/null +++ b/db/migrate/20170421102337_remove_nil_type_services.rb @@ -0,0 +1,12 @@ +class RemoveNilTypeServices < ActiveRecord::Migration + DOWNTIME = false + + def up + execute <<-SQL + DELETE FROM services WHERE type IS NULL OR type = ''; + SQL + end + + def down + end +end diff --git a/spec/factories/services.rb b/spec/factories/services.rb index 88f6c265505..62aa71ae8d8 100644 --- a/spec/factories/services.rb +++ b/spec/factories/services.rb @@ -1,6 +1,19 @@ FactoryGirl.define do factory :service do project factory: :empty_project + type 'Service' + end + + factory :custom_issue_tracker_service, class: CustomIssueTrackerService do + project factory: :empty_project + type 'CustomIssueTrackerService' + category 'issue_tracker' + active true + properties( + project_url: 'https://project.url.com', + issues_url: 'https://issues.url.com', + new_issue_url: 'https://newissue.url.com' + ) end factory :kubernetes_service do diff --git a/spec/lib/gitlab/import_export/project.json b/spec/lib/gitlab/import_export/project.json index 7a0b0b06d4b..bfecfa28ed1 100644 --- a/spec/lib/gitlab/import_export/project.json +++ b/spec/lib/gitlab/import_export/project.json @@ -6980,28 +6980,6 @@ ], "services": [ - { - "id": 164, - "title": null, - "project_id": 5, - "created_at": "2016-06-14T15:02:07.372Z", - "updated_at": "2016-06-14T15:02:07.372Z", - "active": false, - "properties": { - - }, - "template": false, - "push_events": true, - "issues_events": true, - "merge_requests_events": true, - "tag_push_events": true, - "note_events": true, - "build_events": true, - "category": "issue_tracker", - "type": "CustomIssueTrackerService", - "default": true, - "wiki_page_events": true - }, { "id": 100, "title": "JetBrains TeamCity CI", @@ -7019,6 +6997,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "TeamcityService", "category": "ci", "default": false, "wiki_page_events": true @@ -7040,6 +7019,7 @@ "tag_push_events": true, "note_events": true, "pipeline_events": true, + "type": "SlackService", "category": "common", "default": false, "wiki_page_events": true @@ -7061,6 +7041,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "RedmineService", "category": "issue_tracker", "default": false, "wiki_page_events": true @@ -7082,6 +7063,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "PushoverService", "category": "common", "default": false, "wiki_page_events": true @@ -7103,6 +7085,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "PivotalTrackerService", "category": "common", "default": false, "wiki_page_events": true @@ -7125,6 +7108,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "JiraService", "category": "issue_tracker", "default": false, "wiki_page_events": true @@ -7146,6 +7130,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "IrkerService", "category": "common", "default": false, "wiki_page_events": true @@ -7167,6 +7152,7 @@ "tag_push_events": true, "note_events": true, "pipeline_events": true, + "type": "HipchatService", "category": "common", "default": false, "wiki_page_events": true @@ -7188,6 +7174,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "GemnasiumService", "category": "common", "default": false, "wiki_page_events": true @@ -7209,6 +7196,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "FlowdockService", "category": "common", "default": false, "wiki_page_events": true @@ -7230,6 +7218,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "ExternalWikiService", "category": "common", "default": false, "wiki_page_events": true @@ -7251,6 +7240,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "EmailsOnPushService", "category": "common", "default": false, "wiki_page_events": true @@ -7272,6 +7262,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "DroneCiService", "category": "ci", "default": false, "wiki_page_events": true @@ -7293,6 +7284,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "CustomIssueTrackerService", "category": "issue_tracker", "default": false, "wiki_page_events": true @@ -7314,6 +7306,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "CampfireService", "category": "common", "default": false, "wiki_page_events": true @@ -7335,6 +7328,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "BuildkiteService", "category": "ci", "default": false, "wiki_page_events": true @@ -7356,6 +7350,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "BambooService", "category": "ci", "default": false, "wiki_page_events": true @@ -7377,6 +7372,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "AssemblaService", "category": "common", "default": false, "wiki_page_events": true @@ -7398,6 +7394,7 @@ "tag_push_events": true, "note_events": true, "build_events": true, + "type": "AssemblaService", "category": "common", "default": false, "wiki_page_events": true diff --git a/spec/lib/gitlab/import_export/relation_factory_spec.rb b/spec/lib/gitlab/import_export/relation_factory_spec.rb index fcc23a75ca1..06cd8ab87ed 100644 --- a/spec/lib/gitlab/import_export/relation_factory_spec.rb +++ b/spec/lib/gitlab/import_export/relation_factory_spec.rb @@ -60,7 +60,7 @@ describe Gitlab::ImportExport::RelationFactory, lib: true do end context 'original service exists' do - let(:service_id) { Service.create(project: project).id } + let(:service_id) { create(:service, project: project).id } it 'does not have the original service_id' do expect(created_object.service_id).not_to eq(service_id) diff --git a/spec/models/project_services/issue_tracker_service_spec.rb b/spec/models/project_services/issue_tracker_service_spec.rb index fbe6f344a98..869b25b933b 100644 --- a/spec/models/project_services/issue_tracker_service_spec.rb +++ b/spec/models/project_services/issue_tracker_service_spec.rb @@ -8,7 +8,7 @@ describe IssueTrackerService, models: true do let(:service) { RedmineService.new(project: project, active: true) } before do - create(:service, project: project, active: true, category: 'issue_tracker') + create(:custom_issue_tracker_service, project: project) end context 'when service is changed manually by user' do diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index f5ba8f76f40..4d0181ca297 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -7,7 +7,7 @@ describe Service, models: true do end describe 'Validations' do - it { is_expected.to validate_presence_of(:type).on(:create) } + it { is_expected.to validate_presence_of(:type) } end describe "Test Button" do -- cgit v1.2.1 From ce95397cd22cbf5dbcf48d79799091adb740abdf Mon Sep 17 00:00:00 2001 From: Job van der Voort Date: Thu, 27 Apr 2017 16:45:17 +0100 Subject: gitter badge in the readme pointing to gitlabhq/gitlabhq --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 10d69efdc6b..59de828e1ac 100644 --- a/README.md +++ b/README.md @@ -4,6 +4,7 @@ [![Overall test coverage](https://gitlab.com/gitlab-org/gitlab-ce/badges/master/coverage.svg)](https://gitlab.com/gitlab-org/gitlab-ce/pipelines) [![Code Climate](https://codeclimate.com/github/gitlabhq/gitlabhq.svg)](https://codeclimate.com/github/gitlabhq/gitlabhq) [![Core Infrastructure Initiative Best Practices](https://bestpractices.coreinfrastructure.org/projects/42/badge)](https://bestpractices.coreinfrastructure.org/projects/42) +[![Gitter](https://badges.gitter.im/gitlabhq/gitlabhq.svg)](https://gitter.im/gitlabhq/gitlabhq?utm_source=badge&utm_medium=badge&utm_campaign=pr-badge) ## Test coverage -- cgit v1.2.1 From 55fd112bcc92683f1d602d9173399bcf236a061c Mon Sep 17 00:00:00 2001 From: "Luke \"Jared\" Bennett" Date: Thu, 27 Apr 2017 17:02:21 +0100 Subject: [ci skip] added changelog --- ...milestone-sidebar-display-incorect-number-of-mr-when-minimized.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/31292-milestone-sidebar-display-incorect-number-of-mr-when-minimized.yml diff --git a/changelogs/unreleased/31292-milestone-sidebar-display-incorect-number-of-mr-when-minimized.yml b/changelogs/unreleased/31292-milestone-sidebar-display-incorect-number-of-mr-when-minimized.yml new file mode 100644 index 00000000000..dee831c668b --- /dev/null +++ b/changelogs/unreleased/31292-milestone-sidebar-display-incorect-number-of-mr-when-minimized.yml @@ -0,0 +1,4 @@ +--- +title: Fixed milestone sidebar showing incorrect number of MRs when collapsed +merge_request: 10933 +author: -- cgit v1.2.1 From 22ed9c0293b164409d474dd4170b4bf0ebeb92a3 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Thu, 27 Apr 2017 16:49:10 +0300 Subject: Make api/v3/deployments_spec to actually tests v3 Signed-off-by: Dmitriy Zaporozhets --- spec/requests/api/v3/deployments_spec.rb | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/requests/api/v3/deployments_spec.rb b/spec/requests/api/v3/deployments_spec.rb index 694786c3046..0389a264781 100644 --- a/spec/requests/api/v3/deployments_spec.rb +++ b/spec/requests/api/v3/deployments_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -describe API::Deployments do +describe API::V3::Deployments do let(:user) { create(:user) } let(:non_member) { create(:user) } let(:project) { deployment.environment.project } @@ -24,11 +24,11 @@ describe API::Deployments do describe 'GET /projects/:id/deployments' do context 'as member of the project' do it_behaves_like 'a paginated resources' do - let(:request) { get api("/projects/#{project.id}/deployments", user) } + let(:request) { get v3_api("/projects/#{project.id}/deployments", user) } end it 'returns projects deployments' do - get api("/projects/#{project.id}/deployments", user) + get v3_api("/projects/#{project.id}/deployments", user) expect(response).to have_http_status(200) expect(json_response).to be_an Array @@ -40,7 +40,7 @@ describe API::Deployments do context 'as non member' do it 'returns a 404 status code' do - get api("/projects/#{project.id}/deployments", non_member) + get v3_api("/projects/#{project.id}/deployments", non_member) expect(response).to have_http_status(404) end @@ -50,7 +50,7 @@ describe API::Deployments do describe 'GET /projects/:id/deployments/:deployment_id' do context 'as a member of the project' do it 'returns the projects deployment' do - get api("/projects/#{project.id}/deployments/#{deployment.id}", user) + get v3_api("/projects/#{project.id}/deployments/#{deployment.id}", user) expect(response).to have_http_status(200) expect(json_response['sha']).to match /\A\h{40}\z/ @@ -60,7 +60,7 @@ describe API::Deployments do context 'as non member' do it 'returns a 404 status code' do - get api("/projects/#{project.id}/deployments/#{deployment.id}", non_member) + get v3_api("/projects/#{project.id}/deployments/#{deployment.id}", non_member) expect(response).to have_http_status(404) end -- cgit v1.2.1 From 7f625f06d333cbe3846081924a3e7016b2846ae0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 11:59:52 -0500 Subject: Pass project to Blob --- app/controllers/projects/blob_controller.rb | 2 +- app/models/blob.rb | 12 ++++++++++-- app/models/commit.rb | 2 +- app/models/repository.rb | 2 +- 4 files changed, 13 insertions(+), 5 deletions(-) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 9fce1db6742..4c6db91d7c0 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -96,7 +96,7 @@ class Projects::BlobController < Projects::ApplicationController private def blob - @blob ||= Blob.decorate(@repository.blob_at(@commit.id, @path)) + @blob ||= Blob.decorate(@repository.blob_at(@commit.id, @path), @project) if @blob @blob diff --git a/app/models/blob.rb b/app/models/blob.rb index 55872acef51..c6315a6789f 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -6,6 +6,8 @@ class Blob < SimpleDelegator # The maximum size of an SVG that can be displayed. MAXIMUM_SVG_SIZE = 2.megabytes + attr_reader :project + # Wrap a Gitlab::Git::Blob object, or return nil when given nil # # This method prevents the decorated object from evaluating to "truthy" when @@ -16,10 +18,16 @@ class Blob < SimpleDelegator # # blob = Blob.decorate(nil) # puts "truthy" if blob # No output - def self.decorate(blob) + def self.decorate(blob, project) return if blob.nil? - new(blob) + new(blob, project) + end + + def initialize(blob, project) + @project = project + + super(blob) end # Returns the data of the blob. diff --git a/app/models/commit.rb b/app/models/commit.rb index 8b8b3f00202..bb4cb8efd15 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -316,7 +316,7 @@ class Commit def uri_type(path) entry = @raw.tree.path(path) if entry[:type] == :blob - blob = ::Blob.decorate(Gitlab::Git::Blob.new(name: entry[:name])) + blob = ::Blob.decorate(Gitlab::Git::Blob.new(name: entry[:name]), @project) blob.image? || blob.video? ? :raw : :blob else entry[:type] diff --git a/app/models/repository.rb b/app/models/repository.rb index e74edb8e6f7..d02aea49689 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -450,7 +450,7 @@ class Repository def blob_at(sha, path) unless Gitlab::Git.blank_ref?(sha) - Blob.decorate(Gitlab::Git::Blob.find(self, sha, path)) + Blob.decorate(Gitlab::Git::Blob.find(self, sha, path), project) end rescue Gitlab::Git::Repository::NoRepository nil -- cgit v1.2.1 From 13891a03c7388315bb66b30cd0b8e87fa21958b0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:14:08 -0500 Subject: Refactor Blob and add BlobViewer::Base --- app/models/blob.rb | 126 +++++++++++++++++++++++------------------ app/models/blob_viewer/base.rb | 81 ++++++++++++++++++++++++++ 2 files changed, 151 insertions(+), 56 deletions(-) create mode 100644 app/models/blob_viewer/base.rb diff --git a/app/models/blob.rb b/app/models/blob.rb index c6315a6789f..2b2425a926e 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -3,8 +3,10 @@ class Blob < SimpleDelegator CACHE_TIME = 60 # Cache raw blobs referred to by a (mutable) ref for 1 minute CACHE_TIME_IMMUTABLE = 3600 # Cache blobs referred to by an immutable reference for 1 hour - # The maximum size of an SVG that can be displayed. - MAXIMUM_SVG_SIZE = 2.megabytes + MAXIMUM_TEXT_HIGHLIGHT_SIZE = 1.megabyte + + RICH_VIEWERS = [ + ].freeze attr_reader :project @@ -43,82 +45,94 @@ class Blob < SimpleDelegator end def no_highlighting? - size && size > 1.megabyte + size && size > MAXIMUM_TEXT_HIGHLIGHT_SIZE end - def only_display_raw? + def too_large? size && truncated? end - def extension - extname.downcase.delete('.') + def raw_size + if valid_lfs_pointer? + lfs_size + else + size + end end - def svg? - text? && language && language.name == 'SVG' + def raw_binary? + if valid_lfs_pointer? + !rich_viewer&.text_based? + else + binary? + end end - def pdf? - extension == 'pdf' + def extension + @extension ||= extname.downcase.delete('.') end - def ipython_notebook? - text? && language&.name == 'Jupyter Notebook' + def video? + UploaderHelper::VIDEO_EXT.include?(extension) end - def sketch? - binary? && extension == 'sketch' + def readable_text? + text? && !valid_lfs_pointer? && !too_large? end - def stl? - extension == 'stl' + def valid_lfs_pointer? + lfs_pointer? && project.lfs_enabled? end - def markup? - text? && Gitlab::MarkupHelper.markup?(name) + def invalid_lfs_pointer? + lfs_pointer? && !project.lfs_enabled? end - def size_within_svg_limits? - size <= MAXIMUM_SVG_SIZE + def simple_viewer_class + if empty? + BlobViewer::Empty + elsif raw_binary? + BlobViewer::Download + else # text + BlobViewer::Text + end end - def video? - UploaderHelper::VIDEO_EXT.include?(extname.downcase.delete('.')) - end - - def to_partial_path(project) - if lfs_pointer? - if project.lfs_enabled? - 'download' - else - 'text' - end - elsif image? - 'image' - elsif svg? - 'svg' - elsif pdf? - 'pdf' - elsif ipython_notebook? - 'notebook' - elsif sketch? - 'sketch' - elsif stl? - 'stl' - elsif markup? - if only_display_raw? - 'too_large' - else - 'markup' - end - elsif text? - if only_display_raw? - 'too_large' - else - 'text' - end + def rich_viewer_class + if invalid_lfs_pointer? || empty? + nil else - 'download' + rich_viewers_classes.find { |viewer_class| viewer_class.can_render?(self) } + end + end + + def simple_viewer + @simple_viewer ||= simple_viewer_class.new(self) + end + + def rich_viewer + return @rich_viewer if defined?(@rich_viewer) + + @rich_viewer ||= rich_viewer_class&.new(self) + end + + def rendered_as_text?(override_max_size: false) + simple_viewer.is_a?(BlobViewer::Text) && !simple_viewer.render_error(override_max_size: override_max_size) + end + + def show_viewer_switcher? + simple_viewer.is_a?(BlobViewer::Text) && rich_viewer + end + + private + + def rich_viewers_classes + if valid_lfs_pointer? + RICH_VIEWERS + elsif binary? + RICH_VIEWERS.reject(&:text_based?) + else # text + RICH_VIEWERS.select(&:text_based?) end end end diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb new file mode 100644 index 00000000000..37acacc0019 --- /dev/null +++ b/app/models/blob_viewer/base.rb @@ -0,0 +1,81 @@ +module BlobViewer + class Base + class_attribute :partial_name, :type, :extensions, :client_side, :text_based, :switcher_icon, :switcher_title, :max_size, :absolute_max_size + + delegate :partial_path, :rich?, :simple?, :client_side?, :text_based?, to: :class + + attr_reader :blob + + def initialize(blob) + @blob = blob + end + + def self.partial_path + "projects/blob/viewers/#{partial_name}" + end + + def self.rich? + type == :rich + end + + def self.simple? + type == :simple + end + + def self.client_side? + client_side + end + + def server_side? + !client_side? + end + + def self.text_based? + text_based + end + + def self.can_render?(blob) + !extensions || extensions.include?(blob.extension) + end + + def can_override_max_size? + too_large? && !too_large?(override_max_size: true) + end + + def relevant_max_size + if too_large?(override_max_size: true) + absolute_max_size + elsif too_large? + max_size + end + end + + def render_error(override_max_size: false) + if too_large?(override_max_size: override_max_size) + :too_large + elsif server_side_but_stored_in_lfs? + :server_side_but_stored_in_lfs + end + end + + def prepare! + if server_side? && blob.project + blob.load_all_data!(blob.project.repository) + end + end + + private + + def too_large?(override_max_size: false) + if override_max_size + blob.raw_size > absolute_max_size + else + blob.raw_size > max_size + end + end + + def server_side_but_stored_in_lfs? + !client_side? && blob.valid_lfs_pointer? + end + end +end -- cgit v1.2.1 From f112a03cba884aa67d5d601520d4c05ce517f28a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:04:29 -0500 Subject: Add simple blob viewers --- app/models/blob_viewer/client_side.rb | 11 +++++++++++ app/models/blob_viewer/download.rb | 13 +++++++++++++ app/models/blob_viewer/empty.rb | 9 +++++++++ app/models/blob_viewer/simple.rb | 11 +++++++++++ app/models/blob_viewer/text.rb | 11 +++++++++++ app/views/projects/blob/_download.html.haml | 7 ------- app/views/projects/blob/_text.html.haml | 2 -- app/views/projects/blob/viewers/_download.html.haml | 7 +++++++ app/views/projects/blob/viewers/_empty.html.haml | 3 +++ app/views/projects/blob/viewers/_text.html.haml | 1 + 10 files changed, 66 insertions(+), 9 deletions(-) create mode 100644 app/models/blob_viewer/client_side.rb create mode 100644 app/models/blob_viewer/download.rb create mode 100644 app/models/blob_viewer/empty.rb create mode 100644 app/models/blob_viewer/simple.rb create mode 100644 app/models/blob_viewer/text.rb delete mode 100644 app/views/projects/blob/_download.html.haml delete mode 100644 app/views/projects/blob/_text.html.haml create mode 100644 app/views/projects/blob/viewers/_download.html.haml create mode 100644 app/views/projects/blob/viewers/_empty.html.haml create mode 100644 app/views/projects/blob/viewers/_text.html.haml diff --git a/app/models/blob_viewer/client_side.rb b/app/models/blob_viewer/client_side.rb new file mode 100644 index 00000000000..42ec68f864b --- /dev/null +++ b/app/models/blob_viewer/client_side.rb @@ -0,0 +1,11 @@ +module BlobViewer + module ClientSide + extend ActiveSupport::Concern + + included do + self.client_side = true + self.max_size = 10.megabytes + self.absolute_max_size = 50.megabytes + end + end +end diff --git a/app/models/blob_viewer/download.rb b/app/models/blob_viewer/download.rb new file mode 100644 index 00000000000..45cb38a3268 --- /dev/null +++ b/app/models/blob_viewer/download.rb @@ -0,0 +1,13 @@ +module BlobViewer + class Download < Base + include Simple + include ServerSide + + self.partial_name = 'download' + self.text_based = false + + def render_error(*) + nil + end + end +end diff --git a/app/models/blob_viewer/empty.rb b/app/models/blob_viewer/empty.rb new file mode 100644 index 00000000000..60003a7c12a --- /dev/null +++ b/app/models/blob_viewer/empty.rb @@ -0,0 +1,9 @@ +module BlobViewer + class Empty < Base + include Simple + include ServerSide + + self.partial_name = 'empty' + self.text_based = false + end +end diff --git a/app/models/blob_viewer/simple.rb b/app/models/blob_viewer/simple.rb new file mode 100644 index 00000000000..454a20495fc --- /dev/null +++ b/app/models/blob_viewer/simple.rb @@ -0,0 +1,11 @@ +module BlobViewer + module Simple + extend ActiveSupport::Concern + + included do + self.type = :simple + self.switcher_icon = 'code' + self.switcher_title = 'source' + end + end +end diff --git a/app/models/blob_viewer/text.rb b/app/models/blob_viewer/text.rb new file mode 100644 index 00000000000..5f442dadf0f --- /dev/null +++ b/app/models/blob_viewer/text.rb @@ -0,0 +1,11 @@ +module BlobViewer + class Text < Base + include Simple + include ServerSide + + self.partial_name = 'text' + self.text_based = true + self.max_size = 1.megabyte + self.absolute_max_size = 10.megabytes + end +end diff --git a/app/views/projects/blob/_download.html.haml b/app/views/projects/blob/_download.html.haml deleted file mode 100644 index 7908fcae3de..00000000000 --- a/app/views/projects/blob/_download.html.haml +++ /dev/null @@ -1,7 +0,0 @@ -.file-content.blob_file.blob-no-preview - .center - = link_to namespace_project_raw_path(@project.namespace, @project, @id) do - %h1.light - %i.fa.fa-download - %h4 - Download (#{number_to_human_size blob_size(blob)}) diff --git a/app/views/projects/blob/_text.html.haml b/app/views/projects/blob/_text.html.haml deleted file mode 100644 index 20638f6961d..00000000000 --- a/app/views/projects/blob/_text.html.haml +++ /dev/null @@ -1,2 +0,0 @@ -- blob.load_all_data!(@repository) -= render 'shared/file_highlight', blob: blob, repository: @repository diff --git a/app/views/projects/blob/viewers/_download.html.haml b/app/views/projects/blob/viewers/_download.html.haml new file mode 100644 index 00000000000..684240d02c7 --- /dev/null +++ b/app/views/projects/blob/viewers/_download.html.haml @@ -0,0 +1,7 @@ +.file-content.blob_file.blob-no-preview + .center + = link_to blob_raw_url do + %h1.light + = icon('download') + %h4 + Download (#{number_to_human_size(viewer.blob.raw_size)}) diff --git a/app/views/projects/blob/viewers/_empty.html.haml b/app/views/projects/blob/viewers/_empty.html.haml new file mode 100644 index 00000000000..a293a8de231 --- /dev/null +++ b/app/views/projects/blob/viewers/_empty.html.haml @@ -0,0 +1,3 @@ +.file-content.code + .nothing-here-block + Empty file diff --git a/app/views/projects/blob/viewers/_text.html.haml b/app/views/projects/blob/viewers/_text.html.haml new file mode 100644 index 00000000000..a91df321ca0 --- /dev/null +++ b/app/views/projects/blob/viewers/_text.html.haml @@ -0,0 +1 @@ += render 'shared/file_highlight', blob: viewer.blob, repository: @repository -- cgit v1.2.1 From 5cb27e9fa74c7234195e10ade933e8a475cff403 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:21:07 -0500 Subject: Add rich blob viewers --- app/helpers/blob_helper.rb | 9 ++++++--- app/models/blob.rb | 8 ++++++++ app/models/blob_viewer/binary_stl.rb | 10 ++++++++++ app/models/blob_viewer/image.rb | 12 ++++++++++++ app/models/blob_viewer/markup.rb | 10 ++++++++++ app/models/blob_viewer/notebook.rb | 12 ++++++++++++ app/models/blob_viewer/pdf.rb | 12 ++++++++++++ app/models/blob_viewer/rich.rb | 11 +++++++++++ app/models/blob_viewer/server_side.rb | 11 +++++++++++ app/models/blob_viewer/sketch.rb | 12 ++++++++++++ app/models/blob_viewer/svg.rb | 12 ++++++++++++ app/models/blob_viewer/text_stl.rb | 10 ++++++++++ app/views/projects/blob/_image.html.haml | 2 -- app/views/projects/blob/_notebook.html.haml | 5 ----- app/views/projects/blob/_pdf.html.haml | 5 ----- app/views/projects/blob/_sketch.html.haml | 7 ------- app/views/projects/blob/_stl.html.haml | 12 ------------ app/views/projects/blob/_svg.html.haml | 9 --------- app/views/projects/blob/viewers/_image.html.haml | 2 ++ app/views/projects/blob/viewers/_markup.html.haml | 3 +++ app/views/projects/blob/viewers/_notebook.html.haml | 5 +++++ app/views/projects/blob/viewers/_pdf.html.haml | 5 +++++ app/views/projects/blob/viewers/_sketch.html.haml | 7 +++++++ app/views/projects/blob/viewers/_stl.html.haml | 12 ++++++++++++ app/views/projects/blob/viewers/_svg.html.haml | 4 ++++ 25 files changed, 164 insertions(+), 43 deletions(-) create mode 100644 app/models/blob_viewer/binary_stl.rb create mode 100644 app/models/blob_viewer/image.rb create mode 100644 app/models/blob_viewer/markup.rb create mode 100644 app/models/blob_viewer/notebook.rb create mode 100644 app/models/blob_viewer/pdf.rb create mode 100644 app/models/blob_viewer/rich.rb create mode 100644 app/models/blob_viewer/server_side.rb create mode 100644 app/models/blob_viewer/sketch.rb create mode 100644 app/models/blob_viewer/svg.rb create mode 100644 app/models/blob_viewer/text_stl.rb delete mode 100644 app/views/projects/blob/_image.html.haml delete mode 100644 app/views/projects/blob/_notebook.html.haml delete mode 100644 app/views/projects/blob/_pdf.html.haml delete mode 100644 app/views/projects/blob/_sketch.html.haml delete mode 100644 app/views/projects/blob/_stl.html.haml delete mode 100644 app/views/projects/blob/_svg.html.haml create mode 100644 app/views/projects/blob/viewers/_image.html.haml create mode 100644 app/views/projects/blob/viewers/_markup.html.haml create mode 100644 app/views/projects/blob/viewers/_notebook.html.haml create mode 100644 app/views/projects/blob/viewers/_pdf.html.haml create mode 100644 app/views/projects/blob/viewers/_sketch.html.haml create mode 100644 app/views/projects/blob/viewers/_stl.html.haml create mode 100644 app/views/projects/blob/viewers/_svg.html.haml diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 36b16421e8f..ade9e19cea8 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -134,12 +134,15 @@ module BlobHelper end end + def blob_raw_url + namespace_project_raw_path(@project.namespace, @project, @id) + end + # SVGs can contain malicious JavaScript; only include whitelisted # elements and attributes. Note that this whitelist is by no means complete # and may omit some elements. - def sanitize_svg(blob) - blob.data = Gitlab::Sanitizers::SVG.clean(blob.data) - blob + def sanitize_svg_data(data) + Gitlab::Sanitizers::SVG.clean(data) end # If we blindly set the 'real' content type when serving a Git blob we diff --git a/app/models/blob.rb b/app/models/blob.rb index 2b2425a926e..26ee2c883a0 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -6,6 +6,14 @@ class Blob < SimpleDelegator MAXIMUM_TEXT_HIGHLIGHT_SIZE = 1.megabyte RICH_VIEWERS = [ + BlobViewer::Image, + BlobViewer::PDF, + BlobViewer::Sketch, + BlobViewer::BinarySTL, + BlobViewer::TextSTL, + BlobViewer::Notebook, + BlobViewer::SVG, + BlobViewer::Markup, ].freeze attr_reader :project diff --git a/app/models/blob_viewer/binary_stl.rb b/app/models/blob_viewer/binary_stl.rb new file mode 100644 index 00000000000..6cb54d49d86 --- /dev/null +++ b/app/models/blob_viewer/binary_stl.rb @@ -0,0 +1,10 @@ +module BlobViewer + class BinarySTL < Base + include Rich + include ClientSide + + self.partial_name = 'stl' + self.extensions = %w(stl) + self.text_based = false + end +end diff --git a/app/models/blob_viewer/image.rb b/app/models/blob_viewer/image.rb new file mode 100644 index 00000000000..b78f82df38f --- /dev/null +++ b/app/models/blob_viewer/image.rb @@ -0,0 +1,12 @@ +module BlobViewer + class Image < Base + include Rich + include ClientSide + + self.partial_name = 'image' + self.extensions = UploaderHelper::IMAGE_EXT + self.text_based = false + self.switcher_icon = 'picture-o' + self.switcher_title = 'image' + end +end diff --git a/app/models/blob_viewer/markup.rb b/app/models/blob_viewer/markup.rb new file mode 100644 index 00000000000..34740661320 --- /dev/null +++ b/app/models/blob_viewer/markup.rb @@ -0,0 +1,10 @@ +module BlobViewer + class Markup < Base + include Rich + include ServerSide + + self.partial_name = 'markup' + self.extensions = Gitlab::MarkupHelper::EXTENSIONS + self.text_based = true + end +end diff --git a/app/models/blob_viewer/notebook.rb b/app/models/blob_viewer/notebook.rb new file mode 100644 index 00000000000..c5231f1e691 --- /dev/null +++ b/app/models/blob_viewer/notebook.rb @@ -0,0 +1,12 @@ +module BlobViewer + class Notebook < Base + include Rich + include ClientSide + + self.partial_name = 'notebook' + self.extensions = %w(ipynb) + self.text_based = true + self.switcher_icon = 'file-text-o' + self.switcher_title = 'notebook' + end +end diff --git a/app/models/blob_viewer/pdf.rb b/app/models/blob_viewer/pdf.rb new file mode 100644 index 00000000000..1067388c72a --- /dev/null +++ b/app/models/blob_viewer/pdf.rb @@ -0,0 +1,12 @@ +module BlobViewer + class PDF < Base + include Rich + include ClientSide + + self.partial_name = 'pdf' + self.extensions = %w(pdf) + self.text_based = false + self.switcher_icon = 'file-pdf-o' + self.switcher_title = 'PDF' + end +end diff --git a/app/models/blob_viewer/rich.rb b/app/models/blob_viewer/rich.rb new file mode 100644 index 00000000000..be373dbc948 --- /dev/null +++ b/app/models/blob_viewer/rich.rb @@ -0,0 +1,11 @@ +module BlobViewer + module Rich + extend ActiveSupport::Concern + + included do + self.type = :rich + self.switcher_icon = 'file-text-o' + self.switcher_title = 'rendered file' + end + end +end diff --git a/app/models/blob_viewer/server_side.rb b/app/models/blob_viewer/server_side.rb new file mode 100644 index 00000000000..899107d02ea --- /dev/null +++ b/app/models/blob_viewer/server_side.rb @@ -0,0 +1,11 @@ +module BlobViewer + module ServerSide + extend ActiveSupport::Concern + + included do + self.client_side = false + self.max_size = 2.megabytes + self.absolute_max_size = 5.megabytes + end + end +end diff --git a/app/models/blob_viewer/sketch.rb b/app/models/blob_viewer/sketch.rb new file mode 100644 index 00000000000..3de94efe67d --- /dev/null +++ b/app/models/blob_viewer/sketch.rb @@ -0,0 +1,12 @@ +module BlobViewer + class Sketch < Base + include Rich + include ClientSide + + self.partial_name = 'sketch' + self.extensions = %w(sketch) + self.text_based = false + self.switcher_icon = 'file-image-o' + self.switcher_title = 'preview' + end +end diff --git a/app/models/blob_viewer/svg.rb b/app/models/blob_viewer/svg.rb new file mode 100644 index 00000000000..5fd11a98462 --- /dev/null +++ b/app/models/blob_viewer/svg.rb @@ -0,0 +1,12 @@ +module BlobViewer + class SVG < Base + include Rich + include ServerSide + + self.partial_name = 'svg' + self.extensions = %w(svg) + self.text_based = true + self.switcher_icon = 'picture-o' + self.switcher_title = 'image' + end +end diff --git a/app/models/blob_viewer/text_stl.rb b/app/models/blob_viewer/text_stl.rb new file mode 100644 index 00000000000..1b238777c32 --- /dev/null +++ b/app/models/blob_viewer/text_stl.rb @@ -0,0 +1,10 @@ +module BlobViewer + class TextSTL < Base + include Rich + include ClientSide + + self.partial_name = 'stl' + self.extensions = %w(stl) + self.text_based = true + end +end diff --git a/app/views/projects/blob/_image.html.haml b/app/views/projects/blob/_image.html.haml deleted file mode 100644 index 73877d730f5..00000000000 --- a/app/views/projects/blob/_image.html.haml +++ /dev/null @@ -1,2 +0,0 @@ -.file-content.image_file - %img{ src: namespace_project_raw_path(@project.namespace, @project, @id), alt: blob.name } diff --git a/app/views/projects/blob/_notebook.html.haml b/app/views/projects/blob/_notebook.html.haml deleted file mode 100644 index ab1cf933944..00000000000 --- a/app/views/projects/blob/_notebook.html.haml +++ /dev/null @@ -1,5 +0,0 @@ -- content_for :page_specific_javascripts do - = page_specific_javascript_bundle_tag('common_vue') - = page_specific_javascript_bundle_tag('notebook_viewer') - -.file-content#js-notebook-viewer{ data: { endpoint: namespace_project_raw_path(@project.namespace, @project, @id) } } diff --git a/app/views/projects/blob/_pdf.html.haml b/app/views/projects/blob/_pdf.html.haml deleted file mode 100644 index 58dc88e3bf7..00000000000 --- a/app/views/projects/blob/_pdf.html.haml +++ /dev/null @@ -1,5 +0,0 @@ -- content_for :page_specific_javascripts do - = page_specific_javascript_bundle_tag('common_vue') - = page_specific_javascript_bundle_tag('pdf_viewer') - -.file-content#js-pdf-viewer{ data: { endpoint: namespace_project_raw_path(@project.namespace, @project, @id) } } diff --git a/app/views/projects/blob/_sketch.html.haml b/app/views/projects/blob/_sketch.html.haml deleted file mode 100644 index dad9369cb2a..00000000000 --- a/app/views/projects/blob/_sketch.html.haml +++ /dev/null @@ -1,7 +0,0 @@ -- content_for :page_specific_javascripts do - = page_specific_javascript_bundle_tag('common_vue') - = page_specific_javascript_bundle_tag('sketch_viewer') - -.file-content#js-sketch-viewer{ data: { endpoint: namespace_project_raw_path(@project.namespace, @project, @id) } } - .js-loading-icon.text-center.prepend-top-default.append-bottom-default.js-loading-icon{ 'aria-label' => 'Loading Sketch preview' } - = icon('spinner spin 2x', 'aria-hidden' => 'true'); diff --git a/app/views/projects/blob/_stl.html.haml b/app/views/projects/blob/_stl.html.haml deleted file mode 100644 index a9332a0eeb6..00000000000 --- a/app/views/projects/blob/_stl.html.haml +++ /dev/null @@ -1,12 +0,0 @@ -- content_for :page_specific_javascripts do - = page_specific_javascript_bundle_tag('stl_viewer') - -.file-content.is-stl-loading - .text-center#js-stl-viewer{ data: { endpoint: namespace_project_raw_path(@project.namespace, @project, @id) } } - = icon('spinner spin 2x', class: 'prepend-top-default append-bottom-default', 'aria-hidden' => 'true', 'aria-label' => 'Loading') - .text-center.prepend-top-default.append-bottom-default.stl-controls - .btn-group - %button.btn.btn-default.btn-sm.js-material-changer{ data: { type: 'wireframe' } } - Wireframe - %button.btn.btn-default.btn-sm.active.js-material-changer{ data: { type: 'default' } } - Solid diff --git a/app/views/projects/blob/_svg.html.haml b/app/views/projects/blob/_svg.html.haml deleted file mode 100644 index 93be58fc658..00000000000 --- a/app/views/projects/blob/_svg.html.haml +++ /dev/null @@ -1,9 +0,0 @@ -- if blob.size_within_svg_limits? - -# We need to scrub SVG but we cannot do so in the RawController: it would - -# be wrong/strange if RawController modified the data. - - blob.load_all_data!(@repository) - - blob = sanitize_svg(blob) - .file-content.image_file - %img{ src: "data:#{blob.mime_type};base64,#{Base64.encode64(blob.data)}", alt: blob.name } -- else - = render 'too_large' diff --git a/app/views/projects/blob/viewers/_image.html.haml b/app/views/projects/blob/viewers/_image.html.haml new file mode 100644 index 00000000000..640d59b3174 --- /dev/null +++ b/app/views/projects/blob/viewers/_image.html.haml @@ -0,0 +1,2 @@ +.file-content.image_file + %img{ src: blob_raw_url, alt: viewer.blob.name } diff --git a/app/views/projects/blob/viewers/_markup.html.haml b/app/views/projects/blob/viewers/_markup.html.haml new file mode 100644 index 00000000000..ae8f3e05687 --- /dev/null +++ b/app/views/projects/blob/viewers/_markup.html.haml @@ -0,0 +1,3 @@ +- blob = viewer.blob +.file-content.wiki + = render_markup(blob.name, blob.data) diff --git a/app/views/projects/blob/viewers/_notebook.html.haml b/app/views/projects/blob/viewers/_notebook.html.haml new file mode 100644 index 00000000000..2399fb16265 --- /dev/null +++ b/app/views/projects/blob/viewers/_notebook.html.haml @@ -0,0 +1,5 @@ +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('common_vue') + = page_specific_javascript_bundle_tag('notebook_viewer') + +.file-content#js-notebook-viewer{ data: { endpoint: blob_raw_url } } diff --git a/app/views/projects/blob/viewers/_pdf.html.haml b/app/views/projects/blob/viewers/_pdf.html.haml new file mode 100644 index 00000000000..1dd179c4fdc --- /dev/null +++ b/app/views/projects/blob/viewers/_pdf.html.haml @@ -0,0 +1,5 @@ +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('common_vue') + = page_specific_javascript_bundle_tag('pdf_viewer') + +.file-content#js-pdf-viewer{ data: { endpoint: blob_raw_url } } diff --git a/app/views/projects/blob/viewers/_sketch.html.haml b/app/views/projects/blob/viewers/_sketch.html.haml new file mode 100644 index 00000000000..49f716c2c59 --- /dev/null +++ b/app/views/projects/blob/viewers/_sketch.html.haml @@ -0,0 +1,7 @@ +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('common_vue') + = page_specific_javascript_bundle_tag('sketch_viewer') + +.file-content#js-sketch-viewer{ data: { endpoint: blob_raw_url } } + .js-loading-icon.text-center.prepend-top-default.append-bottom-default.js-loading-icon{ 'aria-label' => 'Loading Sketch preview' } + = icon('spinner spin 2x', 'aria-hidden' => 'true'); diff --git a/app/views/projects/blob/viewers/_stl.html.haml b/app/views/projects/blob/viewers/_stl.html.haml new file mode 100644 index 00000000000..e4e9d746176 --- /dev/null +++ b/app/views/projects/blob/viewers/_stl.html.haml @@ -0,0 +1,12 @@ +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('stl_viewer') + +.file-content.is-stl-loading + .text-center#js-stl-viewer{ data: { endpoint: blob_raw_url } } + = icon('spinner spin 2x', class: 'prepend-top-default append-bottom-default', 'aria-hidden' => 'true', 'aria-label' => 'Loading') + .text-center.prepend-top-default.append-bottom-default.stl-controls + .btn-group + %button.btn.btn-default.btn-sm.js-material-changer{ data: { type: 'wireframe' } } + Wireframe + %button.btn.btn-default.btn-sm.active.js-material-changer{ data: { type: 'default' } } + Solid diff --git a/app/views/projects/blob/viewers/_svg.html.haml b/app/views/projects/blob/viewers/_svg.html.haml new file mode 100644 index 00000000000..62f647581b6 --- /dev/null +++ b/app/views/projects/blob/viewers/_svg.html.haml @@ -0,0 +1,4 @@ +- blob = viewer.blob +- data = sanitize_svg_data(blob.data) +.file-content.image_file + %img{ src: "data:#{blob.mime_type};base64,#{Base64.encode64(data)}", alt: blob.name } -- cgit v1.2.1 From 0d1ec11e8974b38b0d749a08fde2817c61326441 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 11:57:43 -0500 Subject: Use Blob methods instead of BlobHelper helpers --- app/controllers/projects/raw_controller.rb | 2 +- app/helpers/blob_helper.rb | 20 ++------------------ app/views/notify/repository_push_email.html.haml | 2 +- app/views/projects/blob/_header.html.haml | 6 +++--- app/views/projects/diffs/_content.html.haml | 4 ++-- app/views/projects/diffs/_diffs.html.haml | 2 +- app/views/projects/diffs/_file.html.haml | 2 +- 7 files changed, 11 insertions(+), 27 deletions(-) diff --git a/app/controllers/projects/raw_controller.rb b/app/controllers/projects/raw_controller.rb index c55b37ae0dd..a0b08ad130f 100644 --- a/app/controllers/projects/raw_controller.rb +++ b/app/controllers/projects/raw_controller.rb @@ -15,7 +15,7 @@ class Projects::RawController < Projects::ApplicationController return if cached_blob? - if @blob.lfs_pointer? && project.lfs_enabled? + if @blob.valid_lfs_pointer? send_lfs_object else send_git_blob @repository, @blob diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index ade9e19cea8..e2092b8bfbf 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -52,7 +52,7 @@ module BlobHelper if !on_top_of_branch?(project, ref) button_tag label, class: "#{common_classes} disabled has-tooltip", title: "You can only #{action} files when you are on a branch", data: { container: 'body' } - elsif blob.lfs_pointer? + elsif blob.valid_lfs_pointer? button_tag label, class: "#{common_classes} disabled has-tooltip", title: "It is not possible to #{action} files that are stored in LFS using the web interface", data: { container: 'body' } elsif can_modify_blob?(blob, project, ref) button_tag label, class: "#{common_classes}", 'data-target' => "#modal-#{modal_type}-blob", 'data-toggle' => 'modal' @@ -95,7 +95,7 @@ module BlobHelper end def can_modify_blob?(blob, project = @project, ref = @ref) - !blob.lfs_pointer? && can_edit_tree?(project, ref) + !blob.valid_lfs_pointer? && can_edit_tree?(project, ref) end def leave_edit_message @@ -118,22 +118,6 @@ module BlobHelper icon("#{file_type_icon_class('file', mode, name)} fw") end - def blob_text_viewable?(blob) - blob && blob.text? && !blob.lfs_pointer? && !blob.only_display_raw? - end - - def blob_rendered_as_text?(blob) - blob_text_viewable?(blob) && blob.to_partial_path(@project) == 'text' - end - - def blob_size(blob) - if blob.lfs_pointer? - blob.lfs_size - else - blob.size - end - end - def blob_raw_url namespace_project_raw_path(@project.namespace, @project, @id) end diff --git a/app/views/notify/repository_push_email.html.haml b/app/views/notify/repository_push_email.html.haml index c6b1db17f91..36512533c58 100644 --- a/app/views/notify/repository_push_email.html.haml +++ b/app/views/notify/repository_push_email.html.haml @@ -74,7 +74,7 @@ - else %hr - blob = diff_file.blob - - if blob && blob.respond_to?(:text?) && blob_text_viewable?(blob) + - if blob && blob.respond_to?(:text?) && blob.readable_text? %table.code.white = render partial: "projects/diffs/line", collection: diff_file.highlighted_diff_lines, as: :line, locals: { diff_file: diff_file, plain: true, email: true } - else diff --git a/app/views/projects/blob/_header.html.haml b/app/views/projects/blob/_header.html.haml index c553db84ee0..81fd8e5792b 100644 --- a/app/views/projects/blob/_header.html.haml +++ b/app/views/projects/blob/_header.html.haml @@ -9,7 +9,7 @@ = copy_file_path_button(blob.path) %small - = number_to_human_size(blob_size(blob)) + = number_to_human_size(blob.raw_size) .file-actions.hidden-xs .btn-group{ role: "group" }< @@ -19,7 +19,7 @@ .btn-group{ role: "group" }< -# only show normal/blame view links for text files - - if blob_text_viewable?(blob) + - if blob.readable_text? - if blame = link_to 'Normal view', namespace_project_blob_path(@project.namespace, @project, @id), class: 'btn btn-sm' @@ -34,7 +34,7 @@ tree_join(@commit.sha, @path)), class: 'btn btn-sm js-data-file-blob-permalink-url' .btn-group{ role: "group" }< - = edit_blob_link if blob_text_viewable?(blob) + = edit_blob_link if blob.readable_text? - if current_user = replace_blob_link = delete_blob_link diff --git a/app/views/projects/diffs/_content.html.haml b/app/views/projects/diffs/_content.html.haml index 438a98c3e95..c781e423c4d 100644 --- a/app/views/projects/diffs/_content.html.haml +++ b/app/views/projects/diffs/_content.html.haml @@ -3,9 +3,9 @@ - return unless blob.respond_to?(:text?) - if diff_file.too_large? .nothing-here-block This diff could not be displayed because it is too large. - - elsif blob.only_display_raw? + - elsif blob.too_large? .nothing-here-block The file could not be displayed because it is too large. - - elsif blob_text_viewable?(blob) + - elsif blob.readable_text? - if !project.repository.diffable?(blob) .nothing-here-block This diff was suppressed by a .gitattributes entry. - elsif diff_file.collapsed? diff --git a/app/views/projects/diffs/_diffs.html.haml b/app/views/projects/diffs/_diffs.html.haml index 4b49bed835f..71a1b9e6c05 100644 --- a/app/views/projects/diffs/_diffs.html.haml +++ b/app/views/projects/diffs/_diffs.html.haml @@ -27,7 +27,7 @@ - diff_commit = commit_for_diff(diff_file) - blob = diff_file.blob(diff_commit) - next unless blob - - blob.load_all_data!(diffs.project.repository) unless blob.only_display_raw? + - blob.load_all_data!(diffs.project.repository) unless blob.too_large? - file_hash = hexdigest(diff_file.file_path) = render 'projects/diffs/file', file_hash: file_hash, project: diffs.project, diff --git a/app/views/projects/diffs/_file.html.haml b/app/views/projects/diffs/_file.html.haml index 4622b980754..f22b385fc0f 100644 --- a/app/views/projects/diffs/_file.html.haml +++ b/app/views/projects/diffs/_file.html.haml @@ -6,7 +6,7 @@ - unless diff_file.submodule? .file-actions.hidden-xs - - if blob_text_viewable?(blob) + - if blob.readable_text? = link_to '#', class: 'js-toggle-diff-comments btn active has-tooltip', title: "Toggle comments for this file", disabled: @diff_notes_disabled do = icon('comment') \ -- cgit v1.2.1 From 30db305dcedfe23635f9fd779a59746fdd5f2e61 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:13:31 -0500 Subject: Move conditional inside copy_blob_source_button --- app/helpers/blob_helper.rb | 6 +++--- app/views/projects/blob/_header.html.haml | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index e2092b8bfbf..095032c98e5 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -208,10 +208,10 @@ module BlobHelper clipboard_button(text: file_path, gfm: "`#{file_path}`", class: 'btn-clipboard btn-transparent prepend-left-5', title: 'Copy file path to clipboard') end - def copy_blob_content_button(blob) - return if markup?(blob.name) + def copy_blob_source_button(blob) + return unless blob.rendered_as_text?(override_max_size: params[:override_max_size]) - clipboard_button(target: ".blob-content[data-blob-id='#{blob.id}']", class: "btn btn-sm", title: "Copy content to clipboard") + clipboard_button(target: ".blob-content[data-blob-id='#{blob.id}']", class: "btn btn-sm js-copy-blob-source-btn", title: "Copy source to clipboard") end def open_raw_file_button(path) diff --git a/app/views/projects/blob/_header.html.haml b/app/views/projects/blob/_header.html.haml index 81fd8e5792b..bb8a53aca6e 100644 --- a/app/views/projects/blob/_header.html.haml +++ b/app/views/projects/blob/_header.html.haml @@ -13,7 +13,7 @@ .file-actions.hidden-xs .btn-group{ role: "group" }< - = copy_blob_content_button(blob) if !blame && blob_rendered_as_text?(blob) + = copy_blob_source_button(blob) unless blame = open_raw_file_button(namespace_project_raw_path(@project.namespace, @project, @id)) = view_on_environment_button(@commit.sha, @path, @environment) if @environment -- cgit v1.2.1 From 44cf2470a97221955679e4975897667c21ff1727 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:14:15 -0500 Subject: Render blob using blob viewers --- app/views/projects/blob/_blob.html.haml | 7 +------ app/views/projects/blob/_content.html.haml | 8 ++++++++ app/views/projects/blob/_viewer.html.haml | 5 +++++ app/views/projects/blob/_viewer_wrapper.html.haml | 8 ++++++++ 4 files changed, 22 insertions(+), 6 deletions(-) create mode 100644 app/views/projects/blob/_content.html.haml create mode 100644 app/views/projects/blob/_viewer.html.haml create mode 100644 app/views/projects/blob/_viewer_wrapper.html.haml diff --git a/app/views/projects/blob/_blob.html.haml b/app/views/projects/blob/_blob.html.haml index 9aafff343f0..3f12d64d044 100644 --- a/app/views/projects/blob/_blob.html.haml +++ b/app/views/projects/blob/_blob.html.haml @@ -26,9 +26,4 @@ %article.file-holder = render "projects/blob/header", blob: blob - - if blob.empty? - .file-content.code - .nothing-here-block - Empty file - - else - = render blob.to_partial_path(@project), blob: blob + = render 'projects/blob/content', blob: blob diff --git a/app/views/projects/blob/_content.html.haml b/app/views/projects/blob/_content.html.haml new file mode 100644 index 00000000000..7bbc8627a2b --- /dev/null +++ b/app/views/projects/blob/_content.html.haml @@ -0,0 +1,8 @@ +- simple_viewer = blob.simple_viewer +- rich_viewer = blob.rich_viewer +- active_viewer = rich_viewer && params[:viewer] != 'simple' ? :rich : :simple + += render 'projects/blob/viewer_wrapper', viewer: simple_viewer, hidden: (active_viewer != :simple) + +- if rich_viewer + = render 'projects/blob/viewer_wrapper', viewer: rich_viewer, hidden: (active_viewer != :rich) diff --git a/app/views/projects/blob/_viewer.html.haml b/app/views/projects/blob/_viewer.html.haml new file mode 100644 index 00000000000..19f1d1251ae --- /dev/null +++ b/app/views/projects/blob/_viewer.html.haml @@ -0,0 +1,5 @@ +- if error = viewer.render_error(override_max_size: params[:override_max_size]) + = render 'projects/blob/render_error', viewer: viewer, error: error +- else + - viewer.prepare! + = render viewer.partial_path, viewer: viewer diff --git a/app/views/projects/blob/_viewer_wrapper.html.haml b/app/views/projects/blob/_viewer_wrapper.html.haml new file mode 100644 index 00000000000..8da5022fbea --- /dev/null +++ b/app/views/projects/blob/_viewer_wrapper.html.haml @@ -0,0 +1,8 @@ +- hidden = local_assigns.fetch(:hidden, false) +- url = url_for(params.merge(format: :json, viewer: viewer.type)) if viewer.server_side? +.blob-viewer{ data: { type: viewer.type, url: url }, class: ('hidden' if hidden) } + - if viewer.server_side? + .text-center.prepend-top-default.append-bottom-default + = icon('spinner spin 2x', 'aria-hidden' => 'true', 'aria-label' => 'Loading content') + - else + = render 'projects/blob/viewer', viewer: viewer -- cgit v1.2.1 From 21d2ebff6226e5438443d2eb4430736e9659b820 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:08:39 -0500 Subject: Render blob viewer error --- app/helpers/blob_helper.rb | 9 +++++++++ app/views/projects/blob/_render_error.html.haml | 16 ++++++++++++++++ app/views/projects/blob/_too_large.html.haml | 5 ----- 3 files changed, 25 insertions(+), 5 deletions(-) create mode 100644 app/views/projects/blob/_render_error.html.haml delete mode 100644 app/views/projects/blob/_too_large.html.haml diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 095032c98e5..8921f0c444a 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -217,4 +217,13 @@ module BlobHelper def open_raw_file_button(path) link_to icon('file-code-o'), path, class: 'btn btn-sm has-tooltip', target: '_blank', rel: 'noopener noreferrer', title: 'Open raw', data: { container: 'body' } end + + def blob_render_error_reason(viewer, error) + case error + when :too_large + "it is larger than #{number_to_human_size(viewer.relevant_max_size)}" + when :server_side_but_stored_in_lfs + "it is stored in LFS" + end + end end diff --git a/app/views/projects/blob/_render_error.html.haml b/app/views/projects/blob/_render_error.html.haml new file mode 100644 index 00000000000..56bcc2b018c --- /dev/null +++ b/app/views/projects/blob/_render_error.html.haml @@ -0,0 +1,16 @@ +- reason = blob_render_error_reason(viewer, error) + +- options = [] +- if error == :too_large && viewer.can_override_max_size? + - options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil))) +- if viewer.rich? && viewer.blob.rendered_as_text?(override_max_size: true) + - options << link_to('view the source', '#', class: 'js-blob-viewer-switcher', data: { viewer: 'simple' }) +- options << link_to('download it', blob_raw_url, target: '_blank', rel: 'noopener noreferrer') + +.file-content.code + .nothing-here-block + The #{viewer.switcher_title} could not be displayed because #{reason}. + + You can + = options.to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe + instead. diff --git a/app/views/projects/blob/_too_large.html.haml b/app/views/projects/blob/_too_large.html.haml deleted file mode 100644 index a505f87df40..00000000000 --- a/app/views/projects/blob/_too_large.html.haml +++ /dev/null @@ -1,5 +0,0 @@ -.file-content.code - .nothing-here-block - The file could not be displayed as it is too large, you can - #{link_to('view the raw file', namespace_project_raw_path(@project.namespace, @project, @id), target: '_blank', rel: 'noopener noreferrer')} - instead. -- cgit v1.2.1 From ee17c659dc3899802ff3a21ddcab0c943d13dd53 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:13:42 -0500 Subject: Add viewer switcher --- app/views/projects/blob/_header.html.haml | 2 ++ app/views/projects/blob/_viewer_switcher.html.haml | 12 ++++++++++++ 2 files changed, 14 insertions(+) create mode 100644 app/views/projects/blob/_viewer_switcher.html.haml diff --git a/app/views/projects/blob/_header.html.haml b/app/views/projects/blob/_header.html.haml index bb8a53aca6e..ed63c6fc3c9 100644 --- a/app/views/projects/blob/_header.html.haml +++ b/app/views/projects/blob/_header.html.haml @@ -12,6 +12,8 @@ = number_to_human_size(blob.raw_size) .file-actions.hidden-xs + = render 'projects/blob/viewer_switcher', blob: blob unless blame + .btn-group{ role: "group" }< = copy_blob_source_button(blob) unless blame = open_raw_file_button(namespace_project_raw_path(@project.namespace, @project, @id)) diff --git a/app/views/projects/blob/_viewer_switcher.html.haml b/app/views/projects/blob/_viewer_switcher.html.haml new file mode 100644 index 00000000000..6d69660e656 --- /dev/null +++ b/app/views/projects/blob/_viewer_switcher.html.haml @@ -0,0 +1,12 @@ +- if blob.show_viewer_switcher? + - simple_viewer = blob.simple_viewer + - rich_viewer = blob.rich_viewer + + .btn-group{ role: "group" } + - simple_label = "Display #{simple_viewer.switcher_title}" + %button.btn.btn-default.btn-sm.js-blob-viewer-switcher.has-tooltip{ 'aria-label' => simple_label, title: simple_label, data: { viewer: 'simple', container: 'body' } }> + = icon(simple_viewer.switcher_icon) + + - rich_label = "Display #{rich_viewer.switcher_title}" + %button.btn.btn-default.btn-sm.js-blob-viewer-switcher.has-tooltip{ 'aria-label' => rich_label, title: rich_label, data: { viewer: 'rich', container: 'body' } }> + = icon(rich_viewer.switcher_icon) -- cgit v1.2.1 From 121c5f66180acd78cedf18b19b0e86ee5bf1204d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:10:21 -0500 Subject: Add JSON endpoint to get simple blob viewer --- app/controllers/concerns/renders_blob.rb | 17 +++++++++++++++++ app/controllers/projects/blob_controller.rb | 15 +++++++++++++-- 2 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 app/controllers/concerns/renders_blob.rb diff --git a/app/controllers/concerns/renders_blob.rb b/app/controllers/concerns/renders_blob.rb new file mode 100644 index 00000000000..ab9f0d01c7b --- /dev/null +++ b/app/controllers/concerns/renders_blob.rb @@ -0,0 +1,17 @@ +module RendersBlob + extend ActiveSupport::Concern + + def render_blob_json(blob) + viewer = + if params[:viewer] == 'rich' + blob.rich_viewer + else + blob.simple_viewer + end + return render_404 unless viewer + + render json: { + html: view_to_html_string("projects/blob/_viewer", viewer: viewer) + } + end +end diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 4c6db91d7c0..173c6bcee53 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -2,6 +2,7 @@ class Projects::BlobController < Projects::ApplicationController include ExtractsPath include CreatesCommit + include RendersBlob include ActionView::Helpers::SanitizeHelper # Raised when given an invalid file path @@ -34,8 +35,18 @@ class Projects::BlobController < Projects::ApplicationController end def show - environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit } - @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last + respond_to do |format| + format.html do + environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit } + @environment = EnvironmentsFinder.new(@project, current_user, environment_params).execute.last + + render 'show' + end + + format.json do + render_blob_json(@blob) + end + end end def edit -- cgit v1.2.1 From 3079a2d63b54377c01ce5d09b96b3d7db20462aa Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 11:49:54 -0500 Subject: Allow LineHighlighter to be triggered by event --- app/assets/javascripts/line_highlighter.js | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/app/assets/javascripts/line_highlighter.js b/app/assets/javascripts/line_highlighter.js index 1821ca18053..5ffa6283a03 100644 --- a/app/assets/javascripts/line_highlighter.js +++ b/app/assets/javascripts/line_highlighter.js @@ -41,7 +41,6 @@ require('vendor/jquery.scrollTo'); LineHighlighter.prototype._hash = ''; function LineHighlighter(hash) { - var range; if (hash == null) { // Initialize a LineHighlighter object // @@ -51,10 +50,21 @@ require('vendor/jquery.scrollTo'); this.setHash = bind(this.setHash, this); this.highlightLine = bind(this.highlightLine, this); this.clickHandler = bind(this.clickHandler, this); + this.highlightHash = this.highlightHash.bind(this); this._hash = hash; this.bindEvents(); - if (hash !== '') { - range = this.hashToRange(hash); + this.highlightHash(); + } + + LineHighlighter.prototype.bindEvents = function() { + $('#blob-content-holder').on('click', 'a[data-line-number]', this.clickHandler); + $('#blob-content-holder').on('highlight:line', this.highlightHash); + }; + + LineHighlighter.prototype.highlightHash = function() { + var range; + if (this._hash !== '') { + range = this.hashToRange(this._hash); if (range[0]) { this.highlightRange(range); $.scrollTo("#L" + range[0], { @@ -64,10 +74,6 @@ require('vendor/jquery.scrollTo'); }); } } - } - - LineHighlighter.prototype.bindEvents = function() { - $('#blob-content-holder').on('click', 'a[data-line-number]', this.clickHandler); }; LineHighlighter.prototype.clickHandler = function(event) { -- cgit v1.2.1 From 796acbe1e05f934c02bc0ca9fca1747aab49267c Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 12:05:09 -0500 Subject: Add BlobViewer JS --- app/assets/javascripts/blob/viewer/index.js | 130 ++++++++++++++++++++++++++++ app/assets/javascripts/dispatcher.js | 2 + app/views/projects/blob/show.html.haml | 3 + 3 files changed, 135 insertions(+) create mode 100644 app/assets/javascripts/blob/viewer/index.js diff --git a/app/assets/javascripts/blob/viewer/index.js b/app/assets/javascripts/blob/viewer/index.js new file mode 100644 index 00000000000..47c20d60446 --- /dev/null +++ b/app/assets/javascripts/blob/viewer/index.js @@ -0,0 +1,130 @@ +``/* eslint-disable no-new */ +/* global Flash */ +export default class BlobViewer { + constructor() { + this.switcherBtns = document.querySelectorAll('.js-blob-viewer-switcher'); + this.copySourceBtn = document.querySelector('.js-copy-blob-source-btn'); + this.simpleViewer = document.querySelector('.blob-viewer[data-type="simple"]'); + this.richViewer = document.querySelector('.blob-viewer[data-type="rich"]'); + this.$blobContentHolder = $('#blob-content-holder'); + + let initialViewerName = document.querySelector('.blob-viewer:not(.hidden)').getAttribute('data-type'); + + if (this.switcherBtns.length) { + this.initBindings(); + + if (location.hash.indexOf('#L') === 0) { + initialViewerName = 'simple'; + } + } + + this.switchToViewer(initialViewerName); + } + + initBindings() { + Array.from(this.switcherBtns) + .forEach((el) => { + el.addEventListener('click', this.switchViewHandler.bind(this)); + }); + + if (this.copySourceBtn) { + this.copySourceBtn.addEventListener('click', () => { + if (this.copySourceBtn.classList.contains('disabled')) return; + + this.switchToViewer('simple'); + }); + } + } + + switchViewHandler(e) { + const target = e.currentTarget; + + e.preventDefault(); + + this.switchToViewer(target.getAttribute('data-viewer')); + } + + toggleCopyButtonState() { + if (!this.copySourceBtn) return; + + if (this.simpleViewer.getAttribute('data-loaded')) { + this.copySourceBtn.setAttribute('title', 'Copy source to clipboard'); + this.copySourceBtn.classList.remove('disabled'); + } else if (this.activeViewer == this.simpleViewer) { + this.copySourceBtn.setAttribute('title', 'Wait for the source to load to copy it to the clipboard'); + this.copySourceBtn.classList.add('disabled'); + } else { + this.copySourceBtn.setAttribute('title', 'Switch to the source to copy it to the clipboard'); + this.copySourceBtn.classList.add('disabled'); + } + + $(this.copySourceBtn).tooltip('fixTitle'); + } + + loadViewer(viewerParam, resolve, reject) { + const viewer = viewerParam; + const url = viewer.getAttribute('data-url'); + + if (!url || viewer.getAttribute('data-loaded') || viewer.getAttribute('data-loading')) { + if (resolve) resolve(); + return; + } + + viewer.setAttribute('data-loading', 'true'); + + $.ajax({ + url, + dataType: 'JSON', + }) + .fail(() => { + if (reject) reject(); + }) + .done((data) => { + viewer.innerHTML = data.html; + $(viewer).syntaxHighlight(); + + viewer.setAttribute('data-loaded', 'true'); + + this.$blobContentHolder.trigger('highlight:line'); + + this.toggleCopyButtonState(); + + if (resolve) resolve(); + }); + } + + switchToViewer(name) { + const newViewer = document.querySelector(`.blob-viewer[data-type='${name}']`); + if (this.activeViewer == newViewer) return; + + const oldButton = document.querySelector('.js-blob-viewer-switcher.active') + const newButton = document.querySelector(`.js-blob-viewer-switcher[data-viewer='${name}']`); + const oldViewer = document.querySelector(`.blob-viewer:not([data-type='${name}'])`); + + if (oldButton) { + oldButton.classList.remove('active'); + } + + if (newButton) { + newButton.classList.add('active'); + newButton.blur(); + } + + if (oldViewer) { + oldViewer.classList.add('hidden'); + } + + newViewer.classList.remove('hidden'); + + this.activeViewer = newViewer; + + this.toggleCopyButtonState(); + + return new Promise((resolve, reject) => { + this.loadViewer(newViewer, resolve, reject); + }) + .catch(() => { + new Flash('Error loading file'); + }); + } +} diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index b20673cd03c..d3d75c4bf4a 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -48,6 +48,7 @@ import BlobForkSuggestion from './blob/blob_fork_suggestion'; import UserCallout from './user_callout'; import { ProtectedTagCreate, ProtectedTagEditList } from './protected_tags'; import ShortcutsWiki from './shortcuts_wiki'; +import BlobViewer from './blob/viewer/index'; const ShortcutsBlob = require('./shortcuts_blob'); @@ -299,6 +300,7 @@ const ShortcutsBlob = require('./shortcuts_blob'); gl.TargetBranchDropDown.bootstrap(); break; case 'projects:blob:show': + new BlobViewer(); gl.TargetBranchDropDown.bootstrap(); initBlob(); break; diff --git a/app/views/projects/blob/show.html.haml b/app/views/projects/blob/show.html.haml index b9b3f3ec7a3..67f57b5e4b9 100644 --- a/app/views/projects/blob/show.html.haml +++ b/app/views/projects/blob/show.html.haml @@ -2,6 +2,9 @@ - page_title @blob.path, @ref = render "projects/commits/head" +- content_for :page_specific_javascripts do + = page_specific_javascript_bundle_tag('blob') + %div{ class: container_class } = render 'projects/last_push' -- cgit v1.2.1 From 7cc4546bc97316e40da367c3894b5e4cf7a155ea Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 13 Apr 2017 11:50:40 -0500 Subject: Add specs --- spec/features/projects/blobs/blob_show_spec.rb | 55 +++++++++--- spec/javascripts/blob/viewer/index_spec.js | 120 +++++++++++++++++++++++++ spec/javascripts/fixtures/blob.rb | 29 ++++++ 3 files changed, 191 insertions(+), 13 deletions(-) create mode 100644 spec/javascripts/blob/viewer/index_spec.js create mode 100644 spec/javascripts/fixtures/blob.rb diff --git a/spec/features/projects/blobs/blob_show_spec.rb b/spec/features/projects/blobs/blob_show_spec.rb index 7cfa5b9716f..9fbb17b3707 100644 --- a/spec/features/projects/blobs/blob_show_spec.rb +++ b/spec/features/projects/blobs/blob_show_spec.rb @@ -4,19 +4,48 @@ feature 'File blob', feature: true do include TreeHelper let(:project) { create(:project, :public, :test_repo) } - let(:merge_request) { create(:merge_request, source_project: project, source_branch: 'feature', target_branch: 'master') } - let(:branch) { 'master' } - let(:file_path) { project.repository.ls_files(project.repository.root_ref)[1] } - - context 'anonymous' do - context 'from blob file path' do - before do - visit namespace_project_blob_path(project.namespace, project, tree_join(branch, file_path)) - end - - it 'updates content' do - expect(page).to have_link 'Edit' - end + + def visit_blob(path, fragment = nil) + visit namespace_project_blob_path(project.namespace, project, tree_join('master', path), anchor: fragment) + end + + context 'text files' do + it 'shows rendered output for SVG' do + visit_blob('files/images/wm.svg') + + expect(page).to have_selector('.blob-viewer[data-type="rich"]') + end + + it 'switches to code view' do + visit_blob('files/images/wm.svg') + + first('.js-blob-viewer-switcher').click + + expect(page).to have_selector('.blob-viewer[data-type="rich"]', visible: false) + expect(page).to have_selector('.blob-viewer[data-type="simple"]') + end + + it 'opens raw mode when linking to a line in SVG file' do + visit_blob('files/images/wm.svg', 'L1') + + expect(page).to have_selector('#LC1.hll') + expect(page).to have_selector('.blob-viewer[data-type="simple"]') + end + + it 'opens raw mode when linking to a line in MD file' do + visit_blob('README.md', 'L1') + + expect(page).to have_selector('#LC1.hll') + expect(page).to have_selector('.blob-viewer[data-type="simple"]') + end + end + + context 'binary files' do + it 'does not show view toggle buttons in toolbar' do + visit_blob('Gemfile.zip') + + expect(first('.file-actions .btn-group')).to have_selector('.btn', count: 1) + expect(first('.file-actions .btn-group .btn')[:title]).to eq('Download blob') end end end diff --git a/spec/javascripts/blob/viewer/index_spec.js b/spec/javascripts/blob/viewer/index_spec.js new file mode 100644 index 00000000000..c95a40689c7 --- /dev/null +++ b/spec/javascripts/blob/viewer/index_spec.js @@ -0,0 +1,120 @@ +/* eslint-disable no-new */ +import BlobViewer from '~/blob/viewer/index'; + +describe('Blob viewer', () => { + preloadFixtures('blob/show.html.raw'); + + beforeEach(() => { + loadFixtures('blob/show.html.raw'); + $('#modal-upload-blob').remove(); + + new BlobViewer(); + + spyOn($, 'ajax').and.callFake(() => { + const d = $.Deferred(); + + d.resolve({ + html: '
testing
', + }); + + return d.promise(); + }); + }); + + afterEach(() => { + location.hash = ''; + }); + + it('loads source file after switching views', (done) => { + document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]').click(); + + setTimeout(() => { + expect($.ajax).toHaveBeenCalled(); + expect( + document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]') + .classList.contains('hidden'), + ).toBeFalsy(); + + done(); + }); + }); + + it('loads source file when line number is in hash', (done) => { + location.hash = '#L1'; + + new BlobViewer(); + + setTimeout(() => { + expect($.ajax).toHaveBeenCalled(); + expect( + document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]') + .classList.contains('hidden'), + ).toBeFalsy(); + + done(); + }); + }); + + it('doesnt reload file if already loaded', (done) => { + const asyncClick = () => new Promise((resolve) => { + document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]').click(); + + setTimeout(resolve); + }); + + asyncClick() + .then(() => { + expect($.ajax).toHaveBeenCalled(); + return asyncClick(); + }) + .then(() => { + expect($.ajax.calls.count()).toBe(1); + expect( + document.querySelector('.blob-viewer[data-type="simple"]').getAttribute('data-loaded'), + ).toBe('true'); + + done(); + }); + }); + + describe('copy blob button', () => { + it('disabled on load', () => { + expect( + document.querySelector('.js-copy-blob-source-btn').classList.contains('disabled'), + ).toBeTruthy(); + }); + + it('has tooltip when disabled', () => { + expect( + document.querySelector('.js-copy-blob-source-btn').getAttribute('data-original-title'), + ).toBe('Switch to the source view to copy the source to the clipboard'); + }); + + it('enables after switching to simple view', (done) => { + document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]').click(); + + setTimeout(() => { + expect($.ajax).toHaveBeenCalled(); + expect( + document.querySelector('.js-copy-blob-source-btn').classList.contains('disabled'), + ).toBeFalsy(); + + done(); + }); + }); + + it('updates tooltip after switching to simple view', (done) => { + document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]').click(); + + setTimeout(() => { + expect($.ajax).toHaveBeenCalled(); + + expect( + document.querySelector('.js-copy-blob-source-btn').getAttribute('data-original-title'), + ).toBe('Copy to clipboard'); + + done(); + }); + }); + }); +}); diff --git a/spec/javascripts/fixtures/blob.rb b/spec/javascripts/fixtures/blob.rb new file mode 100644 index 00000000000..16490ad5039 --- /dev/null +++ b/spec/javascripts/fixtures/blob.rb @@ -0,0 +1,29 @@ +require 'spec_helper' + +describe Projects::BlobController, '(JavaScript fixtures)', type: :controller do + include JavaScriptFixturesHelpers + + let(:admin) { create(:admin) } + let(:namespace) { create(:namespace, name: 'frontend-fixtures' )} + let(:project) { create(:project, :repository, namespace: namespace, path: 'branches-project') } + + render_views + + before(:all) do + clean_frontend_fixtures('blob/') + end + + before(:each) do + sign_in(admin) + end + + it 'blob/show.html.raw' do |example| + get(:show, + namespace_id: project.namespace, + project_id: project, + id: 'add-ipython-files/files/ipython/basic.ipynb') + + expect(response).to be_success + store_frontend_fixture(response, example.description) + end +end -- cgit v1.2.1 From 36e7b322a765fa29a764f2ef1c2f0d28b27a569e Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 20 Apr 2017 11:48:19 -0500 Subject: Download blob viewer is client-side --- app/models/blob_viewer/base.rb | 2 +- app/models/blob_viewer/download.rb | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index 37acacc0019..ce4f129232d 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -75,7 +75,7 @@ module BlobViewer end def server_side_but_stored_in_lfs? - !client_side? && blob.valid_lfs_pointer? + server_side? && blob.valid_lfs_pointer? end end end diff --git a/app/models/blob_viewer/download.rb b/app/models/blob_viewer/download.rb index 45cb38a3268..e406cfc05f7 100644 --- a/app/models/blob_viewer/download.rb +++ b/app/models/blob_viewer/download.rb @@ -1,7 +1,9 @@ module BlobViewer class Download < Base include Simple - include ServerSide + # We pretend the Download viewer is rendered client-side so that it doesn't + # attempt to load the entire blob contents. + include ClientSide self.partial_name = 'download' self.text_based = false -- cgit v1.2.1 From cc3e488c984abe220cbecb35b61af5b04d93f90d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 20 Apr 2017 17:56:24 -0500 Subject: Satisfy eslint --- app/assets/javascripts/blob/viewer/index.js | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/blob/viewer/index.js b/app/assets/javascripts/blob/viewer/index.js index 47c20d60446..a322351219c 100644 --- a/app/assets/javascripts/blob/viewer/index.js +++ b/app/assets/javascripts/blob/viewer/index.js @@ -1,4 +1,4 @@ -``/* eslint-disable no-new */ +/* eslint-disable no-new */ /* global Flash */ export default class BlobViewer { constructor() { @@ -50,7 +50,7 @@ export default class BlobViewer { if (this.simpleViewer.getAttribute('data-loaded')) { this.copySourceBtn.setAttribute('title', 'Copy source to clipboard'); this.copySourceBtn.classList.remove('disabled'); - } else if (this.activeViewer == this.simpleViewer) { + } else if (this.activeViewer === this.simpleViewer) { this.copySourceBtn.setAttribute('title', 'Wait for the source to load to copy it to the clipboard'); this.copySourceBtn.classList.add('disabled'); } else { @@ -61,12 +61,11 @@ export default class BlobViewer { $(this.copySourceBtn).tooltip('fixTitle'); } - loadViewer(viewerParam, resolve, reject) { + loadViewer(viewerParam) { const viewer = viewerParam; const url = viewer.getAttribute('data-url'); if (!url || viewer.getAttribute('data-loaded') || viewer.getAttribute('data-loading')) { - if (resolve) resolve(); return; } @@ -76,9 +75,6 @@ export default class BlobViewer { url, dataType: 'JSON', }) - .fail(() => { - if (reject) reject(); - }) .done((data) => { viewer.innerHTML = data.html; $(viewer).syntaxHighlight(); @@ -88,16 +84,14 @@ export default class BlobViewer { this.$blobContentHolder.trigger('highlight:line'); this.toggleCopyButtonState(); - - if (resolve) resolve(); }); } switchToViewer(name) { const newViewer = document.querySelector(`.blob-viewer[data-type='${name}']`); - if (this.activeViewer == newViewer) return; + if (this.activeViewer === newViewer) return; - const oldButton = document.querySelector('.js-blob-viewer-switcher.active') + const oldButton = document.querySelector('.js-blob-viewer-switcher.active'); const newButton = document.querySelector(`.js-blob-viewer-switcher[data-viewer='${name}']`); const oldViewer = document.querySelector(`.blob-viewer:not([data-type='${name}'])`); @@ -113,18 +107,13 @@ export default class BlobViewer { if (oldViewer) { oldViewer.classList.add('hidden'); } - + newViewer.classList.remove('hidden'); this.activeViewer = newViewer; this.toggleCopyButtonState(); - return new Promise((resolve, reject) => { - this.loadViewer(newViewer, resolve, reject); - }) - .catch(() => { - new Flash('Error loading file'); - }); + this.loadViewer(newViewer); } } -- cgit v1.2.1 From a6536a84e75952ffba7bb5cea07673963d820766 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 20 Apr 2017 18:01:47 -0500 Subject: Fix some specs --- app/helpers/blob_helper.rb | 2 -- app/views/projects/blob/_header.html.haml | 2 +- app/views/shared/snippets/_blob.html.haml | 2 +- spec/features/projects/files/browse_files_spec.rb | 5 +++-- spec/helpers/blob_helper_spec.rb | 5 ++--- 5 files changed, 7 insertions(+), 9 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 8921f0c444a..0e369a63c87 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -209,8 +209,6 @@ module BlobHelper end def copy_blob_source_button(blob) - return unless blob.rendered_as_text?(override_max_size: params[:override_max_size]) - clipboard_button(target: ".blob-content[data-blob-id='#{blob.id}']", class: "btn btn-sm js-copy-blob-source-btn", title: "Copy source to clipboard") end diff --git a/app/views/projects/blob/_header.html.haml b/app/views/projects/blob/_header.html.haml index ed63c6fc3c9..19eca2984db 100644 --- a/app/views/projects/blob/_header.html.haml +++ b/app/views/projects/blob/_header.html.haml @@ -15,7 +15,7 @@ = render 'projects/blob/viewer_switcher', blob: blob unless blame .btn-group{ role: "group" }< - = copy_blob_source_button(blob) unless blame + = copy_blob_source_button(blob) if !blame && blob.rendered_as_text?(override_max_size: params[:override_max_size]) = open_raw_file_button(namespace_project_raw_path(@project.namespace, @project, @id)) = view_on_environment_button(@commit.sha, @path, @environment) if @environment diff --git a/app/views/shared/snippets/_blob.html.haml b/app/views/shared/snippets/_blob.html.haml index 895c3f1e99d..37c66ff2595 100644 --- a/app/views/shared/snippets/_blob.html.haml +++ b/app/views/shared/snippets/_blob.html.haml @@ -9,7 +9,7 @@ .file-actions.hidden-xs .btn-group{ role: "group" }< - = copy_blob_content_button(@snippet) + = copy_blob_source_button(@snippet) = open_raw_file_button(raw_path) - if defined?(download_path) && download_path diff --git a/spec/features/projects/files/browse_files_spec.rb b/spec/features/projects/files/browse_files_spec.rb index d281043caa3..70e96efd557 100644 --- a/spec/features/projects/files/browse_files_spec.rb +++ b/spec/features/projects/files/browse_files_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -feature 'user browses project', feature: true do +feature 'user browses project', feature: true, js: true do let(:project) { create(:project) } let(:user) { create(:user) } @@ -13,7 +13,7 @@ feature 'user browses project', feature: true do scenario "can see blame of '.gitignore'" do click_link ".gitignore" click_link 'Blame' - + expect(page).to have_content "*.rb" expect(page).to have_content "Dmitriy Zaporozhets" expect(page).to have_content "Initial commit" @@ -24,6 +24,7 @@ feature 'user browses project', feature: true do click_link 'files' click_link 'lfs' click_link 'lfs_object.iso' + wait_for_ajax expect(page).not_to have_content 'Download (1.5 MB)' expect(page).to have_content 'version https://git-lfs.github.com/spec/v1' diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index 508aeb7cf67..292bc0d68fd 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/spec/helpers/blob_helper_spec.rb @@ -56,15 +56,14 @@ describe BlobHelper do end end - describe "#sanitize_svg" do + describe "#sanitize_svg_data" do let(:input_svg_path) { File.join(Rails.root, 'spec', 'fixtures', 'unsanitized.svg') } let(:data) { open(input_svg_path).read } let(:expected_svg_path) { File.join(Rails.root, 'spec', 'fixtures', 'sanitized.svg') } let(:expected) { open(expected_svg_path).read } it 'retains essential elements' do - blob = OpenStruct.new(data: data) - expect(sanitize_svg(blob).data).to eq(expected) + expect(sanitize_svg_data(data)).to eq(expected) end end -- cgit v1.2.1 From 0cfb38194c32547b0e6cbcbfcc06f96f2ec05ffb Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 20 Apr 2017 18:27:03 -0500 Subject: Fix feature specs --- features/project/source/browse_files.feature | 3 ++- features/project/source/markdown_render.feature | 12 ++++++++++++ features/steps/project/source/browse_files.rb | 3 +++ features/steps/project/source/markdown_render.rb | 10 ++++++++++ features/steps/shared/markdown.rb | 2 +- 5 files changed, 28 insertions(+), 2 deletions(-) diff --git a/features/project/source/browse_files.feature b/features/project/source/browse_files.feature index d81bc9802bc..472ec9544f3 100644 --- a/features/project/source/browse_files.feature +++ b/features/project/source/browse_files.feature @@ -10,7 +10,8 @@ Feature: Project Source Browse Files Scenario: I browse files for specific ref Given I visit project source page for "6d39438" Then I should see files from repository for "6d39438" - + + @javascript Scenario: I browse file content Given I click on ".gitignore" file in repo Then I should see its content diff --git a/features/project/source/markdown_render.feature b/features/project/source/markdown_render.feature index ecbd721c281..fd583618dcf 100644 --- a/features/project/source/markdown_render.feature +++ b/features/project/source/markdown_render.feature @@ -6,11 +6,13 @@ Feature: Project Source Markdown Render # Tree README + @javascript Scenario: Tree view should have correct links in README Given I go directory which contains README file And I click on a relative link in README Then I should see the correct markdown + @javascript Scenario: I browse files from markdown branch Then I should see files from repository in markdown And I should see rendered README which contains correct links @@ -29,36 +31,42 @@ Feature: Project Source Markdown Render And I click on GitLab API doc directory in README Then I should see correct doc/api directory rendered + @javascript Scenario: I view README in markdown branch to see reference links to file Then I should see files from repository in markdown And I should see rendered README which contains correct links And I click on Maintenance in README Then I should see correct maintenance file rendered + @javascript Scenario: README headers should have header links Then I should see rendered README which contains correct links And Header "Application details" should have correct id and link # Blob + @javascript Scenario: I navigate to doc directory to view documentation in markdown And I navigate to the doc/api/README And I see correct file rendered And I click on users in doc/api/README Then I should see the correct document file + @javascript Scenario: I navigate to doc directory to view user doc in markdown And I navigate to the doc/api/README And I see correct file rendered And I click on raketasks in doc/api/README Then I should see correct directory rendered + @javascript Scenario: I navigate to doc directory to view user doc in markdown And I navigate to the doc/api/README And Header "GitLab API" should have correct id and link # Markdown branch + @javascript Scenario: I browse files from markdown branch When I visit markdown branch Then I should see files from repository in markdown branch @@ -73,6 +81,7 @@ Feature: Project Source Markdown Render And I click on Rake tasks in README Then I should see correct directory rendered for markdown branch + @javascript Scenario: I navigate to doc directory to view documentation in markdown branch When I visit markdown branch And I navigate to the doc/api/README @@ -80,6 +89,7 @@ Feature: Project Source Markdown Render And I click on users in doc/api/README Then I should see the users document file in markdown branch + @javascript Scenario: I navigate to doc directory to view user doc in markdown branch When I visit markdown branch And I navigate to the doc/api/README @@ -87,6 +97,7 @@ Feature: Project Source Markdown Render And I click on raketasks in doc/api/README Then I should see correct directory rendered for markdown branch + @javascript Scenario: Tree markdown links view empty urls should have correct urls When I visit markdown branch Then The link with text "empty" should have url "tree/markdown" @@ -99,6 +110,7 @@ Feature: Project Source Markdown Render # "ID" means "#id" on the tests below, because we are unable to escape the hash sign. # which Spinach interprets as the start of a comment. + @javascript Scenario: All markdown links with ids should have correct urls When I visit markdown branch Then The link with text "ID" should have url "tree/markdownID" diff --git a/features/steps/project/source/browse_files.rb b/features/steps/project/source/browse_files.rb index b4741f06d1b..36fe21a047c 100644 --- a/features/steps/project/source/browse_files.rb +++ b/features/steps/project/source/browse_files.rb @@ -4,6 +4,7 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps include SharedProject include SharedPaths include RepoHelpers + include WaitForAjax step "I don't have write access" do @project = create(:project, :repository, name: "Other Project", path: "other-project") @@ -36,10 +37,12 @@ class Spinach::Features::ProjectSourceBrowseFiles < Spinach::FeatureSteps end step 'I should see its content' do + wait_for_ajax expect(page).to have_content old_gitignore_content end step 'I should see its new content' do + wait_for_ajax expect(page).to have_content new_gitignore_content end diff --git a/features/steps/project/source/markdown_render.rb b/features/steps/project/source/markdown_render.rb index 0f0827f0477..abdbd795cd5 100644 --- a/features/steps/project/source/markdown_render.rb +++ b/features/steps/project/source/markdown_render.rb @@ -5,6 +5,7 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps include SharedAuthentication include SharedPaths include SharedMarkdown + include WaitForAjax step 'I own project "Delta"' do @project = ::Project.find_by(name: "Delta") @@ -34,6 +35,7 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps step 'I should see correct document rendered' do expect(current_path).to eq namespace_project_blob_path(@project.namespace, @project, "markdown/doc/api/README.md") + wait_for_ajax expect(page).to have_content "All API requests require authentication" end @@ -63,6 +65,7 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps step 'I should see correct maintenance file rendered' do expect(current_path).to eq namespace_project_blob_path(@project.namespace, @project, "markdown/doc/raketasks/maintenance.md") + wait_for_ajax expect(page).to have_content "bundle exec rake gitlab:env:info RAILS_ENV=production" end @@ -94,6 +97,7 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps step 'I see correct file rendered' do expect(current_path).to eq namespace_project_blob_path(@project.namespace, @project, "markdown/doc/api/README.md") + wait_for_ajax expect(page).to have_content "Contents" expect(page).to have_link "Users" expect(page).to have_link "Rake tasks" @@ -138,6 +142,7 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps step 'I see correct file rendered in markdown branch' do expect(current_path).to eq namespace_project_blob_path(@project.namespace, @project, "markdown/doc/api/README.md") + wait_for_ajax expect(page).to have_content "Contents" expect(page).to have_link "Users" expect(page).to have_link "Rake tasks" @@ -145,6 +150,7 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps step 'I should see correct document rendered for markdown branch' do expect(current_path).to eq namespace_project_blob_path(@project.namespace, @project, "markdown/doc/api/README.md") + wait_for_ajax expect(page).to have_content "All API requests require authentication" end @@ -162,6 +168,7 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps # Expected link contents step 'The link with text "empty" should have url "tree/markdown"' do + wait_for_ajax find('a', text: /^empty$/)['href'] == current_host + namespace_project_tree_path(@project.namespace, @project, "markdown") end @@ -197,6 +204,7 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps end step 'The link with text "ID" should have url "blob/markdown/README.mdID"' do + wait_for_ajax find('a', text: /^#id$/)['href'] == current_host + namespace_project_blob_path(@project.namespace, @project, "markdown/README.md") + '#id' end @@ -291,10 +299,12 @@ class Spinach::Features::ProjectSourceMarkdownRender < Spinach::FeatureSteps step 'I should see the correct markdown' do expect(current_path).to eq namespace_project_blob_path(@project.namespace, @project, "markdown/doc/api/users.md") + wait_for_ajax expect(page).to have_content "List users" end step 'Header "Application details" should have correct id and link' do + wait_for_ajax header_should_have_correct_id_and_link(2, 'Application details', 'application-details') end diff --git a/features/steps/shared/markdown.rb b/features/steps/shared/markdown.rb index 875d27d9383..6610b97ecb2 100644 --- a/features/steps/shared/markdown.rb +++ b/features/steps/shared/markdown.rb @@ -3,7 +3,7 @@ module SharedMarkdown def header_should_have_correct_id_and_link(level, text, id, parent = ".wiki") node = find("#{parent} h#{level} a#user-content-#{id}") - expect(node[:href]).to eq "##{id}" + expect(node[:href]).to end_with "##{id}" # Work around a weird Capybara behavior where calling `parent` on a node # returns the whole document, not the node's actual parent element -- cgit v1.2.1 From fed9dcd9ed2f064e887332b4e45f2e65465e74c0 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 20 Apr 2017 18:34:57 -0500 Subject: Add test stubs --- app/models/blob.rb | 4 +- spec/controllers/concerns/renders_blob_spec.rb | 5 + spec/models/blob_spec.rb | 192 +-------------------- spec/models/blob_viewer/base_spec.rb | 5 + .../projects/blob/_render_error.html.haml_spec.rb | 5 + spec/views/projects/blob/_viewer.html.haml_spoc.rb | 5 + .../blob/_viewer_switcher.html.haml_spoc.rb | 5 + .../blob/_viewer_wrapper.html.haml_spoc.rb | 5 + 8 files changed, 33 insertions(+), 193 deletions(-) create mode 100644 spec/controllers/concerns/renders_blob_spec.rb create mode 100644 spec/models/blob_viewer/base_spec.rb create mode 100644 spec/views/projects/blob/_render_error.html.haml_spec.rb create mode 100644 spec/views/projects/blob/_viewer.html.haml_spoc.rb create mode 100644 spec/views/projects/blob/_viewer_switcher.html.haml_spoc.rb create mode 100644 spec/views/projects/blob/_viewer_wrapper.html.haml_spoc.rb diff --git a/app/models/blob.rb b/app/models/blob.rb index 26ee2c883a0..1bb9ed03c11 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -28,13 +28,13 @@ class Blob < SimpleDelegator # # blob = Blob.decorate(nil) # puts "truthy" if blob # No output - def self.decorate(blob, project) + def self.decorate(blob, project = nil) return if blob.nil? new(blob, project) end - def initialize(blob, project) + def initialize(blob, project = nil) @project = project super(blob) diff --git a/spec/controllers/concerns/renders_blob_spec.rb b/spec/controllers/concerns/renders_blob_spec.rb new file mode 100644 index 00000000000..5f07afc5479 --- /dev/null +++ b/spec/controllers/concerns/renders_blob_spec.rb @@ -0,0 +1,5 @@ +include 'spec_helper' + +describe Projects::BlobController, RendersBlob, model: true do + # TODO: Test +end diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb index e5dd57fc4bb..fd42a87c37e 100644 --- a/spec/models/blob_spec.rb +++ b/spec/models/blob_spec.rb @@ -27,195 +27,5 @@ describe Blob do end end - describe '#svg?' do - it 'is falsey when not text' do - git_blob = double(text?: false) - - expect(described_class.decorate(git_blob)).not_to be_svg - end - - it 'is falsey when no language is detected' do - git_blob = double(text?: true, language: nil) - - expect(described_class.decorate(git_blob)).not_to be_svg - end - - it' is falsey when language is not SVG' do - git_blob = double(text?: true, language: double(name: 'XML')) - - expect(described_class.decorate(git_blob)).not_to be_svg - end - - it 'is truthy when language is SVG' do - git_blob = double(text?: true, language: double(name: 'SVG')) - - expect(described_class.decorate(git_blob)).to be_svg - end - end - - describe '#pdf?' do - it 'is falsey when file extension is not .pdf' do - git_blob = Gitlab::Git::Blob.new(name: 'git_blob.txt') - - expect(described_class.decorate(git_blob)).not_to be_pdf - end - - it 'is truthy when file extension is .pdf' do - git_blob = Gitlab::Git::Blob.new(name: 'git_blob.pdf') - - expect(described_class.decorate(git_blob)).to be_pdf - end - end - - describe '#ipython_notebook?' do - it 'is falsey when language is not Jupyter Notebook' do - git_blob = double(text?: true, language: double(name: 'JSON')) - - expect(described_class.decorate(git_blob)).not_to be_ipython_notebook - end - - it 'is truthy when language is Jupyter Notebook' do - git_blob = double(text?: true, language: double(name: 'Jupyter Notebook')) - - expect(described_class.decorate(git_blob)).to be_ipython_notebook - end - end - - describe '#sketch?' do - it 'is falsey with image extension' do - git_blob = Gitlab::Git::Blob.new(name: "design.png") - - expect(described_class.decorate(git_blob)).not_to be_sketch - end - - it 'is truthy with sketch extension' do - git_blob = Gitlab::Git::Blob.new(name: "design.sketch") - - expect(described_class.decorate(git_blob)).to be_sketch - end - end - - describe '#video?' do - it 'is falsey with image extension' do - git_blob = Gitlab::Git::Blob.new(name: 'image.png') - - expect(described_class.decorate(git_blob)).not_to be_video - end - - UploaderHelper::VIDEO_EXT.each do |ext| - it "is truthy when extension is .#{ext}" do - git_blob = Gitlab::Git::Blob.new(name: "video.#{ext}") - - expect(described_class.decorate(git_blob)).to be_video - end - end - end - - describe '#stl?' do - it 'is falsey with image extension' do - git_blob = Gitlab::Git::Blob.new(name: 'file.png') - - expect(described_class.decorate(git_blob)).not_to be_stl - end - - it 'is truthy with STL extension' do - git_blob = Gitlab::Git::Blob.new(name: 'file.stl') - - expect(described_class.decorate(git_blob)).to be_stl - end - end - - describe '#to_partial_path' do - let(:project) { double(lfs_enabled?: true) } - - def stubbed_blob(overrides = {}) - overrides.reverse_merge!( - name: nil, - image?: false, - language: nil, - lfs_pointer?: false, - svg?: false, - text?: false, - binary?: false, - stl?: false - ) - - described_class.decorate(Gitlab::Git::Blob.new({})).tap do |blob| - allow(blob).to receive_messages(overrides) - end - end - - it 'handles LFS pointers with LFS enabled' do - blob = stubbed_blob(lfs_pointer?: true, text?: true) - expect(blob.to_partial_path(project)).to eq 'download' - end - - it 'handles LFS pointers with LFS disabled' do - blob = stubbed_blob(lfs_pointer?: true, text?: true) - project = double(lfs_enabled?: false) - expect(blob.to_partial_path(project)).to eq 'text' - end - - it 'handles SVGs' do - blob = stubbed_blob(text?: true, svg?: true) - expect(blob.to_partial_path(project)).to eq 'svg' - end - - it 'handles images' do - blob = stubbed_blob(image?: true) - expect(blob.to_partial_path(project)).to eq 'image' - end - - it 'handles text' do - blob = stubbed_blob(text?: true, name: 'test.txt') - expect(blob.to_partial_path(project)).to eq 'text' - end - - it 'defaults to download' do - blob = stubbed_blob - expect(blob.to_partial_path(project)).to eq 'download' - end - - it 'handles PDFs' do - blob = stubbed_blob(name: 'blob.pdf', pdf?: true) - expect(blob.to_partial_path(project)).to eq 'pdf' - end - - it 'handles iPython notebooks' do - blob = stubbed_blob(text?: true, ipython_notebook?: true) - expect(blob.to_partial_path(project)).to eq 'notebook' - end - - it 'handles Sketch files' do - blob = stubbed_blob(text?: true, sketch?: true, binary?: true) - expect(blob.to_partial_path(project)).to eq 'sketch' - end - - it 'handles STLs' do - blob = stubbed_blob(text?: true, stl?: true) - expect(blob.to_partial_path(project)).to eq 'stl' - end - end - - describe '#size_within_svg_limits?' do - let(:blob) { described_class.decorate(double(:blob)) } - - it 'returns true when the blob size is smaller than the SVG limit' do - expect(blob).to receive(:size).and_return(42) - - expect(blob.size_within_svg_limits?).to eq(true) - end - - it 'returns true when the blob size is equal to the SVG limit' do - expect(blob).to receive(:size).and_return(Blob::MAXIMUM_SVG_SIZE) - - expect(blob.size_within_svg_limits?).to eq(true) - end - - it 'returns false when the blob size is larger than the SVG limit' do - expect(blob).to receive(:size).and_return(1.terabyte) - - expect(blob.size_within_svg_limits?).to eq(false) - end - end + # TODO: Test new methods end diff --git a/spec/models/blob_viewer/base_spec.rb b/spec/models/blob_viewer/base_spec.rb new file mode 100644 index 00000000000..e176d9c751b --- /dev/null +++ b/spec/models/blob_viewer/base_spec.rb @@ -0,0 +1,5 @@ +include 'spec_helper' + +describe BlobViewer::Base, model: true do + # TODO: Test +end diff --git a/spec/views/projects/blob/_render_error.html.haml_spec.rb b/spec/views/projects/blob/_render_error.html.haml_spec.rb new file mode 100644 index 00000000000..fabd444a6ad --- /dev/null +++ b/spec/views/projects/blob/_render_error.html.haml_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe 'app/views/projects/blob/_render_error.html.haml' do + # TODO: Test +end diff --git a/spec/views/projects/blob/_viewer.html.haml_spoc.rb b/spec/views/projects/blob/_viewer.html.haml_spoc.rb new file mode 100644 index 00000000000..1d2055c10fc --- /dev/null +++ b/spec/views/projects/blob/_viewer.html.haml_spoc.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe 'app/views/projects/blob/_viewer.html.haml' do + # TODO: Test +end diff --git a/spec/views/projects/blob/_viewer_switcher.html.haml_spoc.rb b/spec/views/projects/blob/_viewer_switcher.html.haml_spoc.rb new file mode 100644 index 00000000000..337f40d50df --- /dev/null +++ b/spec/views/projects/blob/_viewer_switcher.html.haml_spoc.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe 'app/views/projects/blob/_viewer_switcher.html.haml' do + # TODO: Test +end diff --git a/spec/views/projects/blob/_viewer_wrapper.html.haml_spoc.rb b/spec/views/projects/blob/_viewer_wrapper.html.haml_spoc.rb new file mode 100644 index 00000000000..433ff1e63b9 --- /dev/null +++ b/spec/views/projects/blob/_viewer_wrapper.html.haml_spoc.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe 'app/views/projects/blob/_viewer_wrapper.html.haml' do + # TODO: Test +end -- cgit v1.2.1 From c69a0779fb499fb3c8352eede0b5c6d7bb1117d1 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 21 Apr 2017 13:22:04 -0500 Subject: Address feedback --- app/helpers/blob_helper.rb | 16 ++++++++++++++++ app/models/blob.rb | 8 +++----- app/models/blob_viewer/base.rb | 4 ++-- app/models/blob_viewer/download.rb | 5 +++-- app/views/projects/blob/_content.html.haml | 6 +++--- app/views/projects/blob/_render_error.html.haml | 13 ++----------- 6 files changed, 29 insertions(+), 23 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 0e369a63c87..472662c4ba9 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -224,4 +224,20 @@ module BlobHelper "it is stored in LFS" end end + + def blob_render_error_options(viewer, error) + options = [] + + if error == :too_large && viewer.can_override_max_size? + options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil))) + end + + if viewer.rich? && viewer.blob.rendered_as_text?(override_max_size: true) + options << link_to('view the source', '#', class: 'js-blob-viewer-switcher', data: { viewer: 'simple' }) + end + + options << link_to('download it', blob_raw_url, target: '_blank', rel: 'noopener noreferrer') + + options + end end diff --git a/app/models/blob.rb b/app/models/blob.rb index 1bb9ed03c11..65356f01cc2 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -107,11 +107,9 @@ class Blob < SimpleDelegator end def rich_viewer_class - if invalid_lfs_pointer? || empty? - nil - else - rich_viewers_classes.find { |viewer_class| viewer_class.can_render?(self) } - end + return if invalid_lfs_pointer? || empty? + + rich_viewers_classes.find { |viewer_class| viewer_class.can_render?(self) } end def simple_viewer diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index ce4f129232d..8e6919f1054 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -2,7 +2,7 @@ module BlobViewer class Base class_attribute :partial_name, :type, :extensions, :client_side, :text_based, :switcher_icon, :switcher_title, :max_size, :absolute_max_size - delegate :partial_path, :rich?, :simple?, :client_side?, :text_based?, to: :class + delegate :partial_path, :rich?, :simple?, :client_side?, :server_side?, :text_based?, to: :class attr_reader :blob @@ -26,7 +26,7 @@ module BlobViewer client_side end - def server_side? + def self.server_side? !client_side? end diff --git a/app/models/blob_viewer/download.rb b/app/models/blob_viewer/download.rb index e406cfc05f7..8f293ea6008 100644 --- a/app/models/blob_viewer/download.rb +++ b/app/models/blob_viewer/download.rb @@ -1,8 +1,9 @@ module BlobViewer class Download < Base include Simple - # We pretend the Download viewer is rendered client-side so that it doesn't - # attempt to load the entire blob contents. + # We treat the Download viewer as if it renders the content client-side, + # so that it doesn't attempt to load the entire blob contents and is + # rendered synchronously instead of loaded asynchronously. include ClientSide self.partial_name = 'download' diff --git a/app/views/projects/blob/_content.html.haml b/app/views/projects/blob/_content.html.haml index 7bbc8627a2b..cd4d85591ae 100644 --- a/app/views/projects/blob/_content.html.haml +++ b/app/views/projects/blob/_content.html.haml @@ -1,8 +1,8 @@ - simple_viewer = blob.simple_viewer - rich_viewer = blob.rich_viewer -- active_viewer = rich_viewer && params[:viewer] != 'simple' ? :rich : :simple +- rich_viewer_active = rich_viewer && params[:viewer] != 'simple' -= render 'projects/blob/viewer_wrapper', viewer: simple_viewer, hidden: (active_viewer != :simple) += render 'projects/blob/viewer_wrapper', viewer: simple_viewer, hidden: rich_viewer_active - if rich_viewer - = render 'projects/blob/viewer_wrapper', viewer: rich_viewer, hidden: (active_viewer != :rich) + = render 'projects/blob/viewer_wrapper', viewer: rich_viewer, hidden: !rich_viewer_active diff --git a/app/views/projects/blob/_render_error.html.haml b/app/views/projects/blob/_render_error.html.haml index 56bcc2b018c..026b7c95163 100644 --- a/app/views/projects/blob/_render_error.html.haml +++ b/app/views/projects/blob/_render_error.html.haml @@ -1,16 +1,7 @@ -- reason = blob_render_error_reason(viewer, error) - -- options = [] -- if error == :too_large && viewer.can_override_max_size? - - options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil))) -- if viewer.rich? && viewer.blob.rendered_as_text?(override_max_size: true) - - options << link_to('view the source', '#', class: 'js-blob-viewer-switcher', data: { viewer: 'simple' }) -- options << link_to('download it', blob_raw_url, target: '_blank', rel: 'noopener noreferrer') - .file-content.code .nothing-here-block - The #{viewer.switcher_title} could not be displayed because #{reason}. + The #{viewer.switcher_title} could not be displayed because #{blob_render_error_reason(viewer, error)}. You can - = options.to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe + = blob_render_error_options(viewer, error).to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe instead. -- cgit v1.2.1 From a7fd95cd22062f18474ee038d72fa9e1139a1a84 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 21 Apr 2017 13:33:48 -0500 Subject: Render viewer error synchronously so that 'view the source' link will work --- app/assets/javascripts/blob/viewer/index.js | 25 +++++++++++----------- app/controllers/concerns/renders_blob.rb | 2 +- app/helpers/blob_helper.rb | 2 +- app/views/projects/blob/_content.html.haml | 4 ++-- app/views/projects/blob/_viewer.html.haml | 19 +++++++++++----- app/views/projects/blob/_viewer_switcher.html.haml | 6 +++--- app/views/projects/blob/_viewer_wrapper.html.haml | 8 ------- spec/features/projects/blobs/blob_show_spec.rb | 2 +- spec/javascripts/blob/viewer/index_spec.js | 12 +++++------ spec/views/projects/blob/_viewer.html.haml_spec.rb | 5 +++++ spec/views/projects/blob/_viewer.html.haml_spoc.rb | 5 ----- .../blob/_viewer_switcher.html.haml_spec.rb | 5 +++++ .../blob/_viewer_switcher.html.haml_spoc.rb | 5 ----- .../blob/_viewer_wrapper.html.haml_spoc.rb | 5 ----- 14 files changed, 51 insertions(+), 54 deletions(-) delete mode 100644 app/views/projects/blob/_viewer_wrapper.html.haml create mode 100644 spec/views/projects/blob/_viewer.html.haml_spec.rb delete mode 100644 spec/views/projects/blob/_viewer.html.haml_spoc.rb create mode 100644 spec/views/projects/blob/_viewer_switcher.html.haml_spec.rb delete mode 100644 spec/views/projects/blob/_viewer_switcher.html.haml_spoc.rb delete mode 100644 spec/views/projects/blob/_viewer_wrapper.html.haml_spoc.rb diff --git a/app/assets/javascripts/blob/viewer/index.js b/app/assets/javascripts/blob/viewer/index.js index a322351219c..8a5fb187a71 100644 --- a/app/assets/javascripts/blob/viewer/index.js +++ b/app/assets/javascripts/blob/viewer/index.js @@ -2,7 +2,8 @@ /* global Flash */ export default class BlobViewer { constructor() { - this.switcherBtns = document.querySelectorAll('.js-blob-viewer-switcher'); + this.switcher = document.querySelector('.js-blob-viewer-switcher'); + this.switcherBtns = document.querySelectorAll('.js-blob-viewer-switch-btn'); this.copySourceBtn = document.querySelector('.js-copy-blob-source-btn'); this.simpleViewer = document.querySelector('.blob-viewer[data-type="simple"]'); this.richViewer = document.querySelector('.blob-viewer[data-type="rich"]'); @@ -10,22 +11,22 @@ export default class BlobViewer { let initialViewerName = document.querySelector('.blob-viewer:not(.hidden)').getAttribute('data-type'); - if (this.switcherBtns.length) { - this.initBindings(); + this.initBindings(); - if (location.hash.indexOf('#L') === 0) { - initialViewerName = 'simple'; - } + if (this.switcher && location.hash.indexOf('#L') === 0) { + initialViewerName = 'simple'; } this.switchToViewer(initialViewerName); } initBindings() { - Array.from(this.switcherBtns) - .forEach((el) => { - el.addEventListener('click', this.switchViewHandler.bind(this)); - }); + if (this.switcherBtns.length) { + Array.from(this.switcherBtns) + .forEach((el) => { + el.addEventListener('click', this.switchViewHandler.bind(this)); + }); + } if (this.copySourceBtn) { this.copySourceBtn.addEventListener('click', () => { @@ -91,8 +92,8 @@ export default class BlobViewer { const newViewer = document.querySelector(`.blob-viewer[data-type='${name}']`); if (this.activeViewer === newViewer) return; - const oldButton = document.querySelector('.js-blob-viewer-switcher.active'); - const newButton = document.querySelector(`.js-blob-viewer-switcher[data-viewer='${name}']`); + const oldButton = document.querySelector('.js-blob-viewer-switch-btn.active'); + const newButton = document.querySelector(`.js-blob-viewer-switch-btn[data-viewer='${name}']`); const oldViewer = document.querySelector(`.blob-viewer:not([data-type='${name}'])`); if (oldButton) { diff --git a/app/controllers/concerns/renders_blob.rb b/app/controllers/concerns/renders_blob.rb index ab9f0d01c7b..d478c3bb6ca 100644 --- a/app/controllers/concerns/renders_blob.rb +++ b/app/controllers/concerns/renders_blob.rb @@ -11,7 +11,7 @@ module RendersBlob return render_404 unless viewer render json: { - html: view_to_html_string("projects/blob/_viewer", viewer: viewer) + html: view_to_html_string("projects/blob/_viewer", viewer: viewer, load_asynchronously: false) } end end diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 472662c4ba9..7db9cf0ff01 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -233,7 +233,7 @@ module BlobHelper end if viewer.rich? && viewer.blob.rendered_as_text?(override_max_size: true) - options << link_to('view the source', '#', class: 'js-blob-viewer-switcher', data: { viewer: 'simple' }) + options << link_to('view the source', '#', class: 'js-blob-viewer-switch-btn', data: { viewer: 'simple' }) end options << link_to('download it', blob_raw_url, target: '_blank', rel: 'noopener noreferrer') diff --git a/app/views/projects/blob/_content.html.haml b/app/views/projects/blob/_content.html.haml index cd4d85591ae..7afbd85cd6d 100644 --- a/app/views/projects/blob/_content.html.haml +++ b/app/views/projects/blob/_content.html.haml @@ -2,7 +2,7 @@ - rich_viewer = blob.rich_viewer - rich_viewer_active = rich_viewer && params[:viewer] != 'simple' -= render 'projects/blob/viewer_wrapper', viewer: simple_viewer, hidden: rich_viewer_active += render 'projects/blob/viewer', viewer: simple_viewer, hidden: rich_viewer_active - if rich_viewer - = render 'projects/blob/viewer_wrapper', viewer: rich_viewer, hidden: !rich_viewer_active + = render 'projects/blob/viewer', viewer: rich_viewer, hidden: !rich_viewer_active diff --git a/app/views/projects/blob/_viewer.html.haml b/app/views/projects/blob/_viewer.html.haml index 19f1d1251ae..e0cfe39d1ec 100644 --- a/app/views/projects/blob/_viewer.html.haml +++ b/app/views/projects/blob/_viewer.html.haml @@ -1,5 +1,14 @@ -- if error = viewer.render_error(override_max_size: params[:override_max_size]) - = render 'projects/blob/render_error', viewer: viewer, error: error -- else - - viewer.prepare! - = render viewer.partial_path, viewer: viewer +- hidden = local_assigns.fetch(:hidden, false) +- render_error = viewer.render_error(override_max_size: params[:override_max_size]) +- load_asynchronously = local_assigns.fetch(:load_asynchronously, viewer.server_side?) && render_error.nil? + +- url = url_for(params.merge(viewer: viewer.type, format: :json)) if load_asynchronously +.blob-viewer{ data: { type: viewer.type, url: url }, class: ('hidden' if hidden) } + - if load_asynchronously + .text-center.prepend-top-default.append-bottom-default + = icon('spinner spin 2x', 'aria-hidden' => 'true', 'aria-label' => 'Loading content') + - elsif render_error + = render 'projects/blob/render_error', viewer: viewer, error: render_error + - else + - viewer.prepare! + = render viewer.partial_path, viewer: viewer diff --git a/app/views/projects/blob/_viewer_switcher.html.haml b/app/views/projects/blob/_viewer_switcher.html.haml index 6d69660e656..6a521069418 100644 --- a/app/views/projects/blob/_viewer_switcher.html.haml +++ b/app/views/projects/blob/_viewer_switcher.html.haml @@ -2,11 +2,11 @@ - simple_viewer = blob.simple_viewer - rich_viewer = blob.rich_viewer - .btn-group{ role: "group" } + .btn-group.js-blob-viewer-switcher{ role: "group" } - simple_label = "Display #{simple_viewer.switcher_title}" - %button.btn.btn-default.btn-sm.js-blob-viewer-switcher.has-tooltip{ 'aria-label' => simple_label, title: simple_label, data: { viewer: 'simple', container: 'body' } }> + %button.btn.btn-default.btn-sm.js-blob-viewer-switch-btn.has-tooltip{ 'aria-label' => simple_label, title: simple_label, data: { viewer: 'simple', container: 'body' } }> = icon(simple_viewer.switcher_icon) - rich_label = "Display #{rich_viewer.switcher_title}" - %button.btn.btn-default.btn-sm.js-blob-viewer-switcher.has-tooltip{ 'aria-label' => rich_label, title: rich_label, data: { viewer: 'rich', container: 'body' } }> + %button.btn.btn-default.btn-sm.js-blob-viewer-switch-btn.has-tooltip{ 'aria-label' => rich_label, title: rich_label, data: { viewer: 'rich', container: 'body' } }> = icon(rich_viewer.switcher_icon) diff --git a/app/views/projects/blob/_viewer_wrapper.html.haml b/app/views/projects/blob/_viewer_wrapper.html.haml deleted file mode 100644 index 8da5022fbea..00000000000 --- a/app/views/projects/blob/_viewer_wrapper.html.haml +++ /dev/null @@ -1,8 +0,0 @@ -- hidden = local_assigns.fetch(:hidden, false) -- url = url_for(params.merge(format: :json, viewer: viewer.type)) if viewer.server_side? -.blob-viewer{ data: { type: viewer.type, url: url }, class: ('hidden' if hidden) } - - if viewer.server_side? - .text-center.prepend-top-default.append-bottom-default - = icon('spinner spin 2x', 'aria-hidden' => 'true', 'aria-label' => 'Loading content') - - else - = render 'projects/blob/viewer', viewer: viewer diff --git a/spec/features/projects/blobs/blob_show_spec.rb b/spec/features/projects/blobs/blob_show_spec.rb index 9fbb17b3707..43af91882a4 100644 --- a/spec/features/projects/blobs/blob_show_spec.rb +++ b/spec/features/projects/blobs/blob_show_spec.rb @@ -19,7 +19,7 @@ feature 'File blob', feature: true do it 'switches to code view' do visit_blob('files/images/wm.svg') - first('.js-blob-viewer-switcher').click + first('.js-blob-viewer-switch-btn').click expect(page).to have_selector('.blob-viewer[data-type="rich"]', visible: false) expect(page).to have_selector('.blob-viewer[data-type="simple"]') diff --git a/spec/javascripts/blob/viewer/index_spec.js b/spec/javascripts/blob/viewer/index_spec.js index c95a40689c7..fe45ee3c083 100644 --- a/spec/javascripts/blob/viewer/index_spec.js +++ b/spec/javascripts/blob/viewer/index_spec.js @@ -26,12 +26,12 @@ describe('Blob viewer', () => { }); it('loads source file after switching views', (done) => { - document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]').click(); + document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]').click(); setTimeout(() => { expect($.ajax).toHaveBeenCalled(); expect( - document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]') + document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]') .classList.contains('hidden'), ).toBeFalsy(); @@ -47,7 +47,7 @@ describe('Blob viewer', () => { setTimeout(() => { expect($.ajax).toHaveBeenCalled(); expect( - document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]') + document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]') .classList.contains('hidden'), ).toBeFalsy(); @@ -57,7 +57,7 @@ describe('Blob viewer', () => { it('doesnt reload file if already loaded', (done) => { const asyncClick = () => new Promise((resolve) => { - document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]').click(); + document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]').click(); setTimeout(resolve); }); @@ -91,7 +91,7 @@ describe('Blob viewer', () => { }); it('enables after switching to simple view', (done) => { - document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]').click(); + document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]').click(); setTimeout(() => { expect($.ajax).toHaveBeenCalled(); @@ -104,7 +104,7 @@ describe('Blob viewer', () => { }); it('updates tooltip after switching to simple view', (done) => { - document.querySelector('.js-blob-viewer-switcher[data-viewer="simple"]').click(); + document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]').click(); setTimeout(() => { expect($.ajax).toHaveBeenCalled(); diff --git a/spec/views/projects/blob/_viewer.html.haml_spec.rb b/spec/views/projects/blob/_viewer.html.haml_spec.rb new file mode 100644 index 00000000000..1d2055c10fc --- /dev/null +++ b/spec/views/projects/blob/_viewer.html.haml_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe 'app/views/projects/blob/_viewer.html.haml' do + # TODO: Test +end diff --git a/spec/views/projects/blob/_viewer.html.haml_spoc.rb b/spec/views/projects/blob/_viewer.html.haml_spoc.rb deleted file mode 100644 index 1d2055c10fc..00000000000 --- a/spec/views/projects/blob/_viewer.html.haml_spoc.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'spec_helper' - -describe 'app/views/projects/blob/_viewer.html.haml' do - # TODO: Test -end diff --git a/spec/views/projects/blob/_viewer_switcher.html.haml_spec.rb b/spec/views/projects/blob/_viewer_switcher.html.haml_spec.rb new file mode 100644 index 00000000000..337f40d50df --- /dev/null +++ b/spec/views/projects/blob/_viewer_switcher.html.haml_spec.rb @@ -0,0 +1,5 @@ +require 'spec_helper' + +describe 'app/views/projects/blob/_viewer_switcher.html.haml' do + # TODO: Test +end diff --git a/spec/views/projects/blob/_viewer_switcher.html.haml_spoc.rb b/spec/views/projects/blob/_viewer_switcher.html.haml_spoc.rb deleted file mode 100644 index 337f40d50df..00000000000 --- a/spec/views/projects/blob/_viewer_switcher.html.haml_spoc.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'spec_helper' - -describe 'app/views/projects/blob/_viewer_switcher.html.haml' do - # TODO: Test -end diff --git a/spec/views/projects/blob/_viewer_wrapper.html.haml_spoc.rb b/spec/views/projects/blob/_viewer_wrapper.html.haml_spoc.rb deleted file mode 100644 index 433ff1e63b9..00000000000 --- a/spec/views/projects/blob/_viewer_wrapper.html.haml_spoc.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'spec_helper' - -describe 'app/views/projects/blob/_viewer_wrapper.html.haml' do - # TODO: Test -end -- cgit v1.2.1 From 0e0c760e487651cdbddfce818ca6b69ad43fe071 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 21 Apr 2017 13:59:34 -0500 Subject: Refactor overriding max size --- app/controllers/projects/blob_controller.rb | 2 ++ app/helpers/blob_helper.rb | 10 ++++++++-- app/models/blob.rb | 11 ++++++++--- app/models/blob_viewer/base.rb | 29 +++++++++++------------------ app/models/blob_viewer/download.rb | 2 +- app/views/projects/blob/_header.html.haml | 2 +- app/views/projects/blob/_viewer.html.haml | 2 +- 7 files changed, 32 insertions(+), 26 deletions(-) diff --git a/app/controllers/projects/blob_controller.rb b/app/controllers/projects/blob_controller.rb index 173c6bcee53..be5822b2cd4 100644 --- a/app/controllers/projects/blob_controller.rb +++ b/app/controllers/projects/blob_controller.rb @@ -35,6 +35,8 @@ class Projects::BlobController < Projects::ApplicationController end def show + @blob.override_max_size! if params[:override_max_size] == 'true' + respond_to do |format| format.html do environment_params = @repository.branch_exists?(@ref) ? { ref: @ref } : { commit: @commit } diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 7db9cf0ff01..00cf0ac96c9 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -219,7 +219,13 @@ module BlobHelper def blob_render_error_reason(viewer, error) case error when :too_large - "it is larger than #{number_to_human_size(viewer.relevant_max_size)}" + max_size = + if viewer.absolutely_too_large? + viewer.absolute_max_size + elsif viewer.too_large? + viewer.max_size + end + "it is larger than #{number_to_human_size(max_size)}" when :server_side_but_stored_in_lfs "it is stored in LFS" end @@ -232,7 +238,7 @@ module BlobHelper options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil))) end - if viewer.rich? && viewer.blob.rendered_as_text?(override_max_size: true) + if viewer.rich? && viewer.blob.rendered_as_text? options << link_to('view the source', '#', class: 'js-blob-viewer-switch-btn', data: { viewer: 'simple' }) end diff --git a/app/models/blob.rb b/app/models/blob.rb index 65356f01cc2..dedf60ca14a 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -122,12 +122,17 @@ class Blob < SimpleDelegator @rich_viewer ||= rich_viewer_class&.new(self) end - def rendered_as_text?(override_max_size: false) - simple_viewer.is_a?(BlobViewer::Text) && !simple_viewer.render_error(override_max_size: override_max_size) + def rendered_as_text?(ignore_errors: true) + simple_viewer.is_a?(BlobViewer::Text) && (ignore_errors || simple_viewer.render_error.nil?) end def show_viewer_switcher? - simple_viewer.is_a?(BlobViewer::Text) && rich_viewer + rendered_as_text? && rich_viewer + end + + def override_max_size! + simple_viewer&.override_max_size = true + rich_viewer&.override_max_size = true end private diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index 8e6919f1054..714a063933b 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -5,6 +5,7 @@ module BlobViewer delegate :partial_path, :rich?, :simple?, :client_side?, :server_side?, :text_based?, to: :class attr_reader :blob + attr_accessor :override_max_size def initialize(blob) @blob = blob @@ -38,20 +39,20 @@ module BlobViewer !extensions || extensions.include?(blob.extension) end - def can_override_max_size? - too_large? && !too_large?(override_max_size: true) + def too_large? + blob.raw_size > max_size end - def relevant_max_size - if too_large?(override_max_size: true) - absolute_max_size - elsif too_large? - max_size - end + def absolutely_too_large? + blob.raw_size > absolute_max_size + end + + def can_override_max_size? + too_large? && !absolutely_too_large? end - def render_error(override_max_size: false) - if too_large?(override_max_size: override_max_size) + def render_error + if override_max_size ? absolutely_too_large? : too_large? :too_large elsif server_side_but_stored_in_lfs? :server_side_but_stored_in_lfs @@ -66,14 +67,6 @@ module BlobViewer private - def too_large?(override_max_size: false) - if override_max_size - blob.raw_size > absolute_max_size - else - blob.raw_size > max_size - end - end - def server_side_but_stored_in_lfs? server_side? && blob.valid_lfs_pointer? end diff --git a/app/models/blob_viewer/download.rb b/app/models/blob_viewer/download.rb index 8f293ea6008..dbd6f07dbc8 100644 --- a/app/models/blob_viewer/download.rb +++ b/app/models/blob_viewer/download.rb @@ -9,7 +9,7 @@ module BlobViewer self.partial_name = 'download' self.text_based = false - def render_error(*) + def render_error nil end end diff --git a/app/views/projects/blob/_header.html.haml b/app/views/projects/blob/_header.html.haml index 19eca2984db..b89cd460455 100644 --- a/app/views/projects/blob/_header.html.haml +++ b/app/views/projects/blob/_header.html.haml @@ -15,7 +15,7 @@ = render 'projects/blob/viewer_switcher', blob: blob unless blame .btn-group{ role: "group" }< - = copy_blob_source_button(blob) if !blame && blob.rendered_as_text?(override_max_size: params[:override_max_size]) + = copy_blob_source_button(blob) if !blame && blob.rendered_as_text?(ignore_errors: false) = open_raw_file_button(namespace_project_raw_path(@project.namespace, @project, @id)) = view_on_environment_button(@commit.sha, @path, @environment) if @environment diff --git a/app/views/projects/blob/_viewer.html.haml b/app/views/projects/blob/_viewer.html.haml index e0cfe39d1ec..1c45c4a68ce 100644 --- a/app/views/projects/blob/_viewer.html.haml +++ b/app/views/projects/blob/_viewer.html.haml @@ -1,5 +1,5 @@ - hidden = local_assigns.fetch(:hidden, false) -- render_error = viewer.render_error(override_max_size: params[:override_max_size]) +- render_error = viewer.render_error - load_asynchronously = local_assigns.fetch(:load_asynchronously, viewer.server_side?) && render_error.nil? - url = url_for(params.merge(viewer: viewer.type, format: :json)) if load_asynchronously -- cgit v1.2.1 From ec19703a44aaf427de2b83cec31b72be9a024a42 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 21 Apr 2017 15:50:20 -0500 Subject: Blob responds to text? --- app/views/notify/repository_push_email.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/notify/repository_push_email.html.haml b/app/views/notify/repository_push_email.html.haml index 36512533c58..02eb7c8462c 100644 --- a/app/views/notify/repository_push_email.html.haml +++ b/app/views/notify/repository_push_email.html.haml @@ -74,7 +74,7 @@ - else %hr - blob = diff_file.blob - - if blob && blob.respond_to?(:text?) && blob.readable_text? + - if blob && blob.readable_text? %table.code.white = render partial: "projects/diffs/line", collection: diff_file.highlighted_diff_lines, as: :line, locals: { diff_file: diff_file, plain: true, email: true } - else -- cgit v1.2.1 From a2f4650fc7e5407bbed1f73771306d12823f47da Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 24 Apr 2017 09:27:19 -0500 Subject: Add BlobViewer::Base#binary? method --- app/models/blob.rb | 44 +++++++++++++++++++++++------------------- app/models/blob_viewer/base.rb | 8 ++++++-- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/app/models/blob.rb b/app/models/blob.rb index dedf60ca14a..2225de631bf 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -70,7 +70,11 @@ class Blob < SimpleDelegator def raw_binary? if valid_lfs_pointer? - !rich_viewer&.text_based? + if rich_viewer + rich_viewer.binary? + else + true + end else binary? end @@ -96,22 +100,6 @@ class Blob < SimpleDelegator lfs_pointer? && !project.lfs_enabled? end - def simple_viewer_class - if empty? - BlobViewer::Empty - elsif raw_binary? - BlobViewer::Download - else # text - BlobViewer::Text - end - end - - def rich_viewer_class - return if invalid_lfs_pointer? || empty? - - rich_viewers_classes.find { |viewer_class| viewer_class.can_render?(self) } - end - def simple_viewer @simple_viewer ||= simple_viewer_class.new(self) end @@ -123,7 +111,7 @@ class Blob < SimpleDelegator end def rendered_as_text?(ignore_errors: true) - simple_viewer.is_a?(BlobViewer::Text) && (ignore_errors || simple_viewer.render_error.nil?) + simple_viewer.text? && (ignore_errors || simple_viewer.render_error.nil?) end def show_viewer_switcher? @@ -137,13 +125,29 @@ class Blob < SimpleDelegator private + def simple_viewer_class + if empty? + BlobViewer::Empty + elsif raw_binary? + BlobViewer::Download + else # text + BlobViewer::Text + end + end + def rich_viewers_classes if valid_lfs_pointer? RICH_VIEWERS elsif binary? - RICH_VIEWERS.reject(&:text_based?) + RICH_VIEWERS.select(&:binary?) else # text - RICH_VIEWERS.select(&:text_based?) + RICH_VIEWERS.select(&:text?) end end + + def rich_viewer_class + return if invalid_lfs_pointer? || empty? + + rich_viewers_classes.find { |viewer_class| viewer_class.can_render?(self) } + end end diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index 714a063933b..ae90d977f03 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -2,7 +2,7 @@ module BlobViewer class Base class_attribute :partial_name, :type, :extensions, :client_side, :text_based, :switcher_icon, :switcher_title, :max_size, :absolute_max_size - delegate :partial_path, :rich?, :simple?, :client_side?, :server_side?, :text_based?, to: :class + delegate :partial_path, :rich?, :simple?, :client_side?, :server_side?, :text?, :binary?, to: :class attr_reader :blob attr_accessor :override_max_size @@ -31,10 +31,14 @@ module BlobViewer !client_side? end - def self.text_based? + def self.text? text_based end + def self.binary? + !text? + end + def self.can_render?(blob) !extensions || extensions.include?(blob.extension) end -- cgit v1.2.1 From 0b3ff9c80494307746f79c05f85cfbedc0efe3f6 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 24 Apr 2017 12:51:56 -0500 Subject: Set BlobViewer::Base.binary instead of .text --- app/models/blob_viewer/base.rb | 10 +++++----- app/models/blob_viewer/binary_stl.rb | 2 +- app/models/blob_viewer/download.rb | 2 +- app/models/blob_viewer/empty.rb | 2 +- app/models/blob_viewer/image.rb | 2 +- app/models/blob_viewer/markup.rb | 2 +- app/models/blob_viewer/notebook.rb | 2 +- app/models/blob_viewer/pdf.rb | 4 ++-- app/models/blob_viewer/sketch.rb | 2 +- app/models/blob_viewer/svg.rb | 2 +- app/models/blob_viewer/text.rb | 2 +- app/models/blob_viewer/text_stl.rb | 2 +- 12 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index ae90d977f03..d44d5c70fbb 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -1,6 +1,6 @@ module BlobViewer class Base - class_attribute :partial_name, :type, :extensions, :client_side, :text_based, :switcher_icon, :switcher_title, :max_size, :absolute_max_size + class_attribute :partial_name, :type, :extensions, :client_side, :binary, :switcher_icon, :switcher_title, :max_size, :absolute_max_size delegate :partial_path, :rich?, :simple?, :client_side?, :server_side?, :text?, :binary?, to: :class @@ -31,12 +31,12 @@ module BlobViewer !client_side? end - def self.text? - text_based + def self.binary? + binary end - def self.binary? - !text? + def self.text? + !binary? end def self.can_render?(blob) diff --git a/app/models/blob_viewer/binary_stl.rb b/app/models/blob_viewer/binary_stl.rb index 6cb54d49d86..80393471ef2 100644 --- a/app/models/blob_viewer/binary_stl.rb +++ b/app/models/blob_viewer/binary_stl.rb @@ -5,6 +5,6 @@ module BlobViewer self.partial_name = 'stl' self.extensions = %w(stl) - self.text_based = false + self.binary = true end end diff --git a/app/models/blob_viewer/download.rb b/app/models/blob_viewer/download.rb index dbd6f07dbc8..7bc45fe992c 100644 --- a/app/models/blob_viewer/download.rb +++ b/app/models/blob_viewer/download.rb @@ -7,7 +7,7 @@ module BlobViewer include ClientSide self.partial_name = 'download' - self.text_based = false + self.binary = true def render_error nil diff --git a/app/models/blob_viewer/empty.rb b/app/models/blob_viewer/empty.rb index 60003a7c12a..d9d128eb273 100644 --- a/app/models/blob_viewer/empty.rb +++ b/app/models/blob_viewer/empty.rb @@ -4,6 +4,6 @@ module BlobViewer include ServerSide self.partial_name = 'empty' - self.text_based = false + self.binary = true end end diff --git a/app/models/blob_viewer/image.rb b/app/models/blob_viewer/image.rb index b78f82df38f..c4eae5c79c2 100644 --- a/app/models/blob_viewer/image.rb +++ b/app/models/blob_viewer/image.rb @@ -5,7 +5,7 @@ module BlobViewer self.partial_name = 'image' self.extensions = UploaderHelper::IMAGE_EXT - self.text_based = false + self.binary = true self.switcher_icon = 'picture-o' self.switcher_title = 'image' end diff --git a/app/models/blob_viewer/markup.rb b/app/models/blob_viewer/markup.rb index 34740661320..8fdbab30dd1 100644 --- a/app/models/blob_viewer/markup.rb +++ b/app/models/blob_viewer/markup.rb @@ -5,6 +5,6 @@ module BlobViewer self.partial_name = 'markup' self.extensions = Gitlab::MarkupHelper::EXTENSIONS - self.text_based = true + self.binary = false end end diff --git a/app/models/blob_viewer/notebook.rb b/app/models/blob_viewer/notebook.rb index c5231f1e691..8632b8a9885 100644 --- a/app/models/blob_viewer/notebook.rb +++ b/app/models/blob_viewer/notebook.rb @@ -5,7 +5,7 @@ module BlobViewer self.partial_name = 'notebook' self.extensions = %w(ipynb) - self.text_based = true + self.binary = false self.switcher_icon = 'file-text-o' self.switcher_title = 'notebook' end diff --git a/app/models/blob_viewer/pdf.rb b/app/models/blob_viewer/pdf.rb index 1067388c72a..65805f5f388 100644 --- a/app/models/blob_viewer/pdf.rb +++ b/app/models/blob_viewer/pdf.rb @@ -2,10 +2,10 @@ module BlobViewer class PDF < Base include Rich include ClientSide - + self.partial_name = 'pdf' self.extensions = %w(pdf) - self.text_based = false + self.binary = true self.switcher_icon = 'file-pdf-o' self.switcher_title = 'PDF' end diff --git a/app/models/blob_viewer/sketch.rb b/app/models/blob_viewer/sketch.rb index 3de94efe67d..818456778e1 100644 --- a/app/models/blob_viewer/sketch.rb +++ b/app/models/blob_viewer/sketch.rb @@ -5,7 +5,7 @@ module BlobViewer self.partial_name = 'sketch' self.extensions = %w(sketch) - self.text_based = false + self.binary = true self.switcher_icon = 'file-image-o' self.switcher_title = 'preview' end diff --git a/app/models/blob_viewer/svg.rb b/app/models/blob_viewer/svg.rb index 5fd11a98462..b7e5cd71e6b 100644 --- a/app/models/blob_viewer/svg.rb +++ b/app/models/blob_viewer/svg.rb @@ -5,7 +5,7 @@ module BlobViewer self.partial_name = 'svg' self.extensions = %w(svg) - self.text_based = true + self.binary = false self.switcher_icon = 'picture-o' self.switcher_title = 'image' end diff --git a/app/models/blob_viewer/text.rb b/app/models/blob_viewer/text.rb index 5f442dadf0f..e27b2c2b493 100644 --- a/app/models/blob_viewer/text.rb +++ b/app/models/blob_viewer/text.rb @@ -4,7 +4,7 @@ module BlobViewer include ServerSide self.partial_name = 'text' - self.text_based = true + self.binary = false self.max_size = 1.megabyte self.absolute_max_size = 10.megabytes end diff --git a/app/models/blob_viewer/text_stl.rb b/app/models/blob_viewer/text_stl.rb index 1b238777c32..2bb136a2702 100644 --- a/app/models/blob_viewer/text_stl.rb +++ b/app/models/blob_viewer/text_stl.rb @@ -5,6 +5,6 @@ module BlobViewer self.partial_name = 'stl' self.extensions = %w(stl) - self.text_based = true + self.binary = false end end -- cgit v1.2.1 From b73b16798dd0fe402df42a1e706f3b1034c22270 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Mon, 24 Apr 2017 16:27:43 -0500 Subject: Small code tweaks --- app/models/blob.rb | 30 ++++++++++++++++++------------ lib/gitlab/git/blob.rb | 4 ---- 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/app/models/blob.rb b/app/models/blob.rb index 2225de631bf..0df220b4983 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -60,6 +60,9 @@ class Blob < SimpleDelegator size && truncated? end + # Returns the size of the file that this blob represents. If this blob is an + # LFS pointer, this is the size of the file stored in LFS. Otherwise, this is + # the size of the blob itself. def raw_size if valid_lfs_pointer? lfs_size @@ -68,6 +71,10 @@ class Blob < SimpleDelegator end end + # Returns whether the file that this blob represents is binary. If this blob is + # an LFS pointer, we assume the file stored in LFS is binary, unless a + # text-based rich blob viewer matched on the file's extension. Otherwise, this + # depends on the type of the blob itself. def raw_binary? if valid_lfs_pointer? if rich_viewer @@ -107,7 +114,7 @@ class Blob < SimpleDelegator def rich_viewer return @rich_viewer if defined?(@rich_viewer) - @rich_viewer ||= rich_viewer_class&.new(self) + @rich_viewer = rich_viewer_class&.new(self) end def rendered_as_text?(ignore_errors: true) @@ -135,19 +142,18 @@ class Blob < SimpleDelegator end end - def rich_viewers_classes - if valid_lfs_pointer? - RICH_VIEWERS - elsif binary? - RICH_VIEWERS.select(&:binary?) - else # text - RICH_VIEWERS.select(&:text?) - end - end - def rich_viewer_class return if invalid_lfs_pointer? || empty? - rich_viewers_classes.find { |viewer_class| viewer_class.can_render?(self) } + classes = + if valid_lfs_pointer? + RICH_VIEWERS + elsif binary? + RICH_VIEWERS.select(&:binary?) + else # text + RICH_VIEWERS.select(&:text?) + end + + classes.find { |viewer_class| viewer_class.can_render?(self) } end end diff --git a/lib/gitlab/git/blob.rb b/lib/gitlab/git/blob.rb index 98fd4e78126..e8bb9e1f805 100644 --- a/lib/gitlab/git/blob.rb +++ b/lib/gitlab/git/blob.rb @@ -109,10 +109,6 @@ module Gitlab @binary.nil? ? super : @binary == true end - def empty? - !data || data == '' - end - def data encode! @data end -- cgit v1.2.1 From 964e7d206b2c9ce0c884ee5c13fafa3763a142a2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 25 Apr 2017 15:22:56 -0500 Subject: Add specs --- app/assets/javascripts/blob/pdf/index.js | 2 +- app/models/blob.rb | 4 +- app/models/blob_viewer/base.rb | 6 +- spec/controllers/concerns/renders_blob_spec.rb | 5 - spec/features/projects/blobs/blob_show_spec.rb | 309 +++++++++++++++++++-- spec/helpers/blob_helper_spec.rb | 110 ++++++++ spec/models/blob_spec.rb | 178 +++++++++++- spec/models/blob_viewer/base_spec.rb | 185 +++++++++++- spec/support/helpers/fake_blob_helpers.rb | 50 ++++ .../projects/blob/_render_error.html.haml_spec.rb | 5 - spec/views/projects/blob/_viewer.html.haml_spec.rb | 96 ++++++- .../blob/_viewer_switcher.html.haml_spec.rb | 5 - 12 files changed, 904 insertions(+), 51 deletions(-) delete mode 100644 spec/controllers/concerns/renders_blob_spec.rb create mode 100644 spec/support/helpers/fake_blob_helpers.rb delete mode 100644 spec/views/projects/blob/_render_error.html.haml_spec.rb delete mode 100644 spec/views/projects/blob/_viewer_switcher.html.haml_spec.rb diff --git a/app/assets/javascripts/blob/pdf/index.js b/app/assets/javascripts/blob/pdf/index.js index a74c2db9a61..9161be98853 100644 --- a/app/assets/javascripts/blob/pdf/index.js +++ b/app/assets/javascripts/blob/pdf/index.js @@ -31,7 +31,7 @@ export default () => { }, }, template: ` -
+
diff --git a/app/models/blob.rb b/app/models/blob.rb index 0df220b4983..e87b418d242 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -100,11 +100,11 @@ class Blob < SimpleDelegator end def valid_lfs_pointer? - lfs_pointer? && project.lfs_enabled? + lfs_pointer? && project&.lfs_enabled? end def invalid_lfs_pointer? - lfs_pointer? && !project.lfs_enabled? + lfs_pointer? && !project&.lfs_enabled? end def simple_viewer diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index d44d5c70fbb..d3674530273 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -56,10 +56,10 @@ module BlobViewer end def render_error - if override_max_size ? absolutely_too_large? : too_large? - :too_large - elsif server_side_but_stored_in_lfs? + if server_side_but_stored_in_lfs? :server_side_but_stored_in_lfs + elsif override_max_size ? absolutely_too_large? : too_large? + :too_large end end diff --git a/spec/controllers/concerns/renders_blob_spec.rb b/spec/controllers/concerns/renders_blob_spec.rb deleted file mode 100644 index 5f07afc5479..00000000000 --- a/spec/controllers/concerns/renders_blob_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -include 'spec_helper' - -describe Projects::BlobController, RendersBlob, model: true do - # TODO: Test -end diff --git a/spec/features/projects/blobs/blob_show_spec.rb b/spec/features/projects/blobs/blob_show_spec.rb index 43af91882a4..aea9f66eec3 100644 --- a/spec/features/projects/blobs/blob_show_spec.rb +++ b/spec/features/projects/blobs/blob_show_spec.rb @@ -1,51 +1,314 @@ require 'spec_helper' -feature 'File blob', feature: true do +feature 'File blob', :js, feature: true do include TreeHelper + include WaitForAjax - let(:project) { create(:project, :public, :test_repo) } + let(:project) { create(:project, :public) } def visit_blob(path, fragment = nil) visit namespace_project_blob_path(project.namespace, project, tree_join('master', path), anchor: fragment) end - context 'text files' do - it 'shows rendered output for SVG' do - visit_blob('files/images/wm.svg') + context 'Ruby file' do + before do + visit_blob('files/ruby/popen.rb') - expect(page).to have_selector('.blob-viewer[data-type="rich"]') + wait_for_ajax end - it 'switches to code view' do - visit_blob('files/images/wm.svg') + it 'displays the blob' do + aggregate_failures do + # shows highlighted Ruby code + expect(page).to have_content("require 'fileutils'") - first('.js-blob-viewer-switch-btn').click + # does not show a viewer switcher + expect(page).not_to have_selector('.js-blob-viewer-switcher') - expect(page).to have_selector('.blob-viewer[data-type="rich"]', visible: false) - expect(page).to have_selector('.blob-viewer[data-type="simple"]') + # shows an enabled copy button + expect(page).to have_selector('.js-copy-blob-source-btn:not(.disabled)') + end end + end + + context 'Markdown file' do + context 'visiting directly' do + before do + visit_blob('files/markdown/ruby-style-guide.md') + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # hides the simple viewer + expect(page).to have_selector('.blob-viewer[data-type="simple"]', visible: false) + expect(page).to have_selector('.blob-viewer[data-type="rich"]') + + # shows rendered Markdown + expect(page).to have_link("PEP-8") + + # shows a viewer switcher + expect(page).to have_selector('.js-blob-viewer-switcher') + + # shows a disabled copy button + expect(page).to have_selector('.js-copy-blob-source-btn.disabled') + end + end + + context 'switching to the simple viewer' do + before do + find('.js-blob-viewer-switch-btn[data-viewer=simple]').click + + wait_for_ajax + end - it 'opens raw mode when linking to a line in SVG file' do - visit_blob('files/images/wm.svg', 'L1') + it 'displays the blob' do + aggregate_failures do + # hides the rich viewer + expect(page).to have_selector('.blob-viewer[data-type="simple"]') + expect(page).to have_selector('.blob-viewer[data-type="rich"]', visible: false) - expect(page).to have_selector('#LC1.hll') - expect(page).to have_selector('.blob-viewer[data-type="simple"]') + # shows highlighted Markdown code + expect(page).to have_content("[PEP-8](http://www.python.org/dev/peps/pep-0008/)") + + # shows an enabled copy button + expect(page).to have_selector('.js-copy-blob-source-btn:not(.disabled)') + end + end + + context 'switching to the rich viewer again' do + before do + find('.js-blob-viewer-switch-btn[data-viewer=rich]').click + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # hides the simple viewer + expect(page).to have_selector('.blob-viewer[data-type="simple"]', visible: false) + expect(page).to have_selector('.blob-viewer[data-type="rich"]') + + # shows an enabled copy button + expect(page).to have_selector('.js-copy-blob-source-btn:not(.disabled)') + end + end + end + end end - it 'opens raw mode when linking to a line in MD file' do - visit_blob('README.md', 'L1') + context 'visiting with a line number anchor' do + before do + visit_blob('files/markdown/ruby-style-guide.md', 'L1') + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # hides the rich viewer + expect(page).to have_selector('.blob-viewer[data-type="simple"]') + expect(page).to have_selector('.blob-viewer[data-type="rich"]', visible: false) + + # highlights the line in question + expect(page).to have_selector('#LC1.hll') + + # shows highlighted Markdown code + expect(page).to have_content("[PEP-8](http://www.python.org/dev/peps/pep-0008/)") - expect(page).to have_selector('#LC1.hll') - expect(page).to have_selector('.blob-viewer[data-type="simple"]') + # shows an enabled copy button + expect(page).to have_selector('.js-copy-blob-source-btn:not(.disabled)') + end + end end end - context 'binary files' do - it 'does not show view toggle buttons in toolbar' do + context 'Markdown file (stored in LFS)' do + before do + project.add_master(project.creator) + + Files::CreateService.new( + project, + project.creator, + start_branch: 'master', + branch_name: 'master', + commit_message: "Add Markdown in LFS", + file_path: 'files/lfs/file.md', + file_content: project.repository.blob_at('master', 'files/lfs/lfs_object.iso').data + ).execute + end + + context 'when LFS is enabled on the project' do + before do + allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) + project.update_attribute(:lfs_enabled, true) + + visit_blob('files/lfs/file.md') + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # hides the simple viewer + expect(page).to have_selector('.blob-viewer[data-type="simple"]', visible: false) + expect(page).to have_selector('.blob-viewer[data-type="rich"]') + + # shows an error message + expect(page).to have_content('The rendered file could not be displayed because it is stored in LFS. You can view the source or download it instead.') + + # shows a viewer switcher + expect(page).to have_selector('.js-blob-viewer-switcher') + + # does not show a copy button + expect(page).not_to have_selector('.js-copy-blob-source-btn') + end + end + + context 'switching to the simple viewer' do + before do + find('.js-blob-viewer-switcher .js-blob-viewer-switch-btn[data-viewer=simple]').click + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # hides the rich viewer + expect(page).to have_selector('.blob-viewer[data-type="simple"]') + expect(page).to have_selector('.blob-viewer[data-type="rich"]', visible: false) + + # shows an error message + expect(page).to have_content('The source could not be displayed because it is stored in LFS. You can download it instead.') + + # does not show a copy button + expect(page).not_to have_selector('.js-copy-blob-source-btn') + end + end + end + end + + context 'when LFS is disabled on the project' do + before do + visit_blob('files/lfs/file.md') + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # shows text + expect(page).to have_content('size 1575078') + + # does not show a viewer switcher + expect(page).not_to have_selector('.js-blob-viewer-switcher') + + # shows an enabled copy button + expect(page).to have_selector('.js-copy-blob-source-btn:not(.disabled)') + end + end + end + end + + context 'PDF file' do + before do + project.add_master(project.creator) + + Files::CreateService.new( + project, + project.creator, + start_branch: 'master', + branch_name: 'master', + commit_message: "Add PDF", + file_path: 'files/test.pdf', + file_content: File.read(Rails.root.join('spec/javascripts/blob/pdf/test.pdf')) + ).execute + + visit_blob('files/test.pdf') + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # shows rendered PDF + expect(page).to have_selector('.js-pdf-viewer') + + # does not show a viewer switcher + expect(page).not_to have_selector('.js-blob-viewer-switcher') + + # does not show a copy button + expect(page).not_to have_selector('.js-copy-blob-source-btn') + end + end + end + + context 'ISO file (stored in LFS)' do + context 'when LFS is enabled on the project' do + before do + allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) + project.update_attribute(:lfs_enabled, true) + + visit_blob('files/lfs/lfs_object.iso') + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # shows a download link + expect(page).to have_link('Download (1.5 MB)') + + # does not show a viewer switcher + expect(page).not_to have_selector('.js-blob-viewer-switcher') + + # does not show a copy button + expect(page).not_to have_selector('.js-copy-blob-source-btn') + end + end + end + + context 'when LFS is disabled on the project' do + before do + visit_blob('files/lfs/lfs_object.iso') + + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # shows text + expect(page).to have_content('size 1575078') + + # does not show a viewer switcher + expect(page).not_to have_selector('.js-blob-viewer-switcher') + + # shows an enabled copy button + expect(page).to have_selector('.js-copy-blob-source-btn:not(.disabled)') + end + end + end + end + + context 'ZIP file' do + before do visit_blob('Gemfile.zip') - expect(first('.file-actions .btn-group')).to have_selector('.btn', count: 1) - expect(first('.file-actions .btn-group .btn')[:title]).to eq('Download blob') + wait_for_ajax + end + + it 'displays the blob' do + aggregate_failures do + # shows a download link + expect(page).to have_link('Download (2.11 KB)') + + # does not show a viewer switcher + expect(page).not_to have_selector('.js-blob-viewer-switcher') + + # does not show a copy button + expect(page).not_to have_selector('.js-copy-blob-source-btn') + end end end end diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index 292bc0d68fd..d9154ec8152 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/spec/helpers/blob_helper_spec.rb @@ -104,4 +104,114 @@ describe BlobHelper do expect(Capybara.string(link).find_link('Edit')[:href]).to eq('/gitlab/gitlabhq/edit/master/README.md?mr_id=10') end end + + context 'viewer related' do + include FakeBlobHelpers + + let(:project) { build(:empty_project) } + + let(:viewer_class) do + Class.new(BlobViewer::Base) do + self.max_size = 1.megabyte + self.absolute_max_size = 5.megabytes + self.type = :rich + self.client_side = false + end + end + + let(:viewer) { viewer_class.new(blob) } + let(:blob) { fake_blob } + + before do + assign(:project, project) + assign(:id, File.join('master', blob.path)) + + controller.params[:controller] = 'projects/blob' + controller.params[:action] = 'show' + controller.params[:namespace_id] = project.namespace.to_param + controller.params[:project_id] = project.to_param + controller.params[:id] = File.join('master', blob.path) + end + + describe '#blob_render_error_reason' do + context 'for error :too_large' do + context 'when the blob size is larger than the absolute max size' do + let(:blob) { fake_blob(size: 10.megabytes) } + + it 'returns an error message' do + expect(helper.blob_render_error_reason(viewer, :too_large)).to eq('it is larger than 5 MB') + end + end + + context 'when the blob size is larger than the max size' do + let(:blob) { fake_blob(size: 2.megabytes) } + + it 'returns an error message' do + expect(helper.blob_render_error_reason(viewer, :too_large)).to eq('it is larger than 1 MB') + end + end + end + + context 'for error :server_side_but_stored_in_lfs' do + it 'returns an error message' do + expect(helper.blob_render_error_reason(viewer, :server_side_but_stored_in_lfs)).to eq('it is stored in LFS') + end + end + end + + describe '#blob_render_error_options' do + context 'for error :too_large' do + context 'when the max size can be overridden' do + let(:blob) { fake_blob(size: 2.megabytes) } + + it 'includes a "load it anyway" link' do + expect(helper.blob_render_error_options(viewer, :too_large)).to include(/load it anyway/) + end + end + + context 'when the max size cannot be overridden' do + let(:blob) { fake_blob(size: 10.megabytes) } + + it 'does not include a "load it anyway" link' do + expect(helper.blob_render_error_options(viewer, :too_large)).not_to include(/load it anyway/) + end + end + end + + context 'when the viewer is rich' do + context 'the blob is rendered as text' do + let(:blob) { fake_blob(path: 'file.md') } + + it 'includes a "view the source" link' do + expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).to include(/view the source/) + end + end + + context 'the blob is not rendered as text' do + let(:blob) { fake_blob(path: 'file.pdf', binary: true) } + + it 'does not include a "view the source" link' do + expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).not_to include(/view the source/) + end + end + end + + + context 'when the viewer is not rich' do + before do + viewer_class.type = :simple + end + + let(:blob) { fake_blob(path: 'file.md') } + + it 'does not include a "view the source" link' do + expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).not_to include(/view the source/) + end + end + + it 'includes a "download it" link' do + expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).to include(/download it/) + end + end + end end diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb index fd42a87c37e..80dcfeeb3b5 100644 --- a/spec/models/blob_spec.rb +++ b/spec/models/blob_spec.rb @@ -2,6 +2,14 @@ require 'rails_helper' describe Blob do + include FakeBlobHelpers + + let(:project) { build(:empty_project, lfs_enabled: true) } + + before do + allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) + end + describe '.decorate' do it 'returns NilClass when given nil' do expect(described_class.decorate(nil)).to be_nil @@ -12,7 +20,7 @@ describe Blob do context 'using a binary blob' do it 'returns the data as-is' do data = "\n\xFF\xB9\xC3" - blob = described_class.new(double(binary?: true, data: data)) + blob = fake_blob(binary: true, data: data) expect(blob.data).to eq(data) end @@ -20,12 +28,176 @@ describe Blob do context 'using a text blob' do it 'converts the data to UTF-8' do - blob = described_class.new(double(binary?: false, data: "\n\xFF\xB9\xC3")) + blob = fake_blob(binary: false, data: "\n\xFF\xB9\xC3") expect(blob.data).to eq("\n���") end end end - # TODO: Test new methods + describe '#raw_binary?' do + context 'if the blob is a valid LFS pointer' do + context 'if the extension has a rich viewer' do + context 'if the viewer is binary' do + it 'return true' do + blob = fake_blob(path: 'file.pdf', lfs: true) + + expect(blob.raw_binary?).to be_truthy + end + end + + context 'if the viewer is text-based' do + it 'return false' do + blob = fake_blob(path: 'file.md', lfs: true) + + expect(blob.raw_binary?).to be_falsey + end + end + end + + context "if the extension doesn't have a rich viewer" do + it 'returns true' do + blob = fake_blob(path: 'file.exe', lfs: true) + + expect(blob.raw_binary?).to be_truthy + end + end + end + + context 'if the blob is not an LFS pointer' do + context 'if the blob is binary' do + it 'return true' do + blob = fake_blob(path: 'file.pdf', binary: true) + + expect(blob.raw_binary?).to be_truthy + end + end + + context 'if the blob is text-based' do + it 'return false' do + blob = fake_blob(path: 'file.md') + + expect(blob.raw_binary?).to be_falsey + end + end + end + end + + describe '#extension' do + it 'returns the extension' do + blob = fake_blob(path: 'file.md') + + expect(blob.extension).to eq('md') + end + end + + describe '#simple_viewer' do + context 'when the blob is empty' do + it 'returns an empty viewer' do + blob = fake_blob(data: '') + + expect(blob.simple_viewer).to be_a(BlobViewer::Empty) + end + end + + context 'when the file represented by the blob is binary' do + it 'returns a download viewer' do + blob = fake_blob(binary: true) + + expect(blob.simple_viewer).to be_a(BlobViewer::Download) + end + end + + context 'when the file represented by the blob is text-based' do + it 'returns a text viewer' do + blob = fake_blob + + expect(blob.simple_viewer).to be_a(BlobViewer::Text) + end + end + end + + describe '#rich_viewer' do + context 'when the blob is an invalid LFS pointer' do + before do + project.lfs_enabled = false + end + + it 'returns nil' do + blob = fake_blob(path: 'file.pdf', lfs: true) + + expect(blob.rich_viewer).to be_nil + end + end + + context 'when the blob is empty' do + it 'returns nil' do + blob = fake_blob(data: '') + + expect(blob.rich_viewer).to be_nil + end + end + + context 'when the blob is a valid LFS pointer' do + it 'returns a matching viewer' do + blob = fake_blob(path: 'file.pdf', lfs: true) + + expect(blob.rich_viewer).to be_a(BlobViewer::PDF) + end + end + + context 'when the blob is binary' do + it 'returns a matching binary viewer' do + blob = fake_blob(path: 'file.pdf', binary: true) + + expect(blob.rich_viewer).to be_a(BlobViewer::PDF) + end + end + + context 'when the blob is text-based' do + it 'returns a matching text-based viewer' do + blob = fake_blob(path: 'file.md') + + expect(blob.rich_viewer).to be_a(BlobViewer::Markup) + end + end + end + + describe '#rendered_as_text?' do + context 'when ignoring errors' do + context 'when the simple viewer is text-based' do + it 'returns true' do + blob = fake_blob(path: 'file.md', size: 100.megabytes) + + expect(blob.rendered_as_text?).to be_truthy + end + end + + context 'when the simple viewer is binary' do + it 'returns false' do + blob = fake_blob(path: 'file.pdf', binary: true, size: 100.megabytes) + + expect(blob.rendered_as_text?).to be_falsey + end + end + end + + context 'when not ignoring errors' do + context 'when the viewer has render errors' do + it 'returns false' do + blob = fake_blob(path: 'file.md', size: 100.megabytes) + + expect(blob.rendered_as_text?(ignore_errors: false)).to be_falsey + end + end + + context "when the viewer doesn't have render errors" do + it 'returns true' do + blob = fake_blob(path: 'file.md') + + expect(blob.rendered_as_text?(ignore_errors: false)).to be_truthy + end + end + end + end end diff --git a/spec/models/blob_viewer/base_spec.rb b/spec/models/blob_viewer/base_spec.rb index e176d9c751b..a3e598de56d 100644 --- a/spec/models/blob_viewer/base_spec.rb +++ b/spec/models/blob_viewer/base_spec.rb @@ -1,5 +1,186 @@ -include 'spec_helper' +require 'spec_helper' describe BlobViewer::Base, model: true do - # TODO: Test + include FakeBlobHelpers + + let(:project) { build(:empty_project) } + + let(:viewer_class) do + Class.new(described_class) do + self.extensions = %w(pdf) + self.max_size = 1.megabyte + self.absolute_max_size = 5.megabytes + self.client_side = false + end + end + + let(:viewer) { viewer_class.new(blob) } + + describe '.can_render?' do + context 'when the extension is supported' do + let(:blob) { fake_blob(path: 'file.pdf') } + + it 'returns true' do + expect(viewer_class.can_render?(blob)).to be_truthy + end + end + + context 'when the extension is not supported' do + let(:blob) { fake_blob(path: 'file.txt') } + + it 'returns false' do + expect(viewer_class.can_render?(blob)).to be_falsey + end + end + end + + describe '#too_large?' do + context 'when the blob size is larger than the max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) } + + it 'returns true' do + expect(viewer.too_large?).to be_truthy + end + end + + context 'when the blob size is smaller than the max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) } + + it 'returns false' do + expect(viewer.too_large?).to be_falsey + end + end + end + + describe '#absolutely_too_large?' do + context 'when the blob size is larger than the absolute max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) } + + it 'returns true' do + expect(viewer.absolutely_too_large?).to be_truthy + end + end + + context 'when the blob size is smaller than the absolute max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) } + + it 'returns false' do + expect(viewer.absolutely_too_large?).to be_falsey + end + end + end + + describe '#can_override_max_size?' do + context 'when the blob size is larger than the max size' do + context 'when the blob size is larger than the absolute max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) } + + it 'returns false' do + expect(viewer.can_override_max_size?).to be_falsey + end + end + + context 'when the blob size is smaller than the absolute max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) } + + it 'returns true' do + expect(viewer.can_override_max_size?).to be_truthy + end + end + end + + context 'when the blob size is smaller than the max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) } + + it 'returns false' do + expect(viewer.can_override_max_size?).to be_falsey + end + end + end + + describe '#render_error' do + context 'when the max size is overridden' do + before do + viewer.override_max_size = true + end + + context 'when the blob size is larger than the absolute max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 10.megabytes) } + + it 'returns :too_large' do + expect(viewer.render_error).to eq(:too_large) + end + end + + context 'when the blob size is smaller than the absolute max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) } + + it 'returns nil' do + expect(viewer.render_error).to be_nil + end + end + end + + context 'when the max size is not overridden' do + context 'when the blob size is larger than the max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 2.megabytes) } + + it 'returns :too_large' do + expect(viewer.render_error).to eq(:too_large) + end + end + + context 'when the blob size is smaller than the max size' do + let(:blob) { fake_blob(path: 'file.pdf', size: 10.kilobytes) } + + it 'returns nil' do + expect(viewer.render_error).to be_nil + end + end + end + + context 'when the viewer is server side but the blob is stored in LFS' do + let(:project) { build(:empty_project, lfs_enabled: true) } + + let(:blob) { fake_blob(path: 'file.pdf', lfs: true) } + + before do + allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) + end + + it 'return :server_side_but_stored_in_lfs' do + expect(viewer.render_error).to eq(:server_side_but_stored_in_lfs) + end + end + end + + describe '#prepare!' do + context 'when the viewer is server side' do + let(:blob) { fake_blob(path: 'file.md') } + + before do + viewer_class.client_side = false + end + + it 'loads all blob data' do + expect(blob).to receive(:load_all_data!) + + viewer.prepare! + end + end + + context 'when the viewer is client side' do + let(:blob) { fake_blob(path: 'file.md') } + + before do + viewer_class.client_side = true + end + + it "doesn't load all blob data" do + expect(blob).not_to receive(:load_all_data!) + + viewer.prepare! + end + end + end end diff --git a/spec/support/helpers/fake_blob_helpers.rb b/spec/support/helpers/fake_blob_helpers.rb new file mode 100644 index 00000000000..b29af732ad3 --- /dev/null +++ b/spec/support/helpers/fake_blob_helpers.rb @@ -0,0 +1,50 @@ +module FakeBlobHelpers + class FakeBlob + include Linguist::BlobHelper + + attr_reader :path, :size, :data, :lfs_oid, :lfs_size + + def initialize(path: 'file.txt', size: 1.kilobyte, data: 'foo', binary: false, lfs: nil) + @path = path + @size = size + @data = data + @binary = binary + + @lfs_pointer = lfs.present? + if @lfs_pointer + @lfs_oid = SecureRandom.hex(20) + @lfs_size = 1.megabyte + end + end + + alias_method :name, :path + + def mode + nil + end + + def id + 0 + end + + def binary? + @binary + end + + def load_all_data!(repository) + # No-op + end + + def lfs_pointer? + @lfs_pointer + end + + def truncated? + false + end + end + + def fake_blob(**kwargs) + Blob.decorate(FakeBlob.new(**kwargs), project) + end +end diff --git a/spec/views/projects/blob/_render_error.html.haml_spec.rb b/spec/views/projects/blob/_render_error.html.haml_spec.rb deleted file mode 100644 index fabd444a6ad..00000000000 --- a/spec/views/projects/blob/_render_error.html.haml_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'spec_helper' - -describe 'app/views/projects/blob/_render_error.html.haml' do - # TODO: Test -end diff --git a/spec/views/projects/blob/_viewer.html.haml_spec.rb b/spec/views/projects/blob/_viewer.html.haml_spec.rb index 1d2055c10fc..c790dc79b0a 100644 --- a/spec/views/projects/blob/_viewer.html.haml_spec.rb +++ b/spec/views/projects/blob/_viewer.html.haml_spec.rb @@ -1,5 +1,97 @@ require 'spec_helper' -describe 'app/views/projects/blob/_viewer.html.haml' do - # TODO: Test +describe 'projects/blob/_viewer.html.haml', :view do + include FakeBlobHelpers + + let(:project) { build(:empty_project) } + + let(:viewer_class) do + Class.new(BlobViewer::Base) do + include BlobViewer::Rich + + self.partial_name = 'text' + self.max_size = 1.megabyte + self.absolute_max_size = 5.megabytes + self.client_side = false + end + end + + let(:viewer) { viewer_class.new(blob) } + let(:blob) { fake_blob } + + before do + assign(:project, project) + assign(:id, File.join('master', blob.path)) + + controller.params[:controller] = 'projects/blob' + controller.params[:action] = 'show' + controller.params[:namespace_id] = project.namespace.to_param + controller.params[:project_id] = project.to_param + controller.params[:id] = File.join('master', blob.path) + end + + def render_view + render partial: 'projects/blob/viewer', locals: { viewer: viewer } + end + + context 'when the viewer is server side' do + before do + viewer_class.client_side = false + end + + context 'when there is no render error' do + it 'adds a URL to the blob viewer element' do + render_view + + + expect(rendered).to have_css('.blob-viewer[data-url]') + end + + it 'displays a spinner' do + render_view + + expect(rendered).to have_css('i[aria-label="Loading content"]') + end + end + + context 'when there is a render error' do + let(:blob) { fake_blob(size: 10.megabytes) } + + it 'renders the error' do + render_view + + expect(view).to render_template('projects/blob/_render_error') + end + end + end + + context 'when the viewer is client side' do + before do + viewer_class.client_side = true + end + + context 'when there is no render error' do + it 'prepares the viewer' do + expect(viewer).to receive(:prepare!) + + render_view + end + + it 'renders the viewer' do + render_view + + expect(view).to render_template('projects/blob/viewers/_text') + end + end + + context 'when there is a render error' do + let(:blob) { fake_blob(size: 10.megabytes) } + + it 'renders the error' do + render_view + + expect(view).to render_template('projects/blob/_render_error') + end + end + end end diff --git a/spec/views/projects/blob/_viewer_switcher.html.haml_spec.rb b/spec/views/projects/blob/_viewer_switcher.html.haml_spec.rb deleted file mode 100644 index 337f40d50df..00000000000 --- a/spec/views/projects/blob/_viewer_switcher.html.haml_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'spec_helper' - -describe 'app/views/projects/blob/_viewer_switcher.html.haml' do - # TODO: Test -end -- cgit v1.2.1 From 61fd9ccf47f66ef2d653a35f2b13f36c2a76f4a9 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 25 Apr 2017 16:34:52 -0500 Subject: Add changelog --- changelogs/unreleased/dm-blob-viewers.yml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 changelogs/unreleased/dm-blob-viewers.yml diff --git a/changelogs/unreleased/dm-blob-viewers.yml b/changelogs/unreleased/dm-blob-viewers.yml new file mode 100644 index 00000000000..5e0d41f3f29 --- /dev/null +++ b/changelogs/unreleased/dm-blob-viewers.yml @@ -0,0 +1,5 @@ +--- +title: Add Source/Rendered switch to blobs for SVG, Markdown, Asciidoc and other text + files that can be rendered +merge_request: +author: -- cgit v1.2.1 From dc4726f0a707019c64e60e58d45bb647a43d4ec8 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Tue, 25 Apr 2017 16:38:15 -0500 Subject: Satisfy Rubocop --- spec/helpers/blob_helper_spec.rb | 1 - spec/views/projects/blob/_viewer.html.haml_spec.rb | 1 - 2 files changed, 2 deletions(-) diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index d9154ec8152..8a89cd71e98 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/spec/helpers/blob_helper_spec.rb @@ -196,7 +196,6 @@ describe BlobHelper do end end - context 'when the viewer is not rich' do before do viewer_class.type = :simple diff --git a/spec/views/projects/blob/_viewer.html.haml_spec.rb b/spec/views/projects/blob/_viewer.html.haml_spec.rb index c790dc79b0a..a4915264abe 100644 --- a/spec/views/projects/blob/_viewer.html.haml_spec.rb +++ b/spec/views/projects/blob/_viewer.html.haml_spec.rb @@ -43,7 +43,6 @@ describe 'projects/blob/_viewer.html.haml', :view do it 'adds a URL to the blob viewer element' do render_view - expect(rendered).to have_css('.blob-viewer[data-url]') end -- cgit v1.2.1 From 95658fb62a955a40260d5b7c4ccd51ce72ae1fb9 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 26 Apr 2017 10:09:07 +0100 Subject: Fixed failing JS tests --- app/assets/javascripts/blob/viewer/index.js | 2 +- spec/javascripts/blob/viewer/index_spec.js | 10 +++++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/blob/viewer/index.js b/app/assets/javascripts/blob/viewer/index.js index 8a5fb187a71..7efa8537298 100644 --- a/app/assets/javascripts/blob/viewer/index.js +++ b/app/assets/javascripts/blob/viewer/index.js @@ -1,4 +1,3 @@ -/* eslint-disable no-new */ /* global Flash */ export default class BlobViewer { constructor() { @@ -76,6 +75,7 @@ export default class BlobViewer { url, dataType: 'JSON', }) + .fail(() => new Flash('Error loading source view')) .done((data) => { viewer.innerHTML = data.html; $(viewer).syntaxHighlight(); diff --git a/spec/javascripts/blob/viewer/index_spec.js b/spec/javascripts/blob/viewer/index_spec.js index fe45ee3c083..2031898cf54 100644 --- a/spec/javascripts/blob/viewer/index_spec.js +++ b/spec/javascripts/blob/viewer/index_spec.js @@ -1,7 +1,7 @@ /* eslint-disable no-new */ import BlobViewer from '~/blob/viewer/index'; -describe('Blob viewer', () => { +fdescribe('Blob viewer', () => { preloadFixtures('blob/show.html.raw'); beforeEach(() => { @@ -73,6 +73,10 @@ describe('Blob viewer', () => { document.querySelector('.blob-viewer[data-type="simple"]').getAttribute('data-loaded'), ).toBe('true'); + done(); + }) + .catch(() => { + fail(); done(); }); }); @@ -87,7 +91,7 @@ describe('Blob viewer', () => { it('has tooltip when disabled', () => { expect( document.querySelector('.js-copy-blob-source-btn').getAttribute('data-original-title'), - ).toBe('Switch to the source view to copy the source to the clipboard'); + ).toBe('Switch to the source to copy it to the clipboard'); }); it('enables after switching to simple view', (done) => { @@ -111,7 +115,7 @@ describe('Blob viewer', () => { expect( document.querySelector('.js-copy-blob-source-btn').getAttribute('data-original-title'), - ).toBe('Copy to clipboard'); + ).toBe('Copy source to clipboard'); done(); }); -- cgit v1.2.1 From 88380a04c629d21d39e59a6066cec63b8db89126 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Wed, 26 Apr 2017 10:19:09 +0100 Subject: Added specs for JS method to switch views --- spec/javascripts/blob/viewer/index_spec.js | 41 ++++++++++++++++++++++++++++-- 1 file changed, 39 insertions(+), 2 deletions(-) diff --git a/spec/javascripts/blob/viewer/index_spec.js b/spec/javascripts/blob/viewer/index_spec.js index 2031898cf54..13f122b68b2 100644 --- a/spec/javascripts/blob/viewer/index_spec.js +++ b/spec/javascripts/blob/viewer/index_spec.js @@ -1,14 +1,15 @@ /* eslint-disable no-new */ import BlobViewer from '~/blob/viewer/index'; -fdescribe('Blob viewer', () => { +describe('Blob viewer', () => { + let blob; preloadFixtures('blob/show.html.raw'); beforeEach(() => { loadFixtures('blob/show.html.raw'); $('#modal-upload-blob').remove(); - new BlobViewer(); + blob = new BlobViewer(); spyOn($, 'ajax').and.callFake(() => { const d = $.Deferred(); @@ -121,4 +122,40 @@ fdescribe('Blob viewer', () => { }); }); }); + + describe('switchToViewer', () => { + it('removes active class from old viewer button', () => { + blob.switchToViewer('simple'); + + expect( + document.querySelector('.js-blob-viewer-switch-btn.active[data-viewer="rich"]'), + ).toBeNull(); + }); + + it('adds active class to new viewer button', () => { + const simpleBtn = document.querySelector('.js-blob-viewer-switch-btn[data-viewer="simple"]'); + + spyOn(simpleBtn, 'blur'); + + blob.switchToViewer('simple'); + + expect( + simpleBtn.classList.contains('active'), + ).toBeTruthy(); + expect(simpleBtn.blur).toHaveBeenCalled(); + }); + + it('sends AJAX request when switching to simple view', () => { + blob.switchToViewer('simple'); + + expect($.ajax).toHaveBeenCalled(); + }); + + it('does not send AJAX request when switching to rich view', () => { + blob.switchToViewer('simple'); + blob.switchToViewer('rich'); + + expect($.ajax.calls.count()).toBe(1); + }); + }); }); -- cgit v1.2.1 From 787866a91f4a7125336707dc9e58d338d0d6fde2 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 26 Apr 2017 15:16:38 -0500 Subject: Explain how viewers are selected from RICH_VIEWERS --- app/models/blob.rb | 34 +++++++++++++++++++++++++++------- app/models/blob_viewer/text_stl.rb | 7 +------ 2 files changed, 28 insertions(+), 13 deletions(-) diff --git a/app/models/blob.rb b/app/models/blob.rb index e87b418d242..290df5d5520 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -5,17 +5,37 @@ class Blob < SimpleDelegator MAXIMUM_TEXT_HIGHLIGHT_SIZE = 1.megabyte + # Finding a viewer for a blob happens based only on extension and whether the + # blob is binary or text, which means 1 blob should only be matched by 1 viewer, + # and the order of these viewers doesn't really matter. + # + # However, when the blob is an LFS pointer, we cannot know for sure whether the + # file being pointed to is binary or text. In this case, we match only on + # extension, preferring binary viewers over text ones if both exist, since the + # large files referred to in "Large File Storage" are much more likely to be + # binary than text. + # + # `.stl` files, for example, exist in both binary and text forms, and are + # handled by different viewers (`BinarySTL` and `TextSTL`) depending on blob + # type. LFS pointers to `.stl` files are assumed to always be the binary kind, + # and use the `BinarySTL` viewer. RICH_VIEWERS = [ + BlobViewer::Markup, + BlobViewer::Notebook, + BlobViewer::SVG, + BlobViewer::Image, - BlobViewer::PDF, BlobViewer::Sketch, + + BlobViewer::PDF, + BlobViewer::BinarySTL, BlobViewer::TextSTL, - BlobViewer::Notebook, - BlobViewer::SVG, - BlobViewer::Markup, ].freeze + BINARY_VIEWERS = RICH_VIEWERS.select(&:binary?).freeze + TEXT_VIEWERS = RICH_VIEWERS.select(&:text?).freeze + attr_reader :project # Wrap a Gitlab::Git::Blob object, or return nil when given nil @@ -147,11 +167,11 @@ class Blob < SimpleDelegator classes = if valid_lfs_pointer? - RICH_VIEWERS + BINARY_VIEWERS + TEXT_VIEWERS elsif binary? - RICH_VIEWERS.select(&:binary?) + BINARY_VIEWERS else # text - RICH_VIEWERS.select(&:text?) + TEXT_VIEWERS end classes.find { |viewer_class| viewer_class.can_render?(self) } diff --git a/app/models/blob_viewer/text_stl.rb b/app/models/blob_viewer/text_stl.rb index 2bb136a2702..8184dc0104c 100644 --- a/app/models/blob_viewer/text_stl.rb +++ b/app/models/blob_viewer/text_stl.rb @@ -1,10 +1,5 @@ module BlobViewer - class TextSTL < Base - include Rich - include ClientSide - - self.partial_name = 'stl' - self.extensions = %w(stl) + class TextSTL < BinarySTL self.binary = false end end -- cgit v1.2.1 From c6b2a22f63bc7561beb3e596b14c62021d64c6e7 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 26 Apr 2017 15:29:12 -0500 Subject: Explain BlobViewer::Base#render_error --- app/models/blob_viewer/base.rb | 15 +++++++++++++++ app/models/blob_viewer/download.rb | 1 + 2 files changed, 16 insertions(+) diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index d3674530273..ab89429d07d 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -55,8 +55,23 @@ module BlobViewer too_large? && !absolutely_too_large? end + # This method is used on the server side to check whether we can attempt to + # render the blob at all. Human-readible error messages are found in the + # `BlobHelper#blob_render_error_reason` helper. + # + # This method does not and should not load the entire blob contents into + # memory, and should not be overridden to do so in order to validate the + # format of the blob. + # + # Prefer to implement a client-side viewer, where the JS component loads the + # binary from `blob_raw_url` and does its own format validation and error + # rendering, especially for potentially large binary formats. def render_error if server_side_but_stored_in_lfs? + # Files stored in LFS can only be rendered using a client-side viewer, + # since we do not want to read large amounts of data into memory on the + # server side. Client-side viewers use JS and can fetch the file from + # `blob_raw_url` using AJAX. :server_side_but_stored_in_lfs elsif override_max_size ? absolutely_too_large? : too_large? :too_large diff --git a/app/models/blob_viewer/download.rb b/app/models/blob_viewer/download.rb index 7bc45fe992c..adc06587f69 100644 --- a/app/models/blob_viewer/download.rb +++ b/app/models/blob_viewer/download.rb @@ -9,6 +9,7 @@ module BlobViewer self.partial_name = 'download' self.binary = true + # We can always render the Download viewer, even if the blob is in LFS or too large. def render_error nil end -- cgit v1.2.1 From 87a3bd26fa1c6379801062fd65fea59e587baeee Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 26 Apr 2017 15:48:49 -0500 Subject: Address feedback --- app/helpers/blob_helper.rb | 8 ++-- app/models/blob_viewer/base.rb | 21 +++++----- app/views/projects/blob/_render_error.html.haml | 4 +- app/views/projects/blob/_viewer.html.haml | 2 +- spec/features/projects/blobs/blob_show_spec.rb | 4 +- spec/helpers/blob_helper_spec.rb | 54 ++++++++++++++----------- spec/models/blob_spec.rb | 4 +- 7 files changed, 53 insertions(+), 44 deletions(-) diff --git a/app/helpers/blob_helper.rb b/app/helpers/blob_helper.rb index 00cf0ac96c9..cc47654dc06 100644 --- a/app/helpers/blob_helper.rb +++ b/app/helpers/blob_helper.rb @@ -216,8 +216,8 @@ module BlobHelper link_to icon('file-code-o'), path, class: 'btn btn-sm has-tooltip', target: '_blank', rel: 'noopener noreferrer', title: 'Open raw', data: { container: 'body' } end - def blob_render_error_reason(viewer, error) - case error + def blob_render_error_reason(viewer) + case viewer.render_error when :too_large max_size = if viewer.absolutely_too_large? @@ -231,10 +231,10 @@ module BlobHelper end end - def blob_render_error_options(viewer, error) + def blob_render_error_options(viewer) options = [] - if error == :too_large && viewer.can_override_max_size? + if viewer.render_error == :too_large && viewer.can_override_max_size? options << link_to('load it anyway', url_for(params.merge(viewer: viewer.type, override_max_size: true, format: nil))) end diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index ab89429d07d..3ca73565d81 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -67,15 +67,18 @@ module BlobViewer # binary from `blob_raw_url` and does its own format validation and error # rendering, especially for potentially large binary formats. def render_error - if server_side_but_stored_in_lfs? - # Files stored in LFS can only be rendered using a client-side viewer, - # since we do not want to read large amounts of data into memory on the - # server side. Client-side viewers use JS and can fetch the file from - # `blob_raw_url` using AJAX. - :server_side_but_stored_in_lfs - elsif override_max_size ? absolutely_too_large? : too_large? - :too_large - end + return @render_error if defined?(@render_error) + + @render_error = + if server_side_but_stored_in_lfs? + # Files stored in LFS can only be rendered using a client-side viewer, + # since we do not want to read large amounts of data into memory on the + # server side. Client-side viewers use JS and can fetch the file from + # `blob_raw_url` using AJAX. + :server_side_but_stored_in_lfs + elsif override_max_size ? absolutely_too_large? : too_large? + :too_large + end end def prepare! diff --git a/app/views/projects/blob/_render_error.html.haml b/app/views/projects/blob/_render_error.html.haml index 026b7c95163..9eef6cafd04 100644 --- a/app/views/projects/blob/_render_error.html.haml +++ b/app/views/projects/blob/_render_error.html.haml @@ -1,7 +1,7 @@ .file-content.code .nothing-here-block - The #{viewer.switcher_title} could not be displayed because #{blob_render_error_reason(viewer, error)}. + The #{viewer.switcher_title} could not be displayed because #{blob_render_error_reason(viewer)}. You can - = blob_render_error_options(viewer, error).to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe + = blob_render_error_options(viewer).to_sentence(two_words_connector: ' or ', last_word_connector: ', or ').html_safe instead. diff --git a/app/views/projects/blob/_viewer.html.haml b/app/views/projects/blob/_viewer.html.haml index 1c45c4a68ce..5326bb3e0cf 100644 --- a/app/views/projects/blob/_viewer.html.haml +++ b/app/views/projects/blob/_viewer.html.haml @@ -8,7 +8,7 @@ .text-center.prepend-top-default.append-bottom-default = icon('spinner spin 2x', 'aria-hidden' => 'true', 'aria-label' => 'Loading content') - elsif render_error - = render 'projects/blob/render_error', viewer: viewer, error: render_error + = render 'projects/blob/render_error', viewer: viewer - else - viewer.prepare! = render viewer.partial_path, viewer: viewer diff --git a/spec/features/projects/blobs/blob_show_spec.rb b/spec/features/projects/blobs/blob_show_spec.rb index aea9f66eec3..cc11cb7a55f 100644 --- a/spec/features/projects/blobs/blob_show_spec.rb +++ b/spec/features/projects/blobs/blob_show_spec.rb @@ -149,7 +149,7 @@ feature 'File blob', :js, feature: true do wait_for_ajax end - it 'displays the blob' do + it 'displays an error' do aggregate_failures do # hides the simple viewer expect(page).to have_selector('.blob-viewer[data-type="simple"]', visible: false) @@ -173,7 +173,7 @@ feature 'File blob', :js, feature: true do wait_for_ajax end - it 'displays the blob' do + it 'displays an error' do aggregate_failures do # hides the rich viewer expect(page).to have_selector('.blob-viewer[data-type="simple"]') diff --git a/spec/helpers/blob_helper_spec.rb b/spec/helpers/blob_helper_spec.rb index 8a89cd71e98..379f62f73e1 100644 --- a/spec/helpers/blob_helper_spec.rb +++ b/spec/helpers/blob_helper_spec.rb @@ -108,7 +108,11 @@ describe BlobHelper do context 'viewer related' do include FakeBlobHelpers - let(:project) { build(:empty_project) } + let(:project) { build(:empty_project, lfs_enabled: true) } + + before do + allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) + end let(:viewer_class) do Class.new(BlobViewer::Base) do @@ -122,24 +126,13 @@ describe BlobHelper do let(:viewer) { viewer_class.new(blob) } let(:blob) { fake_blob } - before do - assign(:project, project) - assign(:id, File.join('master', blob.path)) - - controller.params[:controller] = 'projects/blob' - controller.params[:action] = 'show' - controller.params[:namespace_id] = project.namespace.to_param - controller.params[:project_id] = project.to_param - controller.params[:id] = File.join('master', blob.path) - end - describe '#blob_render_error_reason' do context 'for error :too_large' do context 'when the blob size is larger than the absolute max size' do let(:blob) { fake_blob(size: 10.megabytes) } it 'returns an error message' do - expect(helper.blob_render_error_reason(viewer, :too_large)).to eq('it is larger than 5 MB') + expect(helper.blob_render_error_reason(viewer)).to eq('it is larger than 5 MB') end end @@ -147,25 +140,38 @@ describe BlobHelper do let(:blob) { fake_blob(size: 2.megabytes) } it 'returns an error message' do - expect(helper.blob_render_error_reason(viewer, :too_large)).to eq('it is larger than 1 MB') + expect(helper.blob_render_error_reason(viewer)).to eq('it is larger than 1 MB') end end end context 'for error :server_side_but_stored_in_lfs' do + let(:blob) { fake_blob(lfs: true) } + it 'returns an error message' do - expect(helper.blob_render_error_reason(viewer, :server_side_but_stored_in_lfs)).to eq('it is stored in LFS') + expect(helper.blob_render_error_reason(viewer)).to eq('it is stored in LFS') end end end describe '#blob_render_error_options' do + before do + assign(:project, project) + assign(:id, File.join('master', blob.path)) + + controller.params[:controller] = 'projects/blob' + controller.params[:action] = 'show' + controller.params[:namespace_id] = project.namespace.to_param + controller.params[:project_id] = project.to_param + controller.params[:id] = File.join('master', blob.path) + end + context 'for error :too_large' do context 'when the max size can be overridden' do let(:blob) { fake_blob(size: 2.megabytes) } it 'includes a "load it anyway" link' do - expect(helper.blob_render_error_options(viewer, :too_large)).to include(/load it anyway/) + expect(helper.blob_render_error_options(viewer)).to include(/load it anyway/) end end @@ -173,25 +179,25 @@ describe BlobHelper do let(:blob) { fake_blob(size: 10.megabytes) } it 'does not include a "load it anyway" link' do - expect(helper.blob_render_error_options(viewer, :too_large)).not_to include(/load it anyway/) + expect(helper.blob_render_error_options(viewer)).not_to include(/load it anyway/) end end end context 'when the viewer is rich' do context 'the blob is rendered as text' do - let(:blob) { fake_blob(path: 'file.md') } + let(:blob) { fake_blob(path: 'file.md', lfs: true) } it 'includes a "view the source" link' do - expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).to include(/view the source/) + expect(helper.blob_render_error_options(viewer)).to include(/view the source/) end end context 'the blob is not rendered as text' do - let(:blob) { fake_blob(path: 'file.pdf', binary: true) } + let(:blob) { fake_blob(path: 'file.pdf', binary: true, lfs: true) } it 'does not include a "view the source" link' do - expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).not_to include(/view the source/) + expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/) end end end @@ -201,15 +207,15 @@ describe BlobHelper do viewer_class.type = :simple end - let(:blob) { fake_blob(path: 'file.md') } + let(:blob) { fake_blob(path: 'file.md', lfs: true) } it 'does not include a "view the source" link' do - expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).not_to include(/view the source/) + expect(helper.blob_render_error_options(viewer)).not_to include(/view the source/) end end it 'includes a "download it" link' do - expect(helper.blob_render_error_options(viewer, :server_side_but_stored_in_lfs)).to include(/download it/) + expect(helper.blob_render_error_options(viewer)).to include(/download it/) end end end diff --git a/spec/models/blob_spec.rb b/spec/models/blob_spec.rb index 80dcfeeb3b5..7e8a1c8add7 100644 --- a/spec/models/blob_spec.rb +++ b/spec/models/blob_spec.rb @@ -39,7 +39,7 @@ describe Blob do context 'if the blob is a valid LFS pointer' do context 'if the extension has a rich viewer' do context 'if the viewer is binary' do - it 'return true' do + it 'returns true' do blob = fake_blob(path: 'file.pdf', lfs: true) expect(blob.raw_binary?).to be_truthy @@ -66,7 +66,7 @@ describe Blob do context 'if the blob is not an LFS pointer' do context 'if the blob is binary' do - it 'return true' do + it 'returns true' do blob = fake_blob(path: 'file.pdf', binary: true) expect(blob.raw_binary?).to be_truthy -- cgit v1.2.1 From 6287e2825c7efad95d1eab65cdec6818c9df829a Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 27 Apr 2017 08:42:32 -0500 Subject: Fix typo --- app/models/blob_viewer/base.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/blob_viewer/base.rb b/app/models/blob_viewer/base.rb index 3ca73565d81..f944b00c9d3 100644 --- a/app/models/blob_viewer/base.rb +++ b/app/models/blob_viewer/base.rb @@ -56,7 +56,7 @@ module BlobViewer end # This method is used on the server side to check whether we can attempt to - # render the blob at all. Human-readible error messages are found in the + # render the blob at all. Human-readable error messages are found in the # `BlobHelper#blob_render_error_reason` helper. # # This method does not and should not load the entire blob contents into -- cgit v1.2.1 From b6566277be7ef91cd998f48a229e1e95d01397f3 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 27 Apr 2017 12:22:16 -0500 Subject: Add variable to save us a double lookup --- app/assets/javascripts/line_highlighter.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/line_highlighter.js b/app/assets/javascripts/line_highlighter.js index 5ffa6283a03..a6f7bea99f5 100644 --- a/app/assets/javascripts/line_highlighter.js +++ b/app/assets/javascripts/line_highlighter.js @@ -57,8 +57,9 @@ require('vendor/jquery.scrollTo'); } LineHighlighter.prototype.bindEvents = function() { - $('#blob-content-holder').on('click', 'a[data-line-number]', this.clickHandler); - $('#blob-content-holder').on('highlight:line', this.highlightHash); + const $blobContentHolder = $('#blob-content-holder'); + $blobContentHolder.on('click', 'a[data-line-number]', this.clickHandler); + $blobContentHolder.on('highlight:line', this.highlightHash); }; LineHighlighter.prototype.highlightHash = function() { -- cgit v1.2.1 From 6368072fd7bbee0f79de358aa8645f463d768bef Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Mon, 24 Apr 2017 16:23:51 +0100 Subject: improves test settings for chat notification services for empty projects --- .../project_services/chat_notification_service.rb | 2 +- app/models/service.rb | 2 +- ...est-settings-for-services-in-empty-projects.yml | 4 ++ lib/gitlab/data_builder/push.rb | 9 ++-- .../projects/services_controller_spec.rb | 49 +++++++++++++++++++-- .../chat_notification_service_spec.rb | 4 +- spec/models/service_spec.rb | 51 ++++++++++++---------- 7 files changed, 85 insertions(+), 36 deletions(-) create mode 100644 changelogs/unreleased/31138-improve-test-settings-for-services-in-empty-projects.yml diff --git a/app/models/project_services/chat_notification_service.rb b/app/models/project_services/chat_notification_service.rb index f2dfb87dbda..fa782c6fbb7 100644 --- a/app/models/project_services/chat_notification_service.rb +++ b/app/models/project_services/chat_notification_service.rb @@ -22,7 +22,7 @@ class ChatNotificationService < Service end def can_test? - super && valid? + valid? end def self.supported_events diff --git a/app/models/service.rb b/app/models/service.rb index dc76bf925d3..720cdfa00aa 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -131,7 +131,7 @@ class Service < ActiveRecord::Base end def can_test? - !project.empty_repo? + true end # reason why service cannot be tested diff --git a/changelogs/unreleased/31138-improve-test-settings-for-services-in-empty-projects.yml b/changelogs/unreleased/31138-improve-test-settings-for-services-in-empty-projects.yml new file mode 100644 index 00000000000..cb1de425d66 --- /dev/null +++ b/changelogs/unreleased/31138-improve-test-settings-for-services-in-empty-projects.yml @@ -0,0 +1,4 @@ +--- +title: Improves test settings for chat notification services for empty projects +merge_request: 10886 +author: diff --git a/lib/gitlab/data_builder/push.rb b/lib/gitlab/data_builder/push.rb index d76aa38f741..1ff34553f0a 100644 --- a/lib/gitlab/data_builder/push.rb +++ b/lib/gitlab/data_builder/push.rb @@ -41,7 +41,7 @@ module Gitlab type = Gitlab::Git.tag_ref?(ref) ? 'tag_push' : 'push' # Hash to be passed as post_receive_data - data = { + { object_kind: type, event_name: type, before: oldrev, @@ -61,16 +61,15 @@ module Gitlab repository: project.hook_attrs.slice(:name, :url, :description, :homepage, :git_http_url, :git_ssh_url, :visibility_level) } - - data end # This method provide a sample data generated with # existing project and commits to test webhooks def build_sample(project, user) - commits = project.repository.commits(project.default_branch, limit: 3) ref = "#{Gitlab::Git::BRANCH_REF_PREFIX}#{project.default_branch}" - build(project, user, commits.last.id, commits.first.id, ref, commits) + commits = project.repository.commits(project.default_branch.to_s, limit: 3) rescue [] + + build(project, user, commits.last&.id, commits.first&.id, ref, commits) end def checkout_sha(repository, newrev, ref) diff --git a/spec/controllers/projects/services_controller_spec.rb b/spec/controllers/projects/services_controller_spec.rb index 16365642a34..2d892f4a2b7 100644 --- a/spec/controllers/projects/services_controller_spec.rb +++ b/spec/controllers/projects/services_controller_spec.rb @@ -8,6 +8,7 @@ describe Projects::ServicesController do before do sign_in(user) project.team << [user, :master] + controller.instance_variable_set(:@project, project) controller.instance_variable_set(:@service, service) end @@ -18,20 +19,60 @@ describe Projects::ServicesController do end describe "#test" do + context 'when can_test? returns false' do + it 'renders 404' do + allow_any_instance_of(Service).to receive(:can_test?).and_return(false) + + get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html + + expect(response).to have_http_status(404) + end + end + context 'success' do + context 'with empty project' do + let(:project) { create(:empty_project) } + + context 'with chat notification service' do + let(:service) { project.create_microsoft_teams_service(webhook: 'http://webhook.com') } + + it 'redirects and show success message' do + allow_any_instance_of(MicrosoftTeams::Notifier).to receive(:ping).and_return(true) + + get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html + + expect(response).to redirect_to(root_path) + expect(flash[:notice]).to eq('We sent a request to the provided URL') + end + end + + it 'redirects and show success message' do + expect(service).to receive(:test).and_return(success: true, result: 'done') + + get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html + + expect(response).to redirect_to(root_path) + expect(flash[:notice]).to eq('We sent a request to the provided URL') + end + end + it "redirects and show success message" do - expect(service).to receive(:test).and_return({ success: true, result: 'done' }) + expect(service).to receive(:test).and_return(success: true, result: 'done') + get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html - expect(response.status).to redirect_to('/') + + expect(response).to redirect_to(root_path) expect(flash[:notice]).to eq('We sent a request to the provided URL') end end context 'failure' do it "redirects and show failure message" do - expect(service).to receive(:test).and_return({ success: false, result: 'Bad test' }) + expect(service).to receive(:test).and_return(success: false, result: 'Bad test') + get :test, namespace_id: project.namespace.id, project_id: project.id, id: service.id, format: :html - expect(response.status).to redirect_to('/') + + expect(response).to redirect_to(root_path) expect(flash[:alert]).to eq('We tried to send a request to the provided URL but an error occurred: Bad test') end end diff --git a/spec/models/project_services/chat_notification_service_spec.rb b/spec/models/project_services/chat_notification_service_spec.rb index 592c90cda36..8fbe42248ae 100644 --- a/spec/models/project_services/chat_notification_service_spec.rb +++ b/spec/models/project_services/chat_notification_service_spec.rb @@ -11,10 +11,10 @@ describe ChatNotificationService, models: true do describe '#can_test?' do context 'with empty repository' do - it 'returns false' do + it 'returns true' do subject.project = create(:empty_project, :empty_repo) - expect(subject.can_test?).to be false + expect(subject.can_test?).to be true end end diff --git a/spec/models/service_spec.rb b/spec/models/service_spec.rb index 0e2f07e945f..196746fbd24 100644 --- a/spec/models/service_spec.rb +++ b/spec/models/service_spec.rb @@ -7,43 +7,48 @@ describe Service, models: true do end describe "Test Button" do - before do - @service = Service.new - end + describe '#can_test?' do + let(:service) { create(:service, project: project) } - describe "Testable" do - let(:project) { create(:project, :repository) } + context 'when repository is not empty' do + let(:project) { create(:project, :repository) } - before do - allow(@service).to receive(:project).and_return(project) - @testable = @service.can_test? + it 'returns true' do + expect(service.can_test?).to be true + end end - describe '#can_test?' do - it { expect(@testable).to eq(true) } + context 'when repository is empty' do + let(:project) { create(:empty_project) } + + it 'returns true' do + expect(service.can_test?).to be true + end end + end - describe '#test' do - let(:data) { 'test' } + describe '#test' do + let(:data) { 'test' } + let(:service) { create(:service, project: project) } + + context 'when repository is not empty' do + let(:project) { create(:project, :repository) } it 'test runs execute' do - expect(@service).to receive(:execute).with(data) + expect(service).to receive(:execute).with(data) - @service.test(data) + service.test(data) end end - end - describe "With commits" do - let(:project) { create(:project, :repository) } + context 'when repository is empty' do + let(:project) { create(:empty_project) } - before do - allow(@service).to receive(:project).and_return(project) - @testable = @service.can_test? - end + it 'test runs execute' do + expect(service).to receive(:execute).with(data) - describe '#can_test?' do - it { expect(@testable).to eq(true) } + service.test(data) + end end end end -- cgit v1.2.1 From bb7865276bb9adfe34755c3ea358e4951ec37d6f Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 27 Apr 2017 14:54:20 -0500 Subject: Use new renamed markup method --- app/views/projects/blob/viewers/_markup.html.haml | 2 +- spec/features/copy_as_gfm_spec.rb | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/app/views/projects/blob/viewers/_markup.html.haml b/app/views/projects/blob/viewers/_markup.html.haml index ae8f3e05687..b9a998d96ff 100644 --- a/app/views/projects/blob/viewers/_markup.html.haml +++ b/app/views/projects/blob/viewers/_markup.html.haml @@ -1,3 +1,3 @@ - blob = viewer.blob .file-content.wiki - = render_markup(blob.name, blob.data) + = markup(blob.name, blob.data) diff --git a/spec/features/copy_as_gfm_spec.rb b/spec/features/copy_as_gfm_spec.rb index 344e31e5ef5..f197fb44608 100644 --- a/spec/features/copy_as_gfm_spec.rb +++ b/spec/features/copy_as_gfm_spec.rb @@ -479,6 +479,7 @@ describe 'Copy as GFM', feature: true, js: true do context 'from a blob' do before do visit namespace_project_blob_path(project.namespace, project, File.join('master', 'files/ruby/popen.rb')) + wait_for_ajax end context 'selecting one word of text' do @@ -520,6 +521,7 @@ describe 'Copy as GFM', feature: true, js: true do context 'from a GFM code block' do before do visit namespace_project_blob_path(project.namespace, project, File.join('markdown', 'doc/api/users.md')) + wait_for_ajax end context 'selecting one word of text' do -- cgit v1.2.1 From 73ac7b2dd6ad31b0ab3191d123cc0b17c669cefb Mon Sep 17 00:00:00 2001 From: Dosuken shinya Date: Fri, 28 Apr 2017 09:38:32 +0000 Subject: Resolve "Add more tests for spec/controllers/projects/builds_controller_spec.rb" --- app/controllers/application_controller.rb | 4 + app/controllers/projects/builds_controller.rb | 15 +- ...trollers-projects-builds-controller-spec-rb.yml | 4 + config/routes/project.rb | 2 +- .../controllers/projects/builds_controller_spec.rb | 366 ++++++++++++++++++++- spec/factories/ci/builds.rb | 13 + .../security/project/internal_access_spec.rb | 38 +++ .../security/project/private_access_spec.rb | 32 ++ .../security/project/public_access_spec.rb | 38 +++ spec/support/matchers/access_matchers.rb | 4 +- 10 files changed, 496 insertions(+), 20 deletions(-) create mode 100644 changelogs/unreleased/29181-add-more-tests-for-spec-controllers-projects-builds-controller-spec-rb.yml diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index e77094fe2a8..e48f0963ef4 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -118,6 +118,10 @@ class ApplicationController < ActionController::Base end end + def respond_422 + head :unprocessable_entity + end + def no_cache_headers response.headers["Cache-Control"] = "no-cache, no-store, max-age=0, must-revalidate" response.headers["Pragma"] = "no-cache" diff --git a/app/controllers/projects/builds_controller.rb b/app/controllers/projects/builds_controller.rb index 04e8cdf6256..e24fc45d166 100644 --- a/app/controllers/projects/builds_controller.rb +++ b/app/controllers/projects/builds_controller.rb @@ -1,6 +1,6 @@ class Projects::BuildsController < Projects::ApplicationController before_action :build, except: [:index, :cancel_all] - before_action :authorize_read_build!, except: [:cancel, :cancel_all, :retry, :play] + before_action :authorize_read_build!, only: [:index, :show, :status, :raw, :trace] before_action :authorize_update_build!, except: [:index, :show, :status, :raw, :trace] layout 'project' @@ -60,20 +60,22 @@ class Projects::BuildsController < Projects::ApplicationController end def retry - return render_404 unless @build.retryable? + return respond_422 unless @build.retryable? build = Ci::Build.retry(@build, current_user) redirect_to build_path(build) end def play - return render_404 unless @build.playable? + return respond_422 unless @build.playable? build = @build.play(current_user) redirect_to build_path(build) end def cancel + return respond_422 unless @build.cancelable? + @build.cancel redirect_to build_path(@build) end @@ -85,9 +87,12 @@ class Projects::BuildsController < Projects::ApplicationController end def erase - @build.erase(erased_by: current_user) - redirect_to namespace_project_build_path(project.namespace, project, @build), + if @build.erase(erased_by: current_user) + redirect_to namespace_project_build_path(project.namespace, project, @build), notice: "Build has been successfully erased!" + else + respond_422 + end end def raw diff --git a/changelogs/unreleased/29181-add-more-tests-for-spec-controllers-projects-builds-controller-spec-rb.yml b/changelogs/unreleased/29181-add-more-tests-for-spec-controllers-projects-builds-controller-spec-rb.yml new file mode 100644 index 00000000000..7a3d687d73f --- /dev/null +++ b/changelogs/unreleased/29181-add-more-tests-for-spec-controllers-projects-builds-controller-spec-rb.yml @@ -0,0 +1,4 @@ +--- +title: Resolve "Add more tests for spec/controllers/projects/builds_controller_spec.rb" +merge_request: 10244 +author: dosuken123 diff --git a/config/routes/project.rb b/config/routes/project.rb index fa92202c1ea..115ae2324b3 100644 --- a/config/routes/project.rb +++ b/config/routes/project.rb @@ -173,7 +173,7 @@ constraints(ProjectUrlConstrainer.new) do post :retry post :play post :erase - get :trace + get :trace, defaults: { format: 'json' } get :raw end diff --git a/spec/controllers/projects/builds_controller_spec.rb b/spec/controllers/projects/builds_controller_spec.rb index fb4ccfa58c2..22193eac672 100644 --- a/spec/controllers/projects/builds_controller_spec.rb +++ b/spec/controllers/projects/builds_controller_spec.rb @@ -1,14 +1,69 @@ require 'spec_helper' describe Projects::BuildsController do - let(:user) { create(:user) } - let(:project) { create(:empty_project, :public) } + include ApiHelpers - before do - sign_in(user) - end + let(:project) { create(:empty_project, :public) } + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:user) { create(:user) } describe 'GET index' do + context 'when scope is pending' do + before do + create(:ci_build, :pending, pipeline: pipeline) + + get_index(scope: 'pending') + end + + it 'has only pending builds' do + expect(response).to have_http_status(:ok) + expect(assigns(:builds).first.status).to eq('pending') + end + end + + context 'when scope is running' do + before do + create(:ci_build, :running, pipeline: pipeline) + + get_index(scope: 'running') + end + + it 'has only running builds' do + expect(response).to have_http_status(:ok) + expect(assigns(:builds).first.status).to eq('running') + end + end + + context 'when scope is finished' do + before do + create(:ci_build, :success, pipeline: pipeline) + + get_index(scope: 'finished') + end + + it 'has only finished builds' do + expect(response).to have_http_status(:ok) + expect(assigns(:builds).first.status).to eq('success') + end + end + + context 'when page is specified' do + let(:last_page) { project.builds.page.total_pages } + + context 'when page number is eligible' do + before do + create_list(:ci_build, 2, pipeline: pipeline) + + get_index(page: last_page.to_param) + end + + it 'redirects to the page' do + expect(response).to have_http_status(:ok) + expect(assigns(:builds).current_page).to eq(last_page) + end + end + end + context 'number of queries' do before do Ci::Build::AVAILABLE_STATUSES.each do |status| @@ -23,13 +78,8 @@ describe Projects::BuildsController do RequestStore.clear! end - def render - get :index, namespace_id: project.namespace, - project_id: project - end - it "verifies number of queries" do - recorded = ActiveRecord::QueryRecorder.new { render } + recorded = ActiveRecord::QueryRecorder.new { get_index } expect(recorded.count).to be_within(5).of(8) end @@ -39,10 +89,83 @@ describe Projects::BuildsController do pipeline: pipeline, name: name, status: status) end end + + def get_index(**extra_params) + params = { + namespace_id: project.namespace.to_param, + project_id: project + } + + get :index, params.merge(extra_params) + end + end + + describe 'GET show' do + context 'when build exists' do + let!(:build) { create(:ci_build, pipeline: pipeline) } + + before do + get_show(id: build.id) + end + + it 'has a build' do + expect(response).to have_http_status(:ok) + expect(assigns(:build).id).to eq(build.id) + end + end + + context 'when build does not exist' do + before do + get_show(id: 1234) + end + + it 'renders not_found' do + expect(response).to have_http_status(:not_found) + end + end + + def get_show(**extra_params) + params = { + namespace_id: project.namespace.to_param, + project_id: project + } + + get :show, params.merge(extra_params) + end + end + + describe 'GET trace.json' do + before do + get_trace + end + + context 'when build has a trace' do + let(:build) { create(:ci_build, :trace, pipeline: pipeline) } + + it 'returns a trace' do + expect(response).to have_http_status(:ok) + expect(json_response['html']).to eq('BUILD TRACE') + end + end + + context 'when build has no traces' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'returns no traces' do + expect(response).to have_http_status(:ok) + expect(json_response['html']).to be_nil + end + end + + def get_trace + get :trace, namespace_id: project.namespace, + project_id: project, + id: build.id, + format: :json + end end describe 'GET status.json' do - let(:pipeline) { create(:ci_pipeline, project: project) } let(:build) { create(:ci_build, pipeline: pipeline) } let(:status) { build.detailed_status(double('user')) } @@ -71,6 +194,7 @@ describe Projects::BuildsController do before do project.add_developer(user) sign_in(user) + get_trace end @@ -84,6 +208,7 @@ describe Projects::BuildsController do context 'when user is logged in as non member' do before do sign_in(user) + get_trace end @@ -101,4 +226,221 @@ describe Projects::BuildsController do format: :json end end + + describe 'POST retry' do + before do + project.add_developer(user) + sign_in(user) + + post_retry + end + + context 'when build is retryable' do + let(:build) { create(:ci_build, :retryable, pipeline: pipeline) } + + it 'redirects to the retried build page' do + expect(response).to have_http_status(:found) + expect(response).to redirect_to(namespace_project_build_path(id: Ci::Build.last.id)) + end + end + + context 'when build is not retryable' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'renders unprocessable_entity' do + expect(response).to have_http_status(:unprocessable_entity) + end + end + + def post_retry + post :retry, namespace_id: project.namespace, + project_id: project, + id: build.id + end + end + + describe 'POST play' do + before do + project.add_developer(user) + sign_in(user) + + post_play + end + + context 'when build is playable' do + let(:build) { create(:ci_build, :playable, pipeline: pipeline) } + + it 'redirects to the played build page' do + expect(response).to have_http_status(:found) + expect(response).to redirect_to(namespace_project_build_path(id: build.id)) + end + + it 'transits to pending' do + expect(build.reload).to be_pending + end + end + + context 'when build is not playable' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'renders unprocessable_entity' do + expect(response).to have_http_status(:unprocessable_entity) + end + end + + def post_play + post :play, namespace_id: project.namespace, + project_id: project, + id: build.id + end + end + + describe 'POST cancel' do + before do + project.add_developer(user) + sign_in(user) + + post_cancel + end + + context 'when build is cancelable' do + let(:build) { create(:ci_build, :cancelable, pipeline: pipeline) } + + it 'redirects to the canceled build page' do + expect(response).to have_http_status(:found) + expect(response).to redirect_to(namespace_project_build_path(id: build.id)) + end + + it 'transits to canceled' do + expect(build.reload).to be_canceled + end + end + + context 'when build is not cancelable' do + let(:build) { create(:ci_build, :canceled, pipeline: pipeline) } + + it 'returns unprocessable_entity' do + expect(response).to have_http_status(:unprocessable_entity) + end + end + + def post_cancel + post :cancel, namespace_id: project.namespace, + project_id: project, + id: build.id + end + end + + describe 'POST cancel_all' do + before do + project.add_developer(user) + sign_in(user) + end + + context 'when builds are cancelable' do + before do + create_list(:ci_build, 2, :cancelable, pipeline: pipeline) + + post_cancel_all + end + + it 'redirects to a index page' do + expect(response).to have_http_status(:found) + expect(response).to redirect_to(namespace_project_builds_path) + end + + it 'transits to canceled' do + expect(Ci::Build.all).to all(be_canceled) + end + end + + context 'when builds are not cancelable' do + before do + create_list(:ci_build, 2, :canceled, pipeline: pipeline) + + post_cancel_all + end + + it 'redirects to a index page' do + expect(response).to have_http_status(:found) + expect(response).to redirect_to(namespace_project_builds_path) + end + end + + def post_cancel_all + post :cancel_all, namespace_id: project.namespace, + project_id: project + end + end + + describe 'POST erase' do + before do + project.add_developer(user) + sign_in(user) + + post_erase + end + + context 'when build is erasable' do + let(:build) { create(:ci_build, :erasable, :trace, pipeline: pipeline) } + + it 'redirects to the erased build page' do + expect(response).to have_http_status(:found) + expect(response).to redirect_to(namespace_project_build_path(id: build.id)) + end + + it 'erases artifacts' do + expect(build.artifacts_file.exists?).to be_falsey + expect(build.artifacts_metadata.exists?).to be_falsey + end + + it 'erases trace' do + expect(build.trace.exist?).to be_falsey + end + end + + context 'when build is not erasable' do + let(:build) { create(:ci_build, :erased, pipeline: pipeline) } + + it 'returns unprocessable_entity' do + expect(response).to have_http_status(:unprocessable_entity) + end + end + + def post_erase + post :erase, namespace_id: project.namespace, + project_id: project, + id: build.id + end + end + + describe 'GET raw' do + before do + get_raw + end + + context 'when build has a trace file' do + let(:build) { create(:ci_build, :trace, pipeline: pipeline) } + + it 'send a trace file' do + expect(response).to have_http_status(:ok) + expect(response.content_type).to eq 'text/plain; charset=utf-8' + expect(response.body).to eq 'BUILD TRACE' + end + end + + context 'when build does not have a trace file' do + let(:build) { create(:ci_build, pipeline: pipeline) } + + it 'returns not_found' do + expect(response).to have_http_status(:not_found) + end + end + + def get_raw + post :raw, namespace_id: project.namespace, + project_id: project, + id: build.id + end + end end diff --git a/spec/factories/ci/builds.rb b/spec/factories/ci/builds.rb index b62def83ee4..78ddd8d5584 100644 --- a/spec/factories/ci/builds.rb +++ b/spec/factories/ci/builds.rb @@ -79,6 +79,19 @@ FactoryGirl.define do manual end + trait :retryable do + success + end + + trait :cancelable do + pending + end + + trait :erasable do + success + artifacts + end + trait :tags do tag_list [:docker, :ruby] end diff --git a/spec/features/security/project/internal_access_spec.rb b/spec/features/security/project/internal_access_spec.rb index 6ecdc8cbb71..a1a36931824 100644 --- a/spec/features/security/project/internal_access_spec.rb +++ b/spec/features/security/project/internal_access_spec.rb @@ -399,6 +399,44 @@ describe "Internal Project Access", feature: true do end end + describe 'GET /:project_path/builds/:id/trace' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, pipeline: pipeline) } + subject { trace_namespace_project_build_path(project.namespace, project, build.id) } + + context 'when allowed for public and internal' do + before do + project.update(public_builds: true) + end + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(project) } + it { is_expected.to be_allowed_for(:master).of(project) } + it { is_expected.to be_allowed_for(:developer).of(project) } + it { is_expected.to be_allowed_for(:reporter).of(project) } + it { is_expected.to be_allowed_for(:guest).of(project) } + it { is_expected.to be_allowed_for(:user) } + it { is_expected.to be_denied_for(:external) } + it { is_expected.to be_denied_for(:visitor) } + end + + context 'when disallowed for public and internal' do + before do + project.update(public_builds: false) + end + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(project) } + it { is_expected.to be_allowed_for(:master).of(project) } + it { is_expected.to be_allowed_for(:developer).of(project) } + it { is_expected.to be_allowed_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:external) } + it { is_expected.to be_denied_for(:visitor) } + end + end + describe "GET /:project_path/environments" do subject { namespace_project_environments_path(project.namespace, project) } diff --git a/spec/features/security/project/private_access_spec.rb b/spec/features/security/project/private_access_spec.rb index a8fc0624588..5d58494a22a 100644 --- a/spec/features/security/project/private_access_spec.rb +++ b/spec/features/security/project/private_access_spec.rb @@ -388,6 +388,38 @@ describe "Private Project Access", feature: true do end end + describe 'GET /:project_path/builds/:id/trace' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, pipeline: pipeline) } + subject { trace_namespace_project_build_path(project.namespace, project, build.id) } + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(project) } + it { is_expected.to be_allowed_for(:master).of(project) } + it { is_expected.to be_allowed_for(:developer).of(project) } + it { is_expected.to be_allowed_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:external) } + it { is_expected.to be_denied_for(:visitor) } + + context 'when public builds is enabled' do + before do + project.update(public_builds: true) + end + + it { is_expected.to be_allowed_for(:guest).of(project) } + end + + context 'when public builds is disabled' do + before do + project.update(public_builds: false) + end + + it { is_expected.to be_denied_for(:guest).of(project) } + end + end + describe "GET /:project_path/environments" do subject { namespace_project_environments_path(project.namespace, project) } diff --git a/spec/features/security/project/public_access_spec.rb b/spec/features/security/project/public_access_spec.rb index c4d2f50ca14..5df5b710dc4 100644 --- a/spec/features/security/project/public_access_spec.rb +++ b/spec/features/security/project/public_access_spec.rb @@ -219,6 +219,44 @@ describe "Public Project Access", feature: true do end end + describe 'GET /:project_path/builds/:id/trace' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, pipeline: pipeline) } + subject { trace_namespace_project_build_path(project.namespace, project, build.id) } + + context 'when allowed for public' do + before do + project.update(public_builds: true) + end + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(project) } + it { is_expected.to be_allowed_for(:master).of(project) } + it { is_expected.to be_allowed_for(:developer).of(project) } + it { is_expected.to be_allowed_for(:reporter).of(project) } + it { is_expected.to be_allowed_for(:guest).of(project) } + it { is_expected.to be_allowed_for(:user) } + it { is_expected.to be_allowed_for(:external) } + it { is_expected.to be_allowed_for(:visitor) } + end + + context 'when disallowed for public' do + before do + project.update(public_builds: false) + end + + it { is_expected.to be_allowed_for(:admin) } + it { is_expected.to be_allowed_for(:owner).of(project) } + it { is_expected.to be_allowed_for(:master).of(project) } + it { is_expected.to be_allowed_for(:developer).of(project) } + it { is_expected.to be_allowed_for(:reporter).of(project) } + it { is_expected.to be_denied_for(:guest).of(project) } + it { is_expected.to be_denied_for(:user) } + it { is_expected.to be_denied_for(:external) } + it { is_expected.to be_denied_for(:visitor) } + end + end + describe "GET /:project_path/environments" do subject { namespace_project_environments_path(project.namespace, project) } diff --git a/spec/support/matchers/access_matchers.rb b/spec/support/matchers/access_matchers.rb index 7d238850520..3e4ca8b7ab0 100644 --- a/spec/support/matchers/access_matchers.rb +++ b/spec/support/matchers/access_matchers.rb @@ -51,7 +51,7 @@ module AccessMatchers emulate_user(user, @membership) visit(url) - status_code != 404 && current_path != new_user_session_path + status_code == 200 && current_path != new_user_session_path end chain :of do |membership| @@ -66,7 +66,7 @@ module AccessMatchers emulate_user(user, @membership) visit(url) - status_code == 404 || current_path == new_user_session_path + [401, 404].include?(status_code) || current_path == new_user_session_path end chain :of do |membership| -- cgit v1.2.1 From 7dd00a98cee7108239f6e15d9e79710dd8746580 Mon Sep 17 00:00:00 2001 From: Victor Wu Date: Fri, 28 Apr 2017 09:47:59 +0000 Subject: Improve Slack docs --- doc/integration/chat_commands.md | 18 +++---- doc/user/project/integrations/project_services.md | 4 +- doc/user/project/integrations/slack.md | 59 +++++++--------------- .../project/integrations/slack_slash_commands.md | 25 +++++---- 4 files changed, 40 insertions(+), 66 deletions(-) diff --git a/doc/integration/chat_commands.md b/doc/integration/chat_commands.md index 4b0084678d9..c878dc7e650 100644 --- a/doc/integration/chat_commands.md +++ b/doc/integration/chat_commands.md @@ -1,14 +1,14 @@ # Chat Commands -Chat commands allow user to perform common operations on GitLab right from there chat client. -Right now both Mattermost and Slack are supported. +Chat commands in Mattermost and Slack (also called Slack slash commands) allow you to control GitLab and view GitLab content right inside your chat client, without having to leave it. For Slack, this requires a [project service configuration](../user/project/integrations/slack_slash_commands.md). Simply type the command as a message in your chat client to activate it. -## Available commands +Commands are scoped to a project, with a trigger term that is specified during configuration. (We suggest you use the project name as the trigger term for simplicty and clarity.) Taking the trigger term as `project-name`, the commands are: -The trigger is configurable, but for the sake of this example, we'll use `/trigger` -* `/trigger help` - Displays all available commands for this user -* `/trigger issue new <shift+return> <description>` - creates a new issue on the project -* `/trigger issue show <id>` - Shows the issue with the given ID, if you've got access -* `/trigger issue search <query>` - Shows a maximum of 5 items matching the query -* `/trigger deploy <from> to <to>` - Deploy from an environment to another +| Command | Effect | +| ------- | ------ | +| `/project-name help` | Shows all available chat commands | +| `/project-name issue new <title> <shift+return> <description>` | Creates a new issue with title `<title>` and description `<description>` | +| `/project-name issue show <id>` | Shows the issue with id `<id>` | +| `/project-name issue search <query>` | Shows up to 5 issues matching `<query>` | +| `/project-name deploy <from> to <to>` | Deploy from the `<from>` environment to the `<to>` environment | \ No newline at end of file diff --git a/doc/user/project/integrations/project_services.md b/doc/user/project/integrations/project_services.md index 96c91093d7d..31baea507d7 100644 --- a/doc/user/project/integrations/project_services.md +++ b/doc/user/project/integrations/project_services.md @@ -49,8 +49,8 @@ Click on the service links to see further configuration instructions and details | [Mattermost Notifications](mattermost.md) | Receive event notifications in Mattermost | | [Microsoft teams](microsoft_teams.md) | Receive notifications for actions that happen on GitLab into a room on Microsoft Teams using Office 365 Connectors | | Pipelines emails | Email the pipeline status to a list of recipients | -| [Slack Notifications](slack.md) | Receive event notifications in Slack | -| [Slack slash commands](slack_slash_commands.md) | Slack chat and ChatOps slash commands | +| [Slack Notifications](slack.md) | Send GitLab events (e.g. issue created) to Slack as notifications | +| [Slack slash commands](slack_slash_commands.md) | Use slash commands in Slack to control GitLab | | PivotalTracker | Project Management Software (Source Commits Endpoint) | | [Prometheus](prometheus.md) | Monitor the performance of your deployed apps | | Pushover | Pushover makes it easy to get real-time notifications on your Android device, iPhone, iPad, and Desktop | diff --git a/doc/user/project/integrations/slack.md b/doc/user/project/integrations/slack.md index e8b238351ca..af4ca35a215 100644 --- a/doc/user/project/integrations/slack.md +++ b/doc/user/project/integrations/slack.md @@ -1,51 +1,26 @@ # Slack Notifications Service -## On Slack +The Slack Notifications Service allows your GitLab project to send events (e.g. issue created) to your existing Slack team as notifications. This requires configurations in both Slack and GitLab. -To enable Slack integration you must create an incoming webhook integration on -Slack: +> Note: You can also use Slack slash commands to control GitLab inside Slack. This is the separately configured [Slack slash commands](slack_slash_commands.md). -1. [Sign in to Slack](https://slack.com/signin) -1. Visit [Incoming WebHooks](https://my.slack.com/services/new/incoming-webhook/) -1. Choose the channel name you want to send notifications to. -1. Click **Add Incoming WebHooks Integration** -1. Copy the **Webhook URL**, we'll need this later for GitLab. +## Slack Configuration -## On GitLab +1. Sign in to your Slack team and [start a new Incoming WebHooks configuration](https://my.slack.com/services/new/incoming-webhook/). +1. Select the Slack channel where notifications will be sent to by default. Click the **Add Incoming WebHooks integration** button to add the configuration. +1. Copy the **Webhook URL**, which we'll use later in the GitLab configuration. -After you set up Slack, it's time to set up GitLab. +## GitLab Configuration -Navigate to the [Integrations page](project_services.md#accessing-the-project-services) -and select the **Slack notifications** service to configure it. -There, you will see a checkbox with the following events that can be triggered: +1. Navigate to the [Integrations page](project_services.md#accessing-the-project-services) in your project's settings, i.e. **Project > Settings > Integrations**. +1. Select the **Slack notifications** project service to configure it. +1. Check the **Active** checkbox to turn on the service. +1. Check the checkboxes corresponding to the GitLab events you want to send to Slack as a notification. +1. For each event, optionally enter the Slack channel where you want to send the event. (Do _not_ include the `#` symbol.) If left empty, the event will be sent to the default channel that you configured in the Slack Configuration step. +1. Paste the **Webhook URL** that you copied from the Slack Configuration step. +1. Optionally customize the Slack bot username that will be sending the notifications. +1. Configure the remaining options and click `Save changes`. -- Push -- Issue -- Confidential issue -- Merge request -- Note -- Tag push -- Pipeline -- Wiki page +Your Slack team will now start receiving GitLab event notifications as configured. -Below each of these event checkboxes, you have an input field to enter -which Slack channel you want to send that event message. Enter your preferred channel name **without** the hash sign (`#`). - -At the end, fill in your Slack details: - -| Field | Description | -| ----- | ----------- | -| **Webhook** | The [incoming webhook URL][slackhook] which you have to setup on Slack. | -| **Username** | Optional username which can be on messages sent to Slack. Fill this in if you want to change the username of the bot. | -| **Notify only broken pipelines** | If you choose to enable the **Pipeline** event and you want to be only notified about failed pipelines. | - -After you are all done, click **Save changes** for the changes to take effect. - ->**Note:** -You can set "branch,pushed,Compare changes" as highlight words on your Slack -profile settings, so that you can be aware of new commits when somebody pushes -them. - -![Slack configuration](img/slack_configuration.png) - -[slackhook]: https://my.slack.com/services/new/incoming-webhook +![Slack configuration](img/slack_configuration.png) \ No newline at end of file diff --git a/doc/user/project/integrations/slack_slash_commands.md b/doc/user/project/integrations/slack_slash_commands.md index 56f1ba7311e..54e0ee611cb 100644 --- a/doc/user/project/integrations/slack_slash_commands.md +++ b/doc/user/project/integrations/slack_slash_commands.md @@ -2,23 +2,22 @@ > Introduced in GitLab 8.15 -Slack commands give users an extra interface to perform common operations -from the chat environment. This allows one to, for example, create an issue as -soon as the idea was discussed in chat. -For all available commands try the help subcommand, for example: `/gitlab help`, -all review the [full list of commands](../../../integration/chat_commands.md). +Slack slash commands (also known as chat commmands) allow you to control GitLab and view content right inside Slack, without having to leave it. This requires configurations in both Slack and GitLab. -## Prerequisites - -A [team](https://get.slack.help/hc/en-us/articles/217608418-Creating-a-team) in -Slack should be created beforehand, GitLab cannot create it for you. +> Note: GitLab can also send events (e.g. issue created) to Slack as notifications. This is the separately configured [Slack Notifications Service](slack.md). ## Configuration -Go to your project's [Integrations page](project_services.md#accessing-the-project-services) -and select the **Slack slash commands** service to configure it. +1. Slack slash commands are scoped to a project. Navigate to the [Integrations page](project_services.md#accessing-the-project-services) in your project's settings, i.e. **Project > Settings > Integrations**. +1. Select the **Slack slash commands** project service to configure it. This page contains required information to complete the configuration in Slack. Leave this browser tab open. +1. Open a new browser tab and sign in to your Slack team. [Start a new Slash Commands integration](https://my.slack.com/services/new/slash-commands). +1. Enter a trigger term. We suggest you use the project name. Click **Add Slash Command Integration**. +1. Complete the rest of the fields in the Slack configuration page using information from the GitLab browser tab. In particular, the URL needs to be copied and pasted. Click **Save Integration** to complete the configuration in Slack. +1. While still on the Slack configuration page, copy the **token**. Go back to the GitLab browser tab and paste in the **token**. +1. Check the **Active** checkbox and click **Save changes** to complete the configuration in GitLab. ![Slack setup instructions](img/slack_setup.png) -Once you've followed the instructions, mark the service as active and insert the token -you've received from Slack. After saving the service you are good to go! +## Usage + +You can now use the [Slack slash commands](../../../integration/chat_commands.md). \ No newline at end of file -- cgit v1.2.1