From d2283f4f0e98b86800ba6a222593647e4991375d Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Fri, 31 Mar 2017 13:56:27 +0200 Subject: Backport API changes needed to fix sticking in EE These changes are ported over from https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1502 to reduce the number of merge conflicts that may occur. --- .../unreleased/backport-sticking-api-helper-changes.yml | 4 ++++ lib/api/helpers/runner.rb | 6 +++++- lib/api/runner.rb | 15 +++++---------- lib/ci/api/builds.rb | 15 +++++---------- lib/ci/api/helpers.rb | 6 +++++- 5 files changed, 24 insertions(+), 22 deletions(-) create mode 100644 changelogs/unreleased/backport-sticking-api-helper-changes.yml diff --git a/changelogs/unreleased/backport-sticking-api-helper-changes.yml b/changelogs/unreleased/backport-sticking-api-helper-changes.yml new file mode 100644 index 00000000000..170ef152271 --- /dev/null +++ b/changelogs/unreleased/backport-sticking-api-helper-changes.yml @@ -0,0 +1,4 @@ +--- +title: Backport API changes needed to fix sticking in EE +merge_request: +author: diff --git a/lib/api/helpers/runner.rb b/lib/api/helpers/runner.rb index 74848a6e144..1369b021ea4 100644 --- a/lib/api/helpers/runner.rb +++ b/lib/api/helpers/runner.rb @@ -50,10 +50,14 @@ module API forbidden!('Job has been erased!') if job.erased? end - def authenticate_job!(job) + def authenticate_job! + job = Ci::Build.find_by_id(params[:id]) + validate_job!(job) do forbidden! unless job_token_valid?(job) end + + job end def job_token_valid?(job) diff --git a/lib/api/runner.rb b/lib/api/runner.rb index 4c9db2c8716..d288369e362 100644 --- a/lib/api/runner.rb +++ b/lib/api/runner.rb @@ -113,8 +113,7 @@ module API optional :state, type: String, desc: %q(Job's status: success, failed) end put '/:id' do - job = Ci::Build.find_by_id(params[:id]) - authenticate_job!(job) + job = authenticate_job! job.update_attributes(trace: params[:trace]) if params[:trace] @@ -140,8 +139,7 @@ module API optional :token, type: String, desc: %q(Job's authentication token) end patch '/:id/trace' do - job = Ci::Build.find_by_id(params[:id]) - authenticate_job!(job) + job = authenticate_job! error!('400 Missing header Content-Range', 400) unless request.headers.has_key?('Content-Range') content_range = request.headers['Content-Range'] @@ -175,8 +173,7 @@ module API require_gitlab_workhorse! Gitlab::Workhorse.verify_api_request!(headers) - job = Ci::Build.find_by_id(params[:id]) - authenticate_job!(job) + job = authenticate_job! forbidden!('Job is not running') unless job.running? if params[:filesize] @@ -212,8 +209,7 @@ module API not_allowed! unless Gitlab.config.artifacts.enabled require_gitlab_workhorse! - job = Ci::Build.find_by_id(params[:id]) - authenticate_job!(job) + job = authenticate_job! forbidden!('Job is not running!') unless job.running? artifacts_upload_path = ArtifactUploader.artifacts_upload_path @@ -245,8 +241,7 @@ module API optional :token, type: String, desc: %q(Job's authentication token) end get '/:id/artifacts' do - job = Ci::Build.find_by_id(params[:id]) - authenticate_job!(job) + job = authenticate_job! artifacts_file = job.artifacts_file unless artifacts_file.file_storage? diff --git a/lib/ci/api/builds.rb b/lib/ci/api/builds.rb index 746e76a1b1f..95cc6308c3b 100644 --- a/lib/ci/api/builds.rb +++ b/lib/ci/api/builds.rb @@ -86,8 +86,7 @@ module Ci # Example Request: # PATCH /builds/:id/trace.txt patch ":id/trace.txt" do - build = Ci::Build.find_by_id(params[:id]) - authenticate_build!(build) + build = authenticate_build! error!('400 Missing header Content-Range', 400) unless request.headers.has_key?('Content-Range') content_range = request.headers['Content-Range'] @@ -117,8 +116,7 @@ module Ci require_gitlab_workhorse! Gitlab::Workhorse.verify_api_request!(headers) not_allowed! unless Gitlab.config.artifacts.enabled - build = Ci::Build.find_by_id(params[:id]) - authenticate_build!(build) + build = authenticate_build! forbidden!('build is not running') unless build.running? if params[:filesize] @@ -154,8 +152,7 @@ module Ci post ":id/artifacts" do require_gitlab_workhorse! not_allowed! unless Gitlab.config.artifacts.enabled - build = Ci::Build.find_by_id(params[:id]) - authenticate_build!(build) + build = authenticate_build! forbidden!('Build is not running!') unless build.running? artifacts_upload_path = ArtifactUploader.artifacts_upload_path @@ -189,8 +186,7 @@ module Ci # Example Request: # GET /builds/:id/artifacts get ":id/artifacts" do - build = Ci::Build.find_by_id(params[:id]) - authenticate_build!(build) + build = authenticate_build! artifacts_file = build.artifacts_file unless artifacts_file.file_storage? @@ -214,8 +210,7 @@ module Ci # Example Request: # DELETE /builds/:id/artifacts delete ":id/artifacts" do - build = Ci::Build.find_by_id(params[:id]) - authenticate_build!(build) + build = authenticate_build! status(200) build.erase_artifacts! diff --git a/lib/ci/api/helpers.rb b/lib/ci/api/helpers.rb index 996990b464f..5109dc9670f 100644 --- a/lib/ci/api/helpers.rb +++ b/lib/ci/api/helpers.rb @@ -13,10 +13,14 @@ module Ci forbidden! unless current_runner end - def authenticate_build!(build) + def authenticate_build! + build = Ci::Build.find_by_id(params[:id]) + validate_build!(build) do forbidden! unless build_token_valid?(build) end + + build end def validate_build!(build) -- cgit v1.2.1 From b700a84e4069b099d98db11d91a36e3762467e29 Mon Sep 17 00:00:00 2001 From: Danilo Bargen Date: Thu, 30 Mar 2017 14:39:17 +0200 Subject: Log errors in UpdatePagesService --- app/services/base_service.rb | 4 ++++ app/services/projects/update_pages_service.rb | 1 + changelogs/unreleased/pages-debug-log.yml | 4 ++++ 3 files changed, 9 insertions(+) create mode 100644 changelogs/unreleased/pages-debug-log.yml diff --git a/app/services/base_service.rb b/app/services/base_service.rb index 745c2c4b681..a0cb00dba58 100644 --- a/app/services/base_service.rb +++ b/app/services/base_service.rb @@ -24,6 +24,10 @@ class BaseService Gitlab::AppLogger.info message end + def log_error(message) + Gitlab::AppLogger.error message + end + def system_hook_service SystemHooksService.new end diff --git a/app/services/projects/update_pages_service.rb b/app/services/projects/update_pages_service.rb index 523b9f41916..17cf71cf098 100644 --- a/app/services/projects/update_pages_service.rb +++ b/app/services/projects/update_pages_service.rb @@ -46,6 +46,7 @@ module Projects end def error(message, http_status = nil) + log_error("Projects::UpdatePagesService: #{message}") @status.allow_failure = !latest? @status.description = message @status.drop diff --git a/changelogs/unreleased/pages-debug-log.yml b/changelogs/unreleased/pages-debug-log.yml new file mode 100644 index 00000000000..328c8e4615b --- /dev/null +++ b/changelogs/unreleased/pages-debug-log.yml @@ -0,0 +1,4 @@ +--- +title: Log errors during generating of Gitlab Pages to debug log +merge_request: 10335 +author: Danilo Bargen -- cgit v1.2.1 From 0a09925dcef1976970bc2674432f69d46786c38f Mon Sep 17 00:00:00 2001 From: mhasbini Date: Fri, 31 Mar 2017 18:11:28 +0300 Subject: Enable Style/Proc cop for rubocop --- .rubocop.yml | 4 ++++ .rubocop_todo.yml | 5 ----- app/mailers/base_mailer.rb | 4 ++-- app/models/milestone.rb | 2 +- app/models/service.rb | 2 +- changelogs/unreleased/style-proc-cop.yml | 4 ++++ lib/api/api_guard.rb | 2 +- lib/gitlab/ldap/config.rb | 4 ++-- spec/initializers/trusted_proxies_spec.rb | 2 +- 9 files changed, 16 insertions(+), 13 deletions(-) create mode 100644 changelogs/unreleased/style-proc-cop.yml diff --git a/.rubocop.yml b/.rubocop.yml index fa1370ea1f3..ac6b141cea3 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -533,6 +533,10 @@ Style/WhileUntilModifier: Style/WordArray: Enabled: true +# Use `proc` instead of `Proc.new`. +Style/Proc: + Enabled: true + # Metrics ##################################################################### # A calculated magnitude based on number of assignments, diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index c24142c0a11..8588988dc87 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -226,11 +226,6 @@ Style/PredicateName: Style/PreferredHashMethods: Enabled: false -# Offense count: 8 -# Cop supports --auto-correct. -Style/Proc: - Enabled: false - # Offense count: 62 # Cop supports --auto-correct. # Configuration parameters: EnforcedStyle, SupportedStyles. diff --git a/app/mailers/base_mailer.rb b/app/mailers/base_mailer.rb index 79c3c2e62c5..a9b6b33eb5c 100644 --- a/app/mailers/base_mailer.rb +++ b/app/mailers/base_mailer.rb @@ -5,8 +5,8 @@ class BaseMailer < ActionMailer::Base attr_accessor :current_user helper_method :current_user, :can? - default from: Proc.new { default_sender_address.format } - default reply_to: Proc.new { default_reply_to_address.format } + default from: proc { default_sender_address.format } + default reply_to: proc { default_reply_to_address.format } def can? Ability.allowed?(current_user, action, subject) diff --git a/app/models/milestone.rb b/app/models/milestone.rb index e85d5709624..ac205b9b738 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -30,7 +30,7 @@ class Milestone < ActiveRecord::Base validates :title, presence: true, uniqueness: { scope: :project_id } validates :project, presence: true - validate :start_date_should_be_less_than_due_date, if: Proc.new { |m| m.start_date.present? && m.due_date.present? } + validate :start_date_should_be_less_than_due_date, if: proc { |m| m.start_date.present? && m.due_date.present? } strip_attributes :title diff --git a/app/models/service.rb b/app/models/service.rb index e73f7e5d1a3..54550177744 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -25,7 +25,7 @@ class Service < ActiveRecord::Base belongs_to :project, inverse_of: :services has_one :service_hook - validates :project_id, presence: true, unless: Proc.new { |service| service.template? } + validates :project_id, presence: true, unless: proc { |service| service.template? } scope :visible, -> { where.not(type: 'GitlabIssueTrackerService') } scope :issue_trackers, -> { where(category: 'issue_tracker') } diff --git a/changelogs/unreleased/style-proc-cop.yml b/changelogs/unreleased/style-proc-cop.yml new file mode 100644 index 00000000000..25acab740bd --- /dev/null +++ b/changelogs/unreleased/style-proc-cop.yml @@ -0,0 +1,4 @@ +--- +title: Enable Style/Proc cop for rubocop +merge_request: +author: mhasbini diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb index 409cb5b924f..9fcf04efa38 100644 --- a/lib/api/api_guard.rb +++ b/lib/api/api_guard.rb @@ -121,7 +121,7 @@ module API end def oauth2_bearer_token_error_handler - Proc.new do |e| + proc do |e| response = case e when MissingTokenError diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb index 28129198438..46deea3cc9f 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -124,9 +124,9 @@ module Gitlab def name_proc if allow_username_or_email_login - Proc.new { |name| name.gsub(/@.*\z/, '') } + proc { |name| name.gsub(/@.*\z/, '') } else - Proc.new { |name| name } + proc { |name| name } end end diff --git a/spec/initializers/trusted_proxies_spec.rb b/spec/initializers/trusted_proxies_spec.rb index ff8b8daa347..70a18f31744 100644 --- a/spec/initializers/trusted_proxies_spec.rb +++ b/spec/initializers/trusted_proxies_spec.rb @@ -56,7 +56,7 @@ describe 'trusted_proxies', lib: true do end def stub_request(headers = {}) - ActionDispatch::RemoteIp.new(Proc.new { }, false, Rails.application.config.action_dispatch.trusted_proxies).call(headers) + ActionDispatch::RemoteIp.new(proc { }, false, Rails.application.config.action_dispatch.trusted_proxies).call(headers) ActionDispatch::Request.new(headers) end -- cgit v1.2.1 From bf64582f671ae057c853ff97876348574fe14a53 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Fri, 31 Mar 2017 08:46:51 +0200 Subject: Fix warning when cloning repo that already exists fixes the warning: fatal: destination path '~/gitlab-ce/gitlab/tmp/tests/gitlab-test-fork_bare' already exists and is not an empty directory. --- spec/support/test_env.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/spec/support/test_env.rb b/spec/support/test_env.rb index a19a35c2c0d..1b5cb71a6b0 100644 --- a/spec/support/test_env.rb +++ b/spec/support/test_env.rb @@ -131,8 +131,10 @@ module TestEnv set_repo_refs(repo_path, branch_sha) - # We must copy bare repositories because we will push to them. - system(git_env, *%W(#{Gitlab.config.git.bin_path} clone -q --bare #{repo_path} #{repo_path_bare})) + unless File.directory?(repo_path_bare) + # We must copy bare repositories because we will push to them. + system(git_env, *%W(#{Gitlab.config.git.bin_path} clone -q --bare #{repo_path} #{repo_path_bare})) + end end def copy_repo(project) -- cgit v1.2.1 From ca12079ac8de3279cd57ed3df6a6579585799fa1 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 27 Mar 2017 20:46:39 +0200 Subject: Remove code deprecated in pipeline process service --- app/services/ci/process_pipeline_service.rb | 15 ------ spec/services/ci/process_pipeline_service_spec.rb | 59 ----------------------- 2 files changed, 74 deletions(-) diff --git a/app/services/ci/process_pipeline_service.rb b/app/services/ci/process_pipeline_service.rb index 2935d00c075..33edcd60944 100644 --- a/app/services/ci/process_pipeline_service.rb +++ b/app/services/ci/process_pipeline_service.rb @@ -5,8 +5,6 @@ module Ci def execute(pipeline) @pipeline = pipeline - ensure_created_builds! # TODO, remove me in 9.0 - new_builds = stage_indexes_of_created_builds.map do |index| process_stage(index) @@ -73,18 +71,5 @@ module Ci def created_builds pipeline.builds.created end - - # This method is DEPRECATED and should be removed in 9.0. - # - # We need it to maintain backwards compatibility with previous versions - # when builds were not created within one transaction with the pipeline. - # - def ensure_created_builds! - return if created_builds.any? - - Ci::CreatePipelineBuildsService - .new(project, current_user) - .execute(pipeline) - end end end diff --git a/spec/services/ci/process_pipeline_service_spec.rb b/spec/services/ci/process_pipeline_service_spec.rb index d93616c4f50..bb98fb37a90 100644 --- a/spec/services/ci/process_pipeline_service_spec.rb +++ b/spec/services/ci/process_pipeline_service_spec.rb @@ -418,65 +418,6 @@ describe Ci::ProcessPipelineService, '#execute', :services do end end - context 'when there are builds that are not created yet' do - let(:pipeline) do - create(:ci_pipeline, config: config) - end - - let(:config) do - { rspec: { stage: 'test', script: 'rspec' }, - deploy: { stage: 'deploy', script: 'rsync' } } - end - - before do - create_build('linux', stage: 'build', stage_idx: 0) - create_build('mac', stage: 'build', stage_idx: 0) - end - - it 'processes the pipeline' do - # Currently we have five builds with state created - # - expect(builds.count).to eq(0) - expect(all_builds.count).to eq(2) - - # Process builds service will enqueue builds from the first stage. - # - process_pipeline - - expect(builds.count).to eq(2) - expect(all_builds.count).to eq(2) - - # When builds succeed we will enqueue remaining builds. - # - # We will have 2 succeeded, 1 pending (from stage test), total 4 (two - # additional build from `.gitlab-ci.yml`). - # - succeed_pending - process_pipeline - - expect(builds.success.count).to eq(2) - expect(builds.pending.count).to eq(1) - expect(all_builds.count).to eq(4) - - # When pending merge_when_pipeline_succeeds in stage test, we enqueue deploy stage. - # - succeed_pending - process_pipeline - - expect(builds.pending.count).to eq(1) - expect(builds.success.count).to eq(3) - expect(all_builds.count).to eq(4) - - # When the last one succeeds we have 4 successful builds. - # - succeed_pending - process_pipeline - - expect(builds.success.count).to eq(4) - expect(all_builds.count).to eq(4) - end - end - def process_pipeline described_class.new(pipeline.project, user).execute(pipeline) end -- cgit v1.2.1 From 068f4b47c82f6c30dd2ab332f6af29abf8f4a6ae Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Wed, 29 Mar 2017 10:43:16 +0200 Subject: Add changelog entry for deprecating legacy pipelines --- .../unreleased/fix-gb-remove-deprecated-pipeline-processing-code.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/fix-gb-remove-deprecated-pipeline-processing-code.yml diff --git a/changelogs/unreleased/fix-gb-remove-deprecated-pipeline-processing-code.yml b/changelogs/unreleased/fix-gb-remove-deprecated-pipeline-processing-code.yml new file mode 100644 index 00000000000..32862b527fd --- /dev/null +++ b/changelogs/unreleased/fix-gb-remove-deprecated-pipeline-processing-code.yml @@ -0,0 +1,4 @@ +--- +title: Drop support for correctly processing legacy pipelines +merge_request: 10266 +author: -- cgit v1.2.1 From ed5d59d4c4b6ac092223774a859fbaa3bf9c3699 Mon Sep 17 00:00:00 2001 From: mhasbini Date: Sat, 1 Apr 2017 18:55:14 +0300 Subject: Fix symlink icon in project tree --- changelogs/unreleased/22303-symbolic-in-tree.yml | 4 ++++ lib/api/entities.rb | 2 +- lib/gitlab/git/tree.rb | 2 +- spec/lib/gitlab/git/tree_spec.rb | 1 + 4 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/22303-symbolic-in-tree.yml diff --git a/changelogs/unreleased/22303-symbolic-in-tree.yml b/changelogs/unreleased/22303-symbolic-in-tree.yml new file mode 100644 index 00000000000..02444f571d0 --- /dev/null +++ b/changelogs/unreleased/22303-symbolic-in-tree.yml @@ -0,0 +1,4 @@ +--- +title: Fix symlink icon in project tree +merge_request: 9780 +author: mhasbini diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5954aea8041..485590316bd 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -204,7 +204,7 @@ module API expose :id, :name, :type, :path expose :mode do |obj, options| - filemode = obj.mode.to_s(8) + filemode = obj.mode filemode = "0" + filemode if filemode.length < 6 filemode end diff --git a/lib/gitlab/git/tree.rb b/lib/gitlab/git/tree.rb index f7450e8b58f..b722d8a9f56 100644 --- a/lib/gitlab/git/tree.rb +++ b/lib/gitlab/git/tree.rb @@ -33,7 +33,7 @@ module Gitlab root_id: root_tree.oid, name: entry[:name], type: entry[:type], - mode: entry[:filemode], + mode: entry[:filemode].to_s(8), path: path ? File.join(path, entry[:name]) : entry[:name], commit_id: sha, ) diff --git a/spec/lib/gitlab/git/tree_spec.rb b/spec/lib/gitlab/git/tree_spec.rb index 83d2ff8f9b3..82685712b5b 100644 --- a/spec/lib/gitlab/git/tree_spec.rb +++ b/spec/lib/gitlab/git/tree_spec.rb @@ -19,6 +19,7 @@ describe Gitlab::Git::Tree, seed_helper: true do it { expect(dir.commit_id).to eq(SeedRepo::Commit::ID) } it { expect(dir.name).to eq('encoding') } it { expect(dir.path).to eq('encoding') } + it { expect(dir.mode).to eq('40000') } context :subdir do let(:subdir) { Gitlab::Git::Tree.where(repository, SeedRepo::Commit::ID, 'files').first } -- cgit v1.2.1 From 8a71d40e60c799f969af0ed255e931b6c9907634 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 31 Mar 2017 16:59:46 -0700 Subject: Fix race condition where a namespace would be deleted before a project was deleted When deleting a user, the following sequence could happen: 1. Project `mygroup/myproject` is scheduled for deletion 2. The group `mygroup` is deleted 3. Sidekiq worker runs to delete `mygroup/myproject`, but the namespace and routes have been destroyed. Closes #30334 --- app/services/users/destroy_service.rb | 4 ++-- changelogs/unreleased/sh-fix-destroy-user-race.yml | 5 +++++ spec/services/users/destroy_spec.rb | 19 +++++++++++++++++-- 3 files changed, 24 insertions(+), 4 deletions(-) create mode 100644 changelogs/unreleased/sh-fix-destroy-user-race.yml diff --git a/app/services/users/destroy_service.rb b/app/services/users/destroy_service.rb index 833da5bc5d1..a3b32a71a64 100644 --- a/app/services/users/destroy_service.rb +++ b/app/services/users/destroy_service.rb @@ -20,10 +20,10 @@ module Users Groups::DestroyService.new(group, current_user).execute end - user.personal_projects.each do |project| + user.personal_projects.with_deleted.each do |project| # Skip repository removal because we remove directory with namespace # that contain all this repositories - ::Projects::DestroyService.new(project, current_user, skip_repo: true).async_execute + ::Projects::DestroyService.new(project, current_user, skip_repo: true).execute end move_issues_to_ghost_user(user) diff --git a/changelogs/unreleased/sh-fix-destroy-user-race.yml b/changelogs/unreleased/sh-fix-destroy-user-race.yml new file mode 100644 index 00000000000..4d650b51ada --- /dev/null +++ b/changelogs/unreleased/sh-fix-destroy-user-race.yml @@ -0,0 +1,5 @@ +--- +title: Fix race condition where a namespace would be deleted before a project was + deleted +merge_request: +author: diff --git a/spec/services/users/destroy_spec.rb b/spec/services/users/destroy_spec.rb index 9a28c03d968..66c61b7f8ff 100644 --- a/spec/services/users/destroy_spec.rb +++ b/spec/services/users/destroy_spec.rb @@ -17,13 +17,28 @@ describe Users::DestroyService, services: true do expect { Namespace.with_deleted.find(user.namespace.id) }.to raise_error(ActiveRecord::RecordNotFound) end - it 'will delete the project in the near future' do - expect_any_instance_of(Projects::DestroyService).to receive(:async_execute).once + it 'will delete the project' do + expect_any_instance_of(Projects::DestroyService).to receive(:execute).once service.execute(user) end end + context 'projects in pending_delete' do + before do + project.pending_delete = true + project.save + end + + it 'destroys a project in pending_delete' do + expect_any_instance_of(Projects::DestroyService).to receive(:execute).once + + service.execute(user) + + expect { Project.find(project.id) }.to raise_error(ActiveRecord::RecordNotFound) + end + end + context "a deleted user's issues" do let(:project) { create(:project) } -- cgit v1.2.1 From 6a2d022d1d578f8957736de2fb895069c24c072b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 31 Mar 2017 21:29:51 -0700 Subject: Delete users asynchronously --- app/controllers/registrations_controller.rb | 4 ++-- lib/api/users.rb | 2 +- spec/controllers/registrations_controller_spec.rb | 16 ++++++++++++++++ spec/requests/api/users_spec.rb | 10 +++++----- 4 files changed, 24 insertions(+), 8 deletions(-) diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb index a49a1f50a81..8109427a45f 100644 --- a/app/controllers/registrations_controller.rb +++ b/app/controllers/registrations_controller.rb @@ -25,12 +25,12 @@ class RegistrationsController < Devise::RegistrationsController end def destroy - Users::DestroyService.new(current_user).execute(current_user) + DeleteUserWorker.perform_async(current_user.id, current_user.id) respond_to do |format| format.html do session.try(:destroy) - redirect_to new_user_session_path, notice: "Account successfully removed." + redirect_to new_user_session_path, notice: "Account scheduled for removal." end end end diff --git a/lib/api/users.rb b/lib/api/users.rb index a4201fe6fed..530ca0b5235 100644 --- a/lib/api/users.rb +++ b/lib/api/users.rb @@ -293,7 +293,7 @@ module API user = User.find_by(id: params[:id]) not_found!('User') unless user - ::Users::DestroyService.new(current_user).execute(user) + DeleteUserWorker.perform_async(current_user.id, user.id) end desc 'Block a user. Available only for admins.' diff --git a/spec/controllers/registrations_controller_spec.rb b/spec/controllers/registrations_controller_spec.rb index 902911071c4..71dd9ef3eb4 100644 --- a/spec/controllers/registrations_controller_spec.rb +++ b/spec/controllers/registrations_controller_spec.rb @@ -68,4 +68,20 @@ describe RegistrationsController do end end end + + describe '#destroy' do + let(:user) { create(:user) } + + before do + sign_in(user) + end + + it 'schedules the user for destruction' do + expect(DeleteUserWorker).to receive(:perform_async).with(user.id, user.id) + + post(:destroy) + + expect(response.status).to eq(302) + end + end end diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb index 04e7837fd7a..f793c0db2f3 100644 --- a/spec/requests/api/users_spec.rb +++ b/spec/requests/api/users_spec.rb @@ -676,7 +676,7 @@ describe API::Users, api: true do before { admin } it "deletes user" do - delete api("/users/#{user.id}", admin) + Sidekiq::Testing.inline! { delete api("/users/#{user.id}", admin) } expect(response).to have_http_status(204) expect { User.find(user.id) }.to raise_error ActiveRecord::RecordNotFound @@ -684,23 +684,23 @@ describe API::Users, api: true do end it "does not delete for unauthenticated user" do - delete api("/users/#{user.id}") + Sidekiq::Testing.inline! { delete api("/users/#{user.id}") } expect(response).to have_http_status(401) end it "is not available for non admin users" do - delete api("/users/#{user.id}", user) + Sidekiq::Testing.inline! { delete api("/users/#{user.id}", user) } expect(response).to have_http_status(403) end it "returns 404 for non-existing user" do - delete api("/users/999999", admin) + Sidekiq::Testing.inline! { delete api("/users/999999", admin) } expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 User Not Found') end it "returns a 404 for invalid ID" do - delete api("/users/ASDF", admin) + Sidekiq::Testing.inline! { delete api("/users/ASDF", admin) } expect(response).to have_http_status(404) end -- cgit v1.2.1 From 38a108cac9b95860eef261c01588c9c5a4c5db5c Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 30 Mar 2017 21:47:22 -0700 Subject: Fix a few N+1 queries identified by Bullet See !10263 --- app/controllers/admin/abuse_reports_controller.rb | 1 + app/controllers/groups/group_members_controller.rb | 1 + app/controllers/projects/merge_requests_controller.rb | 1 + app/controllers/projects/milestones_controller.rb | 2 +- 4 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/controllers/admin/abuse_reports_controller.rb b/app/controllers/admin/abuse_reports_controller.rb index 5055c318a5f..dc9a6df5f75 100644 --- a/app/controllers/admin/abuse_reports_controller.rb +++ b/app/controllers/admin/abuse_reports_controller.rb @@ -1,6 +1,7 @@ class Admin::AbuseReportsController < Admin::ApplicationController def index @abuse_reports = AbuseReport.order(id: :desc).page(params[:page]) + @abuse_reports.includes(:reporter, :user) end def destroy diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index 0cbf3eb58a3..00c50f9d0ad 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -14,6 +14,7 @@ class Groups::GroupMembersController < Groups::ApplicationController @members = @members.search(params[:search]) if params[:search].present? @members = @members.sort(@sort) @members = @members.page(params[:page]).per(50) + @members.includes(:user) @requesters = AccessRequestsFinder.new(@group).execute(current_user) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 9621b30b251..37e3ac05916 100755 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -39,6 +39,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController @collection_type = "MergeRequest" @merge_requests = merge_requests_collection @merge_requests = @merge_requests.page(params[:page]) + @merge_requests = @merge_requests.includes(merge_request_diff: :merge_request) @issuable_meta_data = issuable_meta_data(@merge_requests, @collection_type) if @merge_requests.out_of_range? && @merge_requests.total_pages != 0 diff --git a/app/controllers/projects/milestones_controller.rb b/app/controllers/projects/milestones_controller.rb index 5922e686cd0..408c0c60cb0 100644 --- a/app/controllers/projects/milestones_controller.rb +++ b/app/controllers/projects/milestones_controller.rb @@ -21,9 +21,9 @@ class Projects::MilestonesController < Projects::ApplicationController @sort = params[:sort] || 'due_date_asc' @milestones = @milestones.sort(@sort) - @milestones = @milestones.includes(:project) respond_to do |format| format.html do + @milestones = @milestones.includes(:project) @milestones = @milestones.page(params[:page]) end format.json do -- cgit v1.2.1 From 6811adafd5e98e1c058f6493072a66d74494387f Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sat, 1 Apr 2017 12:40:59 -0700 Subject: Relax constraint on Wiki IDs, since subdirectories can contain spaces If a subdirectory contains spaces, GitLab would be unable to generate the route in the sidebar, resulting in an Error 500. Closes #30357 --- changelogs/unreleased/sh-relax-wiki-slug-constraint.yml | 4 ++++ config/routes/wiki.rb | 4 +--- 2 files changed, 5 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/sh-relax-wiki-slug-constraint.yml diff --git a/changelogs/unreleased/sh-relax-wiki-slug-constraint.yml b/changelogs/unreleased/sh-relax-wiki-slug-constraint.yml new file mode 100644 index 00000000000..08395b0d28c --- /dev/null +++ b/changelogs/unreleased/sh-relax-wiki-slug-constraint.yml @@ -0,0 +1,4 @@ +--- +title: Relax constraint on Wiki IDs, since subdirectories can contain spaces +merge_request: +author: diff --git a/config/routes/wiki.rb b/config/routes/wiki.rb index a6b3f5d4693..c2da84ff6f2 100644 --- a/config/routes/wiki.rb +++ b/config/routes/wiki.rb @@ -1,5 +1,3 @@ -WIKI_SLUG_ID = { id: /\S+/ }.freeze unless defined? WIKI_SLUG_ID - scope(controller: :wikis) do scope(path: 'wikis', as: :wikis) do get :git_access @@ -8,7 +6,7 @@ scope(controller: :wikis) do post '/', to: 'wikis#create' end - scope(path: 'wikis/*id', as: :wiki, constraints: WIKI_SLUG_ID, format: false) do + scope(path: 'wikis/*id', as: :wiki, format: false) do get :edit get :history post :preview_markdown -- cgit v1.2.1 From af0c08b6f92a0933e24414bea11344162cda6c43 Mon Sep 17 00:00:00 2001 From: mhasbini Date: Sun, 2 Apr 2017 18:54:19 +0300 Subject: Fix redirection after login when the referer have params --- app/controllers/sessions_controller.rb | 2 +- changelogs/unreleased/29669-redirect-referer-params.yml | 4 ++++ spec/controllers/sessions_controller_spec.rb | 16 ++++++++++++++++ 3 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/29669-redirect-referer-params.yml diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 7d81c96262f..d8561871098 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -79,7 +79,7 @@ class SessionsController < Devise::SessionsController if request.referer.present? && (params['redirect_to_referer'] == 'yes') referer_uri = URI(request.referer) if referer_uri.host == Gitlab.config.gitlab.host - referer_uri.path + referer_uri.request_uri else request.fullpath end diff --git a/changelogs/unreleased/29669-redirect-referer-params.yml b/changelogs/unreleased/29669-redirect-referer-params.yml new file mode 100644 index 00000000000..d8fc7f33049 --- /dev/null +++ b/changelogs/unreleased/29669-redirect-referer-params.yml @@ -0,0 +1,4 @@ +--- +title: Fix redirection after login when the referer have params +merge_request: +author: mhasbini diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index a06c29dd91a..9c16a7bc08b 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -211,4 +211,20 @@ describe SessionsController do end end end + + describe '#new' do + before do + @request.env['devise.mapping'] = Devise.mappings[:user] + end + + it 'redirects correctly for referer on same host with params' do + search_path = '/search?search=seed_project' + allow(controller.request).to receive(:referer). + and_return('http://%{host}%{path}' % { host: Gitlab.config.gitlab.host, path: search_path }) + + get(:new, redirect_to_referer: :yes) + + expect(controller.stored_location_for(:redirect)).to eq(search_path) + end + end end -- cgit v1.2.1 From 6bffef880df8768798781d049eb629be834f5cfe Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 2 Apr 2017 09:07:47 -0700 Subject: Add a wait_for_ajax call to ensure Todos page cleans up properly Potentially fixes intermittent failures such as https://gitlab.com/gitlab-org/gitlab-ce/builds/13484375 --- features/steps/dashboard/todos.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/features/steps/dashboard/todos.rb b/features/steps/dashboard/todos.rb index 7bd3c7ee653..3225e19995b 100644 --- a/features/steps/dashboard/todos.rb +++ b/features/steps/dashboard/todos.rb @@ -3,6 +3,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps include SharedPaths include SharedProject include SharedUser + include WaitForAjax step '"John Doe" is a developer of project "Shop"' do project.team << [john_doe, :developer] @@ -138,6 +139,8 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps step 'I should be directed to the corresponding page' do page.should have_css('.identifier', text: 'Merge Request !1') + # Merge request page loads and issues a number of Ajax requests + wait_for_ajax end def should_see_todo(position, title, body, state: :pending) -- cgit v1.2.1 From 2332bf34ac417bb8a0aaba9482379bcfadb6d26e Mon Sep 17 00:00:00 2001 From: mhasbini Date: Sun, 2 Apr 2017 20:48:58 +0300 Subject: Remove unnecessary ORDER BY clause when updating todos --- app/services/todo_service.rb | 2 +- changelogs/unreleased/todo-update-order.yml | 4 ++++ 2 files changed, 5 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/todo-update-order.yml diff --git a/app/services/todo_service.rb b/app/services/todo_service.rb index 2c56cb4c680..b6e88b0280f 100644 --- a/app/services/todo_service.rb +++ b/app/services/todo_service.rb @@ -204,7 +204,7 @@ class TodoService # Only update those that are not really on that state todos = todos.where.not(state: state) todos_ids = todos.pluck(:id) - todos.update_all(state: state) + todos.unscope(:order).update_all(state: state) current_user.update_todos_count_cache todos_ids end diff --git a/changelogs/unreleased/todo-update-order.yml b/changelogs/unreleased/todo-update-order.yml new file mode 100644 index 00000000000..2046b6df11e --- /dev/null +++ b/changelogs/unreleased/todo-update-order.yml @@ -0,0 +1,4 @@ +--- +title: Remove unnecessary ORDER BY clause when updating todos +merge_request: +author: mhasbini -- cgit v1.2.1 From 9c37630081d34518faf98f6579949d284eb627a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 3 Apr 2017 09:28:18 +0200 Subject: Fix a transient failure caused by FFaker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The error occurs when the Ffaker-generate title or description of issue3 contains git, e.g. fugit in this case. Signed-off-by: Rémy Coutable --- spec/finders/issues_finder_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/finders/issues_finder_spec.rb b/spec/finders/issues_finder_spec.rb index ee52dc65175..231fd85c464 100644 --- a/spec/finders/issues_finder_spec.rb +++ b/spec/finders/issues_finder_spec.rb @@ -9,7 +9,7 @@ describe IssuesFinder do let(:label) { create(:label, project: project2) } let(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone, title: 'gitlab') } let(:issue2) { create(:issue, author: user, assignee: user, project: project2, description: 'gitlab') } - let(:issue3) { create(:issue, author: user2, assignee: user2, project: project2) } + let(:issue3) { create(:issue, author: user2, assignee: user2, project: project2, title: 'tanuki', description: 'tanuki') } describe '#execute' do let(:closed_issue) { create(:issue, author: user2, assignee: user2, project: project2, state: 'closed') } -- cgit v1.2.1 From 76b2fa3eeb5f4f8b5ccd70e7307b10e30383c065 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Thu, 30 Mar 2017 21:21:18 +0100 Subject: Fixes method not replacing URL parameters correctly --- app/assets/javascripts/lib/utils/common_utils.js | 27 ++++++++++------- changelogs/unreleased/30264-fix-vue-pagination.yml | 5 ++++ spec/javascripts/lib/utils/common_utils_spec.js | 35 ++++++++++++++++++++++ 3 files changed, 57 insertions(+), 10 deletions(-) create mode 100644 changelogs/unreleased/30264-fix-vue-pagination.yml diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js index 4aad0128aef..46b80c04e20 100644 --- a/app/assets/javascripts/lib/utils/common_utils.js +++ b/app/assets/javascripts/lib/utils/common_utils.js @@ -263,7 +263,7 @@ }); /** - * Updates the search parameter of a URL given the parameter and values provided. + * Updates the search parameter of a URL given the parameter and value provided. * * If no search params are present we'll add it. * If param for page is already present, we'll update it @@ -278,17 +278,24 @@ let search; const locationSearch = window.location.search; - if (locationSearch.length === 0) { - search = `?${param}=${value}`; - } + if (locationSearch.length) { + const parameters = locationSearch.substring(1, locationSearch.length) + .split('&') + .reduce((acc, element) => { + const val = element.split('='); + acc[val[0]] = decodeURIComponent(val[1]); + return acc; + }, {}); - if (locationSearch.indexOf(param) !== -1) { - const regex = new RegExp(param + '=\\d'); - search = locationSearch.replace(regex, `${param}=${value}`); - } + parameters[param] = value; - if (locationSearch.length && locationSearch.indexOf(param) === -1) { - search = `${locationSearch}&${param}=${value}`; + const toString = Object.keys(parameters) + .map(val => `${val}=${encodeURIComponent(parameters[val])}`) + .join('&'); + + search = `?${toString}`; + } else { + search = `?${param}=${value}`; } return search; diff --git a/changelogs/unreleased/30264-fix-vue-pagination.yml b/changelogs/unreleased/30264-fix-vue-pagination.yml new file mode 100644 index 00000000000..d5846e52bcf --- /dev/null +++ b/changelogs/unreleased/30264-fix-vue-pagination.yml @@ -0,0 +1,5 @@ +--- +title: Fixes method not replacing URL parameters correctly and breaking pipelines + pagination +merge_request: +author: diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index 7cf39d37181..5c63e5024f5 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -261,5 +261,40 @@ require('~/lib/utils/common_utils'); }); }, 10000); }); + + describe('gl.utils.setParamInURL', () => { + it('should return the parameter', () => { + expect(gl.utils.setParamInURL('page', 156)).toBe('?page=156'); + expect(gl.utils.setParamInURL('page', '156')).toBe('?page=156'); + }); + + it('should update the existing parameter when its a number', () => { + window.history.pushState({}, null, '?page=15'); + + expect(gl.utils.setParamInURL('page', 16)).toBe('?page=16'); + expect(gl.utils.setParamInURL('page', '16')).toBe('?page=16'); + expect(gl.utils.setParamInURL('page', true)).toBe('?page=true'); + }); + + it('should update the existing parameter when its a string', () => { + window.history.pushState({}, null, '?scope=all'); + + expect(gl.utils.setParamInURL('scope', 'finished')).toBe('?scope=finished'); + }); + + it('should update the existing parameter when more than one parameter exists', () => { + window.history.pushState({}, null, '?scope=all&page=15'); + + expect(gl.utils.setParamInURL('scope', 'finished')).toBe('?scope=finished&page=15'); + }); + + it('should add a new parameter to the end of the existing ones', () => { + window.history.pushState({}, null, '?scope=all'); + + expect(gl.utils.setParamInURL('page', 16)).toBe('?scope=all&page=16'); + expect(gl.utils.setParamInURL('page', '16')).toBe('?scope=all&page=16'); + expect(gl.utils.setParamInURL('page', true)).toBe('?scope=all&page=true'); + }); + }); }); })(); -- cgit v1.2.1 From 20b54b133c1714e6a2b0afbbf4c1770991e8e344 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 31 Mar 2017 10:11:11 +0100 Subject: Clean history state after each test --- spec/javascripts/lib/utils/common_utils_spec.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index 5c63e5024f5..d6dcdbf1d12 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -263,7 +263,12 @@ require('~/lib/utils/common_utils'); }); describe('gl.utils.setParamInURL', () => { + afterEach(() => { + window.history.pushState({}, null, ''); + }); + it('should return the parameter', () => { + window.history.pushState({}, null, ''); expect(gl.utils.setParamInURL('page', 156)).toBe('?page=156'); expect(gl.utils.setParamInURL('page', '156')).toBe('?page=156'); }); -- cgit v1.2.1 From c4d4f4d8577ec3d8e728bc56b3811db33094dd61 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 31 Mar 2017 11:22:31 +0100 Subject: Clean history after every test that changes history --- spec/javascripts/environments/environment_spec.js | 4 ++++ spec/javascripts/lib/utils/common_utils_spec.js | 8 ++++++++ spec/javascripts/vue_shared/components/table_pagination_spec.js | 4 ++++ 3 files changed, 16 insertions(+) diff --git a/spec/javascripts/environments/environment_spec.js b/spec/javascripts/environments/environment_spec.js index 9601575577e..9bcf617fcd8 100644 --- a/spec/javascripts/environments/environment_spec.js +++ b/spec/javascripts/environments/environment_spec.js @@ -91,6 +91,10 @@ describe('Environment', () => { }); describe('pagination', () => { + afterEach(() => { + window.history.pushState({}, null, ''); + }); + it('should render pagination', (done) => { setTimeout(() => { expect( diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index d6dcdbf1d12..0fa4c51560b 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -46,6 +46,10 @@ require('~/lib/utils/common_utils'); spyOn(window.document, 'getElementById').and.callThrough(); }); + afterEach(() => { + window.history.pushState({}, null, ''); + }); + function expectGetElementIdToHaveBeenCalledWith(elementId) { expect(window.document.getElementById).toHaveBeenCalledWith(elementId); } @@ -80,6 +84,10 @@ require('~/lib/utils/common_utils'); window.history.pushState({}, null, '?scope=all&p=2'); }); + afterEach(() => { + window.history.pushState({}, null, ''); + }); + it('should return valid parameter', () => { const value = gl.utils.getParameterByName('scope'); expect(value).toBe('all'); diff --git a/spec/javascripts/vue_shared/components/table_pagination_spec.js b/spec/javascripts/vue_shared/components/table_pagination_spec.js index d1640ffed99..96038718191 100644 --- a/spec/javascripts/vue_shared/components/table_pagination_spec.js +++ b/spec/javascripts/vue_shared/components/table_pagination_spec.js @@ -124,6 +124,10 @@ describe('Pagination component', () => { }); describe('paramHelper', () => { + afterEach(() => { + window.history.pushState({}, null, ''); + }); + it('can parse url parameters correctly', () => { window.history.pushState({}, null, '?scope=all&p=2'); -- cgit v1.2.1 From 670e1d7477b7eb9a6c9e495830ede248674ba55a Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Fri, 31 Mar 2017 15:20:22 +0100 Subject: Change order of specs --- spec/javascripts/lib/utils/common_utils_spec.js | 83 +++++++++++++------------ 1 file changed, 42 insertions(+), 41 deletions(-) diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js index 0fa4c51560b..5a93d479c1f 100644 --- a/spec/javascripts/lib/utils/common_utils_spec.js +++ b/spec/javascripts/lib/utils/common_utils_spec.js @@ -79,13 +79,54 @@ require('~/lib/utils/common_utils'); }); }); + describe('gl.utils.setParamInURL', () => { + afterEach(() => { + window.history.pushState({}, null, ''); + }); + + it('should return the parameter', () => { + window.history.replaceState({}, null, ''); + + expect(gl.utils.setParamInURL('page', 156)).toBe('?page=156'); + expect(gl.utils.setParamInURL('page', '156')).toBe('?page=156'); + }); + + it('should update the existing parameter when its a number', () => { + window.history.pushState({}, null, '?page=15'); + + expect(gl.utils.setParamInURL('page', 16)).toBe('?page=16'); + expect(gl.utils.setParamInURL('page', '16')).toBe('?page=16'); + expect(gl.utils.setParamInURL('page', true)).toBe('?page=true'); + }); + + it('should update the existing parameter when its a string', () => { + window.history.pushState({}, null, '?scope=all'); + + expect(gl.utils.setParamInURL('scope', 'finished')).toBe('?scope=finished'); + }); + + it('should update the existing parameter when more than one parameter exists', () => { + window.history.pushState({}, null, '?scope=all&page=15'); + + expect(gl.utils.setParamInURL('scope', 'finished')).toBe('?scope=finished&page=15'); + }); + + it('should add a new parameter to the end of the existing ones', () => { + window.history.pushState({}, null, '?scope=all'); + + expect(gl.utils.setParamInURL('page', 16)).toBe('?scope=all&page=16'); + expect(gl.utils.setParamInURL('page', '16')).toBe('?scope=all&page=16'); + expect(gl.utils.setParamInURL('page', true)).toBe('?scope=all&page=true'); + }); + }); + describe('gl.utils.getParameterByName', () => { beforeEach(() => { window.history.pushState({}, null, '?scope=all&p=2'); }); afterEach(() => { - window.history.pushState({}, null, ''); + window.history.replaceState({}, null, null); }); it('should return valid parameter', () => { @@ -269,45 +310,5 @@ require('~/lib/utils/common_utils'); }); }, 10000); }); - - describe('gl.utils.setParamInURL', () => { - afterEach(() => { - window.history.pushState({}, null, ''); - }); - - it('should return the parameter', () => { - window.history.pushState({}, null, ''); - expect(gl.utils.setParamInURL('page', 156)).toBe('?page=156'); - expect(gl.utils.setParamInURL('page', '156')).toBe('?page=156'); - }); - - it('should update the existing parameter when its a number', () => { - window.history.pushState({}, null, '?page=15'); - - expect(gl.utils.setParamInURL('page', 16)).toBe('?page=16'); - expect(gl.utils.setParamInURL('page', '16')).toBe('?page=16'); - expect(gl.utils.setParamInURL('page', true)).toBe('?page=true'); - }); - - it('should update the existing parameter when its a string', () => { - window.history.pushState({}, null, '?scope=all'); - - expect(gl.utils.setParamInURL('scope', 'finished')).toBe('?scope=finished'); - }); - - it('should update the existing parameter when more than one parameter exists', () => { - window.history.pushState({}, null, '?scope=all&page=15'); - - expect(gl.utils.setParamInURL('scope', 'finished')).toBe('?scope=finished&page=15'); - }); - - it('should add a new parameter to the end of the existing ones', () => { - window.history.pushState({}, null, '?scope=all'); - - expect(gl.utils.setParamInURL('page', 16)).toBe('?scope=all&page=16'); - expect(gl.utils.setParamInURL('page', '16')).toBe('?scope=all&page=16'); - expect(gl.utils.setParamInURL('page', true)).toBe('?scope=all&page=true'); - }); - }); }); })(); -- cgit v1.2.1 From a3e65ef021c3c1ba90adf58654c2856001cf009a Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Thu, 16 Mar 2017 10:42:58 +0100 Subject: Fix GitHub importer for PRs of deleted forked repositories --- lib/gitlab/github_import/branch_formatter.rb | 4 ++++ lib/gitlab/github_import/pull_request_formatter.rb | 20 +++++++++++++---- .../github_import/pull_request_formatter_spec.rb | 25 ++++++++++++++++++++++ 3 files changed, 45 insertions(+), 4 deletions(-) diff --git a/lib/gitlab/github_import/branch_formatter.rb b/lib/gitlab/github_import/branch_formatter.rb index 5d29e698b27..48bed8fc296 100644 --- a/lib/gitlab/github_import/branch_formatter.rb +++ b/lib/gitlab/github_import/branch_formatter.rb @@ -11,6 +11,10 @@ module Gitlab sha.present? && ref.present? end + def user + raw_data.user.login + end + private def branch_exists? diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index add7236e339..3326fe4bf18 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -1,8 +1,8 @@ module Gitlab module GithubImport class PullRequestFormatter < IssuableFormatter - delegate :exists?, :project, :ref, :repo, :sha, to: :source_branch, prefix: true - delegate :exists?, :project, :ref, :repo, :sha, to: :target_branch, prefix: true + delegate :user, :exists?, :project, :ref, :repo, :sha, to: :source_branch, prefix: true + delegate :user, :exists?, :project, :ref, :repo, :sha, to: :target_branch, prefix: true def attributes { @@ -39,13 +39,23 @@ module Gitlab def source_branch_name @source_branch_name ||= begin if cross_project? - "pull/#{number}/#{source_branch_repo.full_name}/#{source_branch_ref}" + if source_branch_repo + "pull/#{number}/#{source_branch_repo.full_name}/#{source_branch_ref}" + else + "pull/#{number}/#{source_branch_user}/#{source_branch_ref}" + end else source_branch_exists? ? source_branch_ref : "pull/#{number}/#{source_branch_ref}" end end end + def source_branch_exists? + return false if cross_project? + + source_branch.exists? + end + def target_branch @target_branch ||= BranchFormatter.new(project, raw_data.base) end @@ -57,7 +67,9 @@ module Gitlab end def cross_project? - source_branch.repo.id != target_branch.repo.id + return true if source_branch_repo.nil? + + source_branch_repo.id != target_branch_repo.id end def opened? diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index 44423917944..b94330036ab 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -13,6 +13,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha) } let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') } let(:forked_branch) { double(ref: 'master', repo: forked_source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') } + let(:branch_deleted_repo) { double(ref: 'master', repo: nil, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) } let(:octocat) { double(id: 123456, login: 'octocat', email: 'octocat@example.com') } let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') } let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') } @@ -215,6 +216,14 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do expect(pull_request.source_branch_name).to eq 'pull/1347/company/otherproject/master' end end + + context 'when source branch is from a deleted fork' do + let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) } + + it 'prefixes branch name with pull request number and user login to avoid collision' do + expect(pull_request.source_branch_name).to eq 'pull/1347/octocat/master' + end + end end shared_examples 'Gitlab::GithubImport::PullRequestFormatter#target_branch_name' do @@ -290,6 +299,14 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do end end + context 'when source repository does not exist anymore' do + let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) } + + it 'returns true' do + expect(pull_request.cross_project?).to eq true + end + end + context 'when source and target repositories are the same' do let(:raw_data) { double(base_data.merge(head: source_branch)) } @@ -299,6 +316,14 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do end end + describe '#source_branch_exists?' do + let(:raw_data) { double(base_data.merge(head: forked_branch)) } + + it 'returns false when is a cross_project' do + expect(pull_request.source_branch_exists?).to eq false + end + end + describe '#url' do let(:raw_data) { double(base_data) } -- cgit v1.2.1 From 59588ebac012e0bb300d1d7660f5a7360ace5e4e Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Thu, 16 Mar 2017 18:55:50 -0300 Subject: Prefixes source branch name with short SHA to avoid collision --- lib/gitlab/github_import/branch_formatter.rb | 4 ++++ lib/gitlab/github_import/pull_request_formatter.rb | 22 +++++++++++++--------- .../github_import/pull_request_formatter_spec.rb | 20 +++++++++++--------- 3 files changed, 28 insertions(+), 18 deletions(-) diff --git a/lib/gitlab/github_import/branch_formatter.rb b/lib/gitlab/github_import/branch_formatter.rb index 48bed8fc296..18200809637 100644 --- a/lib/gitlab/github_import/branch_formatter.rb +++ b/lib/gitlab/github_import/branch_formatter.rb @@ -15,6 +15,10 @@ module Gitlab raw_data.user.login end + def short_sha(length = 7) + sha.to_s[0..length] + end + private def branch_exists? diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index 3326fe4bf18..bc24c6de120 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -1,8 +1,8 @@ module Gitlab module GithubImport class PullRequestFormatter < IssuableFormatter - delegate :user, :exists?, :project, :ref, :repo, :sha, to: :source_branch, prefix: true - delegate :user, :exists?, :project, :ref, :repo, :sha, to: :target_branch, prefix: true + delegate :user, :project, :ref, :repo, :sha, to: :source_branch, prefix: true + delegate :user, :exists?, :project, :ref, :repo, :sha, :short_sha, to: :target_branch, prefix: true def attributes { @@ -39,17 +39,17 @@ module Gitlab def source_branch_name @source_branch_name ||= begin if cross_project? - if source_branch_repo - "pull/#{number}/#{source_branch_repo.full_name}/#{source_branch_ref}" - else - "pull/#{number}/#{source_branch_user}/#{source_branch_ref}" - end + source_branch_name_prefixed else - source_branch_exists? ? source_branch_ref : "pull/#{number}/#{source_branch_ref}" + source_branch_exists? ? source_branch_ref : source_branch_name_prefixed end end end + def source_branch_name_prefixed + "gh-#{target_branch_short_sha}/#{number}" + end + def source_branch_exists? return false if cross_project? @@ -62,10 +62,14 @@ module Gitlab def target_branch_name @target_branch_name ||= begin - target_branch_exists? ? target_branch_ref : "pull/#{number}/#{target_branch_ref}" + target_branch_exists? ? target_branch_ref : target_branch_name_prefixed end end + def target_branch_name_prefixed + "gl-#{target_branch_short_sha}/#{number}" + end + def cross_project? return true if source_branch_repo.nil? diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index b94330036ab..62a13307dfe 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -4,7 +4,9 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:client) { double } let(:project) { create(:project, :repository) } let(:source_sha) { create(:commit, project: project).id } - let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id } + let(:target_commit) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit) } + let(:target_sha) { target_commit.id } + let(:target_short_sha) { target_commit.id.to_s[0..7] } let(:repository) { double(id: 1, fork: false) } let(:source_repo) { repository } let(:source_branch) { double(ref: 'branch-merged', repo: source_repo, sha: source_sha) } @@ -204,24 +206,24 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do context 'when source branch does not exist' do let(:raw_data) { double(base_data.merge(head: removed_branch)) } - it 'prefixes branch name with pull request number' do - expect(pull_request.source_branch_name).to eq 'pull/1347/removed-branch' + it 'prefixes branch name with to avoid collision' do + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347" end end context 'when source branch is from a fork' do let(:raw_data) { double(base_data.merge(head: forked_branch)) } - it 'prefixes branch name with pull request number and project with namespace to avoid collision' do - expect(pull_request.source_branch_name).to eq 'pull/1347/company/otherproject/master' + it 'prefixes branch name with to avoid collision' do + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347" end end context 'when source branch is from a deleted fork' do let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) } - it 'prefixes branch name with pull request number and user login to avoid collision' do - expect(pull_request.source_branch_name).to eq 'pull/1347/octocat/master' + it 'prefixes branch name with to avoid collision' do + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347" end end end @@ -238,8 +240,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do context 'when target branch does not exist' do let(:raw_data) { double(base_data.merge(base: removed_branch)) } - it 'prefixes branch name with pull request number' do - expect(pull_request.target_branch_name).to eq 'pull/1347/removed-branch' + it 'prefixes branch name with to avoid collision' do + expect(pull_request.target_branch_name).to eq 'gl-2e5d3239/1347' end end end -- cgit v1.2.1 From 291bd873d38094d9bc9b7482566f6dee40096992 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 17 Mar 2017 03:48:06 +0100 Subject: One more change to the branch names to preserve metadata --- lib/gitlab/github_import/pull_request_formatter.rb | 4 ++-- .../gitlab/github_import/pull_request_formatter_spec.rb | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index bc24c6de120..82be1639578 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -47,7 +47,7 @@ module Gitlab end def source_branch_name_prefixed - "gh-#{target_branch_short_sha}/#{number}" + "gh-#{target_branch_short_sha}/#{number}/#{source_branch_user}/#{source_branch_ref}" end def source_branch_exists? @@ -67,7 +67,7 @@ module Gitlab end def target_branch_name_prefixed - "gl-#{target_branch_short_sha}/#{number}" + "gl-#{target_branch_short_sha}/#{number}/#{target_branch_user}/#{target_branch_ref}" end def cross_project? diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index 62a13307dfe..3a4e7bcb903 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -12,9 +12,9 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:source_branch) { double(ref: 'branch-merged', repo: source_repo, sha: source_sha) } let(:forked_source_repo) { double(id: 2, fork: true, name: 'otherproject', full_name: 'company/otherproject') } let(:target_repo) { repository } - let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha) } - let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') } - let(:forked_branch) { double(ref: 'master', repo: forked_source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b') } + let(:target_branch) { double(ref: 'master', repo: target_repo, sha: target_sha, user: octocat) } + let(:removed_branch) { double(ref: 'removed-branch', repo: source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) } + let(:forked_branch) { double(ref: 'master', repo: forked_source_repo, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) } let(:branch_deleted_repo) { double(ref: 'master', repo: nil, sha: '2e5d3239642f9161dcbbc4b70a211a68e5e45e2b', user: octocat) } let(:octocat) { double(id: 123456, login: 'octocat', email: 'octocat@example.com') } let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') } @@ -207,7 +207,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:raw_data) { double(base_data.merge(head: removed_branch)) } it 'prefixes branch name with to avoid collision' do - expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347" + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/removed-branch" end end @@ -215,7 +215,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:raw_data) { double(base_data.merge(head: forked_branch)) } it 'prefixes branch name with to avoid collision' do - expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347" + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/master" end end @@ -223,7 +223,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) } it 'prefixes branch name with to avoid collision' do - expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347" + expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/master" end end end @@ -241,7 +241,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do let(:raw_data) { double(base_data.merge(base: removed_branch)) } it 'prefixes branch name with to avoid collision' do - expect(pull_request.target_branch_name).to eq 'gl-2e5d3239/1347' + expect(pull_request.target_branch_name).to eq 'gl-2e5d3239/1347/octocat/removed-branch' end end end -- cgit v1.2.1 From 148068a8dfc9816c918839751450ed404698939c Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 17 Mar 2017 08:57:44 +0100 Subject: Fix specs --- spec/lib/gitlab/github_import/importer_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/lib/gitlab/github_import/importer_spec.rb b/spec/lib/gitlab/github_import/importer_spec.rb index 8b867fbe322..9d5e20841b5 100644 --- a/spec/lib/gitlab/github_import/importer_spec.rb +++ b/spec/lib/gitlab/github_import/importer_spec.rb @@ -215,9 +215,9 @@ describe Gitlab::GithubImport::Importer, lib: true do let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') } let(:repository) { double(id: 1, fork: false) } let(:source_sha) { create(:commit, project: project).id } - let(:source_branch) { double(ref: 'branch-merged', repo: repository, sha: source_sha) } + let(:source_branch) { double(ref: 'branch-merged', repo: repository, sha: source_sha, user: octocat) } let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id } - let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha) } + let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha, user: octocat) } let(:pull_request) do double( number: 1347, -- cgit v1.2.1 From 0d2f696c3c972ac74177c668ccb1e0a25acf5eab Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Fri, 17 Mar 2017 09:09:54 +0100 Subject: Changelog --- changelogs/unreleased/29541-fix-github-importer-deleted-fork.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/29541-fix-github-importer-deleted-fork.yml diff --git a/changelogs/unreleased/29541-fix-github-importer-deleted-fork.yml b/changelogs/unreleased/29541-fix-github-importer-deleted-fork.yml new file mode 100644 index 00000000000..fcb5798a619 --- /dev/null +++ b/changelogs/unreleased/29541-fix-github-importer-deleted-fork.yml @@ -0,0 +1,4 @@ +--- +title: Fix GitHub Importer for PRs of deleted forked repositories +merge_request: 9992 +author: -- cgit v1.2.1 From c8273ba85898877e47dbb28d0a4a5664f3bf4196 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 22 Mar 2017 14:45:35 -0300 Subject: Fix BrachFormatter for removed users --- lib/gitlab/github_import/branch_formatter.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/gitlab/github_import/branch_formatter.rb b/lib/gitlab/github_import/branch_formatter.rb index 18200809637..81d8715b854 100644 --- a/lib/gitlab/github_import/branch_formatter.rb +++ b/lib/gitlab/github_import/branch_formatter.rb @@ -12,7 +12,7 @@ module Gitlab end def user - raw_data.user.login + raw_data.user&.login || 'unknown' end def short_sha(length = 7) -- cgit v1.2.1 From 3c75ee2823e813e22d357a1f2a7ce21e23be77fb Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Sun, 26 Mar 2017 03:16:27 +0200 Subject: Minor refactor --- lib/gitlab/github_import/branch_formatter.rb | 4 ++-- lib/gitlab/github_import/pull_request_formatter.rb | 15 +++++---------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/lib/gitlab/github_import/branch_formatter.rb b/lib/gitlab/github_import/branch_formatter.rb index 81d8715b854..8aa885fb811 100644 --- a/lib/gitlab/github_import/branch_formatter.rb +++ b/lib/gitlab/github_import/branch_formatter.rb @@ -15,8 +15,8 @@ module Gitlab raw_data.user&.login || 'unknown' end - def short_sha(length = 7) - sha.to_s[0..length] + def short_sha + Commit.truncate_sha(sha) end private diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index 82be1639578..38660a7ccca 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -37,13 +37,12 @@ module Gitlab end def source_branch_name - @source_branch_name ||= begin - if cross_project? + @source_branch_name ||= + if cross_project? || !source_branch_exists? source_branch_name_prefixed else - source_branch_exists? ? source_branch_ref : source_branch_name_prefixed + source_branch_ref end - end end def source_branch_name_prefixed @@ -51,9 +50,7 @@ module Gitlab end def source_branch_exists? - return false if cross_project? - - source_branch.exists? + !cross_project? && source_branch.exists? end def target_branch @@ -61,9 +58,7 @@ module Gitlab end def target_branch_name - @target_branch_name ||= begin - target_branch_exists? ? target_branch_ref : target_branch_name_prefixed - end + @target_branch_name ||= target_branch_exists? ? target_branch_ref : target_branch_name_prefixed end def target_branch_name_prefixed -- cgit v1.2.1 From 3ac5e9ebbf0d7db8c4960754296f33f06986597d Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Wed, 29 Mar 2017 21:46:29 +0200 Subject: Improve specs examples --- spec/lib/gitlab/github_import/pull_request_formatter_spec.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index 3a4e7bcb903..14cb8ac0679 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -206,7 +206,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do context 'when source branch does not exist' do let(:raw_data) { double(base_data.merge(head: removed_branch)) } - it 'prefixes branch name with to avoid collision' do + it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/removed-branch" end end @@ -214,7 +214,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do context 'when source branch is from a fork' do let(:raw_data) { double(base_data.merge(head: forked_branch)) } - it 'prefixes branch name with to avoid collision' do + it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/master" end end @@ -222,7 +222,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do context 'when source branch is from a deleted fork' do let(:raw_data) { double(base_data.merge(head: branch_deleted_repo)) } - it 'prefixes branch name with to avoid collision' do + it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do expect(pull_request.source_branch_name).to eq "gh-#{target_short_sha}/1347/octocat/master" end end -- cgit v1.2.1 From d5f340943e76f952fb566509ea7bc9aefcedf6fd Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Thu, 30 Mar 2017 10:50:29 +0200 Subject: Fix specs --- spec/lib/gitlab/github_import/pull_request_formatter_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index 14cb8ac0679..9b9f7e4d34e 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -240,7 +240,7 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do context 'when target branch does not exist' do let(:raw_data) { double(base_data.merge(base: removed_branch)) } - it 'prefixes branch name with to avoid collision' do + it 'prefixes branch name with gh-:short_sha/:number/:user pattern to avoid collision' do expect(pull_request.target_branch_name).to eq 'gl-2e5d3239/1347/octocat/removed-branch' end end -- cgit v1.2.1 From 88812ce2bf689c65cfd72ff7f6f3537a5b19aafb Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 31 Mar 2017 14:23:42 +0200 Subject: Use gitaly 0.5.0 --- Gemfile | 2 +- Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Gemfile b/Gemfile index 562ee2d1b9e..0c6bb805b98 100644 --- a/Gemfile +++ b/Gemfile @@ -352,4 +352,4 @@ gem 'vmstat', '~> 2.3.0' gem 'sys-filesystem', '~> 1.1.6' # Gitaly GRPC client -gem 'gitaly', '~> 0.3.0' +gem 'gitaly', '~> 0.5.0' diff --git a/Gemfile.lock b/Gemfile.lock index 8382de2b7a0..304fc9f2bb3 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -253,7 +253,7 @@ GEM json get_process_mem (0.2.0) gherkin-ruby (0.3.2) - gitaly (0.3.0) + gitaly (0.5.0) google-protobuf (~> 3.1) grpc (~> 1.0) github-linguist (4.7.6) @@ -899,7 +899,7 @@ DEPENDENCIES fuubar (~> 2.0.0) gemnasium-gitlab-service (~> 0.2) gemojione (~> 3.0) - gitaly (~> 0.3.0) + gitaly (~> 0.5.0) github-linguist (~> 4.7.0) gitlab-flowdock-git-hook (~> 1.0.1) gitlab-markup (~> 1.5.1) -- cgit v1.2.1 From 13487809c7fd9994f14babf3e189b434f61692ca Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Thu, 30 Mar 2017 18:48:22 +0200 Subject: Pass Gitaly Repository messages to workhorse --- lib/gitlab/workhorse.rb | 12 ++++++++++-- spec/lib/gitlab/workhorse_spec.rb | 8 +++++++- 2 files changed, 17 insertions(+), 3 deletions(-) diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index 6fe85af3c30..d1131ad65e0 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -17,14 +17,22 @@ module Gitlab class << self def git_http_ok(repository, user) + repo_path = repository.path_to_repo params = { GL_ID: Gitlab::GlId.gl_id(user), - RepoPath: repository.path_to_repo, + RepoPath: repo_path, } if Gitlab.config.gitaly.enabled - address = Gitlab::GitalyClient.get_address(repository.project.repository_storage) + storage = repository.project.repository_storage + address = Gitlab::GitalyClient.get_address(storage) params[:GitalySocketPath] = URI(address).path + # TODO: use GitalyClient code to assemble the Repository message + params[:Repository] = Gitaly::Repository.new( + path: repo_path, + storage_name: storage, + relative_path: Gitlab::RepoPath.strip_storage_path(repo_path), + ).to_h end params diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index 535c96eeee9..cb7c810124e 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -179,10 +179,11 @@ describe Gitlab::Workhorse, lib: true do describe '.git_http_ok' do let(:user) { create(:user) } + let(:repo_path) { repository.path_to_repo } subject { described_class.git_http_ok(repository, user) } - it { expect(subject).to eq({ GL_ID: "user-#{user.id}", RepoPath: repository.path_to_repo }) } + it { expect(subject).to eq({ GL_ID: "user-#{user.id}", RepoPath: repo_path }) } context 'when Gitaly is enabled' do before do @@ -192,6 +193,11 @@ describe Gitlab::Workhorse, lib: true do it 'includes Gitaly params in the returned value' do gitaly_socket_path = URI(Gitlab::GitalyClient.get_address('default')).path expect(subject).to include({ GitalySocketPath: gitaly_socket_path }) + expect(subject[:Repository]).to include({ + path: repo_path, + storage_name: 'default', + relative_path: project.full_path + '.git', + }) end end end -- cgit v1.2.1 From 2fa10f57c2cde8d77413c18078a445011f8bd015 Mon Sep 17 00:00:00 2001 From: Mustafa YILDIRIM Date: Mon, 3 Apr 2017 12:19:11 +0000 Subject: fix spelling CI_REPOSITORY_URL (line:355) gitab-ci-token to gitlab-ci-token. --- doc/ci/variables/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index b35caf672a8..53c29a4fd98 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -352,7 +352,7 @@ Example values: export CI_JOB_ID="50" export CI_COMMIT_SHA="1ecfd275763eff1d6b4844ea3168962458c9f27a" export CI_COMMIT_REF_NAME="master" -export CI_REPOSITORY_URL="https://gitab-ci-token:abcde-1234ABCD5678ef@example.com/gitlab-org/gitlab-ce.git" +export CI_REPOSITORY_URL="https://gitlab-ci-token:abcde-1234ABCD5678ef@example.com/gitlab-org/gitlab-ce.git" export CI_COMMIT_TAG="1.0.0" export CI_JOB_NAME="spec:other" export CI_JOB_STAGE="test" -- cgit v1.2.1 From a1805cbcd55a28658049b42c12a87a50ab9ab977 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 30 Mar 2017 12:29:52 +0100 Subject: Quiet pipeline emails 1. Never send a pipeline email to anyone other than the user who created the pipeline. 2. Only send pipeline success emails to people with the custom notification setting for enabled. Watchers and participants will never receive this. 3. When custom settings are unset (for new settings and legacy ones), act as if failed_pipeline is set. --- app/models/ci/pipeline.rb | 5 - app/models/notification_setting.rb | 17 +- app/services/notification_recipient_service.rb | 42 ++++- app/services/notification_service.rb | 4 +- .../notifications/_custom_notifications.html.haml | 2 +- changelogs/unreleased/quiet-pipelines.yml | 5 + doc/workflow/notifications.md | 7 +- spec/factories/ci/pipelines.rb | 4 + spec/models/ci/pipeline_spec.rb | 7 +- spec/services/notification_service_spec.rb | 190 +++++++++++++++++---- spec/workers/pipeline_notification_worker_spec.rb | 126 +------------- 11 files changed, 237 insertions(+), 172 deletions(-) create mode 100644 changelogs/unreleased/quiet-pipelines.yml diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index ad7e0b23ff4..49dec770096 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -164,11 +164,6 @@ module Ci builds.latest.with_artifacts_not_expired.includes(project: [:namespace]) end - # For now the only user who participates is the user who triggered - def participants(_current_user = nil) - Array(user) - end - def valid_commit_sha if self.sha == Gitlab::Git::BLANK_SHA self.errors.add(:sha, " cant be 00000000 (branch removal)") diff --git a/app/models/notification_setting.rb b/app/models/notification_setting.rb index 52577bd52ea..e4726e62e93 100644 --- a/app/models/notification_setting.rb +++ b/app/models/notification_setting.rb @@ -60,16 +60,25 @@ class NotificationSetting < ActiveRecord::Base def set_events return if custom? - EMAIL_EVENTS.each do |event| - events[event] = false - end + self.events = {} end # Validates store accessors values as boolean # It is a text field so it does not cast correct boolean values in JSON def events_to_boolean EMAIL_EVENTS.each do |event| - events[event] = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(events[event]) + bool = ActiveRecord::ConnectionAdapters::Column::TRUE_VALUES.include?(public_send(event)) + + events[event] = bool end end + + # Allow people to receive failed pipeline notifications if they already have + # custom notifications enabled, as these are more like mentions than the other + # custom settings. + def failed_pipeline + bool = super + + bool.nil? || bool + end end diff --git a/app/services/notification_recipient_service.rb b/app/services/notification_recipient_service.rb index 940e850600f..8bb995158de 100644 --- a/app/services/notification_recipient_service.rb +++ b/app/services/notification_recipient_service.rb @@ -3,7 +3,7 @@ # class NotificationRecipientService attr_reader :project - + def initialize(project) @project = project end @@ -12,11 +12,7 @@ class NotificationRecipientService custom_action = build_custom_key(action, target) recipients = target.participants(current_user) - - unless NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action) - recipients = add_project_watchers(recipients) - end - + recipients = add_project_watchers(recipients) recipients = add_custom_notifications(recipients, custom_action) recipients = reject_mention_users(recipients) @@ -43,6 +39,28 @@ class NotificationRecipientService recipients.uniq end + def build_pipeline_recipients(target, current_user, action:) + return [] unless current_user + + custom_action = + case action.to_s + when 'failed' + :failed_pipeline + when 'success' + :success_pipeline + end + + notification_setting = notification_setting_for_user_project(current_user, target.project) + + return [] if notification_setting.mention? || notification_setting.disabled? + + return [] if notification_setting.custom? && !notification_setting.public_send(custom_action) + + return [] if (notification_setting.watch? || notification_setting.participating?) && NotificationSetting::EXCLUDED_WATCHER_EVENTS.include?(custom_action) + + reject_users_without_access([current_user], target) + end + def build_relabeled_recipients(target, current_user, labels:) recipients = add_labels_subscribers([], target, labels: labels) recipients = reject_unsubscribed_users(recipients, target) @@ -290,4 +308,16 @@ class NotificationRecipientService def build_custom_key(action, object) "#{action}_#{object.class.model_name.name.underscore}".to_sym end + + def notification_setting_for_user_project(user, project) + project_setting = user.notification_settings_for(project) + + return project_setting unless project_setting.global? + + group_setting = user.notification_settings_for(project.group) + + return group_setting unless group_setting.global? + + user.global_notification_setting + end end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 2c6f849259e..6b186263bd1 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -278,11 +278,11 @@ class NotificationService return unless mailer.respond_to?(email_template) - recipients ||= NotificationRecipientService.new(pipeline.project).build_recipients( + recipients ||= NotificationRecipientService.new(pipeline.project).build_pipeline_recipients( pipeline, pipeline.user, action: pipeline.status, - skip_current_user: false).map(&:notification_email) + ).map(&:notification_email) if recipients.any? mailer.public_send(email_template, pipeline, recipients).deliver_later diff --git a/app/views/shared/notifications/_custom_notifications.html.haml b/app/views/shared/notifications/_custom_notifications.html.haml index a736bfd91e2..708adbc38f1 100644 --- a/app/views/shared/notifications/_custom_notifications.html.haml +++ b/app/views/shared/notifications/_custom_notifications.html.haml @@ -25,7 +25,7 @@ .form-group .checkbox{ class: ("prepend-top-0" if index == 0) } %label{ for: field_id } - = check_box("notification_setting", event, id: field_id, class: "js-custom-notification-event", checked: notification_setting.events[event]) + = check_box("notification_setting", event, id: field_id, class: "js-custom-notification-event", checked: notification_setting.public_send(event)) %strong = notification_event_name(event) = icon("spinner spin", class: "custom-notification-event-loading") diff --git a/changelogs/unreleased/quiet-pipelines.yml b/changelogs/unreleased/quiet-pipelines.yml new file mode 100644 index 00000000000..c02eb59b824 --- /dev/null +++ b/changelogs/unreleased/quiet-pipelines.yml @@ -0,0 +1,5 @@ +--- +title: Only email pipeline creators; only email for successful pipelines with custom + settings +merge_request: +author: diff --git a/doc/workflow/notifications.md b/doc/workflow/notifications.md index 4c52974e103..e91d36987a9 100644 --- a/doc/workflow/notifications.md +++ b/doc/workflow/notifications.md @@ -66,14 +66,13 @@ Below is the table of events users can be notified of: In all of the below cases, the notification will be sent to: - Participants: - the author and assignee of the issue/merge request - - the author of the pipeline - authors of comments on the issue/merge request - anyone mentioned by `@username` in the issue/merge request title or description - anyone mentioned by `@username` in any of the comments on the issue/merge request ...with notification level "Participating" or higher -- Watchers: users with notification level "Watch" (however successful pipeline would be off for watchers) +- Watchers: users with notification level "Watch" - Subscribers: anyone who manually subscribed to the issue/merge request - Custom: Users with notification level "custom" who turned on notifications for any of the events present in the table below @@ -89,8 +88,8 @@ In all of the below cases, the notification will be sent to: | Reopen merge request | | | Merge merge request | | | New comment | The above, plus anyone mentioned by `@username` in the comment, with notification level "Mention" or higher | -| Failed pipeline | The above, plus the author of the pipeline | -| Successful pipeline | The above, plus the author of the pipeline | +| Failed pipeline | The author of the pipeline | +| Successful pipeline | The author of the pipeline, if they have the custom notification setting for successful pipelines set | In addition, if the title or description of an Issue or Merge Request is diff --git a/spec/factories/ci/pipelines.rb b/spec/factories/ci/pipelines.rb index b67c96bc00d..561fbc8e247 100644 --- a/spec/factories/ci/pipelines.rb +++ b/spec/factories/ci/pipelines.rb @@ -48,6 +48,10 @@ FactoryGirl.define do trait :success do status :success end + + trait :failed do + status :failed + end end end end diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb index 53282b999dc..e4a24fd63c2 100644 --- a/spec/models/ci/pipeline_spec.rb +++ b/spec/models/ci/pipeline_spec.rb @@ -1055,10 +1055,13 @@ describe Ci::Pipeline, models: true do end before do - reset_delivered_emails! - project.team << [pipeline.user, Gitlab::Access::DEVELOPER] + pipeline.user.global_notification_setting. + update(level: 'custom', failed_pipeline: true, success_pipeline: true) + + reset_delivered_emails! + perform_enqueued_jobs do pipeline.enqueue pipeline.run diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 5c841843b40..e3146a56495 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -113,7 +113,7 @@ describe NotificationService, services: true do project.add_master(issue.assignee) project.add_master(note.author) create(:note_on_issue, noteable: issue, project_id: issue.project_id, note: '@subscribed_participant cc this guy') - update_custom_notification(:new_note, @u_guest_custom, project) + update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) end @@ -379,7 +379,7 @@ describe NotificationService, services: true do build_team(note.project) reset_delivered_emails! allow_any_instance_of(Commit).to receive(:author).and_return(@u_committer) - update_custom_notification(:new_note, @u_guest_custom, project) + update_custom_notification(:new_note, @u_guest_custom, resource: project) update_custom_notification(:new_note, @u_custom_global) end @@ -457,7 +457,7 @@ describe NotificationService, services: true do add_users_with_subscription(issue.project, issue) reset_delivered_emails! - update_custom_notification(:new_issue, @u_guest_custom, project) + update_custom_notification(:new_issue, @u_guest_custom, resource: project) update_custom_notification(:new_issue, @u_custom_global) end @@ -567,7 +567,7 @@ describe NotificationService, services: true do describe '#reassigned_issue' do before do - update_custom_notification(:reassign_issue, @u_guest_custom, project) + update_custom_notification(:reassign_issue, @u_guest_custom, resource: project) update_custom_notification(:reassign_issue, @u_custom_global) end @@ -760,7 +760,7 @@ describe NotificationService, services: true do describe '#close_issue' do before do - update_custom_notification(:close_issue, @u_guest_custom, project) + update_custom_notification(:close_issue, @u_guest_custom, resource: project) update_custom_notification(:close_issue, @u_custom_global) end @@ -791,7 +791,7 @@ describe NotificationService, services: true do describe '#reopen_issue' do before do - update_custom_notification(:reopen_issue, @u_guest_custom, project) + update_custom_notification(:reopen_issue, @u_guest_custom, resource: project) update_custom_notification(:reopen_issue, @u_custom_global) end @@ -856,14 +856,14 @@ describe NotificationService, services: true do before do build_team(merge_request.target_project) add_users_with_subscription(merge_request.target_project, merge_request) - update_custom_notification(:new_merge_request, @u_guest_custom, project) + update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) update_custom_notification(:new_merge_request, @u_custom_global) reset_delivered_emails! end describe '#new_merge_request' do before do - update_custom_notification(:new_merge_request, @u_guest_custom, project) + update_custom_notification(:new_merge_request, @u_guest_custom, resource: project) update_custom_notification(:new_merge_request, @u_custom_global) end @@ -952,7 +952,7 @@ describe NotificationService, services: true do describe '#reassigned_merge_request' do before do - update_custom_notification(:reassign_merge_request, @u_guest_custom, project) + update_custom_notification(:reassign_merge_request, @u_guest_custom, resource: project) update_custom_notification(:reassign_merge_request, @u_custom_global) end @@ -1026,7 +1026,7 @@ describe NotificationService, services: true do describe '#closed_merge_request' do before do - update_custom_notification(:close_merge_request, @u_guest_custom, project) + update_custom_notification(:close_merge_request, @u_guest_custom, resource: project) update_custom_notification(:close_merge_request, @u_custom_global) end @@ -1056,7 +1056,7 @@ describe NotificationService, services: true do describe '#merged_merge_request' do before do - update_custom_notification(:merge_merge_request, @u_guest_custom, project) + update_custom_notification(:merge_merge_request, @u_guest_custom, resource: project) update_custom_notification(:merge_merge_request, @u_custom_global) end @@ -1108,7 +1108,7 @@ describe NotificationService, services: true do describe '#reopen_merge_request' do before do - update_custom_notification(:reopen_merge_request, @u_guest_custom, project) + update_custom_notification(:reopen_merge_request, @u_guest_custom, resource: project) update_custom_notification(:reopen_merge_request, @u_custom_global) end @@ -1281,40 +1281,172 @@ describe NotificationService, services: true do describe 'Pipelines' do describe '#pipeline_finished' do let(:project) { create(:project, :public, :repository) } - let(:current_user) { create(:user) } let(:u_member) { create(:user) } - let(:u_other) { create(:user) } + let(:u_watcher) { create_user_with_notification(:watch, 'watcher') } + + let(:u_custom_notification_unset) do + create_user_with_notification(:custom, 'custom_unset') + end + + let(:u_custom_notification_enabled) do + user = create_user_with_notification(:custom, 'custom_enabled') + update_custom_notification(:success_pipeline, user, resource: project) + update_custom_notification(:failed_pipeline, user, resource: project) + user + end + + let(:u_custom_notification_disabled) do + user = create_user_with_notification(:custom, 'custom_disabled') + update_custom_notification(:success_pipeline, user, resource: project, value: false) + update_custom_notification(:failed_pipeline, user, resource: project, value: false) + user + end let(:commit) { project.commit } - let(:pipeline) do - create(:ci_pipeline, :success, + + def create_pipeline(user, status) + create(:ci_pipeline, status, project: project, - user: current_user, + user: user, ref: 'refs/heads/master', sha: commit.id, before_sha: '00000000') end before do - project.add_master(current_user) project.add_master(u_member) + project.add_master(u_watcher) + project.add_master(u_custom_notification_unset) + project.add_master(u_custom_notification_enabled) + project.add_master(u_custom_notification_disabled) + reset_delivered_emails! end - context 'without custom recipients' do - it 'notifies the pipeline user' do - notification.pipeline_finished(pipeline) + context 'with a successful pipeline' do + context 'when the creator has default settings' do + before do + pipeline = create_pipeline(u_member, :success) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has watch set' do + before do + pipeline = create_pipeline(u_watcher, :success) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has custom notifications, but without any set' do + before do + pipeline = create_pipeline(u_custom_notification_unset, :success) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has custom notifications disabled' do + before do + pipeline = create_pipeline(u_custom_notification_disabled, :success) + notification.pipeline_finished(pipeline) + end + + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has custom notifications enabled' do + before do + pipeline = create_pipeline(u_custom_notification_enabled, :success) + notification.pipeline_finished(pipeline) + end - should_only_email(current_user, kind: :bcc) + it 'emails only the creator' do + should_only_email(u_custom_notification_enabled, kind: :bcc) + end end end - context 'with custom recipients' do - it 'notifies the custom recipients' do - users = [u_member, u_other] - notification.pipeline_finished(pipeline, users.map(&:notification_email)) + context 'with a failed pipeline' do + context 'when the creator has no custom notification set' do + before do + pipeline = create_pipeline(u_member, :failed) + notification.pipeline_finished(pipeline) + end + + it 'emails only the creator' do + should_only_email(u_member, kind: :bcc) + end + end + + context 'when the creator has watch set' do + before do + pipeline = create_pipeline(u_watcher, :failed) + notification.pipeline_finished(pipeline) + end + + it 'emails only the creator' do + should_only_email(u_watcher, kind: :bcc) + end + end + + context 'when the creator has custom notifications, but without any set' do + before do + pipeline = create_pipeline(u_custom_notification_unset, :failed) + notification.pipeline_finished(pipeline) + end + + it 'emails only the creator' do + should_only_email(u_custom_notification_unset, kind: :bcc) + end + end + + context 'when the creator has custom notifications disabled' do + before do + pipeline = create_pipeline(u_custom_notification_disabled, :failed) + notification.pipeline_finished(pipeline) + end - should_only_email(*users, kind: :bcc) + it 'notifies nobody' do + should_not_email_anyone + end + end + + context 'when the creator has custom notifications set' do + before do + pipeline = create_pipeline(u_custom_notification_enabled, :failed) + notification.pipeline_finished(pipeline) + end + + it 'emails only the creator' do + should_only_email(u_custom_notification_enabled, kind: :bcc) + end + end + + context 'when the creator has no read_build access' do + before do + pipeline = create_pipeline(u_member, :failed) + project.update(public_builds: false) + project.team.truncate + notification.pipeline_finished(pipeline) + end + + it 'does not send emails' do + should_not_email_anyone + end end end end @@ -1385,9 +1517,9 @@ describe NotificationService, services: true do # Create custom notifications # When resource is nil it means global notification - def update_custom_notification(event, user, resource = nil) + def update_custom_notification(event, user, resource: nil, value: true) setting = user.notification_settings_for(resource) - setting.events[event] = true + setting.events[event] = value setting.save end diff --git a/spec/workers/pipeline_notification_worker_spec.rb b/spec/workers/pipeline_notification_worker_spec.rb index 5a7ce2e08c4..139032d77bd 100644 --- a/spec/workers/pipeline_notification_worker_spec.rb +++ b/spec/workers/pipeline_notification_worker_spec.rb @@ -3,131 +3,19 @@ require 'spec_helper' describe PipelineNotificationWorker do include EmailHelpers - let(:pipeline) do - create(:ci_pipeline, - project: project, - sha: project.commit('master').sha, - user: pusher, - status: status) - end - - let(:project) { create(:project, :repository, public_builds: false) } - let(:user) { create(:user) } - let(:pusher) { user } - let(:watcher) { pusher } + let(:pipeline) { create(:ci_pipeline) } describe '#execute' do - before do - reset_delivered_emails! - pipeline.project.team << [pusher, Gitlab::Access::DEVELOPER] - end - - context 'when watcher has developer access' do - before do - pipeline.project.team << [watcher, Gitlab::Access::DEVELOPER] - end - - shared_examples 'sending emails' do - it 'sends emails' do - perform_enqueued_jobs do - subject.perform(pipeline.id) - end - - emails = ActionMailer::Base.deliveries - actual = emails.flat_map(&:bcc).sort - expected_receivers = receivers.map(&:email).uniq.sort - - expect(actual).to eq(expected_receivers) - expect(emails.size).to eq(1) - expect(emails.last.subject).to include(email_subject) - end - end - - context 'with success pipeline' do - let(:status) { 'success' } - let(:email_subject) { "Pipeline ##{pipeline.id} has succeeded" } - let(:receivers) { [pusher, watcher] } - - it_behaves_like 'sending emails' - - context 'with pipeline from someone else' do - let(:pusher) { create(:user) } - let(:watcher) { user } - - context 'with success pipeline notification on' do - before do - watcher.global_notification_setting. - update(level: 'custom', success_pipeline: true) - end - - it_behaves_like 'sending emails' - end - - context 'with success pipeline notification off' do - let(:receivers) { [pusher] } + it 'calls NotificationService#pipeline_finished when the pipeline exists' do + expect(NotificationService).to receive_message_chain(:new, :pipeline_finished) - before do - watcher.global_notification_setting. - update(level: 'custom', success_pipeline: false) - end - - it_behaves_like 'sending emails' - end - end - - context 'with failed pipeline' do - let(:status) { 'failed' } - let(:email_subject) { "Pipeline ##{pipeline.id} has failed" } - - it_behaves_like 'sending emails' - - context 'with pipeline from someone else' do - let(:pusher) { create(:user) } - let(:watcher) { user } - - context 'with failed pipeline notification on' do - before do - watcher.global_notification_setting. - update(level: 'custom', failed_pipeline: true) - end - - it_behaves_like 'sending emails' - end - - context 'with failed pipeline notification off' do - let(:receivers) { [pusher] } - - before do - watcher.global_notification_setting. - update(level: 'custom', failed_pipeline: false) - end - - it_behaves_like 'sending emails' - end - end - end - end + subject.perform(pipeline.id) end - context 'when watcher has no read_build access' do - let(:status) { 'failed' } - let(:email_subject) { "Pipeline ##{pipeline.id} has failed" } - let(:watcher) { create(:user) } - - before do - pipeline.project.team << [watcher, Gitlab::Access::GUEST] - - watcher.global_notification_setting. - update(level: 'custom', failed_pipeline: true) - - perform_enqueued_jobs do - subject.perform(pipeline.id) - end - end + it 'does nothing when the pipeline does not exist' do + expect(NotificationService).not_to receive(:new) - it 'does not send emails' do - should_only_email(pusher, kind: :bcc) - end + subject.perform(Ci::Pipeline.maximum(:id).to_i.succ) end end end -- cgit v1.2.1 From 9543025e88d3d0fe298e95330b8d38802da50cc6 Mon Sep 17 00:00:00 2001 From: Adam Niedzielski Date: Mon, 3 Apr 2017 15:17:04 +0200 Subject: Introduce "polling_interval_multiplier" as application setting Implement module for setting "Poll-Interval" response header. Return 429 in ETag caching middleware when polling is disabled. --- .../admin/application_settings_controller.rb | 1 + app/models/application_setting.rb | 7 ++++- .../admin/application_settings/_form.html.haml | 14 +++++++++ .../introduce-polling-interval-multiplier.yml | 4 +++ ..._interval_multiplier_to_application_settings.rb | 33 +++++++++++++++++++++ db/schema.rb | 3 +- doc/api/settings.md | 7 +++-- lib/api/entities.rb | 1 + lib/api/settings.rb | 3 +- lib/gitlab/etag_caching/middleware.rb | 11 +++++-- lib/gitlab/polling_interval.rb | 22 ++++++++++++++ spec/lib/gitlab/etag_caching/middleware_spec.rb | 13 +++++++++ spec/lib/gitlab/polling_interval_spec.rb | 34 ++++++++++++++++++++++ 13 files changed, 146 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/introduce-polling-interval-multiplier.yml create mode 100644 db/migrate/20170329124448_add_polling_interval_multiplier_to_application_settings.rb create mode 100644 lib/gitlab/polling_interval.rb create mode 100644 spec/lib/gitlab/polling_interval_spec.rb diff --git a/app/controllers/admin/application_settings_controller.rb b/app/controllers/admin/application_settings_controller.rb index 0bfbe47eb4f..515d8e1523b 100644 --- a/app/controllers/admin/application_settings_controller.rb +++ b/app/controllers/admin/application_settings_controller.rb @@ -134,6 +134,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController :unique_ips_limit_enabled, :version_check_enabled, :terminal_max_session_time, + :polling_interval_multiplier, disabled_oauth_sign_in_sources: [], import_sources: [], diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 671a0fe98cc..2961e16f5e0 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -131,6 +131,10 @@ class ApplicationSetting < ActiveRecord::Base presence: true, numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validates :polling_interval_multiplier, + presence: true, + numericality: { greater_than_or_equal_to: 0 } + validates_each :restricted_visibility_levels do |record, attr, value| value&.each do |level| unless Gitlab::VisibilityLevel.options.has_value?(level) @@ -233,7 +237,8 @@ class ApplicationSetting < ActiveRecord::Base signup_enabled: Settings.gitlab['signup_enabled'], terminal_max_session_time: 0, two_factor_grace_period: 48, - user_default_external: false + user_default_external: false, + polling_interval_multiplier: 1 } end diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 3eab065bb9f..5d51a2b5cbc 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -558,5 +558,19 @@ Maximum time for web terminal websocket connection (in seconds). 0 for unlimited. + %fieldset + %legend Real-time features + .form-group + = f.label :polling_interval_multiplier, 'Polling interval multiplier', class: 'control-label col-sm-2' + .col-sm-10 + = f.text_field :polling_interval_multiplier, class: 'form-control' + .help-block + Change this value to influence how frequently the GitLab UI polls for updates. + If you set the value to 2 all polling intervals are multiplied + by 2, which means that polling happens half as frequently. + The multiplier can also have a decimal value. + The default value (1) is a reasonable choice for the majority of GitLab + installations. Set to 0 to completely disable polling. + .form-actions = f.submit 'Save', class: 'btn btn-save' diff --git a/changelogs/unreleased/introduce-polling-interval-multiplier.yml b/changelogs/unreleased/introduce-polling-interval-multiplier.yml new file mode 100644 index 00000000000..3ccae8e327f --- /dev/null +++ b/changelogs/unreleased/introduce-polling-interval-multiplier.yml @@ -0,0 +1,4 @@ +--- +title: Introduce "polling_interval_multiplier" as application setting +merge_request: 10280 +author: diff --git a/db/migrate/20170329124448_add_polling_interval_multiplier_to_application_settings.rb b/db/migrate/20170329124448_add_polling_interval_multiplier_to_application_settings.rb new file mode 100644 index 00000000000..a8affd19a0b --- /dev/null +++ b/db/migrate/20170329124448_add_polling_interval_multiplier_to_application_settings.rb @@ -0,0 +1,33 @@ +# See http://doc.gitlab.com/ce/development/migration_style_guide.html +# for more information on how to write migrations for GitLab. + +class AddPollingIntervalMultiplierToApplicationSettings < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + # Set this constant to true if this migration requires downtime. + DOWNTIME = false + + # When a migration requires downtime you **must** uncomment the following + # constant and define a short and easy to understand explanation as to why the + # migration requires downtime. + # DOWNTIME_REASON = '' + + # When using the methods "add_concurrent_index" or "add_column_with_default" + # you must disable the use of transactions as these methods can not run in an + # existing transaction. When using "add_concurrent_index" make sure that this + # method is the _only_ method called in the migration, any other changes + # should go in a separate migration. This ensures that upon failure _only_ the + # index creation fails and can be retried or reverted easily. + # + # To disable transactions uncomment the following line and remove these + # comments: + disable_ddl_transaction! + + def up + add_column_with_default :application_settings, :polling_interval_multiplier, :decimal, default: 1, allow_null: false + end + + def down + remove_column :application_settings, :polling_interval_multiplier + end +end diff --git a/db/schema.rb b/db/schema.rb index dba242548c1..19aca4b941e 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20170317203554) do +ActiveRecord::Schema.define(version: 20170329124448) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -115,6 +115,7 @@ ActiveRecord::Schema.define(version: 20170317203554) do t.integer "unique_ips_limit_per_user" t.integer "unique_ips_limit_time_window" t.boolean "unique_ips_limit_enabled", default: false, null: false + t.decimal "polling_interval_multiplier", default: 1.0, null: false end create_table "audit_events", force: :cascade do |t| diff --git a/doc/api/settings.md b/doc/api/settings.md index ad975e2e325..d99695ca986 100644 --- a/doc/api/settings.md +++ b/doc/api/settings.md @@ -48,7 +48,8 @@ Example response: "koding_url": null, "plantuml_enabled": false, "plantuml_url": null, - "terminal_max_session_time": 0 + "terminal_max_session_time": 0, + "polling_interval_multiplier": 1.0 } ``` @@ -88,6 +89,7 @@ PUT /application/settings | `plantuml_enabled` | boolean | no | Enable PlantUML integration. Default is `false`. | | `plantuml_url` | string | yes (if `plantuml_enabled` is `true`) | The PlantUML instance URL for integration. | | `terminal_max_session_time` | integer | no | Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time. | +| `polling_interval_multiplier` | decimal | no | Interval multiplier used by endpoints that perform polling. Set to 0 to disable polling. | ```bash curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v4/application/settings?signup_enabled=false&default_project_visibility=internal @@ -124,6 +126,7 @@ Example response: "koding_url": null, "plantuml_enabled": false, "plantuml_url": null, - "terminal_max_session_time": 0 + "terminal_max_session_time": 0, + "polling_interval_multiplier": 1.0 } ``` diff --git a/lib/api/entities.rb b/lib/api/entities.rb index 5954aea8041..01ae2dbd583 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -581,6 +581,7 @@ module API expose :plantuml_enabled expose :plantuml_url expose :terminal_max_session_time + expose :polling_interval_multiplier end class Release < Grape::Entity diff --git a/lib/api/settings.rb b/lib/api/settings.rb index d4d3229f0d1..c7f97ad2aab 100644 --- a/lib/api/settings.rb +++ b/lib/api/settings.rb @@ -110,6 +110,7 @@ module API requires :housekeeping_gc_period, type: Integer, desc: "Number of Git pushes after which 'git gc' is run." end optional :terminal_max_session_time, type: Integer, desc: 'Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time.' + optional :polling_interval_multiplier, type: BigDecimal, desc: 'Interval multiplier used by endpoints that perform polling. Set to 0 to disable polling.' at_least_one_of :default_branch_protection, :default_project_visibility, :default_snippet_visibility, :default_group_visibility, :restricted_visibility_levels, :import_sources, :enabled_git_access_protocol, :gravatar_enabled, :default_projects_limit, @@ -125,7 +126,7 @@ module API :akismet_enabled, :admin_notification_email, :sentry_enabled, :repository_storage, :repository_checks_enabled, :koding_enabled, :plantuml_enabled, :version_check_enabled, :email_author_in_body, :html_emails_enabled, - :housekeeping_enabled, :terminal_max_session_time + :housekeeping_enabled, :terminal_max_session_time, :polling_interval_multiplier end put "application/settings" do attrs = declared_params(include_missing: false) diff --git a/lib/gitlab/etag_caching/middleware.rb b/lib/gitlab/etag_caching/middleware.rb index ffbc6e17dc5..9c98f0d1a30 100644 --- a/lib/gitlab/etag_caching/middleware.rb +++ b/lib/gitlab/etag_caching/middleware.rb @@ -18,8 +18,7 @@ module Gitlab if_none_match = env['HTTP_IF_NONE_MATCH'] if if_none_match == etag - Gitlab::Metrics.add_event(:etag_caching_cache_hit) - [304, { 'ETag' => etag }, ['']] + handle_cache_hit(etag) else track_cache_miss(if_none_match, cached_value_present) @@ -52,6 +51,14 @@ module Gitlab %Q{W/"#{value}"} end + def handle_cache_hit(etag) + Gitlab::Metrics.add_event(:etag_caching_cache_hit) + + status_code = Gitlab::PollingInterval.polling_enabled? ? 304 : 429 + + [status_code, { 'ETag' => etag }, ['']] + end + def track_cache_miss(if_none_match, cached_value_present) if if_none_match.blank? Gitlab::Metrics.add_event(:etag_caching_header_missing) diff --git a/lib/gitlab/polling_interval.rb b/lib/gitlab/polling_interval.rb new file mode 100644 index 00000000000..c44bb1cd14d --- /dev/null +++ b/lib/gitlab/polling_interval.rb @@ -0,0 +1,22 @@ +module Gitlab + class PollingInterval + include Gitlab::CurrentSettings + + HEADER_NAME = 'Poll-Interval'.freeze + + def self.set_header(response, interval:) + if polling_enabled? + multiplier = current_application_settings.polling_interval_multiplier + value = (interval * multiplier).to_i + else + value = -1 + end + + response.headers[HEADER_NAME] = value + end + + def self.polling_enabled? + !current_application_settings.polling_interval_multiplier.zero? + end + end +end diff --git a/spec/lib/gitlab/etag_caching/middleware_spec.rb b/spec/lib/gitlab/etag_caching/middleware_spec.rb index 8b5bfc4dbb0..6ec4360adc2 100644 --- a/spec/lib/gitlab/etag_caching/middleware_spec.rb +++ b/spec/lib/gitlab/etag_caching/middleware_spec.rb @@ -99,6 +99,19 @@ describe Gitlab::EtagCaching::Middleware do middleware.call(build_env(path, if_none_match)) end + + context 'when polling is disabled' do + before do + allow(Gitlab::PollingInterval).to receive(:polling_enabled?). + and_return(false) + end + + it 'returns status code 429' do + status, _, _ = middleware.call(build_env(path, if_none_match)) + + expect(status).to eq 429 + end + end end context 'when If-None-Match header does not match ETag in store' do diff --git a/spec/lib/gitlab/polling_interval_spec.rb b/spec/lib/gitlab/polling_interval_spec.rb new file mode 100644 index 00000000000..56c2847e26a --- /dev/null +++ b/spec/lib/gitlab/polling_interval_spec.rb @@ -0,0 +1,34 @@ +require 'spec_helper' + +describe Gitlab::PollingInterval, lib: true do + let(:polling_interval) { described_class } + + describe '.set_header' do + let(:headers) { {} } + let(:response) { double(headers: headers) } + + context 'when polling is disabled' do + before do + stub_application_setting(polling_interval_multiplier: 0) + end + + it 'sets value to -1' do + polling_interval.set_header(response, interval: 10_000) + + expect(headers['Poll-Interval']).to eq(-1) + end + end + + context 'when polling is enabled' do + before do + stub_application_setting(polling_interval_multiplier: 0.33333) + end + + it 'applies modifier to base interval' do + polling_interval.set_header(response, interval: 10_000) + + expect(headers['Poll-Interval']).to eq(3333) + end + end + end +end -- cgit v1.2.1 From 059e11d87f1948b146febc1b3e93ac5a52863d2b Mon Sep 17 00:00:00 2001 From: Robin Bobbitt Date: Mon, 27 Mar 2017 16:22:01 -0400 Subject: Fix race condition in namespace directory creation --- changelogs/unreleased/namespace-race-condition.yml | 4 ++++ lib/gitlab/shell.rb | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/namespace-race-condition.yml diff --git a/changelogs/unreleased/namespace-race-condition.yml b/changelogs/unreleased/namespace-race-condition.yml new file mode 100644 index 00000000000..2a76b6c74e8 --- /dev/null +++ b/changelogs/unreleased/namespace-race-condition.yml @@ -0,0 +1,4 @@ +--- +title: Fix project creation failure due to race condition in namespace directory creation +merge_request: 10268 +author: Robin Bobbitt diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index da8d8ddb8ed..9864a9c7f1a 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -174,7 +174,10 @@ module Gitlab # add_namespace("/path/to/storage", "gitlab") # def add_namespace(storage, name) - FileUtils.mkdir_p(full_path(storage, name), mode: 0770) unless exists?(storage, name) + path = full_path(storage, name) + FileUtils.mkdir_p(path, mode: 0770) unless exists?(storage, name) + rescue Errno::EEXIST => e + Rails.logger.warn("Directory exists as a file: #{e} at: #{path}") end # Remove directory from repositories storage -- cgit v1.2.1 From 39753bfb9cdd77ed7fc1458afc202b126ea6984d Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Thu, 30 Mar 2017 17:24:36 +0200 Subject: Add feature flags for enabling (Upload|Receive)Pack for Gitaly Closes gitaly#168 --- app/controllers/projects/git_http_controller.rb | 2 +- lib/gitlab/workhorse.rb | 16 ++++++- spec/lib/gitlab/workhorse_spec.rb | 58 ++++++++++++++++++++++--- 3 files changed, 66 insertions(+), 10 deletions(-) diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 278098fcc58..37f6f637ff0 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -57,7 +57,7 @@ class Projects::GitHttpController < Projects::GitHttpClientController def render_ok set_workhorse_internal_api_content_type - render json: Gitlab::Workhorse.git_http_ok(repository, user) + render json: Gitlab::Workhorse.git_http_ok(repository, user, action_name) end def render_http_not_allowed diff --git a/lib/gitlab/workhorse.rb b/lib/gitlab/workhorse.rb index d1131ad65e0..d0637f8b394 100644 --- a/lib/gitlab/workhorse.rb +++ b/lib/gitlab/workhorse.rb @@ -16,7 +16,7 @@ module Gitlab SECRET_LENGTH = 32 class << self - def git_http_ok(repository, user) + def git_http_ok(repository, user, action) repo_path = repository.path_to_repo params = { GL_ID: Gitlab::GlId.gl_id(user), @@ -26,13 +26,25 @@ module Gitlab if Gitlab.config.gitaly.enabled storage = repository.project.repository_storage address = Gitlab::GitalyClient.get_address(storage) - params[:GitalySocketPath] = URI(address).path # TODO: use GitalyClient code to assemble the Repository message params[:Repository] = Gitaly::Repository.new( path: repo_path, storage_name: storage, relative_path: Gitlab::RepoPath.strip_storage_path(repo_path), ).to_h + + feature_enabled = case action.to_s + when 'git_receive_pack' + Gitlab::GitalyClient.feature_enabled?(:post_receive_pack) + when 'git_upload_pack' + Gitlab::GitalyClient.feature_enabled?(:post_upload_pack) + when 'info_refs' + true + else + raise "Unsupported action: #{action}" + end + + params[:GitalySocketPath] = URI(address).path if feature_enabled end params diff --git a/spec/lib/gitlab/workhorse_spec.rb b/spec/lib/gitlab/workhorse_spec.rb index cb7c810124e..3bd2a3238fe 100644 --- a/spec/lib/gitlab/workhorse_spec.rb +++ b/spec/lib/gitlab/workhorse_spec.rb @@ -180,24 +180,68 @@ describe Gitlab::Workhorse, lib: true do describe '.git_http_ok' do let(:user) { create(:user) } let(:repo_path) { repository.path_to_repo } + let(:action) { 'info_refs' } - subject { described_class.git_http_ok(repository, user) } + subject { described_class.git_http_ok(repository, user, action) } - it { expect(subject).to eq({ GL_ID: "user-#{user.id}", RepoPath: repo_path }) } + it { expect(subject).to include({ GL_ID: "user-#{user.id}", RepoPath: repo_path }) } context 'when Gitaly is enabled' do + let(:gitaly_params) do + { + GitalySocketPath: URI(Gitlab::GitalyClient.get_address('default')).path, + } + end + before do allow(Gitlab.config.gitaly).to receive(:enabled).and_return(true) end - it 'includes Gitaly params in the returned value' do - gitaly_socket_path = URI(Gitlab::GitalyClient.get_address('default')).path - expect(subject).to include({ GitalySocketPath: gitaly_socket_path }) - expect(subject[:Repository]).to include({ + it 'includes a Repository param' do + repo_param = { Repository: { path: repo_path, storage_name: 'default', relative_path: project.full_path + '.git', - }) + } } + + expect(subject).to include(repo_param) + end + + { + git_receive_pack: :post_receive_pack, + git_upload_pack: :post_upload_pack + }.each do |action_name, feature_flag| + context "when #{action_name} action is passed" do + let(:action) { action_name } + + context 'when action is enabled by feature flag' do + it 'includes Gitaly params in the returned value' do + allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(feature_flag).and_return(true) + + expect(subject).to include(gitaly_params) + end + end + + context 'when action is not enabled by feature flag' do + it 'does not include Gitaly params in the returned value' do + allow(Gitlab::GitalyClient).to receive(:feature_enabled?).with(feature_flag).and_return(false) + + expect(subject).not_to include(gitaly_params) + end + end + end + end + + context "when info_refs action is passed" do + let(:action) { 'info_refs' } + + it { expect(subject).to include(gitaly_params) } + end + + context 'when action passed is not supported by Gitaly' do + let(:action) { 'download' } + + it { expect { subject }.to raise_exception('Unsupported action: download') } end end end -- cgit v1.2.1 From 09751c75ebaee03f5161c4e86f16a18a8f5b050f Mon Sep 17 00:00:00 2001 From: Ahmad Sherif Date: Mon, 3 Apr 2017 15:27:14 +0200 Subject: Add support for Gitaly calls over TCP connection Closes gitaly#166 --- config/gitlab.yml.example | 2 +- config/initializers/8_gitaly.rb | 2 +- lib/gitlab/gitaly_client.rb | 4 +++- spec/lib/gitlab/gitaly_client_spec.rb | 26 ++++++++++++++++++++++++++ 4 files changed, 31 insertions(+), 3 deletions(-) create mode 100644 spec/lib/gitlab/gitaly_client_spec.rb diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index bd27f01c872..4314e902564 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -461,7 +461,7 @@ production: &base storages: # You must have at least a `default` storage path. default: path: /home/git/repositories/ - gitaly_address: unix:/home/git/gitlab/tmp/sockets/private/gitaly.socket + gitaly_address: unix:/home/git/gitlab/tmp/sockets/private/gitaly.socket # TCP connections are supported too (e.g. tcp://host:port) ## Backup settings backup: diff --git a/config/initializers/8_gitaly.rb b/config/initializers/8_gitaly.rb index 69c0a91d6f0..c7f27c78535 100644 --- a/config/initializers/8_gitaly.rb +++ b/config/initializers/8_gitaly.rb @@ -9,7 +9,7 @@ if Gitlab.config.gitaly.enabled || Rails.env.test? raise "storage #{name.inspect} is missing a gitaly_address" end - unless URI(address).scheme == 'unix' + unless URI(address).scheme.in?(%w(tcp unix)) raise "Unsupported Gitaly address: #{address.inspect}" end diff --git a/lib/gitlab/gitaly_client.rb b/lib/gitlab/gitaly_client.rb index a0dbe0a8c11..fe15fb12adb 100644 --- a/lib/gitlab/gitaly_client.rb +++ b/lib/gitlab/gitaly_client.rb @@ -12,9 +12,11 @@ module Gitlab end def self.new_channel(address) - # NOTE: Gitaly currently runs on a Unix socket, so permissions are + address = address.sub(%r{^tcp://}, '') if URI(address).scheme == 'tcp' + # NOTE: When Gitaly runs on a Unix socket, permissions are # handled using the file system and no additional authentication is # required (therefore the :this_channel_is_insecure flag) + # TODO: Add authentication support when Gitaly is running on a TCP socket. GRPC::Core::Channel.new(address, {}, :this_channel_is_insecure) end diff --git a/spec/lib/gitlab/gitaly_client_spec.rb b/spec/lib/gitlab/gitaly_client_spec.rb new file mode 100644 index 00000000000..55fcf91fb6e --- /dev/null +++ b/spec/lib/gitlab/gitaly_client_spec.rb @@ -0,0 +1,26 @@ +require 'spec_helper' + +describe Gitlab::GitalyClient, lib: true do + describe '.new_channel' do + context 'when passed a UNIX socket address' do + it 'passes the address as-is to GRPC::Core::Channel initializer' do + address = 'unix:/tmp/gitaly.sock' + + expect(GRPC::Core::Channel).to receive(:new).with(address, any_args) + + described_class.new_channel(address) + end + end + + context 'when passed a TCP address' do + it 'strips tcp:// prefix before passing it to GRPC::Core::Channel initializer' do + address = 'localhost:9876' + prefixed_address = "tcp://#{address}" + + expect(GRPC::Core::Channel).to receive(:new).with(address, any_args) + + described_class.new_channel(prefixed_address) + end + end + end +end -- cgit v1.2.1 From 4e3516788fdea3de3c5f06e1981ddc518b05d0fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 23 Mar 2017 14:08:39 +0100 Subject: Don't use FFaker in factories, use sequences instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit FFaker can generate data that randomly break our test suite. This simplifies our factories and use sequences which are more predictive. Signed-off-by: Rémy Coutable --- features/steps/project/hooks.rb | 4 ++-- .../controllers/profiles/personal_access_tokens_spec.rb | 2 +- spec/factories/chat_names.rb | 8 ++------ spec/factories/chat_teams.rb | 5 +---- spec/factories/ci/runners.rb | 4 +--- spec/factories/emails.rb | 2 +- spec/factories/issues.rb | 4 ---- spec/factories/labels.rb | 4 ++-- spec/factories/oauth_applications.rb | 4 ++-- spec/factories/personal_access_tokens.rb | 2 +- spec/factories/project_hooks.rb | 2 +- spec/factories/sequences.rb | 10 ++++++++++ spec/factories/service_hooks.rb | 2 +- spec/factories/snippets.rb | 9 ++------- spec/factories/spam_logs.rb | 6 +++--- spec/factories/system_hooks.rb | 2 +- spec/factories/users.rb | 8 +++----- spec/features/admin/admin_hooks_spec.rb | 2 +- .../admin/admin_users_impersonation_tokens_spec.rb | 2 +- spec/features/issuables/issuable_list_spec.rb | 9 ++++----- spec/features/profiles/personal_access_tokens_spec.rb | 4 ++-- spec/features/u2f_spec.rb | 17 ++++++++--------- spec/lib/gitlab/git_access_spec.rb | 8 ++++---- spec/lib/gitlab/git_spec.rb | 17 ++--------------- spec/mailers/notify_spec.rb | 4 ++-- spec/models/cycle_analytics/plan_spec.rb | 4 ++-- spec/models/cycle_analytics/production_spec.rb | 2 +- spec/models/cycle_analytics/staging_spec.rb | 2 +- spec/models/repository_spec.rb | 17 ++--------------- spec/requests/api/files_spec.rb | 17 ++--------------- spec/requests/api/projects_spec.rb | 2 -- spec/requests/api/v3/files_spec.rb | 4 ++-- spec/requests/api/v3/projects_spec.rb | 2 -- spec/support/cycle_analytics_helpers.rb | 12 +++++------- spec/support/git_helpers.rb | 9 --------- spec/support/issuables_list_metadata_shared_examples.rb | 4 ++-- ...ble_create_service_slash_commands_shared_examples.rb | 2 +- 37 files changed, 76 insertions(+), 142 deletions(-) create mode 100644 spec/factories/sequences.rb delete mode 100644 spec/support/git_helpers.rb diff --git a/features/steps/project/hooks.rb b/features/steps/project/hooks.rb index 37b608ffbd3..0a71833a8a1 100644 --- a/features/steps/project/hooks.rb +++ b/features/steps/project/hooks.rb @@ -23,13 +23,13 @@ class Spinach::Features::ProjectHooks < Spinach::FeatureSteps end step 'I submit new hook' do - @url = FFaker::Internet.uri("http") + @url = 'http://example.org/1' fill_in "hook_url", with: @url expect { click_button "Add Webhook" }.to change(ProjectHook, :count).by(1) end step 'I submit new hook with SSL verification enabled' do - @url = FFaker::Internet.uri("http") + @url = 'http://example.org/2' fill_in "hook_url", with: @url check "hook_enable_ssl_verification" expect { click_button "Add Webhook" }.to change(ProjectHook, :count).by(1) diff --git a/spec/controllers/profiles/personal_access_tokens_spec.rb b/spec/controllers/profiles/personal_access_tokens_spec.rb index dfed1de2046..98a43e278b2 100644 --- a/spec/controllers/profiles/personal_access_tokens_spec.rb +++ b/spec/controllers/profiles/personal_access_tokens_spec.rb @@ -12,7 +12,7 @@ describe Profiles::PersonalAccessTokensController do end it "allows creation of a token with scopes" do - name = FFaker::Product.brand + name = 'My PAT' scopes = %w[api read_user] post :create, personal_access_token: token_attributes.merge(scopes: scopes, name: name) diff --git a/spec/factories/chat_names.rb b/spec/factories/chat_names.rb index 24225468d55..9a0be1a4598 100644 --- a/spec/factories/chat_names.rb +++ b/spec/factories/chat_names.rb @@ -6,11 +6,7 @@ FactoryGirl.define do team_id 'T0001' team_domain 'Awesome Team' - sequence :chat_id do |n| - "U#{n}" - end - sequence :chat_name do |n| - "user#{n}" - end + sequence(:chat_id) { |n| "U#{n}" } + chat_name { generate(:username) } end end diff --git a/spec/factories/chat_teams.rb b/spec/factories/chat_teams.rb index 82f44fa3d15..ffedf69a69b 100644 --- a/spec/factories/chat_teams.rb +++ b/spec/factories/chat_teams.rb @@ -1,9 +1,6 @@ FactoryGirl.define do factory :chat_team, class: ChatTeam do - sequence :team_id do |n| - "abcdefghijklm#{n}" - end - + sequence(:team_id) { |n| "abcdefghijklm#{n}" } namespace factory: :group end end diff --git a/spec/factories/ci/runners.rb b/spec/factories/ci/runners.rb index c3b4aff55ba..05abf60d5ce 100644 --- a/spec/factories/ci/runners.rb +++ b/spec/factories/ci/runners.rb @@ -1,8 +1,6 @@ FactoryGirl.define do factory :ci_runner, class: Ci::Runner do - sequence :description do |n| - "My runner#{n}" - end + sequence(:description) { |n| "My runner#{n}" } platform "darwin" is_shared false diff --git a/spec/factories/emails.rb b/spec/factories/emails.rb index 9794772ac7d..8303861bcfe 100644 --- a/spec/factories/emails.rb +++ b/spec/factories/emails.rb @@ -1,6 +1,6 @@ FactoryGirl.define do factory :email do user - email { FFaker::Internet.email('alias') } + email { generate(:email_alias) } end end diff --git a/spec/factories/issues.rb b/spec/factories/issues.rb index 7e09f1ba8ea..dbd0fff8376 100644 --- a/spec/factories/issues.rb +++ b/spec/factories/issues.rb @@ -1,8 +1,4 @@ FactoryGirl.define do - sequence :issue_created_at do |n| - 4.hours.ago + ( 2 * n ).seconds - end - factory :issue do title author diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb index 5ba8443c62c..5706ab6a767 100644 --- a/spec/factories/labels.rb +++ b/spec/factories/labels.rb @@ -1,6 +1,6 @@ FactoryGirl.define do factory :label, class: ProjectLabel do - sequence(:title) { |n| "label#{n}" } + title { generate(:label) } color "#990000" project factory: :empty_project @@ -16,7 +16,7 @@ FactoryGirl.define do end factory :group_label, class: GroupLabel do - sequence(:title) { |n| "label#{n}" } + title { generate(:label) } color "#990000" group end diff --git a/spec/factories/oauth_applications.rb b/spec/factories/oauth_applications.rb index 86cdc208268..c7ede40f240 100644 --- a/spec/factories/oauth_applications.rb +++ b/spec/factories/oauth_applications.rb @@ -1,8 +1,8 @@ FactoryGirl.define do factory :oauth_application, class: 'Doorkeeper::Application', aliases: [:application] do - name { FFaker::Name.name } + sequence(:name) { |n| "OAuth App #{n}" } uid { Doorkeeper::OAuth::Helpers::UniqueToken.generate } - redirect_uri { FFaker::Internet.uri('http') } + redirect_uri { generate(:url) } owner owner_type 'User' end diff --git a/spec/factories/personal_access_tokens.rb b/spec/factories/personal_access_tokens.rb index 7b15ba47de1..06acaff6cd0 100644 --- a/spec/factories/personal_access_tokens.rb +++ b/spec/factories/personal_access_tokens.rb @@ -2,7 +2,7 @@ FactoryGirl.define do factory :personal_access_token do user token { SecureRandom.hex(50) } - name { FFaker::Product.brand } + sequence(:name) { |n| "PAT #{n}" } revoked false expires_at { 5.days.from_now } scopes ['api'] diff --git a/spec/factories/project_hooks.rb b/spec/factories/project_hooks.rb index 424ecc65759..39c2a9dd1fb 100644 --- a/spec/factories/project_hooks.rb +++ b/spec/factories/project_hooks.rb @@ -1,6 +1,6 @@ FactoryGirl.define do factory :project_hook do - url { FFaker::Internet.uri('http') } + url { generate(:url) } trait :token do token { SecureRandom.hex(10) } diff --git a/spec/factories/sequences.rb b/spec/factories/sequences.rb new file mode 100644 index 00000000000..5b8501aa770 --- /dev/null +++ b/spec/factories/sequences.rb @@ -0,0 +1,10 @@ +FactoryGirl.define do + sequence(:username) { |n| "user#{n}" } + sequence(:name) { |n| "John Doe#{n}" } + sequence(:email) { |n| "user#{n}@example.org" } + sequence(:email_alias) { |n| "user.alias#{n}@example.org" } + sequence(:url) { |n| "http://example#{n}.org" } + sequence(:label) { |n| "label#{n}" } + sequence(:branch) { |n| "my-branch-#{n}" } + sequence(:issue_created_at) { |n| 4.hours.ago + (2 * n).seconds } +end diff --git a/spec/factories/service_hooks.rb b/spec/factories/service_hooks.rb index 6dd6af63f3e..e3f88ab8fcc 100644 --- a/spec/factories/service_hooks.rb +++ b/spec/factories/service_hooks.rb @@ -1,6 +1,6 @@ FactoryGirl.define do factory :service_hook do - url { FFaker::Internet.uri('http') } + url { generate(:url) } service end end diff --git a/spec/factories/snippets.rb b/spec/factories/snippets.rb index 365f12a0c95..fb87c584f0b 100644 --- a/spec/factories/snippets.rb +++ b/spec/factories/snippets.rb @@ -1,11 +1,6 @@ FactoryGirl.define do - sequence :title, aliases: [:content] do - FFaker::Lorem.sentence - end - - sequence :file_name do - FFaker::Internet.user_name - end + sequence(:title, aliases: [:content]) { |n| "My snippet #{n}" } + sequence(:file_name) { |n| "snippet-#{n}.rb" } factory :snippet do author diff --git a/spec/factories/spam_logs.rb b/spec/factories/spam_logs.rb index a4f6d291269..e369f9f13e9 100644 --- a/spec/factories/spam_logs.rb +++ b/spec/factories/spam_logs.rb @@ -1,9 +1,9 @@ FactoryGirl.define do factory :spam_log do user - source_ip { FFaker::Internet.ip_v4_address } + sequence(:source_ip) { |n| "42.42.42.#{n % 255}" } noteable_type 'Issue' - title { FFaker::Lorem.sentence } - description { FFaker::Lorem.paragraph(5) } + sequence(:title) { |n| "Spam title #{n}" } + description { "Spam description\nwith\nmultiple\nlines" } end end diff --git a/spec/factories/system_hooks.rb b/spec/factories/system_hooks.rb index c786e9cb79b..841e1e293e8 100644 --- a/spec/factories/system_hooks.rb +++ b/spec/factories/system_hooks.rb @@ -1,5 +1,5 @@ FactoryGirl.define do factory :system_hook do - url { FFaker::Internet.uri('http') } + url { generate(:url) } end end diff --git a/spec/factories/users.rb b/spec/factories/users.rb index 249dabbaae8..e1ae94a08e4 100644 --- a/spec/factories/users.rb +++ b/spec/factories/users.rb @@ -1,10 +1,8 @@ FactoryGirl.define do - sequence(:name) { FFaker::Name.name } - factory :user, aliases: [:author, :assignee, :recipient, :owner, :creator, :resource_owner] do - email { FFaker::Internet.email } - name - sequence(:username) { |n| "#{FFaker::Internet.user_name}#{n}" } + email { generate(:email) } + name { generate(:name) } + username { generate(:username) } password "12345678" confirmed_at { Time.now } confirmation_token { nil } diff --git a/spec/features/admin/admin_hooks_spec.rb b/spec/features/admin/admin_hooks_spec.rb index f246997d5a2..570c374a89b 100644 --- a/spec/features/admin/admin_hooks_spec.rb +++ b/spec/features/admin/admin_hooks_spec.rb @@ -26,7 +26,7 @@ describe "Admin::Hooks", feature: true do end describe "New Hook" do - let(:url) { FFaker::Internet.uri('http') } + let(:url) { generate(:url) } it 'adds new hook' do visit admin_hooks_path diff --git a/spec/features/admin/admin_users_impersonation_tokens_spec.rb b/spec/features/admin/admin_users_impersonation_tokens_spec.rb index 9ff5c2f9d40..ff23d486355 100644 --- a/spec/features/admin/admin_users_impersonation_tokens_spec.rb +++ b/spec/features/admin/admin_users_impersonation_tokens_spec.rb @@ -16,7 +16,7 @@ describe 'Admin > Users > Impersonation Tokens', feature: true, js: true do describe "token creation" do it "allows creation of a token" do - name = FFaker::Product.brand + name = 'Hello World' visit admin_user_impersonation_tokens_path(user_id: user.username) fill_in "Name", with: name diff --git a/spec/features/issuables/issuable_list_spec.rb b/spec/features/issuables/issuable_list_spec.rb index b90bf6268fd..f324354dd46 100644 --- a/spec/features/issuables/issuable_list_spec.rb +++ b/spec/features/issuables/issuable_list_spec.rb @@ -46,16 +46,16 @@ describe 'issuable list', feature: true do end def create_issuables(issuable_type) - 3.times do + 3.times do |n| issuable = if issuable_type == :issue create(:issue, project: project, author: user) else - create(:merge_request, title: FFaker::Lorem.sentence, source_project: project, source_branch: FFaker::Name.name) + create(:merge_request, source_project: project, source_branch: "#{n}-feature") end 2.times do - create(:note_on_issue, noteable: issuable, project: project, note: 'Test note') + create(:note_on_issue, noteable: issuable, project: project) end create(:award_emoji, :downvote, awardable: issuable) @@ -65,9 +65,8 @@ describe 'issuable list', feature: true do if issuable_type == :issue issue = Issue.reorder(:iid).first merge_request = create(:merge_request, - title: FFaker::Lorem.sentence, source_project: project, - source_branch: FFaker::Name.name) + source_branch: 'my-bug-fix') MergeRequestsClosingIssues.create!(issue: issue, merge_request: merge_request) end diff --git a/spec/features/profiles/personal_access_tokens_spec.rb b/spec/features/profiles/personal_access_tokens_spec.rb index 0917d4dc3ef..99fba594651 100644 --- a/spec/features/profiles/personal_access_tokens_spec.rb +++ b/spec/features/profiles/personal_access_tokens_spec.rb @@ -27,7 +27,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do describe "token creation" do it "allows creation of a personal access token" do - name = FFaker::Product.brand + name = 'My PAT' visit profile_personal_access_tokens_path fill_in "Name", with: name @@ -52,7 +52,7 @@ describe 'Profile > Personal Access Tokens', feature: true, js: true do it "displays an error message" do disallow_personal_access_token_saves! visit profile_personal_access_tokens_path - fill_in "Name", with: FFaker::Product.brand + fill_in "Name", with: 'My PAT' expect { click_on "Create Personal Access Token" }.not_to change { PersonalAccessToken.count } expect(page).to have_content("Name cannot be nil") diff --git a/spec/features/u2f_spec.rb b/spec/features/u2f_spec.rb index a8d00bb8e5a..28373098123 100644 --- a/spec/features/u2f_spec.rb +++ b/spec/features/u2f_spec.rb @@ -1,6 +1,6 @@ require 'spec_helper' -feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: true, js: true do +feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', :js do include WaitForAjax before { allow_any_instance_of(U2fHelper).to receive(:inject_u2f_api?).and_return(true) } @@ -11,8 +11,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: wait_for_ajax end - def register_u2f_device(u2f_device = nil) - name = FFaker::Name.first_name + def register_u2f_device(u2f_device = nil, name: 'My device') u2f_device ||= FakeU2fDevice.new(page, name) u2f_device.respond_to_u2f_registration click_on 'Setup New U2F Device' @@ -62,7 +61,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: expect(page).to have_content('Your U2F device was registered') # Second device - second_device = register_u2f_device + second_device = register_u2f_device(name: 'My other device') expect(page).to have_content('Your U2F device was registered') expect(page).to have_content(first_device.name) @@ -76,7 +75,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: expect(page).to have_content("You've already enabled two-factor authentication using mobile") first_u2f_device = register_u2f_device - second_u2f_device = register_u2f_device + second_u2f_device = register_u2f_device(name: 'My other device') click_on "Delete", match: :first @@ -99,7 +98,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: user.update_attribute(:otp_required_for_login, true) visit profile_account_path manage_two_factor_authentication - register_u2f_device(u2f_device) + register_u2f_device(u2f_device, name: 'My other device') expect(page).to have_content('Your U2F device was registered') expect(U2fRegistration.count).to eq(2) @@ -198,7 +197,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: current_user.update_attribute(:otp_required_for_login, true) visit profile_account_path manage_two_factor_authentication - register_u2f_device + register_u2f_device(name: 'My other device') logout # Try authenticating user with the old U2F device @@ -231,7 +230,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: describe "when a given U2F device has not been registered" do it "does not allow logging in with that particular device" do - unregistered_device = FakeU2fDevice.new(page, FFaker::Name.first_name) + unregistered_device = FakeU2fDevice.new(page, 'My device') login_as(user) unregistered_device.respond_to_u2f_authentication expect(page).to have_content('We heard back from your U2F device') @@ -252,7 +251,7 @@ feature 'Using U2F (Universal 2nd Factor) Devices for Authentication', feature: # Register second device visit profile_two_factor_auth_path expect(page).to have_content("Your U2F device needs to be set up.") - second_device = register_u2f_device + second_device = register_u2f_device(name: 'My other device') logout # Authenticate as both devices diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 48f7754bed8..703b41f95ac 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -183,7 +183,7 @@ describe Gitlab::GitAccess, lib: true do describe '#check_push_access!' do before { merge_into_protected_branch } - let(:unprotected_branch) { FFaker::Internet.user_name } + let(:unprotected_branch) { 'unprotected_branch' } let(:changes) do { push_new_branch: "#{Gitlab::Git::BLANK_SHA} 570e7b2ab refs/heads/wow", @@ -211,9 +211,9 @@ describe Gitlab::GitAccess, lib: true do target_branch = project.repository.lookup('feature') source_branch = project.repository.create_file( user, - FFaker::InternetSE.login_user_name, - FFaker::HipsterIpsum.paragraph, - message: FFaker::HipsterIpsum.sentence, + 'John Doe', + 'This is the file content', + message: 'This is a good commit message', branch_name: unprotected_branch) rugged = project.repository.rugged author = { email: "email@example.com", time: Time.now, name: "Example Git User" } diff --git a/spec/lib/gitlab/git_spec.rb b/spec/lib/gitlab/git_spec.rb index 8eaf7aac264..36f0e6507c8 100644 --- a/spec/lib/gitlab/git_spec.rb +++ b/spec/lib/gitlab/git_spec.rb @@ -1,21 +1,8 @@ require 'spec_helper' describe Gitlab::Git, lib: true do - let(:committer_email) { FFaker::Internet.email } - - # I have to remove periods from the end of the name - # This happened when the user's name had a suffix (i.e. "Sr.") - # This seems to be what git does under the hood. For example, this commit: - # - # $ git commit --author='Foo Sr. ' -m 'Where's my trailing period?' - # - # results in this: - # - # $ git show --pretty - # ... - # Author: Foo Sr - # ... - let(:committer_name) { FFaker::Name.name.chomp("\.") } + let(:committer_email) { 'user@example.org' } + let(:committer_name) { 'John Doe' } describe 'committer_hash' do it "returns a hash containing the given email and name" do diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb index f60c5ffb32a..6a89b007f96 100644 --- a/spec/mailers/notify_spec.rb +++ b/spec/mailers/notify_spec.rb @@ -37,7 +37,7 @@ describe Notify do context 'for issues' do let(:issue) { create(:issue, author: current_user, assignee: assignee, project: project) } - let(:issue_with_description) { create(:issue, author: current_user, assignee: assignee, project: project, description: FFaker::Lorem.sentence) } + let(:issue_with_description) { create(:issue, author: current_user, assignee: assignee, project: project, description: 'My awesome description') } describe 'that are new' do subject { Notify.new_issue_email(issue.assignee_id, issue.id) } @@ -187,7 +187,7 @@ describe Notify do let(:project) { create(:project, :repository) } let(:merge_author) { create(:user) } let(:merge_request) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project) } - let(:merge_request_with_description) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project, description: FFaker::Lorem.sentence) } + let(:merge_request_with_description) { create(:merge_request, author: current_user, assignee: assignee, source_project: project, target_project: project, description: 'My awesome description') } describe 'that are new' do subject { Notify.new_merge_request_email(merge_request.assignee_id, merge_request.id) } diff --git a/spec/models/cycle_analytics/plan_spec.rb b/spec/models/cycle_analytics/plan_spec.rb index 55483fc876a..4f33f3c6d69 100644 --- a/spec/models/cycle_analytics/plan_spec.rb +++ b/spec/models/cycle_analytics/plan_spec.rb @@ -13,7 +13,7 @@ describe 'CycleAnalytics#plan', feature: true do data_fn: -> (context) do { issue: context.create(:issue, project: context.project), - branch_name: context.random_git_name + branch_name: context.generate(:branch) } end, start_time_conditions: [["issue associated with a milestone", @@ -35,7 +35,7 @@ describe 'CycleAnalytics#plan', feature: true do context "when a regular label (instead of a list label) is added to the issue" do it "returns nil" do - branch_name = random_git_name + branch_name = generate(:branch) label = create(:label) issue = create(:issue, project: project) issue.update(label_ids: [label.id]) diff --git a/spec/models/cycle_analytics/production_spec.rb b/spec/models/cycle_analytics/production_spec.rb index e6a826a9418..4744b9e05ea 100644 --- a/spec/models/cycle_analytics/production_spec.rb +++ b/spec/models/cycle_analytics/production_spec.rb @@ -23,7 +23,7 @@ describe 'CycleAnalytics#production', feature: true do # Make other changes on master sha = context.project.repository.create_file( context.user, - context.random_git_name, + context.generate(:branch), 'content', message: 'commit message', branch_name: 'master') diff --git a/spec/models/cycle_analytics/staging_spec.rb b/spec/models/cycle_analytics/staging_spec.rb index 3a02ed81adb..f78d7a23105 100644 --- a/spec/models/cycle_analytics/staging_spec.rb +++ b/spec/models/cycle_analytics/staging_spec.rb @@ -28,7 +28,7 @@ describe 'CycleAnalytics#staging', feature: true do # Make other changes on master sha = context.project.repository.create_file( context.user, - context.random_git_name, + context.generate(:branch), 'content', message: 'commit message', branch_name: 'master') diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index df742ee8084..5899e121e68 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -24,21 +24,8 @@ describe Repository, models: true do repository.commit(merge_commit_id) end - let(:author_email) { FFaker::Internet.email } - - # I have to remove periods from the end of the name - # This happened when the user's name had a suffix (i.e. "Sr.") - # This seems to be what git does under the hood. For example, this commit: - # - # $ git commit --author='Foo Sr. ' -m 'Where's my trailing period?' - # - # results in this: - # - # $ git show --pretty - # ... - # Author: Foo Sr - # ... - let(:author_name) { FFaker::Name.name.chomp("\.") } + let(:author_email) { 'user@example.org' } + let(:author_name) { 'John Doe' } describe '#branch_names_contains' do subject { repository.branch_names_contains(sample_commit.id) } diff --git a/spec/requests/api/files_spec.rb b/spec/requests/api/files_spec.rb index a7fad7f0bdb..8012530f139 100644 --- a/spec/requests/api/files_spec.rb +++ b/spec/requests/api/files_spec.rb @@ -11,21 +11,8 @@ describe API::Files, api: true do ref: 'master' } end - let(:author_email) { FFaker::Internet.email } - - # I have to remove periods from the end of the name - # This happened when the user's name had a suffix (i.e. "Sr.") - # This seems to be what git does under the hood. For example, this commit: - # - # $ git commit --author='Foo Sr. ' -m 'Where's my trailing period?' - # - # results in this: - # - # $ git show --pretty - # ... - # Author: Foo Sr - # ... - let(:author_name) { FFaker::Name.name.chomp("\.") } + let(:author_email) { 'user@example.org' } + let(:author_name) { 'John Doe' } before { project.team << [user, :developer] } diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index a3de4702ad0..2e291eb3cea 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -341,7 +341,6 @@ describe API::Projects, :api do it "assigns attributes to project" do project = attributes_for(:project, { path: 'camelCasePath', - description: FFaker::Lorem.sentence, issues_enabled: false, merge_requests_enabled: false, wiki_enabled: false, @@ -475,7 +474,6 @@ describe API::Projects, :api do it 'assigns attributes to project' do project = attributes_for(:project, { - description: FFaker::Lorem.sentence, issues_enabled: false, merge_requests_enabled: false, wiki_enabled: false, diff --git a/spec/requests/api/v3/files_spec.rb b/spec/requests/api/v3/files_spec.rb index 3b61139a2cd..349fd6b3415 100644 --- a/spec/requests/api/v3/files_spec.rb +++ b/spec/requests/api/v3/files_spec.rb @@ -26,8 +26,8 @@ describe API::V3::Files, api: true do ref: 'master' } end - let(:author_email) { FFaker::Internet.email } - let(:author_name) { FFaker::Name.name.chomp("\.") } + let(:author_email) { 'user@example.org' } + let(:author_name) { 'John Doe' } before { project.team << [user, :developer] } diff --git a/spec/requests/api/v3/projects_spec.rb b/spec/requests/api/v3/projects_spec.rb index b1aa793ec00..40531fe7545 100644 --- a/spec/requests/api/v3/projects_spec.rb +++ b/spec/requests/api/v3/projects_spec.rb @@ -356,7 +356,6 @@ describe API::V3::Projects, api: true do it "assigns attributes to project" do project = attributes_for(:project, { path: 'camelCasePath', - description: FFaker::Lorem.sentence, issues_enabled: false, merge_requests_enabled: false, wiki_enabled: false, @@ -501,7 +500,6 @@ describe API::V3::Projects, api: true do it 'assigns attributes to project' do project = attributes_for(:project, { - description: FFaker::Lorem.sentence, issues_enabled: false, merge_requests_enabled: false, wiki_enabled: false, diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb index c864a705ca4..37ee4d55769 100644 --- a/spec/support/cycle_analytics_helpers.rb +++ b/spec/support/cycle_analytics_helpers.rb @@ -1,15 +1,13 @@ module CycleAnalyticsHelpers - def create_commit_referencing_issue(issue, branch_name: random_git_name) - project.repository.add_branch(user, branch_name, 'master') + def create_commit_referencing_issue(issue, branch_name: nil) + project.repository.add_branch(user, branch_name || generate(:branch), 'master') create_commit("Commit for ##{issue.iid}", issue.project, user, branch_name) end def create_commit(message, project, user, branch_name, count: 1) oldrev = project.repository.commit(branch_name).sha commit_shas = Array.new(count) do |index| - filename = random_git_name - - commit_sha = project.repository.create_file(user, filename, "content", message: message, branch_name: branch_name) + commit_sha = project.repository.create_file(user, generate(:branch), "content", message: message, branch_name: branch_name) project.repository.commit(commit_sha) commit_sha @@ -24,13 +22,13 @@ module CycleAnalyticsHelpers def create_merge_request_closing_issue(issue, message: nil, source_branch: nil) if !source_branch || project.repository.commit(source_branch).blank? - source_branch = random_git_name + source_branch = generate(:branch) project.repository.add_branch(user, source_branch, 'master') end sha = project.repository.create_file( user, - random_git_name, + generate(:branch), 'content', message: 'commit message', branch_name: source_branch) diff --git a/spec/support/git_helpers.rb b/spec/support/git_helpers.rb deleted file mode 100644 index 93422390ef7..00000000000 --- a/spec/support/git_helpers.rb +++ /dev/null @@ -1,9 +0,0 @@ -module GitHelpers - def random_git_name - "#{FFaker::Product.brand}-#{FFaker::Product.brand}-#{rand(1000)}" - end -end - -RSpec.configure do |config| - config.include GitHelpers -end diff --git a/spec/support/issuables_list_metadata_shared_examples.rb b/spec/support/issuables_list_metadata_shared_examples.rb index 4c0f556e736..7ea4073ef2b 100644 --- a/spec/support/issuables_list_metadata_shared_examples.rb +++ b/spec/support/issuables_list_metadata_shared_examples.rb @@ -2,12 +2,12 @@ shared_examples 'issuables list meta-data' do |issuable_type, action = nil| before do @issuable_ids = [] - 2.times do + 2.times do |n| issuable = if issuable_type == :issue create(issuable_type, project: project) else - create(issuable_type, title: FFaker::Lorem.sentence, source_project: project, source_branch: FFaker::Name.name) + create(issuable_type, source_project: project, source_branch: "#{n}-feature") end @issuable_ids << issuable.id diff --git a/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb b/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb index ee492daee30..9e9cdf3e48b 100644 --- a/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb +++ b/spec/support/services/issuable_create_service_slash_commands_shared_examples.rb @@ -7,7 +7,7 @@ shared_examples 'new issuable record that supports slash commands' do let(:assignee) { create(:user) } let!(:milestone) { create(:milestone, project: project) } let!(:labels) { create_list(:label, 3, project: project) } - let(:base_params) { { title: FFaker::Lorem.sentence(3) } } + let(:base_params) { { title: 'My issuable title' } } let(:params) { base_params.merge(defined?(default_params) ? default_params : {}).merge(example_params) } let(:issuable) { described_class.new(project, user, params).execute } -- cgit v1.2.1 From 5f7cb26394921535a6e1e15f3baee7666c3ef654 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 23 Mar 2017 17:23:51 +0100 Subject: Fix the AbuseReport seeder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- db/fixtures/development/18_abuse_reports.rb | 26 +++++++++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/db/fixtures/development/18_abuse_reports.rb b/db/fixtures/development/18_abuse_reports.rb index 8618d10387a..7e67262b59e 100644 --- a/db/fixtures/development/18_abuse_reports.rb +++ b/db/fixtures/development/18_abuse_reports.rb @@ -1,5 +1,25 @@ -require 'factory_girl_rails' +module Db + module Fixtures + class Development + def self.seed + Gitlab::Seeder.quiet do + (AbuseReport.default_per_page + 3).times do + reported_user = + User.create!( + username: FFaker::Internet.user_name, + name: FFaker::Name.name, + email: FFaker::Internet.email, + confirmed_at: DateTime.now, + password: '12345678' + ) -(AbuseReport.default_per_page + 3).times do - FactoryGirl.create(:abuse_report) + AbuseReport.create(reporter: User.take, user: reported_user, message: 'User sends spam') + print '.' + end + end + end + end + end end + +Db::Fixtures::Development.seed -- cgit v1.2.1 From 169dc4cec1f45cdcc548e3e682e4fd6f4c98926a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Thu, 23 Mar 2017 17:37:14 +0100 Subject: Fix brittle specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- spec/factories/issues.rb | 2 +- spec/factories/merge_requests.rb | 2 +- spec/factories/sequences.rb | 2 ++ spec/factories/snippets.rb | 9 +++------ spec/features/admin/admin_browse_spam_logs_spec.rb | 2 +- spec/features/issuables/issuable_list_spec.rb | 4 ++-- spec/support/cycle_analytics_helpers.rb | 4 ++-- spec/support/filter_spec_helper.rb | 4 ++-- 8 files changed, 14 insertions(+), 15 deletions(-) diff --git a/spec/factories/issues.rb b/spec/factories/issues.rb index dbd0fff8376..0b6977e3b17 100644 --- a/spec/factories/issues.rb +++ b/spec/factories/issues.rb @@ -1,6 +1,6 @@ FactoryGirl.define do factory :issue do - title + title { generate(:title) } author project factory: :empty_project diff --git a/spec/factories/merge_requests.rb b/spec/factories/merge_requests.rb index ae0bbbd6aeb..e36fe326e1c 100644 --- a/spec/factories/merge_requests.rb +++ b/spec/factories/merge_requests.rb @@ -1,6 +1,6 @@ FactoryGirl.define do factory :merge_request do - title + title { generate(:title) } author association :source_project, :repository, factory: :project target_project { source_project } diff --git a/spec/factories/sequences.rb b/spec/factories/sequences.rb index 5b8501aa770..2321455bfee 100644 --- a/spec/factories/sequences.rb +++ b/spec/factories/sequences.rb @@ -3,6 +3,8 @@ FactoryGirl.define do sequence(:name) { |n| "John Doe#{n}" } sequence(:email) { |n| "user#{n}@example.org" } sequence(:email_alias) { |n| "user.alias#{n}@example.org" } + sequence(:title) { |n| "My title #{n}" } + sequence(:filename) { |n| "filename-#{n}.rb" } sequence(:url) { |n| "http://example#{n}.org" } sequence(:label) { |n| "label#{n}" } sequence(:branch) { |n| "my-branch-#{n}" } diff --git a/spec/factories/snippets.rb b/spec/factories/snippets.rb index fb87c584f0b..18cb0f5de26 100644 --- a/spec/factories/snippets.rb +++ b/spec/factories/snippets.rb @@ -1,12 +1,9 @@ FactoryGirl.define do - sequence(:title, aliases: [:content]) { |n| "My snippet #{n}" } - sequence(:file_name) { |n| "snippet-#{n}.rb" } - factory :snippet do author - title - content - file_name + title { generate(:title) } + content { generate(:title) } + file_name { generate(:filename) } trait :public do visibility_level Snippet::PUBLIC diff --git a/spec/features/admin/admin_browse_spam_logs_spec.rb b/spec/features/admin/admin_browse_spam_logs_spec.rb index 562ace92598..bee57472270 100644 --- a/spec/features/admin/admin_browse_spam_logs_spec.rb +++ b/spec/features/admin/admin_browse_spam_logs_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe 'Admin browse spam logs' do - let!(:spam_log) { create(:spam_log) } + let!(:spam_log) { create(:spam_log, description: 'abcde ' * 20) } before do login_as :admin diff --git a/spec/features/issuables/issuable_list_spec.rb b/spec/features/issuables/issuable_list_spec.rb index f324354dd46..3dc872ae520 100644 --- a/spec/features/issuables/issuable_list_spec.rb +++ b/spec/features/issuables/issuable_list_spec.rb @@ -51,7 +51,7 @@ describe 'issuable list', feature: true do if issuable_type == :issue create(:issue, project: project, author: user) else - create(:merge_request, source_project: project, source_branch: "#{n}-feature") + create(:merge_request, source_project: project, source_branch: generate(:branch)) end 2.times do @@ -66,7 +66,7 @@ describe 'issuable list', feature: true do issue = Issue.reorder(:iid).first merge_request = create(:merge_request, source_project: project, - source_branch: 'my-bug-fix') + source_branch: generate(:branch)) MergeRequestsClosingIssues.create!(issue: issue, merge_request: merge_request) end diff --git a/spec/support/cycle_analytics_helpers.rb b/spec/support/cycle_analytics_helpers.rb index 37ee4d55769..8ad042f5e3b 100644 --- a/spec/support/cycle_analytics_helpers.rb +++ b/spec/support/cycle_analytics_helpers.rb @@ -1,6 +1,6 @@ module CycleAnalyticsHelpers - def create_commit_referencing_issue(issue, branch_name: nil) - project.repository.add_branch(user, branch_name || generate(:branch), 'master') + def create_commit_referencing_issue(issue, branch_name: generate(:branch)) + project.repository.add_branch(user, branch_name, 'master') create_commit("Commit for ##{issue.iid}", issue.project, user, branch_name) end diff --git a/spec/support/filter_spec_helper.rb b/spec/support/filter_spec_helper.rb index a8e454eb09e..b871b7ffc90 100644 --- a/spec/support/filter_spec_helper.rb +++ b/spec/support/filter_spec_helper.rb @@ -63,9 +63,9 @@ module FilterSpecHelper # # Returns a String def invalidate_reference(reference) - if reference =~ /\A(.+)?.\d+\z/ + if reference =~ /\A(.+)?[^\d]\d+\z/ # Integer-based reference with optional project prefix - reference.gsub(/\d+\z/) { |i| i.to_i + 1 } + reference.gsub(/\d+\z/) { |i| i.to_i + 10_000 } elsif reference =~ /\A(.+@)?(\h{7,40}\z)/ # SHA-based reference with optional prefix reference.gsub(/\h{7,40}\z/) { |v| v.reverse } -- cgit v1.2.1 From 58fe40fbd38dce5d42f5f36681780579fb3f031b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Fri, 24 Mar 2017 19:12:22 +0100 Subject: Don't use FFaker in factories, use sequences instead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- db/fixtures/development/18_abuse_reports.rb | 32 +++++++++++++++-------------- spec/factories/labels.rb | 11 +++++----- spec/factories/sequences.rb | 4 ++-- spec/requests/api/issues_spec.rb | 6 +++--- spec/requests/api/v3/issues_spec.rb | 6 +++--- 5 files changed, 31 insertions(+), 28 deletions(-) diff --git a/db/fixtures/development/18_abuse_reports.rb b/db/fixtures/development/18_abuse_reports.rb index 7e67262b59e..762fab5f0ab 100644 --- a/db/fixtures/development/18_abuse_reports.rb +++ b/db/fixtures/development/18_abuse_reports.rb @@ -1,20 +1,22 @@ module Db module Fixtures - class Development - def self.seed - Gitlab::Seeder.quiet do - (AbuseReport.default_per_page + 3).times do - reported_user = - User.create!( - username: FFaker::Internet.user_name, - name: FFaker::Name.name, - email: FFaker::Internet.email, - confirmed_at: DateTime.now, - password: '12345678' - ) + module Development + class AbuseReport + def self.seed + Gitlab::Seeder.quiet do + (::AbuseReport.default_per_page + 3).times do + reported_user = + ::User.create!( + username: FFaker::Internet.user_name, + name: FFaker::Name.name, + email: FFaker::Internet.email, + confirmed_at: DateTime.now, + password: '12345678' + ) - AbuseReport.create(reporter: User.take, user: reported_user, message: 'User sends spam') - print '.' + ::AbuseReport.create(reporter: ::User.take, user: reported_user, message: 'User sends spam') + print '.' + end end end end @@ -22,4 +24,4 @@ module Db end end -Db::Fixtures::Development.seed +Db::Fixtures::Development::AbuseReport.seed diff --git a/spec/factories/labels.rb b/spec/factories/labels.rb index 5706ab6a767..22c2a1f15e2 100644 --- a/spec/factories/labels.rb +++ b/spec/factories/labels.rb @@ -1,7 +1,10 @@ FactoryGirl.define do - factory :label, class: ProjectLabel do - title { generate(:label) } + trait :base_label do + title { generate(:label_title) } color "#990000" + end + + factory :label, traits: [:base_label], class: ProjectLabel do project factory: :empty_project transient do @@ -15,9 +18,7 @@ FactoryGirl.define do end end - factory :group_label, class: GroupLabel do - title { generate(:label) } - color "#990000" + factory :group_label, traits: [:base_label] do group end end diff --git a/spec/factories/sequences.rb b/spec/factories/sequences.rb index 2321455bfee..c0232ba5bf6 100644 --- a/spec/factories/sequences.rb +++ b/spec/factories/sequences.rb @@ -6,7 +6,7 @@ FactoryGirl.define do sequence(:title) { |n| "My title #{n}" } sequence(:filename) { |n| "filename-#{n}.rb" } sequence(:url) { |n| "http://example#{n}.org" } - sequence(:label) { |n| "label#{n}" } + sequence(:label_title) { |n| "label#{n}" } sequence(:branch) { |n| "my-branch-#{n}" } - sequence(:issue_created_at) { |n| 4.hours.ago + (2 * n).seconds } + sequence(:past_time) { |n| 4.hours.ago + (2 * n).seconds } end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index 91d6fb83c0b..4729adba11c 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -19,7 +19,7 @@ describe API::Issues, api: true do project: project, state: :closed, milestone: milestone, - created_at: generate(:issue_created_at), + created_at: generate(:past_time), updated_at: 3.hours.ago end let!(:confidential_issue) do @@ -28,7 +28,7 @@ describe API::Issues, api: true do project: project, author: author, assignee: assignee, - created_at: generate(:issue_created_at), + created_at: generate(:past_time), updated_at: 2.hours.ago end let!(:issue) do @@ -37,7 +37,7 @@ describe API::Issues, api: true do assignee: user, project: project, milestone: milestone, - created_at: generate(:issue_created_at), + created_at: generate(:past_time), updated_at: 1.hour.ago end let!(:label) do diff --git a/spec/requests/api/v3/issues_spec.rb b/spec/requests/api/v3/issues_spec.rb index 383871d5c38..b1b398a897e 100644 --- a/spec/requests/api/v3/issues_spec.rb +++ b/spec/requests/api/v3/issues_spec.rb @@ -19,7 +19,7 @@ describe API::V3::Issues, api: true do project: project, state: :closed, milestone: milestone, - created_at: generate(:issue_created_at), + created_at: generate(:past_time), updated_at: 3.hours.ago end let!(:confidential_issue) do @@ -28,7 +28,7 @@ describe API::V3::Issues, api: true do project: project, author: author, assignee: assignee, - created_at: generate(:issue_created_at), + created_at: generate(:past_time), updated_at: 2.hours.ago end let!(:issue) do @@ -37,7 +37,7 @@ describe API::V3::Issues, api: true do assignee: user, project: project, milestone: milestone, - created_at: generate(:issue_created_at), + created_at: generate(:past_time), updated_at: 1.hour.ago end let!(:label) do -- cgit v1.2.1 From 5388bb1cca501442be05ddf929fae7d1bfcbd1f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 3 Apr 2017 18:51:29 +0200 Subject: Ensure the AbuseReport fixtures create unique reported users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- db/fixtures/development/18_abuse_reports.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/fixtures/development/18_abuse_reports.rb b/db/fixtures/development/18_abuse_reports.rb index 762fab5f0ab..a3d33706d8e 100644 --- a/db/fixtures/development/18_abuse_reports.rb +++ b/db/fixtures/development/18_abuse_reports.rb @@ -7,7 +7,7 @@ module Db (::AbuseReport.default_per_page + 3).times do reported_user = ::User.create!( - username: FFaker::Internet.user_name, + username: "#{FFaker::Internet.user_name}-reported", name: FFaker::Name.name, email: FFaker::Internet.email, confirmed_at: DateTime.now, -- cgit v1.2.1 From bf69e6291d7c7029c18f21a65abc2579e1825cb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 3 Apr 2017 18:53:44 +0200 Subject: Ensure user has a unique username otherwise `user10` would match `user1` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- spec/requests/api/members_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index 2d37d026a39..84dca51801f 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe API::Members, api: true do include ApiHelpers - let(:master) { create(:user) } + let(:master) { create(:user, username: 'master_user') } let(:developer) { create(:user) } let(:access_requester) { create(:user) } let(:stranger) { create(:user) } -- cgit v1.2.1 From f564cbb21ac31da88841ad93d6051d90677b7c12 Mon Sep 17 00:00:00 2001 From: Bryce Johnson Date: Mon, 3 Apr 2017 17:54:40 +0000 Subject: Make file templates easy to use and discover --- .../javascripts/blob/file_template_mediator.js | 241 +++++++++++++++++++++ .../javascripts/blob/file_template_selector.js | 60 +++++ app/assets/javascripts/blob/template_selector.js | 92 ++++++++ .../template_selectors/blob_ci_yaml_selector.js | 9 - .../template_selectors/blob_ci_yaml_selectors.js | 23 -- .../template_selectors/blob_dockerfile_selector.js | 9 - .../blob_dockerfile_selectors.js | 23 -- .../template_selectors/blob_gitignore_selector.js | 9 - .../template_selectors/blob_gitignore_selectors.js | 23 -- .../template_selectors/blob_license_selector.js | 13 -- .../template_selectors/blob_license_selectors.js | 24 -- .../blob/template_selectors/ci_yaml_selector.js | 32 +++ .../blob/template_selectors/dockerfile_selector.js | 32 +++ .../blob/template_selectors/gitignore_selector.js | 31 +++ .../blob/template_selectors/license_selector.js | 38 ++++ .../blob/template_selectors/template_selector.js | 92 -------- .../blob/template_selectors/type_selector.js | 25 +++ app/assets/javascripts/blob_edit/blob_bundle.js | 3 +- app/assets/javascripts/blob_edit/edit_blob.js | 38 +--- .../templates/issuable_template_selector.js | 2 +- app/assets/stylesheets/pages/editor.scss | 142 +++++++++--- app/views/projects/blob/_editor.html.haml | 20 +- .../projects/blob/_template_selectors.html.haml | 17 ++ app/views/projects/blob/edit.html.haml | 7 +- app/views/projects/blob/new.html.haml | 8 +- ...ake-file-templates-easy-to-use-and-discover.yml | 4 + spec/features/auto_deploy_spec.rb | 4 +- spec/features/projects/blobs/user_create_spec.rb | 2 +- .../project_owner_creates_license_file_spec.rb | 4 +- ...to_create_license_file_in_empty_project_spec.rb | 4 +- .../projects/files/template_type_dropdown_spec.rb | 135 ++++++++++++ spec/features/projects/files/undo_template_spec.rb | 67 ++++++ 32 files changed, 924 insertions(+), 309 deletions(-) create mode 100644 app/assets/javascripts/blob/file_template_mediator.js create mode 100644 app/assets/javascripts/blob/file_template_selector.js create mode 100644 app/assets/javascripts/blob/template_selector.js delete mode 100644 app/assets/javascripts/blob/template_selectors/blob_ci_yaml_selector.js delete mode 100644 app/assets/javascripts/blob/template_selectors/blob_ci_yaml_selectors.js delete mode 100644 app/assets/javascripts/blob/template_selectors/blob_dockerfile_selector.js delete mode 100644 app/assets/javascripts/blob/template_selectors/blob_dockerfile_selectors.js delete mode 100644 app/assets/javascripts/blob/template_selectors/blob_gitignore_selector.js delete mode 100644 app/assets/javascripts/blob/template_selectors/blob_gitignore_selectors.js delete mode 100644 app/assets/javascripts/blob/template_selectors/blob_license_selector.js delete mode 100644 app/assets/javascripts/blob/template_selectors/blob_license_selectors.js create mode 100644 app/assets/javascripts/blob/template_selectors/ci_yaml_selector.js create mode 100644 app/assets/javascripts/blob/template_selectors/dockerfile_selector.js create mode 100644 app/assets/javascripts/blob/template_selectors/gitignore_selector.js create mode 100644 app/assets/javascripts/blob/template_selectors/license_selector.js delete mode 100644 app/assets/javascripts/blob/template_selectors/template_selector.js create mode 100644 app/assets/javascripts/blob/template_selectors/type_selector.js create mode 100644 app/views/projects/blob/_template_selectors.html.haml create mode 100644 changelogs/unreleased/25332-make-file-templates-easy-to-use-and-discover.yml create mode 100644 spec/features/projects/files/template_type_dropdown_spec.rb create mode 100644 spec/features/projects/files/undo_template_spec.rb diff --git a/app/assets/javascripts/blob/file_template_mediator.js b/app/assets/javascripts/blob/file_template_mediator.js new file mode 100644 index 00000000000..3062cd51ee3 --- /dev/null +++ b/app/assets/javascripts/blob/file_template_mediator.js @@ -0,0 +1,241 @@ +/* eslint-disable class-methods-use-this */ +/* global Flash */ + +import FileTemplateTypeSelector from './template_selectors/type_selector'; +import BlobCiYamlSelector from './template_selectors/ci_yaml_selector'; +import DockerfileSelector from './template_selectors/dockerfile_selector'; +import GitignoreSelector from './template_selectors/gitignore_selector'; +import LicenseSelector from './template_selectors/license_selector'; + +export default class FileTemplateMediator { + constructor({ editor, currentAction }) { + this.editor = editor; + this.currentAction = currentAction; + + this.initTemplateSelectors(); + this.initTemplateTypeSelector(); + this.initDomElements(); + this.initDropdowns(); + this.initPageEvents(); + } + + initTemplateSelectors() { + // Order dictates template type dropdown item order + this.templateSelectors = [ + GitignoreSelector, + BlobCiYamlSelector, + DockerfileSelector, + LicenseSelector, + ].map(TemplateSelectorClass => new TemplateSelectorClass({ mediator: this })); + } + + initTemplateTypeSelector() { + this.typeSelector = new FileTemplateTypeSelector({ + mediator: this, + dropdownData: this.templateSelectors + .map((templateSelector) => { + const cfg = templateSelector.config; + + return { + name: cfg.name, + key: cfg.key, + }; + }), + }); + } + + initDomElements() { + const $templatesMenu = $('.template-selectors-menu'); + const $undoMenu = $templatesMenu.find('.template-selectors-undo-menu'); + const $fileEditor = $('.file-editor'); + + this.$templatesMenu = $templatesMenu; + this.$undoMenu = $undoMenu; + this.$undoBtn = $undoMenu.find('button'); + this.$templateSelectors = $templatesMenu.find('.template-selector-dropdowns-wrap'); + this.$filenameInput = $fileEditor.find('.js-file-path-name-input'); + this.$fileContent = $fileEditor.find('#file-content'); + this.$commitForm = $fileEditor.find('form'); + this.$navLinks = $fileEditor.find('.nav-links'); + } + + initDropdowns() { + if (this.currentAction === 'create') { + this.typeSelector.show(); + } else { + this.hideTemplateSelectorMenu(); + } + + this.displayMatchedTemplateSelector(); + } + + initPageEvents() { + this.listenForFilenameInput(); + this.prepFileContentForSubmit(); + this.listenForPreviewMode(); + } + + listenForFilenameInput() { + this.$filenameInput.on('keyup blur', () => { + this.displayMatchedTemplateSelector(); + }); + } + + prepFileContentForSubmit() { + this.$commitForm.submit(() => { + this.$fileContent.val(this.editor.getValue()); + }); + } + + listenForPreviewMode() { + this.$navLinks.on('click', 'a', (e) => { + const urlPieces = e.target.href.split('#'); + const hash = urlPieces[1]; + if (hash === 'preview') { + this.hideTemplateSelectorMenu(); + } else if (hash === 'editor') { + this.showTemplateSelectorMenu(); + } + }); + } + + selectTemplateType(item, el, e) { + if (e) { + e.preventDefault(); + } + + this.templateSelectors.forEach((selector) => { + if (selector.config.key === item.key) { + selector.show(); + } else { + selector.hide(); + } + }); + + this.typeSelector.setToggleText(item.name); + + this.cacheToggleText(); + } + + selectTemplateFile(selector, query, data) { + selector.renderLoading(); + // in case undo menu is already already there + this.destroyUndoMenu(); + this.fetchFileTemplate(selector.config.endpoint, query, data) + .then((file) => { + this.showUndoMenu(); + this.setEditorContent(file); + this.setFilename(selector.config.name); + selector.renderLoaded(); + }) + .catch(err => new Flash(`An error occurred while fetching the template: ${err}`)); + } + + displayMatchedTemplateSelector() { + const currentInput = this.getFilename(); + this.templateSelectors.forEach((selector) => { + const match = selector.config.pattern.test(currentInput); + + if (match) { + this.typeSelector.show(); + this.selectTemplateType(selector.config); + this.showTemplateSelectorMenu(); + } + }); + } + + fetchFileTemplate(apiCall, query, data) { + return new Promise((resolve) => { + const resolveFile = file => resolve(file); + + if (!data) { + apiCall(query, resolveFile); + } else { + apiCall(query, data, resolveFile); + } + }); + } + + setEditorContent(file) { + if (!file && file !== '') return; + + const newValue = file.content || file; + + this.editor.setValue(newValue, 1); + + this.editor.focus(); + + this.editor.navigateFileStart(); + } + + findTemplateSelectorByKey(key) { + return this.templateSelectors.find(selector => selector.config.key === key); + } + + showUndoMenu() { + this.$undoMenu.removeClass('hidden'); + + this.$undoBtn.on('click', () => { + this.restoreFromCache(); + this.destroyUndoMenu(); + }); + } + + destroyUndoMenu() { + this.cacheFileContents(); + this.cacheToggleText(); + this.$undoMenu.addClass('hidden'); + this.$undoBtn.off('click'); + } + + hideTemplateSelectorMenu() { + this.$templatesMenu.hide(); + } + + showTemplateSelectorMenu() { + this.$templatesMenu.show(); + } + + cacheToggleText() { + this.cachedToggleText = this.getTemplateSelectorToggleText(); + } + + cacheFileContents() { + this.cachedContent = this.editor.getValue(); + this.cachedFilename = this.getFilename(); + } + + restoreFromCache() { + this.setEditorContent(this.cachedContent); + this.setFilename(this.cachedFilename); + this.setTemplateSelectorToggleText(); + } + + getTemplateSelectorToggleText() { + return this.$templateSelectors + .find('.js-template-selector-wrap:visible .dropdown-toggle-text') + .text(); + } + + setTemplateSelectorToggleText() { + return this.$templateSelectors + .find('.js-template-selector-wrap:visible .dropdown-toggle-text') + .text(this.cachedToggleText); + } + + getTypeSelectorToggleText() { + return this.typeSelector.getToggleText(); + } + + getFilename() { + return this.$filenameInput.val(); + } + + setFilename(name) { + this.$filenameInput.val(name); + } + + getSelected() { + return this.templateSelectors.find(selector => selector.selected); + } +} diff --git a/app/assets/javascripts/blob/file_template_selector.js b/app/assets/javascripts/blob/file_template_selector.js new file mode 100644 index 00000000000..31dd45fac89 --- /dev/null +++ b/app/assets/javascripts/blob/file_template_selector.js @@ -0,0 +1,60 @@ +/* global Api */ + +export default class FileTemplateSelector { + constructor(mediator) { + this.mediator = mediator; + this.$dropdown = null; + this.$wrapper = null; + } + + init() { + const cfg = this.config; + + this.$dropdown = $(cfg.dropdown); + this.$wrapper = $(cfg.wrapper); + this.$loadingIcon = this.$wrapper.find('.fa-chevron-down'); + this.$dropdownToggleText = this.$wrapper.find('.dropdown-toggle-text'); + + this.initDropdown(); + } + + show() { + if (this.$dropdown === null) { + this.init(); + } + + this.$wrapper.removeClass('hidden'); + } + + hide() { + if (this.$dropdown !== null) { + this.$wrapper.addClass('hidden'); + } + } + + getToggleText() { + return this.$dropdownToggleText.text(); + } + + setToggleText(text) { + this.$dropdownToggleText.text(text); + } + + renderLoading() { + this.$loadingIcon + .addClass('fa-spinner fa-spin') + .removeClass('fa-chevron-down'); + } + + renderLoaded() { + this.$loadingIcon + .addClass('fa-chevron-down') + .removeClass('fa-spinner fa-spin'); + } + + reportSelection(query, el, e, data) { + e.preventDefault(); + return this.mediator.selectTemplateFile(this, query, data); + } +} + diff --git a/app/assets/javascripts/blob/template_selector.js b/app/assets/javascripts/blob/template_selector.js new file mode 100644 index 00000000000..d7c1c32efbd --- /dev/null +++ b/app/assets/javascripts/blob/template_selector.js @@ -0,0 +1,92 @@ +/* eslint-disable class-methods-use-this, no-unused-vars */ + +export default class TemplateSelector { + constructor({ dropdown, data, pattern, wrapper, editor, $input } = {}) { + this.pattern = pattern; + this.editor = editor; + this.dropdown = dropdown; + this.$dropdownContainer = wrapper; + this.$filenameInput = $input || $('#file_name'); + this.$dropdownIcon = $('.fa-chevron-down', dropdown); + + this.initDropdown(dropdown, data); + this.listenForFilenameInput(); + this.renderMatchedDropdown(); + this.initAutosizeUpdateEvent(); + } + + initDropdown(dropdown, data) { + return $(dropdown).glDropdown({ + data, + filterable: true, + selectable: true, + toggleLabel: item => item.name, + search: { + fields: ['name'], + }, + clicked: (item, el, e) => this.fetchFileTemplate(item, el, e), + text: item => item.name, + }); + } + + initAutosizeUpdateEvent() { + this.autosizeUpdateEvent = document.createEvent('Event'); + this.autosizeUpdateEvent.initEvent('autosize:update', true, false); + } + + listenForFilenameInput() { + return this.$filenameInput.on('keyup blur', e => this.renderMatchedDropdown(e)); + } + + renderMatchedDropdown() { + if (!this.$filenameInput.length) { + return null; + } + + const filenameMatches = this.pattern.test(this.$filenameInput.val().trim()); + + if (!filenameMatches) { + return this.$dropdownContainer.addClass('hidden'); + } + return this.$dropdownContainer.removeClass('hidden'); + } + + fetchFileTemplate(item, el, e) { + e.preventDefault(); + return this.requestFile(item); + } + + requestFile(item) { + // This `requestFile` method is an abstract method that should + // be added by all subclasses. + } + + // To be implemented on the extending class + // e.g. Api.gitlabCiYml(query.name, file => this.setEditorContent(file)); + + setEditorContent(file, { skipFocus } = {}) { + if (!file) return; + + const newValue = file.content; + + this.editor.setValue(newValue, 1); + + if (!skipFocus) this.editor.focus(); + + if (this.editor instanceof jQuery) { + this.editor.get(0).dispatchEvent(this.autosizeUpdateEvent); + } + } + + startLoadingSpinner() { + this.$dropdownIcon + .addClass('fa-spinner fa-spin') + .removeClass('fa-chevron-down'); + } + + stopLoadingSpinner() { + this.$dropdownIcon + .addClass('fa-chevron-down') + .removeClass('fa-spinner fa-spin'); + } +} diff --git a/app/assets/javascripts/blob/template_selectors/blob_ci_yaml_selector.js b/app/assets/javascripts/blob/template_selectors/blob_ci_yaml_selector.js deleted file mode 100644 index 5a5954e7751..00000000000 --- a/app/assets/javascripts/blob/template_selectors/blob_ci_yaml_selector.js +++ /dev/null @@ -1,9 +0,0 @@ -/* global Api */ - -import TemplateSelector from './template_selector'; - -export default class BlobCiYamlSelector extends TemplateSelector { - requestFile(query) { - return Api.gitlabCiYml(query.name, (file, config) => this.setEditorContent(file, config)); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/blob_ci_yaml_selectors.js b/app/assets/javascripts/blob/template_selectors/blob_ci_yaml_selectors.js deleted file mode 100644 index 7a4d6a42a03..00000000000 --- a/app/assets/javascripts/blob/template_selectors/blob_ci_yaml_selectors.js +++ /dev/null @@ -1,23 +0,0 @@ -/* global Api */ - -import BlobCiYamlSelector from './blob_ci_yaml_selector'; - -export default class BlobCiYamlSelectors { - constructor({ editor, $dropdowns }) { - this.$dropdowns = $dropdowns || $('.js-gitlab-ci-yml-selector'); - this.initSelectors(editor); - } - - initSelectors(editor) { - this.$dropdowns.each((i, dropdown) => { - const $dropdown = $(dropdown); - return new BlobCiYamlSelector({ - editor, - pattern: /(.gitlab-ci.yml)/, - data: $dropdown.data('data'), - wrapper: $dropdown.closest('.js-gitlab-ci-yml-selector-wrap'), - dropdown: $dropdown, - }); - }); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/blob_dockerfile_selector.js b/app/assets/javascripts/blob/template_selectors/blob_dockerfile_selector.js deleted file mode 100644 index 19f8820a0cb..00000000000 --- a/app/assets/javascripts/blob/template_selectors/blob_dockerfile_selector.js +++ /dev/null @@ -1,9 +0,0 @@ -/* global Api */ - -import TemplateSelector from './template_selector'; - -export default class BlobDockerfileSelector extends TemplateSelector { - requestFile(query) { - return Api.dockerfileYml(query.name, (file, config) => this.setEditorContent(file, config)); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/blob_dockerfile_selectors.js b/app/assets/javascripts/blob/template_selectors/blob_dockerfile_selectors.js deleted file mode 100644 index da067035b43..00000000000 --- a/app/assets/javascripts/blob/template_selectors/blob_dockerfile_selectors.js +++ /dev/null @@ -1,23 +0,0 @@ -import BlobDockerfileSelector from './blob_dockerfile_selector'; - -export default class BlobDockerfileSelectors { - constructor({ editor, $dropdowns }) { - this.editor = editor; - this.$dropdowns = $dropdowns || $('.js-dockerfile-selector'); - this.initSelectors(); - } - - initSelectors() { - const editor = this.editor; - this.$dropdowns.each((i, dropdown) => { - const $dropdown = $(dropdown); - return new BlobDockerfileSelector({ - editor, - pattern: /(Dockerfile)/, - data: $dropdown.data('data'), - wrapper: $dropdown.closest('.js-dockerfile-selector-wrap'), - dropdown: $dropdown, - }); - }); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/blob_gitignore_selector.js b/app/assets/javascripts/blob/template_selectors/blob_gitignore_selector.js deleted file mode 100644 index 0b6b02fc2b3..00000000000 --- a/app/assets/javascripts/blob/template_selectors/blob_gitignore_selector.js +++ /dev/null @@ -1,9 +0,0 @@ -/* global Api */ - -import TemplateSelector from './template_selector'; - -export default class BlobGitignoreSelector extends TemplateSelector { - requestFile(query) { - return Api.gitignoreText(query.name, (file, config) => this.setEditorContent(file, config)); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/blob_gitignore_selectors.js b/app/assets/javascripts/blob/template_selectors/blob_gitignore_selectors.js deleted file mode 100644 index dc485d97677..00000000000 --- a/app/assets/javascripts/blob/template_selectors/blob_gitignore_selectors.js +++ /dev/null @@ -1,23 +0,0 @@ -import BlobGitignoreSelector from './blob_gitignore_selector'; - -export default class BlobGitignoreSelectors { - constructor({ editor, $dropdowns }) { - this.$dropdowns = $dropdowns || $('.js-gitignore-selector'); - this.editor = editor; - this.initSelectors(); - } - - initSelectors() { - this.$dropdowns.each((i, dropdown) => { - const $dropdown = $(dropdown); - - return new BlobGitignoreSelector({ - pattern: /(.gitignore)/, - data: $dropdown.data('data'), - wrapper: $dropdown.closest('.js-gitignore-selector-wrap'), - dropdown: $dropdown, - editor: this.editor, - }); - }); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/blob_license_selector.js b/app/assets/javascripts/blob/template_selectors/blob_license_selector.js deleted file mode 100644 index e9cb31cc2dc..00000000000 --- a/app/assets/javascripts/blob/template_selectors/blob_license_selector.js +++ /dev/null @@ -1,13 +0,0 @@ -/* global Api */ - -import TemplateSelector from './template_selector'; - -export default class BlobLicenseSelector extends TemplateSelector { - requestFile(query) { - const data = { - project: this.dropdown.data('project'), - fullname: this.dropdown.data('fullname'), - }; - return Api.licenseText(query.id, data, (file, config) => this.setEditorContent(file, config)); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/blob_license_selectors.js b/app/assets/javascripts/blob/template_selectors/blob_license_selectors.js deleted file mode 100644 index a44f4f78b2d..00000000000 --- a/app/assets/javascripts/blob/template_selectors/blob_license_selectors.js +++ /dev/null @@ -1,24 +0,0 @@ -/* eslint-disable no-unused-vars, no-param-reassign */ - -import BlobLicenseSelector from './blob_license_selector'; - -export default class BlobLicenseSelectors { - constructor({ $dropdowns, editor }) { - this.$dropdowns = $dropdowns || $('.js-license-selector'); - this.initSelectors(editor); - } - - initSelectors(editor) { - this.$dropdowns.each((i, dropdown) => { - const $dropdown = $(dropdown); - - return new BlobLicenseSelector({ - editor, - pattern: /^(.+\/)?(licen[sc]e|copying)($|\.)/i, - data: $dropdown.data('data'), - wrapper: $dropdown.closest('.js-license-selector-wrap'), - dropdown: $dropdown, - }); - }); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/ci_yaml_selector.js b/app/assets/javascripts/blob/template_selectors/ci_yaml_selector.js new file mode 100644 index 00000000000..935df07677c --- /dev/null +++ b/app/assets/javascripts/blob/template_selectors/ci_yaml_selector.js @@ -0,0 +1,32 @@ +/* global Api */ + +import FileTemplateSelector from '../file_template_selector'; + +export default class BlobCiYamlSelector extends FileTemplateSelector { + constructor({ mediator }) { + super(mediator); + this.config = { + key: 'gitlab-ci-yaml', + name: '.gitlab-ci.yml', + pattern: /(.gitlab-ci.yml)/, + endpoint: Api.gitlabCiYml, + dropdown: '.js-gitlab-ci-yml-selector', + wrapper: '.js-gitlab-ci-yml-selector-wrap', + }; + } + + initDropdown() { + // maybe move to super class as well + this.$dropdown.glDropdown({ + data: this.$dropdown.data('data'), + filterable: true, + selectable: true, + toggleLabel: item => item.name, + search: { + fields: ['name'], + }, + clicked: (query, el, e) => this.reportSelection(query.name, el, e), + text: item => item.name, + }); + } +} diff --git a/app/assets/javascripts/blob/template_selectors/dockerfile_selector.js b/app/assets/javascripts/blob/template_selectors/dockerfile_selector.js new file mode 100644 index 00000000000..b4b4d09c315 --- /dev/null +++ b/app/assets/javascripts/blob/template_selectors/dockerfile_selector.js @@ -0,0 +1,32 @@ +/* global Api */ + +import FileTemplateSelector from '../file_template_selector'; + +export default class DockerfileSelector extends FileTemplateSelector { + constructor({ mediator }) { + super(mediator); + this.config = { + key: 'dockerfile', + name: 'Dockerfile', + pattern: /(Dockerfile)/, + endpoint: Api.dockerfileYml, + dropdown: '.js-dockerfile-selector', + wrapper: '.js-dockerfile-selector-wrap', + }; + } + + initDropdown() { + // maybe move to super class as well + this.$dropdown.glDropdown({ + data: this.$dropdown.data('data'), + filterable: true, + selectable: true, + toggleLabel: item => item.name, + search: { + fields: ['name'], + }, + clicked: (query, el, e) => this.reportSelection(query.name, el, e), + text: item => item.name, + }); + } +} diff --git a/app/assets/javascripts/blob/template_selectors/gitignore_selector.js b/app/assets/javascripts/blob/template_selectors/gitignore_selector.js new file mode 100644 index 00000000000..aefae54ae71 --- /dev/null +++ b/app/assets/javascripts/blob/template_selectors/gitignore_selector.js @@ -0,0 +1,31 @@ +/* global Api */ + +import FileTemplateSelector from '../file_template_selector'; + +export default class BlobGitignoreSelector extends FileTemplateSelector { + constructor({ mediator }) { + super(mediator); + this.config = { + key: 'gitignore', + name: '.gitignore', + pattern: /(.gitignore)/, + endpoint: Api.gitignoreText, + dropdown: '.js-gitignore-selector', + wrapper: '.js-gitignore-selector-wrap', + }; + } + + initDropdown() { + this.$dropdown.glDropdown({ + data: this.$dropdown.data('data'), + filterable: true, + selectable: true, + toggleLabel: item => item.name, + search: { + fields: ['name'], + }, + clicked: (query, el, e) => this.reportSelection(query.name, el, e), + text: item => item.name, + }); + } +} diff --git a/app/assets/javascripts/blob/template_selectors/license_selector.js b/app/assets/javascripts/blob/template_selectors/license_selector.js new file mode 100644 index 00000000000..c8abd689ab4 --- /dev/null +++ b/app/assets/javascripts/blob/template_selectors/license_selector.js @@ -0,0 +1,38 @@ +/* global Api */ + +import FileTemplateSelector from '../file_template_selector'; + +export default class BlobLicenseSelector extends FileTemplateSelector { + constructor({ mediator }) { + super(mediator); + this.config = { + key: 'license', + name: 'LICENSE', + pattern: /^(.+\/)?(licen[sc]e|copying)($|\.)/i, + endpoint: Api.licenseText, + dropdown: '.js-license-selector', + wrapper: '.js-license-selector-wrap', + }; + } + + initDropdown() { + this.$dropdown.glDropdown({ + data: this.$dropdown.data('data'), + filterable: true, + selectable: true, + toggleLabel: item => item.name, + search: { + fields: ['name'], + }, + clicked: (query, el, e) => { + const data = { + project: this.$dropdown.data('project'), + fullname: this.$dropdown.data('fullname'), + }; + + this.reportSelection(query.id, el, e, data); + }, + text: item => item.name, + }); + } +} diff --git a/app/assets/javascripts/blob/template_selectors/template_selector.js b/app/assets/javascripts/blob/template_selectors/template_selector.js deleted file mode 100644 index d7c1c32efbd..00000000000 --- a/app/assets/javascripts/blob/template_selectors/template_selector.js +++ /dev/null @@ -1,92 +0,0 @@ -/* eslint-disable class-methods-use-this, no-unused-vars */ - -export default class TemplateSelector { - constructor({ dropdown, data, pattern, wrapper, editor, $input } = {}) { - this.pattern = pattern; - this.editor = editor; - this.dropdown = dropdown; - this.$dropdownContainer = wrapper; - this.$filenameInput = $input || $('#file_name'); - this.$dropdownIcon = $('.fa-chevron-down', dropdown); - - this.initDropdown(dropdown, data); - this.listenForFilenameInput(); - this.renderMatchedDropdown(); - this.initAutosizeUpdateEvent(); - } - - initDropdown(dropdown, data) { - return $(dropdown).glDropdown({ - data, - filterable: true, - selectable: true, - toggleLabel: item => item.name, - search: { - fields: ['name'], - }, - clicked: (item, el, e) => this.fetchFileTemplate(item, el, e), - text: item => item.name, - }); - } - - initAutosizeUpdateEvent() { - this.autosizeUpdateEvent = document.createEvent('Event'); - this.autosizeUpdateEvent.initEvent('autosize:update', true, false); - } - - listenForFilenameInput() { - return this.$filenameInput.on('keyup blur', e => this.renderMatchedDropdown(e)); - } - - renderMatchedDropdown() { - if (!this.$filenameInput.length) { - return null; - } - - const filenameMatches = this.pattern.test(this.$filenameInput.val().trim()); - - if (!filenameMatches) { - return this.$dropdownContainer.addClass('hidden'); - } - return this.$dropdownContainer.removeClass('hidden'); - } - - fetchFileTemplate(item, el, e) { - e.preventDefault(); - return this.requestFile(item); - } - - requestFile(item) { - // This `requestFile` method is an abstract method that should - // be added by all subclasses. - } - - // To be implemented on the extending class - // e.g. Api.gitlabCiYml(query.name, file => this.setEditorContent(file)); - - setEditorContent(file, { skipFocus } = {}) { - if (!file) return; - - const newValue = file.content; - - this.editor.setValue(newValue, 1); - - if (!skipFocus) this.editor.focus(); - - if (this.editor instanceof jQuery) { - this.editor.get(0).dispatchEvent(this.autosizeUpdateEvent); - } - } - - startLoadingSpinner() { - this.$dropdownIcon - .addClass('fa-spinner fa-spin') - .removeClass('fa-chevron-down'); - } - - stopLoadingSpinner() { - this.$dropdownIcon - .addClass('fa-chevron-down') - .removeClass('fa-spinner fa-spin'); - } -} diff --git a/app/assets/javascripts/blob/template_selectors/type_selector.js b/app/assets/javascripts/blob/template_selectors/type_selector.js new file mode 100644 index 00000000000..56f23ef0568 --- /dev/null +++ b/app/assets/javascripts/blob/template_selectors/type_selector.js @@ -0,0 +1,25 @@ +import FileTemplateSelector from '../file_template_selector'; + +export default class FileTemplateTypeSelector extends FileTemplateSelector { + constructor({ mediator, dropdownData }) { + super(mediator); + this.mediator = mediator; + this.config = { + dropdown: '.js-template-type-selector', + wrapper: '.js-template-type-selector-wrap', + dropdownData, + }; + } + + initDropdown() { + this.$dropdown.glDropdown({ + data: this.config.dropdownData, + filterable: false, + selectable: true, + toggleLabel: item => item.name, + clicked: (item, el, e) => this.mediator.selectTemplateType(item, el, e), + text: item => item.name, + }); + } + +} diff --git a/app/assets/javascripts/blob_edit/blob_bundle.js b/app/assets/javascripts/blob_edit/blob_bundle.js index c5deccf631e..1c64ccf536f 100644 --- a/app/assets/javascripts/blob_edit/blob_bundle.js +++ b/app/assets/javascripts/blob_edit/blob_bundle.js @@ -13,8 +13,9 @@ $(() => { const urlRoot = editBlobForm.data('relative-url-root'); const assetsPath = editBlobForm.data('assets-prefix'); const blobLanguage = editBlobForm.data('blob-language'); + const currentAction = $('.js-file-title').data('current-action'); - new EditBlob(`${urlRoot}${assetsPath}`, blobLanguage); + new EditBlob(`${urlRoot}${assetsPath}`, blobLanguage, currentAction); new NewCommitForm(editBlobForm); } diff --git a/app/assets/javascripts/blob_edit/edit_blob.js b/app/assets/javascripts/blob_edit/edit_blob.js index d3560d5df3b..b37988a674d 100644 --- a/app/assets/javascripts/blob_edit/edit_blob.js +++ b/app/assets/javascripts/blob_edit/edit_blob.js @@ -1,17 +1,13 @@ /* global ace */ -import BlobLicenseSelectors from '../blob/template_selectors/blob_license_selectors'; -import BlobGitignoreSelectors from '../blob/template_selectors/blob_gitignore_selectors'; -import BlobCiYamlSelectors from '../blob/template_selectors/blob_ci_yaml_selectors'; -import BlobDockerfileSelectors from '../blob/template_selectors/blob_dockerfile_selectors'; +import TemplateSelectorMediator from '../blob/file_template_mediator'; export default class EditBlob { - constructor(assetsPath, aceMode) { + constructor(assetsPath, aceMode, currentAction) { this.configureAceEditor(aceMode, assetsPath); - this.prepFileContentForSubmit(); this.initModePanesAndLinks(); this.initSoftWrap(); - this.initFileSelectors(); + this.initFileSelectors(currentAction); } configureAceEditor(aceMode, assetsPath) { @@ -19,6 +15,10 @@ export default class EditBlob { ace.config.loadModule('ace/ext/searchbox'); this.editor = ace.edit('editor'); + + // This prevents warnings re: automatic scrolling being logged + this.editor.$blockScrolling = Infinity; + this.editor.focus(); if (aceMode) { @@ -26,29 +26,13 @@ export default class EditBlob { } } - prepFileContentForSubmit() { - $('form').submit(() => { - $('#file-content').val(this.editor.getValue()); + initFileSelectors(currentAction) { + this.fileTemplateMediator = new TemplateSelectorMediator({ + currentAction, + editor: this.editor, }); } - initFileSelectors() { - this.blobTemplateSelectors = [ - new BlobLicenseSelectors({ - editor: this.editor, - }), - new BlobGitignoreSelectors({ - editor: this.editor, - }), - new BlobCiYamlSelectors({ - editor: this.editor, - }), - new BlobDockerfileSelectors({ - editor: this.editor, - }), - ]; - } - initModePanesAndLinks() { this.$editModePanes = $('.js-edit-mode-pane'); this.$editModeLinks = $('.js-edit-mode a'); diff --git a/app/assets/javascripts/templates/issuable_template_selector.js b/app/assets/javascripts/templates/issuable_template_selector.js index 32067ed1fee..e62f429f1ae 100644 --- a/app/assets/javascripts/templates/issuable_template_selector.js +++ b/app/assets/javascripts/templates/issuable_template_selector.js @@ -1,7 +1,7 @@ /* eslint-disable comma-dangle, max-len, no-useless-return, no-param-reassign, max-len */ /* global Api */ -import TemplateSelector from '../blob/template_selectors/template_selector'; +import TemplateSelector from '../blob/template_selector'; ((global) => { class IssuableTemplateSelector extends TemplateSelector { diff --git a/app/assets/stylesheets/pages/editor.scss b/app/assets/stylesheets/pages/editor.scss index 4af267403d8..f6b8c8ee2bc 100644 --- a/app/assets/stylesheets/pages/editor.scss +++ b/app/assets/stylesheets/pages/editor.scss @@ -1,4 +1,13 @@ .file-editor { + .nav-links { + border-top: 1px solid $border-color; + border-right: 1px solid $border-color; + border-left: 1px solid $border-color; + border-bottom: none; + border-radius: 2px; + background: $gray-normal; + } + #editor { border: none; border-radius: 0; @@ -72,11 +81,7 @@ } .encoding-selector, - .soft-wrap-toggle, - .license-selector, - .gitignore-selector, - .gitlab-ci-yml-selector, - .dockerfile-selector { + .soft-wrap-toggle { display: inline-block; vertical-align: top; font-family: $regular_font; @@ -103,28 +108,9 @@ } } } - - .gitignore-selector, - .license-selector, - .gitlab-ci-yml-selector, - .dockerfile-selector { - .dropdown { - line-height: 21px; - } - - .dropdown-menu-toggle { - vertical-align: top; - width: 220px; - } - } - - .gitlab-ci-yml-selector { - .dropdown-menu-toggle { - width: 250px; - } - } } + @media(max-width: $screen-xs-max){ .file-editor { .file-title { @@ -149,10 +135,7 @@ margin: 3px 0; } - .encoding-selector, - .license-selector, - .gitignore-selector, - .gitlab-ci-yml-selector { + .encoding-selector { display: block; margin: 3px 0; @@ -163,3 +146,104 @@ } } } + +.blob-new-page-title, +.blob-edit-page-title { + margin: 19px 0 21px; + vertical-align: top; + display: inline-block; + + @media(max-width: $screen-sm-max) { + display: block; + margin: 19px 0 12px; + } +} + +.template-selectors-menu { + display: inline-block; + vertical-align: top; + margin: 14px 0 0 16px; + padding: 0 0 0 14px; + border-left: 1px solid $border-color; + + @media(max-width: $screen-sm-max) { + display: block; + width: 100%; + margin: 5px 0; + padding: 0; + border-left: none; + } +} + +.templates-selectors-label { + display: inline-block; + vertical-align: top; + margin-top: 6px; + line-height: 21px; + + @media(max-width: $screen-sm-max) { + display: block; + margin: 5px 0; + } +} + +.template-selector-dropdowns-wrap { + display: inline-block; + margin-left: 8px; + vertical-align: top; + margin: 5px 0 0 8px; + + @media(max-width: $screen-sm-max) { + display: block; + width: 100%; + margin: 0 0 16px; + } + + .license-selector, + .gitignore-selector, + .gitlab-ci-yml-selector, + .dockerfile-selector, + .template-type-selector { + display: inline-block; + vertical-align: top; + font-family: $regular_font; + margin-top: -5px; + + @media(max-width: $screen-sm-max) { + display: block; + width: 100%; + margin: 5px 0; + } + + .dropdown { + line-height: 21px; + } + + .dropdown-menu-toggle { + width: 250px; + vertical-align: top; + + @media(max-width: $screen-sm-max) { + display: block; + width: 100%; + margin: 5px 0; + } + } + + } +} + +.template-selectors-undo-menu { + display: inline-block; + margin: 7px 0 0 10px; + + @media(max-width: $screen-sm-max) { + display: block; + width: 100%; + margin: 20px 0; + } + + button { + margin: -4px 0 0 15px; + } +} diff --git a/app/views/projects/blob/_editor.html.haml b/app/views/projects/blob/_editor.html.haml index e7adef5558a..4b344b2edb9 100644 --- a/app/views/projects/blob/_editor.html.haml +++ b/app/views/projects/blob/_editor.html.haml @@ -1,29 +1,23 @@ +- action = current_action?(:edit) || current_action?(:update) ? 'edit' : 'create' + .file-holder.file.append-bottom-default - .js-file-title.file-title.clearfix + .js-file-title.file-title.clearfix{ data: { current_action: action } } .editor-ref = icon('code-fork') = ref %span.editor-file-name - if current_action?(:edit) || current_action?(:update) = text_field_tag 'file_path', (params[:file_path] || @path), - class: 'form-control new-file-path' + class: 'form-control new-file-path js-file-path-name-input' - if current_action?(:new) || current_action?(:create) %span.editor-file-name \/ = text_field_tag 'file_name', params[:file_name], placeholder: "File name", - required: true, class: 'form-control new-file-name' + required: true, class: 'form-control new-file-name js-file-path-name-input' .pull-right.file-buttons - .license-selector.js-license-selector-wrap.hidden - = dropdown_tag("Choose a License template", options: { toggle_class: 'btn js-license-selector', title: "Choose a license", filter: true, placeholder: "Filter", data: { data: licenses_for_select, project: @project.name, fullname: @project.namespace.human_name } } ) - .gitignore-selector.js-gitignore-selector-wrap.hidden - = dropdown_tag("Choose a .gitignore template", options: { toggle_class: 'btn js-gitignore-selector', title: "Choose a template", filter: true, placeholder: "Filter", data: { data: gitignore_names } } ) - .gitlab-ci-yml-selector.js-gitlab-ci-yml-selector-wrap.hidden - = dropdown_tag("Choose a GitLab CI Yaml template", options: { toggle_class: 'btn js-gitlab-ci-yml-selector', title: "Choose a template", filter: true, placeholder: "Filter", data: { data: gitlab_ci_ymls } } ) - .dockerfile-selector.js-dockerfile-selector-wrap.hidden - = dropdown_tag("Choose a Dockerfile template", options: { toggle_class: 'btn js-dockerfile-selector', title: "Choose a template", filter: true, placeholder: "Filter", data: { data: dockerfile_names } } ) - = button_tag class: 'soft-wrap-toggle btn', type: 'button' do + = button_tag class: 'soft-wrap-toggle btn', type: 'button', tabindex: '-1' do %span.no-wrap = custom_icon('icon_no_wrap') No wrap @@ -31,7 +25,7 @@ = custom_icon('icon_soft_wrap') Soft wrap .encoding-selector - = select_tag :encoding, options_for_select([ "base64", "text" ], "text"), class: 'select2' + = select_tag :encoding, options_for_select([ "base64", "text" ], "text"), class: 'select2', tabindex: '-1' .file-editor.code %pre.js-edit-mode-pane#editor= params[:content] || local_assigns[:blob_data] diff --git a/app/views/projects/blob/_template_selectors.html.haml b/app/views/projects/blob/_template_selectors.html.haml new file mode 100644 index 00000000000..d52733d2bd6 --- /dev/null +++ b/app/views/projects/blob/_template_selectors.html.haml @@ -0,0 +1,17 @@ +.template-selectors-menu + .templates-selectors-label + Template + .template-selector-dropdowns-wrap + .template-type-selector.js-template-type-selector-wrap.hidden + = dropdown_tag("Choose type", options: { toggle_class: 'btn js-template-type-selector', title: "Choose a template type" } ) + .license-selector.js-license-selector-wrap.js-template-selector-wrap.hidden + = dropdown_tag("Apply a License template", options: { toggle_class: 'btn js-license-selector', title: "Apply a license", filter: true, placeholder: "Filter", data: { data: licenses_for_select, project: @project.name, fullname: @project.namespace.human_name } } ) + .gitignore-selector.js-gitignore-selector-wrap.js-template-selector-wrap.hidden + = dropdown_tag("Apply a .gitignore template", options: { toggle_class: 'btn js-gitignore-selector', title: "Apply a template", filter: true, placeholder: "Filter", data: { data: gitignore_names } } ) + .gitlab-ci-yml-selector.js-gitlab-ci-yml-selector-wrap.js-template-selector-wrap.hidden + = dropdown_tag("Apply a GitLab CI Yaml template", options: { toggle_class: 'btn js-gitlab-ci-yml-selector', title: "Apply a template", filter: true, placeholder: "Filter", data: { data: gitlab_ci_ymls } } ) + .dockerfile-selector.js-dockerfile-selector-wrap.js-template-selector-wrap.hidden + = dropdown_tag("Apply a Dockerfile template", options: { toggle_class: 'btn js-dockerfile-selector', title: "Apply a template", filter: true, placeholder: "Filter", data: { data: dockerfile_names } } ) + .template-selectors-undo-menu.hidden + %span.text-info Template applied + %button.btn.btn-sm.btn-info Undo diff --git a/app/views/projects/blob/edit.html.haml b/app/views/projects/blob/edit.html.haml index afe0b5dba45..4b26f944733 100644 --- a/app/views/projects/blob/edit.html.haml +++ b/app/views/projects/blob/edit.html.haml @@ -11,12 +11,15 @@ Someone edited the file the same time you did. Please check out = link_to "the file", namespace_project_blob_path(@project.namespace, @project, tree_join(@target_branch, @file_path)), target: "_blank", rel: 'noopener noreferrer' and make sure your changes will not unintentionally remove theirs. - + .editor-title-row + %h3.page-title.blob-edit-page-title + Edit file + = render 'template_selectors' .file-editor %ul.nav-links.no-bottom.js-edit-mode %li.active = link_to '#editor' do - Edit File + Write %li = link_to '#preview', 'data-preview-url' => namespace_project_preview_blob_path(@project.namespace, @project, @id) do diff --git a/app/views/projects/blob/new.html.haml b/app/views/projects/blob/new.html.haml index 4c449e040ee..2afb909572a 100644 --- a/app/views/projects/blob/new.html.haml +++ b/app/views/projects/blob/new.html.haml @@ -2,10 +2,10 @@ - content_for :page_specific_javascripts do = page_specific_javascript_tag('lib/ace.js') = page_specific_javascript_bundle_tag('blob') - -%h3.page-title - New File - +.editor-title-row + %h3.page-title.blob-new-page-title + New file + = render 'template_selectors' .file-editor = form_tag(namespace_project_create_blob_path(@project.namespace, @project, @id), method: :post, class: 'form-horizontal js-edit-blob-form js-new-blob-form js-quick-submit js-requires-input', data: blob_editor_paths) do = render 'projects/blob/editor', ref: @ref diff --git a/changelogs/unreleased/25332-make-file-templates-easy-to-use-and-discover.yml b/changelogs/unreleased/25332-make-file-templates-easy-to-use-and-discover.yml new file mode 100644 index 00000000000..fc95858f783 --- /dev/null +++ b/changelogs/unreleased/25332-make-file-templates-easy-to-use-and-discover.yml @@ -0,0 +1,4 @@ +--- +title: Remove no-new annotation from file_template_mediator.js. +merge_request: !9782 +author: diff --git a/spec/features/auto_deploy_spec.rb b/spec/features/auto_deploy_spec.rb index ea7a97d1d4f..009e9c6b04c 100644 --- a/spec/features/auto_deploy_spec.rb +++ b/spec/features/auto_deploy_spec.rb @@ -42,7 +42,7 @@ describe 'Auto deploy' do it 'includes OpenShift as an available template', js: true do click_link 'Set up auto deploy' - click_button 'Choose a GitLab CI Yaml template' + click_button 'Apply a GitLab CI Yaml template' within '.gitlab-ci-yml-selector' do expect(page).to have_content('OpenShift') @@ -51,7 +51,7 @@ describe 'Auto deploy' do it 'creates a merge request using "auto-deploy" branch', js: true do click_link 'Set up auto deploy' - click_button 'Choose a GitLab CI Yaml template' + click_button 'Apply a GitLab CI Yaml template' within '.gitlab-ci-yml-selector' do click_on 'OpenShift' end diff --git a/spec/features/projects/blobs/user_create_spec.rb b/spec/features/projects/blobs/user_create_spec.rb index 5686868a0c4..d214a531138 100644 --- a/spec/features/projects/blobs/user_create_spec.rb +++ b/spec/features/projects/blobs/user_create_spec.rb @@ -88,7 +88,7 @@ feature 'New blob creation', feature: true, js: true do scenario 'shows error message' do expect(page).to have_content('Your changes could not be committed because a file with the same name already exists') - expect(page).to have_content('New File') + expect(page).to have_content('New file') expect(page).to have_content('NextFeature') end end diff --git a/spec/features/projects/files/project_owner_creates_license_file_spec.rb b/spec/features/projects/files/project_owner_creates_license_file_spec.rb index ccadc936567..6b281e6d21d 100644 --- a/spec/features/projects/files/project_owner_creates_license_file_spec.rb +++ b/spec/features/projects/files/project_owner_creates_license_file_spec.rb @@ -40,7 +40,7 @@ feature 'project owner creates a license file', feature: true, js: true do scenario 'project master creates a license file from the "Add license" link' do click_link 'Add License' - expect(page).to have_content('New File') + expect(page).to have_content('New file') expect(current_path).to eq( namespace_project_new_blob_path(project.namespace, project, 'master')) expect(find('#file_name').value).to eq('LICENSE') @@ -63,7 +63,7 @@ feature 'project owner creates a license file', feature: true, js: true do def select_template(template) page.within('.js-license-selector-wrap') do - click_button 'Choose a License template' + click_button 'Apply a License template' click_link template wait_for_ajax end 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 420db962318..87322ac2584 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 @@ -14,7 +14,7 @@ feature 'project owner sees a link to create a license file in empty project', f visit namespace_project_path(project.namespace, project) click_link 'Create empty bare repository' click_on 'LICENSE' - expect(page).to have_content('New File') + expect(page).to have_content('New file') expect(current_path).to eq( namespace_project_new_blob_path(project.namespace, project, 'master')) @@ -40,7 +40,7 @@ feature 'project owner sees a link to create a license file in empty project', f def select_template(template) page.within('.js-license-selector-wrap') do - click_button 'Choose a License template' + click_button 'Apply a License template' click_link template wait_for_ajax end diff --git a/spec/features/projects/files/template_type_dropdown_spec.rb b/spec/features/projects/files/template_type_dropdown_spec.rb new file mode 100644 index 00000000000..5ee5e5b4c4e --- /dev/null +++ b/spec/features/projects/files/template_type_dropdown_spec.rb @@ -0,0 +1,135 @@ +require 'spec_helper' + +feature 'Template type dropdown selector', js: true do + let(:project) { create(:project) } + let(:user) { create(:user) } + + before do + project.team << [user, :master] + login_as user + end + + context 'editing a non-matching file' do + before do + create_and_edit_file('.random-file.js') + end + + scenario 'not displayed' do + check_type_selector_display(false) + end + + scenario 'selects every template type correctly' do + fill_in 'file_path', with: '.gitignore' + try_selecting_all_types + end + + scenario 'updates toggle value when input matches' do + fill_in 'file_path', with: '.gitignore' + check_type_selector_toggle_text('.gitignore') + end + end + + context 'editing a matching file' do + before do + visit namespace_project_edit_blob_path(project.namespace, project, File.join(project.default_branch, 'LICENSE')) + end + + scenario 'displayed' do + check_type_selector_display(true) + end + + scenario 'is displayed when input matches' do + check_type_selector_display(true) + end + + scenario 'selects every template type correctly' do + try_selecting_all_types + end + + context 'user previews changes' do + before do + click_link 'Preview Changes' + end + + scenario 'type selector is hidden and shown correctly' do + check_type_selector_display(false) + click_link 'Write' + check_type_selector_display(true) + end + end + end + + context 'creating a matching file' do + before do + visit namespace_project_new_blob_path(project.namespace, project, 'master', file_name: '.gitignore') + end + + scenario 'is displayed' do + check_type_selector_display(true) + end + + scenario 'toggle is set to the correct value' do + check_type_selector_toggle_text('.gitignore') + end + + scenario 'selects every template type correctly' do + try_selecting_all_types + end + end + + context 'creating a file' do + before do + visit namespace_project_new_blob_path(project.namespace, project, project.default_branch) + end + + scenario 'type selector is shown' do + check_type_selector_display(true) + end + + scenario 'toggle is set to the proper value' do + check_type_selector_toggle_text('Choose type') + end + + scenario 'selects every template type correctly' do + try_selecting_all_types + end + end +end + +def check_type_selector_display(is_visible) + count = is_visible ? 1 : 0 + expect(page).to have_css('.js-template-type-selector', count: count) +end + +def try_selecting_all_types + try_selecting_template_type('LICENSE', 'Apply a License template') + try_selecting_template_type('Dockerfile', 'Apply a Dockerfile template') + try_selecting_template_type('.gitlab-ci.yml', 'Apply a GitLab CI Yaml template') + try_selecting_template_type('.gitignore', 'Apply a .gitignore template') +end + +def try_selecting_template_type(template_type, selector_label) + select_template_type(template_type) + check_template_selector_display(selector_label) + check_type_selector_toggle_text(template_type) +end + +def select_template_type(template_type) + find('.js-template-type-selector').click + find('.dropdown-content li', text: template_type).click +end + +def check_template_selector_display(content) + expect(page).to have_content(content) +end + +def check_type_selector_toggle_text(template_type) + dropdown_toggle_button = find('.template-type-selector .dropdown-toggle-text') + expect(dropdown_toggle_button).to have_content(template_type) +end + +def create_and_edit_file(file_name) + visit namespace_project_new_blob_path(project.namespace, project, 'master', file_name: file_name) + click_button "Commit Changes" + visit namespace_project_edit_blob_path(project.namespace, project, File.join(project.default_branch, file_name)) +end diff --git a/spec/features/projects/files/undo_template_spec.rb b/spec/features/projects/files/undo_template_spec.rb new file mode 100644 index 00000000000..5479ea34610 --- /dev/null +++ b/spec/features/projects/files/undo_template_spec.rb @@ -0,0 +1,67 @@ +require 'spec_helper' +include WaitForAjax + +feature 'Template Undo Button', js: true do + let(:project) { create(:project) } + let(:user) { create(:user) } + + before do + project.team << [user, :master] + login_as user + end + + context 'editing a matching file and applying a template' do + before do + visit namespace_project_edit_blob_path(project.namespace, project, File.join(project.default_branch, "LICENSE")) + select_file_template('.js-license-selector', 'Apache License 2.0') + end + + scenario 'reverts template application' do + try_template_undo('http://www.apache.org/licenses/', 'Apply a License template') + end + end + + context 'creating a non-matching file' do + before do + visit namespace_project_new_blob_path(project.namespace, project, 'master') + select_file_template_type('LICENSE') + select_file_template('.js-license-selector', 'Apache License 2.0') + end + + scenario 'reverts template application' do + try_template_undo('http://www.apache.org/licenses/', 'Apply a License template') + end + end +end + +def try_template_undo(template_content, toggle_text) + check_undo_button_display + check_content_reverted(template_content) + check_toggle_text_set(toggle_text) +end + +def check_toggle_text_set(neutral_toggle_text) + expect(page).to have_content(neutral_toggle_text) +end + +def check_undo_button_display + expect(page).to have_content('Template applied') + expect(page).to have_css('.template-selectors-undo-menu .btn-info') +end + +def check_content_reverted(template_content) + find('.template-selectors-undo-menu .btn-info').click + expect(page).not_to have_content(template_content) + expect(find('.template-type-selector .dropdown-toggle-text')).to have_content() +end + +def select_file_template(template_selector_selector, template_name) + find(template_selector_selector).click + find('.dropdown-content li', text: template_name).click + wait_for_ajax +end + +def select_file_template_type(template_type) + find('.js-template-type-selector').click + find('.dropdown-content li', text: template_type).click +end -- cgit v1.2.1 From 336ba94a7bbdd52f07d0f1c7f7bced20c37ad307 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 27 Mar 2017 11:43:03 -0300 Subject: Fetch GitHub project as a mirror to get all refs at once --- app/models/concerns/repository_mirroring.rb | 31 +++++++++++++++++++++++++++++ app/models/project.rb | 4 ++++ app/models/repository.rb | 8 ++++++++ app/services/projects/import_service.rb | 25 ++++++++++++++++++++++- lib/gitlab/github_import/importer.rb | 2 +- lib/gitlab/shell.rb | 19 ++++++++++++++++++ 6 files changed, 87 insertions(+), 2 deletions(-) create mode 100644 app/models/concerns/repository_mirroring.rb diff --git a/app/models/concerns/repository_mirroring.rb b/app/models/concerns/repository_mirroring.rb new file mode 100644 index 00000000000..7aca3f72ae7 --- /dev/null +++ b/app/models/concerns/repository_mirroring.rb @@ -0,0 +1,31 @@ +module RepositoryMirroring + def storage_path + @project.repository_storage_path + end + + def add_remote(name, url) + raw_repository.remote_add(name, url) + rescue Rugged::ConfigError + raw_repository.remote_update(name, url: url) + end + + def remove_remote(name) + raw_repository.remote_delete(name) + true + rescue Rugged::ConfigError + false + end + + def set_remote_as_mirror(name) + config = raw_repository.rugged.config + + # This is used to define repository as equivalent as "git clone --mirror" + config["remote.#{name}.fetch"] = 'refs/*:refs/*' + config["remote.#{name}.mirror"] = true + config["remote.#{name}.prune"] = true + end + + def fetch_remote(remote, forced: false, no_tags: false) + gitlab_shell.fetch_remote(storage_path, path_with_namespace, remote, forced: forced, no_tags: no_tags) + end +end diff --git a/app/models/project.rb b/app/models/project.rb index f1bba56d32c..83660d8c431 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -551,6 +551,10 @@ class Project < ActiveRecord::Base import_type == 'gitea' end + def github_import? + import_type == 'github' + end + def check_limit unless creator.can_create_project? || namespace.kind == 'group' projects_limit = creator.projects_limit diff --git a/app/models/repository.rb b/app/models/repository.rb index 596650353fc..a9c1ce6782d 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -2,6 +2,7 @@ require 'securerandom' class Repository include Gitlab::ShellAdapter + include RepositoryMirroring attr_accessor :path_with_namespace, :project @@ -1033,6 +1034,13 @@ class Repository rugged.references.delete(tmp_ref) if tmp_ref end + def fetch_mirror(remote, url) + add_remote(remote, url) + set_remote_as_mirror(remote) + fetch_remote(remote, forced: true) + remove_remote(remote) + end + def fetch_ref(source_path, source_ref, target_ref) args = %W(#{Gitlab.config.git.bin_path} fetch --no-tags -f #{source_path} #{source_ref}:#{target_ref}) Gitlab::Popen.popen(args, path_to_repo) diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index d484a96f785..794cc1556a4 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -21,7 +21,11 @@ module Projects # In this case, we only want to import issues, not a repository. create_repository elsif !project.repository_exists? - import_repository + if project.github_import? || project.gitea_import? + fetch_repository + else + import_repository + end end end @@ -45,6 +49,25 @@ module Projects end end + def fetch_repository + begin + raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url) + + project.create_repository + project.repository.add_remote(project.import_type, project.import_url) + project.repository.set_remote_as_mirror (project.import_type) + project.repository.fetch_remote(project.import_type, forced: true) + project.repository.remove_remote(project.import_type) + rescue => e + # Expire cache to prevent scenarios such as: + # 1. First import failed, but the repo was imported successfully, so +exists?+ returns true + # 2. Retried import, repo is broken or not imported but +exists?+ still returns true + project.repository.before_import if project.repository_exists? + + raise Error, "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}" + end + end + def import_data return unless has_importer? diff --git a/lib/gitlab/github_import/importer.rb b/lib/gitlab/github_import/importer.rb index eea4a91f17d..a8c0b47e786 100644 --- a/lib/gitlab/github_import/importer.rb +++ b/lib/gitlab/github_import/importer.rb @@ -157,7 +157,7 @@ module Gitlab end def restore_source_branch(pull_request) - project.repository.fetch_ref(repo_url, "pull/#{pull_request.number}/head", pull_request.source_branch_name) + project.repository.create_branch(pull_request.source_branch_name, pull_request.source_branch_sha) end def restore_target_branch(pull_request) diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index da8d8ddb8ed..94073d816e5 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -88,6 +88,25 @@ module Gitlab true end + # Fetch remote for repository + # + # name - project path with namespace + # remote - remote name + # forced - should we use --force flag? + # + # Ex. + # fetch_remote("gitlab/gitlab-ci", "upstream") + # + def fetch_remote(storage, name, remote, forced: false, no_tags: false) + args = [gitlab_shell_projects_path, 'fetch-remote', storage, "#{name}.git", remote, '2100'] + args << '--force' if forced + args << '--no-tags' if no_tags + + output, status = Popen.popen(args) + raise Error, output unless status.zero? + true + end + # Move repository # storage - project's storage path # path - project path with namespace -- cgit v1.2.1 From 20486593bd0a3b0b150cb4b62edcb5eb7793297e Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 27 Mar 2017 11:43:35 -0300 Subject: Skip MR metrics when importing projects from GitHub --- app/models/concerns/importable.rb | 3 +++ app/models/concerns/issuable.rb | 3 ++- app/models/merge_request.rb | 1 - lib/gitlab/github_import/pull_request_formatter.rb | 3 ++- 4 files changed, 7 insertions(+), 3 deletions(-) diff --git a/app/models/concerns/importable.rb b/app/models/concerns/importable.rb index 019ef755849..8bce1c1766b 100644 --- a/app/models/concerns/importable.rb +++ b/app/models/concerns/importable.rb @@ -3,4 +3,7 @@ module Importable attr_accessor :importing alias_method :importing?, :importing + + attr_accessor :skip_metrics + alias_method :skip_metrics?, :skip_metrics end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 4d54426b79e..863b43cec76 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -14,6 +14,7 @@ module Issuable include Awardable include Taskable include TimeTrackable + include Importable # This object is used to gather issuable meta data for displaying # upvotes, downvotes, notes and closing merge requests count for issues and merge requests @@ -99,7 +100,7 @@ module Issuable acts_as_paranoid after_save :update_assignee_cache_counts, if: :assignee_id_changed? - after_save :record_metrics + after_save :record_metrics, unless: :skip_metrics? def update_assignee_cache_counts # make sure we flush the cache for both the old *and* new assignees(if they exist) diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 5ff83944d8c..8d740adb771 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -3,7 +3,6 @@ class MergeRequest < ActiveRecord::Base include Issuable include Referable include Sortable - include Importable belongs_to :target_project, class_name: "Project" belongs_to :source_project, class_name: "Project" diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index 38660a7ccca..b347e0b0feb 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -20,7 +20,8 @@ module Gitlab author_id: author_id, assignee_id: assignee_id, created_at: raw_data.created_at, - updated_at: raw_data.updated_at + updated_at: raw_data.updated_at, + skip_metrics: true } end -- cgit v1.2.1 From a9e432d6be014ddb6b8aa132924890e6a82ad4eb Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 27 Mar 2017 20:16:46 -0300 Subject: Remove unused include from RepositoryImportWorker --- app/workers/repository_import_worker.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/workers/repository_import_worker.rb b/app/workers/repository_import_worker.rb index c8a77e21c12..68a6fd76e70 100644 --- a/app/workers/repository_import_worker.rb +++ b/app/workers/repository_import_worker.rb @@ -1,6 +1,5 @@ class RepositoryImportWorker include Sidekiq::Worker - include Gitlab::ShellAdapter include DedicatedSidekiqQueue attr_accessor :project, :current_user -- cgit v1.2.1 From 0b7d10fb2dd3470a8328133f3273ad9d499e3af2 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 27 Mar 2017 20:48:35 -0300 Subject: Add CHANGELOG --- changelogs/unreleased/fix-github-importer-slowness.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/fix-github-importer-slowness.yml diff --git a/changelogs/unreleased/fix-github-importer-slowness.yml b/changelogs/unreleased/fix-github-importer-slowness.yml new file mode 100644 index 00000000000..c1f8d0e02d5 --- /dev/null +++ b/changelogs/unreleased/fix-github-importer-slowness.yml @@ -0,0 +1,4 @@ +--- +title: Improve performance of GitHub importer for large repositories. +merge_request: 10273 +author: -- cgit v1.2.1 From a6affc66dc93e178f6f96a84fae5a481c1a9803a Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 3 Apr 2017 14:31:48 -0300 Subject: Rename skip_metrics to imported on the importable concern --- app/models/concerns/importable.rb | 4 ++-- app/models/concerns/issuable.rb | 2 +- lib/gitlab/github_import/pull_request_formatter.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/concerns/importable.rb b/app/models/concerns/importable.rb index 8bce1c1766b..c9331eaf4cc 100644 --- a/app/models/concerns/importable.rb +++ b/app/models/concerns/importable.rb @@ -4,6 +4,6 @@ module Importable attr_accessor :importing alias_method :importing?, :importing - attr_accessor :skip_metrics - alias_method :skip_metrics?, :skip_metrics + attr_accessor :imported + alias_method :imported?, :imported end diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 863b43cec76..b4dded7e27e 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -100,7 +100,7 @@ module Issuable acts_as_paranoid after_save :update_assignee_cache_counts, if: :assignee_id_changed? - after_save :record_metrics, unless: :skip_metrics? + after_save :record_metrics, unless: :imported? def update_assignee_cache_counts # make sure we flush the cache for both the old *and* new assignees(if they exist) diff --git a/lib/gitlab/github_import/pull_request_formatter.rb b/lib/gitlab/github_import/pull_request_formatter.rb index b347e0b0feb..150afa31432 100644 --- a/lib/gitlab/github_import/pull_request_formatter.rb +++ b/lib/gitlab/github_import/pull_request_formatter.rb @@ -21,7 +21,7 @@ module Gitlab assignee_id: assignee_id, created_at: raw_data.created_at, updated_at: raw_data.updated_at, - skip_metrics: true + imported: true } end -- cgit v1.2.1 From 6e64071e2065dc20d9a7afdce39a68b6172eea1f Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 3 Apr 2017 14:33:30 -0300 Subject: Fix GitHub pull request formatter spec --- spec/lib/gitlab/github_import/pull_request_formatter_spec.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb index 9b9f7e4d34e..b7c59918a76 100644 --- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb +++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb @@ -64,7 +64,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do author_id: project.creator_id, assignee_id: nil, created_at: created_at, - updated_at: updated_at + updated_at: updated_at, + imported: true } expect(pull_request.attributes).to eq(expected) @@ -90,7 +91,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do author_id: project.creator_id, assignee_id: nil, created_at: created_at, - updated_at: updated_at + updated_at: updated_at, + imported: true } expect(pull_request.attributes).to eq(expected) @@ -117,7 +119,8 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do author_id: project.creator_id, assignee_id: nil, created_at: created_at, - updated_at: updated_at + updated_at: updated_at, + imported: true } expect(pull_request.attributes).to eq(expected) -- cgit v1.2.1 From 35fe9c660cc5309e244ce1bde5f65b4e7ec73e4e Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 3 Apr 2017 14:46:00 -0300 Subject: Move methods that are not related to mirroring to the repository model --- app/models/concerns/repository_mirroring.rb | 24 +++++------------------- app/models/repository.rb | 26 ++++++++++++++++++++------ 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/app/models/concerns/repository_mirroring.rb b/app/models/concerns/repository_mirroring.rb index 7aca3f72ae7..fed336c29d6 100644 --- a/app/models/concerns/repository_mirroring.rb +++ b/app/models/concerns/repository_mirroring.rb @@ -1,21 +1,4 @@ module RepositoryMirroring - def storage_path - @project.repository_storage_path - end - - def add_remote(name, url) - raw_repository.remote_add(name, url) - rescue Rugged::ConfigError - raw_repository.remote_update(name, url: url) - end - - def remove_remote(name) - raw_repository.remote_delete(name) - true - rescue Rugged::ConfigError - false - end - def set_remote_as_mirror(name) config = raw_repository.rugged.config @@ -25,7 +8,10 @@ module RepositoryMirroring config["remote.#{name}.prune"] = true end - def fetch_remote(remote, forced: false, no_tags: false) - gitlab_shell.fetch_remote(storage_path, path_with_namespace, remote, forced: forced, no_tags: no_tags) + def fetch_mirror(remote, url) + add_remote(remote, url) + set_remote_as_mirror(remote) + fetch_remote(remote, forced: true) + remove_remote(remote) end end diff --git a/app/models/repository.rb b/app/models/repository.rb index a9c1ce6782d..adc2fd563ff 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -65,7 +65,7 @@ class Repository # Return absolute path to repository def path_to_repo @path_to_repo ||= File.expand_path( - File.join(@project.repository_storage_path, path_with_namespace + ".git") + File.join(repository_storage_path, path_with_namespace + ".git") ) end @@ -1034,11 +1034,21 @@ class Repository rugged.references.delete(tmp_ref) if tmp_ref end - def fetch_mirror(remote, url) - add_remote(remote, url) - set_remote_as_mirror(remote) - fetch_remote(remote, forced: true) - remove_remote(remote) + def add_remote(name, url) + raw_repository.remote_add(name, url) + rescue Rugged::ConfigError + raw_repository.remote_update(name, url: url) + end + + def remove_remote(name) + raw_repository.remote_delete(name) + true + rescue Rugged::ConfigError + false + end + + def fetch_remote(remote, forced: false, no_tags: false) + gitlab_shell.fetch_remote(repository_storage_path, path_with_namespace, remote, forced: forced, no_tags: no_tags) end def fetch_ref(source_path, source_ref, target_ref) @@ -1158,4 +1168,8 @@ class Repository def repository_event(event, tags = {}) Gitlab::Metrics.add_event(event, { path: path_with_namespace }.merge(tags)) end + + def repository_storage_path + @project.repository_storage_path + end end -- cgit v1.2.1 From 6143642a5ff2bbc63736b4b05d9583e72527fd5e Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 3 Apr 2017 15:48:09 -0300 Subject: Refactoring Projects::ImportService --- app/models/repository.rb | 4 -- app/services/projects/import_service.rb | 50 +++++++--------- spec/models/repository_spec.rb | 8 --- spec/services/projects/import_service_spec.rb | 83 +++++++++++++++++---------- 4 files changed, 75 insertions(+), 70 deletions(-) diff --git a/app/models/repository.rb b/app/models/repository.rb index adc2fd563ff..6b2383b73eb 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -402,10 +402,6 @@ class Repository expire_tags_cache end - def before_import - expire_content_cache - end - # Runs code after the HEAD of a repository is changed. def after_change_head expire_method_caches(METHOD_CACHES_FOR_FILE_TYPES.keys) diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index 794cc1556a4..45ad47c8f44 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -11,7 +11,7 @@ module Projects success rescue => e - error(e.message) + error("Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}") end private @@ -21,11 +21,7 @@ module Projects # In this case, we only want to import issues, not a repository. create_repository elsif !project.repository_exists? - if project.github_import? || project.gitea_import? - fetch_repository - else - import_repository - end + import_repository end end @@ -36,42 +32,40 @@ module Projects end def import_repository + raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url) + begin - raise Error, "Blocked import URL." if Gitlab::UrlBlocker.blocked_url?(project.import_url) - gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url) - rescue => e + if project.github_import? || project.gitea_import? + fetch_repository + else + clone_repository + end + rescue Gitlab::Shell::Error => e # Expire cache to prevent scenarios such as: # 1. First import failed, but the repo was imported successfully, so +exists?+ returns true # 2. Retried import, repo is broken or not imported but +exists?+ still returns true - project.repository.before_import if project.repository_exists? + project.repository.expire_content_cache if project.repository_exists? - raise Error, "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}" + raise Error, e.message end end - def fetch_repository - begin - raise Error, 'Blocked import URL.' if Gitlab::UrlBlocker.blocked_url?(project.import_url) - - project.create_repository - project.repository.add_remote(project.import_type, project.import_url) - project.repository.set_remote_as_mirror (project.import_type) - project.repository.fetch_remote(project.import_type, forced: true) - project.repository.remove_remote(project.import_type) - rescue => e - # Expire cache to prevent scenarios such as: - # 1. First import failed, but the repo was imported successfully, so +exists?+ returns true - # 2. Retried import, repo is broken or not imported but +exists?+ still returns true - project.repository.before_import if project.repository_exists? + def clone_repository + gitlab_shell.import_repository(project.repository_storage_path, project.path_with_namespace, project.import_url) + end - raise Error, "Error importing repository #{project.import_url} into #{project.path_with_namespace} - #{e.message}" - end + def fetch_repository + project.create_repository + project.repository.add_remote(project.import_type, project.import_url) + project.repository.set_remote_as_mirror (project.import_type) + project.repository.fetch_remote(project.import_type, forced: true) + project.repository.remove_remote(project.import_type) end def import_data return unless has_importer? - project.repository.before_import unless project.gitlab_project_import? + project.repository.expire_content_cache unless project.gitlab_project_import? unless importer.execute raise Error, 'The remote data could not be imported.' diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index df742ee8084..b16848f6f4f 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -1293,14 +1293,6 @@ describe Repository, models: true do end end - describe '#before_import' do - it 'flushes the repository caches' do - expect(repository).to receive(:expire_content_cache) - - repository.before_import - end - end - describe '#after_import' do it 'flushes and builds the cache' do expect(repository).to receive(:expire_content_cache) diff --git a/spec/services/projects/import_service_spec.rb b/spec/services/projects/import_service_spec.rb index e5917bb0b7a..09cfa36b3b9 100644 --- a/spec/services/projects/import_service_spec.rb +++ b/spec/services/projects/import_service_spec.rb @@ -26,30 +26,59 @@ describe Projects::ImportService, services: true do result = subject.execute expect(result[:status]).to eq :error - expect(result[:message]).to eq 'The repository could not be created.' + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - The repository could not be created." end end context 'with known url' do before do project.import_url = 'https://github.com/vim/vim.git' + project.import_type = 'github' end - it 'succeeds if repository import is successfully' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true) + context 'with a Github repository' do + it 'succeeds if repository import is successfully' do + expect_any_instance_of(Repository).to receive(:fetch_remote).and_return(true) + expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true) - result = subject.execute + result = subject.execute - expect(result[:status]).to eq :success + expect(result[:status]).to eq :success + end + + it 'fails if repository import fails' do + expect_any_instance_of(Repository).to receive(:fetch_remote).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + + result = subject.execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository" + end end - it 'fails if repository import fails' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + context 'with a non Github repository' do + before do + project.import_url = 'https://bitbucket.org/vim/vim.git' + project.import_type = 'bitbucket' + end - result = subject.execute + it 'succeeds if repository import is successfully' do + expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_return(true) + expect_any_instance_of(Gitlab::BitbucketImport::Importer).to receive(:execute).and_return(true) - expect(result[:status]).to eq :error - expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository" + result = subject.execute + + expect(result[:status]).to eq :success + end + + it 'fails if repository import fails' do + expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + + result = subject.execute + + expect(result[:status]).to eq :error + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Failed to import the repository" + end end end @@ -64,8 +93,8 @@ describe Projects::ImportService, services: true do end it 'succeeds if importer succeeds' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true) - expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true) + allow_any_instance_of(Repository).to receive(:fetch_remote).and_return(true) + allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(true) result = subject.execute @@ -73,48 +102,42 @@ describe Projects::ImportService, services: true do end it 'flushes various caches' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository). - with(project.repository_storage_path, project.path_with_namespace, project.import_url). + allow_any_instance_of(Repository).to receive(:fetch_remote). and_return(true) - expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute). + allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute). and_return(true) - expect_any_instance_of(Repository).to receive(:expire_emptiness_caches). - and_call_original - - expect_any_instance_of(Repository).to receive(:expire_exists_cache). - and_call_original + expect_any_instance_of(Repository).to receive(:expire_content_cache) subject.execute end it 'fails if importer fails' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true) - expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(false) + allow_any_instance_of(Repository).to receive(:fetch_remote).and_return(true) + allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_return(false) result = subject.execute expect(result[:status]).to eq :error - expect(result[:message]).to eq 'The remote data could not be imported.' + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - The remote data could not be imported." end it 'fails if importer raise an error' do - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_return(true) - expect_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_raise(Projects::ImportService::Error.new('Github: failed to connect API')) + allow_any_instance_of(Gitlab::Shell).to receive(:fetch_remote).and_return(true) + allow_any_instance_of(Gitlab::GithubImport::Importer).to receive(:execute).and_raise(Projects::ImportService::Error.new('Github: failed to connect API')) result = subject.execute expect(result[:status]).to eq :error - expect(result[:message]).to eq 'Github: failed to connect API' + expect(result[:message]).to eq "Error importing repository #{project.import_url} into #{project.path_with_namespace} - Github: failed to connect API" end - it 'expires existence cache after error' do + it 'expires content cache after error' do allow_any_instance_of(Project).to receive(:repository_exists?).and_return(false, true) - expect_any_instance_of(Gitlab::Shell).to receive(:import_repository).with(project.repository_storage_path, project.path_with_namespace, project.import_url).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) - expect_any_instance_of(Repository).to receive(:expire_emptiness_caches).and_call_original - expect_any_instance_of(Repository).to receive(:expire_exists_cache).and_call_original + expect_any_instance_of(Gitlab::Shell).to receive(:fetch_remote).and_raise(Gitlab::Shell::Error.new('Failed to import the repository')) + expect_any_instance_of(Repository).to receive(:expire_content_cache) subject.execute end -- cgit v1.2.1 From ff9eb3042ff75bbb69c2d4a23968dc2f5f988761 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 3 Apr 2017 16:36:14 -0300 Subject: Set the right timeout for Gitlab::Shell#fetch_remote --- lib/gitlab/shell.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/gitlab/shell.rb b/lib/gitlab/shell.rb index 94073d816e5..2744d627ceb 100644 --- a/lib/gitlab/shell.rb +++ b/lib/gitlab/shell.rb @@ -93,12 +93,13 @@ module Gitlab # name - project path with namespace # remote - remote name # forced - should we use --force flag? + # no_tags - should we use --no-tags flag? # # Ex. # fetch_remote("gitlab/gitlab-ci", "upstream") # def fetch_remote(storage, name, remote, forced: false, no_tags: false) - args = [gitlab_shell_projects_path, 'fetch-remote', storage, "#{name}.git", remote, '2100'] + args = [gitlab_shell_projects_path, 'fetch-remote', storage, "#{name}.git", remote, '800'] args << '--force' if forced args << '--no-tags' if no_tags -- cgit v1.2.1 From a9af86250c5226505ccce03f4e4aae08fe976be7 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 3 Apr 2017 16:43:27 -0300 Subject: Fix Rubocop offenses --- app/services/projects/import_service.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/services/projects/import_service.rb b/app/services/projects/import_service.rb index 45ad47c8f44..4c72d5e117d 100644 --- a/app/services/projects/import_service.rb +++ b/app/services/projects/import_service.rb @@ -57,7 +57,7 @@ module Projects def fetch_repository project.create_repository project.repository.add_remote(project.import_type, project.import_url) - project.repository.set_remote_as_mirror (project.import_type) + project.repository.set_remote_as_mirror(project.import_type) project.repository.fetch_remote(project.import_type, forced: true) project.repository.remove_remote(project.import_type) end -- cgit v1.2.1 From f61ded8214f47870ed9a644fe83bd9978aa3afa6 Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Mon, 27 Mar 2017 13:28:34 -0400 Subject: Enable the `bullet_logger` setting; enable `raise` in test environment --- Gemfile | 2 +- config/initializers/bullet.rb | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/Gemfile b/Gemfile index 0c6bb805b98..910ab318227 100644 --- a/Gemfile +++ b/Gemfile @@ -260,7 +260,6 @@ group :development do gem 'brakeman', '~> 3.6.0', require: false gem 'letter_opener_web', '~> 1.3.0' - gem 'bullet', '~> 5.5.0', require: false gem 'rblineprof', '~> 0.3.6', platform: :mri, require: false # Better errors handler @@ -272,6 +271,7 @@ group :development do end group :development, :test do + gem 'bullet', '~> 5.5.0', require: !!ENV['ENABLE_BULLET'] gem 'pry-byebug', '~> 3.4.1', platform: :mri gem 'pry-rails', '~> 0.3.4' diff --git a/config/initializers/bullet.rb b/config/initializers/bullet.rb index 95e82966c7a..0ade7109420 100644 --- a/config/initializers/bullet.rb +++ b/config/initializers/bullet.rb @@ -1,6 +1,11 @@ -if ENV['ENABLE_BULLET'] - require 'bullet' +if defined?(Bullet) && ENV['ENABLE_BULLET'] + Rails.application.configure do + config.after_initialize do + Bullet.enable = true - Bullet.enable = true - Bullet.console = true + Bullet.bullet_logger = true + Bullet.console = true + Bullet.raise = Rails.env.test? + end + end end -- cgit v1.2.1 From eccb9536db723a9216001006aa182b39cc02024d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 3 Apr 2017 17:08:20 +0200 Subject: Ensure users have a short username otherwise a click event is triggered outside the search field MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- .../issues/filtered_search/filter_issues_spec.rb | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/features/issues/filtered_search/filter_issues_spec.rb b/spec/features/issues/filtered_search/filter_issues_spec.rb index f463312bf57..004e335dd38 100644 --- a/spec/features/issues/filtered_search/filter_issues_spec.rb +++ b/spec/features/issues/filtered_search/filter_issues_spec.rb @@ -6,8 +6,8 @@ describe 'Filter issues', js: true, feature: true do let!(:group) { create(:group) } let!(:project) { create(:project, group: group) } - let!(:user) { create(:user) } - let!(:user2) { create(:user) } + let!(:user) { create(:user, username: 'joe') } + let!(:user2) { create(:user, username: 'jane') } let!(:label) { create(:label, project: project) } let!(:wontfix) { create(:label, project: project, title: "Won't fix") } @@ -70,7 +70,7 @@ describe 'Filter issues', js: true, feature: true do issue_with_caps_label.labels << caps_sensitive_label issue_with_everything = create(:issue, - title: "Bug report with everything you thought was possible", + title: "Bug report foo was possible", project: project, milestone: milestone, author: user, @@ -687,10 +687,10 @@ describe 'Filter issues', js: true, feature: true do end it 'filters issues by searched text, author, more text, assignee and even more text' do - input_filtered_search("bug author:@#{user.username} report assignee:@#{user.username} with") + input_filtered_search("bug author:@#{user.username} report assignee:@#{user.username} foo") expect_issues_list_count(1) - expect_filtered_search_input('bug report with') + expect_filtered_search_input('bug report foo') end it 'filters issues by searched text, author, assignee and label' do @@ -701,10 +701,10 @@ describe 'Filter issues', js: true, feature: true do end it 'filters issues by searched text, author, text, assignee, text, label and text' do - input_filtered_search("bug author:@#{user.username} report assignee:@#{user.username} with label:~#{bug_label.title} everything") + input_filtered_search("bug author:@#{user.username} assignee:@#{user.username} report label:~#{bug_label.title} foo") expect_issues_list_count(1) - expect_filtered_search_input('bug report with everything') + expect_filtered_search_input('bug report foo') end it 'filters issues by searched text, author, assignee, label and milestone' do @@ -715,10 +715,10 @@ describe 'Filter issues', js: true, feature: true do end it 'filters issues by searched text, author, text, assignee, text, label, text, milestone and text' do - input_filtered_search("bug author:@#{user.username} report assignee:@#{user.username} with label:~#{bug_label.title} everything milestone:%#{milestone.title} you") + input_filtered_search("bug author:@#{user.username} assignee:@#{user.username} report label:~#{bug_label.title} milestone:%#{milestone.title} foo") expect_issues_list_count(1) - expect_filtered_search_input('bug report with everything you') + expect_filtered_search_input('bug report foo') end it 'filters issues by searched text, author, assignee, multiple labels and milestone' do @@ -729,10 +729,10 @@ describe 'Filter issues', js: true, feature: true do end it 'filters issues by searched text, author, text, assignee, text, label1, text, label2, text, milestone and text' do - input_filtered_search("bug author:@#{user.username} report assignee:@#{user.username} with label:~#{bug_label.title} everything label:~#{caps_sensitive_label.title} you milestone:%#{milestone.title} thought") + input_filtered_search("bug author:@#{user.username} assignee:@#{user.username} report label:~#{bug_label.title} label:~#{caps_sensitive_label.title} milestone:%#{milestone.title} foo") expect_issues_list_count(1) - expect_filtered_search_input('bug report with everything you thought') + expect_filtered_search_input('bug report foo') end end -- cgit v1.2.1 From e97d1ae036c6a686c32c7b0e5f6919f3d0566e96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 4 Apr 2017 00:22:52 +0200 Subject: Fix a Knapsack issue that would load support/capybara.rb before support/env.rb MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Once https://github.com/ArturT/knapsack/issues/57 is solved and released in a new gem version, we can remove the KNAPSACK_TEST_DIR in .gitlab-ci.yml. Signed-off-by: Rémy Coutable --- features/support/capybara.rb | 3 --- features/support/env.rb | 4 ---- 2 files changed, 7 deletions(-) diff --git a/features/support/capybara.rb b/features/support/capybara.rb index 1e46b3faf0b..6da8aaac6cb 100644 --- a/features/support/capybara.rb +++ b/features/support/capybara.rb @@ -24,8 +24,5 @@ Capybara.ignore_hidden_elements = false Capybara::Screenshot.prune_strategy = :keep_last_run Spinach.hooks.before_run do - require 'spinach/capybara' - require 'capybara/rails' - TestEnv.eager_load_driver_server end diff --git a/features/support/env.rb b/features/support/env.rb index 26cdd9d746d..06c804b1db7 100644 --- a/features/support/env.rb +++ b/features/support/env.rb @@ -5,10 +5,6 @@ ENV['RAILS_ENV'] = 'test' require './config/environment' require 'rspec/expectations' -require_relative 'capybara' -require_relative 'db_cleaner' -require_relative 'rerun' - if ENV['CI'] require 'knapsack' Knapsack::Adapters::SpinachAdapter.bind -- cgit v1.2.1 From d7e3fadb42e00f680034a70735bcea21b9f74dca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 4 Apr 2017 08:48:49 +0200 Subject: Ensure we generate unique usernames otherwise validations fail MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- db/fixtures/development/18_abuse_reports.rb | 4 ++-- spec/requests/api/v3/members_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/db/fixtures/development/18_abuse_reports.rb b/db/fixtures/development/18_abuse_reports.rb index a3d33706d8e..88d2f784852 100644 --- a/db/fixtures/development/18_abuse_reports.rb +++ b/db/fixtures/development/18_abuse_reports.rb @@ -4,10 +4,10 @@ module Db class AbuseReport def self.seed Gitlab::Seeder.quiet do - (::AbuseReport.default_per_page + 3).times do + (::AbuseReport.default_per_page + 3).times do |i| reported_user = ::User.create!( - username: "#{FFaker::Internet.user_name}-reported", + username: "reported_user_#{i}", name: FFaker::Name.name, email: FFaker::Internet.email, confirmed_at: DateTime.now, diff --git a/spec/requests/api/v3/members_spec.rb b/spec/requests/api/v3/members_spec.rb index 13814ed10c3..af1c5cff67f 100644 --- a/spec/requests/api/v3/members_spec.rb +++ b/spec/requests/api/v3/members_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe API::V3::Members, api: true do include ApiHelpers - let(:master) { create(:user) } + let(:master) { create(:user, username: 'master_user') } let(:developer) { create(:user) } let(:access_requester) { create(:user) } let(:stranger) { create(:user) } -- cgit v1.2.1