From 52fe6522bdf673fc8de27ea241441a7130f14a97 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 15 Dec 2017 13:34:26 -0700 Subject: Remove extra height declarations in CSS; remove block styling from dropdowns --- app/assets/stylesheets/framework/contextual-sidebar.scss | 6 ------ app/assets/stylesheets/framework/dropdowns.scss | 1 - app/assets/stylesheets/framework/sidebar.scss | 6 ------ app/assets/stylesheets/pages/issuable.scss | 3 +-- 4 files changed, 1 insertion(+), 15 deletions(-) diff --git a/app/assets/stylesheets/framework/contextual-sidebar.scss b/app/assets/stylesheets/framework/contextual-sidebar.scss index 8baf7ca23a4..2e417315ed7 100644 --- a/app/assets/stylesheets/framework/contextual-sidebar.scss +++ b/app/assets/stylesheets/framework/contextual-sidebar.scss @@ -9,12 +9,6 @@ padding-left: $contextual-sidebar-width; } - // Override position: absolute - .right-sidebar { - position: fixed; - height: calc(100% - #{$header-height}); - } - .issues-bulk-update.right-sidebar.right-sidebar-expanded .issuable-sidebar-header { padding: 10px 0 15px; } diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 478269f3fcf..66dddfb35c1 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -214,7 +214,6 @@ .dropdown-menu, .dropdown-menu-nav { @include set-invisible; - display: block; position: absolute; width: auto; top: 100%; diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 0742c0a2a09..1145a49065f 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -89,12 +89,6 @@ .right-sidebar { border-left: 1px solid $border-color; - height: calc(100% - #{$header-height}); - - &.affix { - position: fixed; - top: $header-height; - } } .with-performance-bar .right-sidebar.affix { diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index e19196e0c41..48a787aa737 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -122,7 +122,7 @@ } .right-sidebar { - position: absolute; + position: fixed; top: $header-height; bottom: 0; right: 0; @@ -243,7 +243,6 @@ .issuable-sidebar { width: calc(100% + 100px); - height: 100%; overflow-y: scroll; overflow-x: hidden; -webkit-overflow-scrolling: touch; -- cgit v1.2.1 From 36b3dbf29ab5af30658c7e50028d4040498c82bd Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 15 Dec 2017 13:46:16 -0700 Subject: Remove all dropdown animations and set display: none if they're not open --- app/assets/stylesheets/framework/dropdowns.scss | 12 ++---------- app/assets/stylesheets/pages/issuable.scss | 4 ---- 2 files changed, 2 insertions(+), 14 deletions(-) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 66dddfb35c1..22303e6ec14 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -16,20 +16,12 @@ @mixin set-visible { transform: translateY(0); - visibility: visible; - opacity: 1; - transition-duration: 100ms, 150ms, 25ms; - transition-delay: 35ms, 50ms, 25ms; + display: block; } @mixin set-invisible { transform: translateY(-10px); - visibility: hidden; - opacity: 0; - transition-property: opacity, transform, visibility; - transition-duration: 70ms, 250ms, 250ms; - transition-timing-function: linear, $dropdown-animation-timing; - transition-delay: 25ms, 50ms, 0ms; + display: none; } .open { diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 48a787aa737..4bc7aeb127d 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -499,10 +499,6 @@ .with-performance-bar .right-sidebar { top: $header-height + $performance-bar-height; - - .issuable-sidebar { - height: calc(100% - #{$header-height} - #{$performance-bar-height}); - } } .sidebar-move-issue-confirmation-button { -- cgit v1.2.1 From c5e400405e545dddd68bf79e9a581b093159209d Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Fri, 15 Dec 2017 13:57:28 -0700 Subject: Fix sidebar height when performance bar enabled --- app/assets/stylesheets/framework/sidebar.scss | 1 + app/assets/stylesheets/pages/issuable.scss | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/app/assets/stylesheets/framework/sidebar.scss b/app/assets/stylesheets/framework/sidebar.scss index 1145a49065f..d61809cb0a4 100644 --- a/app/assets/stylesheets/framework/sidebar.scss +++ b/app/assets/stylesheets/framework/sidebar.scss @@ -89,6 +89,7 @@ .right-sidebar { border-left: 1px solid $border-color; + height: calc(100% - #{$header-height}); } .with-performance-bar .right-sidebar.affix { diff --git a/app/assets/stylesheets/pages/issuable.scss b/app/assets/stylesheets/pages/issuable.scss index 4bc7aeb127d..e1637618ab2 100644 --- a/app/assets/stylesheets/pages/issuable.scss +++ b/app/assets/stylesheets/pages/issuable.scss @@ -243,6 +243,7 @@ .issuable-sidebar { width: calc(100% + 100px); + height: 100%; overflow-y: scroll; overflow-x: hidden; -webkit-overflow-scrolling: touch; @@ -499,6 +500,10 @@ .with-performance-bar .right-sidebar { top: $header-height + $performance-bar-height; + + .issuable-sidebar { + height: calc(100% - #{$performance-bar-height}); + } } .sidebar-move-issue-confirmation-button { -- cgit v1.2.1 From 325fdc9f1f9d4e4c9375e024a3d146782fcc9623 Mon Sep 17 00:00:00 2001 From: Annabel Dunstone Gray Date: Mon, 18 Dec 2017 11:59:21 -0700 Subject: Remove block styling from search dropdown --- app/assets/stylesheets/pages/search.scss | 7 ------- 1 file changed, 7 deletions(-) diff --git a/app/assets/stylesheets/pages/search.scss b/app/assets/stylesheets/pages/search.scss index 49c8e546bf2..2381d7eb92b 100644 --- a/app/assets/stylesheets/pages/search.scss +++ b/app/assets/stylesheets/pages/search.scss @@ -108,13 +108,6 @@ input[type="checkbox"]:hover { // Custom dropdown positioning .dropdown-menu { - transition-property: opacity, transform; - transition-duration: 250ms, 250ms; - transition-delay: 0ms, 25ms; - transition-timing-function: $dropdown-animation-timing; - transform: translateY(0); - opacity: 0; - display: block; left: -5px; } -- cgit v1.2.1 From b03789395c59ce94c8bdb4e0f4806c8cd7705f46 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Mon, 18 Dec 2017 22:55:51 -0600 Subject: Do not generate links for private NPM modules in blob view --- app/models/blob_viewer/dependency_manager.rb | 13 ++++-- app/models/blob_viewer/package_json.rb | 12 ++++++ .../blob/viewers/_dependency_manager.html.haml | 2 +- .../unreleased/36020-private-npm-modules.yml | 5 +++ spec/models/blob_viewer/package_json_spec.rb | 46 ++++++++++++++++++++++ 5 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/36020-private-npm-modules.yml diff --git a/app/models/blob_viewer/dependency_manager.rb b/app/models/blob_viewer/dependency_manager.rb index a8d9be945dc..cc4950240af 100644 --- a/app/models/blob_viewer/dependency_manager.rb +++ b/app/models/blob_viewer/dependency_manager.rb @@ -27,10 +27,17 @@ module BlobViewer private - def package_name_from_json(key) - prepare! + def json_data + @json_data ||= begin + prepare! + JSON.parse(blob.data) + rescue + {} + end + end - JSON.parse(blob.data)[key] rescue nil + def package_name_from_json(key) + json_data[key] end def package_name_from_method_call(name) diff --git a/app/models/blob_viewer/package_json.rb b/app/models/blob_viewer/package_json.rb index 09221efb56c..6ce61e30d3d 100644 --- a/app/models/blob_viewer/package_json.rb +++ b/app/models/blob_viewer/package_json.rb @@ -16,8 +16,20 @@ module BlobViewer @package_name ||= package_name_from_json('name') end + def package_type + private? ? 'private package' : super + end + def package_url + return nil if private? + "https://www.npmjs.com/package/#{package_name}" end + + private + + def private? + !!json_data['private'] + end end end diff --git a/app/views/projects/blob/viewers/_dependency_manager.html.haml b/app/views/projects/blob/viewers/_dependency_manager.html.haml index a0f0215a5ff..87aa7c1dbf8 100644 --- a/app/views/projects/blob/viewers/_dependency_manager.html.haml +++ b/app/views/projects/blob/viewers/_dependency_manager.html.haml @@ -6,6 +6,6 @@ - if viewer.package_name and defines a #{viewer.package_type} named %strong< - = link_to viewer.package_name, viewer.package_url, target: '_blank', rel: 'noopener noreferrer' + = link_to_if viewer.package_url.present?, viewer.package_name, viewer.package_url, target: '_blank', rel: 'noopener noreferrer' = link_to 'Learn more', viewer.manager_url, target: '_blank', rel: 'noopener noreferrer' diff --git a/changelogs/unreleased/36020-private-npm-modules.yml b/changelogs/unreleased/36020-private-npm-modules.yml new file mode 100644 index 00000000000..a0122e2b360 --- /dev/null +++ b/changelogs/unreleased/36020-private-npm-modules.yml @@ -0,0 +1,5 @@ +--- +title: Do not generate links for private NPM modules in blob view +merge_request: 16002 +author: Mario de la Ossa +type: added diff --git a/spec/models/blob_viewer/package_json_spec.rb b/spec/models/blob_viewer/package_json_spec.rb index 0f8330e91c1..339d4e9e644 100644 --- a/spec/models/blob_viewer/package_json_spec.rb +++ b/spec/models/blob_viewer/package_json_spec.rb @@ -22,4 +22,50 @@ describe BlobViewer::PackageJson do expect(subject.package_name).to eq('module-name') end end + + describe '#package_url' do + it 'returns the package URL' do + expect(subject).to receive(:prepare!) + + expect(subject.package_url).to eq("https://www.npmjs.com/package/#{subject.package_name}") + end + end + + describe '#package_type' do + it 'returns "package"' do + expect(subject).to receive(:prepare!) + + expect(subject.package_type).to eq('package') + end + end + + context 'when package.json has "private": true' do + let(:data) do + <<-SPEC.strip_heredoc + { + "name": "module-name", + "version": "10.3.1", + "private": true + } + SPEC + end + let(:blob) { fake_blob(path: 'package.json', data: data) } + subject { described_class.new(blob) } + + describe '#package_url' do + it 'returns nil' do + expect(subject).to receive(:prepare!) + + expect(subject.package_url).to be_nil + end + end + + describe '#package_type' do + it 'returns "private package"' do + expect(subject).to receive(:prepare!) + + expect(subject.package_type).to eq('private package') + end + end + end end -- cgit v1.2.1 From d02059ddf3fef105a8e835024b589d9eac2140f0 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Mon, 18 Dec 2017 23:14:08 -0600 Subject: BlobViewer::PackageJson - if private link to homepage --- app/models/blob_viewer/package_json.rb | 12 +++++++++--- changelogs/unreleased/36020-private-npm-modules.yml | 2 +- spec/models/blob_viewer/package_json_spec.rb | 7 ++++--- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/models/blob_viewer/package_json.rb b/app/models/blob_viewer/package_json.rb index 6ce61e30d3d..46cd2f04f4d 100644 --- a/app/models/blob_viewer/package_json.rb +++ b/app/models/blob_viewer/package_json.rb @@ -21,9 +21,7 @@ module BlobViewer end def package_url - return nil if private? - - "https://www.npmjs.com/package/#{package_name}" + private? ? homepage : npm_url end private @@ -31,5 +29,13 @@ module BlobViewer def private? !!json_data['private'] end + + def homepage + json_data['homepage'] + end + + def npm_url + "https://www.npmjs.com/package/#{package_name}" + end end end diff --git a/changelogs/unreleased/36020-private-npm-modules.yml b/changelogs/unreleased/36020-private-npm-modules.yml index a0122e2b360..5c2585a602e 100644 --- a/changelogs/unreleased/36020-private-npm-modules.yml +++ b/changelogs/unreleased/36020-private-npm-modules.yml @@ -1,5 +1,5 @@ --- -title: Do not generate links for private NPM modules in blob view +title: Do not generate NPM links for private NPM modules in blob view merge_request: 16002 author: Mario de la Ossa type: added diff --git a/spec/models/blob_viewer/package_json_spec.rb b/spec/models/blob_viewer/package_json_spec.rb index 339d4e9e644..5ed2f4400bc 100644 --- a/spec/models/blob_viewer/package_json_spec.rb +++ b/spec/models/blob_viewer/package_json_spec.rb @@ -45,7 +45,8 @@ describe BlobViewer::PackageJson do { "name": "module-name", "version": "10.3.1", - "private": true + "private": true, + "homepage": "myawesomepackage.com" } SPEC end @@ -53,10 +54,10 @@ describe BlobViewer::PackageJson do subject { described_class.new(blob) } describe '#package_url' do - it 'returns nil' do + it 'returns homepage if any' do expect(subject).to receive(:prepare!) - expect(subject.package_url).to be_nil + expect(subject.package_url).to eq('myawesomepackage.com') end end -- cgit v1.2.1 From c6edae38870a4228e3b964d647b9ef588df11f27 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 5 Dec 2017 14:15:30 +0100 Subject: Load commit in batches for pipelines#index Uses `list_commits_by_oid` on the CommitService, to request the needed commits for pipelines. These commits are needed to display the user that created the commit and the commit title. This includes fixes for tests failing that depended on the commit being `nil`. However, now these are batch loaded, this doesn't happen anymore and the commits are an instance of BatchLoader. --- Gemfile | 2 +- Gemfile.lock | 4 +- app/controllers/projects/pipelines_controller.rb | 2 + app/models/ci/pipeline.rb | 13 +++--- app/models/commit.rb | 20 +++++++-- app/models/repository.rb | 12 ++++++ app/views/projects/pipelines/_info.html.haml | 50 +++++++++++----------- app/workers/expire_pipeline_cache_worker.rb | 2 +- lib/gitlab/git/commit.rb | 13 ++++++ lib/gitlab/gitaly_client/commit_service.rb | 9 ++++ .../projects/pipelines_controller_spec.rb | 13 +++--- spec/models/commit_spec.rb | 39 +++++++++++++++++ spec/models/repository_spec.rb | 48 +++++++++++++++++++++ spec/serializers/pipeline_serializer_spec.rb | 6 +-- 14 files changed, 184 insertions(+), 49 deletions(-) diff --git a/Gemfile b/Gemfile index 6b1c6e16851..b6ffaf80f24 100644 --- a/Gemfile +++ b/Gemfile @@ -263,7 +263,7 @@ gem 'gettext_i18n_rails', '~> 1.8.0' gem 'gettext_i18n_rails_js', '~> 1.2.0' gem 'gettext', '~> 3.2.2', require: false, group: :development -gem 'batch-loader' +gem 'batch-loader', '~> 1.2.1' # Perf bar gem 'peek', '~> 1.0.1' diff --git a/Gemfile.lock b/Gemfile.lock index 11040fab805..a6e3c9e27cc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -78,7 +78,7 @@ GEM thread_safe (~> 0.3, >= 0.3.1) babosa (1.0.2) base32 (0.3.2) - batch-loader (1.1.1) + batch-loader (1.2.1) bcrypt (3.1.11) bcrypt_pbkdf (1.0.0) benchmark-ips (2.3.0) @@ -988,7 +988,7 @@ DEPENDENCIES awesome_print (~> 1.2.0) babosa (~> 1.0.2) base32 (~> 0.3.0) - batch-loader + batch-loader (~> 1.2.1) bcrypt_pbkdf (~> 1.0) benchmark-ips (~> 2.3.0) better_errors (~> 2.1.0) diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb index 7ad7b3003af..e146d0d3cd5 100644 --- a/app/controllers/projects/pipelines_controller.rb +++ b/app/controllers/projects/pipelines_controller.rb @@ -29,6 +29,8 @@ class Projects::PipelinesController < Projects::ApplicationController @pipelines_count = PipelinesFinder .new(project).execute.count + @pipelines.map(&:commit) # List commits for batch loading + respond_to do |format| format.html format.json do diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 28f154581a9..d4690da3be6 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -287,8 +287,12 @@ module Ci Ci::Pipeline.truncate_sha(sha) end + # NOTE: This is loaded lazily and will never be nil, even if the commit + # cannot be found. + # + # Use constructs like: `pipeline.commit.present?` def commit - @commit ||= project.commit_by(oid: sha) + @commit ||= Commit.lazy(project, sha) end def branch? @@ -338,12 +342,9 @@ module Ci end def latest? - return false unless ref - - commit = project.commit(ref) - return false unless commit + return false unless ref && commit.present? - commit.sha == sha + project.commit(ref) == commit end def retried diff --git a/app/models/commit.rb b/app/models/commit.rb index 13c31111134..2be07ca7d3c 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -86,6 +86,20 @@ class Commit def valid_hash?(key) !!(/\A#{COMMIT_SHA_PATTERN}\z/ =~ key) end + + def lazy(project, oid) + BatchLoader.for({ project: project, oid: oid }).batch do |items, loader| + items_by_project = items.group_by { |i| i[:project] } + + items_by_project.each do |project, commit_ids| + oids = commit_ids.map { |i| i[:oid] } + + project.repository.commits_by(oids: oids).each do |commit| + loader.call({ project: commit.project, oid: commit.id }, commit) if commit + end + end + end + end end attr_accessor :raw @@ -103,7 +117,7 @@ class Commit end def ==(other) - (self.class === other) && (raw == other.raw) + other.is_a?(self.class) && raw == other.raw end def self.reference_prefix @@ -224,8 +238,8 @@ class Commit notes.includes(:author) end - def method_missing(m, *args, &block) - @raw.__send__(m, *args, &block) # rubocop:disable GitlabSecurity/PublicSend + def method_missing(method, *args, &block) + @raw.__send__(method, *args, &block) # rubocop:disable GitlabSecurity/PublicSend end def respond_to_missing?(method, include_private = false) diff --git a/app/models/repository.rb b/app/models/repository.rb index 552a354d1ce..fc7081a9f34 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -118,6 +118,18 @@ class Repository @commit_cache[oid] = find_commit(oid) end + def commits_by(oids:) + return [] unless oids.present? + + commits = Gitlab::Git::Commit.batch_by_oid(raw_repository, oids) + + if commits.present? + Commit.decorate(commits, @project) + else + [] + end + end + def commits(ref, path: nil, limit: nil, offset: nil, skip_merges: false, after: nil, before: nil) options = { repo: raw_repository, diff --git a/app/views/projects/pipelines/_info.html.haml b/app/views/projects/pipelines/_info.html.haml index 01ea9356af5..85946aec1f2 100644 --- a/app/views/projects/pipelines/_info.html.haml +++ b/app/views/projects/pipelines/_info.html.haml @@ -1,6 +1,6 @@ #js-pipeline-header-vue.pipeline-header-container -- if @commit +- if @commit.present? .commit-box %h3.commit-title = markdown(@commit.title, pipeline: :single_line) @@ -8,28 +8,28 @@ %pre.commit-description = preserve(markdown(@commit.description, pipeline: :single_line)) -.info-well - - if @commit.status - .well-segment.pipeline-info - .icon-container - = icon('clock-o') - = pluralize @pipeline.total_size, "job" - - if @pipeline.ref - from - = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" - - if @pipeline.duration - in - = time_interval_in_words(@pipeline.duration) - - if @pipeline.queued_duration - = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})" + .info-well + - if @commit.status + .well-segment.pipeline-info + .icon-container + = icon('clock-o') + = pluralize @pipeline.total_size, "job" + - if @pipeline.ref + from + = link_to @pipeline.ref, project_ref_path(@project, @pipeline.ref), class: "ref-name" + - if @pipeline.duration + in + = time_interval_in_words(@pipeline.duration) + - if @pipeline.queued_duration + = "(queued for #{time_interval_in_words(@pipeline.queued_duration)})" - .well-segment.branch-info - .icon-container.commit-icon - = custom_icon("icon_commit") - = link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short" - = link_to("#", class: "js-details-expand hidden-xs hidden-sm") do - %span.text-expander - \... - %span.js-details-content.hide - = link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full" - = clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard") + .well-segment.branch-info + .icon-container.commit-icon + = custom_icon("icon_commit") + = link_to @commit.short_id, project_commit_path(@project, @pipeline.sha), class: "commit-sha js-details-short" + = link_to("#", class: "js-details-expand hidden-xs hidden-sm") do + %span.text-expander + \... + %span.js-details-content.hide + = link_to @pipeline.sha, project_commit_path(@project, @pipeline.sha), class: "commit-sha commit-hash-full" + = clipboard_button(text: @pipeline.sha, title: "Copy commit SHA to clipboard") diff --git a/app/workers/expire_pipeline_cache_worker.rb b/app/workers/expire_pipeline_cache_worker.rb index 3e34de22c19..db73d37868a 100644 --- a/app/workers/expire_pipeline_cache_worker.rb +++ b/app/workers/expire_pipeline_cache_worker.rb @@ -13,7 +13,7 @@ class ExpirePipelineCacheWorker store.touch(project_pipelines_path(project)) store.touch(project_pipeline_path(project, pipeline)) - store.touch(commit_pipelines_path(project, pipeline.commit)) if pipeline.commit + store.touch(commit_pipelines_path(project, pipeline.commit)) unless pipeline.commit.nil? store.touch(new_merge_request_pipelines_path(project)) each_pipelines_merge_request_path(project, pipeline) do |path| store.touch(path) diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb index e90b158fb34..145721dea76 100644 --- a/lib/gitlab/git/commit.rb +++ b/lib/gitlab/git/commit.rb @@ -228,6 +228,19 @@ module Gitlab end end end + + # Only to be used when the object ids will not necessarily have a + # relation to each other. The last 10 commits for a branch for example, + # should go through .where + def batch_by_oid(repo, oids) + repo.gitaly_migrate(:list_commits_by_oid) do |is_enabled| + if is_enabled + repo.gitaly_commit_client.list_commits_by_oid(oids) + else + oids.map { |oid| find(repo, oid) }.compact + end + end + end end def initialize(repository, raw_commit, head = nil) diff --git a/lib/gitlab/gitaly_client/commit_service.rb b/lib/gitlab/gitaly_client/commit_service.rb index 7985f5b5457..fb3e27770b4 100644 --- a/lib/gitlab/gitaly_client/commit_service.rb +++ b/lib/gitlab/gitaly_client/commit_service.rb @@ -169,6 +169,15 @@ module Gitlab consume_commits_response(response) end + def list_commits_by_oid(oids) + request = Gitaly::ListCommitsByOidRequest.new(repository: @gitaly_repo, oid: oids) + + response = GitalyClient.call(@repository.storage, :commit_service, :list_commits_by_oid, request, timeout: GitalyClient.medium_timeout) + consume_commits_response(response) + rescue GRPC::Unknown # If no repository is found, happens mainly during testing + [] + end + def commits_by_message(query, revision: '', path: '', limit: 1000, offset: 0) request = Gitaly::CommitsByMessageRequest.new( repository: @gitaly_repo, diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb index 1604a2da485..35ac999cc65 100644 --- a/spec/controllers/projects/pipelines_controller_spec.rb +++ b/spec/controllers/projects/pipelines_controller_spec.rb @@ -17,13 +17,10 @@ describe Projects::PipelinesController do describe 'GET index.json' do before do - branch_head = project.commit - parent = branch_head.parent - - create(:ci_empty_pipeline, status: 'pending', project: project, sha: branch_head.id) - create(:ci_empty_pipeline, status: 'running', project: project, sha: branch_head.id) - create(:ci_empty_pipeline, status: 'created', project: project, sha: parent.id) - create(:ci_empty_pipeline, status: 'success', project: project, sha: parent.id) + %w(pending running created success).each_with_index do |status, index| + sha = project.commit("HEAD~#{index}") + create(:ci_empty_pipeline, status: status, project: project, sha: sha) + end end subject do @@ -46,7 +43,7 @@ describe Projects::PipelinesController do context 'when performing gitaly calls', :request_store do it 'limits the Gitaly requests' do - expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(8) + expect { subject }.to change { Gitlab::GitalyClient.get_request_count }.by(3) end end end diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb index d18a5c9dfa6..cd955a5eb69 100644 --- a/spec/models/commit_spec.rb +++ b/spec/models/commit_spec.rb @@ -13,6 +13,45 @@ describe Commit do it { is_expected.to include_module(StaticModel) } end + describe '.lazy' do + set(:project) { create(:project, :repository) } + + context 'when the commits are found' do + let(:oids) do + %w( + 498214de67004b1da3d820901307bed2a68a8ef6 + c642fe9b8b9f28f9225d7ea953fe14e74748d53b + 6f6d7e7ed97bb5f0054f2b1df789b39ca89b6ff9 + 048721d90c449b244b7b4c53a9186b04330174ec + 281d3a76f31c812dbf48abce82ccf6860adedd81 + ) + end + + subject { oids.map { |oid| described_class.lazy(project, oid) } } + + it 'batches requests for commits' do + expect(project.repository).to receive(:commits_by).once.and_call_original + + subject.first.title + subject.last.title + end + + it 'maintains ordering' do + subject.each_with_index do |commit, i| + expect(commit.id).to eq(oids[i]) + end + end + end + + context 'when not found' do + it 'returns nil as commit' do + commit = described_class.lazy(project, 'deadbeef').__sync + + expect(commit).to be_nil + end + end + end + describe '#author' do it 'looks up the author in a case-insensitive way' do user = create(:user, email: commit.author_email.upcase) diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index 799d99c0369..8a3531c1898 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -239,6 +239,54 @@ describe Repository do end end + describe '#commits_by' do + set(:project) { create(:project, :repository) } + + shared_examples 'batch commits fetching' do + let(:oids) { TestEnv::BRANCH_SHA.values } + + subject { project.repository.commits_by(oids: oids) } + + it 'finds each commit' do + expect(subject).not_to include(nil) + expect(subject.size).to eq(oids.size) + end + + it 'returns only Commit instances' do + expect(subject).to all( be_a(Commit) ) + end + + context 'when some commits are not found ' do + let(:oids) do + ['deadbeef'] + TestEnv::BRANCH_SHA.values.first(10) + end + + it 'returns only found commits' do + expect(subject).not_to include(nil) + expect(subject.size).to eq(10) + end + end + + context 'when no oids are passed' do + let(:oids) { [] } + + it 'does not call #batch_by_oid' do + expect(Gitlab::Git::Commit).not_to receive(:batch_by_oid) + + subject + end + end + end + + context 'when Gitaly list_commits_by_oid is enabled' do + it_behaves_like 'batch commits fetching' + end + + context 'when Gitaly list_commits_by_oid is enabled', :disable_gitaly do + it_behaves_like 'batch commits fetching' + end + end + describe '#find_commits_by_message' do shared_examples 'finding commits by message' do it 'returns commits with messages containing a given string' do diff --git a/spec/serializers/pipeline_serializer_spec.rb b/spec/serializers/pipeline_serializer_spec.rb index 88d347322a6..c38795ad1a1 100644 --- a/spec/serializers/pipeline_serializer_spec.rb +++ b/spec/serializers/pipeline_serializer_spec.rb @@ -1,6 +1,7 @@ require 'spec_helper' describe PipelineSerializer do + set(:project) { create(:project, :repository) } set(:user) { create(:user) } let(:serializer) do @@ -16,7 +17,7 @@ describe PipelineSerializer do end context 'when a single object is being serialized' do - let(:resource) { create(:ci_empty_pipeline) } + let(:resource) { create(:ci_empty_pipeline, project: project) } it 'serializers the pipeline object' do expect(subject[:id]).to eq resource.id @@ -24,7 +25,7 @@ describe PipelineSerializer do end context 'when multiple objects are being serialized' do - let(:resource) { create_list(:ci_pipeline, 2) } + let(:resource) { create_list(:ci_pipeline, 2, project: project) } it 'serializers the array of pipelines' do expect(subject).not_to be_empty @@ -100,7 +101,6 @@ describe PipelineSerializer do context 'number of queries' do let(:resource) { Ci::Pipeline.all } - let(:project) { create(:project) } before do # Since RequestStore.active? is true we have to allow the -- cgit v1.2.1 From 873bc3a6854689549cf426f726499619205a2798 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Mon, 18 Dec 2017 21:19:57 +0800 Subject: Preserve gem path so that we use the same gems Without this patch, I would end up with: ``` An error occurred in a `before(:suite)` hook. Failure/Error: raise "could not connect to gitaly at #{socket.inspect} after #{sleep_time} seconds" RuntimeError: could not connect to gitaly at "tmp/tests/gitaly/gitaly.socket" after 10 seconds ``` Digging into it, it's because `scripts/gitaly-test-spawn` could not spawn the process, because it cannot find the installed gems. I personally installed all my gems under $HOME, namely with: * `gem install rake --user-install` or: * `bundle install --path ~/.gem` The gems would be installed to `~/.gem/ruby/2.4.0/gems`, where the version is Ruby ABI version. Now we're changing $HOME, making RubyGems think that the gems would be installed to `tmp/tests/ruby/2.4.0/gems` which is apparently not the case. In order to workaround this, we could preserve $GEM_PATH populated by RubyGems, ignoring the default path based on $HOME. --- scripts/gitaly-test-spawn | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/scripts/gitaly-test-spawn b/scripts/gitaly-test-spawn index 8e05eca8d7e..ecb68c6acc6 100755 --- a/scripts/gitaly-test-spawn +++ b/scripts/gitaly-test-spawn @@ -1,7 +1,8 @@ #!/usr/bin/env ruby gitaly_dir = 'tmp/tests/gitaly' -env = { 'HOME' => File.expand_path('tmp/tests') } +env = { 'HOME' => File.expand_path('tmp/tests'), + 'GEM_PATH' => Gem.path.join(':') } args = %W[#{gitaly_dir}/gitaly #{gitaly_dir}/config.toml] # Print the PID of the spawned process -- cgit v1.2.1 From f3f9dedcf70c1bf247945253f3cd9026ab87d421 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 19 Dec 2017 10:46:13 +0000 Subject: Remove transitionend event from GL dropdown --- app/assets/javascripts/gl_dropdown.js | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/gl_dropdown.js b/app/assets/javascripts/gl_dropdown.js index cf4a70e321e..64f258aed64 100644 --- a/app/assets/javascripts/gl_dropdown.js +++ b/app/assets/javascripts/gl_dropdown.js @@ -300,7 +300,7 @@ GitLabDropdown = (function() { return function(data) { _this.fullData = data; _this.parseData(_this.fullData); - _this.focusTextInput(true); + _this.focusTextInput(); if (_this.options.filterable && _this.filter && _this.filter.input && _this.filter.input.val() && _this.filter.input.val().trim() !== '') { return _this.filter.input.trigger('input'); } @@ -790,24 +790,16 @@ GitLabDropdown = (function() { return [selectedObject, isMarking]; }; - GitLabDropdown.prototype.focusTextInput = function(triggerFocus = false) { + GitLabDropdown.prototype.focusTextInput = function() { if (this.options.filterable) { - this.dropdown.one('transitionend', () => { - const initialScrollTop = $(window).scrollTop(); + const initialScrollTop = $(window).scrollTop(); - if (this.dropdown.is('.open')) { - this.filterInput.focus(); - } - - if ($(window).scrollTop() < initialScrollTop) { - $(window).scrollTop(initialScrollTop); - } - }); + if (this.dropdown.is('.open')) { + this.filterInput.focus(); + } - if (triggerFocus) { - // This triggers after a ajax request - // in case of slow requests, the dropdown transition could already be finished - this.dropdown.trigger('transitionend'); + if ($(window).scrollTop() < initialScrollTop) { + $(window).scrollTop(initialScrollTop); } } }; -- cgit v1.2.1 From 08949a189863172a8c9d69680a9c5c2f79ff9202 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 19 Dec 2017 11:13:02 +0000 Subject: Update axios.md --- doc/development/fe_guide/axios.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/development/fe_guide/axios.md b/doc/development/fe_guide/axios.md index 962fe3dcec9..1daa6758171 100644 --- a/doc/development/fe_guide/axios.md +++ b/doc/development/fe_guide/axios.md @@ -11,7 +11,7 @@ This exported module should be used instead of directly using `axios` to ensure ## Usage ```javascript - import axios from '~/lib/utils/axios_utils'; + import axios from './lib/utils/axios_utils'; axios.get(url) .then((response) => { -- cgit v1.2.1 From 73f615a5c6be79a6d464f25575d8ba3f1c98a0a2 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 19 Dec 2017 19:43:50 +0800 Subject: Make sure two except won't overwrite each other This is a pretty boring solution, but I can't think of a good idea right now and this might be good enough for now... --- .gitlab-ci.yml | 44 ++++++++++++++++++-------------------------- 1 file changed, 18 insertions(+), 26 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 2493250f8fb..c26e7f0aeba 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -80,10 +80,14 @@ stages: except: - /(^qa[\/-].*|.*-qa$)/ +.except-docs-and-qa: &except-docs-and-qa + except: + - /(^docs[\/-].*|.*-docs$)/ + - /(^qa[\/-].*|.*-qa$)/ + .rspec-metadata: &rspec-metadata <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test script: @@ -121,8 +125,7 @@ stages: .spinach-metadata: &spinach-metadata <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test script: @@ -222,8 +225,7 @@ review-docs-cleanup: # Retrieve knapsack and rspec_flaky reports retrieve-tests-metadata: <<: *tests-metadata-state - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa stage: prepare cache: key: tests_metadata @@ -378,8 +380,7 @@ spinach-mysql 3 4: *spinach-metadata-mysql .rake-exec: &rake-exec <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache <<: *ruby-static-analysis stage: test @@ -443,8 +444,7 @@ ee_compat_check: # DB migration, rollback, and seed jobs .db-migrate-reset: &db-migrate-reset <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test script: @@ -466,8 +466,7 @@ db:check-schema-pg: .migration-paths: &migration-paths <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test variables: @@ -494,8 +493,7 @@ migration:path-mysql: .db-rollback: &db-rollback <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test script: @@ -512,8 +510,7 @@ db:rollback-mysql: .db-seed_fu: &db-seed_fu <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test variables: @@ -541,8 +538,7 @@ db:seed_fu-mysql: # Frontend-related jobs gitlab:assets:compile: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: test dependencies: [] @@ -564,8 +560,7 @@ gitlab:assets:compile: karma: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache <<: *use-pg stage: test @@ -619,8 +614,7 @@ qa:internal: coverage: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: post-test services: [] @@ -639,8 +633,7 @@ coverage: lint:javascript:report: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache stage: post-test dependencies: @@ -699,8 +692,7 @@ cache gems: gitlab_git_test: <<: *dedicated-runner - <<: *except-docs - <<: *except-qa + <<: *except-docs-and-qa <<: *pull-cache variables: SETUP_DB: "false" -- cgit v1.2.1 From d86cd7c3d9a4613540e2fd81ea3f31795be7917c Mon Sep 17 00:00:00 2001 From: Richard Clamp Date: Tue, 19 Dec 2017 14:53:27 +0000 Subject: Tidy up the documentation of Gitlab HA/Gitlab Application --- doc/administration/high_availability/gitlab.md | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/doc/administration/high_availability/gitlab.md b/doc/administration/high_availability/gitlab.md index 42666357faf..b85a166089d 100644 --- a/doc/administration/high_availability/gitlab.md +++ b/doc/administration/high_availability/gitlab.md @@ -1,6 +1,6 @@ # Configuring GitLab for HA -Assuming you have already configured a database, Redis, and NFS, you can +Assuming you have already configured a [database](database.md), [Redis](redis.md), and [NFS](nfs.md), you can configure the GitLab application server(s) now. Complete the steps below for each GitLab application server in your environment. @@ -48,34 +48,33 @@ for each GitLab application server in your environment. data locations. See [NFS documentation](nfs.md) for `/etc/gitlab/gitlab.rb` configuration values for various scenarios. The example below assumes you've added NFS mounts in the default data locations. - + ```ruby external_url 'https://gitlab.example.com' # Prevent GitLab from starting if NFS data mounts are not available high_availability['mountpoint'] = '/var/opt/gitlab/git-data' - + # Disable components that will not be on the GitLab application server - postgresql['enable'] = false - redis['enable'] = false - + roles ['application_role'] + # PostgreSQL connection details gitlab_rails['db_adapter'] = 'postgresql' gitlab_rails['db_encoding'] = 'unicode' gitlab_rails['db_host'] = '10.1.0.5' # IP/hostname of database server gitlab_rails['db_password'] = 'DB password' - + # Redis connection details gitlab_rails['redis_port'] = '6379' gitlab_rails['redis_host'] = '10.1.0.6' # IP/hostname of Redis server gitlab_rails['redis_password'] = 'Redis Password' ``` - - > **Note:** To maintain uniformity of links across HA clusters, the `external_url` - on the first application server as well as the additional application - servers should point to the external url that users will use to access GitLab. + + > **Note:** To maintain uniformity of links across HA clusters, the `external_url` + on the first application server as well as the additional application + servers should point to the external url that users will use to access GitLab. In a typical HA setup, this will be the url of the load balancer which will - route traffic to all GitLab application servers in the HA cluster. + route traffic to all GitLab application servers in the HA cluster. 1. Run `sudo gitlab-ctl reconfigure` to compile the configuration. -- cgit v1.2.1 From 8b13bdbe4d5805727c2c165f8cf0c7593c4af92d Mon Sep 17 00:00:00 2001 From: Valery Sizov Date: Tue, 19 Dec 2017 13:52:57 +0200 Subject: [CE->EE] Fix spec/lib/gitlab/git/gitlab_projects_spec.rb --- spec/lib/gitlab/git/gitlab_projects_spec.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/git/gitlab_projects_spec.rb b/spec/lib/gitlab/git/gitlab_projects_spec.rb index 18906955df6..24da9589458 100644 --- a/spec/lib/gitlab/git/gitlab_projects_spec.rb +++ b/spec/lib/gitlab/git/gitlab_projects_spec.rb @@ -41,7 +41,8 @@ describe Gitlab::Git::GitlabProjects do end it "fails if the source path doesn't exist" do - expect(logger).to receive(:error).with("mv-project failed: source path <#{tmp_repos_path}/bad-src.git> does not exist.") + expected_source_path = File.join(tmp_repos_path, 'bad-src.git') + expect(logger).to receive(:error).with("mv-project failed: source path <#{expected_source_path}> does not exist.") result = build_gitlab_projects(tmp_repos_path, 'bad-src.git').mv_project('repo.git') expect(result).to be_falsy @@ -50,7 +51,8 @@ describe Gitlab::Git::GitlabProjects do it 'fails if the destination path already exists' do FileUtils.mkdir_p(File.join(tmp_repos_path, 'already-exists.git')) - message = "mv-project failed: destination path <#{tmp_repos_path}/already-exists.git> already exists." + expected_distination_path = File.join(tmp_repos_path, 'already-exists.git') + message = "mv-project failed: destination path <#{expected_distination_path}> already exists." expect(logger).to receive(:error).with(message) expect(gl_projects.mv_project('already-exists.git')).to be_falsy -- cgit v1.2.1 From ac862490392b029ac4937188e02bdf09f4505869 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 19 Dec 2017 10:57:12 +0100 Subject: Don't link LFS-objects multiple times. If Unlinking a fork would fail somewhere after this, the LFS objects might still be linked. Which would cause issues when trying to destroy a project. --- app/services/projects/unlink_fork_service.rb | 2 +- .../bvl-fix-unlinking-with-lfs-objects.yml | 6 ++++++ spec/services/projects/unlink_fork_service_spec.rb | 20 ++++++++++++++++++++ 3 files changed, 27 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml diff --git a/app/services/projects/unlink_fork_service.rb b/app/services/projects/unlink_fork_service.rb index c499f384426..842fe4e09c4 100644 --- a/app/services/projects/unlink_fork_service.rb +++ b/app/services/projects/unlink_fork_service.rb @@ -5,7 +5,7 @@ module Projects if fork_source = @project.fork_source fork_source.lfs_objects.find_each do |lfs_object| - lfs_object.projects << @project + lfs_object.projects << @project unless lfs_object.projects.include?(@project) end refresh_forks_count(fork_source) diff --git a/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml b/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml new file mode 100644 index 00000000000..058d686e74c --- /dev/null +++ b/changelogs/unreleased/bvl-fix-unlinking-with-lfs-objects.yml @@ -0,0 +1,6 @@ +--- +title: Don't link LFS objects to a project when unlinking forks when they were already + linked +merge_request: 16006 +author: +type: fixed diff --git a/spec/services/projects/unlink_fork_service_spec.rb b/spec/services/projects/unlink_fork_service_spec.rb index 2bba71fef4f..3ec6139bfa6 100644 --- a/spec/services/projects/unlink_fork_service_spec.rb +++ b/spec/services/projects/unlink_fork_service_spec.rb @@ -62,6 +62,26 @@ describe Projects::UnlinkForkService do expect(source.forks_count).to be_zero end + context 'when the source has LFS objects' do + let(:lfs_object) { create(:lfs_object) } + + before do + lfs_object.projects << project + end + + it 'links the fork to the lfs object before unlinking' do + subject.execute + + expect(lfs_object.projects).to include(forked_project) + end + + it 'does not fail if the lfs objects were already linked' do + lfs_object.projects << forked_project + + expect { subject.execute }.not_to raise_error + end + end + context 'when the original project was deleted' do it 'does not fail when the original project is deleted' do source = forked_project.forked_from_project -- cgit v1.2.1 From 23a38eac61f44c0ee3baed68ddcf23f28c03b20d Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 19 Dec 2017 16:18:48 +0000 Subject: Fix shortcut links on help page --- app/assets/javascripts/docs/docs_bundle.js | 13 +++++++++++++ app/assets/javascripts/shortcuts.js | 10 ++++++++-- app/views/help/index.html.haml | 10 ++++++++-- changelogs/unreleased/fix-docs-help-shortcut.yml | 5 +++++ config/webpack.config.js | 1 + spec/features/help_pages_spec.rb | 18 ++++++++++++++++++ 6 files changed, 53 insertions(+), 4 deletions(-) create mode 100644 app/assets/javascripts/docs/docs_bundle.js create mode 100644 changelogs/unreleased/fix-docs-help-shortcut.yml diff --git a/app/assets/javascripts/docs/docs_bundle.js b/app/assets/javascripts/docs/docs_bundle.js new file mode 100644 index 00000000000..a32bd6d0fc7 --- /dev/null +++ b/app/assets/javascripts/docs/docs_bundle.js @@ -0,0 +1,13 @@ +import Mousetrap from 'mousetrap'; + +function addMousetrapClick(el, key) { + el.addEventListener('click', () => Mousetrap.trigger(key)); +} + +function domContentLoaded() { + addMousetrapClick(document.querySelector('.js-trigger-shortcut'), '?'); + addMousetrapClick(document.querySelector('.js-trigger-search-bar'), 's'); +} + +document.addEventListener('DOMContentLoaded', domContentLoaded); + diff --git a/app/assets/javascripts/shortcuts.js b/app/assets/javascripts/shortcuts.js index 130730b1700..d2f0d7410da 100644 --- a/app/assets/javascripts/shortcuts.js +++ b/app/assets/javascripts/shortcuts.js @@ -51,7 +51,10 @@ export default class Shortcuts { } onToggleHelp(e) { - e.preventDefault(); + if (e.preventDefault) { + e.preventDefault(); + } + Shortcuts.toggleHelp(this.enabledHelp); } @@ -112,6 +115,9 @@ export default class Shortcuts { static focusSearch(e) { $('#search').focus(); - e.preventDefault(); + + if (e.preventDefault) { + e.preventDefault(); + } } } diff --git a/app/views/help/index.html.haml b/app/views/help/index.html.haml index 021de4f0caf..b8692009225 100644 --- a/app/views/help/index.html.haml +++ b/app/views/help/index.html.haml @@ -1,3 +1,5 @@ += webpack_bundle_tag 'docs' + %div - if current_application_settings.help_page_text.present? = markdown_field(current_application_settings, :help_page_text) @@ -37,8 +39,12 @@ Quick help %ul.well-list %li= link_to 'See our website for getting help', support_url - %li= link_to 'Use the search bar on the top of this page', '#', onclick: 'Shortcuts.focusSearch(event)' - %li= link_to 'Use shortcuts', '#', onclick: 'Shortcuts.toggleHelp()' + %li + %button.btn-blank.btn-link.js-trigger-search-bar{ type: 'button' } + Use the search bar on the top of this page + %li + %button.btn-blank.btn-link.js-trigger-shortcut{ type: 'button' } + Use shortcuts - unless current_application_settings.help_page_hide_commercial_content? %li= link_to 'Get a support subscription', 'https://about.gitlab.com/pricing/' %li= link_to 'Compare GitLab editions', 'https://about.gitlab.com/features/#compare' diff --git a/changelogs/unreleased/fix-docs-help-shortcut.yml b/changelogs/unreleased/fix-docs-help-shortcut.yml new file mode 100644 index 00000000000..8c172e44160 --- /dev/null +++ b/changelogs/unreleased/fix-docs-help-shortcut.yml @@ -0,0 +1,5 @@ +--- +title: Fix shortcut links on help page +merge_request: +author: +type: fixed diff --git a/config/webpack.config.js b/config/webpack.config.js index 78ced4c3e8c..f02fcda827a 100644 --- a/config/webpack.config.js +++ b/config/webpack.config.js @@ -36,6 +36,7 @@ var config = { cycle_analytics: './cycle_analytics/cycle_analytics_bundle.js', commit_pipelines: './commit/pipelines/pipelines_bundle.js', deploy_keys: './deploy_keys/index.js', + docs: './docs/docs_bundle.js', diff_notes: './diff_notes/diff_notes_bundle.js', environments: './environments/environments_bundle.js', environments_folder: './environments/folder/environments_folder_bundle.js', diff --git a/spec/features/help_pages_spec.rb b/spec/features/help_pages_spec.rb index ab896a310be..0d04ed612c2 100644 --- a/spec/features/help_pages_spec.rb +++ b/spec/features/help_pages_spec.rb @@ -32,6 +32,24 @@ describe 'Help Pages' do it_behaves_like 'help page', prefix: '/gitlab' end + + context 'quick link shortcuts', :js do + before do + visit help_path + end + + it 'focuses search bar' do + find('.js-trigger-search-bar').click + + expect(page).to have_selector('#search:focus') + end + + it 'opens shortcuts help dialog' do + find('.js-trigger-shortcut').click + + expect(page).to have_selector('#modal-shortcuts') + end + end end context 'in a production environment with version check enabled', :js do -- cgit v1.2.1 From 9410fbef9fa3979673b3769c5dc40686ea125240 Mon Sep 17 00:00:00 2001 From: Mario de la Ossa Date: Tue, 19 Dec 2017 17:02:56 +0000 Subject: Fix tags in the Activity tab not being clickable --- app/views/events/event/_push.html.haml | 3 +- changelogs/unreleased/33028-event-tag-links.yml | 5 +++ spec/views/events/event/_push.html.haml_spec.rb | 55 +++++++++++++++++++++++++ 3 files changed, 62 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/33028-event-tag-links.yml create mode 100644 spec/views/events/event/_push.html.haml_spec.rb diff --git a/app/views/events/event/_push.html.haml b/app/views/events/event/_push.html.haml index 9a763887b30..f85f5c5be88 100644 --- a/app/views/events/event/_push.html.haml +++ b/app/views/events/event/_push.html.haml @@ -7,7 +7,8 @@ %span.pushed #{event.action_name} #{event.ref_type} %strong - commits_link = project_commits_path(project, event.ref_name) - = link_to_if project.repository.branch_exists?(event.ref_name), event.ref_name, commits_link, class: 'ref-name' + - should_link = event.tag? ? project.repository.tag_exists?(event.ref_name) : project.repository.branch_exists?(event.ref_name) + = link_to_if should_link, event.ref_name, commits_link, class: 'ref-name' = render "events/event_scope", event: event diff --git a/changelogs/unreleased/33028-event-tag-links.yml b/changelogs/unreleased/33028-event-tag-links.yml new file mode 100644 index 00000000000..1d674200dcd --- /dev/null +++ b/changelogs/unreleased/33028-event-tag-links.yml @@ -0,0 +1,5 @@ +--- +title: Fix tags in the Activity tab not being clickable +merge_request: 15996 +author: Mario de la Ossa +type: fixed diff --git a/spec/views/events/event/_push.html.haml_spec.rb b/spec/views/events/event/_push.html.haml_spec.rb new file mode 100644 index 00000000000..f5634de4916 --- /dev/null +++ b/spec/views/events/event/_push.html.haml_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' + +describe 'events/event/_push.html.haml' do + let(:event) { build_stubbed(:push_event) } + + context 'with a branch' do + let(:payload) { build_stubbed(:push_event_payload, event: event) } + + before do + allow(event).to receive(:push_event_payload).and_return(payload) + end + + it 'links to the branch' do + allow(event.project.repository).to receive(:branch_exists?).with(event.ref_name).and_return(true) + link = project_commits_path(event.project, event.ref_name) + + render partial: 'events/event/push', locals: { event: event } + + expect(rendered).to have_link(event.ref_name, href: link) + end + + context 'that has been deleted' do + it 'does not link to the branch' do + render partial: 'events/event/push', locals: { event: event } + + expect(rendered).not_to have_link(event.ref_name) + end + end + end + + context 'with a tag' do + let(:payload) { build_stubbed(:push_event_payload, event: event, ref_type: :tag, ref: 'v0.1.0') } + + before do + allow(event).to receive(:push_event_payload).and_return(payload) + end + + it 'links to the tag' do + allow(event.project.repository).to receive(:tag_exists?).with(event.ref_name).and_return(true) + link = project_commits_path(event.project, event.ref_name) + + render partial: 'events/event/push', locals: { event: event } + + expect(rendered).to have_link(event.ref_name, href: link) + end + + context 'that has been deleted' do + it 'does not link to the tag' do + render partial: 'events/event/push', locals: { event: event } + + expect(rendered).not_to have_link(event.ref_name) + end + end + end +end -- cgit v1.2.1 From 271ebe661103b4b523ae9e50f755df9a7aeedb1d Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 19 Dec 2017 17:48:29 +0000 Subject: Show inline edit button for issues --- app/assets/javascripts/issue_show/components/app.vue | 2 +- app/assets/javascripts/issue_show/components/title.vue | 2 +- app/assets/javascripts/issue_show/index.js | 7 ------- app/views/projects/issues/show.html.haml | 5 ----- changelogs/unreleased/show-inline-edit-btn.yml | 5 +++++ spec/features/issues/issue_detail_spec.rb | 2 +- spec/features/projects/issuable_templates_spec.rb | 8 ++------ 7 files changed, 10 insertions(+), 21 deletions(-) create mode 100644 changelogs/unreleased/show-inline-edit-btn.yml diff --git a/app/assets/javascripts/issue_show/components/app.vue b/app/assets/javascripts/issue_show/components/app.vue index 25ebe5314e0..952f49d522e 100644 --- a/app/assets/javascripts/issue_show/components/app.vue +++ b/app/assets/javascripts/issue_show/components/app.vue @@ -32,7 +32,7 @@ export default { showInlineEditButton: { type: Boolean, required: false, - default: false, + default: true, }, showDeleteButton: { type: Boolean, diff --git a/app/assets/javascripts/issue_show/components/title.vue b/app/assets/javascripts/issue_show/components/title.vue index a363d06d950..b7e6eadd440 100644 --- a/app/assets/javascripts/issue_show/components/title.vue +++ b/app/assets/javascripts/issue_show/components/title.vue @@ -79,7 +79,7 @@ v-tooltip v-if="showInlineEditButton && canUpdate" type="button" - class="btn btn-default btn-edit btn-svg" + class="btn btn-default btn-edit btn-svg js-issuable-edit" v-html="pencilIcon" title="Edit title and description" data-placement="bottom" diff --git a/app/assets/javascripts/issue_show/index.js b/app/assets/javascripts/issue_show/index.js index 7b762496ba5..75dfdedcf1b 100644 --- a/app/assets/javascripts/issue_show/index.js +++ b/app/assets/javascripts/issue_show/index.js @@ -1,5 +1,4 @@ import Vue from 'vue'; -import eventHub from './event_hub'; import issuableApp from './components/app.vue'; import '../vue_shared/vue_resource_interceptor'; @@ -7,12 +6,6 @@ document.addEventListener('DOMContentLoaded', () => { const initialDataEl = document.getElementById('js-issuable-app-initial-data'); const props = JSON.parse(initialDataEl.innerHTML.replace(/"/g, '"')); - $('.js-issuable-edit').on('click', (e) => { - e.preventDefault(); - - eventHub.$emit('open.form'); - }); - return new Vue({ el: document.getElementById('js-issuable-app'), components: { diff --git a/app/views/projects/issues/show.html.haml b/app/views/projects/issues/show.html.haml index eab7879c7bf..1f28d8acff6 100644 --- a/app/views/projects/issues/show.html.haml +++ b/app/views/projects/issues/show.html.haml @@ -39,8 +39,6 @@ = icon('caret-down') .dropdown-menu.dropdown-menu-align-right.hidden-lg %ul - - if can_update_issue - %li= link_to 'Edit', edit_project_issue_path(@project, @issue), class: 'js-issuable-edit' - unless current_user == @issue.author %li= link_to 'Report abuse', new_abuse_report_path(user_id: @issue.author.id, ref_url: issue_url(@issue)) - if can_update_issue @@ -52,9 +50,6 @@ %li.divider %li= link_to 'New issue', new_project_issue_path(@project), title: 'New issue', id: 'new_issue_link' - - if can_update_issue - = link_to 'Edit', edit_project_issue_path(@project, @issue), class: 'hidden-xs hidden-sm btn btn-grouped js-issuable-edit' - = render 'shared/issuable/close_reopen_button', issuable: @issue, can_update: can_update_issue - if can_report_spam diff --git a/changelogs/unreleased/show-inline-edit-btn.yml b/changelogs/unreleased/show-inline-edit-btn.yml new file mode 100644 index 00000000000..8cfe9b7d75a --- /dev/null +++ b/changelogs/unreleased/show-inline-edit-btn.yml @@ -0,0 +1,5 @@ +--- +title: Move edit button to second row on issue page (and change it to a pencil icon) +merge_request: +author: +type: changed diff --git a/spec/features/issues/issue_detail_spec.rb b/spec/features/issues/issue_detail_spec.rb index 4224a8fe5d4..babb0285590 100644 --- a/spec/features/issues/issue_detail_spec.rb +++ b/spec/features/issues/issue_detail_spec.rb @@ -24,7 +24,7 @@ feature 'Issue Detail', :js do visit project_issue_path(project, issue) wait_for_requests - click_link 'Edit' + page.find('.js-issuable-edit').click fill_in 'issuable-title', with: 'issue title' click_button 'Save' wait_for_requests diff --git a/spec/features/projects/issuable_templates_spec.rb b/spec/features/projects/issuable_templates_spec.rb index 0257cd157c9..4319fc2746c 100644 --- a/spec/features/projects/issuable_templates_spec.rb +++ b/spec/features/projects/issuable_templates_spec.rb @@ -32,9 +32,7 @@ feature 'issuable templates', :js do message: 'added issue template', branch_name: 'master') visit project_issue_path project, issue - page.within('.js-issuable-actions') do - click_on 'Edit' - end + page.find('.js-issuable-edit').click fill_in :'issuable-title', with: 'test issue title' end @@ -77,9 +75,7 @@ feature 'issuable templates', :js do message: 'added issue template', branch_name: 'master') visit project_issue_path project, issue - page.within('.js-issuable-actions') do - click_on 'Edit' - end + page.find('.js-issuable-edit').click fill_in :'issuable-title', with: 'test issue title' fill_in :'issue-description', with: prior_description end -- cgit v1.2.1 From 0a0a62c93636627e2c1c34a41af33e178efd7647 Mon Sep 17 00:00:00 2001 From: Clement Ho Date: Tue, 19 Dec 2017 17:50:38 +0000 Subject: Remove related links in MR widget when empty state --- .../vue_merge_request_widget/mr_widget_options.js | 2 +- .../stores/get_state_key.js | 28 ++++++++++++---------- .../stores/mr_widget_store.js | 5 ++++ .../vue_merge_request_widget/stores/state_maps.js | 17 +++++++++++++ .../unreleased/remove-links-mr-empty-state.yml | 5 ++++ .../vue_mr_widget/mr_widget_options_spec.js | 28 ++++++++++++++++++++++ .../vue_mr_widget/stores/mr_widget_store_spec.js | 13 ++++++++++ 7 files changed, 84 insertions(+), 14 deletions(-) create mode 100644 changelogs/unreleased/remove-links-mr-empty-state.yml diff --git a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js index 9cb3edead86..8a9129c385b 100644 --- a/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js +++ b/app/assets/javascripts/vue_merge_request_widget/mr_widget_options.js @@ -62,7 +62,7 @@ export default { return this.mr.hasCI; }, shouldRenderRelatedLinks() { - return !!this.mr.relatedLinks; + return !!this.mr.relatedLinks && !this.mr.isNothingToMergeState; }, shouldRenderDeployments() { return this.mr.deployments.length; diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js index 7c15abfff10..2bace3311c8 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/get_state_key.js @@ -1,30 +1,32 @@ +import { stateKey } from './state_maps'; + export default function deviseState(data) { if (data.project_archived) { - return 'archived'; + return stateKey.archived; } else if (data.branch_missing) { - return 'missingBranch'; + return stateKey.missingBranch; } else if (!data.commits_count) { - return 'nothingToMerge'; + return stateKey.nothingToMerge; } else if (this.mergeStatus === 'unchecked') { - return 'checking'; + return stateKey.checking; } else if (data.has_conflicts) { - return 'conflicts'; + return stateKey.conflicts; } else if (data.work_in_progress) { - return 'workInProgress'; + return stateKey.workInProgress; } else if (this.onlyAllowMergeIfPipelineSucceeds && this.isPipelineFailed) { - return 'pipelineFailed'; + return stateKey.pipelineFailed; } else if (this.hasMergeableDiscussionsState) { - return 'unresolvedDiscussions'; + return stateKey.unresolvedDiscussions; } else if (this.isPipelineBlocked) { - return 'pipelineBlocked'; + return stateKey.pipelineBlocked; } else if (this.hasSHAChanged) { - return 'shaMismatch'; + return stateKey.shaMismatch; } else if (this.mergeWhenPipelineSucceeds) { - return this.mergeError ? 'autoMergeFailed' : 'mergeWhenPipelineSucceeds'; + return this.mergeError ? stateKey.autoMergeFailed : stateKey.mergeWhenPipelineSucceeds; } else if (!this.canMerge) { - return 'notAllowedToMerge'; + return stateKey.notAllowedToMerge; } else if (this.canBeMerged) { - return 'readyToMerge'; + return stateKey.readyToMerge; } return null; } diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js index 707766e08e4..93d31a2a684 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/mr_widget_store.js @@ -1,5 +1,6 @@ import Timeago from 'timeago.js'; import { getStateKey } from '../dependencies'; +import { stateKey } from './state_maps'; import { formatDate } from '../../lib/utils/datetime_utility'; export default class MergeRequestStore { @@ -120,6 +121,10 @@ export default class MergeRequestStore { } } + get isNothingToMergeState() { + return this.state === stateKey.nothingToMerge; + } + static getEventObject(event) { return { author: MergeRequestStore.getAuthorObject(event), diff --git a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js index 9074a064a6d..de980c175fb 100644 --- a/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js +++ b/app/assets/javascripts/vue_merge_request_widget/stores/state_maps.js @@ -31,6 +31,23 @@ const statesToShowHelpWidget = [ 'autoMergeFailed', ]; +export const stateKey = { + archived: 'archived', + missingBranch: 'missingBranch', + nothingToMerge: 'nothingToMerge', + checking: 'checking', + conflicts: 'conflicts', + workInProgress: 'workInProgress', + pipelineFailed: 'pipelineFailed', + unresolvedDiscussions: 'unresolvedDiscussions', + pipelineBlocked: 'pipelineBlocked', + shaMismatch: 'shaMismatch', + autoMergeFailed: 'autoMergeFailed', + mergeWhenPipelineSucceeds: 'mergeWhenPipelineSucceeds', + notAllowedToMerge: 'notAllowedToMerge', + readyToMerge: 'readyToMerge', +}; + export default { stateToComponentMap, statesToShowHelpWidget, diff --git a/changelogs/unreleased/remove-links-mr-empty-state.yml b/changelogs/unreleased/remove-links-mr-empty-state.yml new file mode 100644 index 00000000000..c666bc2c81d --- /dev/null +++ b/changelogs/unreleased/remove-links-mr-empty-state.yml @@ -0,0 +1,5 @@ +--- +title: Remove related links in MR widget when empty state +merge_request: +author: +type: fixed diff --git a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js index 9e6d0aa472c..74b343c573e 100644 --- a/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js +++ b/spec/javascripts/vue_mr_widget/mr_widget_options_spec.js @@ -2,6 +2,7 @@ import Vue from 'vue'; import mrWidgetOptions from '~/vue_merge_request_widget/mr_widget_options'; import eventHub from '~/vue_merge_request_widget/event_hub'; import notify from '~/lib/utils/notify'; +import { stateKey } from '~/vue_merge_request_widget/stores/state_maps'; import mockData from './mock_data'; import mountComponent from '../helpers/vue_mount_component_helper'; @@ -344,4 +345,31 @@ describe('mrWidgetOptions', () => { expect(comps['mr-widget-merge-when-pipeline-succeeds']).toBeDefined(); }); }); + + describe('rendering relatedLinks', () => { + beforeEach((done) => { + vm.mr.relatedLinks = { + assignToMe: null, + closing: ` + Date: Tue, 19 Dec 2017 11:01:14 -0700 Subject: Line up search dropdown with other nav dropdowns --- app/assets/stylesheets/framework/dropdowns.scss | 6 +++++- app/assets/stylesheets/pages/search.scss | 7 ------- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/app/assets/stylesheets/framework/dropdowns.scss b/app/assets/stylesheets/framework/dropdowns.scss index 22303e6ec14..bc907a390d8 100644 --- a/app/assets/stylesheets/framework/dropdowns.scss +++ b/app/assets/stylesheets/framework/dropdowns.scss @@ -28,7 +28,6 @@ .dropdown-menu, .dropdown-menu-nav { @include set-visible; - display: block; min-height: 40px; @media (max-width: $screen-xs-max) { @@ -47,6 +46,11 @@ } } +// Get search dropdown to line up with other nav dropdowns +.search-input-container .dropdown-menu { + margin-top: 11px; +} + .dropdown-toggle { padding: 6px 8px 6px 10px; background-color: $white-light; diff --git a/app/assets/stylesheets/pages/search.scss b/app/assets/stylesheets/pages/search.scss index 2381d7eb92b..c9363188505 100644 --- a/app/assets/stylesheets/pages/search.scss +++ b/app/assets/stylesheets/pages/search.scss @@ -145,13 +145,6 @@ input[type="checkbox"]:hover { background-color: $nav-badge-bg; border-color: $border-color; } - - .dropdown-menu { - transition-duration: 100ms, 75ms; - transition-delay: 75ms, 100ms; - transform: translateY(7px); - opacity: 1; - } } &.has-value { -- cgit v1.2.1 From f8b9a45854a1ce6841ff8d2057856a9424e562ed Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Tue, 19 Dec 2017 18:36:53 +0000 Subject: Prevent some specs from mangling the gitlab-shell checkout --- ...wner_sees_link_to_create_license_file_in_empty_project_spec.rb | 8 +++----- spec/features/tags/master_views_tags_spec.rb | 5 ++--- 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb b/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb index 6c616bf0456..8ac9821b879 100644 --- a/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb +++ b/spec/features/projects/files/project_owner_sees_link_to_create_license_file_in_empty_project_spec.rb @@ -2,15 +2,15 @@ require 'spec_helper' feature 'project owner sees a link to create a license file in empty project', :js do let(:project_master) { create(:user) } - let(:project) { create(:project) } + let(:project) { create(:project_empty_repo) } + background do - project.team << [project_master, :master] + project.add_master(project_master) sign_in(project_master) end scenario 'project master creates a license file from a template' do visit project_path(project) - click_link 'Create empty bare repository' click_on 'LICENSE' expect(page).to have_content('New file') @@ -26,8 +26,6 @@ feature 'project owner sees a link to create a license file in empty project', : expect(file_content).to have_content("Copyright (c) #{Time.now.year} #{project.namespace.human_name}") fill_in :commit_message, with: 'Add a LICENSE file', visible: true - # Remove pre-receive hook so we can push without auth - FileUtils.rm_f(File.join(project.repository.path, 'hooks', 'pre-receive')) click_button 'Commit changes' expect(current_path).to eq( diff --git a/spec/features/tags/master_views_tags_spec.rb b/spec/features/tags/master_views_tags_spec.rb index 9edc7ced163..4662367d843 100644 --- a/spec/features/tags/master_views_tags_spec.rb +++ b/spec/features/tags/master_views_tags_spec.rb @@ -4,18 +4,17 @@ feature 'Master views tags' do let(:user) { create(:user) } before do - project.team << [user, :master] + project.add_master(user) sign_in(user) end context 'when project has no tags' do let(:project) { create(:project_empty_repo) } + before do visit project_path(project) click_on 'README' fill_in :commit_message, with: 'Add a README file', visible: true - # Remove pre-receive hook so we can push without auth - FileUtils.rm_f(File.join(project.repository.path, 'hooks', 'pre-receive')) click_button 'Commit changes' visit project_tags_path(project) end -- cgit v1.2.1